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

On 2026/8/11 16:24, Shuai Xue wrote:
> 
> 
> On 8/8/26 8:23 PM, Tengda Wu wrote:
>> 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.
>>
>> The implementation handles:
>>
>> 1. Stack Loads (LDR): Identify when a register is loaded from a stack
>>     slot and update the register's type state based on the tracked
>>     stack content or compound member types.
>>
>> 2. Stack Stores (STR): Update or create new stack state entries when
>>     a tracked register type is stored to the stack.
>>
>> Similar to loads, stores also support saving one or two registers
>> (in the case of 'stp'). Therefore, propagate_store_reg_state() is
>> introduced. The overall offset calculation follows the same logic as
>> the load implementation:
>>
>>    src->reg1: reg_offset = get_reg_index_offset()
>>    src->reg2: reg_offset = get_reg_index_offset() + reg_size(src->reg1)
>>
>> With these changes, the instruction tracker can now follow data types
>> as they move between registers and memory, specifically for function
>> local variables and compiler-spilled values on arm64.
>>
>> Signed-off-by: Tengda Wu <[email protected]>
>> ---
>>   .../perf/util/annotate-arch/annotate-arm64.c  | 161 ++++++++++++++++--
>>   1 file changed, 148 insertions(+), 13 deletions(-)
>>
>> diff --git a/tools/perf/util/annotate-arch/annotate-arm64.c b/tools/perf/util/annotate-arch/annotate-arm64.c
>> index ed0f0ef2877d..6e09e9707256 100644
>> --- a/tools/perf/util/annotate-arch/annotate-arm64.c
>> +++ b/tools/perf/util/annotate-arch/annotate-arm64.c
>> @@ -407,6 +407,7 @@ static void adjust_reg_index_state(struct type_state *state,
>>    * to the source struct's field offset.
>>    */
>>   static int propagate_load_reg_state(struct type_state *state,
>> +                    struct data_loc_info *dloc,
>>                       struct disasm_line *dl, int dreg,
>>                       struct annotated_op_loc *src,
>>                       int reg_offset, const char *insn_name)
>> @@ -416,6 +417,8 @@ static int propagate_load_reg_state(struct type_state *state,
>>       Dwarf_Die type_die;
>>       u32 insn_offset = dl->al.offset;
>>       int sreg = src->reg1;
>> +    int fbreg = dloc->fbreg;
>> +    int fboff = 0;
>>         if (!has_reg_type(state, dreg))
>>           return -1;
>> @@ -423,7 +426,52 @@ static int propagate_load_reg_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 = sreg == fbreg ? reg_offset - fboff : reg_offset;
>> +
>> +        stack = find_stack_state(state, offset);
>> +        if (stack == NULL) {
>> +            return -1;
>> +        } else if (!stack->compound) {
>> +            tsr->type = stack->type;
>> +            tsr->kind = stack->kind;
>> +            tsr->offset = stack->ptr_offset;
>> +            tsr->imm_value = stack->imm_value;
>> +            tsr->ok = true;
>> +        } else if (die_get_member_type(&stack->type,
>> +                           offset - stack->offset,
>> +                           &type_die)) {
>> +            tsr->type = type_die;
>> +            tsr->kind = TSR_KIND_TYPE;
>> +            tsr->offset = 0;
>> +            tsr->imm_value = 0;
>> +            tsr->ok = true;
>> +        } else {
>> +            return -1;
>> +        }
>> +
>> +        if (sreg == fbreg) {
>> +            pr_debug_dtp("%s [%x] -%#x(stack) -> reg%d",
>> +                     insn_name, insn_offset, -offset, dreg);
>> +        } else {
>> +            pr_debug_dtp("%s [%x] %#x(reg%d) -> reg%d",
>> +                     insn_name, insn_offset, offset, sreg, dreg);
>> +        }
>> +        pr_debug_type_name(&tsr->type, tsr->kind);
>> +        return 0;
>> +    }
>> +
>>       if (!has_reg_type(state, sreg) || !state->regs[sreg].ok)
>>           return -1;
>>   @@ -460,6 +508,7 @@ static int propagate_load_reg_state(struct type_state *state,
>>   }
>>     static void update_load_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)
>> @@ -472,7 +521,7 @@ static void update_load_insn_state(struct type_state *state,
>>           goto out_err_adjust;
>>         /* Handle the first destination register */
>> -    if (propagate_load_reg_state(state, dl, dst->reg1, src,
>> +    if (propagate_load_reg_state(state, dloc, dl, dst->reg1, src,
>>                        reg_offset, insn_name))
>>           goto out_err_adjust;
>>   @@ -481,7 +530,7 @@ static void update_load_insn_state(struct type_state *state,
>>           int reg_size = arm64__reg_size(dl->ops.target.raw);
>>             if (reg_size < 0 ||
>> -            propagate_load_reg_state(state, dl, dst->reg2, src,
>> +            propagate_load_reg_state(state, dloc, dl, dst->reg2, src,
>>                            reg_offset + reg_size, insn_name))
>>               goto out_err_adjust;
>>       }
>> @@ -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;
> 
> If an untracked register is stored into a stack slot that already
> has a tracked type, this leaves the stale entry in place - a later
> load from that slot picks up the old type. The sequence is easy to
> hit: store a tracked register, cross a call, then store a register
> that the call invalidated. The right behaviour would be to drop the
> stack entry when the source is unknown. I see x86 case 3 has the
> exact same pattern, so this probably wants a cross-arch fix rather
> than an arm64-only one.
> 

Agreed. We may need to introduce a delete_stack_state() helper to drop
the stack entry when the source register is unknown.

> Also, this function always returns 0 apart from that early check,
> and the callers ignore the return value anyway. Either make it void
> or return a real status and act on it.
> 

Will do.

>> +
>> +    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 = dreg == fbreg ? reg_offset - fboff : reg_offset;
>> +
>> +        tsr = &state->regs[sreg];
>> +
>> +        stack = find_stack_state(state, offset);
>> +        if (stack) {
>> +            if (!stack->compound)
>> +                set_stack_state(stack, offset, tsr->kind, &tsr->type,
>> +                        tsr->offset, tsr->imm_value);
>> +            /*
>> +             * If it's a compound type, it means attempting to
>> +             * write to a member value of the compound type without
>> +             * changing the compound type itself, so do nothing.
>> +             */
>> +        } else {
>> +            findnew_stack_state(state, offset, tsr->kind, &tsr->type,
>> +                        tsr->offset, tsr->imm_value);
>> +        }
>> +
>> +        if (dreg == fbreg) {
>> +            pr_debug_dtp("%s [%x] reg%d -> -%#x(stack)",
>> +                     insn_name, insn_offset, sreg, -offset);
>> +        } else {
>> +            pr_debug_dtp("%s [%x] reg%d -> %#x(reg%d)",
>> +                     insn_name, insn_offset, sreg, offset, dreg);
>> +        }
>> +        if (tsr->offset != 0) {
>> +            pr_debug_dtp(" reg%d offset %#x ->",
>> +                     sreg, tsr->offset);
>> +        }
>> +        pr_debug_type_name(&tsr->type, tsr->kind);
>> +    }
>> +    /*
>> +     * Ignore other transfers since it'd set a value in a struct
>> +     * and won't change the type.
>> +     */
>> +
>> +    return 0;
>> +}
>> +
>> +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";
> 
> This misparses the exclusive stores. For stxr w2, x1, [x0] the left
> operand is "w2, x1", so multi_regs is true and the handler treats it
> as stp: it records w2's type (the status output, possibly stale) at
> the base offset and the real data register x1 at offset + reg_size,
> and w2 - which the instruction writes - is never invalidated. stxp
> is worse since its third source register is dropped entirely.
> 
> Maybe restrict the propagation to str/stp explicitly and handle the
> stxr/stlxr/stxp/stlxp family separately: their first source register
> is actually a destination and should be invalidated, with only the
> remaining register(s) propagated (or just skip propagation for them
> if that's simpler).
> 

Agreed. For now, we should restrict the propagation to only apply to
regular store instructions like str/stp. I'll strengthen the instruction
matching to ensure exclusive stores are excluded.

>> +    /* 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);
> 
> 
> Same reg_size-from-first-register pattern as the load side, so the
> element-size caveat from that patch applies here too.
> 

For store instructions, there doesn't seem to be a special element-size
instruction like ldpsw on the load side, so using the register size
should be sufficient here (please correct me if I'm wrong). That said,
I agree that renaming "reg_size" to "elem_size" would make the code
more accurate and self-explanatory. I'll update it accordingly.

Thanks,
Tengda