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

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 <[email protected]>
> ---
>   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