Re: [PATCH v4 15/23] perf annotate-arm64: Support store instruction tracking

On 8/8/26 8:23 PM, Tengda Wu wrote:
> Extend update_insn_state() for arm64 to handle store (STR) instructions.
> 
> Unlike load instructions, a store instruction sets a value in the struct
> within the memory where the destination register resides, and does not
> alter its type. Therefore, no processing is required for the transfer.
> 
> The only point to note is that store instructions support pre-index and
> post-index addressing modes, so calling adjust_reg_index_state() is still
> necessary to handle their addressing.
> 
> Signed-off-by: Tengda Wu <[email protected]>
> ---
>   tools/perf/util/annotate-arch/annotate-arm64.c | 13 ++++++++++++-
>   1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/annotate-arch/annotate-arm64.c b/tools/perf/util/annotate-arch/annotate-arm64.c
> index 6557c0ad11b2..ed0f0ef2877d 100644
> --- a/tools/perf/util/annotate-arch/annotate-arm64.c
> +++ b/tools/perf/util/annotate-arch/annotate-arm64.c
> @@ -560,7 +560,7 @@ static void update_insn_state_arm64(struct type_state *state,
>   	 * prevent stale type info from propagating to subsequent instructions.
>   	 */
>   	if (has_reg_type(state, dst->reg1) &&
> -	    strncmp(dl->ins.name, "ld", 2)) {
> +	    strncmp(dl->ins.name, "ld", 2) && strncmp(dl->ins.name, "st", 2)) {
>   		pr_debug_dtp("%s [%x] invalidate reg%d",
>   			     dl->ins.name, insn_offset, dst->reg1);
>   		invalidate_reg_state(&state->regs[dst->reg1]);
> @@ -575,6 +575,17 @@ static void update_insn_state_arm64(struct type_state *state,
>   	/* Memory to register transfers */
>   	if (!strncmp(dl->ins.name, "ld", 2))
>   		update_load_insn_state(state, dl, src, dst);
> +	/* Register to memory transfers */
> +	else if (!strncmp(dl->ins.name, "st", 2)) {
> +		/*
> +		 * Ignore transfers since it'd set a value in a struct
> +		 * and won't change the type.
> +		 *
> +		 * Needs to update the pre-index and post-index addressing
> +		 * modes for the destination register.
> +		 */
> +		adjust_reg_index_state(state, dst, "str", insn_offset);
> +	}
Two points on this call:

First, adjust_reg_index_state() gets post-index with a register
writeback amount wrong, e.g. str x1, [x0], x2. There the memory
operand has multi_regs set (x0 and x2), so reg2 gets the offset
register and op_loc->offset stays 0. The adjustment then degenerates
to offset += 0, and the base keeps its old type and offset while in
reality it moved by an unknown amount - every later access through it
resolves to the wrong struct field. Pre-index is fine since
[base, reg]! isn't a valid encoding, but the post-index register form
should fall back to invalidating the base:

     if (op_loc->multi_regs) {
         invalidate_reg_state(tsr);
         return;
     }

This applies to the load path too since it shares
adjust_reg_index_state().

Second, a nit: the insn name is hardcoded to "str", so stp/stlr and
pre/post-indexed variants all print as "str" in the debug output.
Pass dl->ins.name instead.


Thanks.
Shuai