Re: [PATCH v3 13/21] perf annotate-arm64: Enable instruction tracking support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Missing function call register invalidation for ARM64 data type tracking.
- [Medium] Failure to invalidate secondary destination registers (`reg2`) in multi-register instructions.

Pre-existing issues:
- [High] Missing `multi_regs` evaluation for register operands in load/store instructions.
--

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

perf annotate-arm64: Enable instruction tracking support

Enable basic instruction tracking for arm64 by implementing three essential
functions in the find_data_type_block() call path: arch_supports_insn_tracking,
init_type_state, and update_insn_state.

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

[ ... ]

> @@ -324,6 +326,31 @@ static int extract_op_location_arm64(const struct arch *arch,
>  	return 0;
>  }
>  
> +#ifdef HAVE_LIBDW_SUPPORT
> +static void update_insn_state_arm64(struct type_state *state,
> +				    struct data_loc_info *dloc, Dwarf_Die *cu_die __maybe_unused,
> +				    struct disasm_line *dl)
> +{
> +	struct annotated_insn_loc loc;
> +	struct annotated_op_loc *dst = &loc.ops[INSN_OP_TARGET];
> +	u32 insn_offset = dl->al.offset;
> +
> +	if (annotate_get_insn_location(dloc->arch, dl, &loc) < 0)
> +		return;
> +

[Severity: High]
Does this function miss a check for function calls?

When an instruction is a function call (e.g., bl or blr), this code does not
check ins__is_call(&dl->ins). As a result, it fails to invalidate the
caller-saved registers (x0-x17) and update the return type in x0.

While init_type_state() correctly initializes the caller_saved flags for
these registers, won't omitting the invalidation here cause subsequent
accesses to inherit incorrect type states across function calls?

> +	/*
> +	 * For unsupported instructions with a destination register, invalidate
> +	 * the destination register itself to prevent incorrect type propagation.
> +	 */
> +	if (has_reg_type(state, dst->reg1)) {
> +		pr_debug_dtp("%s [%x] invalidate reg%d\n",
> +			     dl->ins.name, insn_offset, dst->reg1);
> +		invalidate_reg_state(&state->regs[dst->reg1]);
> +		return;
> +	}

[Severity: Medium]
Does this fallback logic need to handle secondary destination registers?

When an unsupported instruction modifies multiple registers (e.g., casp),
this only checks and invalidates the primary destination register
(dst->reg1). 

If the instruction has a secondary register (where dst->multi_regs is
true), should we also invalidate dst->reg2 so it doesn't retain an
incorrect type state?

[Severity: High]
This is a pre-existing issue, but I noticed a potential problem in
arm64_ldst__parse() regarding multi-register instructions:

tools/perf/util/annotate-arch/annotate-arm64.c:arm64_ldst__parse() {
...
	if (arm64__insn_is_source_first(dl)) {
		ops->source.raw = left;
		ops->source.mem_ref = false;
		ops->target.raw = right;
		ops->target.mem_ref = true;
		ops->target.multi_regs = arm64__check_multi_regs(arch, ops->target.raw);
	}
...
}

Here, arm64__check_multi_regs() is only applied to the memory operand side
(ops->target in this case). The register operand side is left with
multi_regs = false. 

Because of this, extract_op_location_arm64() fails to parse the second
register (reg2) for instructions like ldp and stp, leaving it as -1. Won't
this cause the data type tracking logic to silently miss type updates and
queries for the secondary register?

> +}
> +#endif

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