Simplify DWARFCompileUnit / DWARFTypeUnit constructors (NFC).

[samsonov]

Summary

Now that DWARFUnit constructor takes a reference to full DWARFContext,
we can choose between DWO- and non-DWO sections inside the constructor,
instead of passing a bunch of StringRefs around.

[friss]

This looks like a nice improvement to me. Some comments inline just for discussion, nothing to change.

Thanks!

lib/DebugInfo/DWARFUnit.cpp

29–37

We talked at one point of using inheritance to distinguish the DWO Units from the others, do you already have a plan for that? I ask because in my branch I tried various approaches to avoid all these conditionals by having the DWO/standard section selection done using a virtual method, but I couldn't come up with something that I found nicer than this (at which level of the type hierarchy do you introduce the DWO attribute for example?).

lib/DebugInfo/DWARFUnit.h

82–83

It's a pity that we can't get rid of the bool argument that is also a Context derivative, but it's already much nicer like this.

[dblaikie]

I'm not really sure I agree that this is an improvement.

I have a bit of an aversion to making something a dynamic choice (passing a bool down & then doing a bunch of conditionals) compared to a static choice (at the call site, we know which sections to retrieve).

[friss]

In D5659#7, @dblaikie wrote:

I'm not really sure I agree that this is an improvement.

I have a bit of an aversion to making something a dynamic choice (passing a bool down & then doing a bunch of conditionals) compared to a static choice (at the call site, we know which sections to retrieve).

I always disliked the huge list of acronym parameters that got passed to these constructor methods, and I stylistically prefer the proposed patch. But as I commented, the bunch of conditionals isn't that nice either and if someone can come up with a nice solution that combines the best of both worlds then I'm all for it.

Are you concerned about the performance implications of making this choice dynamic or just objecting that something that can be static should be static?

[dblaikie]

In D5659#8, @friss wrote:

In D5659#7, @dblaikie wrote:

I'm not really sure I agree that this is an improvement.

I have a bit of an aversion to making something a dynamic choice (passing a bool down & then doing a bunch of conditionals) compared to a static choice (at the call site, we know which sections to retrieve).

I always disliked the huge list of acronym parameters that got passed to these constructor methods, and I stylistically prefer the proposed patch. But as I commented, the bunch of conditionals isn't that nice either and if someone can come up with a nice solution that combines the best of both worlds then I'm all for it.

Are you concerned about the performance implications of making this choice dynamic or just objecting that something that can be static should be static?

Not concerned about performance implications - mostly just that the static choice seemed to be made where the information needed for the choice existed - and to plumb that information down a few layers, unnecessarily deferring the choice (it's not like we already had a separate DWO/O abstraction - we actually had to push the choice down with the boolean flag) doesn't seem great.

If the problem is just the number of arguments - we can wrap that in a struct and achieve the same terseness without pushing the choice down into layers that are otherwise agnostic of that detail.

[friss]

In D5659#9, @dblaikie wrote

:> If the problem is just the number of arguments - we can wrap that in a struct and achieve the same terseness without pushing the choice down into layers that are otherwise agnostic of that detail.

It's not only about the number of arguments, I like the fact that the Unit choses the sections it needs. I find it more logical from an OO POV than having a helper in DWARFUnitSection make the choice. Ideally, I'd like to get rid of parseDWO totally and have the DWOness be a Unit type attribute (that was the point of my comment regarding the type hierarchy). I have the feeling that this commit gets us one step closer to that.

But I can see your point though, won't argue if you reject the change.

[dblaikie]

In D5659#10, @friss wrote:

In D5659#9, @dblaikie wrote

:> If the problem is just the number of arguments - we can wrap that in a struct and achieve the same terseness without pushing the choice down into layers that are otherwise agnostic of that detail.

It's not only about the number of arguments, I like the fact that the Unit choses the sections it needs. I find it more logical from an OO POV than having a helper in DWARFUnitSection make the choice.

Though DWARFUnitSection is currently dwo/o-agnostic. Unless we were to make it type-specific (which we could do, making the current DWARFUnitSection a CRTP base, perhaps... - but now we've got to pivot in two directions & end up with 4 classes. {DWO,O}x{TypeUnit,CompileUnit}... which seems awkward, though possible. (note that the debug info emission code (lib/CodeGen/AsmPrinter) currently only pivots, in the inheritance hierarchy, by type/compile unit, not by .o/.dwo - though similarly, it's not a perfect fit and there are certainly times I'd like the other axis rather than a lot of "isSplitDwarf" checks)

Ideally, I'd like to get rid of parseDWO totally and have the DWOness be a Unit type attribute (that was the point of my comment regarding the type hierarchy).

I'm not quite sure what you're describing when you say "a unit type attribute" - could you sketch out the ideal end state you have in mind (in terms of inheritance, reuse, where decisions get made, etc)?

I have the feeling that this commit gets us one step closer to that.

But I can see your point though, won't argue if you reject the change.

[samsonov]

In D5659#9, @dblaikie wrote:

In D5659#8, @friss wrote:

In D5659#7, @dblaikie wrote:

I'm not really sure I agree that this is an improvement.

I have a bit of an aversion to making something a dynamic choice (passing a bool down & then doing a bunch of conditionals) compared to a static choice (at the call site, we know which sections to retrieve).

I always disliked the huge list of acronym parameters that got passed to these constructor methods, and I stylistically prefer the proposed patch. But as I commented, the bunch of conditionals isn't that nice either and if someone can come up with a nice solution that combines the best of both worlds then I'm all for it.

Are you concerned about the performance implications of making this choice dynamic or just objecting that something that can be static should be static?

Not concerned about performance implications - mostly just that the static choice seemed to be made where the information needed for the choice existed - and to plumb that information down a few layers, unnecessarily deferring the choice (it's not like we already had a separate DWO/O abstraction - we actually had to push the choice down with the boolean flag) doesn't seem great.

Fair enough. I agree that it would be cleaner to first separate .dwo units from regular ones, and then proceed with this change, so that we don't have to pull conditionals into constructor code.

If the problem is just the number of arguments - we can wrap that in a struct and achieve the same terseness without pushing the choice down into layers that are otherwise agnostic of that detail.

lib/DebugInfo/DWARFUnit.cpp

29–37

I haven't actually tried to code it yet, so probably you're in a better position. I thought of introducing a DWARFDWOContext entity, that would contain .dwo counterparts of regular sections. DWARFDWOContext is contained inside a regular DWARFContext (to represent the case when .dwo sections are not yet moved away from the executable). On the other hand, we should be able to construct DWARFDWOContext directly from an object file (with .dwo extension) - this would be used in DWARFUnit::parseDWO() method, that locates and loads .dwo file for a given compilation unit.

We can make DWARFDWOContext implement the same set of methods as DWARFContext, e.g. DWARFDWOContext::getDebugAbbrev() will return .debug_abbrev.dwo section. Instead of calling DWARFContext.getDebugAbbrevDWO() one may call DWARFContext.getDWOContext().getDebugAbbrev().

[friss]

In D5659#11, @dblaikie wrote:

In D5659#10, @friss wrote:

In D5659#9, @dblaikie wrote

:> If the problem is just the number of arguments - we can wrap that in a struct and achieve the same terseness without pushing the choice down into layers that are otherwise agnostic of that detail.

It's not only about the number of arguments, I like the fact that the Unit choses the sections it needs. I find it more logical from an OO POV than having a helper in DWARFUnitSection make the choice.

Though DWARFUnitSection is currently dwo/o-agnostic. Unless we were to make it type-specific (which we could do, making the current DWARFUnitSection a CRTP base, perhaps... - but now we've got to pivot in two directions & end up with 4 classes. {DWO,O}x{TypeUnit,CompileUnit}... which seems awkward, though possible. (note that the debug info emission code (lib/CodeGen/AsmPrinter) currently only pivots, in the inheritance hierarchy, by type/compile unit, not by .o/.dwo - though similarly, it's not a perfect fit and there are certainly times I'd like the other axis rather than a lot of "isSplitDwarf" checks)

Ideally, I'd like to get rid of parseDWO totally and have the DWOness be a Unit type attribute (that was the point of my comment regarding the type hierarchy).

I'm not quite sure what you're describing when you say "a unit type attribute" - could you sketch out the ideal end state you have in mind (in terms of inheritance, reuse, where decisions get made, etc)?

The idea would be to push the decision further down in the Unit type. This would remove the need for the parseDWO helper, the high level code would just call a generic parse method on the UnitSection. I tried to do something like that but I wasn't satisfied by the end result (I introduced some kind of DWO trait template argument to DWARFUnit to avoid duplicating the code, but then the iterator types got ugly and moreover I ended up with 2 virtual calls per call to parse). So I don't have a precise idea of the outcome, but Alexey was the first one coming up with the idea of specializing the Unit types to account for the DWO specificities thus I thought this patch was the first of his grand plan to make that happen :-)

lib/DebugInfo/DWARFCompileUnit.h

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

lib/DebugInfo/DWARFTypeUnit.h

 
 namespace llvm {
 
 class DWARFTypeUnit : public DWARFUnit {
 private:
   uint64_t TypeHash;
   uint32_t TypeOffset;
 public:
-  DWARFTypeUnit(DWARFContext &Context, const DWARFSection &Section,
-                const DWARFDebugAbbrev *DA, StringRef RS, StringRef SS,
-                StringRef SOS, StringRef AOS, bool LE,
+  DWARFTypeUnit(DWARFContext &Context, const DWARFSection &Section, bool IsDWO,
                 const DWARFUnitSectionBase &UnitSection)
-      : DWARFUnit(Context, Section, DA, RS, SS, SOS, AOS, LE, UnitSection) {}
+      : DWARFUnit(Context, Section, IsDWO, 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

public:
   /// same section this Unit originated from.
   virtual DWARFUnit *getUnitForOffset(uint32_t Offset) const = 0;
 
   void parse(DWARFContext &C, const DWARFSection &Section);
   void parseDWO(DWARFContext &C, const DWARFSection &DWOSection);
 
 protected:
   virtual void parseImpl(DWARFContext &Context, const DWARFSection &Section,
-                         const DWARFDebugAbbrev *DA, StringRef RS, StringRef SS,
-                         StringRef SOS, StringRef AOS, bool isLittleEndian) = 0;
+                         bool IsDWO, 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 {
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 DWARFSection &Section,
-                 const DWARFDebugAbbrev *DA, StringRef RS, StringRef SS,
-                 StringRef SOS, StringRef AOS, bool LE) override {
+  void parseImpl(DWARFContext &Context, const DWARFSection &Section, bool IsDWO,
+                 bool LE) override {
     if (Parsed)
       return;
     DataExtractor Data(Section.Data, LE, 0);
     uint32_t Offset = 0;
     while (Data.isValidOffset(Offset)) {
-      auto U = llvm::make_unique<UnitType>(Context, Section, DA, RS, SS, SOS,
-                                           AOS, LE, *this);
+      auto U = llvm::make_unique<UnitType>(Context, Section, IsDWO, *this);
       if (!U->extract(Data, &Offset))
         break;
       this->push_back(std::move(U));
       Offset = this->back()->getNextUnitOffset();
     }
     Parsed = true;
   }
 };
class DWARFUnit {
   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 DWARFSection &Section,
-            const DWARFDebugAbbrev *DA, StringRef RS, StringRef SS,
-            StringRef SOS, StringRef AOS, bool LE,
+  DWARFUnit(DWARFContext &Context, const DWARFSection &Section, bool IsDWO,
             const DWARFUnitSectionBase &UnitSection);
 
   virtual ~DWARFUnit();
 
   DWARFContext& getContext() const { return Context; }
 
   StringRef getStringSection() const { return StringSection; }
   StringRef getStringOffsetSection() const { return StringOffsetSection; }

lib/DebugInfo/DWARFUnit.cpp

 #include "llvm/Support/Dwarf.h"
 #include "llvm/Support/Path.h"
 #include <cstdio>
 
 using namespace llvm;
 using namespace dwarf;
 
 void DWARFUnitSectionBase::parse(DWARFContext &C, const DWARFSection &Section) {
-  parseImpl(C, Section, C.getDebugAbbrev(), C.getRangeSection(),
-            C.getStringSection(), StringRef(), C.getAddrSection(),
-            C.isLittleEndian());
+  parseImpl(C, Section, false, C.isLittleEndian());
 }
 
 void DWARFUnitSectionBase::parseDWO(DWARFContext &C,
                                     const DWARFSection &DWOSection) {
-  parseImpl(C, DWOSection, C.getDebugAbbrevDWO(), C.getRangeDWOSection(),
-            C.getStringDWOSection(), C.getStringOffsetDWOSection(),
-            C.getAddrSection(), C.isLittleEndian());
+  parseImpl(C, DWOSection, true, C.isLittleEndian());
 }
 
-DWARFUnit::DWARFUnit(DWARFContext &DC, const DWARFSection &Section,
-                     const DWARFDebugAbbrev *DA, StringRef RS, StringRef SS,
-                     StringRef SOS, StringRef AOS, bool LE,
+DWARFUnit::DWARFUnit(DWARFContext &DC, const DWARFSection &Section, bool IsDWO,
                      const DWARFUnitSectionBase &UnitSection)
-    : Context(DC), InfoSection(Section), Abbrev(DA), RangeSection(RS),
-      StringSection(SS), StringOffsetSection(SOS), AddrOffsetSection(AOS),
-      isLittleEndian(LE), UnitSection(UnitSection) {
+    : Context(DC), InfoSection(Section),
+      Abbrev(IsDWO ? DC.getDebugAbbrevDWO() : DC.getDebugAbbrev()),
+      RangeSection(IsDWO ? DC.getRangeDWOSection() : DC.getRangeSection()),
+      StringSection(IsDWO ? DC.getStringDWOSection() : DC.getStringSection()),
+      StringOffsetSection(IsDWO ? DC.getStringOffsetDWOSection() : StringRef()),
+      AddrOffsetSection(DC.getAddrSection()),
+      isLittleEndian(DC.isLittleEndian()), UnitSection(UnitSection) {
   clear();
 }
 
 DWARFUnit::~DWARFUnit() {
 }
 
 bool DWARFUnit::getAddrOffsetSectionItem(uint32_t Index,
                                                 uint64_t &Result) const {