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

[Namhyung Kim <namhyung@kernel.org>]

On Fri, Aug 14, 2026 at 03:58:12PM +0800, Tengda Wu wrote:
> 
> 
> 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@huaweicloud.com>
> >> ---
> >>   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.

Sounds good.

Thanks,
Namhyung