On Thu, Mar 14, 2024 at 11:27:54PM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <rost...@goodmis.org>
>
> 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.schofi...@intel.com>
>
> Reported-by: Alison Schofield <alison.schofi...@intel.com>
> Signed-off-by: Steven Rostedt (Google) <rost...@goodmis.org>
> ---
> 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
>
