[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.
----------------
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.


================
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
+
----------------
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).


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

https://reviews.llvm.org/D73086