Re: [PATCH v2 07/16] perf annotate-arm64: Implement extract_op_location() callback

[Tengda Wu]

On 2026/4/7 15:26, Namhyung Kim wrote:
> On Fri, Apr 03, 2026 at 09:47:51AM +0000, Tengda Wu wrote:
>> Implement the extract_op_location() callback for the arm64 architecture
>> to handle its specific assembly syntax and addressing modes.
>>
>> The extractor handles:
>> 1. Standalone immediate operands (e.g., #0x10).
>> 2. Memory references with diverse addressing modes:
>>    - Signed offset: [base, #imm]
>>    - Pre-index: [base, #imm]!
>>    - Post-index: [base], #imm
>> 3. Multi-register operands and primary/secondary register extraction.
>>
>> This enables 'perf annotate' to resolve memory locations and registers
>> required for data type profiling on arm64.
>>
>> Signed-off-by: Tengda Wu <wutengda@xxxxxxxxxxxxxxx>
>> ---
>>  .../perf/util/annotate-arch/annotate-arm64.c  | 64 +++++++++++++++++++
>>  tools/perf/util/annotate.c                    | 12 ++--
>>  tools/perf/util/annotate.h                    | 10 +++
>>  3 files changed, 81 insertions(+), 5 deletions(-)
>>
>> diff --git a/tools/perf/util/annotate-arch/annotate-arm64.c b/tools/perf/util/annotate-arch/annotate-arm64.c
>> index 8209faaa6086..1fe4c503431b 100644
>> --- a/tools/perf/util/annotate-arch/annotate-arm64.c
>> +++ b/tools/perf/util/annotate-arch/annotate-arm64.c
>> @@ -191,6 +191,69 @@ static const struct ins_ops *arm64__associate_instruction_ops(struct arch *arch,
>>  	return ops;
>>  }
>>  
>> +static int extract_op_location_arm64(const struct arch *arch,
>> +				     struct disasm_line *dl __maybe_unused,
>> +				     const char *op_str, int op_idx __maybe_unused,
>> +				     struct annotated_op_loc *op_loc)
>> +{
>> +	const char *s = op_str;
>> +	char *p = NULL;
>> +
>> +	if (op_str == NULL)
>> +		return 0;
>> +
>> +	/* Handle standalone immediate operands (e.g., #0x10) */
>> +	if (*s == arch->objdump.imm_char) {
>> +		op_loc->offset = strtol(s + 1, &p, 0);
>> +		if (p && p != s + 1)
>> +			op_loc->imm = true;
>> +		return 0;
>> +	}
>> +
>> +	/*
>> +	 * Handle memory references (e.g., [x0, #8]), identify
>> +	 * arm64 specific addressing modes
>> +	 */
>> +	if (*s == arch->objdump.memory_ref_char) {
>> +		op_loc->mem_ref = true;
>> +
>> +		p = strchr(s, ']');
>> +		if (p == NULL)
>> +			return -1;
>> +
>> +		/* Pre-index: [base, #imm]! */
>> +		if (p[1] == '!')
>> +			op_loc->addr_mode = INSN_ADDR_PRE_INDEX;
>> +		/* Post-index: [base], #imm */
>> +		else if (p[1] == ',' && strchr(p + 1, arch->objdump.imm_char))
>> +			op_loc->addr_mode = INSN_ADDR_POST_INDEX;
>> +		/* Signed offset: [base{, #imm}] */
>> +		else
>> +			op_loc->addr_mode = INSN_ADDR_SIGNED_OFFSET;
>> +
>> +		s++;
>> +	}
>> +
>> +	/* Extract the primary register */
>> +	op_loc->reg1 = arch__dwarf_regnum(arch, s);
>> +	if (op_loc->reg1 == -1)
>> +		return -1;
>> +
>> +	/* Move to the next symbol of the operand, if any */
>> +	s = strchr(s, ',');
>> +	if (s == NULL)
>> +		return 0;
>> +	s = skip_spaces(s + 1);
>> +
>> +	/* Parse secondary register or immediate offset */
>> +	if (op_loc->multi_regs)
>> +		op_loc->reg2 = arch__dwarf_regnum(arch, s);
>> +	else if (*s == arch->objdump.imm_char)
>> +		op_loc->offset = strtol(s + 1, &p, 0);
>> +
>> +	return 0;
>> +}
>> +
>>  const struct arch *arch__new_arm64(const struct e_machine_and_e_flags *id,
>>  				   const char *cpuid __maybe_unused)
>>  {
>> @@ -209,6 +272,7 @@ const struct arch *arch__new_arm64(const struct e_machine_and_e_flags *id,
>>  	arch->objdump.memory_ref_char	  = '[';
>>  	arch->objdump.imm_char		  = '#';
>>  	arch->associate_instruction_ops   = arm64__associate_instruction_ops;
>> +	arch->extract_op_location	  = extract_op_location_arm64;
>>  
>>  	/* bl, blr */
>>  	err = regcomp(&arm->call_insn, "^blr?$", REG_EXTENDED);
>> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
>> index 1bf69e00d76d..c4d1cb3a7ae4 100644
>> --- a/tools/perf/util/annotate.c
>> +++ b/tools/perf/util/annotate.c
>> @@ -2452,19 +2452,21 @@ int annotate_check_args(void)
>>  
>>  int arch__dwarf_regnum(const struct arch *arch, const char *str)
>>  {
>> -	const char *p;
>> +	const char *p = str;
>>  	char *regname, *q;
>>  	int reg;
>>  
>> -	p = strchr(str, arch->objdump.register_char);
>> -	if (p == NULL)
>> -		return -1;
>> +	if (arch->objdump.register_char) {
>> +		p = strchr(str, arch->objdump.register_char);
>> +		if (p == NULL)
>> +			return -1;
>> +	}
>>  
>>  	regname = strdup(p);
>>  	if (regname == NULL)
>>  		return -1;
>>  
>> -	q = strpbrk(regname, ",) ");
>> +	q = strpbrk(regname, ",)] ");
>>  	if (q)
>>  		*q = '\0';
>>  
> 
> I think it's better to split changes in this function into a separate
> commit earlier in the series.  Please add description about arm64 asm
> format about register prefix and different memory reference delimiters.
> Probably you want to merge the change to remove 'static' scope here.
> 

Okay.

> 
>> diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
>> index 71195a27d38f..0391c6a9f011 100644
>> --- a/tools/perf/util/annotate.h
>> +++ b/tools/perf/util/annotate.h
>> @@ -496,12 +496,21 @@ int annotate_check_args(void);
>>  
>>  int arch__dwarf_regnum(const struct arch *arch, const char *str);
>>  
>> +enum annotated_addr_mode {
>> +	INSN_ADDR_NONE = 0,
>> +
>> +	INSN_ADDR_SIGNED_OFFSET,
>> +	INSN_ADDR_PRE_INDEX,
>> +	INSN_ADDR_POST_INDEX,
> 
> Probably better to have "PERF" prefix.  Maybe PERF_AAM_SIGNED_OFFSET
> (AAM from annotated_addr_mode) or PERF_ADDR_MODE_SIGNED_OFFSET?
> 
> Thanks,
> Namhyung
> 

I vote for 'PERF_ADDR_MODE_'. It's more explicit and readable than the
abbreviated version.

-- Tengda

> 
>> +};
>> +
>>  /**
>>   * struct annotated_op_loc - Location info of instruction operand
>>   * @reg1: First register in the operand
>>   * @reg2: Second register in the operand
>>   * @offset: Memory access offset in the operand
>>   * @segment: Segment selector register
>> + * @addr_mode: Addressing mode, only valid if @mem_ref is true
>>   * @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 +520,7 @@ struct annotated_op_loc {
>>  	int reg2;
>>  	int offset;
>>  	u8 segment;
>> +	u8 addr_mode;
>>  	bool mem_ref;
>>  	bool multi_regs;
>>  	bool imm;
>> -- 
>> 2.34.1
>>