Re: [PATCH v8 13/15] perf annotate-arm64: Support 'adrp' instruction to track global variables

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] perf annotate-arm64: adjust_reg_index_state() drops global addr writeback
--

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

perf annotate-arm64: Support 'adrp' instruction to track global variables

This commit extends arm64 data type profiling to track global variable
addresses calculated via page-relative adrp instructions. It introduces
TSR_KIND_GLOBAL_ADDR to store the page-aligned base address and resolves
the symbol address and DWARF type on subsequent add or load instructions.

> diff --git a/tools/perf/util/annotate-arch/annotate-arm64.c b/tools/perf/util/annotate-arch/annotate-arm64.c
> index 61df9c4f48776..bd060a823b765 100644
> --- a/tools/perf/util/annotate-arch/annotate-arm64.c
> +++ b/tools/perf/util/annotate-arch/annotate-arm64.c
[ ... ]
> @@ -722,6 +722,33 @@ static int propagate_load_reg_state(struct type_state *state,
>  		return 0;
>  	}
>  
> +	/* Or check if it's a global variable */
> +	if (src_tsr->kind == TSR_KIND_GLOBAL_ADDR) {
> +		u64 ip = dloc->ms->sym->start + dl->al.offset;
> +		u64 addr = src_tsr->imm_value + reg_offset;
> +		int offset;
> +
> +		if (!get_global_var_type(cu_die, dloc, ip, addr, &offset, &type_die) ||
> +		    !die_get_member_type(&type_die, offset, &type_die))
> +			return -1;
[ ... ]
> @@ -991,14 +1018,16 @@ static void update_add_insn_state(struct type_state *state,
>  	if (arch_get_reg_offset(dloc->arch, src, sreg, state, false, &reg_offset))
>  		goto add_non_regoff;
>  
> -	if (src_tsr.kind == TSR_KIND_CONST) {
> +	if (src_tsr.kind == TSR_KIND_CONST || src_tsr.kind == TSR_KIND_GLOBAL_ADDR) {
>  		tsr->kind = src_tsr.kind;
> +		/* For 'adrp + add' pair: resolve the full global variable address. */
>  		tsr->imm_value = src_tsr.imm_value + reg_offset;
>  		tsr->offset = 0;
>  		tsr->ok = src_tsr.ok;

[Severity: Medium]
What happens when a pre-indexed or post-indexed load or store instruction
(for example, ldr x0, [x19, #0x40]! or ldr x0, [x19], #8) uses a
TSR_KIND_GLOBAL_ADDR register as its base?

Both update_load_insn_state() and update_store_insn_state() call
adjust_reg_index_state(), which adds the writeback offset to tsr->offset:

tools/perf/util/annotate-arch/annotate-arm64.c:adjust_reg_index_state() {
	...
	if (arch_get_reg_offset(dloc->arch, op_loc, reg, state, true, &offset)) {
		invalidate_reg_state(tsr);
		return;
	}

	tsr->offset += offset;
	...
}

However, TSR_KIND_GLOBAL_ADDR stores the base address in tsr->imm_value,
and all consumers — propagate_load_reg_state(), update_add_insn_state(),
and check_matching_type() in tools/perf/util/annotate-data.c — compute the
target address from imm_value + reg_offset while ignoring tsr->offset (and
update_add_insn_state() resets tsr->offset to 0).

Should adjust_reg_index_state() update tsr->imm_value (or should the
consumers include tsr->offset) so that pre/post-index writeback updates on
global address registers are not lost on subsequent instructions?

[ ... ]

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