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

[Tengda Wu]

On 2026/8/11 14:33, Shuai Xue wrote:
> 
> 
> On 8/8/26 8:23 PM, Tengda Wu wrote:
>> Currently, arch__dwarf_regnum() assumes that all architectures use a
>> register prefix character (e.g., '%' for x86) defined by
>> arch->objdump.register_char, and uses it to match register names in
>> objdump output. However, this assumption does not hold for arm64,
>> where assembly syntax uses bare register names like 'x0', 'w1'
>> without any prefix.
>>
>> As a result, arm64 builds may fail to correctly recognize register
>> names from objdump disassembly, leading to incomplete or incorrect
>> annotation output.
>>
>> To address this:
>>
>> - Make the register prefix check optional, allowing architectures
>>    without a prefix character to be parsed correctly.
>>
>> - Extend the delimiter set in strpbrk() to include the closing square
>>    bracket ']'. In arm64 assembly, memory operands often use bracketed
>>    syntax such as '[x1, #16]' or '[x2]'. Adding ']' ensures clean
>>    extraction of register names like 'x2' without trailing characters.
>>
>> - Remove the 'static' qualifier from arch__dwarf_regnum() so that it
>>    can be reused by other architecture-specific profiling components
>>    in future changes.
>>
>> Signed-off-by: Tengda Wu <wutengda@xxxxxxxxxxxxxxx>
>> ---
>>   tools/perf/util/annotate.c | 14 ++++++++------
>>   tools/perf/util/annotate.h |  2 ++
>>   2 files changed, 10 insertions(+), 6 deletions(-)
>>
>> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
>> index df70e95a8470..9d8b4d6b859b 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)
>>   {
>> -    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;
> 
> 
> Keeping the early -1 return here is correct, but it exposes an
> inconsistency in this function's failure values. This is the only path
> that returns -1; when the lookup itself fails the tail returns whatever
> get_dwarf_regnum() produced (-ENOENT on arm64, -EINVAL/-ENOENT on x86).
> The callers only check
> 
>     if (op_loc->reg1 == -1)
> 
> so the early return is caught while a real parse failure slips through
> with a negative reg1. Today that is harmless because every consumer
> guards with has_reg_type(), whose unsigned compare rejects negative
> values, but the error propagation is effectively broken and arm64 adds
> more inputs that fail parsing (prfm ops, PC-relative addresses).
> 
> Could you normalise the tail to keep the "success or -1" contract?
> 
>     reg = get_dwarf_regnum(regname, arch->id.e_machine,
>                    arch->id.e_flags);
>     free(regname);
>     return reg < 0 ? -1 : reg;
> 
> Thanks.
> Shuai

Sure. I'm also thinking about whether this change should be done as a
standalone patch, since it appears to be a pre-existing issue.

Thanks,
Tengda