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 <[email protected]>
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 <[email protected]>
---
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
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 <[email protected]>
Closes: https://lore.kernel.org/all/CA+G9fYs=OTKAZS6g1P1Ewadfr0qoe6LgOVSohqkXmFXotEODdg@mail.gmail.com/
Signed-off-by: Nathan Chancellor <[email protected]>
---
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
On Tue, 19 Mar 2024 09:07:51 -0700
Nathan Chancellor <[email protected]> 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
On Tue, Mar 19, 2024 at 06:15:09PM -0400, Steven Rostedt wrote:
> On Tue, 19 Mar 2024 09:07:51 -0700
> Nathan Chancellor <[email protected]> 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
On Tue, Mar 19, 2024 at 9:08 AM Nathan Chancellor <[email protected]> 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 <[email protected]>
> Closes: https://lore.kernel.org/all/CA+G9fYs=OTKAZS6g1P1Ewadfr0qoe6LgOVSohqkXmFXotEODdg@mail.gmail.com/
> Signed-off-by: Nathan Chancellor <[email protected]>
> ---
> 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 <[email protected]> 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
> >
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 <[email protected]>
Reviewed-by: Justin Stitt <[email protected]>
> ---
> 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