Re: [PATCH 2/2] Add C++ iterators

[Mark Wielaard <mjw at redhat dot com>]

Hi Petr,

Sorry for being slow. Struggling with both the DWARF and C++ concepts at
the same time is a little daunting. I am happy with the implementation
and C++ concepts used. But still have some questions on how we expose
some of the DWARF concepts.

On Tue, 2015-04-14 at 16:47 +0200, Petr Machata wrote:
> - every iterator implemented in an own .cc module
> 
> - new header files libdw, libdwfl with iterator declarations, and a new
>   library libdwpp (-ldwpp) that ships the code
> 
> - the iterators are in versioned namespaces.  When changes are
>   introduced, new version namespace is added, in which the new
>   interfaces are specified and then the old namespace is imported,
>   bringing is the rest that wasn't changed.  The new namespace then gets
>   imported into the top-level "elfutils" namespace, so that people can
>   simply write elfutils::die_tree_iterator.  Under the hood, this
>   resolves to, say, elfutils::v1::die_tree_iterator, and that's what the
>   ABI-level symbol is.
> 
>   This allows us to expose the layout of the individual classes, and
>   avoid the overhead of pimpl.  It also avoids the messiness of symbol
>   version files.
> 
>   Select private symbols that need header-file visibility were defined
>   as attribute_hidden, the rest is either visible, or stashed in
>   anonymous namespaces in .cc.

I like all this. Thanks.

> - Add unit_iterator, child_iterator, die_tree_iterator and
>   attr_iterator for libdw, and dwfl_module_iterator for libdwfl.

I like the child, attr and dwfl_module iterators.

I have a small concern about the unit_iterator to make it "future
proof". One of the suggestions for DWARFv5 is to merge the type units
into .debug_info from the separate .debug_type section. See 
http://dwarfstd.org/ShowIssue.php?issue=130526.1
This will cause us some pain because it changes the CU header adding an
extra field (for all CU types). Luckily the length and version fields
stay at the start, so the libdw parsing code should be able to cope when
we see the new version. But it might cause some brainteasers to keep the
dwarf_next_unit, dwarf_offdie and dwarf_offdie_types functions sane
(maybe we'll just need new versions to cope with DWARFv5). It looks like
the C++ unit_iterator interface abstracts all these implementation
issues away. But it might be a good idea to already add the notion of
unit_type to the struct unit_info.

Or maybe not, you get the Dwarf_Die cudie so you can already inspect the
tag. And we iterate over all unit types already (compile, partial,
type). So the only thing to change for handling DWARFv5 correctly would
be the private unit_iterator::move () implementation. We just should
make clear the order is not guaranteed. OK. I don't have any concern
anymore. It is good as it is :)

But I am a little confused about the die_tree_iterator. There are two
issues I don't fully understand. That might be because my imagined use
case just doesn't the use case this was designed for.

It looks like the use case this was designed for was to iterate over all
Dwarf_Dies in one single Dwarf file or starting at a particular
unit_iterator to iterate over all remaining units in a Dwarf file and
provide all (raw) Dwarf_Dies.

Is that a common use case? I would have imagined that you would iterate
over just the Dwarf_Dies of one particular unit separately or given a
particular Dwarf_Die would like it to iterate over just the die_tree
under that DIE. Given the current design of the die_tree_iterator how
would one do that?

Also I think that we really should provide an easy way to walk the
logical DIE tree. Which means resolving DW_TAG_imported_unit DIEs and
just continue with the children of the imported unit at that point.
There should of course also be an option to walk the "raw" DIE tree. But
it feels wrong to let the user do logical walks on their own, checking
for imported_unit tags, walking the imported_unit children tree, keep
their own parent stack, etc.

That of course does mean the current internal tracking using just a
Dwarf_Off is not enough, because resolving imported_units might cross
Dwarfs.

Let me know what you think of the use case that involves just iterating
over a specific (logical) Dwarf_Die subtree. Should we introduce some
completely different iterator for that? Or is that done easily "by hand"
using the current iterators?

Thanks,

Mark