Re: [RFC PATCH v1 5/5] perf annotate-arch: Add ARM64 data type profiling support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Eager global variable resolution in ADRP causes incorrect type binding when the page boundary falls inside an adjacent variable.
- [High] Missing register type state invalidation for unhandled instructions leads to stale type propagation.
- [Low] Comment mismatch regarding bit 26 in A64_INSN_LDR_LIT_MASK.
--

commit 429208e90da6fddfa29cce218b8f6998420f5ba0
Author: Shuai Xue <xueshuai@linux.alibaba.com>

perf annotate-arch: Add ARM64 data type profiling support
    
This commit adds data type profiling support for ARM64, allowing
perf annotate --code-with-type to show data types accessed by memory
instructions. It implements raw instruction decoding to track register
states across ADRP, ADD, and MOV instructions.

> diff --git a/tools/perf/util/annotate-arch/annotate-arm64.c b/tools/perf/util/annotate-arch/annotate-arm64.c
> index b98aaf9a8a7be..887ed22c4ca05 100644
> --- a/tools/perf/util/annotate-arch/annotate-arm64.c
> +++ b/tools/perf/util/annotate-arch/annotate-arm64.c
[ ... ]
> @@ -14,6 +23,47 @@ struct arch_arm64 {
>  	regex_t jump_insn;
>  };
>  
> +/*
> + * ARM64 instruction encoding masks and values.
> + * Derived from ARM Architecture Reference Manual, C4.1 A64 encoding index.
> + *
> + * These mirror the definitions in arch/arm64/include/asm/insn.h but are
> + * duplicated here because that header depends on kernel-only macros
> + * (BUILD_BUG_ON, __always_inline).
> + */
> +
> +/* GP Load/Store: bit[27]=1, bit[26]=0 (GP, not SIMD/FP), bit[25]=0 */
> +#define A64_INSN_GP_LS_MASK	0x0e000000
> +#define A64_INSN_GP_LS_VAL	0x08000000
> +
> +/* LDR/LDRSW (literal): bits[29:27]=011, bit[26]=0 -- must be excluded from GP LS */
> +#define A64_INSN_LDR_LIT_MASK	0x3b000000

[Severity: Low]
This isn't a bug, but does the mask definition exclude bit 26, contradicting
the preceding comment that states bit[26]=0?

> +#define A64_INSN_LDR_LIT_VAL	0x18000000
> +
> +/*
> + * Load/Store register (register offset):
[ ... ]
> @@ -69,6 +119,285 @@ static const struct ins_ops arm64_mov_ops = {
[ ... ]
> +static void update_insn_state_arm64(struct type_state *state,
> +				    struct data_loc_info *dloc,
> +				    Dwarf_Die *cu_die,
> +				    struct disasm_line *dl)
> +{
> +	u32 insn = dl->raw.raw_insn;
> +	int rd, rn;
> +	struct type_state_reg *tsr;
> +
> +	if (arm64_is_adrp(insn)) {
> +		u64 pc, page_addr;
> +		int offset;
> +		Dwarf_Die type_die;
> +
> +		rd = A64_RT(insn);
> +		if (!has_reg_type(state, rd))
> +			return;
> +
> +		tsr = &state->regs[rd];
> +
> +		pc = map__rip_2objdump(dloc->ms->map,
> +				       dloc->ms->sym->start + dl->al.offset);
> +		page_addr = arm64_adrp_target(pc, insn);
> +
> +		/*
> +		 * Try to resolve the global variable at this page address.
> +		 * If not found, store it as a constant for later ADD resolution.
> +		 */
> +		if (get_global_var_type(cu_die, dloc,
> +					dloc->ms->sym->start + dl->al.offset,
> +					page_addr, &offset, &type_die)) {

[Severity: High]
Does eager global variable resolution in ADRP cause incorrect type binding when
the page boundary falls inside an adjacent variable?

If the 4KB-aligned page address happens to fall within the memory span of a
preceding global variable, get_global_var_type() might immediately resolve the
address to that wrong variable. The subsequent ADD instruction then adds its
immediate offset to the wrong variable's offset, never attempting to resolve
the true target variable.

Should type resolution be deferred until the full address is formed in the
subsequent ADD?

> +			tsr->type = type_die;
> +			tsr->kind = TSR_KIND_POINTER;
> +			tsr->offset = offset;
> +			tsr->ok = true;
> +
> +			pr_debug_dtp("adrp [%x] global addr=%#"PRIx64" -> reg%d",
> +				     (u32)dl->al.offset, page_addr, rd);
> +			pr_debug_type_name(&tsr->type, tsr->kind);
> +		} else {
[ ... ]
> +	if (arm64_is_mov_reg(insn)) {
> +		int rm;
> +
> +		rd = A64_RT(insn);
> +		rm = A64_RM(insn);
> +
> +		if (!has_reg_type(state, rd))
> +			return;
> +
> +		tsr = &state->regs[rd];
> +
> +		if (!has_reg_type(state, rm) || !state->regs[rm].ok) {
> +			tsr->ok = false;
> +			return;
> +		}
> +
> +		tsr->type = state->regs[rm].type;
> +		tsr->kind = state->regs[rm].kind;
> +		tsr->offset = state->regs[rm].offset;
> +		tsr->imm_value = state->regs[rm].imm_value;
> +		tsr->ok = true;
> +
> +		pr_debug_dtp("mov [%x] reg%d -> reg%d",
> +			     (u32)dl->al.offset, rm, rd);
> +		pr_debug_type_name(&tsr->type, tsr->kind);
> +		return;
> +	}
> +}

[Severity: High]
Are we missing register type state invalidation for unhandled instructions
here?

If an unhandled instruction clobbers a register, this function simply ignores
the instruction and implicitly returns. The old type state for that register
is never invalidated, causing it to retain a stale data type. Subsequent
memory accesses using the clobbered register could falsely resolve against the
stale type, polluting the output with false positives.

Should there be an invalidation fallback for unhandled instructions, similar
to how x86 explicitly calls invalidate_reg_state() for unhandled clobbering
instructions?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623130234.8709-1-xueshuai@linux.alibaba.com?part=5