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

[David Blaikie via Phabricator via llvm-commits]

dblaikie 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());
----------------
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)


================
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");
----------------
/might/ be worth pulling these 4 lines out as a lambda to use from the if/else branches, but probably not... 


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:3036-3048
+      if (!Value.empty())
+        // FIXME: Add support for DWARF64.
+        Asm->OutStreamer->emitSymbolValue(
+            this->InfoHolder.getStringPool()
+                .getEntry(*Asm, (Name + " " + Value).str())
+                .getSymbol(),
+            /*Size=*/4);
----------------
Might be nice to refactor this in both the original codepath and the new codepath you're adding (either before or after this commit) to compute the string once & share the rest of this expression..
```
std::string Str = Value.empty() ? Name.str() : (Name + ' ' + Value).str();
Asm->OutStreamer->emitSymbol(this->InfoHolder.getStringPool().getEntry(*Asm, Str).getSymbol(), 4);
```



================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:3088-3091
+                      dwarf::DW_MACRO_end_file, [&](unsigned Form) {
+                        return (getDwarfVersion() >= 5)
+                                   ? dwarf::MacroString(Form)
+                                   : dwarf::GnuMacroString(Form);
----------------
Looks like maybe this could skip the std::function_ref, and do this:
```
emitMacroFileImpl(F, U, dwarf::DW_MACRO_start_file,
                      dwarf::DW_MACRO_end_file, 
                        (getDwarfVersion() >= 5)
                                   ? dwarf::MacroString
                                   : dwarf::GnuMacroString);
```


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

https://reviews.llvm.org/D82975