Re: [PATCH v2] perf trace: Introduce --show-cpu option to display cpu id

[sashiko-bot@kernel.org]

Sashiko AI review found 2 potential issue(s):
- [High] Hardcoded CPU 0 misattributes interrupted syscalls.
- [Medium] Tautological check on unsigned integer and format string mismatch.
--

commit 1c192dba8ca61a6516bab340989a37ca856bfece
Author: Aaron Tomlin <atomlin@atomlin.com>

perf trace: Introduce --show-cpu option to display cpu id

This commit adds a new --show-cpu command-line option to perf trace. 
When enabled, it extracts the CPU ID from the perf sample and includes it 
in the trace output immediately following the timestamp.

> diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> index 311d9da9896a41..264e4762ea28d2 100644
> --- a/tools/perf/builtin-trace.c
> +++ b/tools/perf/builtin-trace.c

[ ... ]

> +static size_t trace__fprintf_cpu(u32 cpu, FILE *fp)
> +{
> +	size_t printed = 0;
> +
> +	if (cpu >= 0)

Since cpu is an unsigned 32-bit integer (u32), is this comparison always
true? This might trigger a compiler warning for tautological comparisons.

> +		printed += fprintf(fp, "[%03d] ", cpu);

Given that cpu is unsigned, should this use the %03u format specifier
instead of %03d to avoid a format signedness mismatch?

> +
> +	return printed;
> +}

[ ... ]

> @@ -2704,7 +2729,9 @@ static int trace__printf_interrupted_entry(struct trace *trace)
>  	if (!ttrace->entry_pending)
>  		return 0;
>  
> -	printed  = trace__fprintf_entry_head(trace, trace->current, 0, false, ttrace->entry_time, trace->output);
> +	printed = trace__fprintf_entry_head(trace, trace->current, 0, false,
> +					    ttrace->entry_time, 0,
> +					    trace->output);

Does passing a hardcoded 0 for the cpu argument cause all interrupted events
to be incorrectly attributed to CPU 0 when the --show-cpu option is enabled? 

If the actual entry CPU isn't saved in struct thread_trace, would it be
better to pass (u32)-1 here so the CPU printout is properly omitted,
matching the check inside trace__fprintf_entry_head()?

[ ... ]

> @@ -3277,6 +3308,9 @@ static int trace__event_handler(struct trace *trace, struct evsel *evsel,
>  	trace__printf_interrupted_entry(trace);
>  	trace__fprintf_tstamp(trace, sample->time, trace->output);
>  
> +	if (trace->show_cpu && sample->cpu != (u32)-1)
> +		fprintf(trace->output, "[%03d] ", sample->cpu);

Similar to the function above, should this use %03u since sample->cpu is
an unsigned integer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260423192445.131351-1-atomlin@atomlin.com?part=1