Re: [PATCH v6 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] perf annotate: stale lifetime metadata retained on load
- [Medium] perf annotate: incorrect type propagation for TSR_KIND_POINTER loads
--

commit 9763211f6135d1b6c3e44acf4ac681c34ac60395
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.

> diff --git a/tools/perf/util/annotate-arch/annotate-arm64.c b/tools/perf/util/annotate-arch/annotate-arm64.c
> index d3d994a22cf92..11b2bdceaacb1 100644
> --- a/tools/perf/util/annotate-arch/annotate-arm64.c
> +++ b/tools/perf/util/annotate-arch/annotate-arm64.c

[ ... ]

> @@ -461,11 +461,210 @@ 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_spacing)
> +{
> +	struct type_state_reg *tsr;
> +	struct type_state_reg *src_tsr = src_states[0];
> +	Dwarf_Die type_die;
> +	u32 insn_offset = dl->al.offset;
> +	int sreg = src->reg1;
> +	int reg_offset;
> +
> +	if (!has_reg_type(state, dreg))
> +		return -1;
> +
> +	tsr = &state->regs[dreg];
> +	tsr->copied_from = -1;
> +
> +retry:
> +	if (arch_get_reg_offset(dloc->arch, src, sreg, state, false, &reg_offset))

[Severity: Medium]
When acquiring the destination register state here, shouldn't the lifetime
metadata be cleared (e.g., via tsr->lifetime_active = false;)?

If the register previously held a variable with an active DWARF lifetime, the
newly loaded value inherits this stale lifetime. During function calls,
caller-saved registers with active lifetimes are preserved rather than
invalidated. This causes the register's new type to incorrectly survive the
call, leading to stale type tracking downstream.

> +		goto load_non_regoff;
> +
> +	reg_offset += mem_spacing;

[ ... ]

> +	/* Handle dereference of TSR_KIND_POINTER registers */
> +	if (src_tsr->kind == TSR_KIND_POINTER &&
> +	    die_get_member_type(&src_tsr->type,
> +				src_tsr->offset + reg_offset, &type_die)) {
> +		tsr->type = src_tsr->type;
> +		tsr->kind = TSR_KIND_TYPE;
> +		tsr->offset = src_tsr->offset + reg_offset;
> +		tsr->imm_value = 0;
> +		tsr->ok = true;

[Severity: Medium]
Does this incorrectly assign the parent struct's type (src_tsr->type) instead
of the member's type (type_die), corrupting type tracking for the loaded
value?

If the destination register is subsequently used as a base address,
check_matching_type() will fail because it expects a TSR_KIND_TYPE register
to hold a pointer type, whereas it now holds a struct type. This breaks
downstream type inference for the loaded value.

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