Re: [PATCH v5 14/26] perf annotate-data: Add arch_get_reg_offset helper

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The newly computed effective `offset` from `arch_get_reg_offset` is not propagated to `dloc->type_offset` before querying stack variables, breaking stack type tracking for indexed addressing modes on ARM64.
- [Low] The debug logging variable `offset_sign` is not reset across loop iterations, causing positive offsets to be printed as negative.
--

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

perf annotate-data: Add arch_get_reg_offset helper

Adds a helper to compute the effective offset from a base register
for instruction operands. Currently handles arm64 addressing modes
and falls back to the instruction's raw offset for other architectures.

> diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
> index c51b20a7af9b6..754c0efe2077e 100644
> --- a/tools/perf/util/annotate-data.c
> +++ b/tools/perf/util/annotate-data.c
[ ... ]
> @@ -1132,18 +1200,26 @@ static enum type_match_result check_matching_type(struct type_state *state,
>  	Dwarf_Word size;
>  	u32 insn_offset = dl->al.offset;
>  	int reg = dloc->op->reg1;
> -	int offset = dloc->op->offset;
> +	int offset;
>  	const char *offset_sign = "";
>  	bool retry = true;
>  
> -	if (offset < 0) {
> -		offset = -offset;
> -		offset_sign = "-";
> +again:
> +	if (arch_get_reg_offset(dloc->arch, dloc->op, reg, state, false, &offset)) {
> +		/*
> +		 * Fall back to the instruction's offset. This prevents the
> +		 * register type of the instruction from being completely lost.
> +		 * Compared to dropping this type inference entirely, providing
> +		 * a rough type hint is likely more useful.
> +		 */
> +		offset = dloc->op->offset;
>  	}
>  
> -again:
> +	if (offset < 0)
> +		offset_sign = "-";

[Severity: Low]
Does this code leave offset_sign set to "-" across loop iterations?

If check_matching_type() triggers a retry (goto again) after processing a
negative offset, and the new offset for the retry is positive, offset_sign
will still point to "-" instead of being cleared.

[Severity: Medium]
Since this code now dynamically computes offset using arch_get_reg_offset(),
is it necessary to update dloc->type_offset with this newly computed value?

Later in this function, during stack variable lookups, the code relies on
dloc->type_offset:

tools/perf/util/annotate-data.c:check_matching_type() {
    ...
    check_non_register:
        if (reg == dloc->fbreg || reg == state->stack_reg) {
            ...
            stack = find_stack_state(state, dloc->type_offset);
    ...
}

And similarly for CFA stack lookups:

tools/perf/util/annotate-data.c:check_matching_type() {
    ...
        stack = find_stack_state(state, dloc->type_offset - fboff);
    ...
}

If dloc->type_offset isn't updated to reflect the true effective offset
calculated by arch_get_reg_offset(), will find_stack_state() end up
using a stale or incorrect offset (e.g. 0 instead of the index offset) for
stack-based array accesses on ARM64?

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