Re: [PATCH v4 06/23] perf annotate: Adapt arch__dwarf_regnum() for arm64

On 2026/8/8 21:07, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] The patch's fix to enable arm64 register parsing is unreachable and ineffective because the callers still unconditionally require a register prefix.
> - [Medium] The new optional prefix logic in `arch__dwarf_regnum()` fails to strip the opening bracket from memory operands.
> --
> 
> --- Patch [6]: [PATCH v4 06/23] perf annotate: Adapt arch__dwarf_regnum() for arm64 ---
> commit 1ed2e0e5199a9033fdb64817a106aecaa0871796
> Author: Tengda Wu <[email protected]>
> 
> perf annotate: Adapt arch__dwarf_regnum() for arm64
> 
> This commit makes the register prefix check in arch__dwarf_regnum() optional
> to support arm64 assembly syntax. It also extends the delimiter set in
> strpbrk() to handle bracketed memory operands and removes the static
> qualifier from arch__dwarf_regnum().
> 
>> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
>> index df70e95a84704..9d8b4d6b859b8 100644
>> --- a/tools/perf/util/annotate.c
>> +++ b/tools/perf/util/annotate.c
> 
> [ ... ]
> 
>> @@ -2472,21 +2472,23 @@ int annotate_check_args(void)
>>  	return 0;
>>  }
>>  
>> -static int arch__dwarf_regnum(const struct arch *arch, const char *str)
>> +int arch__dwarf_regnum(const struct arch *arch, const char *str)
> 
> [Severity: Medium]
> Does this change fully enable arm64 register parsing?
> 
> Looking at the callers of arch__dwarf_regnum(), they appear to still
> unconditionally enforce the presence of a register prefix character.
> 
> For example, extract_reg_offset() checks:
> 
>     if (arch->objdump.register_char == 0)
>         return -1;
> 
> And annotate_get_insn_location() strictly requires a match:
> 
>     if (*s == arch->objdump.register_char) {
>         op_loc->reg1 = arch__dwarf_regnum(arch, s);
>     }
> 
> Will these un-updated callers prevent arm64 registers from actually being
> parsed?
> 

This seems to be a patch ordering problem. I performed arm64 adaptation
for the above functions too early, before exporting the arch-specific
extract_op_location callback. As a result, sashiko flagged those call
sites which are only used by x86 logic. If I reorder the patches by
moving the current 0007 patch earlier, it should avoid these warnings:

Reorder: 0007 => 0005 => 0006 => 0008
Where:
  0005 perf annotate: Adapt arch__dwarf_regnum() for arm64
  0006 perf dwarf-regs: Adapt get_dwarf_regnum() for arm64
  0007 perf annotate: Introduce extract_op_location callback for arch-specific parsing
  0008 perf annotate-arm64: Implement extract_op_location() callback

>>  {
>> -	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, ",)] ");
> 
> [Severity: Medium]
> Since the prefix check is now optional, will p point to the opening bracket
> for arm64 memory operands like [x2, #8]?
> 
> This strpbrk() strips the closing bracket ], but is there logic to strip the
> leading [ before the string is passed to get_dwarf_regnum()? Can this pass
> [x2 as the register name and cause it to be rejected?
> 
>>  	if (q)
>>  		*q = '\0';
>>
> 

I don't consider this an issue. Even if '[x2' is mistakenly passed as
the register name, get_dwarf_regnum() will reject the invalid register
string, which prevents the problem from spreading further.