Page MenuHomePhabricator

TypedError for recoverable error handling
ClosedPublic

Authored by lhames on Mar 1 2016, 12:47 PM.

Details

Summary

This patch adds the TypedError scheme that was initially posted on llvm-dev as an [RFC] in early February. It is largely unchanged from the original version, but adds unit tests and preliminary documentation in the programmers manual.

Diff Detail

Event Timeline

lhames updated this revision to Diff 49536.Mar 1 2016, 12:47 PM
lhames retitled this revision from to TypedError for recoverable error handling.
lhames updated this object.
lhames added a reviewer: chandlerc.
lhames set the repository for this revision to rL LLVM.
lhames changed the visibility from "Public (No Login Required)" to "All Users".
lhames added subscribers: llvm-commits, grosbach, dblaikie and 4 others.
lhames added a comment.Mar 1 2016, 1:15 PM

As described in the original RFC (see "Error handling in LLVM libraries." on the llvm-dev mailing list), this patch adds support for an error-handling system tentatively named TypedError. This scheme can be thought of as a hybrid between error returns an exceptions: Errors in the scheme are explicitly represented as return types in the API (like error codes), but error values are structured, user-defined types, similar to c++ exceptions.

The motivation and details of the scheme can be read in the original RFC, my expectation is that this thread will focus on the implementation details.

lhames added a subscriber: resistor.Mar 1 2016, 1:16 PM
dblaikie added inline comments.Mar 1 2016, 1:19 PM
unittests/Support/TypedErrorTest.cpp
247 ↗(On Diff #49536)

Oh, right, one of the things I was going to suggest is to see if you could make this less indented - since it'll involve potetnially multiple lambdas like this. If the API looked like this instead, the clang-tidy indent would be much less:

Oh, maybe not. Seems there's a special case for a /single/ lambda, like this:

catchAllTypedErrors(std::move(E), [&](std::unique_ptr<CustomError> CE) {
  ...
});

But I can't seem to tickle that formatting to cascade into multiple such lambdas... /maybe/ worth talking to the clang-tidy folks to see if there's a formatting (eg: when any number of trailing arguments are lambdas, to format it like this:

catchAllTypedErrors(std::move(E), [&](std::unique_ptr<CustomSubError> SE) {
  CustomErrorInfo2 = SE->getInfo();
  CustomErrorExtraInfo = SE->getExtraInfo();
  return TypedError();
}, [&](std::unique_ptr<CustomError> CE) {
  // Assert that the CustomError instance above is handled before the
  // CustomSubError - joinTypedErrors should preserve error ordering.
  if (CustomErrorInfo2 != 0)
    abort();
  CustomErrorInfo1 = CE->getInfo();
  return TypedError();
});

& we could try to figure out some way to not need the handleTypedError wrapper.. which might be nice even if we don't get the formatting improvement (less boilerplate is always a win)

Hi Lang,

I want to ask what I feel is an important high-level question here: how does this approach differ, fundamentally, from exceptions, and why should we go down the route of building our own error handling scheme rather than “just” adopting exceptions?

To answer my own question, my impression is that your proposed scheme is functionally similar to exceptions, with the major deviation of being opt-in rather than opt-out. While opt-in is intellectually appealing at first glance, I’m not convinced that it is actually the best solution to some of the problems at play here. In particular, an opt-in scheme will require threading the error-handling type system through many layers of generic and/or boilerplate code in order to allow various callbacks in target backends to report errors properly.

As a concrete example, every backend under the sun has some degree of implicit contract between the backend and the frontend regarding supported IR constructs. This can be as “simple” as ABI lowering and intrinsic names for a typical CPU target, but can be far more complex for targets with non-traditional execution models. In either case, it seems very desirable for the target in the abstract, and instruction selection in particular, to be able to report violations of this contract via the error reporting mechanism. However, to do some in the context of SelectionDAG or FastISel (and I see no particular reason why GlobalISel will be particularly different) would require threading error checking through vast swaths of code both in the instruction selection infrastructure and in the callbacks for the targets themselves. Moreover, instruction selection tends to be a performance sensitive component of the compiler, which argues in favor of exceptions which are, at least notionally, zero-cost.

I’m not claiming here to have a global optimally solution to the problem, and I emphatically do want to see LLVM be better behaved about error handling in general. But it’s not clear to me exactly where the right cost/benefit tradeoff lies in terms of error handling technologies.

—Owen

Gentle ping.

That's a great patch overall! I enjoyed reading it :)

Please see comments below.

docs/ProgrammersManual.rst
304

Don't you get warning for default with fully-covered switch in this case?

373

make_typed_error is a bit "long" (the "..." here will almost always lead to wrapping (bad for vertical space efficiency).
What about:

return TypedError<FloatingPointError>(...)

The same remark applies to all the API that repeat "TypedError" everywhere (catchTypedError, etc.). Can we at least drop the "typed" part where possible?

include/llvm/Support/TypedError.h
148 ↗(On Diff #49536)

Could this ctor be expressed in terms of the move assignment operator?

355 ↗(On Diff #49536)

s/pubilc/public

359 ↗(On Diff #49536)

I like reading the handle<MyError>(...) but won't it be actually handle<MyError>(...)?

496 ↗(On Diff #49536)

s/cla/class/

499 ↗(On Diff #49536)

I assumed you almost copy/pasted the ErrorOr implementation right? Some weirdness here and there but it has been pretty well tested so...

537 ↗(On Diff #49536)

What do you mean?

554 ↗(On Diff #49536)

The error storage never needs deletion?
Is it handled differently somehow?

642 ↗(On Diff #49536)

Doc.

lhames added inline comments.Mar 11 2016, 11:00 AM
docs/ProgrammersManual.rst
373

So far I've heard general enthusiasm (and no objections) to renaming this class and the utilities to just "Error". I'll re-work the patch - that should cut down on this boilerplate a little.

include/llvm/Support/TypedError.h
148 ↗(On Diff #49536)

Yes it could. I'll fix that up.

355 ↗(On Diff #49536)

Good catch.

359 ↗(On Diff #49536)

I don't follow the question?

If you mean that this syntax is a bit verbose - I agree. I'll see if I can improve this.

499 ↗(On Diff #49536)

I did. :)

537 ↗(On Diff #49536)

This comment was copied over from ErrorOr - I think it's stale. I'll get rid of it.

554 ↗(On Diff #49536)

Oops. This is an omission. ErrorOr doesn't need to delete its error storage because it's just a std::error_code, but TypedErrorOr does. I'll fix that up.

mehdi_amini added inline comments.Mar 11 2016, 11:04 AM
include/llvm/Support/TypedError.h
359 ↗(On Diff #49536)

The question was terrible (bad copy/paste). Let me phrase it again:

The doxygen says handle<MyError>(...), but from what I can understand the function prototype below does not match and the code would be handleTypedError<MyError>(...)`.
I like the former more :)

lhames updated this revision to Diff 50620.Mar 14 2016, 11:42 AM
lhames removed rL LLVM as the repository for this revision.
lhames changed the visibility from "All Users" to "Public (No Login Required)".

Big readability update:

(1) Main classes / utilities renamed from TypedError -> Error.
(2) Added ErrorHandlerTraits machinery to deduce handler type for handler RTTI checks. This removes the need to wrap handlers in a 'handleTypedError<T>' call.
(3) Rename catch* to handle*.

The new handler idiom is:

handleErrors(std::move(E),
  [](const CustomErrType &EI) {
    // ...
  },
  [](const CustomErrType2 &EI) {
    //...
  },
  ...);

Otherwise the scheme is unchanged from the original patch.

dblaikie added inline comments.Mar 14 2016, 4:19 PM
docs/ProgrammersManual.rst
419

Indentation

include/llvm/Support/Error.h
350–380

Where's all this get used? Didn't immediately spot any test cases related to member function error handlers.

448–462

Can this not be implemented with a lambda like other handlers? (something about the "appliesTo" functionality?)

617–618

That feels a tad heavyweight & perhaps nicer to do with just a couple of named functions, but I'm not wedded to the notion.

unittests/Support/ErrorTest.cpp
149

Might be worth having a consumeError that takes a non-const ref (lvalue ref) so you don't have to add the std::move. Just to make the tests a bit more terse. Seems easy/handy enough.

182

Is it worth testing named handlers separately from function objects? Is there anything special going on in the implementation that would make these cases different?

258–273

Probably worth bringing this sort of formatting to the attention of djasper and the clang-format folks, see if we can do anything better here

262

why are the error handlers returning errors? (maybe I'll figure this out before I finish reviewing the patch...)

So I see from the docs this is for dynamically unhandled errors - do you have a use case in mind for that already? Could we defer adding it until we do? Might it be worth allowing void returning handlers for the common case of a handler that won't dynamically fail to handle the error?

269–270

Presumably these two lines should be an EXPECT_ or ASSERT_ style macro, rather than an anonymous abort()?

280–288

what's going on witmh the indentation and brace matching here?

321

Test case might be easier to follow if the value returned was not itself another error (to demonstrate that it's entirely independent - maybe a string or unique_ptr<int> if you want a move-only thing, etc)

383–386

You can just roll the contents of the lambda into the EXPECT_DEATH:

EXPECT_DEATH({ Expected<int> A = make_error<CustomError>(42); }, ... );
lhames marked 2 inline comments as done.Mar 14 2016, 5:29 PM
lhames added inline comments.
include/llvm/Support/Error.h
350–380

Lambdas. :)

The six empty specializations are for (in order)

[](ErrT&) mutable { ... }
[](ErrT&) { ... }
[](const ErrT&) mutable { ... }
[](const ErrT&) { ... }
[](std::unique_ptr<ErrT>) mutable { ... }
[](std::unique_ptr<ErrT>) { ... }

I think 90% of lambdas will be of the form 'const ErrT&', and another 9% will be 'std::unique_ptr<ErrT>' (which is useful when you want to modify and "re-throw" an error).

The other combinations - mutable versions of the above, plus the non-const reference versions, were just signatures that I felt would be useful for debugging handlers or error types by adding counters.

448–462

Indeed it can - this is a hangover from the earlier design. I'll remove it.

617–618

This was just yanked over from ErrorOr. I'm not wedded to it either, but it's battle-tested, Since getting the corner cases right for this may be tricky I'd rather change it post-commit once this code has been hammered on a bit.

unittests/Support/ErrorTest.cpp
149

Sounds reasonable. I'll add that.

182

Named handlers are function pointers, whereas lambdas are member functions. These tests are exercising different ErrorHandlerTraits specializations.

258–273

This is actually clang-format output, and I think it gets it right here.

Very short lambdas may get put on the same line, which looks odd - I could suggest that they specialize that.

262

Basically enabling "rethrow"-like functionality. It applies in all the same cases that re-throw would be used.

I think I could add extra ErrorHandlerTraits specializations to detect void handlers and automatically return "Error::success()" into the handleErrors machinery.

269–270

I wasn't sure about how gtest would handle nesting an EXPECT call inside another EXPECT. I'll give it a shot and if nothing explodes I'm happy to switch it over.

280–288

clang-format. :)

Time to file a bug I guess.

321

I'm not sure I follow that.

Error handlers can only return Error (or, if I implement the extension described earlier, void).

383–386

Will do.

lhames updated this revision to Diff 50697.Mar 14 2016, 10:20 PM

Updated with suggested changes from Dave. Most importantly - error handlers can now have 'void' return type, which is equivalent to returning success.

dblaikie edited edge metadata.Mar 15 2016, 3:02 PM

Looking pretty good. Tried applying the patch - death tests that check for the specific text of the assertion are not portable as you've written them, because different platforms describe the assertion differently ("Assertion failed: "x"" versus "Assertion x failed"). Maybe just look for the text message you've passed to the assertion, rather than any flavor text around it. Though even that's not /whole/y reliable, of course (up to the implementation what it puts in that message) - but may work on the platforms we care about.

I think the ErrorHandlerTraits + test cases might be able to be simplified/reduced by implementing some more general and orthogonal constructs. If we had a trait for "arguments to a callable" (as is described here: http://stackoverflow.com/questions/21465394/accept-any-kind-of-callable-and-also-know-argument-type ) and then just used std::result_of, we could probably get away from this code having to worry about members V non-members, etc. Might be able to do the unique_ptr versus ref variants just with overloading & decltype to get the return type... it's a bit hand wavy, I realize, but happy to work through it in more detail if you like.

include/llvm/Support/Error.h
493

Drop this?

506

Drop the return here?

unittests/Support/ErrorTest.cpp
291

Drop this?

302

And this?

mehdi_amini added inline comments.Mar 15 2016, 3:18 PM
unittests/Support/ErrorTest.cpp
407

How does it work with assertions disabled? (alternatively: where is this identified as "requires: assert"?)

To avoid the issue reported by David, you might roll your own "assert" so that you control the exact printed message?

lhames marked an inline comment as done.Mar 15 2016, 4:46 PM

Updated patch should be in shortly.

include/llvm/Support/Error.h
493

Yep.

506

Also yep.

unittests/Support/ErrorTest.cpp
291

Yep.

302

Yep.

407

I'm updating the Error class to disable the 'Checked' bit in Release mode, so I'll wrap all these tests with #ifndef NDEBUG.

lhames updated this revision to Diff 50783.Mar 15 2016, 4:55 PM
lhames edited edge metadata.
lhames marked an inline comment as done.

Disable the 'Checked' bit on Error in release mode, update death unit-tests accordingly.

Also tidy up some handlers in the unit-test, and fix a test that was relying on implementation specific assertion output.

I think the ErrorHandlerTraits + test cases might be able to be simplified/reduced by implementing some more general and orthogonal constructs. If we had a trait for "arguments to a callable" (as is described here: http://stackoverflow.com/questions/21465394/accept-any-kind-of-callable-and-also-know-argument-type ) and then just used std::result_of, we could probably get away from this code having to worry about members V non-members, etc. Might be able to do the unique_ptr versus ref variants just with overloading & decltype to get the return type... it's a bit hand wavy, I realize, but happy to work through it in more detail if you like.

I think factoring out the argument-matching stuff into a general callable_traits class would be useful, and I'm happy to do it, . I need to do something like that anyway for some improvements I want to make to the RPC API. That said, I think we can do that post commit - I want to take some time to think about the right way to do it, and I don't want to block the people who are waiting for this Error support to land.

lhames accepted this revision.Mar 15 2016, 6:08 PM
lhames added a reviewer: lhames.

Accepted by Dave in mailing list correspondence.

This revision is now accepted and ready to land.Mar 15 2016, 6:08 PM
lhames closed this revision.Mar 15 2016, 6:09 PM

Committed in r263609.