[DebugInfo] Refactored out `debug_line.dwo` emission from `DwarfTypeUnit` to `DwarfDebug`

[SouraVX]

Summary

Emission of debug_line.dwo was restricted by presence of
DwarfTypeUnits and the associated code was also hard-wired just
to satisfy this requirement.

This patch pulls out the emission code of debug_line.dwo from
DwarfTypeUnits to DwarfDebug so that, if required(for instance
debug_macro.dwo needs this), it can be leveraged easily.

This patch is based on @dblaikie feedback on D81476. Thanks for this!

[dblaikie]

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h

405

What's the motivation for this change to use unique_ptr here?

[SouraVX]

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h

405

The intent/motivation here is that, we don't want to leak memory and simplify memory management(at least for pointers).
Does this introduces any overhead(to whole thing) ?? or may introduce other potential bugs/undesired behavior that I'm not aware of ?
Happy to roll back.

[dblaikie]

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h

405

Not sure I follow why there would be memory leaks or complications if this remained as it was before the patch - what concerns did you have about the way the code was phrased previously (using an MCDwarfDwoLineTable value, without unique_ptr or other indirection)?

[SouraVX]

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h

405

Sorry I misunderstood the context of your question(I thought "Why unique_ptr" over raw ptr), and hence the confusion.

what concerns did you have about the way the code was phrased previously (using an MCDwarfDwoLineTable value, without unique_ptr or other indirection)?

I first tried patching-up this at very minimal(Following value semantics as it was earlier), later I realized it can't(looks to me) be done that way, due to limitation in existing design.

SplitTypeUnitFileTable was used only for Emission and some initialization effort later will end up initializing MCDwarfDwoLineTable *SplitLineTable( in DwarfTypeUnit) which was used to do the real work.

Emission of debug_line is also mixed in it, for instance: consider this snippet(before patch) from DwarfUnit.

 unsigned DwarfTypeUnit::getOrCreateSourceID(const DIFile *File) {
   // Qucik boucne check whether we need a split unit line table.
   if (!SplitLineTable) // SplitLineTable member of `DwarfTypeUnit` initialized in           
                                // constructor using `SplitTypeUnitFileTable`
     return getCU().getOrCreateSourceID(File);
...
code for `debug_line.dwo` emission

So let's say, we go by value semantics, code will look like(mostly):

 unsigned DwarfTypeUnit::getOrCreateSourceID(const DIFile *File) {
      // Qucik boucne check whether we need a split unit line table.
   if (!DD.getSplitUnitLineTable().HasSplitLineTable)
                      // `HasSplitLineTable` is private member of `MCDwarfDwoLineTable`.
     return getCU().getOrCreateSourceID(File);
...
code for `debug_line.dwo` emission

If you look close enough, you'll realize that's also not going to work and you won't have debug_line.dwo in any case(Since HasSplitLineTable is initialized to false by default and it can be only changed by the code below(which will never be executed in first place)). + This also demands to expose private member HasSplitLineTable to public.

Overall this path doesn't looks promising(at least to me). And using pointer semantics(used carefully) solves the problem cleanly and doesn't degenerate anything, more flexible/maintainable.

Conclusion: I've a patch using existing semantics(Can share if you want to have a look :) ). It is causing following test case failure(due to above described problem) in regression suite.

Failing Tests (3):
  LLVM :: CodeGen/X86/dwarf-headers.ll
  LLVM :: CodeGen/X86/dwarf-split-line-2.ll
  LLVM :: DebugInfo/X86/generate-odr-hash.ll

I hope this make sense, Thank You for bearing with me.

[SouraVX]

@dblaikie Are you Convinced/Okay with explanation and the overall changes.

[dblaikie]

In D82084#2116615, @SouraVX wrote:

@dblaikie Are you Convinced/Okay with explanation and the overall changes.

It's on my list of things to look at

[SouraVX]

Gentle Ping :)

[SouraVX]

Apologies for If I'm pinging too frequently :) . Gentle Ping to all reviewers again! :)

[SouraVX]

@dblaikie and other reviewers, these changes seems Okay to you folks ?

[dblaikie]

In D82084#2156058, @SouraVX wrote:

@dblaikie and other reviewers, these changes seems Okay to you folks ?

Still looking, and yeah - I'm not sure this is the right direction in general, might be easier for me to just write up the change.(for this and D81476). Working on it.

[SouraVX]

Tried one more time disregarding both the approach followed previously.
This time finally worked out(in a much cleaner way).
It exposes getDwoLineTable private method. But IMO that's needed
anyways, since it will used/needed subsequently in D81476.

@dblaikie, Could you please share your thoughts on this.
I hope this should avoid the rework from your side.

Thanks!

[dblaikie]

In D82084#2160122, @SouraVX wrote:

Tried one more time disregarding both the approach followed previously.
This time finally worked out(in a much cleaner way).
It exposes getDwoLineTable private method. But IMO that's needed
anyways, since it will used/needed subsequently in D81476.

@dblaikie, Could you please share your thoughts on this.
I hope this should avoid the rework from your side.

Thanks!

I've posted this as a possible alternative solution/direction - seems simple enough to me, but perhaps I'm missing some cases/understanding? D84278

[SouraVX]

Fixed By: https://reviews.llvm.org/D84278

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h

class DwarfDebug : public DebugHandlerBase {
   /// Holder for the skeleton information.
   DwarfFile SkeletonHolder;
 
   /// Store file names for type units under fission in a line table
   /// header that will be emitted into debug_line.dwo.
   // FIXME: replace this with a map from comp_dir to table so that we
   // can emit multiple tables during LTO each of which uses directory
   // 0, referencing the comp_dir of all the type units that use it.
   MCDwarfDwoLineTable SplitTypeUnitFileTable;
   /// @}
 
   /// True iff there are multiple CUs in this module.
   bool SingleCU;
   bool IsDarwin;
 
   AddressPool AddrPool;
 
   /// Accelerator tables.
   AccelTable<DWARF5AccelTableData> AccelDebugNames;
   AccelTable<AppleAccelTableOffsetData> AccelNames;
   AccelTable<AppleAccelTableOffsetData> AccelObjC;
   AccelTable<AppleAccelTableOffsetData> AccelNamespace;
   AccelTable<AppleAccelTableTypeData> AccelTypes;
 
   // Identify a debugger for "tuning" the debug info.
   DebuggerKind DebuggerTuning = DebuggerKind::Default;
 
-  MCDwarfDwoLineTable *getDwoLineTable(const DwarfCompileUnit &);
 
   const SmallVectorImpl<std::unique_ptr<DwarfCompileUnit>> &getUnits() {
     return InfoHolder.getUnits();
   }
 
   using InlinedEntity = DbgValueHistoryMap::InlinedEntity;
 
   void ensureAbstractEntityIsCreated(DwarfCompileUnit &CU,
public:
   /// Perform an MD5 checksum of \p Identifier and return the lower 64 bits.
   static uint64_t makeTypeSignature(StringRef Identifier);
 
   /// Add a DIE to the set of types that we're going to pull into
   /// type units.
   void addDwarfTypeUnitType(DwarfCompileUnit &CU, StringRef Identifier,
                             DIE &Die, const DICompositeType *CTy);
 
+  MCDwarfDwoLineTable *getDwoLineTable(const DwarfCompileUnit &);
+
   class NonTypeUnitContext {
     DwarfDebug *DD;
     decltype(DwarfDebug::TypeUnitsUnderConstruction) TypeUnitsUnderConstruction;
     friend class DwarfDebug;
     NonTypeUnitContext(DwarfDebug *DD);
   public:
     NonTypeUnitContext(NonTypeUnitContext&&) = default;
     ~NonTypeUnitContext();

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

void DwarfDebug::addDwarfTypeUnitType(DwarfCompileUnit &CU,
   if (!Ins.second) {
     CU.addDIETypeSignature(RefDie, Ins.first->second);
     return;
   }
 
   bool TopLevelType = TypeUnitsUnderConstruction.empty();
   AddrPool.resetUsedFlag();
 
-  auto OwnedUnit = std::make_unique<DwarfTypeUnit>(CU, Asm, this, &InfoHolder,
-                                                    getDwoLineTable(CU));
+  auto OwnedUnit = std::make_unique<DwarfTypeUnit>(CU, Asm, this, &InfoHolder);
   DwarfTypeUnit &NewTU = *OwnedUnit;
   DIE &UnitDie = NewTU.getUnitDie();
   TypeUnitsUnderConstruction.emplace_back(std::move(OwnedUnit), CTy);
 
   NewTU.addUInt(UnitDie, dwarf::DW_AT_language, dwarf::DW_FORM_data2,
                 CU.getLanguage());
 
   uint64_t Signature = makeTypeSignature(Identifier);

llvm/lib/CodeGen/AsmPrinter/DwarfUnit.h

private:
   virtual bool isDwoUnit() const = 0;
   const MCSymbol *getCrossSectionRelativeBaseAddress() const override;
 };
 
 class DwarfTypeUnit final : public DwarfUnit {
   uint64_t TypeSignature;
   const DIE *Ty;
   DwarfCompileUnit &CU;
-  MCDwarfDwoLineTable *SplitLineTable;
   bool UsedLineTable = false;
 
   unsigned getOrCreateSourceID(const DIFile *File) override;
   void finishNonUnitTypeDIE(DIE& D, const DICompositeType *CTy) override;
   bool isDwoUnit() const override;
 
 public:
   DwarfTypeUnit(DwarfCompileUnit &CU, AsmPrinter *A, DwarfDebug *DW,
-                DwarfFile *DWU, MCDwarfDwoLineTable *SplitLineTable = nullptr);
+                DwarfFile *DWU);
 
   void setTypeSignature(uint64_t Signature) { TypeSignature = Signature; }
   void setType(const DIE *Ty) { this->Ty = Ty; }
 
   /// Emit the header for this unit, not including the initial length field.
   void emitHeader(bool UseOffsets) override;
   unsigned getHeaderSize() const override {
     return DwarfUnit::getHeaderSize() + sizeof(uint64_t) + // Type Signature

llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp

 
 DwarfUnit::DwarfUnit(dwarf::Tag UnitTag, const DICompileUnit *Node,
                      AsmPrinter *A, DwarfDebug *DW, DwarfFile *DWU)
     : DIEUnit(A->getDwarfVersion(), A->MAI->getCodePointerSize(), UnitTag),
       CUNode(Node), Asm(A), DD(DW), DU(DWU), IndexTyDie(nullptr) {
 }
 
 DwarfTypeUnit::DwarfTypeUnit(DwarfCompileUnit &CU, AsmPrinter *A,
-                             DwarfDebug *DW, DwarfFile *DWU,
-                             MCDwarfDwoLineTable *SplitLineTable)
-    : DwarfUnit(dwarf::DW_TAG_type_unit, CU.getCUNode(), A, DW, DWU), CU(CU),
-      SplitLineTable(SplitLineTable) {
-}
+                             DwarfDebug *DW, DwarfFile *DWU)
+    : DwarfUnit(dwarf::DW_TAG_type_unit, CU.getCUNode(), A, DW, DWU), CU(CU) {}
 
 DwarfUnit::~DwarfUnit() {
   for (unsigned j = 0, M = DIEBlocks.size(); j < M; ++j)
     DIEBlocks[j]->~DIEBlock();
   for (unsigned j = 0, M = DIELocs.size(); j < M; ++j)
     DIELocs[j]->~DIELoc();
 }
 
Optional<MD5::MD5Result> DwarfUnit::getMD5AsBytes(const DIFile *File) const {
   // An MD5 checksum is 16 bytes.
   std::string ChecksumString = fromHex(Checksum->Value);
   MD5::MD5Result CKMem;
   std::copy(ChecksumString.begin(), ChecksumString.end(), CKMem.Bytes.data());
   return CKMem;
 }
 
 unsigned DwarfTypeUnit::getOrCreateSourceID(const DIFile *File) {
+  MCDwarfDwoLineTable *SplitLineTable = DD->getDwoLineTable(getCU());
   if (!SplitLineTable)
     return getCU().getOrCreateSourceID(File);
   if (!UsedLineTable) {
     UsedLineTable = true;
     // This is a split type unit that needs a line table.
     addSectionOffset(getUnitDie(), dwarf::DW_AT_stmt_list, 0);
   }
   return SplitLineTable->getFile(File->getDirectory(), File->getFilename(),