Factor the Unit section parsing into the DWARFUnitSection class.

[friss]

Summary

No functional change.

[samsonov]

lib/DebugInfo/DWARFUnit.h

68

initialize Parsed in this constructor as well.

74

Can't you fetch all the sections' contents (debug_abbrev, debug_ranges, debug_addr etc.) by using a "Context" argument?

[dblaikie]

lib/DebugInfo/DWARFUnit.h

68

I /think/ you can just use "SmallVector(std::move(DUS))" here.

77

(non-actionable gripe: I'm not a huge fan of the extra flag here (not that it's any worse than the previous check - and it's marginally better in some ways)... calling parse repeatedly only to have it no-op because it's already been parsed does feel a bit weird.

What would happen if the users instead had an Optional<DWARFUnitSection> and would check that, then construct a DWARFUnitSection passing in the various sections, etc - so there was no possibility of a non-parsed DWARFUnitSection. They were constructed with the data and parsed during construction only?

(OK, maybe semi-actionable, but could just be fanciful ideas on my part))

[friss]

lib/DebugInfo/DWARFUnit.h

74

Not trivially, because the sections are different between dwo and normal sections. Would you prefer that the patch implements parse() and parseDWO() that fetch the sections themselves (and have the current parse() become a private parseImpl() method)?

77

You have already mentioned it and I didn't forget it. I too find that the added flag looks clumsy, and for an NFC change, I could have just left the old check as it was (but I really dislike the previous empty() check).

As you point out, doing it with Optional would mean removing the laziness, or rather moving it into another accessor. This looks orthogonal and big enough to mandate another patch if it is wanted. I can look into it in a followup patch.

[dblaikie]

lib/DebugInfo/DWARFUnit.h

77

Agreed on all counts.

[samsonov]

lib/DebugInfo/DWARFUnit.h

74

Yes, I'd prefer that. This would make the caller's code simpler. After that (in the follow-up changes), we can probably go on and sink getDebugAbbrev() / getRangeSection() / isLittleEndian() and calls like that further down to DWARFUnit (not DWARFUnitSection) constructor: after your recent changes DWARFUnit ctor now takes a reference to full DWARFContext.

We might even discriminate between regular and .dwo units by making them different classes, inherited from DWARFUnit. For instance, both regular and dwo units now have a reference to corresponding .dwo unit (DWOHolder stuff), which makes no sense.

[friss]

As per review comments, move the collection of debug sections into helpers rather
than passing all the sections explicitely to the parse methods.

[samsonov]

I think this is fine for a incremental change (but see a comment below).

lib/DebugInfo/DWARFUnit.h

39

Can these methods take const DWARFContext::Section & instead of StringRef/RelocAddrMap ?

[friss]

lib/DebugInfo/DWARFUnit.h

39

Nope because you can't forward declare nested structs and including DWARFContext.h in DWARFUnit.h isn't possible (circular dependency). We could unnest the Section struct from the Context object to allow this.

[samsonov]

lib/DebugInfo/DWARFUnit.h

39

Yes. I think it's fine to commit this change as is, and then hoist Section (essentially, StringRef/RelocAddrMap pair) from DWARFContext and pass it here in a small follow-up change. Thanks!

[friss]

Closed by commit rL219098 (authored by @friss).

lib/DebugInfo/DWARFContext.cpp

DWARFContext::getLineTableForUnit(DWARFUnit *cu) {
 
   // We have to parse it first.
   DataExtractor lineData(getLineSection().Data, isLittleEndian(),
                          cu->getAddressByteSize());
   return Line->getOrParseLineTable(lineData, stmtOffset);
 }
 
 void DWARFContext::parseCompileUnits() {
-  if (!CUs.empty())
-    return;
-  uint32_t offset = 0;
-  const DataExtractor &DIData = DataExtractor(getInfoSection().Data,
-                                              isLittleEndian(), 0);
-  while (DIData.isValidOffset(offset)) {
-    std::unique_ptr<DWARFCompileUnit> CU(new DWARFCompileUnit(*this,
-        getDebugAbbrev(), getInfoSection().Data, getRangeSection(),
-        getStringSection(), StringRef(), getAddrSection(),
-        &getInfoSection().Relocs, isLittleEndian(), CUs));
-    if (!CU->extract(DIData, &offset)) {
-      break;
-    }
-    CUs.push_back(std::move(CU));
-    offset = CUs.back()->getNextUnitOffset();
-  }
+  CUs.parse(*this, getInfoSection().Data, getInfoSection().Relocs);
 }
 
 void DWARFContext::parseTypeUnits() {
   if (!TUs.empty())
     return;
   for (const auto &I : getTypesSections()) {
-    uint32_t offset = 0;
-    const DataExtractor &DIData =
-        DataExtractor(I.second.Data, isLittleEndian(), 0);
     TUs.push_back(DWARFUnitSection<DWARFTypeUnit>());
-    auto &TUS = TUs.back();
-    while (DIData.isValidOffset(offset)) {
-      std::unique_ptr<DWARFTypeUnit> TU(new DWARFTypeUnit(*this,
-           getDebugAbbrev(), I.second.Data, getRangeSection(),
-           getStringSection(), StringRef(), getAddrSection(),
-           &I.second.Relocs, isLittleEndian(), TUS));
-      if (!TU->extract(DIData, &offset))
-        break;
-      TUS.push_back(std::move(TU));
-      offset = TUS.back()->getNextUnitOffset();
-    }
+    TUs.back().parse(*this, I.second.Data, I.second.Relocs);
   }
 }
 
 void DWARFContext::parseDWOCompileUnits() {
-  if (!DWOCUs.empty())
-    return;
-  uint32_t offset = 0;
-  const DataExtractor &DIData =
-      DataExtractor(getInfoDWOSection().Data, isLittleEndian(), 0);
-  while (DIData.isValidOffset(offset)) {
-    std::unique_ptr<DWARFCompileUnit> DWOCU(new DWARFCompileUnit(*this,
-        getDebugAbbrevDWO(), getInfoDWOSection().Data, getRangeDWOSection(),
-        getStringDWOSection(), getStringOffsetDWOSection(), getAddrSection(),
-        &getInfoDWOSection().Relocs, isLittleEndian(), DWOCUs));
-    if (!DWOCU->extract(DIData, &offset)) {
-      break;
-    }
-    DWOCUs.push_back(std::move(DWOCU));
-    offset = DWOCUs.back()->getNextUnitOffset();
-  }
+  DWOCUs.parseDWO(*this, getInfoDWOSection().Data, getInfoDWOSection().Relocs);
 }
 
 void DWARFContext::parseDWOTypeUnits() {
   if (!DWOTUs.empty())
     return;
   for (const auto &I : getTypesDWOSections()) {
-    uint32_t offset = 0;
-    const DataExtractor &DIData =
-        DataExtractor(I.second.Data, isLittleEndian(), 0);
     DWOTUs.push_back(DWARFUnitSection<DWARFTypeUnit>());
-    auto &TUS = DWOTUs.back();
-    while (DIData.isValidOffset(offset)) {
-      std::unique_ptr<DWARFTypeUnit> TU(new DWARFTypeUnit(*this,
-          getDebugAbbrevDWO(), I.second.Data, getRangeDWOSection(),
-          getStringDWOSection(), getStringOffsetDWOSection(), getAddrSection(),
-          &I.second.Relocs, isLittleEndian(), TUS));
-      if (!TU->extract(DIData, &offset))
-        break;
-      TUS.push_back(std::move(TU));
-      offset = TUS.back()->getNextUnitOffset();
-    }
+    DWOTUs.back().parseDWO(*this, I.second.Data, I.second.Relocs);
   }
 }
 
 DWARFCompileUnit *DWARFContext::getCompileUnitForOffset(uint32_t Offset) {
   parseCompileUnits();
   return CUs.getUnitForOffset(Offset);
 }
 

lib/DebugInfo/DWARFUnit.h

 /// Base class for all DWARFUnitSection classes. This provides the
 /// functionality common to all unit types.
 class DWARFUnitSectionBase {
 public:
   /// Returns the Unit that contains the given section offset in the
   /// same section this Unit originated from.
   virtual DWARFUnit *getUnitForOffset(uint32_t Offset) const = 0;
 
+  void parse(DWARFContext &C, StringRef SectionData, const RelocAddrMap &Map);
+  void parseDWO(DWARFContext &C, StringRef SectionData, const RelocAddrMap &Map);
+
 protected:
+  virtual void parseImpl(DWARFContext &Context, const DWARFDebugAbbrev *DA,
+                         StringRef Section, StringRef RS, StringRef SS,
+                         StringRef SOS, StringRef AOS, const RelocAddrMap &M,
+                         bool isLittleEndian) = 0;
+
   ~DWARFUnitSectionBase() {}
 };
 
 /// Concrete instance of DWARFUnitSection, specialized for one Unit type.
 template<typename UnitType>
 class DWARFUnitSection final : public SmallVector<std::unique_ptr<UnitType>, 1>,
                                public DWARFUnitSectionBase {
 
   struct UnitOffsetComparator {
     bool operator()(uint32_t LHS,
                     const std::unique_ptr<UnitType> &RHS) const {
       return LHS < RHS->getNextUnitOffset();
     }
   };
 
+  bool Parsed;
+
 public:
-  DWARFUnitSection() {}
+  DWARFUnitSection() : Parsed(false) {}
   DWARFUnitSection(DWARFUnitSection &&DUS) :
-    SmallVector<std::unique_ptr<UnitType>, 1>(std::move(DUS)) {}
+    SmallVector<std::unique_ptr<UnitType>, 1>(std::move(DUS)), Parsed(DUS.Parsed) {}
 
   typedef llvm::SmallVectorImpl<std::unique_ptr<UnitType>> UnitVector;
   typedef typename UnitVector::iterator iterator;
   typedef llvm::iterator_range<typename UnitVector::iterator> iterator_range;
 
   UnitType *getUnitForOffset(uint32_t Offset) const {
     auto *CU = std::upper_bound(this->begin(), this->end(), Offset,
                                 UnitOffsetComparator());
     if (CU != this->end())
       return CU->get();
     return nullptr;
   }
+
+ private:
+  void parseImpl(DWARFContext &Context, const DWARFDebugAbbrev *DA,
+                 StringRef Section, StringRef RS, StringRef SS, StringRef SOS,
+                 StringRef AOS, const RelocAddrMap &M, bool LE) override {
+    if (Parsed)
+      return;
+    DataExtractor Data(Section, LE, 0);
+    uint32_t Offset = 0;
+    while (Data.isValidOffset(Offset)) {
+      auto U = make_unique<UnitType>(Context, DA, Section, RS, SS, SOS, AOS, &M,
+                                     Data.isLittleEndian(), *this);
+      if (!U->extract(Data, &Offset))
+        break;
+      this->push_back(std::move(U));
+      Offset = this->back()->getNextUnitOffset();
+    }
+    Parsed = true;
+  }
 };
 
 class DWARFUnit {
   DWARFContext &Context;
 
   const DWARFDebugAbbrev *Abbrev;
   StringRef InfoSection;
   StringRef RangeSection;

lib/DebugInfo/DWARFUnit.cpp

 #include "llvm/DebugInfo/DWARFFormValue.h"
 #include "llvm/Support/Dwarf.h"
 #include "llvm/Support/Path.h"
 #include <cstdio>
 
 using namespace llvm;
 using namespace dwarf;
 
+
+void DWARFUnitSectionBase::parse(DWARFContext &C, StringRef SectionData,
+                                 const RelocAddrMap &Map) {
+  parseImpl(C, C.getDebugAbbrev(), SectionData, C.getRangeSection(),
+            C.getStringSection(), StringRef(), C.getAddrSection(), Map,
+            C.isLittleEndian());
+}
+
+void DWARFUnitSectionBase::parseDWO(DWARFContext &C, StringRef SectionData,
+                                    const RelocAddrMap &Map) {
+  parseImpl(C, C.getDebugAbbrevDWO(), SectionData, C.getRangeDWOSection(),
+            C.getStringDWOSection(), C.getStringOffsetDWOSection(),
+            C.getAddrSection(), Map, C.isLittleEndian());
+}
+
 DWARFUnit::DWARFUnit(DWARFContext &DC, const DWARFDebugAbbrev *DA,
                      StringRef IS, StringRef RS, StringRef SS, StringRef SOS,
                      StringRef AOS, const RelocAddrMap *M, bool LE,
                      const DWARFUnitSectionBase& UnitSection)
   : Context(DC), Abbrev(DA), InfoSection(IS), RangeSection(RS),
     StringSection(SS), StringOffsetSection(SOS), AddrOffsetSection(AOS),
     RelocMap(M), isLittleEndian(LE), UnitSection(UnitSection)  {
   clear();