Re: [PATCH v3 18/21] perf annotate-arm64: Support 'add' instruction tracking

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Missing register invalidation when source registers are known non-pointer types, causing the destination register to incorrectly retain stale type information.
--

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

perf annotate-arm64: Support 'add' instruction tracking

Extend update_insn_state() for arm64 to track 'add' instructions for
structure member address calculation, which commonly appear as:

  add  dreg, base, #offset
  add  dreg, base, reg2     (reg2 holds a constant)

For such instructions, determine the final offset value from either the
immediate or the constant in reg2.

> diff --git a/tools/perf/util/annotate-arch/annotate-arm64.c b/tools/perf/util/annotate-arch/annotate-arm64.c
> index be07a85057c8c..1fed188117192 100644
> --- 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 calculation of a register holding a typed pointer */
> +	if (src_tsr.kind == TSR_KIND_POINTER ||
> +	    (src_tsr.kind == TSR_KIND_TYPE &&
> +	     dwarf_tag(&src_tsr.type) == DW_TAG_pointer_type)) {
[ ... ]
> +		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]
When the source registers are not tracked pointers or valid types, the code
falls through the if/else if chain and returns implicitly here without calling
invalidate_reg_state(tsr).

Since update_insn_state_arm64() specifically exempts "add" instructions from
generic register invalidation (as seen below), won't dreg incorrectly retain
its previous stale type information?

[ ... ]
> @@ -589,7 +673,7 @@ static void update_insn_state_arm64(struct type_state *state,
>  	 * the destination register itself to prevent incorrect type propagation.
>  	 */
>  	if (has_reg_type(state, dst->reg1) &&
> -	    strcmp(dl->ins.name, "mov") &&
> +	    strcmp(dl->ins.name, "add") && strcmp(dl->ins.name, "mov") &&
>  	    strncmp(dl->ins.name, "ld", 2) && strncmp(dl->ins.name, "st", 2)) {
>  		pr_debug_dtp("%s [%x] invalidate reg%d\n",
>  			     dl->ins.name, insn_offset, dst->reg1);

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