Re: [PATCH v5 25/26] perf annotate-arm64: Support per-cpu variable access tracking

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The newly added logic to track per-cpu base registers in 'add' and 'ldr' instructions is entirely unreachable dead code because `arch_get_reg_offset` prematurely aborts the tracking state.
--

commit e3331fba4432f08f3db300129b5060bba477930c
Author: Tengda Wu <wutengda@huaweicloud.com>

perf annotate-arm64: Support per-cpu variable access tracking

This commit extends update_insn_state_arm64() to handle per-cpu variable
addressing. It detects 'adrp + ldr' pairs that resolve to the
'__per_cpu_offset' symbol, and attempts to propagate this type information
through subsequent 'ldr' and 'add' instructions. However, the newly added
logic is entirely unreachable dead code because arch_get_reg_offset()
prematurely aborts the tracking state.

> diff --git a/tools/perf/util/annotate-arch/annotate-arm64.c b/tools/perf/util/annotate-arch/annotate-arm64.c
> index 9daf959055457..33fef3d153786 100644
> --- a/tools/perf/util/annotate-arch/annotate-arm64.c
> +++ b/tools/perf/util/annotate-arch/annotate-arm64.c

[ ... ]

> @@ -649,6 +660,12 @@ static int propagate_load_reg_state(struct type_state *state,
>  		return 0;
>  	}
>  
> +	/*
> +	 * If src_tsr->kind is TSR_KIND_PERCPU_BASE, treat it as an array.
> +	 * No special handling is needed here; just fall through to retry
> +	 * and resolve the type from the second register.
> +	 */
> +

[Severity: Medium]
Is this fall-through logic in propagate_load_reg_state() for intermediate
Pattern A ('ldr') instructions actually reachable?

Earlier in propagate_load_reg_state(), arch_get_reg_offset() forces an early
exit by returning -1 when evaluating a multi-register load where the second
register isn't a constant. This happens before this new per-cpu fallback
logic can be reached, making this dead code.

>  	/* Or try another register if any */
>  	if (src->multi_regs && src->reg1 != src->reg2 && sreg != src->reg2 &&
>  	    !(src->extend_type || src->shift_type)) {

[ ... ]

> @@ -949,6 +966,34 @@ static void update_add_insn_state(struct type_state *state,
>  		return;
>  	}
>  
> +	if (src_tsr.kind == TSR_KIND_PERCPU_BASE) {
> +		int reg2;
> +
> +		if (!src->multi_regs) {
> +			invalidate_reg_state(tsr);
> +			return;
> +		}
> +
> +		reg2 = (sreg == src->reg1) ? src->reg2 : src->reg1;
> +
> +		if (!has_reg_type(state, reg2) || !state->regs[reg2].ok) {
> +			invalidate_reg_state(tsr);
> +			return;
> +		}
> +
> +		/* Treat percpu as array: inherit type from reg2 */
> +		tsr->type = state->regs[reg2].type;
> +		tsr->kind = state->regs[reg2].kind;
> +		tsr->offset = state->regs[reg2].offset;
> +		tsr->imm_value = state->regs[reg2].imm_value;
> +		tsr->ok = state->regs[reg2].ok;
> +
> +		pr_debug_dtp("add [%x] percpu (reg%d, reg%d) -> reg%d",
> +			     insn_offset, src->reg1, src->reg2, dreg);
> +		pr_debug_type_name(&tsr->type, tsr->kind);
> +		return;
> +	}
> +

[Severity: Medium]
Does this block ever execute when handling a per-cpu address calculation
(Pattern B like 'add x0, x6, x5') in update_add_insn_state()?

When arch_get_reg_offset() executes earlier in update_add_insn_state(), it
returns -1 for multi-register non-memory operations (like 'add') unless one
register is a TSR_KIND_CONST or the base is a known TSR_KIND_POINTER.

Since the registers here are PERCPU_BASE and GLOBAL_ADDR, it returns -1
and jumps to the try_other_reg label, permanently bypassing this new tracking
logic.

>  try_other_reg:
>  	/* Try another register if any */
>  	if (src->multi_regs && src->reg1 != src->reg2 && sreg != src->reg2 &&

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1788872630.git.wutengda@huaweicloud.com?part=25