Re: [PATCH v3 14/21] perf annotate-arm64: Support load instruction tracking

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Missing invalidation of the destination register on type resolution failure during load instruction tracking.
- [Medium] The data type profiling logic ignores the second destination register of load pair (ldp) instructions, leading to stale state retention.
--

commit 579f01582f34ef50d993c4cb489f025353f8dbcc
Author: Tengda Wu <wutengda@huaweicloud.com>

perf annotate-arm64: Support load instruction tracking

This commit extends update_insn_state_arm64() to handle LDR instructions.
It tracks register state changes when data is loaded from memory to registers,
supporting signed offset, pre-index, and post-index addressing modes.

> diff --git a/tools/perf/util/annotate-arch/annotate-arm64.c b/tools/perf/util/annotate-arch/annotate-arm64.c
> index 48b1c94273eb6..b48545f35620c 100644
> --- a/tools/perf/util/annotate-arch/annotate-arm64.c
> +++ b/tools/perf/util/annotate-arch/annotate-arm64.c

[ ... ]

> +static void update_load_insn_state(struct type_state *state,
> +				   struct disasm_line *dl,
> +				   struct annotated_op_loc *src,
> +				   struct annotated_op_loc *dst)
> +{
> +	struct type_state_reg *tsr;
> +	struct type_state_reg src_tsr;
> +	Dwarf_Die type_die;
> +	u32 insn_offset = dl->al.offset;
> +	int reg_offset;
> +	int sreg = src->reg1;
> +	int dreg = dst->reg1;

[Severity: Medium]
Does this ignore the second destination register for load pair (ldp)
instructions?

If the instruction writes to two destination registers (e.g.,
ldp x0, x1, [x2]), it seems dst->reg2 is not checked or invalidated here,
which could leave it with stale type information. Does the parsing logic in
arm64_ldst__parse() also need to set ops->target.multi_regs for this to be
properly tracked?

> +
> +	if (!has_reg_type(state, dreg))
> +		goto out_adjust;
> +
> +	tsr = &state->regs[dreg];
> +	tsr->copied_from = -1;
> +
> +retry:
> +	if (!has_reg_type(state, sreg) || !state->regs[sreg].ok) {
> +		invalidate_reg_state(tsr);
> +		return;
> +	}
> +
> +	src_tsr = state->regs[sreg];
> +	reg_offset = get_reg_index_offset(src);
> +
> +	/* Dereference the pointer if it has one */
> +	if (src_tsr.kind == TSR_KIND_TYPE &&
> +	    die_deref_ptr_type(&src_tsr.type,
> +			       src_tsr.offset + reg_offset, &type_die)) {
> +		tsr->type = type_die;
> +		tsr->kind = TSR_KIND_TYPE;
> +		tsr->offset = 0;
> +		tsr->ok = true;
> +
> +		if (src->multi_regs) {
> +			pr_debug_dtp("ldr [%x] %#x(reg%d, reg%d) -> reg%d",
> +				     insn_offset, reg_offset, src->reg1,
> +				     src->reg2, dreg);
> +		} else {
> +			pr_debug_dtp("ldr [%x] %#x(reg%d) -> reg%d",
> +				     insn_offset, reg_offset, sreg, dreg);
> +		}
> +		pr_debug_type_name(&tsr->type, tsr->kind);
> +	}
> +	/* Or try another register if any */
> +	else if (src->multi_regs && src->reg1 != src->reg2 && sreg != src->reg2) {
> +		sreg = src->reg2;
> +		goto retry;
> +	}

[Severity: Medium]
If the type resolution fails and execution bypasses both of these if/else
blocks, it falls through to out_adjust.

Should the destination register be invalidated here?

It looks like tsr->ok could remain true from its previous state, leaving it
with incorrect type information.

> +
> +out_adjust:
> +	adjust_reg_index_state(state, src, "ldr", insn_offset);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260701035355.752944-1-wutengda@huaweicloud.com?part=14