Re: [PATCH 3/3] perf script: Provide weak stubs for instruction decoding

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The weak stub for `dump_insn` returns `NULL`, causing undefined behavior and a potential segmentation fault when passed to `fprintf`.
- [Low] Architectural layering violation by placing utility function stubs in a specific subcommand module.
--

commit c0a4d858da63f7e967ef9c465327ead66a0bd83b
Author: Li Guan <guanli.oerv@isrc.iscas.ac.cn>

perf script: Provide weak stubs for instruction decoding

This commit provides global weak stubs for dump_insn and
arch_is_uncond_branch to resolve undefined reference linker errors
when cross-compiling perf for non-x86 architectures like ARM64 and
RISC-V.

> diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
> index c8ac9f01a36bc..80a8f0e80b42d 100644
> --- a/tools/perf/builtin-script.c
> +++ b/tools/perf/builtin-script.c
> @@ -4608,3 +4608,25 @@ int cmd_script(int argc, const char **argv)
>  out:
>  	return err;
>  }
> +
> +/*
> + * Provide weak stubs for architectures that have not yet implemented
> + * instruction decoding. This prevents linker errors (undefined
> + * references) on non-x86 architectures (like ARM64, RISC-V) when
> + * building perf script.
> + */

Since dump_insn and arch_is_uncond_branch are general utility functions
declared in tools/perf/util/dump-insn.h, is it appropriate to place their
weak fallback implementations in a specific subcommand file?

This creates a structural inversion where a general utility header relies
on a subcommand's compilation to satisfy linker requirements. Would it be
better to place these stubs in a dedicated utility file like
tools/perf/util/dump-insn.c to preserve module boundaries and allow future
reuse across other perf tools?

> +const char * __weak dump_insn(struct perf_insn *x __maybe_unused,
> +			      u64 ip __maybe_unused,
> +			      u8 *inbuf __maybe_unused,
> +			      int inlen __maybe_unused,
> +			      int *lenp __maybe_unused)
> +{
> +	return NULL;

Does returning a null pointer here cause a segmentation fault?

Looking at any_dump_insn(), if Capstone disassembly is not used or fails,
the result is unconditionally formatted using fprintf:

fprintf(fp, "%s", dump_insn(...));

Passing a null pointer to the %s format specifier is undefined behavior.
While some standard C libraries like glibc defensively print "(null)", other
implementations commonly used on ARM or embedded systems (such as musl or
bionic) will directly dereference the null pointer, crashing perf script.

Should this return a valid static string literal like "<bad>" instead,
which matches the failure behavior of the x86 implementation?

> +}
> +
> +int __weak arch_is_uncond_branch(const unsigned char *buf __maybe_unused,
> +				 size_t len __maybe_unused,
> +				 int x86_64 __maybe_unused)
> +{
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260513154851.1085334-1-guanli.oerv@isrc.iscas.ac.cn?part=3