Re: [PATCH v4 15/23] perf annotate-arm64: Support store instruction tracking

On 2026/8/8 21:11, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] For post-index addressing modes using a register offset (e.g., `ldr x0, [x1], x2` or `str x1, [x0], x2`), the base register's type state is incorrectly preserved instead of being invalidated.
> --
> 
> --- Patch [15]: [PATCH v4 15/23] perf annotate-arm64: Support store instruction tracking ---
> commit 5069bd6d8cc957f98ba504ae076e8ee35699d61a
> Author: Tengda Wu <[email protected]>
> 
> perf annotate-arm64: Support store instruction tracking
> 
> This commit extends the arm64 update_insn_state() function to handle
> store (STR) instructions. It ignores the transfer itself since it won't
> change the type, but calls adjust_reg_index_state() to handle addressing
> modes for the destination register.
> 
>> diff --git a/tools/perf/util/annotate-arch/annotate-arm64.c b/tools/perf/util/annotate-arch/annotate-arm64.c
>> --- a/tools/perf/util/annotate-arch/annotate-arm64.c
>> +++ b/tools/perf/util/annotate-arch/annotate-arm64.c
> 
> [ ... ]
> 
>> @@ -575,6 +575,17 @@ static void update_insn_state_arm64(struct type_state *state,
>>  	/* Memory to register transfers */
>>  	if (!strncmp(dl->ins.name, "ld", 2))
>>  		update_load_insn_state(state, dl, src, dst);
>> +	/* Register to memory transfers */
>> +	else if (!strncmp(dl->ins.name, "st", 2)) {
>> +		/*
>> +		 * Ignore transfers since it'd set a value in a struct
>> +		 * and won't change the type.
>> +		 *
>> +		 * Needs to update the pre-index and post-index addressing
>> +		 * modes for the destination register.
>> +		 */
>> +		adjust_reg_index_state(state, dst, "str", insn_offset);
> 
> [Severity: Medium]
> When using post-index addressing modes with a register offset (e.g., 
> "str x1, [x0], x2"), does adjust_reg_index_state() correctly invalidate 
> the base register's state?
> 
> Looking at adjust_reg_index_state() in the same file, when op_loc->multi_regs 
> is true (indicating a dynamic register offset), op_loc->offset is 0:
> 
> tools/perf/util/annotate-arch/annotate-arm64.c:adjust_reg_index_state() {
>     ...
> 	tsr->offset = op_loc->offset + tsr->offset;
>     ...
> }
> 
> Since op_loc->offset is 0 in this case, this seems to retain the old static
> offset instead of clearing the state via invalidate_reg_state().
> 
> Can this cause incorrect struct field attribution in the perf annotate 
> --data-type output for subsequent accesses using the modified base register?
> 

Agreed.

>> +	}
>>  }
>>  #endif
>