Re: [PATCH v4 20/23] perf annotate-arm64: Support 'add' instruction tracking

[Tengda Wu]

On 2026/8/25 5:58, Namhyung Kim wrote:
> On Wed, Aug 19, 2026 at 11:04:36AM +0800, Tengda Wu wrote:
>>
>>
>> On 2026/8/11 16:45, Shuai Xue wrote:
>>>
>>>
>>> On 8/8/26 8:23 PM, Tengda Wu wrote:
>>>> Extend update_insn_state() for arm64 to track 'add' instructions for
>>>> structure member address calculation, which commonly appear as:
>>>>
>>>>    add  dreg, base, #offset
>>>>    add  dreg, base, reg2     (reg2 holds a constant)
>>>>
>>>> Unlike x86, the arm64 'add' instruction has an extra base register among
>>>> its source operands. Therefore, in terms of propagating the data type,
>>>> it is essentially performing a 'mov', except that before the 'mov', it
>>>> first needs to be updated by adding the offset or reg2.
>>>>
>>>> A real-world example is shown below:
>>>>
>>>>    ffff80008001c9a8 <flush_ptrace_hw_breakpoint>:
>>>>    ffff80008001c9c4:  add  x19, x0, #0xeb8   // x0 (task_struct*) + 0xeb8 -> x19
>>>> * ffff80008001c9d0:  ldr  x0, [x19]
>>>>
>>>> Before this commit, the type flow broke at the 'add' instruction,
>>>> leaving the subsequent load with no type information:
>>>>
>>>>    chk [28] reg19 offset=0 ok=0 kind=0 cfa : no type information
>>>>    final result: no type information
>>>>
>>>> After this commit, the tracker correctly follows the member address
>>>> calculation:
>>>>
>>>>    var [0] reg0 offset 0 type='struct task_struct*'
>>>>    add [1c] address of 0xeb8(reg0) -> reg19 type='struct task_struct*'
>>>>    chk [28] reg19 offset=0 ok=1 kind=1 (struct task_struct*) : Good!
>>>>    found by insn track: 0(reg19) type-offset=0xeb8
>>>>    final result: type='struct task_struct'
>>>>
>>>> Signed-off-by: Tengda Wu <wutengda@xxxxxxxxxxxxxxx>
>>>> ---
>>>>   .../perf/util/annotate-arch/annotate-arm64.c  | 87 ++++++++++++++++++-
>>>>   1 file changed, 85 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/tools/perf/util/annotate-arch/annotate-arm64.c b/tools/perf/util/annotate-arch/annotate-arm64.c
>>>> index 7b780bad8c07..eaeb4433fc3a 100644
>>>> --- a/tools/perf/util/annotate-arch/annotate-arm64.c
>>>> +++ b/tools/perf/util/annotate-arch/annotate-arm64.c
>>>> @@ -688,6 +688,87 @@ static void update_mov_insn_state(struct type_state *state,
>>>>       pr_debug_type_name(&tsr->type, tsr->kind);
>>>>   }
>>>>   +static void update_add_insn_state(struct type_state *state,
>>>> +                  struct disasm_line *dl,
>>>> +                  struct annotated_op_loc *src,
>>>> +                  struct annotated_op_loc *dst)
>>>> +{
>>>> +    struct type_state_reg *tsr;
>>>> +    struct type_state_reg src_tsr;
>>>> +    u32 insn_offset = dl->al.offset;
>>>> +    int sreg = src->reg1;
>>>> +    int dreg = dst->reg1;
>>>> +    u64 imm_value;
>>>> +
>>>> +    if (!has_reg_type(state, dreg))
>>>> +        return;
>>>> +
>>>> +    tsr = &state->regs[dreg];
>>>> +    tsr->copied_from = -1;
>>>> +
>>>> +retry:
>>>> +    if (!has_reg_type(state, sreg) || !state->regs[sreg].ok) {
>>>> +        invalidate_reg_state(tsr);
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    src_tsr = state->regs[sreg];
>>>> +
>>>> +    /*
>>>> +     * Handle 'add' instructions of the form:
>>>> +     *   add  dreg, base, #offset     (immediate offset)
>>>> +     *   add  dreg, base, reg2        (reg2 holds a constant)
>>>> +     *
>>>> +     * For case 2, retrieve the constant value from reg2
>>>> +     * and use it as the offset.
>>>> +     */
>>>> +    imm_value = src->offset;
>>>> +    if (src->multi_regs) {
>>>> +        int reg2 = (sreg == src->reg1) ? src->reg2 : src->reg1;
>>>> +
>>>> +        if (!has_reg_type(state, reg2) || !state->regs[reg2].ok) {
>>>> +            /* Unable to resolve type for dst, bail out */
>>>> +            invalidate_reg_state(tsr);
>>>> +            return;
>>>> +        }
>>>> +        if (state->regs[reg2].kind == TSR_KIND_CONST)
>>>> +            imm_value = state->regs[reg2].imm_value;
>>>
>>> First, when reg2 is valid but not TSR_KIND_CONST - say a pointer
>>> freshly loaded from memory - the addend is unknown at analysis time,
>>> yet imm_value silently stays 0 and propagation continues, tracking
>>> the destination as base + 0. Note the asymmetry: no state at all
>>> invalidates, but state that doesn't yield a value pretends the
>>> addend is zero. Unknown addend should invalidate as well:
>>>
>>>         if (state->regs[reg2].kind != TSR_KIND_CONST) {
>>>             invalidate_reg_state(tsr);
>>>             return;
>>>         }
>>>
>>
>> It might not be a direct invalidation, but rather swapping reg1/reg2 and
>> then retrying.
>>
>>> Second, the shifted/extended forms still break even with a constant
>>> reg2: extract_op_location_arm64() drops the "lsl #3" (or uxtw/sxtw)
>>> modifier, so add x0, x1, x2, lsl #3 contributes the unshifted value
>>> and maps later accesses to the wrong field. Maybe flag such operands
>>> during extraction (or fail the reg2 extraction) so the handler can
>>> skip them.
>>>
>>
>> Okay, so it seems shifted/extended forms are unavoidable. We may need to
>> introduce more fields into annotated_op_loc to record them, like this:
>>
>> diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
>> index 138d9258281c..b00e0ad96023 100644
>> --- a/tools/perf/util/annotate.h
>> +++ b/tools/perf/util/annotate.h
>> @@ -501,6 +501,8 @@ int arch__dwarf_regnum(const struct arch *arch, const char *str);
>>   * @offset: Memory access offset in the operand
>>   * @segment: Segment selector register
>>   * @addr_mode: Addressing mode, only valid if @mem_ref is true
>> + * @extend_type: ARM64 register extend specifier (enum annotated_ext_type)
>> + * @shift: ARM64 shift amount (e.g., 0 to 4)
>>   * @mem_ref: Whether the operand accesses memory
>>   * @multi_regs: Whether the second register is used
>>   * @imm: Whether the operand is an immediate value (in offset)
>> @@ -511,6 +513,8 @@ struct annotated_op_loc {
>>  	int offset;
>>  	u8 segment;
>>  	u8 addr_mode;
>> +	u8 extend_type;
>> +	u8 shift;
>>  	bool mem_ref;
>>  	bool multi_regs;
>>  	bool imm;
>> @@ -542,6 +546,19 @@ enum annotated_addr_mode {
>>  	PERF_ADDR_MODE_POST_INDEX,
>>  };
>>  
>> +enum annotated_ext_type {
>> +	PERF_EXT_NONE = 0,
>> +
>> +	PERF_EXT_UXTB,
>> +	PERF_EXT_SXTB,
>> +	PERF_EXT_UXTH,
>> +	PERF_EXT_SXTH,
>> +	PERF_EXT_UXTW,
>> +	PERF_EXT_SXTW,
>> +	PERF_EXT_UXTX,  /* LSL is equivalent to UXTX */

This comment is inaccurate and will be removed.

>> +	PERF_EXT_SXTX,
>> +};
>> +
> 
> Looks ok to me.  But more comments are needed for those who are not
> familiar with the arm64 ISA.
> 
> Thanks,
> Namhyung
> 

Got it. Will add more comments in v5.

Thanks,
Tengda

> 
>>  /**
>>   * struct annotated_insn_loc - Location info of instruction
>>   * @ops: Array of location info for source and target operands
>>
>>
>>>> +    }
>>>> +
>>>> +    if (src_tsr.kind == TSR_KIND_CONST) {
>>>> +        tsr->kind = src_tsr.kind;
>>>> +        tsr->imm_value = src_tsr.imm_value + imm_value;
>>>> +        tsr->offset = 0;
>>>> +        tsr->ok = src_tsr.ok;
>>>> +
>>>> +        pr_debug_dtp("add [%x] imm %#"PRIx64"(reg%d) -> reg%d\n",
>>>> +                 insn_offset, imm_value, sreg, dreg);
>>>
>>> And since add is commutative, this early return drops the pointer
>>> side for add rd, const_reg, ptr_reg: sreg starts as reg1, hits the
>>> CONST case and returns before the goto retry at the bottom ever gets
>>> a chance to evaluate the pointer register. The result should be the
>>> pointer with the constant applied as an offset, not a plain constant.
>>> Maybe check the other register for a pointer type before falling into
>>> the CONST case (or move the CONST handling after the retry).
>>>
>>
>> Yeah, this issue is a side effect of the previous incomplete handling and
>> will be fixed together.
>>
>> Thanks,
>> Tengda
>>