Re: [PATCH v4 19/23] perf annotate-arm64: Support 'mov' instruction tracking

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Conservative register invalidation incorrectly drops data types for comparison instructions like 'cmp', 'cmn', 'tst', and 'teq'.
- [Medium] Missing DWARF register lifetime propagation and clearing in ARM64 `mov` instruction tracking.
--

--- Patch [19]: [PATCH v4 19/23] perf annotate-arm64: Support 'mov' instruction tracking ---
commit 16c512fdcea449b7238a3bf5f1a3afbffde8144e
Author: Tengda Wu <[email protected]>

perf annotate-arm64: Support 'mov' instruction tracking

Extend update_insn_state() for arm64 to support register-to-register and
immediate-to-register 'mov' instructions. This propagates data types and
constant values to improve subsequent instruction inference.

> diff --git a/tools/perf/util/annotate-arch/annotate-arm64.c b/tools/perf/util/annotate-arch/annotate-arm64.c
> index 6e09e97072568..7b780bad8c070 100644
> --- a/tools/perf/util/annotate-arch/annotate-arm64.c
> +++ b/tools/perf/util/annotate-arch/annotate-arm64.c

[ ... ]

> +static void update_mov_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;
> +	u32 insn_offset = dl->al.offset;
> +	int sreg = src->reg1;
> +	int dreg = dst->reg1;
> +
> +	if (!has_reg_type(state, dreg))
> +		return;
> +
> +	tsr = &state->regs[dreg];
> +	tsr->copied_from = -1;
> +
> +	if (src->imm) {
> +		tsr->kind = TSR_KIND_CONST;
> +		tsr->imm_value = src->offset;
> +		tsr->offset = 0;
> +		tsr->ok = true;

[Severity: Medium]
Does this code need to clear lifetime_active for the destination register?

If an immediate value overwrites a register, failing to clear lifetime_active
might incorrectly protect the new constant from being invalidated across
function calls.

> +
> +		pr_debug_dtp("mov [%x] imm=%#"PRIx64" -> reg%d\n",
> +			     insn_offset, tsr->imm_value, dreg);
> +		return;
> +	}
> +
> +	if (!has_reg_type(state, sreg) || !state->regs[sreg].ok) {
> +		invalidate_reg_state(tsr);
> +		return;
> +	}
> +
> +	tsr->type = state->regs[sreg].type;
> +	tsr->kind = state->regs[sreg].kind;
> +	tsr->imm_value = state->regs[sreg].imm_value;
> +	tsr->offset = state->regs[sreg].offset;
> +	tsr->ok = state->regs[sreg].ok;

[Severity: Medium]
Does this code miss propagating the DWARF register lifetime?

If lifetime_active is not propagated from state->regs[sreg] to tsr during a
register-to-register move, a valid type state might be prematurely invalidated
across function calls.

> +
> +	if (tsr->kind == TSR_KIND_TYPE || tsr->kind == TSR_KIND_POINTER)
> +		tsr->copied_from = sreg;
> +
> +	pr_debug_dtp("mov [%x] reg%d -> reg%d", insn_offset, sreg, dreg);
> +	pr_debug_type_name(&tsr->type, tsr->kind);
> +}

[ ... ]

> @@ -703,6 +750,7 @@ static void update_insn_state_arm64(struct type_state *state,
>  	 * prevent stale type info from propagating to subsequent instructions.
>  	 */
>  	if (has_reg_type(state, dst->reg1) &&
> +	    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",
>  			     dl->ins.name, insn_offset, dst->reg1);
>  		invalidate_reg_state(&state->regs[dst->reg1]);

[Severity: Medium]
Does this conservative register invalidation incorrectly drop data types for
comparison instructions like cmp, cmn, tst, and teq?

Because comparison instructions are not explicitly exempted alongside mov,
ld, and st, their first operand (which can be parsed as a target register)
will be unconditionally invalidated. This causes data type profiling to
silently fail for subsequent uses of a register after it's checked with a
comparison instruction.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=19