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

[Namhyung Kim]

On Tue, Aug 11, 2026 at 04:16:59PM +0800, Tengda Wu wrote:
> 
> 
> 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.

Right, please send it as a separate patch.

Thanks,
Namhyung