[PATCH] Store TypeUnits in a SmallVector<DWARFUnitSection> instead of a single DWARFUnitSection.

Frederic Riss friss at apple.com
Thu Sep 25 06:37:44 PDT 2014


================
Comment at: lib/DebugInfo/DWARFContext.cpp:87
@@ -86,3 +86,3 @@
 
   if ((DumpType == DIDT_All || DumpType == DIDT_Types) && getNumTypeUnits()) {
     OS << "\n.debug_types contents:\n";
----------------
dblaikie wrote:
> 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. 
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).

================
Comment at: lib/DebugInfo/DWARFContext.cpp:383
@@ -378,1 +382,3 @@
         DataExtractor(I.second.Data, isLittleEndian(), 0);
+    DWOTUs.push_back(DWARFUnitSection<DWARFTypeUnit>());
+    auto &TUS = DWOTUs.back();
----------------
dblaikie wrote:
> 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)
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.

================
Comment at: lib/DebugInfo/DWARFContext.h:33
@@ -32,3 +32,3 @@
   DWARFUnitSection<DWARFCompileUnit> CUs;
-  DWARFUnitSection<DWARFTypeUnit> TUs;
+  SmallVector<DWARFUnitSection<DWARFTypeUnit>,1> TUs;
   std::unique_ptr<DWARFDebugAbbrev> Abbrev;
----------------
dblaikie wrote:
> 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)
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.

http://reviews.llvm.org/D5482






More information about the llvm-commits mailing list