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

[sashiko-bot@kernel.org]

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).

> +
> +	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?

> +			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