Re: [PATCH 1/3] perf dwarf-aux: Add die_is_compound_type() to handle C++ class types

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Empty Base Optimization (EBO) causes zero-sized base classes to shadow data members at the same offset.
- [Medium] Virtual inheritance uses complex location expressions that are unsupported, silently defaulting to offset 0.
--

--- Patch [1]: [PATCH 1/3] perf dwarf-aux: Add die_is_compound_type() to handle C++ class types ---
commit 9d64a45d6e2e13c946726cd0182607e969f8dd87
Author: Yanbo Zhao <[email protected]>

perf dwarf-aux: Add die_is_compound_type() to handle C++ class types

Introduce the die_is_compound_type() helper which checks for
DW_TAG_structure_type, DW_TAG_union_type, and DW_TAG_class_type, and
convert all the existing open-coded struct/union tag checks to use it.
Also accept DW_TAG_inheritance so member lookup descends into C++ base
classes.

> diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
> --- a/tools/perf/util/annotate-data.c
> +++ b/tools/perf/util/annotate-data.c
> @@ -230,9 +230,9 @@ static int __add_member_cb(Dwarf_Die *die, void *arg)
>  	Dwarf_Word size, loc, bit_size = 0;
>  	Dwarf_Attribute attr;
>  	struct strbuf sb;
> -	int tag;
> +	int tag = dwarf_tag(die);
>  
> -	if (dwarf_tag(die) != DW_TAG_member)
> +	if (tag != DW_TAG_member && tag != DW_TAG_inheritance)
>  		return DIE_FIND_CB_SIBLING;

[Severity: Medium]
Since DW_TAG_inheritance is now processed here, how does this handle virtual
inheritance? Virtual base classes often use complex DWARF location
expressions (like DW_OP_deref).

Looking at the location parsing slightly further down in __add_member_cb():

    if (dwarf_attr_integrate(die, DW_AT_data_member_location, &attr)) {
        if (dwarf_formudata(&attr, &loc) != 0) {
            if (die_get_data_member_location(die, &loc) != 0)
                loc = 0;
        }
    }

If die_get_data_member_location() fails due to an unsupported complex
expression, it silently defaults to offset 0. Will this incorrectly assign
virtual base classes an offset of 0, potentially masking actual fields at
offset 0 like the vtable pointer?

[ ... ]
> diff --git a/tools/perf/util/dwarf-aux.c b/tools/perf/util/dwarf-aux.c
> --- a/tools/perf/util/dwarf-aux.c
> +++ b/tools/perf/util/dwarf-aux.c
[ ... ]
> @@ -2053,7 +2061,7 @@ static int __die_find_member_offset_cb(Dwarf_Die *die_mem, void *arg)
>  	Dwarf_Word offset = (long)arg;
>  	int tag = dwarf_tag(die_mem);
>  
> -	if (tag != DW_TAG_member)
> +	if (tag != DW_TAG_member && tag != DW_TAG_inheritance)
>  		return DIE_FIND_CB_SIBLING;

[Severity: Medium]
With DW_TAG_inheritance now included, does this code correctly handle the
Empty Base Optimization (EBO)?

Zero-sized base classes share the same offset as the first data member.
Further down in __die_find_member_offset_cb(), there is an exact offset
match check:

    if (offset == loc)
        return DIE_FIND_CB_END;

Since an empty base class often appears first in DWARF before the data
member, will this unconditionally return DIE_FIND_CB_END when matching the
empty base class? This appears to stop the search and cause data type
resolution to fail for the actual data member at the same offset.

Additionally, __die_find_member_offset_cb() contains similar location parsing
to __add_member_cb() which falls back to 0 if die_get_data_member_location()
fails. Could this also cause virtual base classes to incorrectly resolve to
offset 0 here?

>  	/* Unions might not have location */

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