[PATCH] D72828: [DWARF5] Added support for emission of debug_macro section.

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

This doesn't seem to be tested - the couple of test updates are to keep them passing after changing the ordering of sections - ideally that change (changing the ordering of sections, so strings can come after macros) should be committed separately (just change the order and update the few tests that need it) & explicit/full test coverage for this feature should be added and should use llvm-dwarfdump (so this patch should come after the one that adds functionality to llvm-dwarfdump for dumping this section).



================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:2812
+        this->InfoHolder.getStringPool()
+            .getEntry(*Asm, Name.str() + ' ' + Value.str())
+            .getSymbol(),
----------------
Could this be Name + ' ' + Value? (perhaps it could be (Name + ' ' + Value).str() if it needs to be a string in the end - that would be a bit more efficient than the current code that stringifies the Name and Value separately)


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:2825
+      Asm->emitInt8(' ');
+      Asm->OutStreamer->AddComment("Macro Value=" + Value);
+      Asm->OutStreamer->EmitBytes(Value);
----------------
Probably skip the "= + Value" part here (the same isn't done for the "Macro String"/name above - because the string itself is probably legible in the assembly? So the description is useful, but the value itself will be evident in the assembly without needing to be repeated in the comment)


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:2834
+                                   unsigned StartFile, unsigned EndFile,
+                                   StringRef (*Func)(unsigned Form)) {
+
----------------
Rename "Func" to a more informative name - "MacToString", perhaps?


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

https://reviews.llvm.org/D72828