[DWARF5] Enable .debug_line.dwo section emission if macro info is present

[SouraVX]

Summary

Previously the .debug_line.dwo section was only emitted only if type
units are present. This patch further extends emission for the case if
.debug_macro.dwo section is present.

For enabling this, I've refactored out SplitLineTable from
DwarfTypeUnit to DwarfUnit so that it can be emitted for when
type units are not present. Emission is further restricted to only
when .debug_macro.dwo section is present.

Ideally, this should've splitted into more than one patches, but
that can be done post review. This patch is intended to convey the overall
direction.

[probinson]

llvm/lib/CodeGen/AsmPrinter/DwarfUnit.h

298

either this->SplitLineTable = SplitLineTable; or change the name of the parameter.

[SouraVX]

Addressed @probinson review comments. Thanks for this!

[SouraVX]

llvm/lib/CodeGen/AsmPrinter/DwarfUnit.h

298

Let's keep the former. Thanks!

[SouraVX]

Ping @ all reviewers!

[probinson]

LGTM.

[dblaikie]

I think this needs a bit more work - or at least I have a few more questions.

llvm/include/llvm/MC/MCDwarf.h

303

This function appears to be unused (& should be removed/omitted), if I'm not mistaken.

305

Is this necessary? If no one ever retrieves/uses the label anyway.

307

Can this be avoided? Currently the split line table is enabled/emitted implicitly, based on whether or not there's anything in it, I think? Hopefully that behavior can extend to the debug_macro use case as well?

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

1018–1025

(hopefully this whole block can be removed/avoided - based on the prior point about "enableSplitLineTable" Being avoided)

Also, putting a CU stmt_list 0 seems like it has a good chance to confuse/break consumers - since they could conclude that this attribute means that all decl_files should be looked up in the debug_line.dwo section, whereas they should probably be looking in the .debug_line section?

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h

406–410

This is effectively unused and should be removed/avoided - also, comments generally should refer to/explain/justify the current state of the code, not a future state. (variables, etc, can be introduced as/when they are needed, not before)

The future use case for debug_macro.dwo's debug_line_offset might be fine with a hardcoded zero (as you had used below for the stmt_list ("NewCU.addSectionOffset(Die, dwarf::DW_AT_stmt_list, 0);")), avoiding the need for an explicit label here. (though a label could be used, if it is, I'd probably suggest having the MCDwarfDwoLineTable generate it itself, when requested - and could also use that as a signal to emit the debug_line.dwo if needed))

783

This function is unused and should be removed/avoided.

llvm/lib/CodeGen/AsmPrinter/DwarfUnit.h

84–85

Rather than plumbing this through to every unit - might be better to go the other way, removing the existing plumbing into DwarfTypeUnit, and replace it with accessing this directly from DwarfDebug whenever needed. (since there's only one of them anyway - so we don't need to track different ones per-unit)

[SouraVX]

Thanks @dblaikie for reviewing this!

llvm/include/llvm/MC/MCDwarf.h

303

Ah Sorry, but I think I conveyed the intent(Splitting up later, if deemed necessary) in patch summary correctly. All these functions will be later used by D80945.

305

Yes this is crucial(setting up label explicitly) for setting up debug_line_offset in debug_line.dwo.
This will be finally emitted in ASM as:
.Lsplit_unit_line_table0
Later in debug_line_offset can be emitted as a difference of labels as:
.Lsplit_unit_line_table0-.debug_line.dwo

307

Not I believe, since SplitLineTable is emitted/enabled for the case when TypeUnits are present. This helper allow us to enable emission(later handeld in MCDwarf)if macro information is present.

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

1018–1025

Also, putting a CU stmt_list 0 seems like it has a good chance to confuse/break consumers - since they could conclude that this attribute means that all decl_files should be looked up in the debug_line.dwo section, whereas they should probably be looking in the .debug_line section?

This whole block is bounded by the else condition(also further restricted by macro info) i.e control-flow enters here only for SplitDwarf. So there won't be any ambiguity.

[SouraVX]

llvm/lib/CodeGen/AsmPrinter/DwarfUnit.h

84–85

Sorry, not able to follow you here. SplitLineTable was already present in DwarfTypeUnit hence it was restricting the enablement for present case.
So I moved it from there and promoted to DwarfUnit so that both case(TypeUnit and debug_macro.dwo) where SplitLineTable is required should be able to use it.

[dblaikie]

llvm/include/llvm/MC/MCDwarf.h

303

It's great to stage refactorings separately where possible - but generally the state of the codebase should be fairly self-consistent at any point in time. So adjusting the code to make the line table accessible to more places (eg: removing the pointer from DwarfTypeUnit and to just use it from DwarfDebug directly before adding more usage (or the refactoring you've proposed, adding it to all DwarfUnits, if that was the way to go - which I don't think it is)).

305

Could it just be emitted as zero, the same way as the stmt_list you had added?

In any case, it should be added in the patch that uses it, not before.

307

I /believe/ SplitLineTable is enabled not by the presence of type units in general, but by those type units using "getFile" which adds entries to the line table and enables it. So it's enabled implicitly when files are added - I would hope that the same would be true of the macro section. It could implicitly enable the split line table by using it, not by having an explicit invocation that enables it.

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

1018–1025

I meant ambiguity in the resulting DWARF.

Now you'll have a split unit with a stmt_list of zero, but the skeleton unit for that split unit will also have a stmt_list. So a consumer, when trying to resolve the index in a decl_file inside the split unit, might see the stmt_list in the split CU and decide that's the line table it should consult - when in reality it's the line table in the skeleton unit that is meant to be consulted in that case.

[SouraVX]

Based on the feedback by @dblaikie (Thanks for this)! Filed separate refactoring revison D82084. Planning to revise this after we reach consensus on that(D82084).
Thanks!

[dblaikie]

Posted an alternative direction/proposal here: D84278

[SouraVX]

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

llvm/include/llvm/MC/MCDwarf.h

public:
   unsigned getFile(StringRef Directory, StringRef FileName,
                    Optional<MD5::MD5Result> Checksum, uint16_t DwarfVersion,
                    Optional<StringRef> Source) {
     HasSplitLineTable = true;
     return cantFail(Header.tryGetFile(Directory, FileName, Checksum, Source,
                                       DwarfVersion));
   }
 
+  MCSymbol *getLabel() const { return Header.Label; }
+
+  void setLabel(MCSymbol *Label) { Header.Label = Label; }
+
+  void enableSplitLineTable() { HasSplitLineTable = true; }
+
   void Emit(MCStreamer &MCOS, MCDwarfLineTableParams Params,
             MCSection *Section) const;
 };
 
 class MCDwarfLineTable {
   MCDwarfLineTableHeader Header;
   MCLineSection MCLineSections;
 

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h

class DwarfDebug : public DebugHandlerBase {
   /// Separated Dwarf Variables
   /// In general these will all be for bits that are left in the
   /// original object file, rather than things that are meant
   /// to be in the .dwo sections.
 
   /// Holder for the skeleton information.
   DwarfFile SkeletonHolder;
 
-  /// Store file names for type units under fission in a line table
+  /// Store file names for split 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;
+  // 0, referencing the comp_dir of all the split units that use it.
+  MCDwarfDwoLineTable SplitUnitFileTable;
   /// @}
+  /// Start symbol for SplitUnitFileTable, this will be referenced by
+  /// debug_macro.dwo for setting up `debug_line_offset`.
+  MCSymbol *SplitUnitFileTableStartSym;
 
   /// True iff there are multiple CUs in this module.
   bool SingleCU;
   bool IsDarwin;
 
   AddressPool AddrPool;
 
   /// Accelerator tables.
   AccelTable<DWARF5AccelTableData> AccelDebugNames;
public:
   /// @{
   bool tuneForGDB() const { return DebuggerTuning == DebuggerKind::GDB; }
   bool tuneForLLDB() const { return DebuggerTuning == DebuggerKind::LLDB; }
   bool tuneForSCE() const { return DebuggerTuning == DebuggerKind::SCE; }
   /// @}
 
   void addSectionLabel(const MCSymbol *Sym);
   const MCSymbol *getSectionLabel(const MCSection *S);
+  MCSymbol *getSplitFileTableStartSym() { return SplitUnitFileTableStartSym; }
   void insertSectionLabel(const MCSymbol *S);
 
   static void emitDebugLocValue(const AsmPrinter &AP, const DIBasicType *BT,
                                 const DbgValueLoc &Value,
                                 DwarfExpression &DwarfExpr);
 };
 
 } // end namespace llvm
 
 #endif // LLVM_LIB_CODEGEN_ASMPRINTER_DWARFDEBUG_H

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

void DwarfDebug::finishUnitAttributes(const DICompileUnit *DIUnit,
   if (!useSplitDwarf()) {
     NewCU.initStmtList();
 
     // If we're using split dwarf the compilation dir is going to be in the
     // skeleton CU and so we don't need to duplicate it here.
     if (!CompilationDir.empty())
       NewCU.addString(Die, dwarf::DW_AT_comp_dir, CompilationDir);
     addGnuPubAttributes(NewCU, Die);
+  } else if (!DIUnit->getMacros().empty()) {
+    // .debug_macro.dwo needs to reference .debug_line.dwo for
+    // setting up `debug_line_offset`. Initialize the split unit line table
+    // to be emitted later in .debug_line.dwo section.
+    MCDwarfDwoLineTable *SplitLineTable = getDwoLineTable(NewCU);
+    SplitLineTable->enableSplitLineTable();
+    NewCU.initSplitLineTable(SplitLineTable);
+    NewCU.addSectionOffset(Die, dwarf::DW_AT_stmt_list, 0);
   }
 
   if (useAppleExtensionAttributes()) {
     if (DIUnit->isOptimized())
       NewCU.addFlag(Die, dwarf::DW_AT_APPLE_optimized);
 
     StringRef Flags = DIUnit->getFlags();
     if (!Flags.empty())

 // abbreviations for the .debug_info.dwo section.
 void DwarfDebug::emitDebugAbbrevDWO() {
   assert(useSplitDwarf() && "No split dwarf?");
   InfoHolder.emitAbbrevs(Asm->getObjFileLowering().getDwarfAbbrevDWOSection());
 }
 
 void DwarfDebug::emitDebugLineDWO() {
   assert(useSplitDwarf() && "No split dwarf?");
-  SplitTypeUnitFileTable.Emit(
-      *Asm->OutStreamer, MCDwarfLineTableParams(),
+  SplitUnitFileTableStartSym = Asm->createTempSymbol("split_unit_line_table");
+  SplitUnitFileTable.setLabel(SplitUnitFileTableStartSym);
+  SplitUnitFileTable.Emit(*Asm->OutStreamer, MCDwarfLineTableParams(),
                           Asm->getObjFileLowering().getDwarfLineDWOSection());
 }
 
 void DwarfDebug::emitStringOffsetsTableHeaderDWO() {
   assert(useSplitDwarf() && "No split dwarf?");
   InfoHolder.getStringPool().emitStringOffsetsTableHeader(
       *Asm, Asm->getObjFileLowering().getDwarfStrOffDWOSection(),
       InfoHolder.getStringOffsetsStartSym());
 }

 void DwarfDebug::emitDebugAddr() {
   AddrPool.emit(*Asm, Asm->getObjFileLowering().getDwarfAddrSection());
 }
 
 MCDwarfDwoLineTable *DwarfDebug::getDwoLineTable(const DwarfCompileUnit &CU) {
   if (!useSplitDwarf())
     return nullptr;
   const DICompileUnit *DIUnit = CU.getCUNode();
-  SplitTypeUnitFileTable.maybeSetRootFile(
+  SplitUnitFileTable.maybeSetRootFile(
       DIUnit->getDirectory(), DIUnit->getFilename(),
       CU.getMD5AsBytes(DIUnit->getFile()), DIUnit->getSource());
-  return &SplitTypeUnitFileTable;
+  return &SplitUnitFileTable;
 }
 
 uint64_t DwarfDebug::makeTypeSignature(StringRef Identifier) {
   MD5 Hash;
   Hash.update(Identifier);
   // ... take the least significant 8 bytes and return those. Our MD5
   // implementation always returns its results in little endian, so we actually
   // need the "high" word.

llvm/lib/CodeGen/AsmPrinter/DwarfUnit.h

protected:
   /// A list of all the DIELocs in use.
   std::vector<DIELoc *> DIELocs;
 
   /// This map is used to keep track of subprogram DIEs that need
   /// DW_AT_containing_type attribute. This attribute points to a DIE that
   /// corresponds to the MDNode mapped with the subprogram DIE.
   DenseMap<DIE *, const DINode *> ContainingTypeMap;
 
-  DwarfUnit(dwarf::Tag, const DICompileUnit *Node, AsmPrinter *A, DwarfDebug *DW,
-            DwarfFile *DWU);
+  DwarfUnit(dwarf::Tag, const DICompileUnit *Node, AsmPrinter *A,
+            DwarfDebug *DW, DwarfFile *DWU,
+            MCDwarfDwoLineTable *SplitLineTable = nullptr);
 
   bool applySubprogramDefinitionAttributes(const DISubprogram *SP, DIE &SPDie);
 
   bool shareAcrossDWOCUs() const;
   bool isShareableAcrossCUs(const DINode *D) const;
 
+  MCDwarfDwoLineTable *SplitLineTable;
+
 public:
   // Accessors.
   AsmPrinter* getAsmPrinter() const { return Asm; }
   MCSymbol *getEndLabel() const { return EndLabel; }
   uint16_t getLanguage() const { return CUNode->getSourceLanguage(); }
   const DICompileUnit *getCUNode() const { return CUNode; }
 
   uint16_t getDwarfVersion() const { return DD->getDwarfVersion(); }
public:
 
   /// If the \p File has an MD5 checksum, return it as an MD5Result
   /// allocated in the MCContext.
   Optional<MD5::MD5Result> getMD5AsBytes(const DIFile *File) const;
 
   /// Get context owner's DIE.
   DIE *createTypeDIE(const DICompositeType *Ty);
 
+  void initSplitLineTable(MCDwarfDwoLineTable *SplitLineTable) {
+    SplitLineTable = SplitLineTable;
+  }
+
 protected:
   ~DwarfUnit();
 
   /// Create new static data member DIE.
   DIE *getOrCreateStaticMemberDIE(const DIDerivedType *DT);
 
   /// Look up the source ID for the given file. If none currently exists,
   /// create a new ID and insert it in the line table.
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,

llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp

 void DIEDwarfExpression::commitTemporaryBuffer() { OutDIE.takeValues(TmpDIE); }
 
 bool DIEDwarfExpression::isFrameRegister(const TargetRegisterInfo &TRI,
                                          unsigned MachineReg) {
   return MachineReg == TRI.getFrameRegister(*AP.MF);
 }
 
 DwarfUnit::DwarfUnit(dwarf::Tag UnitTag, const DICompileUnit *Node,
-                     AsmPrinter *A, DwarfDebug *DW, DwarfFile *DWU)
+                     AsmPrinter *A, DwarfDebug *DW, DwarfFile *DWU,
+                     MCDwarfDwoLineTable *SplitLineTable)
     : DIEUnit(A->getDwarfVersion(), A->MAI->getCodePointerSize(), UnitTag),
-      CUNode(Node), Asm(A), DD(DW), DU(DWU), IndexTyDie(nullptr) {
-}
+      CUNode(Node), Asm(A), DD(DW), DU(DWU), IndexTyDie(nullptr),
+      SplitLineTable(SplitLineTable) {}
 
 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) {
-}
+    : DwarfUnit(dwarf::DW_TAG_type_unit, CU.getCUNode(), A, DW, DWU,
+                SplitLineTable),
+      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();
 }
 

llvm/test/DebugInfo/X86/debug-line-macro-dwo.ll

+; This test checks emission of .debug_line.dwo section when
+; -gdwarf-5 -gsplit-dwarf -fdebug-macro is specified.
+
+; RUN: %llc_dwarf -dwarf-version=5 -O0 -filetype=asm \
+; RUN:   -split-dwarf-file=test.dwo < %s | FileCheck %s
+
+; CHECK-LABEL: .debug_line.dwo
+; CHECK-NEXT: .Lsplit_unit_line_table0:
+
+; ModuleID = 'test.c'
+source_filename = "test.c"
+target datalayout = "e-m:e-p200:32:32-p201:32:32-p202:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!14, !15, !16}
+!llvm.ident = !{!17}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 11.0.0", isOptimized: false, runtimeVersion: 0, splitDebugFilename: "test.dwo", emissionKind: FullDebug, enums: !2, macros: !3, splitDebugInlining: false, nameTableKind: None)
+!1 = !DIFile(filename: "test.c", directory: "/home/", checksumkind: CSK_MD5, checksum: "ef6a7032e0c7ceeef614583f2c00dc80")
+!2 = !{}
+!3 = !{!4}
+!4 = !DIMacroFile(file: !1, nodes: !5)
+!5 = !{!6, !10, !13}
+!6 = !DIMacroFile(line: 1, file: !7, nodes: !8)
+!7 = !DIFile(filename: "./foo.h", directory: "/home/", checksumkind: CSK_MD5, checksum: "0f0cd0e15b44f49d3944992c8dc28661")
+!8 = !{!9}
+!9 = !DIMacro(type: DW_MACINFO_define, line: 1, name: "FOO", value: "5")
+!10 = !DIMacroFile(line: 2, file: !11, nodes: !12)
+!11 = !DIFile(filename: "./bar.h", directory: "/home/", checksumkind: CSK_MD5, checksum: "bf4b34c263eaaa1d7085c18243b8d100")
+!12 = !{!13}
+!13 = !DIMacro(type: DW_MACINFO_undef, line: 14, name: "YEA")
+!14 = !{i32 7, !"Dwarf Version", i32 5}
+!15 = !{i32 2, !"Debug Info Version", i32 3}
+!16 = !{i32 1, !"wchar_size", i32 4}
+!17 = !{!"clang version 11.0.0"}