From: "Steven Rostedt (Google)" <[email protected]>
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 <[email protected]>
Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
Fixes: 433e1d88a3be ("tracing: Add warning if string in __assign_str() does not match __string()")
Signed-off-by: Steven Rostedt (Google) <[email protected]>
---
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 Tue, Mar 12, 2024 at 11:30:02AM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <[email protected]>
>
> 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 <[email protected]>
> Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
> 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) <[email protected]>
> ---
> 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 <[email protected]> wrote:
> > Reported-by: kernel test robot <[email protected]>
> > Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
> > 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/[email protected]/
-- Steve
On Wed, 13 Mar 2024 13:45:50 -0400
Steven Rostedt <[email protected]> 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__); \