Re: [PATCH v3 02/21] perf capstone: Fix arm64 jump/adrp disassembly mismatch with objdump

[Namhyung Kim <namhyung@kernel.org>]

On Wed, Jul 01, 2026 at 02:44:13PM +0800, Tengda Wu wrote:
> 
> 
> On 2026/7/1 12:07, sashiko-bot@kernel.org wrote:
> > Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> > - [High] Symbol resolution fails for relocated kernels (KASLR) and user-space shared libraries due to mismatched address spaces.
> > - [Low] Build failure due to undeclared `machine` variable and undefined `machine__normalized_is()` function in `capstone_init()`.
> > - [Low] Missing `<stdlib.h>` inclusion causes musl libc build failure.
> > - [Low] Usage of `ARM64_GRP_BRANCH_RELATIVE` causes build failures with Capstone versions prior to 5.0.
> > --
> > 
> > commit 1f9744f04e456b1209677658edb7f30a5b26e56d
> > Author: Tengda Wu <wutengda@huaweicloud.com>
> > 
> > perf capstone: Fix arm64 jump/adrp disassembly mismatch with objdump
> > 
> > The patch implements extended formatting for arm64 jump and adrp instructions
> > during symbol__disassemble_capstone() to match objdump's expected output style.
> > It adds instruction detail resolution in capstone_init() and formats the
> > output with correct symbol and offset mappings.
> > 
> >> diff --git a/tools/perf/util/capstone.c b/tools/perf/util/capstone.c
> >> index 9bba78ee0c5a2..a9c0a1d2eb036 100644
> >> --- a/tools/perf/util/capstone.c
> >> +++ b/tools/perf/util/capstone.c
> > 
> > [ ... ]
> > 
> >> @@ -218,6 +218,12 @@ static int capstone_init(uint16_t e_machine, csh *cs_handle, bool is64, bool is_
> >>  		 * on x86 by investigating instruction details.
> >>  		 */
> >>  		perf_cs_option(*cs_handle, CS_OPT_DETAIL, CS_OPT_ON);
> >> +	} else if (machine__normalized_is(machine, "arm64")) {
> > 
> > [Severity: Low]
> > Does this code cause a build failure? The variable machine is not declared
> > in capstone_init(), and the function machine__normalized_is() does not appear
> > to be defined in the codebase.
> > 
> 
> Agreed. This is due to commit 12c4737f55f2 ("perf capstone: Determine architecture from
> e_machine"), which changed the capstone interface. I haven't rebased this part yet.
> The base-commit already fails to build with capstone, so I didn't notice this one.

Yep, it should be 'else if (arch == CS_ARCH_ARM64)' instead.

> 
> >> +		/*
> >> +		 * Same as x86: arm64 needs instruction details to resolve
> >> +		 * symbolic addresses
> >> +		 */
> >> +		perf_cs_option(*cs_handle, CS_OPT_DETAIL, CS_OPT_ON);
> >>  	}
> >>  
> >>  	return 0;
> > 
> > [ ... ]
> > 
> >> +static bool is_pc_relative_insn(struct cs_insn *insn)
> >> +{
> >> +	int i;
> >> +
> >> +	if (insn->id == ARM64_INS_ADR || insn->id == ARM64_INS_ADRP)
> >> +		return true;
> >> +
> >> +	if (insn->detail == NULL)
> >> +		return false;
> >> +
> >> +	for (i = 0; i < insn->detail->groups_count; i++) {
> >> +		if (insn->detail->groups[i] == ARM64_GRP_JUMP ||
> >> +		    insn->detail->groups[i] == ARM64_GRP_CALL ||
> >> +		    insn->detail->groups[i] == ARM64_GRP_BRANCH_RELATIVE)
> > 
> > [Severity: Low]
> > Will this break compilation against Capstone versions prior to 5.0?
> > ARM64_GRP_BRANCH_RELATIVE was introduced in Capstone 5.0. Since the perf
> > tool's build feature detection doesn't mandate a specific Capstone version,
> > relying on this enum might cause build failures on older setups.

I don't think it's added in 5.0.  I can see it in the 4.0 version too.

https://github.com/capstone-engine/capstone/blob/4.0/include/capstone/arm64.h#L1149

> > 
> 
> This enum was added in Capstone 5.0 over two years ago. I think we can
> safely assume most systems are on a newer version now. I'll leave it as-is.

Two years ago is not old enough to ignore them.  In fact, I have a
commit to fix the capstone build issue by adding the missing constants.

https://lore.kernel.org/linux-perf-users/20260706234836.815254-1-namhyung@kernel.org/

> 
> >> +			return true;
> >> +	}
> >> +
> >> +	return false;
> >> +}
> >> +
> >> +static void format_capstone_insn_arm64(struct cs_insn *insn, char *buf,
> >> +				       size_t len, struct annotate_args *args,
> >> +				       u64 addr)
> >> +{
> > 
> > [ ... ]
> > 
> >> +	if (is_pc_relative_insn(insn)) {
> >> +		/* Extract last immediate value as address */
> >> +		last_imm = strrchr(buf, '#');
> >> +		if (!last_imm)
> >> +			return;
> >> +
> >> +		orig_addr = strtoull(last_imm + 1, &endptr, 16);
> > 
> > [Severity: Low]
> > Can this cause a build failure on musl libc? The POSIX standard function
> > strtoull() is used here, but stdlib.h is not explicitly included in the
> > file. Relying on implicit inclusions often works on glibc but breaks on
> > musl libc.
> > 
> 
> I'd say we can ignore this. strtoull() is used all over the perf codebase
> and gets included indirectly. I don't think this is a real issue.

Yep, most of these comments are false alarms.

Thanks,
Namhyung

> 
> >> +		if (endptr == last_imm + 1)
> >> +			return;
> >> +
> >> +		/* Relocate map that contains the address */
> >> +		if (dso__kernel(map__dso(map))) {
> >> +			found_map = maps__find(map__kmaps(map), orig_addr);
> >> +			if (found_map == NULL)
> >> +				return;
> >> +			map = found_map;
> >> +		}
> >> +
> >> +		/* Convert it to map-relative address for search */
> >> +		addr = map__map_ip(map, orig_addr);
> > 
> > [Severity: High]
> > Does this code fail to resolve symbols for relocated kernels (KASLR) or
> > user-space shared libraries (PIE)?
> > 
> > The extracted orig_addr is in the objdump address space. However,
> > maps__find() and map__map_ip() expect absolute memory addresses. By omitting
> > the translation step via map__objdump_2mem(map, orig_addr) (which the x86
> > implementation handles correctly), KASLR relocated kernels will fail in
> > maps__find(), and user-space shared libraries will underflow in
> > map__map_ip().
> > 
> 
> Agreed. I missed map__objdump_2mem() here.
> 
> >> +
> >> +		sym = map__find_symbol(map, addr);
> >> +		if (sym == NULL) {
> >> +			map__put(found_map);
> >> +			return;
> >> +		}
> > 
>