Re: [PATCH] tracing: Add __string_src() helper to help compilers not to get confused

[Alison Schofield]

On Thu, Mar 14, 2024 at 11:27:54PM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <rostedt@xxxxxxxxxxx>
> 
> The __string() helper macro of the TRACE_EVENT() macro is used to
> determine how much of the ring buffer needs to be allocated to fit the
> given source string. Some trace events have a string that is dependent on
> another variable that could be NULL, and in those cases the string is
> passed in to be NULL.
> 
> The __string() macro can handle being passed in a NULL pointer for which
> it will turn it into "(null)". It does that with:
> 
>   strlen((src) ? (const char *)(src) : "(null)") + 1
> 
> But if src itself has the same conditional type it can confuse the
> compiler. That is:
> 
>   __string(r ? dev(r)->name : NULL)
> 
> Would turn into:
> 
>  strlen((r ? dev(r)->name : NULL) ? (r ? dev(r)->name : NULL) : "(null)" + 1
> 
> For which the compiler thinks that NULL is being passed to strlen() and
> gives this kind of warning:
> 
> ./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)
> 
> Instead, create a static inline function that takes the src string and
> will return the string if it is not NULL and will return "(null)" if it
> is. This will then make the strlen() line:
> 
>  strlen(__string_src(src)) + 1
> 
> Where the compiler can see that strlen() will not end up with NULL and
> does not warn about it.
> 
> Note that this depends on commit 51270d573a8d ("tracing/net_sched: Fix
> tracepoints that save qdisc_dev() as a string") being applied, as passing
> the qdisc_dev() into __string_src() will give an error.
> 
> Link: https://lore.kernel.org/all/ZfNmfCmgCs4Nc+EH@aschofie-mobl2/


Reviewed-by: Alison Schofield <alison.schofield@xxxxxxxxx>


> 
> Reported-by: Alison Schofield <alison.schofield@xxxxxxxxx>
> Signed-off-by: Steven Rostedt (Google) <rostedt@xxxxxxxxxxx>
> ---
>  include/trace/stages/stage5_get_offsets.h | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/include/trace/stages/stage5_get_offsets.h b/include/trace/stages/stage5_get_offsets.h
> index e6b96608f452..c6a62dfb18ef 100644
> --- a/include/trace/stages/stage5_get_offsets.h
> +++ b/include/trace/stages/stage5_get_offsets.h
> @@ -9,6 +9,16 @@
>  #undef __entry
>  #define __entry entry
>  
> +#ifndef __STAGE5_STRING_SRC_H
> +#define __STAGE5_STRING_SRC_H
> +static inline const char *__string_src(const char *str)
> +{
> +       if (!str)
> +	       return EVENT_NULL_STR;
> +       return str;
> +}
> +#endif /* __STAGE5_STRING_SRC_H */
> +
>  /*
>   * 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 +57,7 @@
>  
>  #undef __string
>  #define __string(item, src) __dynamic_array(char, item,			\
> -		    strlen((const char *)(src) ? : EVENT_NULL_STR) + 1)	\
> +		    strlen(__string_src(src)) + 1)			\
>  	__data_offsets->item##_ptr_ = src;
>  
>  #undef __string_len
> @@ -70,7 +80,7 @@
>  
>  #undef __rel_string
>  #define __rel_string(item, src) __rel_dynamic_array(char, item,		\
> -		    strlen((const char *)(src) ? : EVENT_NULL_STR) + 1)	\
> +		    strlen(__string_src(src)) + 1)			\
>  	__data_offsets->item##_ptr_ = src;
>  
>  #undef __rel_string_len
> -- 
> 2.43.0
>