Re: [PATCH v8 09/17] perf arch common: Use perf_env e_machine rather than arch

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Logic inversion in `perf_env__single_address_space` breaks kernel/user address space resolution.
- [Medium] Exact `EM_HOST` check breaks native binutils fallback for 32-bit profiling data on 64-bit hosts.
--

commit e2d5820d159463d4dd20c88669d8e471fefbb4c5
Author: Ian Rogers <irogers@google.com>

perf arch common: Use perf_env e_machine rather than arch

This patch replaces architecture string matching with ELF machine
constants in the perf architecture common code. It updates the
binutils path lookup and single address space checks to use
the e_machine values from the perf environment.

> diff --git a/tools/perf/arch/common.c b/tools/perf/arch/common.c
> index 21836f70f231..e9b5b61feffe 100644
> --- a/tools/perf/arch/common.c
> +++ b/tools/perf/arch/common.c

[ ... ]

> @@ -145,7 +147,8 @@ static int perf_env__lookup_binutils_path(struct perf_env *env,
>  					  const char *name, char **path)
>  {

[ ... ]

>  	/*
>  	 * We don't need to try to find objdump path for native system.
>  	 * Just use default binutils path (e.g.: "objdump").
>  	 */
> -	if (!strcmp(perf_env__arch(NULL), arch))
> +	if (e_machine == EM_HOST)
>  		goto out;

The previous check used perf_env__arch() which normalized both 32-bit and
64-bit variants to a single string like "x86". With the exact EM_HOST
comparison, if we are analyzing a 32-bit trace (EM_386) on a 64-bit host
(EM_X86_64), will this evaluate to false and skip the native fallback?

Could this regression cause the tool to wrongly search for a cross-compiler
prefix instead of successfully using the native multi-arch objdump?

[ ... ]

> @@ -237,5 +252,5 @@ int perf_env__lookup_objdump(struct perf_env *env, char **path)
>   */
>  bool perf_env__single_address_space(struct perf_env *env)
>  {
> -	return strcmp(perf_env__arch(env), "sparc");
> +	return perf_env__e_machine(env, /*e_flags=*/NULL) == EM_SPARC;
>  }

Does this introduce a logic inversion regression?

The original implementation using strcmp() evaluated to non-zero (true)
for non-SPARC architectures and zero (false) for SPARC. The new
implementation returns true only for SPARC and false for everything else.

Also, should this check include EM_SPARCV9 to properly handle 64-bit SPARC
traces, since the previous strcmp() against "sparc" would have matched the
normalized string for both?

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