[PATCH] D73086: [DWARF5] Added support for debug_macro section parsing and dumping in llvm-dwarfdump.
James Henderson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Mar 16 03:53:19 PDT 2020
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
More information about the llvm-commits
mailing list