[PATCH] D73086: [WIP][DWARF5] Added support for debug_macro section parsing and dumping in llvm-dwarfdump.

[Sourabh Singh Tomar via Phabricator via llvm-commits]

SouraVX marked 19 inline comments as done.
SouraVX added a comment.

> Something you may wish to consider is writing the tests as unit tests rather than using dwarfdump to test them, since the code is in a library. It may require a little extra legwork to set up initially (writing dwarf generator code, like there already is for debug info and debug line), but I think there is a decent payoff in return. It'll allow you to avoid the noise that some of these tests have.

Nice suggestion thanks!, but I think that should be taken as a separate exercise. what's your take on this?



================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:449
+                 DObj->getMacroSection().Data)) {
+    Expected<const DWARFDebugMacro *> ExpectedMacro = getDebugMacro();
+    if (!ExpectedMacro)
----------------
jhenderson wrote:
> It might make sense for this code to follow a similar pattern to the .debug_line code below, with an extra layer that allows parsing and dumping of individual blocks. That way you can say "dump the block starting at offset 0x1000" or whatever.
Thanks! for the suggestion here. I've partially tried to address concern related to this also in this inline comment.

> These three functions all look very similar..


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:859
+
+Expected<const DWARFDebugMacro *> DWARFContext::getDebugMacinfo() {
   if (Macinfo)
----------------
jhenderson wrote:
> These three functions all look very similar. I wonder if it makes sense to create a small helper that does the common stuff. It could take a reference to the unique_ptr, the `IsMacro` boolean etc as needed.
Since these are mostly 5 liner functions handling their cases cleanly as needed, introducing one more function just to reduce 2-3 LOC doesn't sound good idea(personal opinion). 
Plus problem in this entire patch(due to code shared by macro/macinfo) is that, you flip one thing and you'll endup modifying everything (handling macro/macinfo, parsing/dumping)  just to make sure that existing functionality is not broken.



================
Comment at: llvm/test/DebugInfo/X86/debug-macro-macinfo.s:8
+# CHECK-NEXT: DW_MACRO_start_file - lineno: 0 filenum: 0
+# CHECK-NEXT:   DW_MACRO_define_strp - lineno: 1 macro: DWARF_VERSION 5
+# CHECK-NEXT: DW_MACRO_end_file
----------------
jhenderson wrote:
> Why is this indented?
emission of `DW_FORM_define_strp` form is getting checked here. Is this what you're asking ?


================
Comment at: llvm/test/DebugInfo/X86/debug-macro-macinfo.s:30-39
+	.section	.debug_str,"MS", at progbits,1
+.Linfo_string0:
+	.asciz	"clang version 11.0.0" # string offset=0
+.Linfo_string1:
+	.asciz	"a-v5.c"                # string offset=105
+.Linfo_string2:
+	.asciz	"/macro" # string offset=112
----------------
jhenderson wrote:
> Is any of this block needed?
Yes, If `Flags` in Macro header indicate `debug_line_offset` is present then it is needed. That's why here.


================
Comment at: llvm/test/DebugInfo/X86/debug-macro-macinfo.s:45
+	.byte	0
+	.file	1 "/macro" "a-v4.c"
+	.byte	1
----------------
jhenderson wrote:
> Is this line relevant?
Yes, `macinfo` section represent this way, If accidentally removed would result in assmbler error.


================
Comment at: llvm/test/DebugInfo/X86/debug-macro-macinfo.s:55-63
+	.section	.debug_str,"MS", at progbits,1
+.Linfo_string4:
+	.asciz	"clang version 11.0.0" # string offset=0
+.Linfo_string5:
+	.asciz	"a-v4.c"               # string offset=105
+.Linfo_string6:
+	.asciz	"/macro" # string offset=112
----------------
jhenderson wrote:
> Can you get rid of this block?
Sorry this one is not needed, removed thanks!


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

https://reviews.llvm.org/D73086