Re: [PATCH v7 5/6] perf annotate: Be robust to annotating without a thread

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The patch actively introduces a silent fallback to the host architecture for userspace maps, defeating its stated purpose.
- [Medium] Returning a positive error code (ENOTSUP) from hist_entry__annotate_printf causes the caller to misinterpret the error as a valid count of filtered lines.
--

--- Patch [5]: [PATCH v7 5/6] perf annotate: Be robust to annotating without a thread ---
commit fcda5c254c589dfc6e3daa2b89d76183be5ba280
Author: Ian Rogers <irogers@google.com>

perf annotate: Be robust to annotating without a thread

This commit attempts to improve architecture detection for map symbols when
a thread isn't provided. It implements fallbacks using the DSO ELF header
and kmaps to avoid incorrectly falling back to the host architecture during
cross-platform analysis.

> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index 53b2a224b21df..df70e95a84704 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
[ ... ]
> @@ -982,24 +982,43 @@ void symbol__calc_percent(struct symbol *sym, struct evsel *evsel)
[ ... ]
> +	if (ms->thread) {
> +		machine = maps__machine(thread__maps(ms->thread));
> +		e_machine = thread__e_machine(ms->thread, machine, &e_flags);
> +		if (machine && machine->env)
> +			cpuid = machine->env->cpuid;
> +	} else if (dso) {
> +		struct maps *kmaps = (map && dso__kernel(dso)) ? map__kmaps(map) : NULL;
> +		struct machine *kmap_machine = kmaps ? maps__machine(kmaps) : NULL;
> +
> +		e_machine = dso__e_machine(dso, kmap_machine, &e_flags);
> +		if (kmap_machine && kmap_machine->env)
> +			cpuid = kmap_machine->env->cpuid;
>  	}
>  
> -	machine = maps__machine(thread__maps(thread));
> -	e_machine = thread__e_machine(thread, machine, &e_flags);
> -	arch = arch__find(e_machine, e_flags, machine->env ? machine->env->cpuid : NULL);
> +	if (e_machine == EM_NONE)
> +		e_machine = thread__e_machine(NULL, NULL, &e_flags);

[Severity: High]
Does this code silently fall back to the host architecture, defeating the stated
purpose of the patch? The commit message mentions "perfectly preventing silent,
incorrect host fallbacks to uname()", but when a user-space DSO is missing or
uses JIT, dso__e_machine() might return EM_NONE. Passing NULL to
thread__e_machine() triggers the uname() fallback, which assigns EM_HOST and
could cause Capstone to disassemble using the wrong architecture instead of
correctly aborting.

> +
> +	arch = arch__find(e_machine, e_flags, cpuid);
>  	if (arch == NULL) {
[ ... ]
> @@ -1251,6 +1270,11 @@ int hist_entry__annotate_printf(struct hist_entry *he, struct evsel *evsel)
>  		evsel_name = buf;
>  	}
>  
> +	if (map_symbol__get_arch(ms, &apd.arch)) {
> +		free(filename);
> +		return ENOTSUP;

[Severity: Medium]
Will returning a positive error code (ENOTSUP) here cause the caller to
misinterpret the error as a valid count of filtered lines? 
perf_top__show_details() interprets any positive return value from
hist_entry__annotate_printf() as the number of lines filtered out, incorrectly
printing '95 lines not displayed' rather than handling the failure. Should this
return a negative error code like -ENOTSUP?

> +	}
> +
>  	graph_dotted_len = printf(" %-*.*s|	Source code & Disassembly of %s for %s (%" PRIu64 " samples, "
>  				  "percent: %s)\n",
>  				  width, width, symbol_conf.show_total_period ? "Period" :
> diff --git a/tools/perf/util/capstone.c b/tools/perf/util/capstone.c
> index 00e0141cae8db..74213daf87862 100644
> --- a/tools/perf/util/capstone.c
> +++ b/tools/perf/util/capstone.c
[ ... ]
> @@ -423,9 +423,22 @@ int symbol__disassemble_capstone(const char *filename, struct symbol *sym,
>  	    !strcmp(args->options->disassembler_style, "att"))
>  		disassembler_style = true;
>  
> -	e_machine = thread__e_machine_endian(args->ms->thread,
> -					     /*machine=*/NULL,
> -					     /*e_flags=*/NULL, &is_big_endian);
> +	if (args->ms->thread) {
> +		e_machine = thread__e_machine_endian(args->ms->thread,
> +						     /*machine=*/NULL,
> +						     /*e_flags=*/NULL, &is_big_endian);
> +	} else if (dso) {
> +		struct maps *kmaps = (map && dso__kernel(dso)) ? map__kmaps(map) : NULL;
> +		struct machine *kmap_machine = kmaps ? maps__machine(kmaps) : NULL;
> +
> +		e_machine = dso__e_machine_endian(dso, kmap_machine, /*e_flags=*/NULL,
> +						  &is_big_endian);
> +	}
> +	if (!e_machine || e_machine == EM_NONE) {
> +		e_machine = thread__e_machine_endian(NULL,
> +						     /*machine=*/NULL,
> +						     /*e_flags=*/NULL, &is_big_endian);

[Severity: High]
Does this code also silently fall back to the host architecture here? Similar to
map_symbol__get_arch(), passing NULL to thread__e_machine_endian() results in
using the host architecture instead of aborting properly when annotating
cross-platform perf.data files.

> +	}
>  	if (capstone_init(e_machine, &handle, is_64bit, is_big_endian, disassembler_style) < 0)
>  		goto err;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260723045949.988740-1-irogers@google.com?part=5