Re: [PATCH v5 03/26] perf annotate-arm64: Generalize arm64_mov__parse to support more instructions

[Namhyung Kim <namhyung@kernel.org>]

On Tue, Sep 08, 2026 at 01:05:06PM +0000, Tengda Wu wrote:
> As the default parser for arm64 instructions, arm64_mov__parse()
> currently only supports parsing instructions with the 'addr <symbol>'
> suffix. Other instructions are not supported, causing a lack of
> source and target information for most regular instructions.
> 
> Generalize arm64_mov__parse to extend its parsing capabilities:
> 1. Parse 'ops->target.raw' and 'ops->source.raw' by default.
> 2. Utilize arm64__check_multi_regs() to identify if source and
>    target contain multiple registers.
> 3. Maintain backward compatibility for instructions containing the
>    'addr <symbol>' suffix.
> 
> Signed-off-by: Tengda Wu <wutengda@huaweicloud.com>

Reviewed-by: Namhyung Kim <namhyung@kernel.org>

Thanks,
Namhyung

> ---
>  .../perf/util/annotate-arch/annotate-arm64.c  | 166 ++++++++++++++----
>  1 file changed, 134 insertions(+), 32 deletions(-)
> 
> diff --git a/tools/perf/util/annotate-arch/annotate-arm64.c b/tools/perf/util/annotate-arch/annotate-arm64.c
> index 33080fdca125..151184210691 100644
> --- a/tools/perf/util/annotate-arch/annotate-arm64.c
> +++ b/tools/perf/util/annotate-arch/annotate-arm64.c
> @@ -3,6 +3,8 @@
>  #include <errno.h>
>  #include <stdlib.h>
>  #include <string.h>
> +#include <linux/ctype.h>
> +#include <linux/string.h>
>  #include <linux/zalloc.h>
>  #include <regex.h>
>  #include "../annotate.h"
> @@ -14,59 +16,158 @@ struct arch_arm64 {
>  	regex_t jump_insn;
>  };
>  
> -static int arm64_mov__parse(const struct arch *arch __maybe_unused,
> +static bool arm64__is_reg(const char *op)
> +{
> +	if (!op || !*op)
> +		return false;
> +
> +	/*
> +	 * General-purpose registers: x0-x30, w0-w30.
> +	 * Check for 'x' or 'w' prefix followed by a numeric index.
> +	 */
> +	if ((op[0] == 'x' || op[0] == 'w') && isdigit(op[1]))
> +		return true;
> +
> +	/* Special-purpose registers: sp, wzr, xzr. */
> +	if (!strncmp(op, "sp", 2) || !strncmp(op, "xzr", 3) ||
> +	    !strncmp(op, "wzr", 3))
> +		return true;
> +
> +	/* TODO: Support more registers. */
> +	return false;
> +}
> +
> +static bool arm64__check_multi_regs(const struct arch *arch, const char *op)
> +{
> +	const char *p = op;
> +	int reg_count = 0;
> +
> +	while (p && *p) {
> +		p = skip_spaces(p);
> +		if (*p == arch->objdump.memory_ref_char)
> +			p++;
> +
> +		if (arm64__is_reg(p))
> +			reg_count++;
> +
> +		if (reg_count >= 2)
> +			return true;
> +
> +		/* Move to next operand after comma */
> +		p = strchr(p, ',');
> +		if (p)
> +			p++;
> +	}
> +
> +	return false;
> +}
> +
> +/*
> + * Duplicate @insn, stripping the comment and trailing whitespace.
> + * Returns a newly allocated string which the caller must free(),
> + * or NULL on allocation failure or if @insn is NULL.
> + */
> +static char *rstrip_space_and_comment(const char *insn, char comment_char)
> +{
> +	const char *end, *comment;
> +	size_t len;
> +	char *result;
> +
> +	if (insn == NULL)
> +		return NULL;
> +
> +	comment = strchr(insn, comment_char);
> +	if (comment != NULL)
> +		end = comment;
> +	else
> +		end = insn + strlen(insn);
> +
> +	while (end > insn && isspace(end[-1]))
> +		--end;
> +
> +	len = end - insn;
> +	result = malloc(len + 1);
> +	if (result == NULL)
> +		return NULL;
> +
> +	memcpy(result, insn, len);
> +	result[len] = '\0';
> +
> +	return result;
> +}
> +
> +static int arm64_mov__parse(const struct arch *arch,
>  			    struct ins_operands *ops,
>  			    struct map_symbol *ms __maybe_unused,
>  			    struct disasm_line *dl __maybe_unused)
>  {
> -	char *s = strchr(ops->raw, ','), *target, *endptr;
> +	char *s = strchr(ops->raw, ','), *source, *endptr;
>  
>  	if (s == NULL)
>  		return -1;
>  
> +	/* Parse target */
>  	*s = '\0';
> -	ops->source.raw = strdup(ops->raw);
> +	ops->target.raw = strdup(ops->raw);
>  	*s = ',';
>  
> -	if (ops->source.raw == NULL)
> -		return -1;
> -
> -	target = ++s;
> -	ops->target.raw = strdup(target);
>  	if (ops->target.raw == NULL)
> -		goto out_free_source;
> +		return -1;
>  
> -	ops->target.addr = strtoull(target, &endptr, 16);
> -	if (endptr == target)
> -		goto out_free_target;
> +	ops->target.multi_regs = arm64__check_multi_regs(arch, ops->target.raw);
>  
> -	s = strchr(endptr, '<');
> -	if (s == NULL)
> -		goto out_free_target;
> -	endptr = strchr(s + 1, '>');
> -	if (endptr == NULL)
> -		goto out_free_target;
> -
> -	*endptr = '\0';
> -	*s = ' ';
> -	ops->target.name = strdup(s);
> -	*s = '<';
> -	*endptr = '>';
> -	if (ops->target.name == NULL)
> -		goto out_free_target;
> +	/* Parse source, stripping comment if present */
> +	source = skip_spaces(++s);
> +	ops->source.raw = rstrip_space_and_comment(source, arch->objdump.comment_char);
> +	if (ops->source.raw == NULL) {
> +		zfree(&ops->target.raw);
> +		return -1;
> +	}
> +
> +	ops->source.multi_regs = arm64__check_multi_regs(arch, ops->source.raw);
> +
> +	/*
> +	 * Parse 'addr <symbol>' from source (if any).
> +	 *
> +	 * A raw hex string may be a scalar immediate, not an address.
> +	 * Only validate 'source.addr' if accompanied by a '<symbol>' tag,
> +	 * otherwise reset it to 0 to avoid false positive address tracking.
> +	 */
> +	ops->source.addr = strtoull(ops->source.raw, &endptr, 16);
> +	if (endptr != ops->source.raw) {
> +		s = strchr(endptr, '<');
> +		if (s == NULL) {
> +			ops->source.addr = 0;
> +			return 0;
> +		}
> +		endptr = strrchr(s + 1, '>');
> +		if (endptr == NULL) {
> +			ops->source.addr = 0;
> +			return 0;
> +		}
> +
> +		*endptr = '\0';
> +		ops->source.name = strdup(s + 1);
> +		*endptr = '>';
> +		if (ops->source.name == NULL) {
> +			ops->source.addr = 0;
> +			return 0;
> +		}
> +	}
>  
>  	return 0;
> +}
>  
> -out_free_target:
> -	zfree(&ops->target.raw);
> -out_free_source:
> -	zfree(&ops->source.raw);
> -	return -1;
> +static int arm64_mov__scnprintf(const struct ins *ins, char *bf, size_t size,
> +				struct ins_operands *ops, int max_ins_name)
> +{
> +	return scnprintf(bf, size, "%-*s %s, %s", max_ins_name, ins->name,
> +			 ops->target.raw, ops->source.name ?: ops->source.raw);
>  }
>  
>  static const struct ins_ops arm64_mov_ops = {
>  	.parse	   = arm64_mov__parse,
> -	.scnprintf = mov__scnprintf,
> +	.scnprintf = arm64_mov__scnprintf,
>  };
>  
>  static const struct ins_ops *arm64__associate_instruction_ops(struct arch *arch, const char *name)
> @@ -103,6 +204,7 @@ const struct arch *arch__new_arm64(const struct e_machine_and_e_flags *id,
>  	arch->id = *id;
>  	arch->objdump.comment_char	  = '/';
>  	arch->objdump.skip_functions_char = '+';
> +	arch->objdump.memory_ref_char	  = '[';
>  	arch->associate_instruction_ops   = arm64__associate_instruction_ops;
>  
>  	/* bl, blr */
> -- 
> 2.34.1
>