Re: [PATCH v3 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 1 potential issue(s) to consider:
- [Medium] perf: cross-platform analysis broken by host pointer size in vtable_get_rettype()
--

commit bbc900a432273e7459ac998c5ec0f21d8e575f31
Author: Yanbo Zhao <yzhao62@ncsu.edu>

perf annotate: Resolve C++ virtual function calls in x86 insn tracking

This commit resolves C++ virtual function calls in x86 instruction tracking by
identifying the vtable pointer and the slot index from the type of the object
pointer using DWARF. This allows perf to track the return type of the function
called through the vtable.

> diff --git a/tools/perf/util/annotate-arch/annotate-x86.c b/tools/perf/util/annotate-arch/annotate-x86.c
> index 1acf31a2c7597..36b51f1bab3d0 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]
Is the target binary's vtable index calculated using the host's pointer size
here?

If a user on a 64-bit host analyzes a perf.data file recorded from a 32-bit
binary, the memory offset will be divided by 8 instead of 4.

This yields the wrong vtable index, which can cause the tool to return the
wrong type or fail to resolve the call.

> +
> +	if (die_find_virtual_func(class_die, index, &func_die) == NULL)
> +		return false;
> +
> +	return die_get_real_type(&func_die, type_die) != NULL;
> +}

[ ... ]

> 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]
Will this assignment use the host's pointer size for tracking the newly
added pointer types (TSR_KIND_VTABLE_PTR and TSR_KIND_VFUNC_PTR) on the stack?

Similar to the vtable index calculation, using sizeof(void *) can cause a
size mismatch when the target architecture's pointer size differs from the
host architecture.

>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261005201010.36493-1-yzhao62@ncsu.edu?part=3