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

[Igor Kudrin via Phabricator via llvm-commits]

ikudrin added a comment.

We are almost there.

I added comments only for one test file. Please, consider them thoroughly and apply similar adjustments to the other tests.



================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDebugMacro.h:85
   struct MacroList {
+    MacroHeader Header;
     SmallVector<Entry, 4> Macros;
----------------
I suppose it would be useful to add a comment that the value of 0 in `Header.Version` means that the parsed data is from a `.debug_macinfo[.dwo]` section and other fields in the `Header` are uninitialized in that case.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:855
+                               /*IsMacro=*/true))
+    return std::move(Err);
+  return Macro.get();
----------------
If you call this method twice, and the first call returns an error, the second call will still return some partially parsed `DWARFDebugMacro` object. This is inconsistent. You should call `Macro.reset()` before returning an error.

The same for two other similar methods.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:111
 
+    uint64_t StrOffset = 0;
     switch (E.Type) {
----------------
The variable is used only in `DW_MACRO_define_strp`/`DW_MACRO_undef_strp` cases. It does not need to preserve the value outside the handler. Thus, why not embrace the body of those cases in braces and move the variable there making it even more local?


================
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:
> SouraVX wrote:
> > 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.
> Okay. I'd guess that you could delete most of the strings, right?
Only `.Linfo_string3` is used. There are no references to other strings in the file, so they should be removed.

BTW, the comments for the strings are misleading, because all of them except `.Linfo_string0` have different offsets than stated.


================
Comment at: llvm/test/DebugInfo/X86/debug-macro-macinfo.s:4
+
+# RUN: llvm-mc -triple x86_64-unknown-linux -filetype=obj -dwarf-version=5 %s -o -|\
+# RUN: llvm-dwarfdump -debug-macro - | FileCheck -strict-whitespace -match-full-lines %s
----------------
jhenderson wrote:
> `|\` -> `| \` for readability
I guess you do not need `-dwarf-version=5`. It affects the format of the generated `.debug_line` section, but the parser does not read it and the test does not check anything related to it.


================
Comment at: llvm/test/DebugInfo/X86/debug-macro-macinfo.s:24
+	.byte	2                       # Flags: 32 bit, debug_line_offset present
+	.long	.Lline_table_start0     # lineptr
+	.byte	3                       # DW_MACRO_start_file
----------------
You do not need to reference the real line table because it is not used in the parser/dumper. Just use `0` here. And fix the comment.


================
Comment at: llvm/test/DebugInfo/X86/debug-macro-macinfo.s:33
+	.byte	0                       # End Of Macro List Mark
+	.section	.debug_str,"MS", at progbits,1
+.Linfo_string0:
----------------
There should be an empty line before `.section` to improve readability.


================
Comment at: llvm/test/DebugInfo/X86/debug-macro-macinfo.s:42
+	.asciz	"DWARF_VERSION 5"       # string offset=149
+	.section	.debug_line,"", at progbits
+.Lline_table_start0:
----------------
In fact, this section may be removed because it is not used.


================
Comment at: llvm/test/DebugInfo/X86/debug-macro-macinfo.s:46
+.Lcu_macro_begin1:
+	.byte	3
+	.byte	0
----------------
Please add comments for this section describing the values, the same way as that is done for `.debug_macro`.


================
Comment at: llvm/test/DebugInfo/X86/debug-macro-macinfo.s:48
+	.byte	0
+	.file	1 "/macro" "a-v4.c"
+	.byte	1
----------------
In fact, this definition also does not affect the test and can be removed.


================
Comment at: llvm/test/DebugInfo/X86/debug-macro-macinfo.s:52-55
+	.ascii	"DWARF_VERSION"
+	.byte	32
+	.byte	52
+	.byte	0
----------------
You may just use `.asciz "DWARF_VERSION 4"` instead. It is the same but more readable.


================
Comment at: llvm/test/DebugInfo/X86/debug-macro-macinfo.s:58-64
+	.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
----------------
This second `.debug_str` and all three strings inside it are not needed. Please, remove them.


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

https://reviews.llvm.org/D73086