Re: [PATCH v3 05/21] perf annotate-arm64: Handle load and store instructions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] arm64_ldst__parse fails to parse PC-relative load instructions, breaking operand extraction for literal pool loads.
- [Medium] Failure to strip inline comments before parsing operands allows comments to corrupt `multi_regs` evaluation.
--

commit 9939a67ebc615cf049eb33c68f83aa3646abb3f0
Author: Tengda Wu <wutengda@huaweicloud.com>

perf annotate-arm64: Handle load and store instructions

This commit adds ldst_ops to parse data types and offsets for ARM64 memory
access instructions using regex prefix matching.

> diff --git a/tools/perf/util/annotate-arch/annotate-arm64.c b/tools/perf/util/annotate-arch/annotate-arm64.c
> index a1667237114a6..451500def8c12 100644
> --- a/tools/perf/util/annotate-arch/annotate-arm64.c
> +++ b/tools/perf/util/annotate-arch/annotate-arm64.c
[ ... ]
> @@ -141,6 +142,104 @@ static const struct ins_ops arm64_mov_ops = {
[ ... ]
> +static int arm64_ldst__parse(const struct arch *arch,
> +			     struct ins_operands *ops,
> +			     struct map_symbol *ms __maybe_unused,
> +			     struct disasm_line *dl)
> +{
> +	char *s, *left, *right;
> +
> +	right = s = strchr(ops->raw, arch->objdump.memory_ref_char);
> +	if (!s)
> +		return -1;

[Severity: Medium]
Will this fail to parse PC-relative load instructions?

When processing a PC-relative load instruction (e.g., ldr x0, 4005c0 <sym>),
the 'ld' prefix matches the new ldst_insn regex in arch__new_arm64() and
routes it to arm64_ldst__parse().

Since PC-relative loads lack a bracket ('['), strchr() will return NULL,
causing the function to immediately return -1. Does this leave operands
unpopulated and break annotation features like data type tracking for
literal pool loads?

> +
> +	while (s > ops->raw && *s != ',')
> +		--s;
> +
> +	if (s == ops->raw)
> +		return -1;
> +
> +	*s = '\0';
> +	left = strdup(ops->raw);
> +
> +	*s = ',';
> +	if (!left)
> +		return -1;
> +
> +	right = strdup(right);
> +	if (!right) {
> +		zfree(&left);
> +		return -1;
> +	}
> +
> +	if (arm64__insn_is_source_first(dl)) {
> +		ops->source.raw = left;
> +		ops->source.mem_ref = false;
> +
> +		ops->target.raw = right;
> +		ops->target.mem_ref = true;
> +		ops->target.multi_regs = arm64__check_multi_regs(arch, ops->target.raw);

[Severity: Medium]
Does this code incorrectly include inline comments when evaluating multi_regs?

The string duplicated into 'right' includes trailing comments from the raw
operand string because they are not stripped prior to parsing.

If objdump outputs an inline comment containing a comma and a string
resembling a register name (for example: // read, x2), could
arm64__check_multi_regs() wrongly flag the operand as having multiple
registers and corrupt data type profiling?

> +	} else {
> +		ops->source.raw = right;
> +		ops->source.mem_ref = true;
> +		ops->source.multi_regs = arm64__check_multi_regs(arch, ops->source.raw);
> +
> +		ops->target.raw = left;
> +		ops->target.mem_ref = false;
> +	}
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260701035355.752944-1-wutengda@huaweicloud.com?part=5