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

On 2026/8/8 21:07, [email protected] wrote:
> 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.
> 

Indeed, it appears I need to move rstrip_space_and_comment to the beginning of
the function, processing ops->raw first.

> [ ... ]
>> +	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?
> 

Yes, this only affects the display of ldr PC-relative loads, for example:

Expected: ldr x0, <__kvm_nvhe_$d>
Current : ldr x0, ffff800080f40c68 <__kvm_nvhe_$d>

This is not a serious issue, but I will still add the .scnprintf callback
as a follow-up.