[PATCH 0/2] tracing: Fully silence instance of -Wstring-compare

[PATCH 0/2] tracing: Fully silence instance of -Wstring-compare

[Nathan Chancellor]

Hi all,

This series fully resolves the new instance of -Wstring-compare from
within the __assign_str() macro. The first patch resolves a build
failure with GCC that would be seen with just the second patch applied.
The second patch actually hides the warning.

NOTE: This is based on trace/for-next, which does not contain the
minimum LLVM version bump that occurred later in the current merge
window, so this uses

  __diag_ignore(clang, 11, ...

instead of

  __diag_ignore(clang, 13, ...

which will be required when this is merged into Linus's tree. If you can
base this series on a tree that has the merge commit e5eb28f6d1af ("Merge
tag 'mm-nonmm-stable-2024-03-14-09-36' of
git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm") in it, then that
change can be done at application time, rather than merge time (or I can
send a v2). It would be really nice for this to make the merge window so
that this warning does not proliferate into other trees that base on
-rc1.

---
Nathan Chancellor (2):
      compiler_types: Ensure __diag_clang() is always available
      tracing: Ignore -Wstring-compare with diagnostic macros

 include/linux/compiler_types.h               | 4 ++++
 include/trace/stages/stage6_event_callback.h | 5 +++++
 2 files changed, 9 insertions(+)
---
base-commit: 7604256cecef34a82333d9f78262d3180f4eb525
change-id: 20240319-tracing-fully-silence-wstring-compare-e71e2fd17b2a

Best regards,
-- 
Nathan Chancellor <nathan@kernel.org>

[PATCH 1/2] compiler_types: Ensure __diag_clang() is always available

[Nathan Chancellor]

Attempting to use __diag_clang() and build with GCC results in a build
error:

  include/linux/compiler_types.h:468:38: error: 'ignore' undeclared (first use in this function); did you mean 'inode'?
    468 |         __diag_ ## compiler(version, ignore, option)
        |                                      ^~~~~~

This error occurs because __diag_clang() is only defined in
compiler-clang.h, which is only included when using clang as the
compiler. This error has not been seen before because __diag_clang() has
only been used in __diag_ignore_all(), which is defined in both
compiler-clang.h and compiler-gcc.h.

Add an empty stub for __diag_clang() in compiler_types.h, so that it is
always defined and just becomes a no-op when using GCC.

Fixes: f014a00bbeb0 ("compiler-clang.h: Add __diag infrastructure for clang")
Signed-off-by: Nathan Chancellor <nathan@kernel.org>
---
 include/linux/compiler_types.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
index 3e64ec0f7ac8..fb0c3ff5497d 100644
--- a/include/linux/compiler_types.h
+++ b/include/linux/compiler_types.h
@@ -461,6 +461,10 @@ struct ftrace_likely_data {
 #define __diag_GCC(version, severity, string)
 #endif
 
+#ifndef __diag_clang
+#define __diag_clang(version, severity, string)
+#endif
+
 #define __diag_push()	__diag(push)
 #define __diag_pop()	__diag(pop)
 

-- 
2.44.0

[PATCH 2/2] tracing: Ignore -Wstring-compare with diagnostic macros

[Nathan Chancellor]

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_);	\
+		__diag_pop();						\
 		memcpy(__str__, __data_offsets.dst##_ptr_ ? :		\
 		       EVENT_NULL_STR, __len__);			\
 		__str__[__len__] = '\0';				\

-- 
2.44.0

Re: [PATCH 0/2] tracing: Fully silence instance of -Wstring-compare

[Steven Rostedt]

On Tue, 19 Mar 2024 09:07:51 -0700
Nathan Chancellor <nathan@kernel.org> wrote:

> Hi all,
> 
> This series fully resolves the new instance of -Wstring-compare from
> within the __assign_str() macro. The first patch resolves a build
> failure with GCC that would be seen with just the second patch applied.
> The second patch actually hides the warning.
> 
> NOTE: This is based on trace/for-next, which does not contain the
> minimum LLVM version bump that occurred later in the current merge
> window, so this uses
> 
>   __diag_ignore(clang, 11, ...
> 
> instead of
> 
>   __diag_ignore(clang, 13, ...
> 
> which will be required when this is merged into Linus's tree. If you can
> base this series on a tree that has the merge commit e5eb28f6d1af ("Merge
> tag 'mm-nonmm-stable-2024-03-14-09-36' of
> git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm") in it, then that
> change can be done at application time, rather than merge time (or I can
> send a v2). It would be really nice for this to make the merge window so
> that this warning does not proliferate into other trees that base on
> -rc1.
> 

I'm guessing this isn't needed with the last update.

-- Steve

Re: [PATCH 0/2] tracing: Fully silence instance of -Wstring-compare

[Nathan Chancellor]

On Tue, Mar 19, 2024 at 06:15:09PM -0400, Steven Rostedt wrote:
> On Tue, 19 Mar 2024 09:07:51 -0700
> Nathan Chancellor <nathan@kernel.org> wrote:
> 
> > Hi all,
> > 
> > This series fully resolves the new instance of -Wstring-compare from
> > within the __assign_str() macro. The first patch resolves a build
> > failure with GCC that would be seen with just the second patch applied.
> > The second patch actually hides the warning.
> > 
> > NOTE: This is based on trace/for-next, which does not contain the
> > minimum LLVM version bump that occurred later in the current merge
> > window, so this uses
> > 
> >   __diag_ignore(clang, 11, ...
> > 
> > instead of
> > 
> >   __diag_ignore(clang, 13, ...
> > 
> > which will be required when this is merged into Linus's tree. If you can
> > base this series on a tree that has the merge commit e5eb28f6d1af ("Merge
> > tag 'mm-nonmm-stable-2024-03-14-09-36' of
> > git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm") in it, then that
> > change can be done at application time, rather than merge time (or I can
> > send a v2). It would be really nice for this to make the merge window so
> > that this warning does not proliferate into other trees that base on
> > -rc1.
> > 
> 
> I'm guessing this isn't needed with the last update.

Correct, this is resolved in Linus's tree and should be in -next
tomorrow. The first patch may be worth sending standalone, I'll think
about sending it later but that can go in via some other tree, not
yours.

Cheers,
Nathan

Re: [PATCH 2/2] tracing: Ignore -Wstring-compare with diagnostic macros

[Justin Stitt]

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
>

Re: [PATCH 1/2] compiler_types: Ensure __diag_clang() is always available

[Justin Stitt]

Hi,

On Tue, Mar 19, 2024 at 09:07:52AM -0700, Nathan Chancellor wrote:
> Attempting to use __diag_clang() and build with GCC results in a build
> error:
> 
>   include/linux/compiler_types.h:468:38: error: 'ignore' undeclared (first use in this function); did you mean 'inode'?
>     468 |         __diag_ ## compiler(version, ignore, option)
>         |                                      ^~~~~~
> 
> This error occurs because __diag_clang() is only defined in
> compiler-clang.h, which is only included when using clang as the
> compiler. This error has not been seen before because __diag_clang() has
> only been used in __diag_ignore_all(), which is defined in both
> compiler-clang.h and compiler-gcc.h.
> 
> Add an empty stub for __diag_clang() in compiler_types.h, so that it is
> always defined and just becomes a no-op when using GCC.
> 
> Fixes: f014a00bbeb0 ("compiler-clang.h: Add __diag infrastructure for clang")
> Signed-off-by: Nathan Chancellor <nathan@kernel.org>

Reviewed-by: Justin Stitt <justinstitt@google.com>

> ---
>  include/linux/compiler_types.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
> index 3e64ec0f7ac8..fb0c3ff5497d 100644
> --- a/include/linux/compiler_types.h
> +++ b/include/linux/compiler_types.h
> @@ -461,6 +461,10 @@ struct ftrace_likely_data {
>  #define __diag_GCC(version, severity, string)
>  #endif
>  
> +#ifndef __diag_clang
> +#define __diag_clang(version, severity, string)
> +#endif
> +
>  #define __diag_push()	__diag(push)
>  #define __diag_pop()	__diag(pop)
>  
> 
> -- 
> 2.44.0

Thanks
Justin

Re: [PATCH 2/2] tracing: Ignore -Wstring-compare with diagnostic macros

[Steven Rostedt]

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
> >