Re: [PATCH v5 16/26] 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] Unintended rollback of valid type state for the first destination register in a multi-register load (e.g., ldp) if the second register's type cannot be resolved.
- [Medium] Missing support for `TSR_KIND_POINTER` in `propagate_load_reg_state` causes type tracking to silently fail for variables whose addresses are held in registers.
--

commit 6913b761b417f151400c62773b3dd0ee4dcddbf2
Author: Tengda Wu <wutengda@huaweicloud.com>

perf annotate-arm64: Support load instruction tracking

Extend update_insn_state_arm64() to handle load instructions, tracking
register state changes when data is loaded from memory to registers. Introduces
propagate_load_reg_state() and update_load_insn_state() to handle standard
loads and load pairs.

> diff --git a/tools/perf/util/annotate-arch/annotate-arm64.c b/tools/perf/util/annotate-arch/annotate-arm64.c
> index 9a32265265223..c239ad9473dcc 100644
> --- a/tools/perf/util/annotate-arch/annotate-arm64.c
> +++ b/tools/perf/util/annotate-arch/annotate-arm64.c

[ ... ]

> @@ -455,11 +455,206 @@ static bool is_readonly_branch_or_cmp(const char *name)

[ ... ]

> +static int propagate_load_reg_state(struct type_state *state,
> +				    struct data_loc_info *dloc,
> +				    struct disasm_line *dl, int dreg,
> +				    struct annotated_op_loc *src,
> +				    struct type_state_reg **src_states,
> +				    int mem_offset)
> +{

[ ... ]

> +	if (!src_tsr || !src_tsr->ok)
> +		return -1;
> +
> +	/* 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)) {

[Severity: Medium]
Will this silently fail to propagate types for pointers since it restricts
dereferencing strictly to TSR_KIND_TYPE?

If the source register state is TSR_KIND_POINTER (e.g., from an earlier address
calculation), it appears the pointer dereference will fail type propagation
here, reducing data type profiling coverage.

> +		tsr->type = type_die;
> +		tsr->kind = TSR_KIND_TYPE;
> +		tsr->offset = 0;
> +		tsr->ok = true;

[ ... ]

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

[ ... ]

> +	/* Handle the first destination register */
> +	if (propagate_load_reg_state(state, dloc, dl, dst->reg1,
> +				     src, src_states, /*mem_offset=*/0))
> +		goto out_err_adjust;
> +
> +	/* Handle the second destination register (if any) */
> +	if (dst->multi_regs) {
> +		int mem_offset;
> +
> +		/*
> +		 * ldpsw loads two 32-bit signed words into 64-bit registers.
> +		 * Memory spacing between elements is 4 bytes, not the register size.
> +		 */
> +		if (!strcmp(dl->ins.name, "ldpsw"))
> +			mem_offset = 4;
> +		else
> +			mem_offset = arm64__reg_size(dl->ops.target.raw);
> +
> +		if (mem_offset < 0 ||
> +		    propagate_load_reg_state(state, dloc, dl, dst->reg2,
> +					     src, src_states, mem_offset))
> +			goto out_err_adjust;
> +	}
> +
> +out_adjust:
> +	adjust_reg_index_state(state, dloc, dl, src);
> +	return;
> +
> +out_err_adjust:
> +	if (has_reg_type(state, dst->reg1))
> +		invalidate_reg_state(&state->regs[dst->reg1]);
> +	if (dst->multi_regs && has_reg_type(state, dst->reg2))
> +		invalidate_reg_state(&state->regs[dst->reg2]);
> +	goto out_adjust;
> +}

[Severity: Medium]
Does this unintentionally discard valid type state for the first destination
register if the second destination register's type cannot be resolved?

If an arm64 ldp instruction is analyzed where the first member is trackable,
but the second member cannot be resolved, this error path seems to
unconditionally invalidate the successfully resolved state of dst->reg1.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1788872630.git.wutengda@huaweicloud.com?part=16