DebugInfo: Filter DWARFv5 TUs out of the debug_info unit list when CUs requested

[dblaikie]

Summary

Since DWARFv5 places TUs in debug_info, some of DWARFContext's APIs have
become a bit erroneous, including TUs in the CU list by accident.

Implementation: Use a filtering iterator when walking the debug_info units looking for CUs.

Drawback: The range type is now different from other unit queries, and the iteration may be slightly slower due to the filtering.

Trying to pick between this solution and D87937

[jhenderson]

My instinct is that I prefer a third approach - simply keep both combined and separate lists - the combined list (could be a vector or an ordered map) is the offset-ordered list of all units, whilst there are then separate lists for compile and type units, with members being pointers to the units stored in the primary list. I think this has the advantage of being very explicit about things, with the drawback being that there are multiple members to keep in sync (possibly alleviated by having a wrapper class around the containers).

If I had to pick between the two existing options, I think I have a marginal preference for this one, since it requires less code and is less surprising to someone who might not be too familiar with the code - I would expect a list of units to be in offset order naturally, and not expect to have to go through some sort of sorting procedure in the future.

[dblaikie]

In D87935#2284630, @jhenderson wrote:

My instinct is that I prefer a third approach - simply keep both combined and separate lists - the combined list (could be a vector or an ordered map) is the offset-ordered list of all units, whilst there are then separate lists for compile and type units, with members being pointers to the units stored in the primary list. I think this has the advantage of being very explicit about things, with the drawback being that there are multiple members to keep in sync (possibly alleviated by having a wrapper class around the containers).

My concern there is higher memory usage/processing time - something I'm currently trying to combat. (& in fact, considering how a broader refactoring of libDebugInfoDWARF might look - to avoid parsing things a given client isn't using - I guess by that token, it might be nice to be able to read just the compile units and skip over the type units entirely - though, then if someone does ask for the debug_info units you'd need to go back and parse them to build a second list). I guess if you're OK with the lists being independently lazily built (well, not entirely independent - they would share any elements that overlap... - that'd make ownershp difficult, though), maybe it'd make sense that way. I guess alternatively I could start experimenting with a fully lazy parsing that doesn't keep a list at all - just walks the list, building one at a time... though that's going to be way more involved than I can get into right now (cross-CU referencing, etc).

If I had to pick between the two existing options, I think I have a marginal preference for this one, since it requires less code and is less surprising to someone who might not be too familiar with the code - I would expect a list of units to be in offset order naturally, and not expect to have to go through some sort of sorting procedure in the future.

Fair - appreciate the perspective. One other wrinkle here is that if we support the "type_units()" API (I don't think anyone's actually using it, so I'd probably delete it for now) it'll need to use a fancy iterator solution too - concat_iterator + filter_iterator to take the v5 type units out of the debug_info units, and concat that with the v4 type units from debug_types.

[jhenderson]

In D87935#2285841, @dblaikie wrote:

In D87935#2284630, @jhenderson wrote:

If I had to pick between the two existing options, I think I have a marginal preference for this one, since it requires less code and is less surprising to someone who might not be too familiar with the code - I would expect a list of units to be in offset order naturally, and not expect to have to go through some sort of sorting procedure in the future.

Fair - appreciate the perspective. One other wrinkle here is that if we support the "type_units()" API (I don't think anyone's actually using it, so I'd probably delete it for now) it'll need to use a fancy iterator solution too - concat_iterator + filter_iterator to take the v5 type units out of the debug_info units, and concat that with the v4 type units from debug_types.

I'm happy for that little extra complexity. I think it intuitively makes sense, even if it does mean messing about a little in the code.

llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h

166

Fixes the clang-tidy warning, and also more in keeping with isTypeUnit. (Also isCU would be acceptable).

[dblaikie]

In D87935#2289415, @jhenderson wrote:

In D87935#2285841, @dblaikie wrote:

In D87935#2284630, @jhenderson wrote:

If I had to pick between the two existing options, I think I have a marginal preference for this one, since it requires less code and is less surprising to someone who might not be too familiar with the code - I would expect a list of units to be in offset order naturally, and not expect to have to go through some sort of sorting procedure in the future.

Fair - appreciate the perspective. One other wrinkle here is that if we support the "type_units()" API (I don't think anyone's actually using it, so I'd probably delete it for now) it'll need to use a fancy iterator solution too - concat_iterator + filter_iterator to take the v5 type units out of the debug_info units, and concat that with the v4 type units from debug_types.

I'm happy for that little extra complexity. I think it intuitively makes sense, even if it does mean messing about a little in the code.

Fair enough. It's adequate for needs right now - though I'd like to revisit/keep in our mind the idea that maybe these APIs should all be a bit more lightweight/only-as-needed if we can head in that direction. (I'm concerned that to get there may not be readily made incremental, but we'll see - maybe we can pick away at it)

Appreciate the help considering the alternatives here!

llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h

166

Ah, sure thing - (& had a weird bit of leftover where I had two versions of this function (one here and one in DWARFUnit.h) & things were working anyway... which is vaguely unsettling - anyway, removed that duplication in the process of fixing the rename)

llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h

public:
   void dump(raw_ostream &OS, DIDumpOptions DumpOpts) override {
     std::array<Optional<uint64_t>, DIDT_ID_Count> DumpOffsets;
     dump(OS, DumpOpts, DumpOffsets);
   }
 
   bool verify(raw_ostream &OS, DIDumpOptions DumpOpts = {}) override;
 
   using unit_iterator_range = DWARFUnitVector::iterator_range;
+  using compile_unit_range = DWARFUnitVector::compile_unit_range;
 
   /// Get units from .debug_info in this context.
   unit_iterator_range info_section_units() {
     parseNormalUnits();
     return unit_iterator_range(NormalUnits.begin(),
                                NormalUnits.begin() +
                                    NormalUnits.getNumInfoUnits());
   }
 
   /// Get units from .debug_types in this context.
   unit_iterator_range types_section_units() {
     parseNormalUnits();
     return unit_iterator_range(
         NormalUnits.begin() + NormalUnits.getNumInfoUnits(), NormalUnits.end());
   }
 
+  static bool CUOnly(const std::unique_ptr<DWARFUnit> &U) {
+    return !U->isTypeUnit();
+  }
+
   /// Get compile units in this context.
-  unit_iterator_range compile_units() { return info_section_units(); }
+  compile_unit_range compile_units() {
+    return make_filter_range(info_section_units(), CUOnly);
+  }
 
   /// Get type units in this context.
   unit_iterator_range type_units() { return types_section_units(); }
 
   /// Get all normal compile/type units in this context.
   unit_iterator_range normal_units() {
     parseNormalUnits();
     return unit_iterator_range(NormalUnits.begin(), NormalUnits.end());
public:
   /// Get units from .debug_types.dwo in the DWO context.
   unit_iterator_range dwo_types_section_units() {
     parseDWOUnits();
     return unit_iterator_range(DWOUnits.begin() + DWOUnits.getNumInfoUnits(),
                                DWOUnits.end());
   }
 
   /// Get compile units in the DWO context.
-  unit_iterator_range dwo_compile_units() { return dwo_info_section_units(); }
+  compile_unit_range dwo_compile_units() {
+    return make_filter_range(dwo_info_section_units(), CUOnly);
+  }
 
   /// Get type units in the DWO context.
   unit_iterator_range dwo_type_units() { return dwo_types_section_units(); }
 
   /// Get all units in the DWO context.
   unit_iterator_range dwo_units() {
     parseDWOUnits();
     return unit_iterator_range(DWOUnits.begin(), DWOUnits.end());

llvm/include/llvm/DebugInfo/DWARF/DWARFDebugMacro.h

#include "llvm/BinaryFormat/Dwarf.def"
   std::vector<MacroList> MacroLists;
 
 public:
   DWARFDebugMacro() = default;
 
   /// Print the macro list found within the debug_macinfo/debug_macro section.
   void dump(raw_ostream &OS) const;
 
-  Error parseMacro(DWARFUnitVector::iterator_range Units,
+  Error parseMacro(DWARFUnitVector::compile_unit_range Units,
                    DataExtractor StringExtractor,
                    DWARFDataExtractor MacroData) {
     return parseImpl(Units, StringExtractor, MacroData, /*IsMacro=*/true);
   }
 
   Error parseMacinfo(DWARFDataExtractor MacroData) {
     return parseImpl(None, None, MacroData, /*IsMacro=*/false);
   }
 
   /// Return whether the section has any entries.
   bool empty() const { return MacroLists.empty(); }
 
 private:
   /// Parse the debug_macinfo/debug_macro section accessible via the 'MacroData'
   /// parameter.
-  Error parseImpl(Optional<DWARFUnitVector::iterator_range> Units,
+  Error parseImpl(Optional<DWARFUnitVector::compile_unit_range> Units,
                   Optional<DataExtractor> StringExtractor,
                   DWARFDataExtractor Data, bool IsMacro);
 };
 
 } // end namespace llvm
 
 #endif // LLVM_DEBUGINFO_DWARF_DWARFDEBUGMACRO_H

llvm/include/llvm/DebugInfo/DWARF/DWARFUnit.h

public:
   uint64_t getNextUnitOffset() const {
     return Offset + Length + getUnitLengthFieldByteSize();
   }
 };
 
 const DWARFUnitIndex &getDWARFUnitIndex(DWARFContext &Context,
                                         DWARFSectionKind Kind);
 
+bool isCU(const std::unique_ptr<DWARFUnit> &U);
+
 /// Describe a collection of units. Intended to hold all units either from
 /// .debug_info and .debug_types, or from .debug_info.dwo and .debug_types.dwo.
 class DWARFUnitVector final : public SmallVector<std::unique_ptr<DWARFUnit>, 1> {
   std::function<std::unique_ptr<DWARFUnit>(uint64_t, DWARFSectionKind,
                                            const DWARFSection *,
                                            const DWARFUnitIndex::Entry *)>
       Parser;
   int NumInfoUnits = -1;
 
 public:
   using UnitVector = SmallVectorImpl<std::unique_ptr<DWARFUnit>>;
   using iterator = typename UnitVector::iterator;
   using iterator_range = llvm::iterator_range<typename UnitVector::iterator>;
 
+  using compile_unit_range =
+      decltype(make_filter_range(std::declval<iterator_range>(), isCU));
+
   DWARFUnit *getUnitForOffset(uint64_t Offset) const;
   DWARFUnit *getUnitForIndexEntry(const DWARFUnitIndex::Entry &E);
 
   /// Read units from a .debug_info or .debug_types section.  Calls made
   /// before finishedInfoUnits() are assumed to be for .debug_info sections,
   /// calls after finishedInfoUnits() are for .debug_types sections.  Caller
   /// must not mix calls to addUnitsForSection and addUnitsForDWOSection.
   void addUnitsForSection(DWARFContext &C, const DWARFSection &Section,
private:
   /// clearDIEs - Clear parsed DIEs to keep memory usage low.
   void clearDIEs(bool KeepCUDie);
 
   /// parseDWO - Parses .dwo file for current compile unit. Returns true if
   /// it was actually constructed.
   bool parseDWO();
 };
 
+inline bool isCU(const std::unique_ptr<DWARFUnit> &U) {
+  return !U->isTypeUnit();
+}
+
 } // end namespace llvm
 
 #endif // LLVM_DEBUGINFO_DWARF_DWARFUNIT_H

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

 }
 
 uint8_t DWARFContext::getCUAddrSize() {
   // In theory, different compile units may have different address byte
   // sizes, but for simplicity we just use the address byte size of the
   // first compile unit. In practice the address size field is repeated across
   // various DWARF headers (at least in version 5) to make it easier to dump
   // them independently, not to enable varying the address size.
-  unit_iterator_range CUs = compile_units();
+  auto CUs = compile_units();
   return CUs.empty() ? 0 : (*CUs.begin())->getAddressByteSize();
 }

llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp

for (const Entry &E : Macros.Macros) {
         break;
       }
       OS << "\n";
     }
   }
 }
 
 Error DWARFDebugMacro::parseImpl(
-    Optional<DWARFUnitVector::iterator_range> Units,
+    Optional<DWARFUnitVector::compile_unit_range> Units,
     Optional<DataExtractor> StringExtractor, DWARFDataExtractor Data,
     bool IsMacro) {
   uint64_t Offset = 0;
   MacroList *M = nullptr;
   using MacroToUnitsMap = DenseMap<uint64_t, DWARFUnit *>;
   MacroToUnitsMap MacroToUnits;
   if (IsMacro && Data.isValidOffset(Offset)) {
     // Keep a mapping from Macro contribution to CUs, this will