Make getUnitForOffset a DWARFUnit method instead of a DWARFContext method.

[friss]

Summary

This commit moves the Unit lookup code from the DWARFContext into
a static member of the DWARFUnit. Using a CRTP, each Unit embeds
a reference to the vector of all Units extracted from the same
section. This vector is qureried in getUnitForOffset in order to
be able to extract DIEs referenced from a foreign Unit (this happens
with LTO type uniquing for example). This works whatever the Unit's
origin: .debug_info, .debug_types or their dwo variants.

The DWARFContext API is left untouched by forwarding it lookup code
to the DWARFUnit static member.

This code will be exercised/tested in a followup patch that makes
dwarfdump emit the name for DW_AT_abstract_origin and DW_AT_specification
attributes.

Note that this commit contains a fixed version of the unit lookup code
that hasn't been approved yet.

[dblaikie]

lib/DebugInfo/DWARFContext.cpp

394

You shouldn't need to specify UnitType here, should you? Template argument deduction should fire on the first parameter. (similarly at the other call site)

lib/DebugInfo/DWARFUnit.h

217

You shouldn't need to specify UnitType here, should you? Template argument deduction should fire on the first parameter.

232

This doesn't necessarily have to be moved to the DWARFUnit - it could remain in the DWARFContext - though perhaps there's a circular referencing problem that makes this more convenient/possible here rather than where it was originally?

234

This looks like it's also changing the semantics based on one of your other code reviews - it'd be good to keep these things separate. (similarly I see the getOffset -> getNextUnitOffset change in the UnitOffsetComparator above)

[friss]

lib/DebugInfo/DWARFContext.cpp

394

No, type deduction doesn't seem to work here. Just a guess: the implicit conversion from SmallVector to SmallVectorImpl might be an issue?

lib/DebugInfo/DWARFUnit.h

217

As said above, it doesn't work.

232

Honestly, when I moved it to a header, I just thought that something comparing Units is better suited in the DWARFUnit class. Now that you mention it there is actually a circular include issue that prevents it from being in the DWARFContext header.

234

Yes, I mentioned it it the revision log. I needed a working patch to test it out, but if you agree that this is what you want I can easily respin it with the same semantic as before.

[samsonov]

This doesn't seem right to me. DWARFUnit shouldn't know or keep track of another units in the same section.

[samsonov]

Oh, I see that this was suggested in http://reviews.llvm.org/D5264, which I lost track of. Let me catch up with that thread.

[samsonov]

lib/DebugInfo/DWARFUnit.h

218

See my suggestion in D5264 review thread. You can introduce a new template class DWARFUnitSection<T>,
which would be a wrapper around SmallVector<std::unique_ptr<T>>, and have
DWARFCompileUnit hold a reference to DWARFUnitSection<DWARFCompileUnit>,
and DWARFTypeUnit hold a reference to DWARFUnitSection<DWARFTypeUnit>.

Then instead of introducing Unit::getUnitForOffset(), you can have an (abstract) DWARFUnit::getSection(), and
DWARFUnitSection::getUnitForOffset().

[friss]

Rewrite the patch according to Alexey's comments.

This patch introduces the DWARFUnitSection abstraction that is a collection
of units with a generic base class allowing to query for the Unit containing
a certain offset.

This patch should induce no functional change.

[dblaikie]

Looks good to me once the two minor comments are addressed.

Thanks for your patience!

lib/DebugInfo/DWARFUnit.h

65

Missing override

66

drop the '*' - SmallVector's iterators shouldn't be assumed to be pointers (we could one day have a checked iterator implementation with assertions, etc)

[samsonov]

LGTM. I'd rather not make DWARFUnitSection inherit from SmallVector - unit section is not a vector, vector is just a way we represent a set of units, and I'm not sure we want to expose all SmallVector accessors to DWARFUnitSection users. This change is fine for now, though. We will be able to implement correct accessors and/or introduce DWARFUnitSection::parse() function later as needed.

Thanks!

[friss]

Yeah, I did the minimal patch. Having it inherit from SmallVector allows to keep most of the other code (mostly in the Context) untouched. You'd have to implement a bunch of proxy methods to make the Context happy while not inheriting from the Vector, but it's doable as a later step if it's the road we want to go.

[samsonov]

Sure. Please outline this in the commit message and proceed.

[dblaikie]

In D5310#14, @samsonov wrote:

LGTM. I'd rather not make DWARFUnitSection inherit from SmallVector - unit section is not a vector, vector is just a way we represent a set of units, and I'm not sure we want to expose all SmallVector accessors to DWARFUnitSection users.

'spose I should've noticed that, though I didn't. Agreed with your sentiments. :/

The derived template can have a bunch of accessors without exposing them to the other clients.

[friss]

Closed by r217747 and its companion fix commit r217751

lib/DebugInfo/DWARFCompileUnit.h

 #include "DWARFUnit.h"
 
 namespace llvm {
 
 class DWARFCompileUnit : public DWARFUnit {
 public:
   DWARFCompileUnit(DWARFContext& Context, const DWARFDebugAbbrev *DA,
                    StringRef IS, StringRef RS, StringRef SS, StringRef SOS,
-                   StringRef AOS, const RelocAddrMap *M, bool LE)
-    : DWARFUnit(Context, DA, IS, RS, SS, SOS, AOS, M, LE) {}
+                   StringRef AOS, const RelocAddrMap *M, bool LE,
+                   const DWARFUnitSectionBase &UnitSection)
+    : DWARFUnit(Context, DA, IS, RS, SS, SOS, AOS, M, LE, UnitSection) {}
   void dump(raw_ostream &OS);
   // VTable anchor.
   ~DWARFCompileUnit() override;
 };
 
 }
 
 #endif

lib/DebugInfo/DWARFContext.h

 
 namespace llvm {
 
 /// DWARFContext
 /// This data structure is the top level entity that deals with dwarf debug
 /// information parsing. The actual data is supplied through pure virtual
 /// methods that a concrete implementation provides.
 class DWARFContext : public DIContext {
-  typedef SmallVector<std::unique_ptr<DWARFCompileUnit>, 1> CUVector;
-  typedef SmallVector<std::unique_ptr<DWARFTypeUnit>, 1> TUVector;
 
-  CUVector CUs;
-  TUVector TUs;
+  DWARFUnitSection<DWARFCompileUnit> CUs;
+  DWARFUnitSection<DWARFTypeUnit> TUs;
   std::unique_ptr<DWARFDebugAbbrev> Abbrev;
   std::unique_ptr<DWARFDebugLoc> Loc;
   std::unique_ptr<DWARFDebugAranges> Aranges;
   std::unique_ptr<DWARFDebugLine> Line;
   std::unique_ptr<DWARFDebugFrame> DebugFrame;
 
-  CUVector DWOCUs;
-  TUVector DWOTUs;
+  DWARFUnitSection<DWARFCompileUnit> DWOCUs;
+  DWARFUnitSection<DWARFTypeUnit> DWOTUs;
   std::unique_ptr<DWARFDebugAbbrev> AbbrevDWO;
   std::unique_ptr<DWARFDebugLocDWO> LocDWO;
 
   DWARFContext(DWARFContext &) LLVM_DELETED_FUNCTION;
   DWARFContext &operator=(DWARFContext &) LLVM_DELETED_FUNCTION;
 
   /// Read compile units from the debug_info section (if necessary)
   /// and store them in CUs.
public:
   DWARFContext() : DIContext(CK_DWARF) {}
 
   static bool classof(const DIContext *DICtx) {
     return DICtx->getKind() == CK_DWARF;
   }
 
   void dump(raw_ostream &OS, DIDumpType DumpType = DIDT_All) override;
 
-  typedef iterator_range<CUVector::iterator> cu_iterator_range;
-  typedef iterator_range<TUVector::iterator> tu_iterator_range;
+  typedef DWARFUnitSection<DWARFCompileUnit>::iterator_range cu_iterator_range;
+  typedef DWARFUnitSection<DWARFTypeUnit>::iterator_range tu_iterator_range;
 
   /// Get compile units in this context.
   cu_iterator_range compile_units() {
     parseCompileUnits();
     return cu_iterator_range(CUs.begin(), CUs.end());
   }
 
   /// Get type units in this context.

lib/DebugInfo/DWARFContext.cpp

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()));
+        &getInfoSection().Relocs, isLittleEndian(), CUs));
     if (!CU->extract(DIData, &offset)) {
       break;
     }
     CUs.push_back(std::move(CU));
     offset = CUs.back()->getNextUnitOffset();
   }
 }
 
 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);
     while (DIData.isValidOffset(offset)) {
       std::unique_ptr<DWARFTypeUnit> TU(new DWARFTypeUnit(*this,
            getDebugAbbrev(), I.second.Data, getRangeSection(),
            getStringSection(), StringRef(), getAddrSection(),
-           &I.second.Relocs, isLittleEndian()));
+           &I.second.Relocs, isLittleEndian(), TUs));
       if (!TU->extract(DIData, &offset))
         break;
       TUs.push_back(std::move(TU));
       offset = TUs.back()->getNextUnitOffset();
     }
   }
 }
 
 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()));
+        &getInfoDWOSection().Relocs, isLittleEndian(), DWOCUs));
     if (!DWOCU->extract(DIData, &offset)) {
       break;
     }
     DWOCUs.push_back(std::move(DWOCU));
     offset = DWOCUs.back()->getNextUnitOffset();
   }
 }
 
 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);
     while (DIData.isValidOffset(offset)) {
       std::unique_ptr<DWARFTypeUnit> TU(new DWARFTypeUnit(*this,
           getDebugAbbrevDWO(), I.second.Data, getRangeDWOSection(),
           getStringDWOSection(), getStringOffsetDWOSection(), getAddrSection(),
-          &I.second.Relocs, isLittleEndian()));
+          &I.second.Relocs, isLittleEndian(), DWOTUs));
       if (!TU->extract(DIData, &offset))
         break;
       DWOTUs.push_back(std::move(TU));
       offset = DWOTUs.back()->getNextUnitOffset();
     }
   }
 }
 
-namespace {
-  struct OffsetComparator {
-
-    bool operator()(const std::unique_ptr<DWARFCompileUnit> &LHS,
-                    const std::unique_ptr<DWARFCompileUnit> &RHS) const {
-      return LHS->getOffset() < RHS->getOffset();
-    }
-    bool operator()(const std::unique_ptr<DWARFCompileUnit> &LHS,
-                    uint32_t RHS) const {
-      return LHS->getOffset() < RHS;
-    }
-    bool operator()(uint32_t LHS,
-                    const std::unique_ptr<DWARFCompileUnit> &RHS) const {
-      return LHS < RHS->getOffset();
-    }
-  };
-}
-
 DWARFCompileUnit *DWARFContext::getCompileUnitForOffset(uint32_t Offset) {
   parseCompileUnits();
-
-  std::unique_ptr<DWARFCompileUnit> *CU =
-      std::lower_bound(CUs.begin(), CUs.end(), Offset, OffsetComparator());
-  if (CU != CUs.end()) {
-    return CU->get();
-  }
-  return nullptr;
+  return CUs.getUnitForOffset(Offset);
 }
 
 DWARFCompileUnit *DWARFContext::getCompileUnitForAddress(uint64_t Address) {
   // First, get the offset of the compile unit.
   uint32_t CUOffset = getDebugAranges()->findAddress(Address);
   // Retrieve the compile unit.
   return getCompileUnitForOffset(CUOffset);
 }

lib/DebugInfo/DWARFTypeUnit.h

 
 class DWARFTypeUnit : public DWARFUnit {
 private:
   uint64_t TypeHash;
   uint32_t TypeOffset;
 public:
   DWARFTypeUnit(DWARFContext &Context, const DWARFDebugAbbrev *DA,
                 StringRef IS, StringRef RS, StringRef SS, StringRef SOS,
-                StringRef AOS, const RelocAddrMap *M, bool LE)
-    : DWARFUnit(Context, DA, IS, RS, SS, SOS, AOS, M, LE) {}
+                StringRef AOS, const RelocAddrMap *M, bool LE,
+                const DWARFUnitSectionBase &UnitSection)
+    : DWARFUnit(Context, DA, IS, RS, SS, SOS, AOS, M, LE, UnitSection) {}
   uint32_t getHeaderSize() const override {
     return DWARFUnit::getHeaderSize() + 12;
   }
   void dump(raw_ostream &OS);
 protected:
   bool extractImpl(DataExtractor debug_info, uint32_t *offset_ptr) override;
 };
 
 }
 
 #endif
 

lib/DebugInfo/DWARFUnit.h

 namespace object {
 class ObjectFile;
 }
 
 class DWARFContext;
 class DWARFDebugAbbrev;
 class StringRef;
 class raw_ostream;
+class DWARFUnit;
+
+/// 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;
+};
+
+/// Concrete instance of DWARFUnitSection, specialized for one Unit type.
+template<typename UnitType>
+class DWARFUnitSection : public SmallVector<std::unique_ptr<UnitType>, 1>,
+                         public DWARFUnitSectionBase {
+
+  struct UnitOffsetComparator {
+    bool operator()(const std::unique_ptr<UnitType> &LHS,
+                    const std::unique_ptr<UnitType> &RHS) const {
+      return LHS->getOffset() < RHS->getOffset();
+    }
+    bool operator()(const std::unique_ptr<UnitType> &LHS,
+                    uint32_t RHS) const {
+      return LHS->getOffset() < RHS;
+    }
+    bool operator()(uint32_t LHS,
+                    const std::unique_ptr<UnitType> &RHS) const {
+      return LHS < RHS->getOffset();
+    }
+  };
+
+public:
+  typedef SmallVectorImpl<std::unique_ptr<UnitType>> UnitVector;
+  typedef typename UnitVector::iterator iterator;
+  typedef iterator_range<typename UnitVector::iterator> iterator_range;
+
+  UnitType *getUnitForOffset(uint32_t Offset) const {
+    auto *CU = std::lower_bound(this->begin(), this->end(), Offset,
+                                UnitOffsetComparator());
+    if (CU != this->end())
+      return CU->get();
+    return nullptr;
+  }
+};
 
 class DWARFUnit {
   DWARFContext &Context;
 
   const DWARFDebugAbbrev *Abbrev;
   StringRef InfoSection;
   StringRef RangeSection;
   uint32_t RangeSectionBase;
   StringRef StringSection;
   StringRef StringOffsetSection;
   StringRef AddrOffsetSection;
   uint32_t AddrOffsetSectionBase;
   const RelocAddrMap *RelocMap;
   bool isLittleEndian;
+  const DWARFUnitSectionBase &UnitSection;
 
   uint32_t Offset;
   uint32_t Length;
   uint16_t Version;
   const DWARFAbbreviationDeclarationSet *Abbrevs;
   uint8_t AddrSize;
   uint64_t BaseAddr;
   // The compile unit debug information entry items.

 protected:
   virtual bool extractImpl(DataExtractor debug_info, uint32_t *offset_ptr);
   /// Size in bytes of the unit header.
   virtual uint32_t getHeaderSize() const { return 11; }
 
 public:
   DWARFUnit(DWARFContext& Context, const DWARFDebugAbbrev *DA, StringRef IS,
             StringRef RS, StringRef SS, StringRef SOS, StringRef AOS,
-            const RelocAddrMap *M, bool LE);
+            const RelocAddrMap *M, bool LE, const DWARFUnitSectionBase &UnitSection);
 
   virtual ~DWARFUnit();
 
   DWARFContext& getContext() const { return Context; }
 
   StringRef getStringSection() const { return StringSection; }
   StringRef getStringOffsetSection() const { return StringOffsetSection; }
   void setAddrOffsetSection(StringRef AOS, uint32_t Base) {
public:
 
   void collectAddressRanges(DWARFAddressRangesVector &CURanges);
 
   /// getInlinedChainForAddress - fetches inlined chain for a given address.
   /// Returns empty chain if there is no subprogram containing address. The
   /// chain is valid as long as parsed compile unit DIEs are not cleared.
   DWARFDebugInfoEntryInlinedChain getInlinedChainForAddress(uint64_t Address);
 
+  /// getUnitSection - Return the DWARFUnitSection containing this unit.
+  const DWARFUnitSectionBase &getUnitSection() const { return UnitSection; }
+
 private:
   /// Size in bytes of the .debug_info data associated with this compile unit.
   size_t getDebugInfoSize() const { return Length + 4 - getHeaderSize(); }
 
   /// extractDIEsIfNeeded - Parses a compile unit and indexes its DIEs if it
   /// hasn't already been done. Returns the number of DIEs parsed at this call.
   size_t extractDIEsIfNeeded(bool CUDieOnly);
   /// extractDIEsToVector - Appends all parsed DIEs to a vector.
private:
   /// getSubprogramForAddress - Returns subprogram DIE with address range
   /// encompassing the provided address. The pointer is alive as long as parsed
   /// compile unit DIEs are not cleared.
   const DWARFDebugInfoEntryMinimal *getSubprogramForAddress(uint64_t Address);
 };
 
 }
 
 #endif

lib/DebugInfo/DWARFUnit.cpp

 #include "llvm/Support/Path.h"
 #include <cstdio>
 
 using namespace llvm;
 using namespace dwarf;
 
 DWARFUnit::DWARFUnit(DWARFContext &DC, const DWARFDebugAbbrev *DA,
                      StringRef IS, StringRef RS, StringRef SS, StringRef SOS,
-                     StringRef AOS, const RelocAddrMap *M, bool LE)
+                     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) {
+    RelocMap(M), isLittleEndian(LE), UnitSection(UnitSection)  {
   clear();
 }
 
 DWARFUnit::~DWARFUnit() {
 }
 
 bool DWARFUnit::getAddrOffsetSectionItem(uint32_t Index,
                                                 uint64_t &Result) const {