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

[David Stenberg via Phabricator via llvm-commits]

dstenb marked 3 inline comments 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:
> 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. 


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:3032-3035
+      Asm->emitULEB128(Type);
+      Asm->OutStreamer->AddComment("Line Number");
+      Asm->emitULEB128(M.getLine());
+      Asm->OutStreamer->AddComment("Macro String");
----------------
dblaikie wrote:
> /might/ be worth pulling these 4 lines out as a lambda to use from the if/else branches, but probably not... 
Although there is some code duplication, I think I prefer to keep it as is.


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

https://reviews.llvm.org/D82975