Re: [PATCH v4 18/23] perf annotate-arm64: Support stack variable tracking
From: Tengda Wu
Date: Fri Aug 14 2026 - 07:46:44 EST
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 <wutengda@xxxxxxxxxxxxxxx>
>> ---
>> .../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