Re: [PATCH v4 04/23] perf annotate-arm64: Handle load and store instructions

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] PC-relative loads can be incorrectly parsed and truncated if their associated symbol name or trailing comment contains a bracket '[' (e.g., C++ operator[]).
- [Medium] The arm64_ldst_ops struct omits a .scnprintf callback, breaking UI formatting for PC-relative loads and discarding resolved symbols.
--

--- Patch [4]: [PATCH v4 04/23] perf annotate-arm64: Handle load and store instructions ---
commit e23857fdcfc409dfdff0cd84b9fe8f31c4283050
Author: Tengda Wu <[email protected]>

perf annotate-arm64: Handle load and store instructions

Add ldst_ops to handle load and store instructions in order to parse
the data types and offsets associated with PMU events for memory access
instructions. Matches instruction prefixes to handle arm64 variants.

> diff --git a/tools/perf/util/annotate-arch/annotate-arm64.c b/tools/perf/util/annotate-arch/annotate-arm64.c
> index 5e824fd617ee5..32752a3bc756b 100644
> --- a/tools/perf/util/annotate-arch/annotate-arm64.c
> +++ b/tools/perf/util/annotate-arch/annotate-arm64.c
[ ... ]
> @@ -163,6 +164,108 @@ static const struct ins_ops arm64_mov_ops = {
>  	.scnprintf = arm64_mov__scnprintf,
>  };
>  
> +static bool arm64__insn_is_source_on_left(struct disasm_line *dl)
> +{
[ ... ]
> +static int arm64_ldst__parse(const struct arch *arch,
> +			     struct ins_operands *ops,
> +			     struct map_symbol *ms,
> +			     struct disasm_line *dl)
> +{
> +	char *s, *left, *right;
> +
> +	right = s = strchr(ops->raw, arch->objdump.memory_ref_char);
> +	if (!s) {
> +		/* Fallback to default parser for PC-relative loads. */
> +		return arm64_mov__parse(arch, ops, ms, dl);
> +	}

[Severity: Medium]
For PC-relative loads, if the associated symbol name contains a bracket '['
(such as a C++ operator[] or Objective-C method) or if there is an unstripped
comment containing a bracket, will this strchr() incorrectly match it instead
of falling back to arm64_mov__parse()?

If so, it appears the subsequent string manipulation would use this incorrect
bracket position, dropping the characters in between and potentially corrupting
the operands.

[ ... ]
> +	ops->source.multi_regs = arm64__check_multi_regs(arch, ops->source.raw);
> +	ops->target.multi_regs = arm64__check_multi_regs(arch, ops->target.raw);
> +
> +	return 0;
> +}
> +
> +static struct ins_ops arm64_ldst_ops = {
> +	.parse	   = arm64_ldst__parse,
> +};

[Severity: Medium]
Does omitting a .scnprintf callback here break UI formatting for PC-relative
loads?

Because this structure lacks .scnprintf, it will fall back to
ins__raw_scnprintf(). For PC-relative loads that successfully fall back to
arm64_mov__parse() and resolve the symbol into ops->source.name, the raw
formatter will ignore the resolved symbol and just print ops->raw. Could this
result in the UI discarding the resolved symbol and displaying raw hex
addresses instead?

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