Re: [PATCH v4 14/23] perf annotate-arm64: Support load instruction tracking
From: Tengda Wu
Date: Thu Aug 13 2026 - 10:56:46 EST
On 2026/8/11 15:36, Shuai Xue wrote:
>
>
> On 8/8/26 8:23 PM, Tengda Wu wrote:
>> Extend update_insn_state_arm64() to handle LDR instructions, tracking
>> register state changes when data is loaded from memory to registers.
>>
>> The implementation handles the three primary arm64 addressing modes:
>> 1. Signed offset: [base, #imm|reg]
>> 2. Pre-index: [base, #imm]!
>> 3. Post-index: [base], #imm
>>
>> Before updating, check the addressing mode via get_reg_index_offset() to
>> obtain the actual source's reg_offset, and then propagate the type.
>>
>> Since a load instruction may have two destination registers (in ldp cases),
>> introduce propagate_load_reg_state() to propagate the type for a specified
>> destination register using a given reg_offset. The respective reg_offset
>> values for the two registers are as follows:
>>
>> dst->reg1: reg_offset = get_reg_index_offset()
>> dst->reg2: reg_offset = get_reg_index_offset() + reg_size(dst->reg1)
>>
>> Finally, handle the side effects of pre-index and post-index addressing
>> via adjust_reg_index_state().
>>
>> A real-world example is shown below:
>>
>> ffff80008011f5b0 <pick_task_stop>:
>> ffff80008011f5b8: ldr x0, [x0, #2712] // x0: struct rq* -> task_struct*
>> * ffff80008011f5c0: ldr w1, [x0, #104]
>>
>> Before this commit, the type of x0 was incorrectly inferred as 'struct rq':
>>
>> find data type for 0x68(reg0) at pick_task_stop+0x10
>> var [8] reg0 offset 0 type='struct rq*'
>> chk [10] reg0 offset=0x68 ok=1 kind=1 (struct rq*) : Good!
>> final result: type='struct rq'
>>
>> After this commit, the type of x0 is correctly inferred as 'struct task_struct':
>>
>> find data type for 0x68(reg0) at pick_task_stop+0x10
>> var [8] reg0 offset 0 type='struct rq*'
>> ldr [8] 0xa98(reg0) -> reg0 type='struct task_struct*'
>> chk [10] reg0 offset=0x68 ok=1 kind=1 (struct task_struct*) : Good!
>> final result: type='struct task_struct'
>>
>> Signed-off-by: Li Huafei <lihuafei1@xxxxxxxxxx>
>> Signed-off-by: Tengda Wu <wutengda@xxxxxxxxxxxxxxx>
>> ---
>> .../perf/util/annotate-arch/annotate-arm64.c | 148 +++++++++++++++++-
>> 1 file changed, 147 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/perf/util/annotate-arch/annotate-arm64.c b/tools/perf/util/annotate-arch/annotate-arm64.c
>> index acff14ca01e0..6557c0ad11b2 100644
>> --- a/tools/perf/util/annotate-arch/annotate-arm64.c
>> +++ b/tools/perf/util/annotate-arch/annotate-arm64.c
>> @@ -358,11 +358,152 @@ static int extract_op_location_arm64(const struct arch *arch,
>> }
>> #ifdef HAVE_LIBDW_SUPPORT
>> +static int arm64__reg_size(const char *reg)
>> +{
>> + if (!reg || !*reg || !arm64__is_reg(reg))
>> + return -1;
>
> Since arm64__is_reg() rejects xzr and SIMD registers, something like
> ldp xzr, x19, [sp] ends up with multi_regs = false and x19 never
> invalidated or tracked. Admittedly a corner case - but maybe worth
> handling if extending the register set is cheap.
>
Agreed. But I'd propose that we first support recognizing xzr/wzr only,
and leave SIMD registers for a future extension when we properly add SIMD support.
There shouldn't be a case where SIMD registers and general-purpose registers
appear together within the same operands, right? If so, then skipping SIMD
support for now should be fine.
>
>> + if (reg[0] == 'w')
>> + return 4;
>> +
>> + if (reg[0] == 'x' || !strncmp(reg, "sp", 2))
>> + return 8;
>> +
>> + return -1;
>> +}
>> +
>> +static int get_reg_index_offset(struct annotated_op_loc *op_loc)
>> +{
>> + return op_loc->addr_mode == PERF_ADDR_MODE_POST_INDEX ? 0 : op_loc->offset;
>> +}
>> +
>> +/* Apply addressing mode (pre-index, post-index) to register state */
>> +static void adjust_reg_index_state(struct type_state *state,
>> + struct annotated_op_loc *op_loc,
>> + const char *insn_name, u32 insn_offset)
>> +{
>> + struct type_state_reg *tsr;
>> + int reg = op_loc->reg1;
>> +
>> + if (op_loc->addr_mode != PERF_ADDR_MODE_PRE_INDEX &&
>> + op_loc->addr_mode != PERF_ADDR_MODE_POST_INDEX)
>> + return;
>> +
>> + if (!has_reg_type(state, reg) || !state->regs[reg].ok)
>> + return;
>> +
>> + tsr = &state->regs[reg];
>> + tsr->copied_from = -1;
>> + tsr->offset = op_loc->offset + tsr->offset;
>> +
>> + pr_debug_dtp("%s [%x] %s-index %#x(reg%d) -> reg%d", insn_name,
>> + insn_offset, op_loc->addr_mode == PERF_ADDR_MODE_PRE_INDEX ?
>> + "pre" : "post", op_loc->offset, reg, reg);
>> + pr_debug_type_name(&tsr->type, tsr->kind);
>> +}
>> +
>> +/*
>> + * For load insns: propagate type from @src to @dreg, applying @reg_offset
>> + * to the source struct's field offset.
>> + */
>> +static int propagate_load_reg_state(struct type_state *state,
>> + struct disasm_line *dl, int dreg,
>> + struct annotated_op_loc *src,
>> + int reg_offset, const char *insn_name)
>> +{
>> + struct type_state_reg *tsr;
>> + struct type_state_reg src_tsr;
>> + Dwarf_Die type_die;
>> + u32 insn_offset = dl->al.offset;
>> + int sreg = src->reg1;
>> +
>> + if (!has_reg_type(state, dreg))
>> + return -1;
>> +
>> + tsr = &state->regs[dreg];
>> + tsr->copied_from = -1;
>> +
>> +retry:
>> + if (!has_reg_type(state, sreg) || !state->regs[sreg].ok)
>> + return -1;
>> +
>> + src_tsr = state->regs[sreg];
>> +
>> + /* Dereference the pointer if it has one */
>> + if (src_tsr.kind == TSR_KIND_TYPE &&
>> + die_deref_ptr_type(&src_tsr.type,
>> + src_tsr.offset + reg_offset, &type_die)) {
>> + tsr->type = type_die;
>> + tsr->kind = TSR_KIND_TYPE;
>> + tsr->offset = 0;
>> + tsr->ok = true;
>> +
>> + if (src->multi_regs) {
>> + pr_debug_dtp("%s [%x] %#x(reg%d, reg%d) -> reg%d",
>> + insn_name, insn_offset, reg_offset,
>> + src->reg1, src->reg2, dreg);
>> + } else {
>> + pr_debug_dtp("%s [%x] %#x(reg%d) -> reg%d",
>> + insn_name, insn_offset, reg_offset,
>> + sreg, dreg);
>> + }
>> + pr_debug_type_name(&tsr->type, tsr->kind);
>> + return 0;
>> + }
>> + /* Or try another register if any */
>> + else if (src->multi_regs && src->reg1 != src->reg2 && sreg != src->reg2) {
>> + sreg = src->reg2;
>> + goto retry;
>> + }
>> +
>> + return -1;
>> +}
>> +
>> +static void update_load_insn_state(struct type_state *state,
>> + struct disasm_line *dl,
>> + struct annotated_op_loc *src,
>> + struct annotated_op_loc *dst)
>> +{
>> + int reg_offset = get_reg_index_offset(src);
>> + const char *insn_name = dst->multi_regs ? "ldp" : "ldr";
>> +
>> + if (!has_reg_type(state, dst->reg1) ||
>> + (dst->multi_regs && !has_reg_type(state, dst->reg2)))
>> + goto out_err_adjust;
>> +
>> + /* Handle the first destination register */
>> + if (propagate_load_reg_state(state, dl, dst->reg1, src,
>> + reg_offset, insn_name))
>> + goto out_err_adjust;
>> +
>> + /* Handle the second destination register (ldp only) */
>> + if (dst->multi_regs) {
>> + int reg_size = arm64__reg_size(dl->ops.target.raw);
>
> Two issues here:
>
> First, propagate_load_reg_state() snapshots state->regs[sreg] at call
> time. For an aliased pair like ldp x0, x1, [x0], the first call
> overwrites x0, and the second call then reads the freshly loaded type
> as the base instead of the original one. Better snapshot the source
> state once in update_load_insn_state() and pass it down.
>
Yeah, should snapshot first.
> Second, ldpsw loads two 32-bit words from memory (sign-extended into
> x-registers), so the element spacing is 4, not the 8 that
> arm64__reg_size() derives from the destination register. The element
> size needs to come from the mnemonic.
>
Agreed.
>
>> +
>> + if (reg_size < 0 ||
>> + propagate_load_reg_state(state, dl, dst->reg2, src,
>> + reg_offset + reg_size, insn_name))
>> + goto out_err_adjust;
>> + }
>> +
>> +out_adjust:
>> + adjust_reg_index_state(state, src, insn_name, dl->al.offset);
>> + return;
>> +
>> +out_err_adjust:
>> + if (has_reg_type(state, dst->reg1))
>> + invalidate_reg_state(&state->regs[dst->reg1]);
>> + if (dst->multi_regs && has_reg_type(state, dst->reg2))
>> + invalidate_reg_state(&state->regs[dst->reg2]);
>> + goto out_adjust;
>> +}
>> +
>> static void update_insn_state_arm64(struct type_state *state,
>> struct data_loc_info *dloc, Dwarf_Die *cu_die,
>> struct disasm_line *dl)
>> {
>> struct annotated_insn_loc loc;
>> + struct annotated_op_loc *src = &loc.ops[INSN_OP_SOURCE];
>> struct annotated_op_loc *dst = &loc.ops[INSN_OP_TARGET];
>> u32 insn_offset = dl->al.offset;
>> @@ -418,7 +559,8 @@ static void update_insn_state_arm64(struct type_state *state,
>> * Invalidate destination register(s) for unsupported instructions to
>> * prevent stale type info from propagating to subsequent instructions.
>> */
>> - if (has_reg_type(state, dst->reg1)) {
>> + if (has_reg_type(state, dst->reg1) &&
>> + strncmp(dl->ins.name, "ld", 2)) {
>
> For stores the operands are reversed, so dst is the memory operand
> and dst->reg1 is the base register. This then invalidates the base
> pointer on every store, which is exactly the state we need to resolve
> the memory access type. The x86 version guards this with
> !dst->mem_ref; arm64 should too:
>
> if (has_reg_type(state, dst->reg1) && !dst->mem_ref &&
> strncmp(dl->ins.name, "ld", 2)) {
>
Agreed.
Thanks,
Tengda