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

[Sourabh Singh Tomar via Phabricator via llvm-commits]

SouraVX marked 3 inline comments as done.
SouraVX added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h:281
   /// Get a pointer to the parsed dwo DebugMacro object.
   const DWARFDebugMacro *getDebugMacinfoDWO();
 
----------------
probinson wrote:
> The name "MacinfoDWO" suggests the .dwo file has a .debug_macinfo section, rather than a .debug_macro section.  I have not been paying much attention to the .dwo stuff, but I'm a little surprised that you did not have to add a `getDebugMacroDWO()` method.
> Or, if .dwo files always have .debug_macro and never .debug_macinfo, then perhaps this method should be renamed?
If I understand you correctly, are you concerned about `debug_macro.dwo` section for dumping purpose, that's on my TODO list. I'm planning to do as a separate patch.
+ this was part of refactoring D75037.
Besides, if you have any thoughts on this patch overall implementation. Please share!


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:155
+  // FIXME: Add support for DWARF64
+  if (Flagdata == MACRO_OFFSET_SIZE) {
+    Error Err =
----------------
probinson wrote:
> You want to check only the single bit here, for example:
> `if (Flagdata & MACRO_OFFSET_SIZE == 1)`
> 
Thanks for this! I'll incorporate this in next revision alongside concerns of other reviewers if remaining.


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

https://reviews.llvm.org/D73086