[PATCH] D78736: [DWARF5]: Added support for dumping strx forms in llvm-dwarfdump

[Sourabh Singh Tomar via Phabricator via llvm-commits]

SouraVX added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDebugMacro.h:108
+  // FIXME: We should provide a better abstraction.
+  Error parse(DWARFUnitVector::iterator_range Units,
+              DataExtractor StringExtractor, DWARFDataExtractor MacroData,
----------------
ikudrin wrote:
> The method looks weird. It would be better to add two separate methods to parse macro and macinfo sections respectively.
I thought we already had a discussion/agreement on this:
https://reviews.llvm.org/D78736#2007450


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:173
+      if (MacroContributionOffset == MacroToUnits.end())
+        return createStringError(errc::invalid_argument,
+                                 "Macro contribution of the unit not found");
----------------
ikudrin wrote:
> This probably should be a recoverable error. It would be good to report the offset of the macro table.
I guess it should be non-recoverable error since:
- Once an `Macro Offset` is not found, rest of the macro `unit` or a particular contribution of the CU(containing `strx` forms) becomes non-parseable(Since we cannot establish the mapping b/w macro contribution to String contribution). 
This is based on premises that the contribution is homogeneous, in a sense, contribution contains single type(in this case `strx`) form to represent the macro information. This is expected since most producers don't use multiple forms(`strp` `strx` or pre-v5 forms such as `define` ) when producing macro info for a CU.
This also holds true for the 2nd case/comment where a `String Contribution` is not found.
This seems recoverable error in a mixed form(`strp` or `DW_MACRO_define`) contribution scenario: but this is hand crafted case.
```
debug_macro: 
Macro Header
DW_MACRO_define_strx
....
DW_MACRO_define
```


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

https://reviews.llvm.org/D78736