| Message ID | 20240319-tracing-fully-silence-wstring-compare-v1-2-81adb44403f5@kernel.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | tracing: Fully silence instance of -Wstring-compare | expand |
On Tue, Mar 19, 2024 at 9:08 AM Nathan Chancellor <nathan@kernel.org> wrote: > > Commit b1afefa62ca9 ("tracing: Use strcmp() in __assign_str() WARN_ON() > check") addressed a clang warning, -Wstring-compare, with the use of > __builtin_constant_p() to dispatch to strcmp() if the source string is a > string literal and a direct comparison if not. Unfortunately, even with > this change, the warning is still present because __builtin_constant_p() > is not evaluated at this stage of the pipeline, so clang still thinks > the else branch could occur for this situation: > > include/trace/events/sunrpc.h:705:4: error: result of comparison against a string literal is unspecified (use an explicit string comparison function instead) [-Werror,-Wstring-compare] > ... > include/trace/stages/stage6_event_callback.h:40:15: note: expanded from macro '__assign_str' > 40 | (src) != __data_offsets.dst##_ptr_); \ > | ^ > ... > > Use the compiler diagnostic macros to disable this warning around the > WARN_ON_ONCE() expression since a string comparison function, strcmp(), > will always be used for the comparison of string literals. > > Fixes: b1afefa62ca9 ("tracing: Use strcmp() in __assign_str() WARN_ON() check") > Reported-by: Linux Kernel Functional Testing <lkft@linaro.org> > Closes: https://lore.kernel.org/all/CA+G9fYs=OTKAZS6g1P1Ewadfr0qoe6LgOVSohqkXmFXotEODdg@mail.gmail.com/ > Signed-off-by: Nathan Chancellor <nathan@kernel.org> > --- > include/trace/stages/stage6_event_callback.h | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/include/trace/stages/stage6_event_callback.h b/include/trace/stages/stage6_event_callback.h > index 83da83a0c14f..56a4eea5a48e 100644 > --- a/include/trace/stages/stage6_event_callback.h > +++ b/include/trace/stages/stage6_event_callback.h > @@ -35,9 +35,14 @@ > do { \ > char *__str__ = __get_str(dst); \ > int __len__ = __get_dynamic_array_len(dst) - 1; \ > + __diag_push(); \ > + __diag_ignore(clang, 11, "-Wstring-compare", \ > + "__builtin_constant_p() ensures strcmp()" \ > + "will be used for string literals"); \ > WARN_ON_ONCE(__builtin_constant_p(src) ? \ > strcmp((src), __data_offsets.dst##_ptr_) : \ > (src) != __data_offsets.dst##_ptr_); \ What exactly is the point of the literal string comparison? Why doesn't strcmp do the trick? > + __diag_pop(); \ > memcpy(__str__, __data_offsets.dst##_ptr_ ? : \ > EVENT_NULL_STR, __len__); \ > __str__[__len__] = '\0'; \ > > -- > 2.44.0 >
On Tue, 19 Mar 2024 17:30:41 -0700 Justin Stitt <justinstitt@google.com> wrote: > > diff --git a/include/trace/stages/stage6_event_callback.h b/include/trace/stages/stage6_event_callback.h > > index 83da83a0c14f..56a4eea5a48e 100644 > > --- a/include/trace/stages/stage6_event_callback.h > > +++ b/include/trace/stages/stage6_event_callback.h > > @@ -35,9 +35,14 @@ > > do { \ > > char *__str__ = __get_str(dst); \ > > int __len__ = __get_dynamic_array_len(dst) - 1; \ > > + __diag_push(); \ > > + __diag_ignore(clang, 11, "-Wstring-compare", \ > > + "__builtin_constant_p() ensures strcmp()" \ > > + "will be used for string literals"); \ > > WARN_ON_ONCE(__builtin_constant_p(src) ? \ > > strcmp((src), __data_offsets.dst##_ptr_) : \ > > (src) != __data_offsets.dst##_ptr_); \ > > What exactly is the point of the literal string comparison? Why > doesn't strcmp do the trick? This is done in the hotpath, and is only for debugging. The string passed into __string() should be the same as the string passed into __assign_str(). But this is moot as I ended up always using strcmp() even if it will slow down the recording of the event. Next merge window the src parameter (along with the strcmp() checks) are going away. -- Steve > > > + __diag_pop(); \ > > memcpy(__str__, __data_offsets.dst##_ptr_ ? : \ > > EVENT_NULL_STR, __len__); \ > > __str__[__len__] = '\0'; \ > > > > -- > > 2.44.0 > >
diff --git a/include/trace/stages/stage6_event_callback.h b/include/trace/stages/stage6_event_callback.h index 83da83a0c14f..56a4eea5a48e 100644 --- a/include/trace/stages/stage6_event_callback.h +++ b/include/trace/stages/stage6_event_callback.h @@ -35,9 +35,14 @@ do { \ char *__str__ = __get_str(dst); \ int __len__ = __get_dynamic_array_len(dst) - 1; \ + __diag_push(); \ + __diag_ignore(clang, 11, "-Wstring-compare", \ + "__builtin_constant_p() ensures strcmp()" \ + "will be used for string literals"); \ WARN_ON_ONCE(__builtin_constant_p(src) ? \ strcmp((src), __data_offsets.dst##_ptr_) : \ (src) != __data_offsets.dst##_ptr_); \ + __diag_pop(); \ memcpy(__str__, __data_offsets.dst##_ptr_ ? : \ EVENT_NULL_STR, __len__); \ __str__[__len__] = '\0'; \
Commit b1afefa62ca9 ("tracing: Use strcmp() in __assign_str() WARN_ON() check") addressed a clang warning, -Wstring-compare, with the use of __builtin_constant_p() to dispatch to strcmp() if the source string is a string literal and a direct comparison if not. Unfortunately, even with this change, the warning is still present because __builtin_constant_p() is not evaluated at this stage of the pipeline, so clang still thinks the else branch could occur for this situation: include/trace/events/sunrpc.h:705:4: error: result of comparison against a string literal is unspecified (use an explicit string comparison function instead) [-Werror,-Wstring-compare] ... include/trace/stages/stage6_event_callback.h:40:15: note: expanded from macro '__assign_str' 40 | (src) != __data_offsets.dst##_ptr_); \ | ^ ... Use the compiler diagnostic macros to disable this warning around the WARN_ON_ONCE() expression since a string comparison function, strcmp(), will always be used for the comparison of string literals. Fixes: b1afefa62ca9 ("tracing: Use strcmp() in __assign_str() WARN_ON() check") Reported-by: Linux Kernel Functional Testing <lkft@linaro.org> Closes: https://lore.kernel.org/all/CA+G9fYs=OTKAZS6g1P1Ewadfr0qoe6LgOVSohqkXmFXotEODdg@mail.gmail.com/ Signed-off-by: Nathan Chancellor <nathan@kernel.org> --- include/trace/stages/stage6_event_callback.h | 5 +++++ 1 file changed, 5 insertions(+)