Re: [PATCH v4 20/23] perf annotate-arm64: Support 'add' instruction tracking

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing state invalidation when adding a non-constant dynamic variable to a pointer causes the type tracker to silently assume an offset of 0.
- [High] Incorrect offset calculation for scaled ADD instructions due to ignored shift and extend modifiers.
- [Medium] Pointer type tracking is dropped for commutative ADD instructions when the first operand is a constant, due to a premature early return.
--

--- Patch [20]: [PATCH v4 20/23] perf annotate-arm64: Support 'add' instruction tracking ---
commit 8c78c99c0185c9b7a385f10b087f345530162939
Author: Tengda Wu <[email protected]>

perf annotate-arm64: Support 'add' instruction tracking

Extend update_insn_state() for arm64 to track 'add' instructions for
structure member address calculation.

diff --git a/tools/perf/util/annotate-arch/annotate-arm64.c b/tools/perf/util/annotate-arch/annotate-arm64.c
--- a/tools/perf/util/annotate-arch/annotate-arm64.c
+++ b/tools/perf/util/annotate-arch/annotate-arm64.c

[ ... ]

> +static void update_add_insn_state(struct type_state *state,
> +				  struct disasm_line *dl,
> +				  struct annotated_op_loc *src,
> +				  struct annotated_op_loc *dst)
> +{

[ ... ]

> +	/*
> +	 * Handle 'add' instructions of the form:
> +	 *   add  dreg, base, #offset     (immediate offset)
> +	 *   add  dreg, base, reg2        (reg2 holds a constant)
> +	 *
> +	 * For case 2, retrieve the constant value from reg2
> +	 * and use it as the offset.
> +	 */
> +	imm_value = src->offset;
> +	if (src->multi_regs) {
> +		int reg2 = (sreg == src->reg1) ? src->reg2 : src->reg1;
> +
> +		if (!has_reg_type(state, reg2) || !state->regs[reg2].ok) {
> +			/* Unable to resolve type for dst, bail out */
> +			invalidate_reg_state(tsr);
> +			return;
> +		}
> +		if (state->regs[reg2].kind == TSR_KIND_CONST)
> +			imm_value = state->regs[reg2].imm_value;
> +	}

[Severity: High]
If reg2 is dynamic (not TSR_KIND_CONST), does this code fail to invalidate
the destination tracking state?

It looks like imm_value will silently remain set to its previous value 
(src->offset), which could cause the type tracker to assume an incorrect
offset of 0 instead of properly invalidating the tracking state.

[Severity: High]
Does this offset calculation ignore shift and extend modifiers for scaled
ADD instructions?

For example, if the instruction is "add x0, x1, x2, LSL #3", it appears we
will add the unscaled raw imm_value of reg2 to the struct offset, mapping
subsequent memory accesses to the wrong struct field or array index.

> +
> +	if (src_tsr.kind == TSR_KIND_CONST) {
> +		tsr->kind = src_tsr.kind;
> +		tsr->imm_value = src_tsr.imm_value + imm_value;
> +		tsr->offset = 0;
> +		tsr->ok = src_tsr.ok;
> +
> +		pr_debug_dtp("add [%x] imm %#"PRIx64"(reg%d) -> reg%d\n",
> +			     insn_offset, imm_value, sreg, dreg);
> +		return;
> +	}

[Severity: Medium]
For commutative ADD instructions where the first operand is a constant,
does this early return drop the pointer type tracking?

If the instruction is "add rd, const_reg, ptr_reg", it looks like sreg will
evaluate to TSR_KIND_CONST and trigger this return before the pointer
register (ptr_reg) can be evaluated via the "goto retry" logic below.

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