Re: [PATCH 2/3] perf dwarf-aux: Add C++ class, inheritance and vtable helpers

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] `die_find_virtual_func` returns a `DW_TAG_inheritance` DIE instead of a `DW_TAG_subprogram` DIE when resolving inherited virtual functions.
- [High] `die_get_vtable_index` inconsistently returns a byte offset instead of an array index on pre-DWARF 5 binaries.
- [Medium] `die_get_base_class` bypasses DWARF expression handling for `DW_AT_data_member_location`, causing incorrect offsets on older DWARF formats.
--

--- Patch [2]: [PATCH 2/3] perf dwarf-aux: Add C++ class, inheritance and vtable helpers ---
commit 11247af931b847d92f3746e9aef5b7921e7db0a0
Author: Yanbo Zhao <[email protected]>

perf dwarf-aux: Add C++ class, inheritance and vtable helpers

This patch adds several DWARF helper functions to support C++ virtual function
inspection. It includes utilities for detecting source language, retrieving
base classes, inspecting virtual tables, and locating class members. The
helpers fall back to older DWARF constructs when DWARF 5 constants are
unavailable.

> diff --git a/tools/perf/util/dwarf-aux.c b/tools/perf/util/dwarf-aux.c
> index 88ba0ec23a686..e991eed963a8a 100644
> --- a/tools/perf/util/dwarf-aux.c
> +++ b/tools/perf/util/dwarf-aux.c

> [ ... ]

> @@ -68,6 +112,90 @@ bool die_is_compound_type(Dwarf_Die *type_die)

> [ ... ]

> +Dwarf_Die *die_get_base_class(Dwarf_Die *class_die, Dwarf_Die *base_die, int *offset)
> +{
> +	Dwarf_Die inherit_die;
> +	Dwarf_Attribute attr;
> +	Dwarf_Word loc;
> +
> +	if (die_find_child(class_die, __die_find_inheritance_cb,
> +			   &inherit_die, &inherit_die) == NULL)
> +		return NULL;
> +
> +	if (__die_get_real_type(&inherit_die, base_die) == NULL)
> +		return NULL;
> +
> +	if (dwarf_attr_integrate(&inherit_die, DW_AT_data_member_location, &attr) &&
> +	    dwarf_formudata(&attr, &loc) == 0) {
> +		*offset = loc;
> +	} else {
> +		*offset = 0;
> +	}

[Severity: Medium]
On older DWARF binaries, DW_AT_data_member_location is emitted as a location
block (e.g., DW_OP_plus_uconst) rather than a simple udata constant. 

Since dwarf_formudata() fails on blocks, does this silently fall back to
setting the offset to 0, yielding the wrong base class offset?

Could this use the existing die_get_data_member_location() utility which
already handles both dwarf_formudata() and location expressions?

> +
> +	return base_die;
> +}
> +
> +int die_get_vtable_index(Dwarf_Die *func_die, int *index)
> +{

> [ ... ]

> +	/* Fallback to DW_AT_vtable_elem_location (older DWARF) */
> +	if (dwarf_attr(func_die, DW_AT_vtable_elem_location, &attr)) {
> +		Dwarf_Op *expr;
> +		size_t expr_len;
> +
> +		/* Compile often emits it as a simple constant expression or block */
> +		if (dwarf_getlocation(&attr, &expr, &expr_len) == 0 && expr_len > 0) {
> +			if (expr[0].atom == DW_OP_constu) {
> +				*index = expr[0].number;

[Severity: High]
Does this fallback logic return a byte offset instead of an array index on
pre-DWARF 5 binaries?

If a binary uses DWARF 4, DW_AT_vtable_elem_location yields the byte offset
in the vtable. Should this byte offset be divided by the pointer size before
being assigned to the index to ensure it behaves consistently with DWARF 5?

> +				return 0;
> +			}

> [ ... ]

> @@ -2164,6 +2292,70 @@ Dwarf_Die *die_get_member_type(Dwarf_Die *type_die, int offset,

> [ ... ]

> +static int __die_find_virtual_func_cb(Dwarf_Die *die_mem, void *arg)
> +{

> [ ... ]

> +	if (tag == DW_TAG_inheritance) {
> +		Dwarf_Die base_type;
> +
> +		if (__die_get_real_type(die_mem, &base_type)) {
> +			if (die_find_child(&base_type,
> +					   __die_find_virtual_func_cb,
> +					   ad, &ad->func_die))
> +				return DIE_FIND_CB_END;
> +		}
> +	}
> +
> +	return DIE_FIND_CB_SIBLING;
> +}
> +
> +Dwarf_Die *die_find_virtual_func(Dwarf_Die *class_die, int index, Dwarf_Die *die_mem)
> +{
> +	struct find_virtual_func_data ad = {
> +		.index = index,
> +		.found = false,
> +	};
> +
> +	if (die_find_child(class_die, __die_find_virtual_func_cb, &ad, die_mem))
> +		return die_mem;

[Severity: High]
When resolving a virtual function implemented in a base class, does this logic
return a DW_TAG_inheritance DIE instead of the actual DW_TAG_subprogram DIE?

If the nested search in __die_find_virtual_func_cb() successfully finds the
subprogram, it populates ad->func_die and returns DIE_FIND_CB_END. However,
the outer die_find_child() loop in die_find_virtual_func() terminates and
returns its current iteration pointer, die_mem.

Since die_mem points to the DW_TAG_inheritance DIE at that moment, does this
ignore the populated ad.func_die and return the inheritance DIE to the caller?

> +
> +	return NULL;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2