[PATCH] D15495: Macro Support in Dwarf emission

[Adrian Prantl via llvm-commits]

aprantl added inline comments.

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:1874
@@ +1873,3 @@
+    // Assure there is one space between macro name and macro value.
+    assert(Value.data()[0] != ' ' && "Macro value has a space prefix");
+    Size += 1; // Space
----------------
If this violates the spec, this should be check in the debug info verifier instead of an assertion here.

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:1878
@@ +1877,3 @@
+  }
+  Size += 1; // EOS
+
----------------
Is EOS supposed to be "end of string"? Maybe NUL or null-terminator is more common.

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:1891
@@ +1890,3 @@
+    // Assure there is one space between macro name and macro value.
+    assert(Value.data()[0] != ' ' && "Macro value has a space prefix");
+    Asm->EmitInt8(' ');
----------------
Same here.

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:1912
@@ +1911,3 @@
+    else if (auto *F = dyn_cast<DIMacroFile>(MN))
+      Size += getMacroFileSize(*F, U);
+  }
----------------
maybe add an assertion/unreachable for unhandled cases?

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:1919
@@ +1918,3 @@
+
+void DwarfDebug::emitMacroFile(DIMacroFile &F, DwarfCompileUnit &U) {
+  assert(F.getMacinfoType() == dwarf::DW_MACINFO_start_file);
----------------
Both here and in emitMacro I'm feeling a little uneasy about all the replicated logic between .*emit and .*getSize. It's just waiting to get out of sync :-)
Is there any sensible way to fuse the emission and the size computation (either by templatizing or passing in a lambda)?

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:1954
@@ +1953,3 @@
+      else if (auto *F = dyn_cast<DIMacroFile>(MN))
+        emitMacroFile(*F, U);
+    }
----------------
else assert maybe?

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.h:435
@@ -434,3 +434,3 @@
 
-  /// Emit visible names into a debug ranges section.
+  /// Emit address ranges into a debug ranges section.
   void emitDebugRanges();
----------------
Ideally, these should be a separate NFC commit. Thanks for noticing!


http://reviews.llvm.org/D15495