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

[David Blaikie via Phabricator via llvm-commits]

dblaikie accepted this revision.
dblaikie added inline comments.
This revision is now accepted and ready to land.


================
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:
> 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.
+1 here - while I don't feel super invested in the error handling here, I'd like us to stick to consistent habits here - please at least provide a follow-up patch in short-order after this one lands that changes this to a non-fatal error & tests that.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:177-180
+      auto MacroContributionOffset = MacroToUnits.find(M->Offset);
+      if (MacroContributionOffset == MacroToUnits.end())
+        return createStringError(errc::invalid_argument,
+                                 "Macro contribution of the unit not found");
----------------
This variable name and error message seem a bit off. This isn't about the fact that the "macro contribution" for "the unit" couldn't be found - but somewhat the opposite "No compile unit found for this macro contribution (needed for discovering the str_offsets contribution when decoding a strx form in the macro contribution)" or something like that, perhaps.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:184
+              Data.getULEB128(&Offset));
+      if (!StrOffset.hasValue())
+        return createStringError(
----------------
Prefer just the straight boolean test:
```
if (!StrOffset)
```


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:190
+          MacroContributionOffset->second->getStringExtractor().getCStr(
+              &(*StrOffset));
       break;
----------------
No need for the extra () here, just:
```
&*StrOffset
```


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

https://reviews.llvm.org/D78736