Re: [PATCH v2 3/3] perf annotate: Resolve C++ virtual function calls in x86 insn tracking

[Namhyung Kim <namhyung@kernel.org>]

On Wed, Sep 30, 2026 at 09:11:12PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] perf: cross-platform analysis broken by hardcoded host pointer size
> - [Medium] perf: vtable pointer load ignores multi-register index
> --
> 
> commit a71181b28893b295ef8ae1558faf7b1c66e3593b
> Author: Yanbo Zhao <yzhao62@ncsu.edu>
> 
> perf annotate: Resolve C++ virtual function calls in x86 insn tracking
> 
> This commit introduces tracking of C++ virtual function calls by resolving
> vtable pointer loads and virtual function calls through the vtable in x86
> instruction state tracking. It enhances update_insn_state_x86() to handle
> TSR_KIND_VTABLE_PTR and TSR_KIND_VFUNC_PTR register states.
> 
> > diff --git a/tools/perf/util/annotate-arch/annotate-x86.c b/tools/perf/util/annotate-arch/annotate-x86.c
> > index 1acf31a2c7597..d21192b74b6bf 100644
> > --- a/tools/perf/util/annotate-arch/annotate-x86.c
> > +++ b/tools/perf/util/annotate-arch/annotate-x86.c
> 
> [ ... ]
> 
> > @@ -216,6 +216,29 @@ static void invalidate_reg_state(struct type_state_reg *reg)
> >  	reg->copied_from = -1;
> >  }
> >  
> > +/*
> > + * Get the return type of the C++ virtual function at the @offset in the
> > + * vtable of @class_die.  Returns false if it's not found or the function
> > + * returns void.
> > + */
> > +static bool vtable_get_rettype(Dwarf_Die *class_die, int offset,
> > +			       Dwarf_Die *type_die)
> > +{
> > +	Dwarf_Die func_die;
> > +	int index;
> > +
> > +	if (offset < 0)
> > +		return false;
> > +
> > +	/* TODO: arch-dependent pointer size */
> > +	index = offset / sizeof(void *);
> 
> [Severity: Medium]
> Will dividing the target's vtable byte offset by the host's sizeof(void *)
> compute an incorrect slot index when analyzing data from an architecture
> with a different pointer size?
> 
> Relying on host execution details like sizeof(void *) breaks
> cross-platform analysis (e.g., analyzing 32-bit x86 perf.data on a
> 64-bit host).

I think this is fine for now as we also have other places assuming the
host pointer size.  We'll fix it later.

> 
> > +
> > +	if (die_find_virtual_func(class_die, index, &func_die) == NULL)
> > +		return false;
> > +
> > +	return die_get_real_type(&func_die, type_die) != NULL;
> > +}
> > +
> >  static void update_insn_state_x86(struct type_state *state,
> 
> [ ... ]
> 
> > @@ -622,13 +669,38 @@ static void update_insn_state_x86(struct type_state *state,
> >  			}
> >  			pr_debug_type_name(&tsr->type, tsr->kind);
> >  		}
> > +		/* Load a function pointer from the vtable (for 'call *%reg') */
> > +		else if (has_reg_type(state, sreg) && state->regs[sreg].ok &&
> > +			 state->regs[sreg].kind == TSR_KIND_VTABLE_PTR &&
> > +			 vtable_get_rettype(&state->regs[sreg].type, src->offset,
> > +					    &type_die)) {
> 
> [Severity: Medium]
> Does this query the wrong vtable slot by ignoring the index register when
> src->multi_regs is true?
> 
> If the operand is an indexed memory reference using multiple registers
> (e.g., mov 0x8(%rbx, %rcx, 8), %rax), src->offset contains only the
> static displacement part, not the dynamic index. Should this verify
> !src->multi_regs before querying the DWARF vtable with src->offset?

Multiple register is not handled well in the current code.  Dealing with
dynamic index would require more careful instruction tracking.

Anyway, it'd be safer to check !src->multi_regs here.

Thanks,
Namhyung

> 
> > +			tsr->type = type_die;
> > +			tsr->kind = TSR_KIND_VFUNC_PTR;
> > +			tsr->offset = 0;
> > +			tsr->ok = true;
> > +
> > +			pr_debug_dtp("mov [%x] %#x(reg%d) -> reg%d",
> > +				     insn_offset, src->offset, sreg, dst->reg1);
> > +			pr_debug_type_name(&tsr->type, tsr->kind);
> > +		}
> >  		/* And then dereference the pointer if it has one */
> 
> [ ... ]
> 
> > diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
> > index 8a9d3f2eec4dc..e9bf76cc28633 100644
> > --- a/tools/perf/util/annotate-data.c
> > +++ b/tools/perf/util/annotate-data.c
> 
> [ ... ]
> 
> > @@ -715,8 +723,11 @@ void set_stack_state(struct type_state_stack *stack, int offset, u8 kind,
> >  			    Dwarf_Die *type_die, int ptr_offset)
> >  {
> >  	Dwarf_Word size;
> > +	bool is_pointer = (kind == TSR_KIND_POINTER ||
> > +			   kind == TSR_KIND_VTABLE_PTR ||
> > +			   kind == TSR_KIND_VFUNC_PTR);
> >  
> > -	if (kind == TSR_KIND_POINTER) {
> > +	if (is_pointer) {
> >  		/* TODO: arch-dependent pointer size */
> >  		size = sizeof(void *);
> >  	}
> 
> [Severity: Medium]
> Like the vtable_get_rettype() issue above, will hardcoding the host's
> pointer size here corrupt the stack state tracking for the target
> architecture?
> 
> [ ... ]
> 
> > diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
> > index 5478c134e7e3b..5d96ee4c866b6 100644
> > --- a/tools/perf/util/disasm.c
> > +++ b/tools/perf/util/disasm.c
> 
> [ ... ]
> 
> > @@ -300,6 +300,11 @@ static int call__parse(const struct arch *arch, struct ins_operands *ops, struct
> >  	if (tok != NULL) {
> >  		endptr++;
> >  
> > +		/* Save the operand (without '*') to extract register and offset */
> > +		ops->target.raw = strdup(tok + 1);
> 
> [Severity: Low]
> This is a pre-existing issue, but this file calls strdup() without
> explicitly including <string.h>.
> 
> Can we add the explicit <string.h> include? The perf subsystem guide states
> that POSIX standard headers should be explicitly included to prevent musl
> compilation failures from missing implicit inclusions.
> 
> > +		if (ops->target.raw == NULL)
> > +			return -1;
> > +
> >  		/* Indirect call can use a non-rip register and offset: callq  *0x8(%rbx).
> >  		 * Do not parse such instruction.  */
> >  		if (strstr(endptr, "(%r") == NULL)
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260930210038.196928-1-yzhao62@ncsu.edu?part=3