Re: [PATCH v4 18/23] perf annotate-arm64: Support stack variable tracking

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Incorrect register mapping and stack state propagation for exclusive store instructions (stxr, stxp).
- [Medium] Failure to invalidate stack state when storing an unknown register type.
--

--- Patch [18]: [PATCH v4 18/23] perf annotate-arm64: Support stack variable tracking ---
commit af17741623908e66e62f48c13604a02748d39721
Author: Tengda Wu <[email protected]>
Subject: perf annotate-arm64: Support stack variable tracking

Extend update_insn_state() for arm64 to track data types stored on the
stack. This allows 'perf annotate' to maintain type information for
local variables that are spilled to or loaded from stack slots.

> diff --git a/tools/perf/util/annotate-arch/annotate-arm64.c b/tools/perf/util/annotate-arch/annotate-arm64.c
> index ed0f0ef2877d0..6e09e97072568 100644
> --- a/tools/perf/util/annotate-arch/annotate-arm64.c
> +++ b/tools/perf/util/annotate-arch/annotate-arm64.c
[ ... ]
> @@ -498,6 +547,100 @@ static void update_load_insn_state(struct type_state *state,
>  	goto out_adjust;
>  }
>  
> +/*
> + * For store insns: propagate type from @sreg to the memory location
> + * referenced by @dreg, applying @reg_offset to the destination memory offset.
> + */
> +static int propagate_store_reg_state(struct type_state *state,
> +				     struct data_loc_info *dloc,
> +				     struct disasm_line *dl, int sreg, int dreg,
> +				     int reg_offset, const char *insn_name)
> +{
> +	struct type_state_reg *tsr;
> +	u32 insn_offset = dl->al.offset;
> +	int fbreg = dloc->fbreg;
> +	int fboff = 0;
> +
> +	if (!has_reg_type(state, sreg) || !state->regs[sreg].ok)
> +		return -1;

[Severity: Medium]
Will this early return fail to invalidate the stack state when storing an
unknown register type?

If a register without a known type is stored to a stack location that
previously held a known type, this early return appears to bypass stack
invalidation. This would leave stale type information intact for that
stack offset, causing subsequent loads from that slot to erroneously
propagate the old type. While similar logic exists in the x86
implementation, does it need to be corrected here to prevent stale types?

[ ... ]
> +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)
> +{
> +	int reg_offset = get_reg_index_offset(dst);
> +	const char *insn_name = src->multi_regs ? "stp" : "str";
> +
> +	/* Handle the first source register */
> +	propagate_store_reg_state(state, dloc, dl, src->reg1, dst->reg1,
> +				  reg_offset, insn_name);
> +
> +	/* Handle the second source register (stp only) */
> +	if (src->multi_regs) {
> +		int reg_size = arm64__reg_size(dl->ops.source.raw);
> +
> +		if (reg_size >= 0)
> +			propagate_store_reg_state(state, dloc, dl, src->reg2,
> +						  dst->reg1, reg_offset + reg_size,
> +						  insn_name);
> +	}

[Severity: Medium]
Does this logic correctly handle exclusive store instructions like stxr or
stxp?

When the instruction tracker parses arm64 exclusive store instructions, it
appears arm64__check_multi_regs() sets src->multi_regs to true because of
the comma-separated registers.

Would update_store_insn_state() then incorrectly assume the instruction is
stp, and propagate the type of the status register to the base memory
offset, while assigning the true source register to the offset + reg_size?

> +
> +	adjust_reg_index_state(state, dst, insn_name, dl->al.offset);
> +}
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=18