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

[Igor Kudrin via Phabricator via llvm-commits]

ikudrin 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,
----------------
SouraVX wrote:
> 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
It looks like I was not clear enough there. I meant to move the implementation into a private method `parseImpl()` and add two public methods `parseMacinfo()` and `parseMacro()`. This will make the public interface of the class clear while the implementation may be improved later.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:151
+      if (!IsMacro)
+        return createStringError(
+            errc::invalid_argument,
----------------
For consistency, the actions should be the same as in the `default` case. From the perspective of MACINFO sections, all these encodings are unknown.


================
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");
----------------
SouraVX wrote:
> 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
> ```
In general, if an invalid record can be safely skipped, we prefer to do that, generate a recoverable error, and continue parsing. Otherwise, recoverable errors would not exist at all.

Anyway, that is not very critical for this patch. Handling error cases may be improved later when/if someone needs that.


================
Comment at: llvm/test/DebugInfo/X86/debug-macro-multi-cu-strx.s:102
+       .long   .Linfo_string2
+       .long   12                      # Unit length
+       .short  5                       # Version
----------------
That is a bit weird that units with 3 and 2 string offsets both have the same unit length.


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

https://reviews.llvm.org/D78736