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

Tue Mar 17 03:44:10 PDT 2020

SouraVX marked 4 inline comments as done.
SouraVX 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.
----------------
jhenderson wrote:
> SouraVX wrote:
> > jhenderson wrote:
> > > parsing a macinfo...
> > > it self -> itself
> > Is it okay to keep it the way it self or something like "`macinfo` or `macinfo.dwo` section".
> > Since this also covers `macinfo.dwo` case also. ?
> Sorry, `macinfo[.dwo]` was fine (I wanted to correct the like of 'a'). Feel free to reinstate that bit.
Sure will do!

================
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:
> SouraVX wrote:
> > jhenderson wrote:
> > > 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?
> > I think we don't need one "verbose" case, since there is no difference b/w both cases.
> Does verbose mode dump at a different time for .debug_macro etc, like the .debug_line code does? If it does, we should probably still have it (but obviously it can share the same checks).
No, Verbose mode isn't part of macro/macinfo since the beginning. I think it's due to nature of it's contents.

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

https://reviews.llvm.org/D73086