| Message ID | 20240312113002.00031668@gandalf.local.home (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | b1afefa62ca9d77c8b1d386c3512fb4f21cb7db1 |
| Headers | show |
| Series | tracing: Use strcmp() in __assign_str() WARN_ON() check | expand |
On Tue, Mar 12, 2024 at 11:30:02AM -0400, Steven Rostedt wrote: > From: "Steven Rostedt (Google)" <rostedt@goodmis.org> > > The WARN_ON() check in __assign_str() to catch where the source variable > to the macro doesn't match the source variable to __string() gives an > error in clang: > > >> include/trace/events/sunrpc.h:703:4: warning: result of comparison against a string literal is unspecified (use an explicit string comparison function instead) [-Wstring-compare] > 670 | __assign_str(progname, "unknown"); > > That's because the __assign_str() macro has: > > WARN_ON_ONCE((src) != __data_offsets.dst##_ptr_); > > Where "src" is a string literal. Clang warns when comparing a string > literal directly as it is undefined to what the value of the literal is. > > Since this is still to make sure the same string that goes to __string() > is the same as __assign_str(), for string literals do a test for that and > then use strcmp() in those cases > > Note that this depends on commit 51270d573a8d ("tracing/net_sched: Fix > tracepoints that save qdisc_dev() as a string") being applied, as this was > what found that bug. > > Reported-by: kernel test robot <lkp@intel.com> > Closes: https://lore.kernel.org/oe-kbuild-all/202402292111.KIdExylU-lkp@intel.com/ > Fixes: 433e1d88a3be ("tracing: Add warning if string in __assign_str() does not match __string()") Is this change destined for 6.9 or 6.10? I applied it to current trace/core (eb1533d156d3) along with 51270d573a8d but the warning is still present. I even tried __builtin_choose_expr(__is_constexpr((src)), strcmp((src), __data_offsets.dst##_ptr_), (src) != __data_offsets.dst##_ptr_)); but not even that silenced the warning. I think we will still need a diag directive to fully silence this warning. > Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org> > --- > include/trace/stages/stage6_event_callback.h | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/include/trace/stages/stage6_event_callback.h b/include/trace/stages/stage6_event_callback.h > index a0c15f67eabe..83da83a0c14f 100644 > --- a/include/trace/stages/stage6_event_callback.h > +++ b/include/trace/stages/stage6_event_callback.h > @@ -35,7 +35,9 @@ > do { \ > char *__str__ = __get_str(dst); \ > int __len__ = __get_dynamic_array_len(dst) - 1; \ > - WARN_ON_ONCE((src) != __data_offsets.dst##_ptr_); \ > + WARN_ON_ONCE(__builtin_constant_p(src) ? \ > + strcmp((src), __data_offsets.dst##_ptr_) : \ > + (src) != __data_offsets.dst##_ptr_); \ > memcpy(__str__, __data_offsets.dst##_ptr_ ? : \ > EVENT_NULL_STR, __len__); \ > __str__[__len__] = '\0'; \ > -- > 2.43.0 >
On Wed, 13 Mar 2024 09:59:03 -0700 Nathan Chancellor <nathan@kernel.org> wrote: > > Reported-by: kernel test robot <lkp@intel.com> > > Closes: https://lore.kernel.org/oe-kbuild-all/202402292111.KIdExylU-lkp@intel.com/ > > Fixes: 433e1d88a3be ("tracing: Add warning if string in __assign_str() does not match __string()") > > Is this change destined for 6.9 or 6.10? I applied it to current > trace/core (eb1533d156d3) along with 51270d573a8d but the warning is > still present. I even tried > > __builtin_choose_expr(__is_constexpr((src)), > strcmp((src), __data_offsets.dst##_ptr_), > (src) != __data_offsets.dst##_ptr_)); > > but not even that silenced the warning. I think we will still need a > diag directive to fully silence this warning. Yes, you said that the warning is still there, but the bug it shows should not be. I believe that's because clang still evaluates the (src) != ... even when the source is a contast and it warns about it. But if src is a constant, we do not want to do the !=, we want to do the slower strcmp(). Let me test to make sure that when src is a string "like this" that it does the strcmp(). Otherwise, we may have to always do the strcmp(), which I really would like to avoid. BTW, I triggered another bug with strcmp(): https://lore.kernel.org/all/20240313093454.3909afe7@gandalf.local.home/ -- Steve
On Wed, 13 Mar 2024 13:45:50 -0400 Steven Rostedt <rostedt@goodmis.org> wrote: > Let me test to make sure that when src is a string "like this" that it does > the strcmp(). Otherwise, we may have to always do the strcmp(), which I > really would like to avoid. I added the below patch and enabled sched_switch and it triggered the warning (expected if it went the strcmp() path). I then changed it to be: #define __assign_str(dst, src) \ do { \ char *__str__ = __get_str(dst); \ int __len__ = __get_dynamic_array_len(dst) - 1; \ WARN_ON_ONCE(__builtin_constant_p(src) ? \ strcmp((src), __data_offsets.dst##_ptr_) : \ - (src) != __data_offsets.dst##_ptr_); \ + (src) == __data_offsets.dst##_ptr_); \ memcpy(__str__, __data_offsets.dst##_ptr_ ? : \ EVENT_NULL_STR, __len__); \ __str__[__len__] = '\0'; \ } while (0) And the sched_switch did not trigger (expected). So it seems that it should not be a problem. Note, I only tested this with gcc and not clang. But I guess there's also the case where we have: __assign_str(str, field ? field : "NULL") But hopefully that's not an issue. -- Steve diff --git a/include/trace/events/sched.h b/include/trace/events/sched.h index dbb01b4b7451..eaacd0c4e899 100644 --- a/include/trace/events/sched.h +++ b/include/trace/events/sched.h @@ -236,6 +236,7 @@ TRACE_EVENT(sched_switch, __array( char, next_comm, TASK_COMM_LEN ) __field( pid_t, next_pid ) __field( int, next_prio ) + __string( test, "this") ), TP_fast_assign( @@ -246,6 +247,7 @@ TRACE_EVENT(sched_switch, memcpy(__entry->prev_comm, prev->comm, TASK_COMM_LEN); __entry->next_pid = next->pid; __entry->next_prio = next->prio; + __assign_str(test, "this"); /* XXX SCHED_DEADLINE */ ), diff --git a/include/trace/stages/stage6_event_callback.h b/include/trace/stages/stage6_event_callback.h index 83da83a0c14f..cf301c723fd0 100644 --- a/include/trace/stages/stage6_event_callback.h +++ b/include/trace/stages/stage6_event_callback.h @@ -36,7 +36,7 @@ char *__str__ = __get_str(dst); \ int __len__ = __get_dynamic_array_len(dst) - 1; \ WARN_ON_ONCE(__builtin_constant_p(src) ? \ - strcmp((src), __data_offsets.dst##_ptr_) : \ + !strcmp((src), __data_offsets.dst##_ptr_) : \ (src) != __data_offsets.dst##_ptr_); \ memcpy(__str__, __data_offsets.dst##_ptr_ ? : \ EVENT_NULL_STR, __len__); \
diff --git a/include/trace/stages/stage6_event_callback.h b/include/trace/stages/stage6_event_callback.h index a0c15f67eabe..83da83a0c14f 100644 --- a/include/trace/stages/stage6_event_callback.h +++ b/include/trace/stages/stage6_event_callback.h @@ -35,7 +35,9 @@ do { \ char *__str__ = __get_str(dst); \ int __len__ = __get_dynamic_array_len(dst) - 1; \ - WARN_ON_ONCE((src) != __data_offsets.dst##_ptr_); \ + WARN_ON_ONCE(__builtin_constant_p(src) ? \ + strcmp((src), __data_offsets.dst##_ptr_) : \ + (src) != __data_offsets.dst##_ptr_); \ memcpy(__str__, __data_offsets.dst##_ptr_ ? : \ EVENT_NULL_STR, __len__); \ __str__[__len__] = '\0'; \