[PATCH] D121801: [llmv-pdbutil] Replace ExitOnError with explicit error handling.

[Alexandre Ganea via Phabricator via llvm-commits]

aganea accepted this revision.
aganea added a comment.
This revision is now accepted and ready to land.

LGTM, thanks!

In D121801#3386781 <https://reviews.llvm.org/D121801#3386781>, @rnk wrote:

> Essentially, this looks good to me. I have some minor concerns that as we migrate dumping functionality into library code, it will tend to increase the code size of production tools such as the linker. Maybe archive semantics and section GC paper over those problems. However, in the end I don't think it makes sense to worry about unmeasured, hypothetical code size problems. If there are problems, I guess it would make sense to factor out "dumping" functionanlity from the PDB and CodeView libraries into their own CVDump helper library or something. This is similar to how we pulled YAML writing out of lib/Object.

@CarlosAlbertoEnciso Would you be able to provide some numbers please along with patch (2)? At least we'll be settled if this code move should go in a new lib or not.



================
Comment at: llvm/tools/llvm-pdbutil/DumpOutputStyle.cpp:455
 
-  ExitOnError Err("Unexpected error processing modules: ");
-
----------------
CarlosAlbertoEnciso wrote:
> aganea wrote:
> > The user would loose the context after removal of this line, if the message is printed on stderr. Can you move this line to `L201`, or replace it by an extra `StringError` if you plan on moving this entire file to the PDB library? It could be something like:
> > ```
> >   if (opts::dump::DumpSymbols) {
> >     auto EC = File.isPdb() ? dumpModuleSymsForPdb() : dumpModuleSymsForObj();
> >     if (EC)
> >       return joinErrors(createStringError(inconvertibleErrorCode(), "Unexpected error processing modules:"), EC);
> >   }
> > ```
> Very valid point.
> 
> The following functions are affected:
> 
> ```
> Error DumpOutputStyle::dumpModules();
> Error DumpOutputStyle::dumpModuleFiles();
> Error DumpOutputStyle::dumpSymbolStats();
> Error DumpOutputStyle::dumpModuleSymsForPdb();
> 
> ```
> What I am suggesting is to pass an additional parameter to the functions that are intended to be moved to the PDB library that now are returning an Error and add your suggested code to those functions.
> 
> ```
> if (EC)
>     return joinErrors(createStringError(inconvertibleErrorCode(), ExtraMessage), EC);
> ```
> 
I guess you can also move the `ExitOnError` closer to the caller site:
```
Error DumpOutputStyle::dump() {
  ...
  if (opts::dump::DumpModules) {
    ExitOnError Err("Unexpected error processing modules: ");
    Err(dumpModules());
  }
```


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

https://reviews.llvm.org/D121801