Simplify arange output

[• rafael]

Summary

I was looking at emitDebugARanges and noticed it was the only user of SectionMap.

The attached patch changes it to compute SectionMap and saves a call to sort.

[dblaikie]

lib/CodeGen/AsmPrinter/DwarfDebug.cpp

1780

The code doesn't need to be split before/after this Sections construction, does it? Perhaps it could be kept/put together in a separate function and called - just with the Sections vector passed in?

1819

Is this code movement independent of the other code changes? It looks like it might just be general cleanup?

Or does the order of elements inserted into the Spans values matter/must be stable in the old code and doesn't need to be in the new code somehow?

[• rafael]

rebased

[• rafael]

The code doesn't need to be split before/after this Sections construction, does it? Perhaps it could be kept/put together in a separate function and called - just with the Sections vector passed in?

It does. The code before creates a SectionMap that is then used to
create Sections. The second part of the code uses Sections.

+ // If we have no section (e.g. common), just write out

+ // individual spans for each symbol.

Is this code movement independent of the other code changes? It looks like it might just be general cleanup?

It is. I committed that first and I will upload a rebased patch.

Cheers,
Rafael

[• rafael]

ping?

[• rafael]

rebased

[• rafael]

ping

lib/CodeGen/AsmPrinter/DwarfDebug.cpp

1780

It needs to be split since the part above creates SectionMap, from which Sections is created.

[dblaikie]

Looks good -thanks! sorry for the delay

(some optional/follow-up comments provided)

lib/CodeGen/AsmPrinter/DwarfDebug.cpp

1758–1760

Refactoring might've been easier to understand if these two chunks had been pulled out into functions first, then the calls moved. (& might keep this function from getting quite so long/make it easier to read if there are some good names for these chunks of code (probably taken from the comment text))

1810

Don't really need this typedef, it's only used once.

[• rafael]

r230693.

lib/CodeGen/AsmPrinter/DwarfDebug.h

class DwarfDebug : public AsmPrinterHandler {
   DenseMap<const DIE *, DwarfCompileUnit *> CUDieMap;
 
   // List of all labels used in aranges generation.
   std::vector<SymbolCU> ArangeLabels;
 
   // Size of each symbol emitted (for those symbols that have a specific size).
   DenseMap<const MCSymbol *, uint64_t> SymSize;
 
-  // Provides a unique id per text section.
-  typedef DenseMap<const MCSection *, SmallVector<SymbolCU, 8> > SectionMapType;
-  SectionMapType SectionMap;
-
   LexicalScopes LScopes;
 
   // Collection of abstract variables.
   DenseMap<const MDNode *, std::unique_ptr<DbgVariable>> AbstractVariables;
   SmallVector<std::unique_ptr<DbgVariable>, 64> ConcreteVariables;
 
   // Collection of DebugLocEntry. Stored in a linked list so that DIELocLists
   // can refer to them in spite of insertions into this list.
class DwarfDebug : public AsmPrinterHandler {
   void finishVariableDefinitions();
 
   void finishSubprogramDefinitions();
 
   /// \brief Finish off debug information after all functions have been
   /// processed.
   void finalizeModuleInfo();
 
-  /// \brief Emit labels to close any remaining sections that have been left
-  /// open.
-  void endSections();
-
   /// \brief Emit the debug info section.
   void emitDebugInfo();
 
   /// \brief Emit the abbreviation section.
   void emitAbbreviations();
 
   /// \brief Emit the last address of the section and the end of
   /// the line matrix.

lib/CodeGen/AsmPrinter/DwarfDebug.cpp

for (MDNode *N : CU_Nodes->operands()) {
     // Emit imported_modules last so that the relevant context is already
     // available.
     for (unsigned i = 0, e = ImportedEntities.getNumElements(); i != e; ++i)
       constructAndAddImportedEntityDIE(CU, ImportedEntities.getElement(i));
   }
 
   // Tell MMI that we have debug info.
   MMI->setDebugInfoAvailability(true);
-
-  // Prime section data.
-  SectionMap[Asm->getObjFileLowering().getTextSection()];
 }
 
 void DwarfDebug::finishVariableDefinitions() {
   for (const auto &Var : ConcreteVariables) {
     DIE *VariableDie = Var->getDIE();
     assert(VariableDie);
     // FIXME: Consider the time-space tradeoff of just storing the unit pointer
     // in the ConcreteVariables list, rather than looking it up again here.
void DwarfDebug::finalizeModuleInfo() {
   }
 
   // Compute DIE offsets and sizes.
   InfoHolder.computeSizeAndOffsets();
   if (useSplitDwarf())
     SkeletonHolder.computeSizeAndOffsets();
 }
 
-void DwarfDebug::endSections() {
-  // Filter labels by section.
-  for (const SymbolCU &SCU : ArangeLabels) {
-    if (SCU.Sym->isInSection()) {
-      // Make a note of this symbol and it's section.
-      const MCSection *Section = &SCU.Sym->getSection();
-      if (!Section->getKind().isMetadata())
-        SectionMap[Section].push_back(SCU);
-    } else {
-      // Some symbols (e.g. common/bss on mach-o) can have no section but still
-      // appear in the output. This sucks as we rely on sections to build
-      // arange spans. We can do it without, but it's icky.
-      SectionMap[nullptr].push_back(SCU);
-    }
-  }
-
-  // Build a list of sections used.
-  std::vector<const MCSection *> Sections;
-  for (const auto &it : SectionMap) {
-    const MCSection *Section = it.first;
-    Sections.push_back(Section);
-  }
-
-  // Sort the sections into order.
-  // This is only done to ensure consistent output order across different runs.
-  std::sort(Sections.begin(), Sections.end(), SectionSort);
-
-  // Add terminating symbols for each section.
-  for (unsigned ID = 0, E = Sections.size(); ID != E; ID++) {
-    const MCSection *Section = Sections[ID];
-    MCSymbol *Sym = nullptr;
-
-    if (Section) {
-      // We can't call MCSection::getLabelEndName, as it's only safe to do so
-      // if we know the section name up-front. For user-created sections, the
-      // resulting label may not be valid to use as a label. (section names can
-      // use a greater set of characters on some systems)
-      Sym = Asm->GetTempSymbol("debug_end", ID);
-      Asm->OutStreamer.SwitchSection(Section);
-      Asm->OutStreamer.EmitLabel(Sym);
-    }
-
-    // Insert a final terminator.
-    SectionMap[Section].push_back(SymbolCU(nullptr, Sym));
-  }
-}
-
 // Emit all Dwarf sections that should come after the content.
 void DwarfDebug::endModule() {
   assert(CurFn == nullptr);
   assert(CurMI == nullptr);
 
   // If we aren't actually generating debug info (check beginModule -
   // conditionalized on !DisableDebugInfoPrinting and the presence of the
   // llvm.dbg.cu metadata node)
   if (!DwarfInfoSectionSym)
     return;
 
-  // End any existing sections.
-  // TODO: Does this need to happen?
-  endSections();
-
   // Finalize the debug info for the module.
   finalizeModuleInfo();
 
   emitDebugStr();
 
   // Emit all the DIEs into a debug info section.
   emitDebugInfo();
 

 
 struct ArangeSpan {
   const MCSymbol *Start, *End;
 };
 
 // Emit a debug aranges section, containing a CU lookup for any
 // address we can tie back to a CU.
 void DwarfDebug::emitDebugARanges() {
-  // Start the dwarf aranges section.
-  Asm->OutStreamer.SwitchSection(
-      Asm->getObjFileLowering().getDwarfARangesSection());
+  // Provides a unique id per text section.
+  typedef DenseMap<const MCSection *, SmallVector<SymbolCU, 8>> SectionMapType;
+  SectionMapType SectionMap;
 
-  typedef DenseMap<DwarfCompileUnit *, std::vector<ArangeSpan>> SpansType;
+  // Prime section data.
+  SectionMap[Asm->getObjFileLowering().getTextSection()];
 
-  SpansType Spans;
+  // Filter labels by section.
+  for (const SymbolCU &SCU : ArangeLabels) {
+    if (SCU.Sym->isInSection()) {
+      // Make a note of this symbol and it's section.
+      const MCSection *Section = &SCU.Sym->getSection();
+      if (!Section->getKind().isMetadata())
+        SectionMap[Section].push_back(SCU);
+    } else {
+      // Some symbols (e.g. common/bss on mach-o) can have no section but still
+      // appear in the output. This sucks as we rely on sections to build
+      // arange spans. We can do it without, but it's icky.
+      SectionMap[nullptr].push_back(SCU);
+    }
+  }
 
   // Build a list of sections used.
   std::vector<const MCSection *> Sections;
   for (const auto &it : SectionMap) {
     const MCSection *Section = it.first;
     Sections.push_back(Section);
   }
 
   // Sort the sections into order.
   // This is only done to ensure consistent output order across different runs.
   std::sort(Sections.begin(), Sections.end(), SectionSort);
 
-  // Build a set of address spans, sorted by CU.
+  // Add terminating symbols for each section.
+  for (unsigned ID = 0, E = Sections.size(); ID != E; ID++) {
+    const MCSection *Section = Sections[ID];
+    MCSymbol *Sym = nullptr;
+
+    if (Section) {
+      // We can't call MCSection::getLabelEndName, as it's only safe to do so
+      // if we know the section name up-front. For user-created sections, the
+      // resulting label may not be valid to use as a label. (section names can
+      // use a greater set of characters on some systems)
+      Sym = Asm->GetTempSymbol("debug_end", ID);
+      Asm->OutStreamer.SwitchSection(Section);
+      Asm->OutStreamer.EmitLabel(Sym);
+    }
+
+    // Insert a final terminator.
+    SectionMap[Section].push_back(SymbolCU(nullptr, Sym));
+  }
+
+  typedef DenseMap<DwarfCompileUnit *, std::vector<ArangeSpan>> SpansType;
+
+  SpansType Spans;
+
   for (const MCSection *Section : Sections) {
     SmallVector<SymbolCU, 8> &List = SectionMap[Section];
     if (List.size() < 2)
       continue;
 
     // If we have no section (e.g. common), just write out
     // individual spans for each symbol.
     if (!Section) {
       for (const SymbolCU &Cur : List) {
         ArangeSpan Span;
         Span.Start = Cur.Sym;
         Span.End = nullptr;
         if (Cur.CU)
           Spans[Cur.CU].push_back(Span);
for (size_t n = 1, e = List.size(); n < e; n++) {
         Span.Start = StartSym;
         Span.End = Cur.Sym;
         Spans[Prev.CU].push_back(Span);
         StartSym = Cur.Sym;
       }
     }
   }
 
+  // Start the dwarf aranges section.
+  Asm->OutStreamer.SwitchSection(
+      Asm->getObjFileLowering().getDwarfARangesSection());
+
   unsigned PtrSize = Asm->getDataLayout().getPointerSize();
 
   // Build a list of CUs used.
   std::vector<DwarfCompileUnit *> CUs;
   for (const auto &it : Spans) {
     DwarfCompileUnit *CU = it.first;
     CUs.push_back(CU);
   }