Re: [PATCH v5 13/26] perf annotate-arm64: Enable instruction tracking support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Incomplete initialization of ARM64 caller-saved registers in init_type_state() misses x30 (Link Register) and x18. According to the ARM64 AAPCS64 ABI, x30 is intrinsically caller-saved (it is clobbered by function calls to store the return address), and x18 is also a temporary/caller-saved register (unless strictly reserved by a platform ABI). Because these registers are omitted during initialization, their `caller_saved` flags remain `false`. Consequently, their data type states are not invalidated across function calls, allowing stale type info to erroneously propagate.
--

commit 7f29650747bf4bd656328dd491ef0f827ab60d8c
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. This
allows arm64 to support basic variable type inference during
instruction tracking.

> diff --git a/tools/perf/util/annotate-arch/annotate-arm64.c b/tools/perf/util/annotate-arch/annotate-arm64.c
> index d8a5904359b1a..8fda4d4d68887 100644
> --- a/tools/perf/util/annotate-arch/annotate-arm64.c
> +++ b/tools/perf/util/annotate-arch/annotate-arm64.c
[ ... ]
> @@ -442,6 +446,77 @@ static void extract_op_location_arm64(const struct arch *arch,
[ ... ]
> +	if (ins__is_call(&dl->ins)) {
> +		struct symbol *func = dl->ops.target.sym;
> +		const char *call_name;
> +		u64 call_addr;
> +
> +		call_name = func ? func->name : dl->ops.target.name;
> +		pr_debug_dtp("call [%x] %s\n", insn_offset, call_name ?: "<unknown>");
> +
> +		/* Invalidate caller-saved registers after call */
> +		call_addr = map__rip_2objdump(dloc->ms->map,
> +					      dloc->ms->sym->start + dl->al.offset);
> +		for (unsigned int i = 0; i < ARRAY_SIZE(state->regs); i++) {
> +			struct type_state_reg *reg = &state->regs[i];
> +
> +			if (!reg->caller_saved)
> +				continue;

[Severity: Medium]
Because x30 and x18 are omitted from being marked as caller-saved during type
state initialization, their caller_saved flags will be false.

Are these registers unintentionally skipped here, preventing them from being
invalidated across function calls?

> +			/* Keep register valid within DWARF location lifetime */
> +			if (reg->lifetime_active && call_addr < reg->lifetime_end)
> +				continue;
> +			invalidate_reg_state(reg);
> +		}
> +		return;
> +	}
[ ... ]
> diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
> index 104b80d471f1b..c51b20a7af9b6 100644
> --- a/tools/perf/util/annotate-data.c
> +++ b/tools/perf/util/annotate-data.c
[ ... ]
> @@ -178,6 +179,13 @@ static void init_type_state(struct type_state *state, const struct arch *arch)
>  		state->regs[11].caller_saved = true;
>  		state->ret_reg = 0;
>  		state->stack_reg = X86_REG_SP;
> +	} else if (arch__is_arm64(arch)) {
> +		int i;
> +
> +		for (i = 0; i < 18; i++)
> +			state->regs[i].caller_saved = true;

[Severity: Medium]
According to the ARM64 AAPCS64 ABI, x30 (Link Register) is intrinsically
caller-saved since it is clobbered by function calls to store the return
address.

x18 is also typically a temporary/caller-saved register unless strictly reserved
by a platform ABI.

This loop halts at 17, failing to set the caller_saved flag for x18 and x30.

Could this omission allow stale data type information for these registers to
erroneously propagate across function calls?

> +		state->ret_reg = 0;
> +		state->stack_reg = ARM64_REG_SP;
>  	}
>  }

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