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

[Tengda Wu <wutengda@huaweicloud.com>]

On 2026/9/8 21:30, sashiko-bot@kernel.org wrote:
> 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.
> 

Indeed. The return happens too early.

>>  	/* 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.
> 

Agreed. This is the same issue as above. Will fix in v6.

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