Re: [PATCH v3 16/21] perf annotate-arm64: Support stack variable tracking

[Namhyung Kim <namhyung@kernel.org>]

On Wed, Jul 01, 2026 at 04:16:47AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> - [Medium] Incomplete handling of ARM64 load/store pair instructions (ldp/stp) causes type tracking corruption by ignoring the second register.
> - [Medium] Missing invalidation of destination register when type resolution fails during a memory load.
> - [Medium] Missing invalidation of stack state when storing an unknown register type.
> - [Medium] Incorrect stack offset calculation when accessing memory via the stack pointer while a different frame base register is active.
> --
> 
> commit 3e734515747646c2063edf487f47005c1433cf1a
> Author: Tengda Wu <wutengda@huaweicloud.com>
> 
> perf annotate-arm64: Support stack variable tracking
> 
> This commit extends update_insn_state() for arm64 to track data types
> stored on the stack. It handles stack loads and stores to update the
> register's type state or stack state. This allows 'perf annotate' to
> maintain type information for local variables as they move between registers
> and memory.
> 
> > diff --git a/tools/perf/util/annotate-arch/annotate-arm64.c b/tools/perf/util/annotate-arch/annotate-arm64.c
> > index f283596bb0589..255ccaf331309 100644
> > --- a/tools/perf/util/annotate-arch/annotate-arm64.c
> > +++ b/tools/perf/util/annotate-arch/annotate-arm64.c
> [ ... ]
> > @@ -368,6 +371,8 @@ static void update_load_insn_state(struct type_state *state,
> >  	int reg_offset;
> >  	int sreg = src->reg1;
> >  	int dreg = dst->reg1;
>         ^^^^^^^^^^^^^^^^^^^^
> [Severity: Medium]
> When handling ARM64 load pair instructions (ldp), does this code ignore the
> second destination register (dst->reg2)? The second register of a load pair
> could retain its stale type instead of being updated, which might lead to
> false type propagation.

Splitting "ldp" case would prevent this kind of issue.

> 
> > +	int fbreg = dloc->fbreg;
> > +	int fboff = 0;
> >  
> >  	if (!has_reg_type(state, dreg))
> >  		goto out_adjust;
> > @@ -375,7 +380,52 @@ static void update_load_insn_state(struct type_state *state,
> >  	tsr = &state->regs[dreg];
> >  	tsr->copied_from = -1;
> >  
> > +	if (dloc->fb_cfa) {
> > +		u64 ip = dloc->ms->sym->start + dl->al.offset;
> > +		u64 pc = map__rip_2objdump(dloc->ms->map, ip);
> > +
> > +		if (die_get_cfa(dloc->di->dbg, pc, &fbreg, &fboff) < 0)
> > +			fbreg = -1;
> > +	}
> > +
> >  retry:
> > +	/* Check stack variables with offset */
> > +	if (sreg == fbreg || sreg == state->stack_reg) {
> > +		struct type_state_stack *stack;
> > +		int offset = src->offset - fboff;
>                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> [Severity: Medium]
> Will this subtract the frame base offset even when the access is relative to
> the stack pointer (when sreg == state->stack_reg)? If the stack pointer is
> used and fbreg is different (e.g., x29), this could yield an incorrect
> internal stack offset and lose type information.

I think it's a common issue that can happen on other arch.  On x86, I
never saw where stack pointer is used when fbreg is RBP.  Generally I
think it won't generate code to use RSP for memory access when frame
base register is set to RBP.

Thanks,
Namhyung

> 
> > +
> > +		stack = find_stack_state(state, offset);
> [ ... ]
> > @@ -413,6 +463,70 @@ static void update_load_insn_state(struct type_state *state,
> >  	adjust_reg_index_state(state, src, "ldr", insn_offset);
>         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> [Severity: Medium]
> Is the destination register left uninvalidated if type resolution fails
> during a memory load and the code falls through the multi-regs retry logic
> to the out_adjust label? The destination register could retain its prior type
> instead of being invalidated, even though its value was overwritten by an
> unknown memory load.
> 
> >  }
> >  
> > +static void update_store_insn_state(struct type_state *state,
> > +				    struct data_loc_info *dloc,
> > +				    struct disasm_line *dl,
> > +				    struct annotated_op_loc *src,
> > +				    struct annotated_op_loc *dst)
> > +{
> > +	struct type_state_reg *tsr;
> > +	u32 insn_offset = dl->al.offset;
> > +	int sreg = src->reg1;
> > +	int dreg = dst->reg1;
>         ^^^^^^^^^^^^^^^^^^^^
> [Severity: Medium]
> Similar to the load instructions, does this logic ignore the second source
> register (src->reg2) for ARM64 store pair instructions (stp)? The second
> register of a store pair wouldn't be saved to the stack.
> 
> > +	int fbreg = dloc->fbreg;
> > +	int fboff = 0;
> > +
> > +	if (!has_reg_type(state, sreg) || !state->regs[sreg].ok)
> > +		goto out_adjust;
>         ^^^^^^^^^^^^^^^^
> [Severity: Medium]
> By skipping the rest of the function here, are we bypassing the invalidation
> of the stack state? If an untracked register is stored into a stack slot that
> previously held a tracked type, the stale type might persist on the stack.
> 
> > +
> > +	if (dloc->fb_cfa) {
> > +		u64 ip = dloc->ms->sym->start + dl->al.offset;
> > +		u64 pc = map__rip_2objdump(dloc->ms->map, ip);
> > +
> > +		if (die_get_cfa(dloc->di->dbg, pc, &fbreg, &fboff) < 0)
> > +			fbreg = -1;
> > +	}
> > +
> > +	/* Check stack variables with offset */
> > +	if (dreg == fbreg || dreg == state->stack_reg) {
> > +		struct type_state_stack *stack;
> > +		int offset = dst->offset - fboff;
>                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> [Severity: Medium]
> Could this calculation yield an incorrect internal stack offset by
> subtracting the frame base offset even when the access is relative to the
> stack pointer? This could happen if the frame base register is different from
> the stack pointer.
> 
> > +
> > +		tsr = &state->regs[sreg];
> [ ... ]
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260701035355.752944-1-wutengda@huaweicloud.com?part=16