Re: [PATCH v4 07/23] perf annotate: Introduce extract_op_location callback for arch-specific parsing

On 2026/8/8 21:11, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] Propagating errors from extract_reg_offset aborts instruction state processing, breaking register invalidation, and uses a flawed condition.
> --
> 
> --- Patch [7]: [PATCH v4 07/23] perf annotate: Introduce extract_op_location callback for arch-specific parsing ---
> commit ea4cc06c47d3d877b7277db8368241769c5b60d3
> Author: Tengda Wu <[email protected]>
> 
> perf annotate: Introduce extract_op_location callback for arch-specific parsing
> 
> Assembly syntax for operands varies significantly across different
> architectures, which prevents the operand location (op_loc) parsing
> logic in annotate_get_insn_location() from being directly reused.
> 
> To simplify the core logic and improve maintainability, move the
> operand parsing inside the for_each_insn_op_loc loop into arch-specific
> extract_op_location callbacks.
> 
>> This refactoring is primarily a cleanup,
>> but includes a minor x86-specific change: errors from extract_reg_offset()
>> are now propagated, so unrecognized operands are correctly rejected
>> instead of being silently ignored, , and testing confirms no functional
>> regression from this change.
> 
> [Severity: Medium]
> Are we certain this change does not introduce functional regressions?
> 
> Propagating errors out of extract_reg_offset() appears to break type state
> tracking for absolute memory references.
> 

This has been validated through local testing, and all statistical metrics
remain unchanged.
I noticed there are two commas in the commit message and will correct that.


>> diff --git a/tools/perf/util/annotate-arch/annotate-x86.c b/tools/perf/util/annotate-arch/annotate-x86.c
>> index 7e61365363938..6044a68966f14 100644
>> --- a/tools/perf/util/annotate-arch/annotate-x86.c
>> +++ b/tools/perf/util/annotate-arch/annotate-x86.c
> [ ... ]
>> @@ -809,6 +811,85 @@ static void update_insn_state_x86(struct type_state *state,
> [ ... ]
>> +static int extract_reg_offset(const struct arch *arch, const char *str,
>> +			      struct annotated_op_loc *op_loc)
>> +{
> [ ... ]
>> +	op_loc->reg1 = arch__dwarf_regnum(arch, p);
>> +	if (op_loc->reg1 == -1)
>> +		return -1;
> 
> [Severity: Medium]
> Will this error check correctly identify unrecognized registers?
> 
> If an instruction contains an unrecognized register like '%zmm0',
> arch__dwarf_regnum() returns -2 (-ENOENT). The check for -1 will miss
> unrecognized registers, but it will trigger for absolute memory references
> like 'mov (0x1234), %rax' which lack a '%' character entirely.
> 

Shuai raised this point as well. The fix will be to normalize
arch__dwarf_regnum() to return -1.

>> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
>> index 9d8b4d6b859b8..20be609ebed24 100644
>> --- a/tools/perf/util/annotate.c
>> +++ b/tools/perf/util/annotate.c
> [ ... ]
>> @@ -2572,6 +2527,7 @@ int annotate_get_insn_location(const struct arch *arch, struct disasm_line *dl,
> [ ... ]
>> +		if (arch->extract_op_location) {
>> +			ret = arch->extract_op_location(arch, dl, insn_str, i, op_loc);
>> +			if (ret)
>> +				return ret;
>>  		}
>>  	}
> 
> [Severity: Medium]
> Does propagating this failure cause issues in update_insn_state_x86()?
> 
> When absolute memory references return -1 and trigger this early return,
> annotate_get_insn_location() will fail.
> 
> If update_insn_state_x86() bails out early because of this failure:
> 
> tools/perf/util/annotate-arch/annotate-x86.c:update_insn_state_x86() {
>     ...
> 	if (annotate_get_insn_location(dloc->arch, dl, &loc) < 0)
> 		return;
>     ...
> }
> 
> Won't this bypass the fallback logic that invalidates the destination register?
> This could cause the analyzer to incorrectly assume the destination register
> retains its previous type state, when it actually contains unknown data.
> 

Indeed, the return value should not be handled directly; instead, the op_loc
information should be resolved as completely as possible.
This was also the approach before the change, and I will keep it consistent
by removing the ret value check.