Re: [PATCH v4 16/23] perf annotate-data: Expand type_state_reg imm_value to u64

[Tengda Wu]

On 2026/8/11 16:10, Shuai Xue wrote:
> 
> 
> On 8/8/26 8:23 PM, Tengda Wu wrote:
>> The imm_value in struct type_state_reg is defined as u32, which limits
>> the size of values it can pass.
>>
>> Promote imm_value from u32 to u64 and adjust the print format specifier
>> in pr_debug_dtp() accordingly.
>>
>> Signed-off-by: Tengda Wu <wutengda@xxxxxxxxxxxxxxx>
>> ---
>>   tools/perf/util/annotate-arch/annotate-x86.c | 2 +-
>>   tools/perf/util/annotate-data.h              | 2 +-
>>   2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/perf/util/annotate-arch/annotate-x86.c b/tools/perf/util/annotate-arch/annotate-x86.c
>> index ee4e3e7f3209..eec3d8ce00b8 100644
>> --- a/tools/perf/util/annotate-arch/annotate-x86.c
>> +++ b/tools/perf/util/annotate-arch/annotate-x86.c
>> @@ -540,7 +540,7 @@ static void update_insn_state_x86(struct type_state *state,
>>               tsr->offset = 0;
>>               tsr->ok = true;
>>   -            pr_debug_dtp("mov [%x] imm=%#x -> reg%d\n",
>> +            pr_debug_dtp("mov [%x] imm=%#"PRIx64" -> reg%d\n",
>>                        insn_offset, tsr->imm_value, dst->reg1);
>>               return;
>>           }
>> diff --git a/tools/perf/util/annotate-data.h b/tools/perf/util/annotate-data.h
>> index 453e13bbe3e2..91b83e94c51b 100644
>> --- a/tools/perf/util/annotate-data.h
>> +++ b/tools/perf/util/annotate-data.h
>> @@ -173,7 +173,7 @@ extern struct annotated_data_stat ann_data_stat;
>>    */
>>   struct type_state_reg {
>>       Dwarf_Die type;
>> -    u32 imm_value;
>> +    u64 imm_value;
>>       /*
> One subtlety this introduces: annotated_op_loc.offset is an int, and
> immediates are parsed into it via strtol(), so assignments like
> tsr->imm_value = src->offset now sign-extend instead of preserving
> the 32-bit bit pattern. mov $0xdeadbeef used to track 0xdeadbeef,
> now it tracks 0xffffffffdeadbeef, while the register actually holds
> the zero-extended value. (Canonical kernel addresses happen to
> sign-extend back to the right value, which masks this in the common
> case.)
> 

It appears that assignments from offset to imm_value occur in only a few places:

add/sub: (already existing)
  u64 imm_value = -1ULL;
  imm_value = src->offset;  // int to u64

mov immediate: (newly introduced)
  tsr->imm_value = src->offset;  // int to u64


> Relatedly, this widening doesn't actually help 64-bit immediates on
> the strtol() path - movabs is still truncated to int at parse time.

Indeed, for movabs instructions, the immediate value is truncated due to
the width of offset and strtol() as well.

> The real consumers that need u64 are the arm64 adrp and stack paths,
> which feed imm_value from ops.source.addr / stack state directly, so
> the change is justified. But maybe worth spelling that out, and
> considering a follow-up that parses immediates with strtoull() into a
> dedicated u64 field instead of overloading the signed offset field.
> 
> Thanks,

To summarize, there are three issues:

1. Sign-extension issue in add/sub (pre-existing)
2. Sign-extension issue in mov immediate (newly introduced)
3. Truncation issue in movabs (pre-existing)

In this patch, I'd like to fix issue #2 first by adding a type cast to
avoid the sign-extension problem:

  tsr->imm_value = (s64)src->offset;

As for issues #1 and #3, which are pre-existing, a possible solution would
be to promote offset to 64-bit as well. I'm thinking of addressing those in
a separate patch series, since it involves multiple architectures and would
need careful review.

Thanks,
Tengda