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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDebugMacro.h:87
+    // A value 0 in the `Header.Version` field indicates that we're parsing
+    // macinfo[.dwo] section which doesn't have header it self, hence
+    // for that case other fields in the `Header` are uninitialized.
----------------
parsing a macinfo...
it self -> itself


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:855
+                               /*IsMacro=*/true))
+    return std::move(Err);
+  return Macro.get();
----------------
ikudrin wrote:
> 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.
This sort of change is exactly why I think we should avoid duplicating the logic in three places. It would have been easy to add it only in one place.


================
Comment at: llvm/test/DebugInfo/X86/debug-macro-macinfo.s:2
+## This test checks that llvm-dwarfdump can dump both debug_macro and debug_macinfo
+## sections present in the single object.
+
----------------
Still need to do "the same object" instead of "the single object".


================
Comment at: llvm/test/DebugInfo/X86/debug-macro-macinfo.s:2
+# RUN: llvm-mc -triple x86_64-unknown-linux --filetype=obj -dwarf-version=5 %s -o -|\
+# RUN: llvm-dwarfdump -v - | FileCheck %s
+
----------------
jhenderson wrote:
> You should have a verbose and non-verbose version of this test, as the printing is usually different.
> 
> Also, consider adding --strict-whitespace and --match-full-lines to the FileCheck call to ensure the whole content is checked and spacing is correct.
I don't see a verbose case still?


================
Comment at: llvm/test/DebugInfo/X86/debug-macro-v5.s:7
+
+# CHECK:.debug_macro contents:
+# CHECK-NEXT:0x00000000:
----------------
SouraVX wrote:
> jhenderson wrote:
> > SouraVX wrote:
> > > jhenderson wrote:
> > > > `#      CHECK:.debug_macro contents:`
> > > > 
> > > > same below for the other `CHECK:` line.
> > > Is spacing the you're concerned about here ?
> > Right. It allows easier reading of the line, because it lines up the actual content being checked with the rest of the content from the subsequent CHECK-NEXT lines.
> To avoid same mistakes again, and avoiding confusion -- Following modification you're suggesting -
> 
> `#  CHECK:.debug_macro contents:` -- CHECK pattern(1 leading space) in all test cases
> should modified be modified with "2" leading spaces in all test cases i.e 
> `#    CHECK:debug_macro `
> and like wise in subsequent `CHECK-NEXT` ?
Thanks, this bit looks fine now.


================
Comment at: llvm/test/DebugInfo/X86/unsupported-opcode_operands_table-debug-macro-v5.s:2
+## This test checks llvm-dwarfdump emits correct error diagnostics for the
+## unsupported case where opcode_operands_table flag is present in the debug_macro
+## section header.
----------------
where the opcode_operands_table


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

https://reviews.llvm.org/D73086