Re: [PATCH v2] cxl/trace: Properly initialize cxl_poison region name

[Steven Rostedt <rostedt@goodmis.org>]

On Thu, 14 Mar 2024 15:36:48 -0700
Alison Schofield <alison.schofield@intel.com> wrote:

> On Thu, Mar 14, 2024 at 05:17:37PM -0400, Steven Rostedt wrote:
> > On Thu, 14 Mar 2024 14:05:00 -0700
> > Alison Schofield <alison.schofield@intel.com> wrote:
> >   
> > > > I'm still curious to why NULL didn't work. I guess it may never have as I
> > > > noticed there's nothing else doing that. There are cases that a variable
> > > > returns NULL and the __string() handles it. But I guess the compiler gets
> > > > confused if the NULL is a possible return to the condition in __string().    
> > > 
> > > Here's the full warning spew:
> > > 
> > > In file included from ./include/trace/define_trace.h:102,
> > >                  from drivers/cxl/core/trace.h:713,
> > >                  from drivers/cxl/core/trace.c:8:
> > > drivers/cxl/core/./trace.h: In function ‘trace_event_get_offsets_cxl_poison’:
> > > ./include/trace/stages/stage5_get_offsets.h:50:21: warning: argument 1 null where non-null expected [-Wnonnull]
> > >    50 |                     strlen((src) ? (const char *)(src) : "(null)") + 1)
> > >       |                     ^~~~~~
> > > ./include/trace/trace_events.h:263:9: n  
> > 
> > The full warning didn't add any new information. The above was all I
> > needed. But I'm curious if you applied this patch if the warning will go
> > away. (Note I didn't test nor compile this).
> > 
> > -- Steve
> > 
> > diff --git a/include/trace/stages/stage5_get_offsets.h b/include/trace/stages/stage5_get_offsets.h
> > index e30a13be46ba..dfae18d8f4df 100644
> > --- a/include/trace/stages/stage5_get_offsets.h
> > +++ b/include/trace/stages/stage5_get_offsets.h
> > @@ -9,6 +9,13 @@
> >  #undef __entry
> >  #define __entry entry
> >    
> 
> +#ifndef STAGE5_H_INCLUDED
> +#define STAGE5_H_INCLUDED
> > +static inline const char *__string_src(const char *str)
> > +{
> > +	if (!str)
> > +		return "(null)";
> > +	return str;
> > +}  
> +#endif /* STAGE5_H_INCLUDED */
> 
> Happy to try it out...
> 
> Your diff, with the ifdef above, makes the compiler happy and it functions
> as wanted - no garbage in the fields.

I created the patch:

  https://lore.kernel.org/linux-trace-kernel/20240314232754.345cea82@rorschach.local.home/

That adds this. But now the kernel will not build because it requires the
fix of this patch. Otherwise it fails with:

In file included from /work/build/trace/nobackup/linux-test.git/include/trace/define_trace.h:102,
                 from /work/build/trace/nobackup/linux-test.git/drivers/cxl/core/trace.h:713,
                 from /work/build/trace/nobackup/linux-test.git/drivers/cxl/core/trace.c:8:
/work/build/trace/nobackup/linux-test.git/drivers/cxl/core/./trace.h: In function ‘trace_event_get_offsets_cxl_poison’:
/work/build/trace/nobackup/linux-test.git/drivers/cxl/core/./trace.h:660:34: error: passing argument 1 of ‘__string_src’ from incompatible pointer type [-Werror=incompatible-pointer-types]
  660 |                 __string(region, region)
      |                                  ^~~~~~
      |                                  |
      |                                  struct cxl_region *

I like that the patch also catches other bugs where non strings are passed
to __string(). But I can't send this to Linus if it breaks the build.

I'm breaking my pull request up as there's some other places that break the
build with my new checks. I'll do a pull of all my updates first, and after
Linus pulls in my change I will base the next pull off of that merge commit.

But this requires that this fix is in Linus's tree before that. If not, can
I take this fix and add it to that later pull request with all the tests. I
want to make sure that the kernel always builds.

-- Steve


> 
> > +
> >  /*
> >   * Fields should never declare an array: i.e. __field(int, arr[5])
> >   * If they do, it will cause issues in parsing and possibly corrupt the
> > @@ -47,7 +54,7 @@
> >  
> >  #undef __string
> >  #define __string(item, src) __dynamic_array(char, item,			\
> > -		    strlen((src) ? (const char *)(src) : "(null)") + 1)
> > +		    strlen(__string_src(src)) + 1)
> >  
> >  #undef __string_len
> >  #define __string_len(item, src, len) __dynamic_array(char, item, (len) + 1)