Re: [PATCH v4 14/23] perf annotate-arm64: Support load instruction tracking

On 2026/8/8 21:08, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
> - [Medium] `update_load_insn_state` incorrectly uses the newly loaded type as the base pointer for the second register in `ldp` instructions when the base register overlaps with the first destination register (e.g., `ldp x0, x1, [x0]`).
> - [Medium] `update_insn_state_arm64` blindly invalidates the base pointer type state for store instructions because it fails to check `!dst->mem_ref` before invalidating `dst->reg1` for non-load instructions.
> - [Medium] Incorrect memory offset calculation for the second register in `LDPSW` (Load Pair of Signed Words) instructions due to reliance on the destination register prefix for size calculation.
> - [Medium] Incomplete register recognition in `arm64__is_reg()` prevents processing or defensive invalidation of the second destination register in `ldp` instructions involving zero registers (`xzr`) or SIMD registers.
> - [Medium] `arm64_ldst__parse` incorrectly parses PC-relative memory loads if an end-of-line comment contains a bracket (`[`), improperly interpreting the comment as the memory operand.
> --
> 
> --- Patch [14]: [PATCH v4 14/23] perf annotate-arm64: Support load instruction tracking ---
> commit d3c58ddf95c5da043aecdd1f99723182e79fd318
> Author: Tengda Wu <[email protected]>
> Subject: perf annotate-arm64: Support load instruction tracking
> 
> This commit extends update_insn_state_arm64() to handle LDR and LDP
> instructions. It tracks register state changes when data is loaded from
> memory into registers by propagating the data type of the base register
> to the destination register, handling various addressing modes.
> 
>> --- a/tools/perf/util/annotate-arch/annotate-arm64.c
>> +++ b/tools/perf/util/annotate-arch/annotate-arm64.c
>> @@ -358,11 +358,152 @@ static int extract_op_location_arm64(const struct arch *arch,
>>  }
>>  
>>  #ifdef HAVE_LIBDW_SUPPORT
>> +static int arm64__reg_size(const char *reg)
>> +{
>> +	if (!reg || !*reg || !arm64__is_reg(reg))
> 
> [Severity: Medium]
> Since arm64__is_reg() does not recognize zero registers like xzr or SIMD
> registers, can this cause us to incorrectly skip processing the second
> destination register in ldp instructions?
> 
> If the first register is unrecognized (for example, in ldp xzr, x19, [sp]),
> arm64__check_multi_regs() might evaluate multi_regs to false. This would
> cause update_load_insn_state() to skip the second register entirely, leaving
> its stale type state intact.
> 
>> +		return -1;
> 

Agreed. I will add xzr/wzr register recognition to arm64__is_reg(). SIMD registers
are not supported for now.


> [ ... ]
> 
>> +static void update_load_insn_state(struct type_state *state,
>> +				   struct disasm_line *dl,
>> +				   struct annotated_op_loc *src,
>> +				   struct annotated_op_loc *dst)
>> +{
>> +	int reg_offset = get_reg_index_offset(src);
>> +	const char *insn_name = dst->multi_regs ? "ldp" : "ldr";
>> +
>> +	if (!has_reg_type(state, dst->reg1) ||
>> +	    (dst->multi_regs && !has_reg_type(state, dst->reg2)))
>> +		goto out_err_adjust;
>> +
>> +	/* Handle the first destination register */
>> +	if (propagate_load_reg_state(state, dl, dst->reg1, src,
>> +				     reg_offset, insn_name))
>> +		goto out_err_adjust;
> 
> [Severity: Medium]
> Can this inadvertently corrupt the type tracking state when the base register
> and the first destination register overlap?
> 
> If we process an instruction like ldp x0, x1, [x0], the first call to
> propagate_load_reg_state() updates state->regs[dst->reg1] (which is x0).
> 
> When the second call to propagate_load_reg_state() runs for the second
> destination register:
> 
> 	if (dst->multi_regs) {
> 		int reg_size = arm64__reg_size(dl->ops.target.raw);
> 		...
> 		propagate_load_reg_state(state, dl, dst->reg2, src,
> 
> it will read from src->reg1 (which is also x0). Because the state for x0
> was already overwritten in the first call, the second call would use the
> newly loaded type as the base pointer instead of the original base pointer
> type.
> 

Agreed. I need to snapshot the source register state beforehand.

>> +
>> +	/* Handle the second destination register (ldp only) */
>> +	if (dst->multi_regs) {
>> +		int reg_size = arm64__reg_size(dl->ops.target.raw);
> 
> [Severity: Medium]
> Does arm64__reg_size() return the correct memory offset for ldpsw
> instructions?
> 
> For a Load Pair of Signed Words (ldpsw), the instruction loads two 32-bit
> values from memory into 64-bit registers. Using arm64__reg_size() on the
> destination register (which starts with 'x') returns 8, but the memory
> spacing should be 4 bytes. 
> 
> This would result in passing reg_offset + 8 instead of reg_offset + 4 to
> propagate_load_reg_state() for the second register, leading to an incorrect
> struct field offset lookup.
> 

Agreed. I missed ldpsw (from the architecture reference manual, it appears to
be the only special case among load-pair instructions).

>> +
>> +		if (reg_size < 0 ||
>> +		    propagate_load_reg_state(state, dl, dst->reg2, src,
>> +					     reg_offset + reg_size, insn_name))
>> +			goto out_err_adjust;
>> +	}
> 
> [ ... ]
> 
>> @@ -418,7 +559,8 @@ static void update_insn_state_arm64(struct type_state *state,
>>  	 * Invalidate destination register(s) for unsupported instructions to
>>  	 * prevent stale type info from propagating to subsequent instructions.
>>  	 */
>> -	if (has_reg_type(state, dst->reg1)) {
>> +	if (has_reg_type(state, dst->reg1) &&
>> +	    strncmp(dl->ins.name, "ld", 2)) {
> 
> [Severity: Medium]
> Should we check if the destination is actually a memory reference before
> invalidating the register?
> 
> For store instructions (like str x0, [x1, #16]), arm64_ldst__parse() reverses
> the operands so that dst becomes the memory operand and dst->reg1 is the
> base pointer (x1). 
> 
> By unconditionally invalidating dst->reg1 for any instruction that doesn't
> start with "ld", we would erroneously destroy the type state of the base
> pointer used in the store.
> 

Agreed.

>>  		pr_debug_dtp("%s [%x] invalidate reg%d",
>>  			     dl->ins.name, insn_offset, dst->reg1);
>>  		invalidate_reg_state(&state->regs[dst->reg1]);
> 
> [ ... ]
> 
>> +	/* Memory to register transfers */
>> +	if (!strncmp(dl->ins.name, "ld", 2))
>> +		update_load_insn_state(state, dl, src, dst);
>>  }
>>  #endif
> 
> [Severity: Medium]
> Could PC-relative memory loads end up with missing operands here if their
> end-of-line comments contain a bracket?
> 
> In arm64_ldst__parse(), which runs before we reach this point, strchr()
> searches for a bracket. If a PC-relative load lacks brackets in its operands
> but contains one in the comment (for example, ldr x0, 1234 <sym> // [foo]),
> the parser matches the comment bracket and parses backwards.
> 
> This drops the actual PC-relative target entirely, which would prevent
> update_load_insn_state() from successfully tracking the type when evaluated
> here.
> 

The comment issue in arm64_ldst__parse() has already been mentioned previously
and will be fixed. PC-relative loads (ldr) themselves generally do not
carry type information; they are typically used to load a label address
for branching:

  ffff800081d0c0a4: 58000068 ldr x8, ffff800081d0c0b0 <cpu_resume+0x28>
  ffff800081d0c0a8: d61f0100 br x8

So I think this case can be ignored.

Thanks,
Tengda