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

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.

> 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.

> 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.

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