Re: [PATCH v2 09/16] perf annotate-arm64: Support load instruction tracking

[Namhyung Kim <namhyung@kernel.org>]

On Fri, Apr 03, 2026 at 09:47:53AM +0000, Tengda Wu wrote:
> Implement update_insn_state() for arm64 to track register state changes
> during load (LDR) instructions. This is essential for maintaining accurate
> type information when data is moved from memory to registers.
> 
> The implementation handles the three primary arm64 addressing modes:
> 1. Signed offset: [base, #imm]
> 2. Pre-index: [base, #imm]!
> 3. Post-index: [base], #imm
> 
> Introduce adjust_reg_index_state() to handle the side effects of pre-index
> and post-index addressing, where the base register is updated with the
> offset after or before the memory access. This ensures that the register's
> offset within a structure is correctly tracked across sequential
> instructions.
> 
> 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]  // PMU sample at offset 0x68
> 
> 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@huawei.com>
> Signed-off-by: Tengda Wu <wutengda@huaweicloud.com>
> ---
>  .../perf/util/annotate-arch/annotate-arm64.c  | 87 +++++++++++++++++++
>  1 file changed, 87 insertions(+)
> 
> diff --git a/tools/perf/util/annotate-arch/annotate-arm64.c b/tools/perf/util/annotate-arch/annotate-arm64.c
> index 1fe4c503431b..cac2bf0021c9 100644
> --- a/tools/perf/util/annotate-arch/annotate-arm64.c
> +++ b/tools/perf/util/annotate-arch/annotate-arm64.c
> @@ -9,6 +9,8 @@
>  #include <regex.h>
>  #include "../annotate.h"
>  #include "../disasm.h"
> +#include "../annotate-data.h"
> +#include "../debug.h"
>  
>  struct arch_arm64 {
>  	struct arch arch;
> @@ -254,6 +256,88 @@ static int extract_op_location_arm64(const struct arch *arch,
>  	return 0;
>  }
>  
> +#ifdef HAVE_LIBDW_SUPPORT
> +static int get_mem_offset(struct annotated_op_loc *op_loc, int type_offset)
> +{
> +	if (op_loc->addr_mode == INSN_ADDR_POST_INDEX)
> +		return type_offset;
> +
> +	return op_loc->offset + type_offset;
> +}
> +
> +static void adjust_reg_index_state(struct type_state *state, int reg,
> +				   struct annotated_op_loc *op_loc,
> +				   const char *insn_name, u32 insn_offset)
> +{
> +	struct type_state_reg *tsr;
> +
> +	if (!has_reg_type(state, reg) ||
> +	    (op_loc->addr_mode != INSN_ADDR_PRE_INDEX &&
> +	    op_loc->addr_mode != INSN_ADDR_POST_INDEX))
> +		return;
> +
> +	tsr = &state->regs[reg];
> +	tsr->offset = op_loc->offset + tsr->offset;
> +	tsr->ok = true;

Maybe we can skip setting 'ok' here?


> +
> +	pr_debug_dtp("%s [%x] %s-index %#x(reg%d) -> reg%d", insn_name,
> +		     insn_offset, op_loc->addr_mode == INSN_ADDR_PRE_INDEX ?
> +		     "pre" : "post", op_loc->offset, reg, reg);
> +	pr_debug_type_name(&tsr->type, tsr->kind);
> +}
> +
> +static void update_insn_state_arm64(struct type_state *state,
> +				    struct data_loc_info *dloc, Dwarf_Die * cu_die __maybe_unused,
> +				    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];
> +	struct type_state_reg *tsr;
> +	Dwarf_Die type_die;
> +	u32 insn_offset = dl->al.offset;
> +	int sreg, dreg;
> +
> +	if (annotate_get_insn_location(dloc->arch, dl, &loc) < 0)
> +		return;
> +
> +	sreg = src->reg1;
> +	dreg = dst->reg1;
> +
> +	/* Memory to register transfers */
> +	if (!strncmp(dl->ins.name, "ld", 2)) {
> +		struct type_state_reg dst_tsr;
> +
> +		if (!has_reg_type(state, sreg) ||
> +		    !has_reg_type(state, dreg) ||
> +		    !state->regs[dreg].ok)
> +			return;

I think we should set state->regs[sreg].ok to false if sreg is valid.

> +
> +		tsr = &state->regs[sreg];
> +		tsr->copied_from = -1;
> +		dst_tsr = state->regs[dreg];
> +
> +		/* Dereference the pointer if it has one */
> +		if (dst_tsr.kind == TSR_KIND_TYPE &&
> +		    die_deref_ptr_type(&dst_tsr.type,
> +				       get_mem_offset(dst, dst_tsr.offset),
> +				       &type_die)) {
> +			tsr->type = type_die;
> +			tsr->kind = TSR_KIND_TYPE;
> +			tsr->offset = 0;
> +			tsr->ok = true;
> +
> +			pr_debug_dtp("ldr [%x] %#x(reg%d) -> reg%d",
> +				     insn_offset, dst->offset, dreg, sreg);

It could confuse people since src and dst are opposite.  Maybe you can
change the parse function to set the source and target (destination)
properly for each instruction.


> +			pr_debug_type_name(&tsr->type, tsr->kind);
> +
> +			adjust_reg_index_state(state, dreg, dst, "ldr", insn_offset);
> +		}

Also you will need to update index offset and set the 'ok' state
properly.

Thanks,
Namhyung

> +		return;
> +	}
> +}
> +#endif
> +
>  const struct arch *arch__new_arm64(const struct e_machine_and_e_flags *id,
>  				   const char *cpuid __maybe_unused)
>  {
> @@ -273,6 +357,9 @@ const struct arch *arch__new_arm64(const struct e_machine_and_e_flags *id,
>  	arch->objdump.imm_char		  = '#';
>  	arch->associate_instruction_ops   = arm64__associate_instruction_ops;
>  	arch->extract_op_location	  = extract_op_location_arm64;
> +#ifdef HAVE_LIBDW_SUPPORT
> +	arch->update_insn_state		  = update_insn_state_arm64;
> +#endif
>  
>  	/* bl, blr */
>  	err = regcomp(&arm->call_insn, "^blr?$", REG_EXTENDED);
> -- 
> 2.34.1
>