<div dir="ltr"><div class="gmail_extra">Hi All,<div><br></div><div>Now that this thread has accumulated some feedback, I thought I'd try to summarize my thoughts on error handling in LLVM, and why I think this proposal is worth adopting:</div><div><br></div><div>(1) Failure to check an error *is* a programmatic error, and our error system should reflect this. This makes it easy to spot mistakes in our error handling and correct them.</div><div><br></div><div>(2) Error returns should describe all the information about an error that a client could reasonably use to recover from the error. This requirement goes beyond what a flat enum, or even an enum with a category (like std::error_code) is capable of expressing. For example, it is possible to express "missing_file" in an enum, but not "missing_file: /path/to/file". A library client could meaningfully respond to the latter by re-generating the file (if they knew how) and re-trying the operation.</div><div><br></div><div>(3) There may be many levels of stack between the point where an error is raised, and the point where it can be handled. A generic error handling system needs to bridge this gap, bringing all the information needed to handle an error together in one place. In the process, the system should unwind through all levels of the stack that can't handle the error allowing resources to be freed, and ensuring execution does not continue into code that can't handle the error. (A corollary to this is that a generic error handling system should never respond to an error in user-supplied input by aborting).</div><div><br></div><div>(4) Errors should be easy to break on in a debugger.</div><div><br></div><div>(5) Errors should have minimal / no overhead in the success case.</div><div><br></div><div><br></div><div>I think we have consensus that point (1) is a serious problem. My scheme addresses this problem by including a 'checked' bit that verifies that errors (and even success values) have been inspected before being discarded. However, we could do something similar with a wrapper around std::error_code without adopting the custom-error-class aspect of my proposal.</div><div><br></div><div>Points (2) and (3) (which are closely related) can't be addressed with std::error_code. My scheme does address these points by allowing users to define custom error classes.</div><div><br></div><div>Point (4) is addressed in my scheme by setting breakpoints on error class constructors. With a wrapper around std::error_code, the best you can do is to set a conditional breakpoint looking for construction with a particular enum value.</div><div><br></div><div>Point (5) is a wash - both TypedError and std::error_code are cheap in the success case.</div><div><br></div><div><br></div><div>One of the concerns that has been raised with my system is complexity. I don't think the system is overly complex conceptually (though the implementation does involve some non-trivial template goop). I presented the system in detail in my original email, but I'll try to summarize here:</div><div><br></div><div>This system provides an owning "error pointer" class, TypedError, that is as cheap as std::error_code to move around, return from functions, etc. Success values are indicated by a null "error pointer" value that is extremely cheap to construct. Failure values are indicated by a non-null error-pointer. The cost of constructing a failure value is just the cost of constructing the pointee describing the error, plus the cost of handling the error (if that happens). The pointee must be a user-defined class that works with the typed-error RTTI system. We provide a utility class, TypedErrorInfo, to make it easy to construct such classes:</div><div><br></div><div>class MyError : TypedErrorInfo<MyError> { ... };</div><div><br></div><div>User defined error classes must also implement a log method to render the error to a stream, however this can usually be done in a couple of lines. Since we expect the number of user-defined error classes to be small, this overhead should be minimal. For usage examples on all this, please see the error demo attached to my original email.</div><div><br></div><div>So, given the benefits of this proposal, I think the complexity is well justified.</div><div><br></div><div>An alternative scheme that has been floated is using a combination of error_code and diagnostic handlers, but I have two strong objections to this as a general solution:</div><div><br></div><div>(1) Error returns are already required to ensure that code further up the stack does not continue past an error. Given the fundamental necessity of error returns, it makes sense to re-use them for diagnostics where possible, as it avoids the need to *also* thread a diagnostic handler through any function that could fail. If used as a general solution, diagnostic handlers would quickly become unwieldy, especially when calling between libraries. For example, if library A calls library B, which calls library C, and library C can produce an error, you are now forced thread a diagnostic handler through library A and B for library C's sake. This does not scale well.</div><div><br></div><div>I recognize that diagnostic handlers have totally valid use cases. I am not proposing that we remove any of the existing diagnostic handlers from LLVM, and I'm not against introducing new ones where there is a use-case. However, in use-cases where TypedError is necessary to enable recovery and sufficient to produce good error messages, I think diagnostic handlers are superfluous.</div><div><br></div><div>(2) By retaining std::error_code, the error_code + diagnostic-handler scheme still falls short on points (2) and (3) on my original list. That is, the combination of diagnostic handlers and std::error_code only works if the best "recovery" you can do is report an error on a stream and then bail out or continue, based on a coarse-grained error_code. That's good enough for a lot of command-line utilities, but not good enough for general library design.</div><div><br></div><div>Responses to this proposal seem mostly positive so far, and I hope this summary addresses the concerns that have been raised, but I'm very happy to continue the conversation if not.</div><div><br></div><div>That said, I'm really keen to start improving error handling in libObject and the JIT, so in the interest of maintaining some momentum on this proposal, here's how I plan to proceed if this proposal is adopted: </div><div><br></div><div>(1) Write up a set of unit-tests for the TypedError system, and a new section of the LLVM Programmer's Manual describing its use.</div><div><br></div><div>(2) Re-submit the original prototype (with minor tweaks) plus the unit-tests and programmers manual additions to llvm-commits fro review.</div><div><br></div><div>(3) Begin threading TypedError and TypedErrorOr through MachOObjectFile in libObject, using ECError as a bridge between TypedError/TypedErrorOr and std::error_code/ErrorOr.</div><div><br></div><div>The act of threading this through libObject will hit LLD, MCJIT and ORC, DebugInfo, LTO, ProfilingData and a host of command-line utilities. However, the ECError glue is fairly straightforward and idiomatic, and provides us with an explicit (grepable) surface area that can be chipped away at. Working with Kevin Enderby, my next step would be to plumb TypedError through the MachO-specific parts of llvm-nm and llvm-objdump. Based on our experiments with the prototype, we expect to be able to significantly improve the diagnostics produced by these tools for broken object files.</div><div><br></div><div>In the long-term I expect many parts of LLVM may want to migrate from std::error_code to TypedError, but that's ultimately a decision for the maintainers of the respective parts. I'll be very happy to provide input and support, but I don't want to pre-empt anyone else's decision on whether to adopt this.</div><div><br></div><div>Cheers,</div><div>Lang.</div><div><br></div><div class="gmail_quote">On Wed, Feb 10, 2016 at 1:36 PM, Lang Hames <span dir="ltr"><<a href="mailto:lhames@gmail.com" target="_blank">lhames@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><span><div><font><span style="background-color:rgba(255,255,255,0)"><font color="#000000">Hi Rafael,</font></span></font></div><div><br></div><div><font><div style="color:rgb(80,0,80);font-size:13px"><blockquote type="cite"><font color="#000000"><span style="background-color:rgba(255,255,255,0)">What prevents you from using a diag handler in the jit server that<br>sends errors/warnings over the RPCChannel?</span></font></blockquote></div></font></div><div><font><span style="background-color:rgba(255,255,255,0)"><font color="#000000"><br></font></span></font></div></span><span><div><font><span style="background-color:rgba(255,255,255,0)"><font color="#000000">Sorry - this is a non-trivial problem to </font></span></font>think through, so to speed things up, here are the issues:</div><div style="font-size:13px"><br></div></span><div style="font-size:13px">(1) A diagnostic handler can only exit or continue, neither of which are sufficient to deal with an error in the general case. You need to be able to stop and unwind to some point in the stack where the error can be handled, meaningfully releasing / cleaning-up resources as you go.</div><span><div style="font-size:13px"><br></div><div style="font-size:13px">(2) Using a diagnostic handler for structured error serialization removes the cohesion between error serialization / deserialization and the error types themselves. It also removes the cohesion between serialization and deserialization: serialization happens in the diagnostic handler, deserialization somewhere else. All this makes it very easy to forget to update serialization/deserialization code when error types change, and for serialization/deserialization to get out of sync with one another. Diagnostic handlers really aren't designed for this problem.</div><div style="font-size:13px"><br></div><div style="font-size:13px">In my design, anyone building a client/server system on top of ORC can make any error reportable over the wire by inheriting from some "Serializable" interface and writing their serialization/deserialization code in the error type itself. The server can then contain a checkTypedError block that boils down to:</div><div style="font-size:13px"><br></div><div style="font-size:13px"><font face="monospace, monospace">while (1) {</font></div><div style="font-size:13px"><font face="monospace, monospace">  if (auto Err = handleServerCommand(Channel))</font></div><div style="font-size:13px"><font face="monospace, monospace">    if (Err is handleable by the server)</font></div><div style="font-size:13px"><font face="monospace, monospace">      /* handle it */</font></div><div style="font-size:13px"><font face="monospace, monospace">    else if (Err.isA<Serializable>())</font></div><div style="font-size:13px"><font face="monospace, monospace">      Err->serialize(Channel); /* Report it to the client, let them respond */</font></div><div style="font-size:13px"><font face="monospace, monospace">    else</font></div><div style="font-size:13px"><font face="monospace, monospace">      return Err; /* Bail out of the server loop */</font></div><div style="font-size:13px"><font face="monospace, monospace">}</font></div><div style="font-size:13px"><font face="monospace, monospace"><br></font></div></span><div style="font-size:13px"><font face="monospace, monospace">Cheers,</font></div><div style="font-size:13px"><font face="monospace, monospace">Lang.</font></div></div><div class="gmail_extra"><br><div class="gmail_quote"><span>On Wed, Feb 10, 2016 at 10:55 AM, Lang Hames <span dir="ltr"><<a href="mailto:lhames@gmail.com" target="_blank">lhames@gmail.com</a>></span> wrote:<br></span><div><div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="auto"><div>Hi Rafael,</div><div><br></div><div><span><blockquote type="cite"><font color="#000000"><span style="background-color:rgba(255,255,255,0)">What prevents you from using a diag handler in the jit server that<br>sends errors/warnings over the RPCChannel?</span></font></blockquote><div><br></div></span><div>What would you do with errors that can't reasonable be serialized when they reach the diagnostic handler?</div><div><br></div>And what would you do with the serialized bytes on the client end?</div><span><div><br></div><div>- Lang.<br><br>Sent from my iPhone</div></span><div><div><div><br>On Feb 10, 2016, at 10:31 AM, Rafael Espíndola <<a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@gmail.com</a>> wrote:<br><br></div><blockquote type="cite"><div><blockquote type="cite"><span>I recently added support for remote JITing to ORC. There are in-tree library</span><br></blockquote><blockquote type="cite"><span>facilities for JITing code into a process on the other end of an abstract</span><br></blockquote><blockquote type="cite"><span>"RPCChannel". What happens if something goes wrong at one end? We want to be</span><br></blockquote><blockquote type="cite"><span>able to communicate an error back across the RPCChannel (assuming it's still</span><br></blockquote><blockquote type="cite"><span>intact) so the other side can recover or fail gracefully. That means we need</span><br></blockquote><blockquote type="cite"><span>to be able to serialize an error with enough information to describe what</span><br></blockquote><blockquote type="cite"><span>went wrong. There's no practical way to maintain a serialization routine for</span><br></blockquote><blockquote type="cite"><span>all possible std::error_codes that might come up, even if they were powerful</span><br></blockquote><blockquote type="cite"><span>enough to describe everything that could go wrong (which again, being static</span><br></blockquote><blockquote type="cite"><span>kinds, they're not). With my proposal however, a JITError base class can be</span><br></blockquote><blockquote type="cite"><span>defined as:</span><br></blockquote><blockquote type="cite"><span></span><br></blockquote><blockquote type="cite"><span>class JITError : public TypedErrorInfo<JITError> {</span><br></blockquote><blockquote type="cite"><span>public:</span><br></blockquote><blockquote type="cite"><span>  virtual void serialize(RPCChannel &C) const = 0;</span><br></blockquote><blockquote type="cite"><span>};</span><br></blockquote><blockquote type="cite"><span></span><br></blockquote><span></span><br><span>What prevents you from using a diag handler in the jit server that</span><br><span>sends errors/warnings over the RPCChannel?</span><br><span></span><br><span>Cheers,</span><br><span>Rafael</span><br></div></blockquote></div></div></div></blockquote></div></div></div><br></div>
</blockquote></div><br></div></div>