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

[David Blaikie via Phabricator via llvm-commits]

dblaikie requested changes to this revision.
dblaikie added a comment.
This revision now requires changes to proceed.

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



================
Comment at: llvm/include/llvm/MC/MCDwarf.h:303
 
+  MCSymbol *getLabel() const { return Header.Label; }
+
----------------
This function appears to be unused (& should be removed/omitted), if I'm not mistaken.


================
Comment at: llvm/include/llvm/MC/MCDwarf.h:305
+
+  void setLabel(MCSymbol *Label) { Header.Label = Label; }
+
----------------
Is this necessary? If no one ever retrieves/uses the label anyway.


================
Comment at: llvm/include/llvm/MC/MCDwarf.h:307
+
+  void enableSplitLineTable() { HasSplitLineTable = true; }
+
----------------
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?


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:1018-1025
+  } 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);
----------------
(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?


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h:406-409
   /// @}
+  /// Start symbol for SplitUnitFileTable, this will be referenced by
+  /// debug_macro.dwo for setting up `debug_line_offset`.
+  MCSymbol *SplitUnitFileTableStartSym;
----------------
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))


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h:783
   const MCSymbol *getSectionLabel(const MCSection *S);
+  MCSymbol *getSplitFileTableStartSym() { return SplitUnitFileTableStartSym; }
   void insertSectionLabel(const MCSymbol *S);
----------------
This function is unused and should be removed/avoided.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfUnit.h:84-85
 
+  MCDwarfDwoLineTable *SplitLineTable;
+
 public:
----------------
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)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81476/new/

https://reviews.llvm.org/D81476