Page MenuHomePhabricator

Store TypeUnits in a SmallVector<DWARFUnitSection> instead of a single DWARFUnitSection.
ClosedPublic

Authored by friss on Sep 24 2014, 10:18 AM.

Details

Summary

There will be multiple TypeUnits in an unlinked object that will be extracted
from different sections. Now that we have DWARFUnitSection that is supposed
to represent an input section, we need a DWARFUnitSection<TypeUnit> per
input .debug_types section.

Once this is done, the interface is homogenous and we can move the Section
parsing code into DWARFUnitSection.

Diff Detail

Repository
rL LLVM

Event Timeline

friss updated this revision to Diff 14042.Sep 24 2014, 10:18 AM
friss retitled this revision from to Store TypeUnits in a SmallVector<DWARFUnitSection> instead of a single DWARFUnitSection..
friss added reviewers: samsonov, dblaikie.
friss updated this object.
friss added a subscriber: Unknown Object (MLST).
dblaikie accepted this revision.Sep 24 2014, 10:31 AM
dblaikie edited edge metadata.

Looks fine to me - some /totally optional/ comments, just if you think they're improvements.

lib/DebugInfo/DWARFContext.cpp
87 ↗(On Diff #14042)

getNumTypeUnits is now going to be potentially misleading - it'll specify the number of type unit sections, not the number of type units.

That happens to be the same for type units (for well constructed DWARF - but again, we might be dealing with bad DWARF when using this tool, so it's interesting to consider the possibility of present though empty type_units sections, etc) but isn't for DWO type units (which, when I first implemented them, went in separate sections - but then I later fixed it per the DWARF Fission spec, to put them all in the same section).

Might be a place where the 'empty(range)' template in that patch I showed previously could be handy - then there's no need for the extra getNum* functions, simply size(dwo_type_unit_sections()) - same behavior, but not quite as misleading.

Honestly though, I'm not sure how much value the "getNum*" calls provide here, just printing the section header seems pretty benign/possibly desired.

Some food for thought.

383 ↗(On Diff #14042)

I'm sure I've used this idiom before - I'm not sure if it's nicer than something more like:

Foo f;
...
v.push_back(std::move(f));

(which I'm sure I've used as well)

lib/DebugInfo/DWARFContext.h
33 ↗(On Diff #14042)

If it's going to have a small size of 1, maybe just use std::vector?

(granted I'm not quite as averse to the standard containers as some other LLVM contributors/owners - so opinions certainly differ)

41 ↗(On Diff #14042)

same here

This revision is now accepted and ready to land.Sep 24 2014, 10:31 AM
samsonov accepted this revision.Sep 24 2014, 7:03 PM
samsonov edited edge metadata.

LGTM

friss added inline comments.Sep 25 2014, 6:37 AM
lib/DebugInfo/DWARFContext.cpp
87 ↗(On Diff #14042)

Yeah I agree. I won't change it right now, because I think all the getNum methods will disappear anyway or get renamed(they are never used to actually get the number of units, but just to know if there is at least one).

383 ↗(On Diff #14042)

I think I prefer the one I used. I think can be more efficient depending on the actual move semantics of the object, but I'm not able to come up with a simple example were it really would matter.

lib/DebugInfo/DWARFContext.h
33 ↗(On Diff #14042)

I used the idiom that the Unit lists were using before we moved them into DWARFUnitSection. I might change that, we can't predict an actual average size for the vector, thus SmallVector won't bring us anything.

(Alexey's already signed off here - but thanks for following up on my comments, sounds fine)

friss closed this revision.Sep 26 2014, 5:25 AM
friss updated this revision to Diff 14108.

Closed by commit rL218513 (authored by @friss).