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

188

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?

190

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)

216

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

[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

188

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.

190

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.

216

As said above, it doesn't work.

[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

202

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

 //===-- DWARFCompileUnit.h --------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_LIB_DEBUGINFO_DWARFCOMPILEUNIT_H
 #define LLVM_LIB_DEBUGINFO_DWARFCOMPILEUNIT_H
 
 #include "DWARFUnit.h"
 
 namespace llvm {
 
-class DWARFCompileUnit : public DWARFUnit {
+class DWARFCompileUnit : public DWARFUnitImpl<DWARFCompileUnit> {
 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 UnitVector &Units)
+    : DWARFUnitImpl(Context, DA, IS, RS, SS, SOS, AOS, M, LE, Units) {}
   void dump(raw_ostream &OS);
   // VTable anchor.
   ~DWARFCompileUnit() override;
 };
 
 }
 
 #endif

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 DWARFUnit::getUnitForOffset<DWARFCompileUnit>(CUs, 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

 //===-- DWARFTypeUnit.h -----------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_LIB_DEBUGINFO_DWARFTYPEUNIT_H
 #define LLVM_LIB_DEBUGINFO_DWARFTYPEUNIT_H
 
 #include "DWARFUnit.h"
 
 namespace llvm {
 
-class DWARFTypeUnit : public DWARFUnit {
+class DWARFTypeUnit : public DWARFUnitImpl<DWARFTypeUnit> {
 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 UnitVector &Units)
+    : DWARFUnitImpl(Context, DA, IS, RS, SS, SOS, AOS, M, LE, Units) {}
   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

class DWARFUnit {
   public:
     DWOHolder(StringRef DWOPath);
     DWARFUnit *getUnit() const { return DWOU; }
   };
   std::unique_ptr<DWOHolder> DWO;
 
 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);
 
   virtual ~DWARFUnit();
 
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);
 
+  /// 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;
+
 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:
   /// parseDWO - Parses .dwo file for current compile unit. Returns true if
   /// it was actually constructed.
   bool parseDWO();
 
   /// 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);
+
+  template<class UnitType>
+  struct UnitOffsetComparator {
+    bool operator()(const std::unique_ptr<UnitType> &LHS,
+                    const std::unique_ptr<UnitType> &RHS) const {
+      return LHS->getNextUnitOffset() < RHS->getNextUnitOffset();
+    }
+    bool operator()(const std::unique_ptr<UnitType> &LHS,
+                    uint32_t RHS) const {
+      return LHS->getNextUnitOffset() < RHS;
+    }
+    bool operator()(uint32_t LHS,
+                    const std::unique_ptr<UnitType> &RHS) const {
+      return LHS < RHS->getNextUnitOffset();
+    }
+  };
+
+public:
+  template<typename UnitType>
+  static UnitType *
+  getUnitForOffset(const SmallVectorImpl<std::unique_ptr<UnitType>> &Units,
+                   uint32_t Offset) {
+    auto *Unit = std::upper_bound(Units.begin(), Units.end(), Offset,
+                                  UnitOffsetComparator<UnitType>());
+    if (Unit != Units.end())
+      return Unit->get();
+    return nullptr;
+  }
 };
 
+/// Small wrapper for the DWARFUnit class that allows to embed a
+/// reference to the list of all the units extracted from the same
+/// section as this one.
+template<class UnitType>
+class DWARFUnitImpl : public DWARFUnit {
+protected:
+  typedef SmallVectorImpl<std::unique_ptr<UnitType>> UnitVector;
+  /// This is a reference to the vector of all units extracted from
+  /// the same section as this. The vector is owned by the DWARFContext.
+  const UnitVector &Units;
+
+public:
+  DWARFUnitImpl(DWARFContext& Context, const DWARFDebugAbbrev *DA, StringRef IS,
+                StringRef RS, StringRef SS, StringRef SOS, StringRef AOS,
+                const RelocAddrMap *M, bool LE, const UnitVector &Units)
+    : DWARFUnit(Context, DA, IS, RS, SS, SOS, AOS, M, LE), Units(Units) {}
+
+  DWARFUnit *getUnitForOffset(uint32_t Offset) const override {
+    return DWARFUnit::getUnitForOffset<UnitType>(Units, Offset);
   }
+};
 
+}
 #endif