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

[Tengda Wu <wutengda@huaweicloud.com>]

On 2026/9/8 21:22, sashiko-bot@kernel.org wrote:
> 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.
> 

Yes, it should be changed to:

  offset_sign = offset < 0 ? "-" : "";

> [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?
> 

Yes, dloc->type_offset should be replaced with the currently computed
offset when calling find_stack_state.

Thanks,
Tengda