diff mbox series

tracing: Add __string_src() helper to help compilers not to get confused

Message ID 20240314232754.345cea82@rorschach.local.home (mailing list archive)
State Accepted
Commit 7604256cecef34a82333d9f78262d3180f4eb525
Headers show
Series tracing: Add __string_src() helper to help compilers not to get confused | expand

Commit Message

Steven Rostedt March 15, 2024, 3:27 a.m. UTC 
From: "Steven Rostedt (Google)" <rostedt@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/

Reported-by: Alison Schofield <alison.schofield@intel.com>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 include/trace/stages/stage5_get_offsets.h | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

Comments

Alison Schofield March 15, 2024, 7:53 p.m. UTC | #1 
On Thu, Mar 14, 2024 at 11:27:54PM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <rostedt@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.schofield@intel.com>


> 
> Reported-by: Alison Schofield <alison.schofield@intel.com>
> Signed-off-by: Steven Rostedt (Google) <rostedt@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
>
diff mbox series

Patch

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