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

[sashiko-bot@kernel.org]

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.

> +	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.

> +
> +		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