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

[Dan Williams <dan.j.williams@intel.com>]

Steven Rostedt wrote:
> 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.

Acked-by: Dan Williams <dan.j.williams@intel.com>

...for you taking the fix through your tree, and yes, please Cc stable
on it as well.