Re: [PATCH] libdw: Add dwarf_peel_type. Use it in dwarf_aggregate_size.

[Mark Wielaard <mjw at redhat dot com>]

On Fri, 2014-10-10 at 14:30 -0700, Josh Stone wrote:
> On 10/10/2014 11:42 AM, Roland McGrath wrote:
> > The description in your prologue and the libdw.h comment do not match what
> > the code actually does.  You describe it as going until it hits one of the
> > known categories of actual type.  But what it actually does is go only as
> > long as it is seeing one of the known categories of wrapper type.  I'm not
> > really sure which it should do (or if it should instead do something
> > different from either), but the description should match the behavior.
> > 
> > As described, it would fall down if a new tag were introduced for a new
> > category of actual type.  As implemented, it would fall down if a new tag
> > were introduced for a new category of wrapper type.
> 
> I think peeling through something unrecognized is worse than stopping,
> so I'd update the description to match the current code.

Yeah, you caught me. No good deed goes unpunished. For once I write the
documentation before the code to make sure I don't forget. And then of
course I find out it cannot actually written as documented, but forget
to update the documentation...

> FWIW, DWARF4 Figure 15 (under section 5.2) lists type modifier tags.  Of
> those those we should peel through const, restrict, and volatile, and
> stop at pointer, ref, and rvalue_ref.  I'm not sure about packed or
> shared though.

Both packed and shared change the structural layout of the underlying
type, so they are different from type aliases or qualifier tags.

>   It's odd to me that typedef didn't make this list, but
> it should be peeled too.

Yes, DWARF has a special type for type aliases (typedefs), but kind of
lumps qualifiers, storage and structural type modifiers together. In the
text that is, there is no actual way to identify this group of tags.
They don't even share a common unique attribute. As can be seen by...

> > A third potential approach would be to attempt future-proofing for those
> > cases.  That is, just keep going as long as there is a DW_AT_type
> > attribute.  But that would need a special case for DW_TAG_pointer_type and
> > DW_TAG_reference_type, where you want to stop even though it has a
> > DW_AT_type referring to another type.  I can't think of any other cases
> > where an "actual" type has a DW_AT_type, but there might well be some.  If
> > any new tag were like DW_TAG_pointer_type rather than like
> > DW_TAG_const_type et al, then this approach would fall down there.
> 
> Going through DWARF4 appendix A for those DW_TAG_*_type which suggest
> DW_AT_type, I'd also exclude these from peeling:
> 
> DW_TAG_array_type
> DW_TAG_enumeration_type
> DW_TAG_ptr_to_member_type
> DW_TAG_set_type
> DW_TAG_subrange_type
> DW_TAG_subroutine_type

Right, there is no one semantic/structural meaning to DW_AT_type. So
depending on it being present isn't really a good way to distinguish
between the types to include/exclude.

> > So firstly we need to decide which kinds of future addition we expect and
> > thus how to handle the future-proofing.  Then we need to have descriptions
> > and implementation that match.
> 
> IMO, a blacklist of what not to peel is more dangerous to the future
> than a whitelist of what we know can be peeled.

Agreed. That is why I ended up implementing it as an explicit whitelist
that just includes typedef, const_type, volatile_type and restrict_type.

Other possible future modifier type tags with the same properties are
are DW_TAG_atomic_type which will probably be added with DWARFv5 (or as
GNU extension), and maybe DW_TAG_GNU_aligned_type which I have been
toying with for GCC/GDB. But I think that will be turned into an
attribute instead.

What would be the correct way to describe that future versions might add
those to the white list of tags to be peeled? My goal really is to have
a type peel function that user code can depend on if they need to do
something like find the underlying return type in the backends or like
the dwarf_aggregate_size function. So that when DWARFv5 comes out, or
some new GNU qualifier tag is added, all the user has to do is upgrade
to a newer elfutils that knows about those.

Thanks,

Mark