DwarfDebug: Reduce duplication in addAccel*** methods

[labath]

Summary

Each of the four methods had a dozen lines and was doing almost exactly
the same thing: get the appropriate accelerator table kind and insert an
entry into it. I move this common logic to a helper function and make
these methods delegate to it.

This came up in the context of D49493, where I've needed to make adding
a string to a string pool slightly more complicated, and it seemed to
make sense to do it in one place instead of five.

To make this work I've needed to unify the interface of the AccelTable
data types, as some used to store DIE& and others DIE*. I chose to unify
to a reference as that's what the caller uses.

This technically isn't NFC, because it changes the StringPool used for
apple tables in the DWO case (now it uses the main file like DWARF v5
instead of the DWO file). However, that shouldn't matter, as DWO is not
a thing on apple targets (clang frontend simply ignores -gsplit-dwarf).

[labath]

This technically isn't NFC, because it changes the StringPool used for
apple tables in the DWO case (now it uses the main file like DWARF v5
instead of the DWO file). However, that shouldn't matter, as DWO is not
a thing on apple targets (clang frontend simply ignores -gsplit-dwarf).

In fact I would go as far as to say this makes things more correct. Back when apple tables were hooked up to -glldb, I tried to use them on linux with -gsplit-dwarf. The compilation broke because we couldn't perform the split step (it wanted to keep the apple tables in the main file, but they were linked to the debug_str.dwo section).

[JDevlieghere]

LGTM. This is definitely much better!

lib/CodeGen/AsmPrinter/DwarfDebug.cpp

2418 ↗

(On Diff #156250)

I think I slightly prefer a switch here with an unreachable, so we have a NO-OP in case the default hasn't been resolved yet (instead or arbitrarily defaulting to DWARF).

[labath]

lib/CodeGen/AsmPrinter/DwarfDebug.cpp

2418 ↗

(On Diff #156250)

I've changed this to a switch statement, but I wouldn't recommend relying on llvm_unreachable being a noop (compilers like to turn it into a trap instruction).

llvm/trunk/include/llvm/CodeGen/AccelTable.h

void emitDWARF5AccelTable(
     ArrayRef<MCSymbol *> CUs,
     llvm::function_ref<unsigned(const DWARF5AccelTableStaticData &)>
         getCUIndexForEntry);
 
 /// Accelerator table data implementation for simple Apple accelerator tables
 /// with just a DIE reference.
 class AppleAccelTableOffsetData : public AppleAccelTableData {
 public:
-  AppleAccelTableOffsetData(const DIE *D) : Die(D) {}
+  AppleAccelTableOffsetData(const DIE &D) : Die(D) {}
 
   void emit(AsmPrinter *Asm) const override;
 
 #ifndef _MSC_VER
   // The line below is rejected by older versions (TBD) of MSVC.
   static constexpr Atom Atoms[] = {
       Atom(dwarf::DW_ATOM_die_offset, dwarf::DW_FORM_data4)};
 #else
   // FIXME: Erase this path once the minimum MSCV version has been bumped.
   static const SmallVector<Atom, 4> Atoms;
 #endif
 
 #ifndef NDEBUG
   void print(raw_ostream &OS) const override;
 #endif
 protected:
-  uint64_t order() const override { return Die->getOffset(); }
+  uint64_t order() const override { return Die.getOffset(); }
 
-  const DIE *Die;
+  const DIE &Die;
 };
 
 /// Accelerator table data implementation for Apple type accelerator tables.
 class AppleAccelTableTypeData : public AppleAccelTableOffsetData {
 public:
-  AppleAccelTableTypeData(const DIE *D) : AppleAccelTableOffsetData(D) {}
+  AppleAccelTableTypeData(const DIE &D) : AppleAccelTableOffsetData(D) {}
 
   void emit(AsmPrinter *Asm) const override;
 
 #ifndef _MSC_VER
   // The line below is rejected by older versions (TBD) of MSVC.
   static constexpr Atom Atoms[] = {
       Atom(dwarf::DW_ATOM_die_offset, dwarf::DW_FORM_data4),
       Atom(dwarf::DW_ATOM_die_tag, dwarf::DW_FORM_data2),

llvm/trunk/lib/CodeGen/AsmPrinter/AccelTable.cpp

llvm::function_ref<unsigned(const DWARF5AccelTableStaticData &)>
         getCUIndexForEntry) {
   Contents.finalize(Asm, "names");
   Dwarf5AccelTableWriter<DWARF5AccelTableStaticData>(Asm, Contents, CUs,
                                                      getCUIndexForEntry)
       .emit();
 }
 
 void AppleAccelTableOffsetData::emit(AsmPrinter *Asm) const {
-  Asm->emitInt32(Die->getDebugSectionOffset());
+  Asm->emitInt32(Die.getDebugSectionOffset());
 }
 
 void AppleAccelTableTypeData::emit(AsmPrinter *Asm) const {
-  Asm->emitInt32(Die->getDebugSectionOffset());
-  Asm->emitInt16(Die->getTag());
+  Asm->emitInt32(Die.getDebugSectionOffset());
+  Asm->emitInt16(Die.getTag());
   Asm->emitInt8(0);
 }
 
 void AppleAccelTableStaticOffsetData::emit(AsmPrinter *Asm) const {
   Asm->emitInt32(Offset);
 }
 
 void AppleAccelTableStaticTypeData::emit(AsmPrinter *Asm) const {

llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.h

void ensureAbstractVariableIsCreatedIfScoped(DwarfCompileUnit &CU, InlinedVariable IV,
                                                const MDNode *Scope);
 
   DbgVariable *createConcreteVariable(DwarfCompileUnit &TheCU,
                                       LexicalScope &Scope, InlinedVariable IV);
 
   /// Construct a DIE for this abstract scope.
   void constructAbstractSubprogramScopeDIE(DwarfCompileUnit &SrcCU, LexicalScope *Scope);
 
-  /// Helper function to add a name to the .debug_names table, using the
-  /// appropriate string pool.
-  void addAccelDebugName(StringRef Name, const DIE &Die);
+  template <typename DataT>
+  void addAccelNameImpl(AccelTable<DataT> &AppleAccel, StringRef Name,
+                        const DIE &Die);
 
   void finishVariableDefinitions();
 
   void finishSubprogramDefinitions();
 
   /// Finish off debug information after all functions have been
   /// processed.
   void finalizeModuleInfo();

llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

if (TopLevelType) {
     for (auto &TU : TypeUnitsToAdd) {
       InfoHolder.computeSizeAndOffsetsForUnit(TU.first.get());
       InfoHolder.emitUnit(TU.first.get(), useSplitDwarf());
     }
   }
   CU.addDIETypeSignature(RefDie, Signature);
 }
 
-void DwarfDebug::addAccelDebugName(StringRef Name, const DIE &Die) {
-  assert(getAccelTableKind() == AccelTableKind::Dwarf);
+// Add the Name along with its companion DIE to the appropriate accelerator
+// table (for AccelTableKind::Dwarf it's always AccelDebugNames, for
+// AccelTableKind::Apple, we use the table we got as an argument). If
+// accelerator tables are disabled, this function does nothing.
+template <typename DataT>
+void DwarfDebug::addAccelNameImpl(AccelTable<DataT> &AppleAccel, StringRef Name,
+                                  const DIE &Die) {
+  if (getAccelTableKind() == AccelTableKind::None)
+    return;
 
   DwarfFile &Holder = useSplitDwarf() ? SkeletonHolder : InfoHolder;
-  AccelDebugNames.addName(Holder.getStringPool().getEntry(*Asm, Name), Die);
-}
+  DwarfStringPoolEntryRef Ref =
+      Holder.getStringPool().getEntry(*Asm, Name);
 
-// Accelerator table mutators - add each name along with its companion
-// DIE to the proper table while ensuring that the name that we're going
-// to reference is in the string table. We do this since the names we
-// add may not only be identical to the names in the DIE.
-void DwarfDebug::addAccelName(StringRef Name, const DIE &Die) {
   switch (getAccelTableKind()) {
   case AccelTableKind::Apple:
-    AccelNames.addName(InfoHolder.getStringPool().getEntry(*Asm, Name), &Die);
+    AppleAccel.addName(Ref, Die);
     break;
   case AccelTableKind::Dwarf:
-    addAccelDebugName(Name, Die);
+    AccelDebugNames.addName(Ref, Die);
     break;
-  case AccelTableKind::None:
-    return;
   case AccelTableKind::Default:
     llvm_unreachable("Default should have already been resolved.");
+  case AccelTableKind::None:
+    llvm_unreachable("None handled above");
   }
 }
 
+void DwarfDebug::addAccelName(StringRef Name, const DIE &Die) {
+  addAccelNameImpl(AccelNames, Name, Die);
+}
+
 void DwarfDebug::addAccelObjC(StringRef Name, const DIE &Die) {
-  if (getAccelTableKind() != AccelTableKind::Apple)
-    return;
-  AccelObjC.addName(InfoHolder.getStringPool().getEntry(*Asm, Name), &Die);
+  // ObjC names go only into the Apple accelerator tables.
+  if (getAccelTableKind() == AccelTableKind::Apple)
+    addAccelNameImpl(AccelObjC, Name, Die);
 }
 
 void DwarfDebug::addAccelNamespace(StringRef Name, const DIE &Die) {
-  switch (getAccelTableKind()) {
-  case AccelTableKind::Apple:
-    AccelNamespace.addName(InfoHolder.getStringPool().getEntry(*Asm, Name),
-                           &Die);
-    break;
-  case AccelTableKind::Dwarf:
-    addAccelDebugName(Name, Die);
-    break;
-  case AccelTableKind::None:
-    return;
-  case AccelTableKind::Default:
-    llvm_unreachable("Default should have already been resolved.");
-  }
+  addAccelNameImpl(AccelNamespace, Name, Die);
 }
 
 void DwarfDebug::addAccelType(StringRef Name, const DIE &Die, char Flags) {
-  switch (getAccelTableKind()) {
-  case AccelTableKind::Apple:
-    AccelTypes.addName(InfoHolder.getStringPool().getEntry(*Asm, Name), &Die);
-    break;
-  case AccelTableKind::Dwarf:
-    addAccelDebugName(Name, Die);
-    break;
-  case AccelTableKind::None:
-    return;
-  case AccelTableKind::Default:
-    llvm_unreachable("Default should have already been resolved.");
-  }
+  addAccelNameImpl(AccelTypes, Name, Die);
 }
 
 uint16_t DwarfDebug::getDwarfVersion() const {
   return Asm->OutStreamer->getContext().getDwarfVersion();
 }