Re: [PATCH v4 14/23] perf annotate-arm64: Support load instruction tracking

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 <[email protected]>
>> Signed-off-by: Tengda Wu <[email protected]>
>> ---
>>   .../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