[PATCH] D82975: [DebugInfo] Allow GNU macro extension to be emitted

[David Stenberg via Phabricator via llvm-commits]

dstenb marked an inline comment as done.
dstenb added inline comments.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:1359-1368
+        dwarf::Attribute MacrosAttr = getDwarfVersion() >= 5
+                                          ? dwarf::DW_AT_macros
+                                          : dwarf::DW_AT_GNU_macros;
         if (useSplitDwarf())
           TheCU.addSectionDelta(
-              TheCU.getUnitDie(), dwarf::DW_AT_macros, U.getMacroLabelBegin(),
+              TheCU.getUnitDie(), MacrosAttr, U.getMacroLabelBegin(),
               TLOF.getDwarfMacroDWOSection()->getBeginSymbol());
----------------
dblaikie wrote:
> dstenb wrote:
> > dblaikie wrote:
> > > dstenb wrote:
> > > > dblaikie wrote:
> > > > > Looks like this might be wrong for v4 + split DWARF + using macro? Or perhaps this code isn't reachable by that combination?
> > > > > 
> > > > > Might be more clear, then, to sink the MacrosAttr choice down into the "else" clause here, and assert in the split DWARF case that the version >= 5? (possibly including a note about how the pre-v5, GCC debug_macro extension isn't supported with Split DWARF)
> > > > Sorry, in what way does this look wrong? If I am not overlooking something, this look the same as what GCC emits for the attribute in the `-g3 -gdwarf-4 -gsplit-dwarf` case.
> > > > 
> > > > Regardless of the above, doing like you suggest and adding an assert seems like a good idea. 
> > > > Sorry, in what way does this look wrong? If I am not overlooking something, this look the same as what GCC emits for the attribute in the -g3 -gdwarf-4 -gsplit-dwarf case.
> > > 
> > > I think that's what we were discussing at length previously in this review, that GNU debug_macro + v4 + split DWARF seems a bit ill specified, and it's probably best to have -fdebug-macro + v4 + -gsplit-dwarf continue to use debug_macinfo rather than the ill-specified and not-implemented-by-any-consumer v4 Split DWARF .debug_macro?
> > > 
> > > https://reviews.llvm.org/D82975#2142264
> > > 
> > Okay, yes, I guess it's bad to assume that the attribute part is valid if the whole GNU debug_macro + v4 + split DWARF format seems ill specified. I have added an assert as you suggested.
> This code would hit the assertion with `-gsplit-dwarf -fdebug-macro -use-gnu-debug-macro`, yes?
> 
> While the -use-gnu-debug-macro isn't a driver option (ie: not a public feature), probably best to have `UseDebugMacroSection` set to false when using v4 split DWARF?
> ```
> UseDebugMacroSection = DwarfVersion >= 5 || (UseGNUDebugMacro && !HasSplitDwarf);
> ```
> 
> (& a test that this combination behaves as expected)
Yes, I'll do that. I thought that this could be done later on when this is promoted from a hidden LLVM flag to being enabled by the tuning, but perhaps it's better to just do it from the start.


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

https://reviews.llvm.org/D82975