[PATCH] D73086: [WIP][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/lib/DebugInfo/DWARF/DWARFContext.cpp:830
+  if (auto Err = MacinfoDWO->parse(this->getStringExtractor(), MacinfoDWOData,
+                                   false /* IsMacro */))
+    WithColor::defaultErrorHandler(std::move(Err));
----------------
dblaikie wrote:
> SouraVX wrote:
> > dblaikie wrote:
> > > Here and below, I believe the format LLVM prefers for naming boolean parameters is:
> > > ```
> > > /*IsMacro=*/false
> > > ```
> > > Having the comment before the literal, and using '='
> > > 
> > > 
> > Will do Thanks! for correcting this.
> > BTW, do you have any thoughts on error handling, in this implementation ?
> > Currently this implementation, handles errors while parsing(mostly macro section, but also bloats up `macinfo` too,/due to shared execution paths/), if any error encountered return(recoverable error) to caller and continue subsequent execution of `dump` routine which will eventually dump whatever has been correctly parsed and terminate finally.
> I haven't been paying too much attention to error handling - but that sounds reasonable/consistent with other work in improving error handling in libDebugInfoDWARF.
> 
> @jhenderson might have the most context on error handling philosophies here, I think
Using the RecoverableErrorHandler allows programs to either continue or stop depending on the library client, and seems to be a good fit here. I haven't yet had a chance to look at the whole thing to be certain, but in general we should avoid directly reporting errors within library code, and instead use handlers or return llvm::Error/Expected to provide the error information back up the pile.

You may want to consider whether there are cases where it makes sense to keep parsing within the section, even if something goes wrong. In that case, you'll need to pass the handler down the stack, call it, and then continue parsing.

If you're interested more in the topic, I did a lightning talk on it a couple of years ago, in relation to some older work I did in the .debug_line code. See https://www.youtube.com/watch?v=YSEY4pg1YB0 for the video.


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

https://reviews.llvm.org/D73086