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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/include/llvm/MC/MCDwarf.h:303
 
+  MCSymbol *getLabel() const { return Header.Label; }
+
----------------
SouraVX wrote:
> 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.
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)).


================
Comment at: llvm/include/llvm/MC/MCDwarf.h:305
+
+  void setLabel(MCSymbol *Label) { Header.Label = Label; }
+
----------------
SouraVX wrote:
> 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`
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.


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


================
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);
----------------
SouraVX wrote:
> 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.
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81476