Re: [PATCH v11 14/19] perf env: Refactor perf_env__arch_strerrno

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The commit message claims to hide the declaration if libtraceevent is not present, but the code removes the conditional compilation, exposing it unconditionally.
- [Low] Generated C code uses newer ELF machine constants (e.g., EM_LOONGARCH, EM_CSKY) without fallback definitions, breaking the build on systems with older glibc.
--

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

perf env: Refactor perf_env__arch_strerrno

This commit refactors perf_env__arch_strerrno to take an e_machine and an
errno value instead of an architecture string. It updates arch_errno_names.sh
to generate an e_machine to function mapping and alters the call sites
in builtin-trace.c to use the newly available e_machine.

> perf_env__arch_strerrno is only available with libtraceevent so hide
> the declaration if no libtraceevent.

[Severity: Medium]
The commit message mentions hiding the declaration when libtraceevent is
missing, but the patch actually removes the HAVE_LIBTRACEEVENT guards. Could
the commit message be more precise about the intent?

> diff --git a/tools/perf/trace/beauty/arch_errno_names.sh b/tools/perf/trace/beauty/arch_errno_names.sh
> index d48d8561a7bb4..aae3d92a7893a 100755
> --- a/tools/perf/trace/beauty/arch_errno_names.sh
> +++ b/tools/perf/trace/beauty/arch_errno_names.sh

[ ... ]

> @@ -52,21 +52,50 @@ process_arch()
>  		|IFS=, create_errno_lookup_func "$arch"
>  }
>  
> +arch_to_e_machine()
> +{
> +	case "$1" in
> +	alpha)      printf '\tcase EM_ALPHA:\n' ;;
> +	arc)        printf '\tcase EM_ARC:\n' ;;
> +	arm)        printf '\tcase EM_ARM:\n' ;;
> +	arm64)      printf '\tcase EM_AARCH64:\n' ;;
> +	csky)       printf '\tcase EM_CSKY:\n' ;;
> +	hexagon)    printf '\tcase EM_HEXAGON:\n' ;;
> +	loongarch)  printf '\tcase EM_LOONGARCH:\n' ;;

[ ... ]

> @@ -74,6 +103,8 @@ cat <<EoHEADER
>  /* SPDX-License-Identifier: GPL-2.0 */
>  
>  #include <string.h>
> +#include <stdint.h>
> +#include <elf.h>
>  
>  EoHEADER

[Severity: Low]
Can this cause a build failure on systems with older glibc versions?

The script unconditionally outputs cases for newer ELF machine constants
like EM_CSKY and EM_LOONGARCH into the generated C code, relying solely
on the system elf.h.

If a host has an older glibc where these constants are not yet defined,
it would result in undeclared identifier errors. Is a fallback definition
or internal header needed here?

> diff --git a/tools/perf/util/env.c b/tools/perf/util/env.c
> index fae70b07ba8de..b41562fb06c63 100644
> --- a/tools/perf/util/env.c
> +++ b/tools/perf/util/env.c

[ ... ]

> @@ -851,16 +851,11 @@ const char *perf_env__arch(struct perf_env *env)
>  	return arch;
>  }
>  
> -const char *perf_env__arch_strerrno(struct perf_env *env __maybe_unused, int err __maybe_unused)
> -{
> -#if defined(HAVE_LIBTRACEEVENT)
> -	if (env->arch_strerrno == NULL)
> -		env->arch_strerrno = arch_syscalls__strerrno_function(perf_env__arch(env));
> +const char *arch_syscalls__strerrno(uint16_t e_machine, int err);
>  
> -	return env->arch_strerrno ? env->arch_strerrno(err) : "no arch specific strerrno function";
> -#else
> -	return "!HAVE_LIBTRACEEVENT";
> -#endif
> +const char *perf_env__arch_strerrno(uint16_t e_machine, int err)
> +{
> +	return arch_syscalls__strerrno(e_machine, err);
>  }

[Severity: Medium]
Does this unintentionally expose perf_env__arch_strerrno unconditionally?

The commit message states the intent is to hide the declaration if there
is no libtraceevent, but this code removes the HAVE_LIBTRACEEVENT guards
entirely.

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