Re: [PATCH v4 9/9] 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:
- [Medium] The newly added error check for `map_symbol__get_arch` returns early and leaves malformed output on stdout.
- [Medium] Inconsistent fallback logic for `e_machine` causes capstone initialization to fail when a DSO architecture cannot be determined.
--

--- Patch [9]: [PATCH v4 9/9] perf annotate: Be robust to annotating without a thread ---
commit 0c50b4f94a148dd6a11cef4fb5824f4934eab627
Author: Ian Rogers <irogers@google.com>
Subject: perf annotate: Be robust to annotating without a thread

If a thread isn't given to thread__get_arch, try harder to determine the arch
for disassembly by falling back to reading e_machine from a map's dso or
defaulting to the host. Ensure other uses of map_symbol thread don't assume
it is non-NULL.

> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index 02505222d8c2d..b6041f8608164 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
[ ... ]
> @@ -1266,7 +1291,10 @@ int hist_entry__annotate_printf(struct hist_entry *he, struct evsel *evsel)
>  
>  	apd.addr_fmt_width = annotated_source__addr_fmt_width(&notes->src->source,
>  							      notes->src->start);
> -	thread__get_arch(ms->thread, &apd.arch);
> +	if (map_symbol__get_arch(ms, &apd.arch)) {
> +		free(filename);
> +		return -ENOTSUP;
> +	}

[Severity: Medium]
Does returning early here leave the console output in a malformed state?

Looking at hist_entry__annotate_printf(), we've already printed the
"Source code & Disassembly..." header and a dotted line to stdout before
reaching this point. Aborting here without a newline might leave the
user's console output half-printed and corrupted.

>  	apd.dbg = dso__debuginfo(dso);
>  
>  	list_for_each_entry(pos, &notes->src->source, node) {
[ ... ]
> diff --git a/tools/perf/util/capstone.c b/tools/perf/util/capstone.c
> index 5ad537fea4360..15152a2c1b25c 100644
> --- a/tools/perf/util/capstone.c
> +++ b/tools/perf/util/capstone.c
> @@ -411,9 +411,18 @@ 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) {
> +		e_machine = dso__e_machine_endian(dso, /*machine=*/NULL, /*e_flags=*/NULL,
> +						  &is_big_endian);
> +	} else {
> +		e_machine = thread__e_machine_endian(NULL,
> +						     /*machine=*/NULL,
> +						     /*e_flags=*/NULL, &is_big_endian);
> +	}

[Severity: Medium]
If dso__e_machine_endian() returns EM_NONE, does this bypass the host
architecture fallback?

When annotating without a thread but with a valid DSO, we take this path.
If the DSO's ELF header is missing or unreadable and yields EM_NONE, the
if/else structure prevents us from executing the thread__e_machine_endian(NULL)
fallback. This could cause capstone_init() to fail and force a fallback to
the slower objdump.

>  	if (capstone_init(e_machine, &handle, is_64bit, is_big_endian, disassembler_style) < 0)
>  		goto err;
>  
> @@ -526,9 +535,18 @@ int symbol__disassemble_capstone_powerpc(const char *filename __maybe_unused,
>  	    !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) {
> +		e_machine = dso__e_machine_endian(dso, /*machine=*/NULL, /*e_flags=*/NULL,
> +						  &is_big_endian);
> +	} else {
> +		e_machine = thread__e_machine_endian(NULL,
> +						     /*machine=*/NULL,
> +						     /*e_flags=*/NULL, &is_big_endian);
> +	}

[Severity: Medium]
Does this have the same fallback logic issue as symbol__disassemble_capstone()?

If dso__e_machine_endian() returns EM_NONE, it appears we will skip the
fallback here as well.

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

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