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

[Sourabh Singh Tomar via Phabricator via llvm-commits]

SouraVX marked 3 inline comments as done.
SouraVX added a comment.

Thanks @dblaikie for reviewing this!



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


================
Comment at: llvm/include/llvm/MC/MCDwarf.h:305
+
+  void setLabel(MCSymbol *Label) { Header.Label = Label; }
+
----------------
dblaikie wrote:
> Is this necessary? If no one ever retrieves/uses the label anyway.
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`


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


================
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);
----------------
dblaikie wrote:
> (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?
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81476