[PATCH] D74481: [Debuginfo][NFC] Create common error handlers for DWARFContext.

[Alexey Lapshin via Phabricator via llvm-commits]

avl marked an inline comment as done.
avl added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/DIContext.h:208
+
+  std::function<void(Error)> RecoverableErrorHandler = nullptr;
+  std::function<void(Error)> WarningHandler = nullptr;
----------------
JDevlieghere wrote:
> avl wrote:
> > jhenderson wrote:
> > > avl wrote:
> > > > JDevlieghere wrote:
> > > > > Could we make the default handlers static members of the DWARFContext and use those instead of nullptr?
> > > > i.e. to make:
> > > > 
> > > > 
> > > > ```
> > > > class DWARFContext {
> > > > ...
> > > >  static std::function<void(Error)> RecoverableErrorHandler = defaultErrorHandler;
> > > >  static std::function<void(Error)> WarningHandler = defaultWarningHandler;
> > > > }
> > > > 
> > > > struct DIDumpOptions {
> > > > .....
> > > >   std::function<void(Error)> RecoverableErrorHandler = DWARFContext::RecoverableErrorHandler;
> > > >   std::function<void(Error)> WarningHandler = DWARFContext::WarningHandler;
> > > > };
> > > > 
> > > > ```
> > > > 
> > > > The goal of this refactoring is to allow to redefine error handlers. So that, when DWARFContext is used inside the linker, the linker could specify it`s own error handlers. There would be possible to provide custom error handlers to DWARFContext:  either by passing them in the constructor, either by giving them through setters (setRecoverableErrorHandler()) methods. So that concrete instance of  DWARFContext would have custom error handlers. That scheme would not work if RecoverableErrorHandler and WarningHandler were static methods of DWARFContext.
> > > > 
> > > > So, I propose that we would not do this.
> > > > 
> > > > 
> > > > Additionally, DWARFContext is more specific class than DIDumpOptions. So it would not be good to refer it inside DIDumpOptions. It would probably be necessary to move ErrorHandlers into DIContext if that solution would be taken. 
> > > Something I've been thinking about is whether something really high-level, e.g. Error.h, has some default handlers that can be used throughout LLVM. Configuring it on a system-wide level may not be right, but at least the defaults could then be used by all the different tools, unless there was a good reason not to. What do you think?
> > Speaking of general solution, I think default handler of following form would not be useful in global Error.h: 
> > 
> > 
> > ```
> > static void defaultRecoverableErrorHandler ( Error Err ) {
> >     WithColor::error() << toString(std::move(E)) << '\n';
> > } 
> > 
> > ```
> > Because if it would be inserted in many places - then it would not be possible to replace it if necessary. It would be useful if the general solution allows a mechanism to replace default implementation. From that point of view, it would be better to avoid direct access to defaultRecoverableErrorHandler.
> > Some error-handling model could look like this :
> > 
> > ```
> > Error.h
> > class ErrorManager {
> > public:
> >    void setHandler ( std::function<void(Error)> H = defaultHandler ) {
> >     Handler = H; 
> >    }
> > 
> >    void handle (Error Err) {
> >      Handler(std::move(Err));
> >    }
> >    std::function<void(Error)> getHandler () {
> >       return Handler;  
> >    }
> > 
> > private:
> >    void defaultHandler ( Error Err ) {
> >      WithColor::error() << toString(std::move(Err)) << '\n';
> >    } 
> >    std::function<void(Error)> Handler;
> > };
> > 
> > ```
> > So that instead following code:
> > 
> > 
> > ```
> > if ( Error Err = foo() ) {
> >   defaultRecoverableErrorHandler(Err) 
> > }
> > 
> > ```
> > There would be something like this:
> > 
> > 
> > ```
> > if ( Error Err = foo() ) {
> >   Manager.handle(Err) 
> > }
> > 
> > ```
> > If default implementation would be OK, then the user could instantiate global single ErrorManager and do nothing more.
> > 
> > If the default implementation would not be OK, then it is possible to provide alternative handler or extend ErrorManager. 
> > 
> > above approach is similar to already existed:
> > 
> > void handleAllErrors(Error E, HandlerTs &&... Handlers) 
> > 
> > with the difference that handlers are already set and should not be specified at usage point.
> I agree that, given LLVM's library centric design, a default handler in Error.h is probably a bit risky, but I do think they would make sense to have in WithColor.h, which is already centered around printing. 
The case I am aware of is following : 


```
WithColor.h
static void defaultRecoverableErrorHandler ( Error Err ) {
    WithColor::error() << toString(std::move(E)) << '\n';
}

DIContext.h
struct DIDumpOptions {
.....
  std::function<void(Error)> RecoverableErrorHandler = defaultRecoverableErrorHandler;
};


DWARFContext.setRecoverableHandler(MyNewHandler);

DIDumpOptions options;
DWARFContext.dump(OS, options);   


```
Although there was set MyNewHandler for DWARFContext there would still be used defaultRecoverableErrorHandler. That behavior looks unintuitive. 

If that is not a big deal, I will continue with the default handler in WithColor.h solution.
Also, there is a workaround for that case:


```
DIDumpOptions options;
options.RecevableHandler = MyNewHandler;
DWARFContext.dump(OS, options);   

```

My original intention was to have a single redefinition point which would affect all usage places.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74481