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

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?

>  {
> -	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';
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6