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

On 2026/8/11 16:00, Shuai Xue wrote:
> 
> 
> 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;
>     }
> 

The manual says that "[base], reg" is only used in SIMD instructions,
and since SIMD is not currently supported, the impact is limited.
However, for the sake of accuracy, I will add this check. Thank you.

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

I referred to the x86 approach here (x86 also does not distinguish specific
instruction names; for example, all mov* variants are printed as "mov").
Originally, I only intended to distinguish between the two categories, ldr
and str. Of course, using the specific instruction name is not a problem,
and I will fix it accordingly.

Thanks,
Tengda