[llvm-dev] [RFC] Error handling in LLVM libraries.

[Lang Hames via llvm-dev]

Hi All,

I've been thinking lately about how to improve LLVM's error model and error reporting. A lack of good error reporting in Orc and MCJIT has forced me to spend a lot of time investigating hard-to-debug errors that could easily have been identified if we provided richer error information to the client, rather than just aborting. Kevin Enderby has made similar observations about the state of libObject and the difficulty of producing good error messages for damaged object files. I expect to encounter more issues like this as I continue work on the MachO side of LLD. I see tackling the error modeling problem as a first step towards improving error handling in general: if we make it easy to model errors, it may pave the way for better error handling in many parts of our libraries.

At present in LLVM we model errors with std::error_code (and its helper, ErrorOr) and use diagnostic streams for error reporting. Neither of these seem entirely up to the job of providing a solid error-handling mechanism for library code. Diagnostic streams are great if all you want to do is report failure to the user and then terminate, but they can't be used to distinguish between different kinds of errors, and so are unsuited to many use-cases (especially error recovery). On the other hand, std::error_code allows error kinds to be distinguished, but suffers a number of drawbacks:

1. It carries no context: It tells you what went wrong, but not where or why, making it difficult to produce good diagnostics.

2. It's extremely easy to ignore or forget: instances can be silently dropped.

3. It's not especially debugger friendly: Most people call the error_code constructors directly for both success and failure values. Breakpoints have to be set carefully to avoid stopping when success values are constructed.

In fairness to std::error_code, it has some nice properties too:

1. It's extremely lightweight.

2. It's explicit in the API (unlike exceptions).

3. It doesn't require C++ RTTI (a requirement for use in LLVM).

To address these shortcomings I have prototyped a new error-handling scheme partially inspired by C++ exceptions. The aim was to preserve the performance and API visibility of std::error_code, while allowing users to define custom error classes and inheritance relationships between them. My hope is that library code could use this scheme to model errors in a meaningful way, allowing clients to inspect the error information and recover where possible, or provide a rich diagnostic when aborting.

The scheme has three major "moving parts":

1. A new 'TypedError' class that can be used as a replacement for std::error_code. E.g.

std::error_code foo();

becomes

TypedError foo();

The TypedError class serves as a lightweight wrapper for the real error information (see (2)). It also contains a 'Checked' flag, initially set to false, that tracks whether the error has been handled or not. If a TypedError is ever destructed without being checked (or passed on to someone else) it will call std::terminate(). TypedError cannot be silently dropped.

2. A utility class, TypedErrorInfo, for building error class hierarchies rooted at 'TypedErrorInfoBase' with custom RTTI. E.g.

// Define a new error type implicitly inheriting from TypedErrorInfoBase.

class MyCustomError : public TypedErrorInfo<MyCustomError> {

public:

  // Custom error info.

};

// Define a subclass of MyCustomError.

class MyCustomSubError : public TypedErrorInfo<MyCustomSubError, MyCustomError> {

public:

  // Extends MyCustomError, adds new members.

};

3.  A set of utility functions that use the custom RTTI system to inspect and handle typed errors. For example 'catchAllTypedErrors' and 'handleTypedError' cooperate to handle error instances in a type-safe way:

TypedError foo() {

  if (SomeFailureCondition)

    return make_typed_error<MyCustomError>();

}

TypedError Err = foo();

catchAllTypedErrors(std::move(Err),

  handleTypedError<MyCustomError>(

    [](std::unique_ptr<MyCustomError> E) {

      // Handle the error.

      return TypedError(); // <- Indicate success from handler.

    }

  )

);

If your initial reaction is "Too much boilerplate!" I understand, but take comfort: (1) In the overwhelmingly common case of simply returning errors, the usage is identical to std::error_code:

if (TypedError Err = foo())

  return Err;

and (2) the boilerplate for catching errors is usually easily contained in a handful of utility functions, and tends not to crowd the rest of your source code. My initial experiments with this scheme involved updating many source lines, but did not add much code at all beyond the new error classes that were introduced.

I believe that this scheme addresses many of the shortcomings of std::error_code while maintaining the strengths: 

1. Context - Custom error classes enable the user to attach as much contextual information as desired.

2. Difficult to drop - The 'checked' flag in TypedError ensures that it can't be dropped, it must be explicitly "handled", even if that only involves catching the error and doing nothing.

3. Debugger friendly - You can set a breakpoint on any custom error class's constructor to catch that error being created. Since the error class hierarchy is rooted you can break on TypedErrorInfoBase::TypedErrorInfoBase to catch any error being raised.

4. Lightweight - Because TypedError instances are just a pointer and a checked-bit, move-constructing it is very cheap. We may also want to consider ignoring the 'checked' bit in release mode, at which point TypedError should be as cheap as std::error_code.

5. Explicit - TypedError is represented explicitly in the APIs, the same as std::error_code.

6. Does not require C++ RTTI - The custom RTTI system does not rely on any standard C++ RTTI features.

This scheme also has one attribute that I haven't seen in previous error handling systems (though my experience in this area is limited): Errors are not copyable, due to ownership semantics of TypedError. I think this actually neatly captures the idea that there is a chain of responsibility for dealing with any given error. Responsibility may be transferred (e.g. by returning it to a caller), but it cannot be duplicated as it doesn't generally make sense for multiple people to report or attempt to recover from the same error.

I've tested this prototype out by threading it through the object-creation APIs of libObject and using custom error classes to report errors in MachO headers. My initial experience is that this has enabled much richer error messages than are possible with std::error_code.

To enable interaction with APIs that still use std::error_code I have added a custom ECError class that wraps a std::error_code, and can be converted back to a std::error_code using the typedErrorToErrorCode function. For now, all custom error code classes should (and do, in the prototype) derive from this utility class. In my experiments, this has made it easy to thread TypedError selectively through parts of the API. Eventually my hope is that TypedError could replace std::error_code for user-facing APIs, at which point custom errors would no longer need to derive from ECError, and ECError could be relegated to a utility for interacting with other codebases that still use std::error_code.

So - I look forward to hearing your thoughts. :)

Cheers,

Lang.

Attached files:

typed_error.patch - Adds include/llvm/Support/TypedError.h (also adds anchor() method to lib/Support/ErrorHandling.cpp).

error_demo.tgz - Stand-alone program demo'ing basic use of the TypedError API.

libobject_typed_error_demo.patch - Threads TypedError through the binary-file creation methods (createBinary, createObjectFile, etc). Proof-of-concept for how TypedError can be integrated into an existing system.

[Hal Finkel via llvm-dev]

Generically, I like this idea.

However, regarding context, I wonder about the best model. When we designed the diagnostic reporting interface (by which I mean the bits in include/llvm/IR/DiagnosticInfo.h and include/llvm/IR/DiagnosticPrinter.h), the ability to carry context was very important. There, however, because the objects are being passed via a callback to the user-installed handler, they can carry pointers/references to objects (Values, Functions, etc.) that will go away once the object that detected the error is destroyed. In the model you're proposing, all of the context must be contained within the error object itself (because, by the time the context is useful, an arbitrary amount of the call stack to the error-detection point has already been unwound). This greatly limits the amount of information that can be efficiently stored as context in the error object. Depending on the use cases, it might be better to pass the context to some kind of error-handler callback than to try to pack it all into a long-lived error object. Thoughts?

Thanks again,
Hal

> _______________________________________________
> LLVM Developers mailing list
> llvm...@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>

--
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory
_______________________________________________
LLVM Developers mailing list
llvm...@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

[James Y Knight via llvm-dev]

Regarding one point in particular:

2. Difficult to drop - The 'checked' flag in TypedError ensures that it can't be dropped, it must be explicitly "handled", even if that only involves catching the error and doing nothing.

It seems to me that "[[clang::warn_unused_result]] class TypedError" is probably sufficient for ensuring people check a status return value; I'm not sure runtime checking really brings much additional value there.

[Lang Hames via llvm-dev]

Hi James,

It seems to me that "[[clang::warn_unused_result]] class TypedError" is probably sufficient for ensuring people check a status return value; I'm not sure runtime checking really brings much additional value there.

I see the attribute as complimentary. The runtime check provides a stronger guarantee: the error cannot be dropped on any path, rather than just "the result is used". The attribute can help you catch obvious violations of this at compile time.

- Lang.

Sent from my iPhone

[Mehdi Amini via llvm-dev]

Hi Lang,

I’m glad someone tackle this long lived issue :)

I’ve started to think about it recently but didn’t as far as you did!

On Feb 2, 2016, at 5:29 PM, Lang Hames via llvm-dev <llvm...@lists.llvm.org> wrote:

Hi All,

I've been thinking lately about how to improve LLVM's error model and error reporting. A lack of good error reporting in Orc and MCJIT has forced me to spend a lot of time investigating hard-to-debug errors that could easily have been identified if we provided richer error information to the client, rather than just aborting. Kevin Enderby has made similar observations about the state of libObject and the difficulty of producing good error messages for damaged object files. I expect to encounter more issues like this as I continue work on the MachO side of LLD. I see tackling the error modeling problem as a first step towards improving error handling in general: if we make it easy to model errors, it may pave the way for better error handling in many parts of our libraries.

At present in LLVM we model errors with std::error_code (and its helper, ErrorOr) and use diagnostic streams for error reporting. Neither of these seem entirely up to the job of providing a solid error-handling mechanism for library code. Diagnostic streams are great if all you want to do is report failure to the user and then terminate, but they can't be used to distinguish between different kinds of errors

I’m not sure to understand this claim? You are supposed to be able to extend and subclass the type of diagnostics? (I remember doing it for an out-of-tree LLVM-based project).

, and so are unsuited to many use-cases (especially error recovery). On the other hand, std::error_code allows error kinds to be distinguished, but suffers a number of drawbacks:

1. It carries no context: It tells you what went wrong, but not where or why, making it difficult to produce good diagnostics.

2. It's extremely easy to ignore or forget: instances can be silently dropped.

3. It's not especially debugger friendly: Most people call the error_code constructors directly for both success and failure values. Breakpoints have to be set carefully to avoid stopping when success values are constructed.

In fairness to std::error_code, it has some nice properties too:

1. It's extremely lightweight.

2. It's explicit in the API (unlike exceptions).

3. It doesn't require C++ RTTI (a requirement for use in LLVM).

To address these shortcomings I have prototyped a new error-handling scheme partially inspired by C++ exceptions. The aim was to preserve the performance and API visibility of std::error_code, while allowing users to define custom error classes and inheritance relationships between them. My hope is that library code could use this scheme to model errors in a meaningful way, allowing clients to inspect the error information and recover where possible, or provide a rich diagnostic when aborting.

The scheme has three major "moving parts":

1. A new 'TypedError' class that can be used as a replacement for std::error_code. E.g.

std::error_code foo();

becomes

TypedError foo();

The TypedError class serves as a lightweight wrapper for the real error information (see (2)). It also contains a 'Checked' flag, initially set to false, that tracks whether the error has been handled or not. If a TypedError is ever destructed without being checked (or passed on to someone else) it will call std::terminate(). TypedError cannot be silently dropped.

I really like the fact that not checking the error triggers an error (this is the "hard to misuse” part of API design IMO).

You don’t mention it, but I’d rather see this “checked” flag compiled out with NDEBUG.

2. A utility class, TypedErrorInfo, for building error class hierarchies rooted at 'TypedErrorInfoBase' with custom RTTI. E.g.

// Define a new error type implicitly inheriting from TypedErrorInfoBase.

class MyCustomError : public TypedErrorInfo<MyCustomError> {

public:

  // Custom error info.

};

// Define a subclass of MyCustomError.

class MyCustomSubError : public TypedErrorInfo<MyCustomSubError, MyCustomError> {

public:

  // Extends MyCustomError, adds new members.

};

3.  A set of utility functions that use the custom RTTI system to inspect and handle typed errors. For example 'catchAllTypedErrors' and 'handleTypedError' cooperate to handle error instances in a type-safe way:

TypedError foo() {

  if (SomeFailureCondition)

    return make_typed_error<MyCustomError>();

}

TypedError Err = foo();

catchAllTypedErrors(std::move(Err),

  handleTypedError<MyCustomError>(

    [](std::unique_ptr<MyCustomError> E) {

      // Handle the error.

      return TypedError(); // <- Indicate success from handler.

What does success or failure means for the handler?

    }

  )

);

If your initial reaction is "Too much boilerplate!" I understand, but take comfort: (1) In the overwhelmingly common case of simply returning errors, the usage is identical to std::error_code:

if (TypedError Err = foo())

  return Err;

and (2) the boilerplate for catching errors is usually easily contained in a handful of utility functions, and tends not to crowd the rest of your source code. My initial experiments with this scheme involved updating many source lines, but did not add much code at all beyond the new error classes that were introduced.

I believe that this scheme addresses many of the shortcomings of std::error_code while maintaining the strengths: 

1. Context - Custom error classes enable the user to attach as much contextual information as desired.

2. Difficult to drop - The 'checked' flag in TypedError ensures that it can't be dropped, it must be explicitly "handled", even if that only involves catching the error and doing nothing.

3. Debugger friendly - You can set a breakpoint on any custom error class's constructor to catch that error being created. Since the error class hierarchy is rooted you can break on TypedErrorInfoBase::TypedErrorInfoBase to catch any error being raised.

4. Lightweight - Because TypedError instances are just a pointer and a checked-bit, move-constructing it is very cheap. We may also want to consider ignoring the 'checked' bit in release mode, at which point TypedError should be as cheap as std::error_code.

Oh here you mention compiling out the “checked” flag :)

5. Explicit - TypedError is represented explicitly in the APIs, the same as std::error_code.

6. Does not require C++ RTTI - The custom RTTI system does not rely on any standard C++ RTTI features.

This scheme also has one attribute that I haven't seen in previous error handling systems (though my experience in this area is limited): Errors are not copyable, due to ownership semantics of TypedError. I think this actually neatly captures the idea that there is a chain of responsibility for dealing with any given error. Responsibility may be transferred (e.g. by returning it to a caller), but it cannot be duplicated as it doesn't generally make sense for multiple people to report or attempt to recover from the same error.

I've tested this prototype out by threading it through the object-creation APIs of libObject and using custom error classes to report errors in MachO headers. My initial experience is that this has enabled much richer error messages than are possible with std::error_code.

To enable interaction with APIs that still use std::error_code I have added a custom ECError class that wraps a std::error_code, and can be converted back to a std::error_code using the typedErrorToErrorCode function. For now, all custom error code classes should (and do, in the prototype) derive from this utility class. In my experiments, this has made it easy to thread TypedError selectively through parts of the API. Eventually my hope is that TypedError could replace std::error_code for user-facing APIs, at which point custom errors would no longer need to derive from ECError, and ECError could be relegated to a utility for interacting with other codebases that still use std::error_code.

So - I look forward to hearing your thoughts. :)

Is your call to catchAllTypedErrors(…) actually like a switch on the type of the error? What about a syntax that looks like a switch?

switchErr(std::move(Err))

    .case< MyCustomError>([] () { /* … */ })

    .case< MyOtherCustomError>([] () { /* … */ })

    .default([] () { /* … */ })

— 

Mehdi       

Cheers,

Lang.

Attached files:

typed_error.patch - Adds include/llvm/Support/TypedError.h (also adds anchor() method to lib/Support/ErrorHandling.cpp).

error_demo.tgz - Stand-alone program demo'ing basic use of the TypedError API.

libobject_typed_error_demo.patch - Threads TypedError through the binary-file creation methods (createBinary, createObjectFile, etc). Proof-of-concept for how TypedError can be integrated into an existing system.

<typed_error.patch><error_demo.tgz><thread_typederror_through_object_creation.patch>_______________________________________________

[Lang Hames via llvm-dev]

Hi Hal,

However, regarding context, I wonder about the best model. When we designed the diagnostic reporting interface (by which I mean the bits in include/llvm/IR/DiagnosticInfo.h and include/llvm/IR/DiagnosticPrinter.h), the ability to carry context was very important. There, however, because the objects are being passed via a callback to the user-installed handler, they can carry pointers/references to objects (Values, Functions, etc.) that will go away once the object that detected the error is destroyed. In the model you're proposing, all of the context must be contained within the error object itself (because, by the time the context is useful, an arbitrary amount of the call stack to the error-detection point has already been unwound). This greatly limits the amount of information that can be efficiently stored as context in the error object. Depending on the use cases, it might be better to pass the context to some kind of error-handler callback than to try to pack it all into a long-lived error object. Thoughts?

I think this is one of the trickiest problems when designing an error return for C++. In garbage-collected languages you can attach all sorts of useful things and the reference from the error value will keep them alive. In C++ any non-owning reference or pointer type could be pointing into space by the time you reach the error handler (from the library's point of view).

I think the best you can do here is document the pitfalls and provide guidelines for designing error types. Kevin and I noted the following two relevant guidelines while discussing exactly this problem:

(1) Errors should not contain non-owning references or pointers. (Preferably, errors should only contain value types)

(2) New error types should only be introduced to model errors than clients could conceivably recover from, or where different clients may want to format the error messages differently. Any error that is only useful for diagnostic purposes should probably use a class along the lines of:

class DiagnosticError ... {

  std::string Msg;

  void log(ostream &os) { os << Msg; }

};

Given the parallels with exceptions, I suspect many of the same design guidelines would apply here.

I'm also not averse to mixing diagnostic streams with my system where they make sense - I think it's always a matter of choosing the right tool for the job. I just need a solution for the error recovery problem, and diagnostic streams don't provide one. :)

Cheers,

Lang.

[Lang Hames via llvm-dev]

Hi Mehdi,

> I’m not sure to understand this claim? You are supposed to be able to extend and subclass the type of diagnostics? (I remember doing it for an out-of-tree LLVM-based project).

You can subclass diagnostics, but subclassing (on its own) only lets you change the behaviour of the diagnostic/error itself. What we need, and what this patch supplies, is a way to choose a particular handler based on the type of the error. For that you need RTTI, so this patch introduces a new RTTI scheme that I think is more suitable for errors types*, since unlike LLVM's existing RTTI system it doesn't require you to enumerate the types up-front.

* If this RTTI system is considered generically useful it could be split out into its own utility. It's slightly higher cost than LLVM's system: One byte of BSS per type, and a walk from the dynamic type of the error to the root of the type-hierarchy (with possible early exit) for each type check.

> What does success or failure means for the handler?

It gives the handler an opportunity to inspect and then "re-throw" an error if necessary: A handler might not know whether it can recover based on type alone, or it may not want to recover at all, but instead attach some context to provide a richer diagnostic.

As a concrete example, one of our motivating cases is processing object files in archives. Down in the object file processing code, a load command might be found to be malformed, but at that point there's no context to tell us that the object that it's in is part of an archive, so the best diagnostic we could produce is "In foo.o: malformed load command at index N". A (straw-man) improved system might look like this:

class ObjectError ... { // <- Root of all object-file errors

  std::string ArchiveName = "";

  std::string ObjectName = "";

  std::error_code EC;

  void log(raw_ostream &OS) const override {

    if (!ArchiveName.empty())

      OS << "In archive '" << ArchiveName << "', ";

    OS << "In object file '" << ObjectName << "', " << EC.message();

  }

};

TypedError processArchive(Archive &A) {

  TypedError Err;

  for (auto &Obj : A) {

    auto Err = processObject(Obj);

    if (auto E2 =

      catchTypedErrors(std::move(Err),

        handleTypedError<ObjectError>([&](std::unique_ptr<ObjectError> OE) {

          OE->ArchiveName = A.getName();

          return TypedError(std::move(OE));

        }))

     return E2;

  }

}

In this example, any error (whether an ObjectError or something else) will be intercepted by the 'catchTypedErrors' function. If the error *isn't* an ObjectError it'll be returned unmodified out of catchTypedErrors, triggering an immediate return from processArchive. If it *is* an ObjectError then the handler will be run, giving us an opportunity to tag the error as having occurred within archive A.

Again - this is a straw-man example: I think we can do better again for diagnostics of this kind, but it showcases the value of being able to modify errors while they're in-flight.

> Is your call to catchAllTypedErrors(…) actually like a switch on the type of the error? What about a syntax that looks like a switch?

>

> switchErr(std::move(Err))

>    .case< MyCustomError>([] () { /* … */ })

>    .case< MyOtherCustomError>([] () { /* … */ })

>    .default([] () { /* … */ })

It's similar to a switch, but it's actually more like a list of regular C++ exception catch blocks (the name 'catchTypedError' is a nod to this).

The big difference is that you're not trying to find "the matching handler" in the set of options. Instead, the list of handlers is evaluated in order until one is found that fits, then that handler alone is executed. So if you had the following:

class MyBaseError : public TypedErrorInfo<MyBaseError> {};

class MyDerivedError : public TypedErrorInfo<MyDerivedError, MyBaseError> {}; // <- MyDerivedError inherits from MyBaseError.

and you wrote something like this:

catchTypedErrors(std::move(Err),

  handleTypedError<MyBaseError>([&](std::unique_ptr<MyBaseError> B) {

  }),

  handleTypedError<MyDerivedError>([&](std::unique_ptr<MyDerivedError> D) {

  })

);

The second handler will never run: All 'Derived' errors are 'Base' errors, the first handler fits, so it's the one that will be run.

We could go for something more like a switch, but then you have to define the notion of "best fit" for a type, which might be difficult (especially if I extend this to support multiple inheritance in error hierarchies. ;). I think it's easier to reason about "first handler that fits".

Cheers,

Lang.

[Mehdi Amini via llvm-dev]

On Feb 2, 2016, at 10:42 PM, Lang Hames <lha...@gmail.com> wrote:

Hi Mehdi,

> I’m not sure to understand this claim? You are supposed to be able to extend and subclass the type of diagnostics? (I remember doing it for an out-of-tree LLVM-based project).

You can subclass diagnostics, but subclassing (on its own) only lets you change the behaviour of the diagnostic/error itself. What we need, and what this patch supplies, is a way to choose a particular handler based on the type of the error.

If you subclass a diagnostic right now, isn’t the RTTI information available to the handler, which can then achieve the same dispatching / custom handling per type of diagnostic?

(I’m not advocating the diagnostic system, which I found less convenient to use than what you are proposing)

For that you need RTTI, so this patch introduces a new RTTI scheme that I think is more suitable for errors types*, since unlike LLVM's existing RTTI system it doesn't require you to enumerate the types up-front.

It looks like I’m missing a piece of something as it is not clear why is this strictly needed. I may have to actually look closer at the code itself.

Sure, this case shows “success” of the handler, now what is a failure of the handler and how is it handled?

> Is your call to catchAllTypedErrors(…) actually like a switch on the type of the error? What about a syntax that looks like a switch?

>

> switchErr(std::move(Err))

>    .case< MyCustomError>([] () { /* … */ })

>    .case< MyOtherCustomError>([] () { /* … */ })

>    .default([] () { /* … */ })

It's similar to a switch, but it's actually more like a list of regular C++ exception catch blocks (the name 'catchTypedError' is a nod to this).

The big difference is that you're not trying to find "the matching handler" in the set of options. Instead, the list of handlers is evaluated in order until one is found that fits, then that handler alone is executed. So if you had the following:

class MyBaseError : public TypedErrorInfo<MyBaseError> {};

class MyDerivedError : public TypedErrorInfo<MyDerivedError, MyBaseError> {}; // <- MyDerivedError inherits from MyBaseError.

and you wrote something like this:

catchTypedErrors(std::move(Err),

  handleTypedError<MyBaseError>([&](std::unique_ptr<MyBaseError> B) {

  }),

  handleTypedError<MyDerivedError>([&](std::unique_ptr<MyDerivedError> D) {

  })

);

The second handler will never run: All 'Derived' errors are 'Base' errors, the first handler fits, so it's the one that will be run.

We could go for something more like a switch, but then you have to define the notion of "best fit" for a type, which might be difficult (especially if I extend this to support multiple inheritance in error hierarchies. ;). I think it's easier to reason about "first handler that fits”.

Oh I was seeing it as a “first match as well”, just bike shedding the syntax as the function calls with a long flat list of lambdas as argument didn’t seem like the best we can do at the first sight.

— 

Mehdi

[Lang Hames via llvm-dev]

Hi Mehdi,

> If you subclass a diagnostic right now, isn’t the RTTI information available to the handler, which can then achieve the same dispatching / custom handling per type of diagnostic?

> (I’m not advocating the diagnostic system, which I found less convenient to use than what you are proposing)

I have to confess I haven't looked at the diagnostic classes closely. I'll take a look and get back to you on this one. :)

> > For that you need RTTI, so this patch introduces a new RTTI scheme that I think is more suitable for errors types*, since unlike LLVM's existing RTTI system it doesn't require you to enumerate the types up-front.

> It looks like I’m missing a piece of something as it is not clear why is this strictly needed. I may have to actually look closer at the code itself.

LLVM's RTTI requires you to define an enum value for each type in your hierarchy (see http://llvm.org/docs/HowToSetUpLLVMStyleRTTI.html), which means you need to know about all your potential subclasses up-front. That's not an option for a generic error class that might be subclassed in arbitrary ways.

The new RTTI system uses something closer to LLVM's Pass IDs: TypedErrorInfo (a utility which all errors must inherit from) introduces a new static char for each error type and uses its address as an ID. When you ask an error value "are you a subclass of type T" (via the isSameOrSubClass method) the call goes to the parent TypedErrorInfo object, which compares T's ID with its own. If it matches it returns true, if it doesn't match then the call gets forwarded to the parent class, then its parent class, and so on. If you hit the root of the type-tree (i.e. TypedErrorInfoBase) without matching the ID, then you weren't a subclass of T.

> Sure, this case shows “success” of the handler, now what is a failure of the handler and how is it handled?

Sorry - that was a bad example to choose: That was actually showcasing failure, not success. Success looks like this:

TypedError bar() {
  TypedError Err = foo;

  if (auto E2 =
    catchTypedErrors(std::move(Err),

      handleTypedError<MyError>([&](std::unique_ptr<MyError> M) {

        // Deal with 'M' somehow.
        return TypedError();
      }))
    return E2;

  // Proceed with 'bar' if 'Err' is handled.
}

A key observation is that catchTypedErrors itself returns an error. It has to, because you may not have provided it with an exhaustive list of handlers, and it needs a way to return unhanded errors. So: If no handler gets invoked, catchTypedErrors will just return 'Err' again. If 'Err' was an error, then E2 will also be an error, and you'll immediately return from 'bar', passing responsibility for the error up the stack. So far so good. Now consider what we should do if, instead, we *did* invoke a handler. One option would be to say that if a handler gets invoked then catchTypedErrors always returns 'TypedError()', indicating success, but that's an assertion that any error that's caught is definitely resolved. Sadly, we can't rely on that, so instead we allow the handler to supply the return value for catchTypedErrors. If the handler supplies 'TypedError()' then the error is considered truly 'handled' - E2 becomes TypedError(), the if condition is false (indicating there is no error) and the function goes on its way. If the handler supplies an error, then E2 becomes an error, the if condition is true, and we exit the function immediately, returning the error to the caller.

Hope that clears things up a bit. It takes a bit of staring at the first time, but I find it helpful to think of it as being analogous to a 're-throw' in an exception handler: Returning success (i.e. TypedError()) means continue the function, anything else means re-throw the error.

Cheers,

Lang.

[Lang Hames via llvm-dev]

> The new RTTI system uses something closer to LLVM's Pass IDs...

I should stress that all of this is totally opaque to clients: all you have to do to define your own error type is extend the TypedErrorInfo template, as below for example, and it will take care of the RTTI for you:

// A minimal new error class:

class MyError : public TypedErrorInfo<MyError> {

};

// Subclassing 'MyError':

class MySubError : public TypedErrorInfo<MySubError, MyError> {

};

- Lang.

[Craig, Ben via llvm-dev]

I've had some experience dealing with rich error descriptions without exceptions before.  The scheme I used was somewhat similar to what you have.  Here are some items to consider.

* How will the following code be avoided?  The answer may be compile time error, runtime error, style recommendations, or maybe something else.

TypedError Err = foo();
// no checking in between
Err = foo();

* How about this?

TypedError Err = foo();
functionWithHorribleSideEffects();
if(Err) return;

* Do you anticipate giving these kinds of errors to out of tree projects?  If so, are there any kind of binary compatibility guarantee?

* What about errors that should come out of constructors?  Or <shudder> destructors?

* If a constructor fails and doesn't establish it's invariant, what will prevent the use of that invalid object?

* How many subclasses do you expect to make of TypedError? Less than 10? More than 100?

* How common is it to want to handle a specific error code in a non-local way?  In my experience, I either want a specific error handled locally, or a fail / not-failed from farther away.  The answer to this question may influence the number of subclasses you want to make.

* Are file, line number, and / or call stack information captured?  I've found file and line number information to be incredibly useful from a productivity standpoint.

_______________________________________________
LLVM Developers mailing list
llvm...@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

[James Y Knight via llvm-dev]

On Tue, Feb 2, 2016 at 9:23 PM, Lang Hames <lha...@gmail.com> wrote:

I see the attribute as complimentary. The runtime check provides a stronger guarantee: the error cannot be dropped on any path, rather than just "the result is used". The attribute can help you catch obvious violations of this at compile time.

 

I agree the runtime check can catch something additional, it just doesn't feel to me like the extra complexity has been justified.

Or, at least, I'd have imagined a much simpler and straightforward interface would be fully sufficient. E.g., instead of the all the catch/handle stuff, if you want to handle a particular class specially, how about just using an if?

TypedError err = somethingThatCanFail();

if (err) {

  if (err.isClassOf(...)) {

    whatever;

  else

    return err;
}

[Mehdi Amini via llvm-dev]

On Feb 2, 2016, at 11:33 PM, Lang Hames <lha...@gmail.com> wrote:

Hi Mehdi,

> If you subclass a diagnostic right now, isn’t the RTTI information available to the handler, which can then achieve the same dispatching / custom handling per type of diagnostic?

> (I’m not advocating the diagnostic system, which I found less convenient to use than what you are proposing)

I have to confess I haven't looked at the diagnostic classes closely. I'll take a look and get back to you on this one. :)

> > For that you need RTTI, so this patch introduces a new RTTI scheme that I think is more suitable for errors types*, since unlike LLVM's existing RTTI system it doesn't require you to enumerate the types up-front.

> It looks like I’m missing a piece of something as it is not clear why is this strictly needed. I may have to actually look closer at the code itself.

LLVM's RTTI requires you to define an enum value for each type in your hierarchy (see http://llvm.org/docs/HowToSetUpLLVMStyleRTTI.html), which means you need to know about all your potential subclasses up-front. That's not an option for a generic error class that might be subclassed in arbitrary ways.

For a generic error class it is not an option indeed, but I was looking at it in the context of LLVM internal use, so just like our RTTI is not an option for “generic RTTI” but fits our need, we could (not should) do the same with ErrorHandling.

The new RTTI system uses something closer to LLVM's Pass IDs: TypedErrorInfo (a utility which all errors must inherit from) introduces a new static char for each error type and uses its address as an ID. When you ask an error value "are you a subclass of type T" (via the isSameOrSubClass method) the call goes to the parent TypedErrorInfo object, which compares T's ID with its own. If it matches it returns true, if it doesn't match then the call gets forwarded to the parent class, then its parent class, and so on. If you hit the root of the type-tree (i.e. TypedErrorInfoBase) without matching the ID, then you weren't a subclass of T.

Nice, and since this is on the error path we don’t care if it is not “as fast as” the custom LLVM RTTI.

> Sure, this case shows “success” of the handler, now what is a failure of the handler and how is it handled?

Sorry - that was a bad example to choose: That was actually showcasing failure, not success. Success looks like this:

TypedError bar() {
  TypedError Err = foo;
  if (auto E2 =
    catchTypedErrors(std::move(Err),
      handleTypedError<MyError>([&](std::unique_ptr<MyError> M) {

        // Deal with 'M' somehow.
        return TypedError();
      }))
    return E2;

  // Proceed with 'bar' if 'Err' is handled.
}

A key observation is that catchTypedErrors itself returns an error. It has to, because you may not have provided it with an exhaustive list of handlers, and it needs a way to return unhanded errors. So: If no handler gets invoked, catchTypedErrors will just return 'Err' again. If 'Err' was an error, then E2 will also be an error, and you'll immediately return from 'bar', passing responsibility for the error up the stack. So far so good. Now consider what we should do if, instead, we *did* invoke a handler. One option would be to say that if a handler gets invoked then catchTypedErrors always returns 'TypedError()', indicating success, but that's an assertion that any error that's caught is definitely resolved. Sadly, we can't rely on that, so instead we allow the handler to supply the return value for catchTypedErrors. If the handler supplies 'TypedError()' then the error is considered truly 'handled' - E2 becomes TypedError(), the if condition is false (indicating there is no error) and the function goes on its way. If the handler supplies an error, then E2 becomes an error, the if condition is true, and we exit the function immediately, returning the error to the caller.

Hope that clears things up a bit. It takes a bit of staring at the first time, but I find it helpful to think of it as being analogous to a 're-throw' in an exception handler: Returning success (i.e. TypedError()) means continue the function, anything else means re-throw the error.

OK got it now, the “empty” TypedError()is the key :)

(and I was using success/failure terminology reversed compare to you)

— 

Mehdi

[Vedant Kumar via llvm-dev]

Hi Lang,

> I expect to encounter more issues like this as I continue work on the MachO side of LLD. I see tackling the error modeling problem as a first step towards improving error handling in general: if we make it easy to model errors, it may pave the way for better error handling in many parts of our libraries.

+ 1, I'd like to use this throughout lib/ProfileData. It's a bit frustrating to see crashers which simply state: "Malformed profile data". It'd great to know _where_ the issue actually is.

vedant

[Lang Hames via llvm-dev]

Hi Craig,

> TypedError Err = foo();

> // no checking in between
> Err = foo();

This will cause an abort - the assignment operator for TypedError checks that you're not overwriting an unhanded error.

> TypedError Err = foo();
> functionWithHorribleSideEffects();
> if (Err) return;

This is potentially reasonable code - it's impossible to distinguish in general from:

TypedError Err = foo();

functionWithPerfectlyReasonableSideEffects();

if (Err) return;

That said, to avoid problems related to this style we can offer style guidelines. Idiomatic usage of the system looks like:

if (auto Err = foo())

  return Err;

functionWithHorribleSideEffects();

This is how people tend to write error checks in most of the LLVM code I've seen to date.

> Do you anticipate giving these kinds of errors to out of tree projects?  If so, are there any kind of binary compatibility guarantee?

Out of tree projects can use the TypedError.h header and derive their own error classes. This is all pure C++, I don't think there are binary compatibility issues.

> What about errors that should come out of constructors?  Or <shudder> destructors?

TypedError can't be "thrown" in the same way that C++ exceptions can. It's an ordinary C++ value. You can't return an error from a constructor, but you can pass a reference to an error in and set that. In general the style guideline for a "may-fail" constructors would be to write something like this:

class Foo {

public:

  static TypedErrorOr<Foo> create(int X, int Y) {

    TypedError Err;

    Foo F(X, Y, Err);

    if (Err)

      return std::move(Err);

    return std::move(F);

  }

private:

  Foo(int x, int y, TypedError &Err) {

    if (x == y) {

      Err = make_typed_error<BadFoo>();

      return;

    }

  }

};

Then you have:

TypedErrorOr<Foo> F = Foo::create(X, Y);

The only way to catch failure of a destructor is for the class to hold a reference to a TypedError, and set that. This is extremely difficult to do correctly, but as far is I know all error schemes suffer from poor interaction with destructors. In LLVM failing destructors are very rare, so I don't anticipate this being a problem in general.

>  If a constructor fails and doesn't establish it's invariant, what will prevent the use of that invalid object?

If the style guideline above is followed the invalid object will never be returned to the user. Care must be taken to ensure that the destructor can destruct the partially constructed object, but that's always the case.

>  How many subclasses do you expect to make of TypedError? Less than 10? More than 100?

This is a support library, so it's not possible to reason about how many external clients will want to use it in their projects, or how many errors they would define. In LLVM I'd like to see us adopt a 'less-is-more' approach: New error types should be introduced sparingly, and each new error type should require a rationale for its existence. In particular, distinct error types should only be introduced when it's reasonable for some client to make a meaningful distinction between them. If an error is only being returned in order to produce a string diagnostic, a generic StringDiagnosticError should suffice.

Answering your question more directly: In the LLVM code I'm familiar with I can see room for more than 10 error types, but fewer than 100.

> How common is it to want to handle a specific error code in a non-local way?  In my experience, I either want a specific error handled locally, or a fail / not-failed from farther away.  The answer to this question may influence the number of subclasses you want to make.

Errors usually get handled locally, or just produce a diagnostic and failure, however there are some cases where we want non-local recovery from specific errors. The archive-walking example I gave earlier is one such case. You're right on the point about subclasses too - that's what I was hoping to capture with my comment above: only introduce an error type if it's meaningful for a client to distinguish it from other errors.

 > Are file, line number, and / or call stack information captured?  I've found file and line number information to be incredibly useful from a productivity standpoint.

I think that information is helpful for programmatic errors, but those are better represented by asserts or "report_fatal_error". This system is intended to support modelling of non-programmatic errors - bad input, resource failures and the like. For those, the specific point in the code where the error was triggered is less useful. If such information is needed, this system makes it easy to break on the failure point in a debugger.

Cheers,

Lang.

[Lang Hames via llvm-dev]

Hi James,

The complexity involved in runtime checking is minimal. In terms of source complexity the checking code runs only ~20 lines total (it's orthogonal to the RTTI system and utilities, which take up the bulk of the code). The runtime overhead would be minimal in debug builds, and non-existent in release if we turn off checking there.

Runtime checking is significantly more powerful too. Take an anti-pattern that I've seen a few times:

for (auto &Elem : Collection) {

  if (std::error_code Err = foo(Elem))

    if (Err == recoverable_error_code) {

      // Skip items with 'recoverable' failures. 

      continue;

    }

  // Do stuff with elem.

}

This is the kind of code I want to stop: The kind where we pay just enough lip service to the error to feel like we've "handled" it, so we can get on with what we really want to do. This code will fail at runtime with unpredictable results if Err is anything other than 'success' or 'recoverable_error_code', but it does inspect the return type, so an attribute won't generate any warning.

The advantage of catchTypedErrors over an if statement is that it lets you defer errors, which we wanted to be able to do in our archive walking code:

TypedError processArchive(Archive &A) {

  TypedError Errs;

  for (auto &Obj : A) {

    if (auto Err = processObject(Obj))

      if (Err.isA<ObjectError>()) {

        // processObject failed because our object was bad. We want to report

        // this to the user, but we also want to walk the rest of the archive

        // to collect further diagnostics, or take other meaningful actions.

        // For now, just append 'Err' to the list of deferred errors.

        Errs = join_error(std::move(Errs), std::move(Err));

        continue;

      } else

        return join_error(std::move(Err), std::move(Errs));

    // Do more work.

  }

  return Errs;

}

and now in main, you can have:

catchTypedErrors(processArchive(A),

  handleTypedError<ObjectError>([&](std::unique_ptr<ObjectError> OE) {

    ...

  })

);

And this one handler will be able to deal with all your deferred object errors.

For clients who know up-front that they'll never have to deal with compound errors, the if-statement would be fine, but I think it's better not to assume that.

I want to stress that I appreciate the distaste boilerplate, but as I mentioned in the proposal actually catching errors is the uncommon case, so it's probably ok if it's a little bit ugly.

Cheers,

Lang.

[Lang Hames via llvm-dev]

Hi Mehdi,

> For a generic error class it is not an option indeed, but I was looking at it in the context of LLVM internal use, so just like our RTTI is not an option for “generic RTTI” but fits our need, we could (not should) do the same with ErrorHandling.

Definitely. If this was LLVM only there'd be a strong case for using the existing RTTI system. The reason for the new RTTI system is that it would let us re-use this error class in other LLVM projects (LLD, LLDB, etc) without having to enumerate their errors in LLVM. 

> Nice, and since this is on the error path we don’t care if it is not “as fast as” the custom LLVM RTTI.

Yep.

> OK got it now, the “empty” TypedError()is the key :)

> (and I was using success/failure terminology reversed compare to you)

Yeah - this is confusing. It's no worse than 'std::error_code()', but it's no better either. Maybe introducing a utility wrapper like 'TypedErrorSuccess()' or even 'ESuccess()' would make things more readable.

- Lang.

[Mehdi Amini via llvm-dev]

On Feb 3, 2016, at 10:48 AM, Lang Hames <lha...@gmail.com> wrote:

Hi Mehdi,

> For a generic error class it is not an option indeed, but I was looking at it in the context of LLVM internal use, so just like our RTTI is not an option for “generic RTTI” but fits our need, we could (not should) do the same with ErrorHandling.

Definitely. If this was LLVM only there'd be a strong case for using the existing RTTI system. The reason for the new RTTI system is that it would let us re-use this error class in other LLVM projects (LLD, LLDB, etc) without having to enumerate their errors in LLVM. 

I think it is great :)

Side question on the related work because I’m curious: did you look for similar generic scheme for error handling offered by other C++ libraries? Maybe the common scheme is to use C++ exceptions but since many folks disable them (hello Google ;-) ) there has been probably many other attempts to address this.

> Nice, and since this is on the error path we don’t care if it is not “as fast as” the custom LLVM RTTI.

Yep.

On this topic of not paying the price on the non-error code path, it would be nice to not pay for constructing all the lambda captures when there is no error. I can imagine many way of expressing a “try/catch” like syntax to achieve this using macros, but not really without that. 

Have you thought about this?

Thanks,

Mehdi

[Craig, Ben via llvm-dev]

This is mostly in line with what I thought the answers would be, so +1 from me, at least for the concept.  I haven't peered into the implementation.

[Lang Hames via llvm-dev]

Hi Mehdi,

> Side question on the related work because I’m curious: did you look for similar generic scheme for error handling offered by other C++ libraries? Maybe the common scheme is to use C++ exceptions but since many folks disable them (hello Google ;-) ) there has been probably many other attempts to address this.

I did look around, but not very hard. Among the people I asked, everyone was either using exceptions, std::error_code, or had turned on the diagnostic that James Knight suggested. I did some preliminary web-searching, but that didn't turn up anything interesting. If anybody has seen other error handling schemes of note I'd love to hear about them.

> On this topic of not paying the price on the non-error code path, it would be nice to not pay for constructing all the lambda captures when there is no error.

If your catchTypedErrors call is under an if-test then the lambdas are in a scope that won't be entered on the non-error path:

if (auto Err = foo())

  if (auto E2 = catchTypedErrors(std::move(Errs), <lambdas here>))

    return;

I think (though I haven't tested this) that most lambdas should inline away to next to nothing, so I don't expect the overhead to be noticeable in either case.

- Lang.

[Mehdi Amini via llvm-dev]

On Feb 3, 2016, at 11:32 AM, Lang Hames <lha...@gmail.com> wrote:

Hi Mehdi,

> Side question on the related work because I’m curious: did you look for similar generic scheme for error handling offered by other C++ libraries? Maybe the common scheme is to use C++ exceptions but since many folks disable them (hello Google ;-) ) there has been probably many other attempts to address this.

I did look around, but not very hard. Among the people I asked, everyone was either using exceptions, std::error_code, or had turned on the diagnostic that James Knight suggested. I did some preliminary web-searching, but that didn't turn up anything interesting. If anybody has seen other error handling schemes of note I'd love to hear about them.

> On this topic of not paying the price on the non-error code path, it would be nice to not pay for constructing all the lambda captures when there is no error.

If your catchTypedErrors call is under an if-test then the lambdas are in a scope that won't be entered on the non-error path:

if (auto Err = foo())

  if (auto E2 = catchTypedErrors(std::move(Errs), <lambdas here>))

    return;

Sure, but I was looking at avoiding to have to do this extra level of manual test, to reduce the “boilerplate” effect. For instance I’d like to write something like (straw man):

TRY(foo())

CATCH(lambda1 here)

CATCH(lambda2 here)

CATCH(lambda3 here)

or (straw man as well):

auto Err = foo();

HANDLE(Err) {

  MATCH(MyCustomType) {

    // code

  } 

  MATCH(MyOtherCustomType) {

    // code

  }

  DEFAULT {

  

  }

}

I think (though I haven't tested this) that most lambdas should inline away to next to nothing, so I don't expect the overhead to be noticeable in either case.

My guess is that is will be very dependent on what the lambdas is actually capturing and how many there are, and if catchTypedErrors is actually inlined. 

But I’m sure we would sort this out anyway *if* it turns out they’re not ;)

— 

Mehdi

[Lang Hames via llvm-dev]

Hi Mehdi,

> > I think (though I haven't tested this) that most lambdas should inline away to next to nothing, so I don't expect the overhead to be noticeable in either case.

> My guess is that is will be very dependent on what the lambdas is actually capturing and how many there are, and if catchTypedErrors is actually inlined. 

> But I’m sure we would sort this out anyway *if* it turns out they’re not ;)

Yep. Some experimentation is warranted here.

- Lang.

[Philip Reames via llvm-dev]

On 02/02/2016 05:29 PM, Lang Hames via llvm-dev wrote:

Hi All,

I've been thinking lately about how to improve LLVM's error model and error reporting. A lack of good error reporting in Orc and MCJIT has forced me to spend a lot of time investigating hard-to-debug errors that could easily have been identified if we provided richer error information to the client, rather than just aborting. Kevin Enderby has made similar observations about the state of libObject and the difficulty of producing good error messages for damaged object files. I expect to encounter more issues like this as I continue work on the MachO side of LLD. I see tackling the error modeling problem as a first step towards improving error handling in general: if we make it easy to model errors, it may pave the way for better error handling in many parts of our libraries.

Separate from the discussion on how we represent errors, we need to decide what errors we want to detect, which we try to diagnose, and which are recoverable.  Each of these is a distinct design decision.

For instance, I'd be really curious to hear more about your use case in Orc and MCJIT.  We're using MCJIT and I've found the error model presented to be perfectly adequate for our purposes.   I've sometimes wished that MCJIT did a slightly better job *diagnosing* failures, but I have yet to see a case where I'd actually want the compiler to recover from incorrect input rather than failing with a clean message so that I can fix the bug in the calling code. 

One thing I'm seriously concerned about is setting false expectations.  Unless we are actually going to invest in supporting recovery from (say) malformed input, we should not present an interface which seems to allow that possibility.  A hard failure is *much* preferable to a soft error with the library in a potentially corrupt state.  I have seen several software projects head down the road of soft errors with partial recovery and it's *always* a disaster.  (Your proposal around checked-by-default errors is a good step in this direction, but is not by itself enough.)

Philip

p.s. I'm purposely not commenting on the representation question because I haven't read the proposal in enough detail to have a strong opinion. 

[Lang Hames via llvm-dev]

Hi Philip,

> Separate from the discussion on how we represent errors, we need to decide what errors we want to detect, which we try to diagnose, and which are recoverable.  Each of these is a distinct design decision.

Absolutely. This proposal should be interpreted with that in mind - it gives us a better toolkit for describing certain kinds of errors, but it should not be shoehorned into situations where it does not fit. Programmatic errors are an obvious example: they should remain hard errors (asserts/report_fatal_error/llvm_unreachable).

On MCJIT/Orc - I'm glad the current situation hasn't caused you too much pain, but other clients have complained about it. I think the key observation is that neither MCJIT nor ORC assume that the bit code they're compiling is in-memory. That means that both should be able to report "resource not available" errors (e.g. when JITing bitcode off a network mount that has temporarily dropped out). This problem has become especially acute now that we have library support for remote-JITing: Any call to any lazily compiled function may fail due to resource drop-outs, and we should be able to report that to the user to give them a chance to try to recover and re-try compilation.

On the commitment front: The driving use case for me currently is the llvm object-file tools and LLD, but I know that proper error handling in the JIT is something I want to tackle in not-too-distant future, so having a toolkit to deal with it is a big step in the right direction.

- Lang.

[Lang Hames via llvm-dev]

Oops... 

> "I think the key observation is that neither MCJIT nor ORC assume that the bit code they're compiling is in-memory."

IR is obviously always in memory. This should be "neither MCJIT nor ORC can assume that the objects being linked are in-memory".

- Lang.

[Rafael Espíndola]

On 3 February 2016 at 02:33, Lang Hames via llvm-dev

<llvm...@lists.llvm.org> wrote:
> Hi Mehdi,
>
>> If you subclass a diagnostic right now, isn’t the RTTI information
>> available to the handler, which can then achieve the same dispatching /
>> custom handling per type of diagnostic?
>> (I’m not advocating the diagnostic system, which I found less convenient
>> to use than what you are proposing)
>
> I have to confess I haven't looked at the diagnostic classes closely. I'll
> take a look and get back to you on this one. :)

The main thing I like about the diagnostic system is that it lets us
differentiate two related but independent concepts:

* Giving the human using the program diagnostics about what went wrong.
* Propagating an error to the caller so that the upper library layer
can handle it or pass it up the stack.

For example, when given a broken bitcode file we are able to produce
the diagnostic "Unknown attribute kind (52)" which shows exactly what
we are complaining about. We are able to do that because the
diagnostic is produced close to the spot where the error is detected.

That is way more than what we need to pass to the caller. In fact, the
only cases we need to differentiate are "this is not a bitcode file at
all" and "the file is broken", which we do with the following enum +
std::error_code.

enum class BitcodeError { InvalidBitcodeSignature = 1, CorruptedBitcode };

So we use the context to produce a diagnostic, but the error doesn't
need to store it. Compare that with what we had before r225562.

Note that with error_code one can define arbitrary error categories.

Other advantages are:

* It is easy to use fatal error in simple programs by just using a
diganostic handler that exits.
* Very debugger friendly. It is easy to put a breakpoint at the diag handler.

Given that distinction, what I agree that is really missing is
infrastructure to make sure std::error_code is handled and for
filtering it.

So, could you achieve your objectives by:

* Moving the diagnostic handling code to Support so that all of llvm can use it.
* Defining TypedError as a wrapper over std::error_code with a
destructor that verifies the error was handled?

Thanks,
Rafael

[Sean Silva via llvm-dev]

Yeah, the ability to breakpoint in the diag handler is a huge win!

 

Given that distinction, what I agree that is really missing is
infrastructure to make sure std::error_code is handled and for
filtering it.

So, could you achieve your objectives by:

* Moving the diagnostic handling code to Support so that all of llvm can use it.

Do you mean everything from clang and LLVM? Or just the stuff in LLVM?

The clang stuff would be a lot of work, but we did something similar for libOption when it started becoming clear that multiple programs would benefit from it (in particular LLD).

-- Sean Silva

[Lang Hames via llvm-dev]

Hi Rafael,

> The main thing I like about the diagnostic system is that it lets us

> differentiate two related but independent concepts:
>
> * Giving the human using the program diagnostics about what went wrong.
> * Propagating an error to the caller so that the upper library layer
> can handle it or pass it up the stack.

I don't think these are really independent. Whether or not you need to emit a diagnostic depends on whether a caller can handle the corresponding error, which isn't something you know at the point where the error is raised. That's the idea behind the 'log' method on TypedErrorInfo: It lets you transform meaningful information about the problem into a log message *after* you know whether it can be handled or not. Recoverable errors can be logged (if the client wants to do so), but they don't have to be.

Using TypedError for diagnostics also means one fewer friction point when composing library code: Rather than having to agree on both the error return type and the diagnostic context type you only have to get agreement on the error return type.

> That is way more than what we need to pass to the caller. In fact, the
> only cases we need to differentiate are "this is not a bitcode file at
> all" and "the file is broken", which we do with the following enum +
> std::error_code.

By contrast, in my system this would be:

class InvalidBitcodeSignature : TypedErrorInfo<InvalidBitcodeSignature> {};

class CorruptedBitcode : TypedErrorInfo<CorruptedBitcode> {

public:

  CorruptedBitcode(std::string Msg) : Msg(Msg) {}

  void log(raw_ostream &OS) const override { OS << Msg; }

private:

  std::string Msg; 

};

Once you factor out the need to define an error category, I suspect my system actually requires less code, not that either of them require much.

> Note that with error_code one can define arbitrary error categories.

The problem with error categories is that they're limited to static groupings of kinds of errors, but the user might need dynamic information to recover.

Consider someone who wants to use libObject to write an object-file repair tool: A "bad_symbol" error code tells you what went wrong, but not where or why, so it's not much use in attempting a fix. On the other hand "bad_symbol { idx = 10 }" gives you enough information to pop up a dialog, ask the user what the symbol at index 10 should be, then re-write that symbol table entry.

> Other advantages are:
>
> * It is easy to use fatal error in simple programs by just using a
> diganostic handler that exits.
> * Very debugger friendly. It is easy to put a breakpoint at the diag handler.

This proposal provides the same advantages. I noted the second point in the original RFC. The first is easily implemented using an idiom I've seen in llvm-objdump and elsewhere:

void check(TypedError Err) {

  if (!Err)

    return;

  logAllUnhandledErrors(std::move(Err), errs(), "<tool name>");

  exit(1);

}

...

TypedErrorOr<Result> R = doSomething();

check(R.takeError());

... *R;

Side-note: you can actually go one better and replace this idiom with:

template <typename T>

T check(TypedErrorOr<T> ValOrErr) {

  if (!ValOrErr) {

    logAllUnhandledErrors(ValOrErr.takeError(), errs(), "<tool name>");

    exit(1);

  }

  return std::move(*ValOrErr);

}

...

Result R = check(doSomething());

...

Mind you, this new idiom works equally well for ErrorOr/std::error_code. 

> So, could you achieve your objectives by:

>
> * Moving the diagnostic handling code to Support so that all of llvm can use it.
> * Defining TypedError as a wrapper over std::error_code with a
> destructor that verifies the error was handled?

Using TypedError as a wrapper for std::error_code would prevent errors from be silently dropped, which is a huge step forward. (Though TypedError would be a misnomer - we'd need a new name. :)

Using a diagnostic handler would allow richer diagnostics from libObject, but it supports a narrower class of error-recovery than my proposal. I'm inclined to favor my proposal as a strictly more general solution.

Cheers,

Lang.

 

[Rafael Espíndola]

>> * Moving the diagnostic handling code to Support so that all of llvm can
>> use it.
>
>
> Do you mean everything from clang and LLVM? Or just the stuff in LLVM?
> The clang stuff would be a lot of work, but we did something similar for
> libOption when it started becoming clear that multiple programs would
> benefit from it (in particular LLD).

Well, everything *could* use it, but things like clang would probably
stay with the more specialized tools they have.

Cheers,

[Lang Hames via llvm-dev]

Hi Sean,

> Yeah, the ability to breakpoint in the diag handler is a huge win!

I think both schemes support that equally well. In my scheme, if you have an error MyError, you can just write:

(lldb) b MyError

to set a breakpoint on the constructor.

- Lang.

[Rafael Espíndola]

> I don't think these are really independent. Whether or not you need to emit
> a diagnostic depends on whether a caller can handle the corresponding error,
> which isn't something you know at the point where the error is raised.

But you do in the diag handler. For example, if you are trying to open
multiple files, some of which are bitcode, you know to ignore
InvalidBitcodeSignature.

> That's the idea behind the 'log' method on TypedErrorInfo: It lets you
> transform meaningful information about the problem into a log message
> *after* you know whether it can be handled or not. Recoverable errors can be
> logged (if the client wants to do so), but they don't have to be.

...

> By contrast, in my system this would be:
>
> class InvalidBitcodeSignature : TypedErrorInfo<InvalidBitcodeSignature> {};
> class CorruptedBitcode : TypedErrorInfo<CorruptedBitcode> {
> public:
> CorruptedBitcode(std::string Msg) : Msg(Msg) {}
> void log(raw_ostream &OS) const override { OS << Msg; }
> private:
> std::string Msg;
> };

This highlights why I think it is important to keep diagnostics and
errors separate. In the solution you have there is a need to allocate
a std::string, even if that is never used. If it was always a
std::string it would not be that bad, but the framework seems designed
to allow even more context to be stored, opening the way for fun cases
of error handling interfering with lifetime management.

> Consider someone who wants to use libObject to write an object-file repair
> tool: A "bad_symbol" error code tells you what went wrong, but not where or
> why, so it's not much use in attempting a fix. On the other hand "bad_symbol
> { idx = 10 }" gives you enough information to pop up a dialog, ask the user
> what the symbol at index 10 should be, then re-write that symbol table
> entry.

Using error results to find deep information you want to patch seems
like a bad idea. A tool that wants to patch symbol indexes should be
reading them directly.

>
>> Other advantages are:
>>
>> * It is easy to use fatal error in simple programs by just using a
>> diganostic handler that exits.
>> * Very debugger friendly. It is easy to put a breakpoint at the diag
>> handler.
>
> This proposal provides the same advantages. I noted the second point in the
> original RFC. The first is easily implemented using an idiom I've seen in
> llvm-objdump and elsewhere:
>
> void check(TypedError Err) {
> if (!Err)
> return;
>
> logAllUnhandledErrors(std::move(Err), errs(), "<tool name>");
> exit(1);
> }

That is not the same that you get with a diagnostic handler. What you
get is an exit after the error was propagated over the library layer,
instead of an exit at the point where the issue is found.

We use the above code now because lib/Object has no diagnostic handler.

> Side-note: you can actually go one better and replace this idiom with:
>
> template <typename T>
> T check(TypedErrorOr<T> ValOrErr) {
> if (!ValOrErr) {
> logAllUnhandledErrors(ValOrErr.takeError(), errs(), "<tool name>");
> exit(1);
> }
> return std::move(*ValOrErr);
> }

Yes, we do pretty much that in ELF/COFF lld.

> Using TypedError as a wrapper for std::error_code would prevent errors from
> be silently dropped, which is a huge step forward. (Though TypedError would
> be a misnomer - we'd need a new name. :)

CheckedError?

> Using a diagnostic handler would allow richer diagnostics from libObject,
> but it supports a narrower class of error-recovery than my proposal. I'm
> inclined to favor my proposal as a strictly more general solution.

I would prefer the diagnostic handler for exactly the same reason :-)
Generality has a cost, and I don't think we should invest on it until
we have a concrete case where that is needed. Given that I would
suggest going the more incremental way. Add a CheckedError that wraps
error_code and use to make sure the errors are checked. When a better
diagnostic is needed, pass in a diagnostic handler.

If we ever get to the point were we really want to *return* more than
an error_code, it will be a much smaller refactoring to
s/CheckedError/TypedError/.

Cheers,

[Lang Hames via llvm-dev]

> > I don't think these are really independent. Whether or not you need to emit
> > a diagnostic depends on whether a caller can handle the corresponding error,
> > which isn't something you know at the point where the error is raised.

> But you do in the diag handler. For example, if you are trying to open
> multiple files, some of which are bitcode, you know to ignore
> InvalidBitcodeSignature.

That means anticipating the kinds of errors you do/don't want to recover from at the point at which you insatiate your diagnostic handler. At that point you may not have the information that you need to know whether you want to recover. E.g. What if one path through library code can recover, and another can't? Worse still, what if the divergence point between these two paths is in library code itself? Now you need to thread two diagnostic handlers up to that point. This scales very poorly.

Simply put: diagnostic handlers aren't library friendly. It's tough enough to get all libraries to agree on an error type, without having them all agree on a diagnostic handlers too, and thread them everywhere.

> This highlights why I think it is important to keep diagnostics and

> errors separate. In the solution you have there is a need to allocate
> a std::string, even if that is never used.

Errors are only constructed on the error path. There is no construction on the success path.

> If it was always a
> std::string it would not be that bad, but the framework seems designed
> to allow even more context to be stored, opening the way for fun cases
> of error handling interfering with lifetime management.

This is a real problem, but I think the solution is to have a style guideline: You can't use reference types (in the general sense - references, pointers, etc.) in errors. Errors that would otherwise require references should have their error-message stringified at the point of creation.

> > Consider someone who wants to use libObject to write an object-file repair

> > tool: A "bad_symbol" error code tells you what went wrong, but not where or
> > why, so it's not much use in attempting a fix. On the other hand "bad_symbol
> > { idx = 10 }" gives you enough information to pop up a dialog, ask the user
> > what the symbol at index 10 should be, then re-write that symbol table
> > entry.

> Using error results to find deep information you want to patch seems
> like a bad idea. A tool that wants to patch symbol indexes should be
> reading them directly.

I'm not sure I agree, but I don't have strong feelings on this particular example - it was intended as a straw man. My point is that error recover is useful in general: there is a reason things like exceptions exist. Diagnostic handlers are very poorly suited to general error recovery.

> > void check(TypedError Err) {
> >   if (!Err)
> >     return;
> >
> >   logAllUnhandledErrors(std::move(Err), errs(), "<tool name>");
> >   exit(1);
> > }

> That is not the same that you get with a diagnostic handler. What you

> get is an exit after the error was propagated over the library layer,
> instead of an exit at the point where the issue is found.

As long as you log the error I'm not sure I see a meaningful difference? If you're not debugging you shouldn't be crashing in your library. If you are debugging you want to set a breakpoint on the error constructor instead.

> I would prefer the diagnostic handler for exactly the same reason :-)

> Generality has a cost...

Could you explain what you see as the cost in this instance?

The scheme has a higher cost that std::error_code alone when you're returning an error, but I don't think optimizing the error case is a sensible thing to do. It has a potentially *lower* cost on the success path, as you don't need to take up an argument for the diagnostic handler.

The most serious objection that you've raised, to my mind, is the potential lifetime issues if people mistakenly use references in their error types. I'm planning to write additions to llvm's coding guidelines to cover that, which I think should be sufficient.

On the flip side, compared to a diagnostic handler, I think the scheme I've proposed has the following benefits:

- No need to thread a diagnostic handler type through the world (and consequently no need to agree on its type). Retrofitting rich error logging to an interface that already returns errors requires less work than adding a diagnostic handler.

- The ability to describe error hierarchies (e.g. the class of all errors that represent malformed objects, which may be further subclassed to describe specific errors).

- The ability for client code to meaningfully distinguish between error instances, rather than just error types.

> , and I don't think we should invest on it until

> we have a concrete case where that is needed.

The error infrastructure investment is largely done, and I'm volunteering to maintain it. Myself and Kevin Enderby are signing up to weave this through libObject and the JIT. Other libraries could be converted as people see fit, with ECError providing an interface between the std::error_code world and TypedError.

> Given that I would
> suggest going the more incremental way. Add a CheckedError that wraps
> error_code and use to make sure the errors are checked. When a better
> diagnostic is needed, pass in a diagnostic handler.

Our first use-case for this system is libObject, where we want to use this to enable richer error messages. An incremental approach would involve threading a diagnostic handler through everything (and returning CheckedError), then un-threading it again would involve a lot of churn. I'd prefer to move directly to the scheme I'm proposing. 

Cheers,

Lang.

[David Blaikie via llvm-dev]

On Tue, Feb 9, 2016 at 4:23 PM, Lang Hames via llvm-dev <llvm...@lists.llvm.org> wrote:

> > I don't think these are really independent. Whether or not you need to emit
> > a diagnostic depends on whether a caller can handle the corresponding error,
> > which isn't something you know at the point where the error is raised.

> But you do in the diag handler. For example, if you are trying to open
> multiple files, some of which are bitcode, you know to ignore
> InvalidBitcodeSignature.

That means anticipating the kinds of errors you do/don't want to recover from at the point at which you insatiate your diagnostic handler. At that point you may not have the information that you need to know whether you want to recover. E.g. What if one path through library code can recover, and another can't?

+1 to this (I had the same thought - stateful handlers would be pretty tricky, doubly so with \/ this point)

 

Worse still, what if the divergence point between these two paths is in library code itself? Now you need to thread two diagnostic handlers up to that point. This scales very poorly.

Simply put: diagnostic handlers aren't library friendly. It's tough enough to get all libraries to agree on an error type, without having them all agree on a diagnostic handlers too, and thread them everywhere.

I think it depends a bit on the library & how generic it is - likely lower level libraries (like libObject) will want a granular error-based result, not a diagnostic engine. (the clients will be more widely varied (than, say, Clang) & have different diagnostic/behavioral needs, etc)

But I think it makes total sense for Clang to use a diagnostic handling approach for the most part (all the possible ways that C++ can be written incorrectly essentially amount to the same thing for a consumer of Clang - code was bad & I need some text (and fixits, notes, etc) to tell the user why), the IR verifier similarly - and possibly LLD, for example, depending on how broad the use cases become.

 

[Lang Hames via llvm-dev]

Hi David,

But I think it makes total sense for Clang to use a diagnostic handling approach for the most part (all the possible ways that C++ can be written incorrectly essentially amount to the same thing for a consumer of Clang - code was bad & I need some text (and fixits, notes, etc) to tell the user why), the IR verifier similarly - and possibly LLD, for example, depending on how broad the use cases become.

Absolutely. The way I phrased my objection is overly broad. Diagnostic handlers have valid use-cases, but they're not good at conveying structured information to clients, which is a useful thing to do in general. I'm definitely not proposing that we replace clang's diagnostic handler with my scheme.

- Lang.

Sent from my iPhone

[Pete Cooper via llvm-dev]

Hi Rafael, Lang

On Feb 9, 2016, at 3:27 PM, Rafael Espíndola via llvm-dev <llvm...@lists.llvm.org> wrote:

If we ever get to the point were we really want to *return* more than
an error_code, it will be a much smaller refactoring to
s/CheckedError/TypedError/.

So i think you’re right that we should use a CheckedError (ECError in Lang’s patch.  I don’t mind on the final name), but since a CheckedError isa TypedError, seems like we don’t have anything to lose with the generality of TypedError.

That is, if we provide TypedError as a base, then anyone can extend it as needed, and clients who only need ECError can use that.  This is more or less covered in one of Lang’s patches where he changes the return type to TypedError, but the error being returned is 'return make_typed_error<ECError>(EC);’.  If a client then decided to return something other than an ECError, you wouldn’t have to change that function return type, or any of its callers.

Cheers,

Pete

[Rafael Espíndola]

>> This highlights why I think it is important to keep diagnostics and
>> errors separate. In the solution you have there is a need to allocate
>> a std::string, even if that is never used.
>
> Errors are only constructed on the error path. There is no construction on
> the success path.

But they are always created, even if it as error the caller wants to
ignore. For example, you will always create a "file foo.o in bar.a is
not a bitcode" message (or copy sufficient information for that to be
created). With a dignostic handler no copying is needed, since the
call happens in the context where the error is found. It is easy to
see us in a position where a lot of context is copied because some
client somewhere might want it.

So I am worried we are coding for the hypothetical and adding
complexity. In particular, if we are going this way I think it is
critical that your patch *removes* the existing diagnostic handlers
(while making sure test/Bitcode/invalid.ll still passes) so that we
don't end up with two overlapping solutions. We were still not even
done converting away from bool+std::string :-(

[David Blaikie via llvm-dev]

On Wed, Feb 10, 2016 at 6:47 AM, Rafael Espíndola <llvm...@lists.llvm.org> wrote:

>> This highlights why I think it is important to keep diagnostics and
>> errors separate. In the solution you have there is a need to allocate
>> a std::string, even if that is never used.
>
> Errors are only constructed on the error path. There is no construction on
> the success path.

But they are always created, even if it as error the caller wants to
ignore. For example, you will always create a "file foo.o in bar.a is
not a bitcode" message (or copy sufficient information for that to be
created). With a dignostic handler no copying is needed, since the
call happens in the context where the error is found. It is easy to
see us in a position where a lot of context is copied because some
client somewhere might want it.

So I am worried we are coding for the hypothetical and adding
complexity. In particular, if we are going this way I think it is
critical that your patch *removes* the existing diagnostic handlers
(while making sure test/Bitcode/invalid.ll still passes) so that we
don't end up with two overlapping solutions.

Removes diagnostic handlers in other parts of LLVM (& Clang) - the IR verifier's diagnostic handling is what you're referring to here ^?

I don't think that would be an improvement - it seems like different situations call for different tools (as I was saying yesterday on this thread). In some places a diagnostic handler is the right tool, and in some places error codes/results/etc are the right tool. We already live in that world & it seems like a reasonable one (there doesn't seem to be a fundamental conflict between our bool+std::string or error_code returns and existing diagnostic handlers - I think they can reasonably coexist in different parts of the codebase for different use cases)

[Rafael Espíndola]

In which case we do need a plain checked error_code. Right now we use
a diagnostic handler in the BitcodeReader, but it is really easy to
miss propagating an error out. We shouldn't be in a position where we
have to decide to use an diagnostic handler or have checked errors. It
should be possible to have both if it is not desired that the new
error handling replaces the use of diagnostic handling.

[Paweł Bylica]

Sorry for the question out of nowhere, but what happened to ErrorOr<> scheme?

[Lang Hames via llvm-dev]

ErrorOr still exists, and generally useful. This proposal introduces a similar class, TypedErrorOr, that replaces the std::error_code with a TypedError.

Any more detailed discussion of ErrorOr usage should probably happen on another thread - this one is already getting long. :)

- Lang.

[David Blaikie via llvm-dev]

I don't think we are or would be in that position based on this proposal - it sounds to me like the TypedError has an implementation that wraps error_code & that one could be used in places where we want a diagnostic handler + checked errors (& as a side benefit we get hierarchical errors, which can be handy) without having to bundle all the necessary state for diagnostic emission through that path if it happens not to be the right tool for the job in some contexts.

[Lang Hames via llvm-dev]

Hi Rafael,

> But they are always created, even if it as error the caller wants to

> ignore. For example, you will always create a "file foo.o in bar.a is
> not a bitcode" message (or copy sufficient information for that to be
> created). With a dignostic handler no copying is needed, since the
> call happens in the context where the error is found. It is easy to
> see us in a position where a lot of context is copied because some
> client somewhere might want it.

I'm not saying that this system would replace diagnostic handlers in general, or that it should be shoehorned into places where it doesn't fit. This system is a replacement for std::error_code, because std::error_code is deficient as an error return. It happens that once you have a generic error return value you can use it for a lot of simple diagnostic cases, rather than threading a diagnostic handler everywhere. If you want to mix and match my error scheme with a diagnostic handler you still can.

As to the performance concern - you're trying to optimizing the error case. As noted, my scheme is slower than std::error_code on the error_path, but not prohibitively so. I've thought about this, and cannot imagine a reasonable use of this system that would lead to unacceptable overhead on the error path.

> So I am worried we are coding for the hypothetical and adding complexity. 

Don't worry - I'm too busy with real problems to tackle hypothetical ones. Here's another example of a problem that my proposal solves:

I recently added support for remote JITing to ORC. There are in-tree library facilities for JITing code into a process on the other end of an abstract "RPCChannel". What happens if something goes wrong at one end? We want to be able to communicate an error back across the RPCChannel (assuming it's still intact) so the other side can recover or fail gracefully. That means we need to be able to serialize an error with enough information to describe what went wrong. There's no practical way to maintain a serialization routine for all possible std::error_codes that might come up, even if they were powerful enough to describe everything that could go wrong (which again, being static kinds, they're not). With my proposal however, a JITError base class can be defined as:

class JITError : public TypedErrorInfo<JITError> {

public:

  virtual void serialize(RPCChannel &C) const = 0;

};

Now you just describe serialization/deserialization for each error when you define it. :)

(Yes - this hand waves over deserialization. It's solvable. The gory details will add nothing to this discussion).

> In particular, if we are going this way I think it is

> critical that your patch *removes* the existing diagnostic handlers
> (while making sure test/Bitcode/invalid.ll still passes) so that we
> don't end up with two overlapping solutions. We were still not even
> done converting away from bool+std::string :-(

I think I've covered this now, but once more for emphasis: I'm not going to start tearing all the diagnostic handlers out. I absolutely see the value of diagnostic handlers for producing intricate diagnostics. My proposal tackles the error return problem, and it turns out that once you have a customizable error return value you can use it to produce decent diagnostics, as long as the client doesn't need to be able to control the formatting of those diagnostics. This property will allow us to produce better diagnostics that we currently do in several tools, without the need to introduce diagnostic handlers there. We can still introduce them later if we want to.

Cheers,

Lang.

[Rafael Espíndola]

> I recently added support for remote JITing to ORC. There are in-tree library
> facilities for JITing code into a process on the other end of an abstract
> "RPCChannel". What happens if something goes wrong at one end? We want to be
> able to communicate an error back across the RPCChannel (assuming it's still
> intact) so the other side can recover or fail gracefully. That means we need
> to be able to serialize an error with enough information to describe what
> went wrong. There's no practical way to maintain a serialization routine for
> all possible std::error_codes that might come up, even if they were powerful
> enough to describe everything that could go wrong (which again, being static
> kinds, they're not). With my proposal however, a JITError base class can be
> defined as:
>
> class JITError : public TypedErrorInfo<JITError> {
> public:
> virtual void serialize(RPCChannel &C) const = 0;
> };
>

What prevents you from using a diag handler in the jit server that
sends errors/warnings over the RPCChannel?

[Lang Hames via llvm-dev]

Hi Rafael,

What prevents you from using a diag handler in the jit server that
sends errors/warnings over the RPCChannel?

What would you do with errors that can't reasonable be serialized when they reach the diagnostic handler?

And what would you do with the serialized bytes on the client end?

- Lang.

Sent from my iPhone

[Lang Hames via llvm-dev]

Hi Rafael,

What prevents you from using a diag handler in the jit server that
sends errors/warnings over the RPCChannel?

Sorry - this is a non-trivial problem to think through, so to speed things up, here are the issues:

(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.

(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.

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:

while (1) {

  if (auto Err = handleServerCommand(Channel))

    if (Err is handleable by the server)

      /* handle it */

    else if (Err.isA<Serializable>())

      Err->serialize(Channel); /* Report it to the client, let them respond */

    else

      return Err; /* Bail out of the server loop */

}

Cheers,

Lang.

[Lang Hames via llvm-dev]

Hi All,

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:

(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.

(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.

(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).

(4) Errors should be easy to break on in a debugger.

(5) Errors should have minimal / no overhead in the success case.

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.

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.

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.

Point (5) is a wash - both TypedError and std::error_code are cheap in the success case.

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:

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:

class MyError : TypedErrorInfo<MyError> { ... };

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.

So, given the benefits of this proposal, I think the complexity is well justified.

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:

(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.

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.

(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.

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.

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: 

(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.

(2) Re-submit the original prototype (with minor tweaks) plus the unit-tests and programmers manual additions to llvm-commits fro review.

(3) Begin threading TypedError and TypedErrorOr through MachOObjectFile in libObject, using ECError as a bridge between TypedError/TypedErrorOr and std::error_code/ErrorOr.

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.

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.

Cheers,

Lang.

[Rafael Espíndola]

> 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.

That means we will forever have an incomplete transition.

We still haven't finished moving to the "new" naming style. Moving to
the current path handling took from Nov 2009 to Jun 2013.

I am still not convinced that the new system is better than a
asserting wrapper over std::error_code with diag handlers, but I don't
have the time to change Orc to use to show it. Given that I also
failed to convince you otherwise, we will probably have to go with the
view of who can actually code this.

But *please*, make an effort to use it everywhere. We still have a mix
of bool+std::string, std::error_code and ErrorOr. Are each of these
better than what you are proposing in one case or the other? Probably.
Is it worth it having unique snow flakes? I don't think so.

In particular, please don't change just the "MachO-specific part of
X". In the past I have made an effort to keep MachO bits current when
working in MC and lib/Object. Please do the corresponding effort. For
example, if you change llvm-nm, please change all of it

[Mehdi Amini via llvm-dev]

> On Feb 10, 2016, at 7:19 PM, Rafael Espíndola <rafael.e...@gmail.com> wrote:
>
>> 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.
>
> That means we will forever have an incomplete transition.
>
> We still haven't finished moving to the "new" naming style. Moving to
> the current path handling took from Nov 2009 to Jun 2013.
>
> I am still not convinced that the new system is better than a
> asserting wrapper over std::error_code with diag handlers, but I don't
> have the time to change Orc to use to show it. Given that I also
> failed to convince you otherwise, we will probably have to go with the
> view of who can actually code this.
>
> But *please*, make an effort to use it everywhere. We still have a mix
> of bool+std::string, std::error_code and ErrorOr. Are each of these
> better than what you are proposing in one case or the other? Probably.
> Is it worth it having unique snow flakes? I don't think so.
>
> In particular, please don't change just the "MachO-specific part of
> X". In the past I have made an effort to keep MachO bits current when
> working in MC and lib/Object. Please do the corresponding effort. For
> example, if you change llvm-nm, please change all of it

FWIW, I support the effort Lang is leading, so it is a +1 on my side to move on!
And I also agree with Rafael that it would be really better to limit as much as possible the mix of multiple reporting inside a component.

Lang: please CC me on the reviews, I'll be happy to follow the development.

--
Mehdi

[Lang Hames via llvm-dev]

Hi Rafael,

> > 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.

> That means we will forever have an incomplete transition.

I included that caveat because it's not my place to insist on adoption, but it is my hope is that this system could eventually replace other error-return systems in LLVM.

> But *please*, make an effort to use it everywhere. We still have a mix

> of bool+std::string, std::error_code and ErrorOr. Are each of these
> better than what you are proposing in one case or the other? Probably.
> Is it worth it having unique snow flakes?  I don't think so.

I'm happy to take this approach, and I agree: having a consistent scheme for error returns everywhere is ideal. 

> In particular, please don't change just the "MachO-specific part of
> X". In the past I have made an effort to keep MachO bits current when
> working in MC and lib/Object. Please do the corresponding effort. For
> example, if you change llvm-nm, please change all of it

I'm also happy to do this.

It's neither practical nor prudent to try to re-write the world in one go, so there's no escaping a transition period with some glue code. That's no reason not to do this though, and assuming the community's experience with the system is as positive as mine was with the prototype, I think we could aim to move to this as an essentially universal error-return solution in the future.

Cheers,

Lang.

[Lang Hames via llvm-dev]

Hi Mehdi,

Thanks for the +1, and for offering to help out with reviews. I really appreciate that. :)

- Lang.

[Rafael Espíndola]

> It's neither practical nor prudent to try to re-write the world in one go,

No, but it is even less to assume it will re-write itself.

A long time ago "incomplete transitions" was listed as one of the main
issues in the gcc codebase
(ftp://ftp.cygwinports.org/pub/gcc/summit/2003/A%20maintenance%20programmer's%20view%20of%20gcc.pdf)

I don't thing our situation is as bad, but it is getting uncomfortable.

[Michael Spencer via llvm-dev]

On Tue, Feb 2, 2016 at 5:29 PM, Lang Hames via llvm-dev
<llvm...@lists.llvm.org> wrote:
> Hi All,
>
> I've been thinking lately about how to improve LLVM's error model and error
> reporting. A lack of good error reporting in Orc and MCJIT has forced me to
> spend a lot of time investigating hard-to-debug errors that could easily
> have been identified if we provided richer error information to the client,
> rather than just aborting. Kevin Enderby has made similar observations about
> the state of libObject and the difficulty of producing good error messages
> for damaged object files. I expect to encounter more issues like this as I
> continue work on the MachO side of LLD. I see tackling the error modeling
> problem as a first step towards improving error handling in general: if we
> make it easy to model errors, it may pave the way for better error handling
> in many parts of our libraries.
>
> At present in LLVM we model errors with std::error_code (and its helper,
> ErrorOr) and use diagnostic streams for error reporting. Neither of these
> seem entirely up to the job of providing a solid error-handling mechanism
> for library code. Diagnostic streams are great if all you want to do is
> report failure to the user and then terminate, but they can't be used to
> distinguish between different kinds of errors, and so are unsuited to many
> use-cases (especially error recovery). On the other hand, std::error_code
> allows error kinds to be distinguished, but suffers a number of drawbacks:
>
> 1. It carries no context: It tells you what went wrong, but not where or
> why, making it difficult to produce good diagnostics.
> 2. It's extremely easy to ignore or forget: instances can be silently
> dropped.
> 3. It's not especially debugger friendly: Most people call the error_code
> constructors directly for both success and failure values. Breakpoints have
> to be set carefully to avoid stopping when success values are constructed.
>
> In fairness to std::error_code, it has some nice properties too:
>
> 1. It's extremely lightweight.
> 2. It's explicit in the API (unlike exceptions).
> 3. It doesn't require C++ RTTI (a requirement for use in LLVM).
>
> To address these shortcomings I have prototyped a new error-handling scheme
> partially inspired by C++ exceptions. The aim was to preserve the
> performance and API visibility of std::error_code, while allowing users to
> define custom error classes and inheritance relationships between them. My
> hope is that library code could use this scheme to model errors in a
> meaningful way, allowing clients to inspect the error information and
> recover where possible, or provide a rich diagnostic when aborting.
>
> The scheme has three major "moving parts":
>
> 1. A new 'TypedError' class that can be used as a replacement for
> std::error_code. E.g.
>
> std::error_code foo();
>
> becomes
>
> TypedError foo();
>
> The TypedError class serves as a lightweight wrapper for the real error
> information (see (2)). It also contains a 'Checked' flag, initially set to
> false, that tracks whether the error has been handled or not. If a
> TypedError is ever destructed without being checked (or passed on to someone
> else) it will call std::terminate(). TypedError cannot be silently dropped.
>
> 2. A utility class, TypedErrorInfo, for building error class hierarchies
> rooted at 'TypedErrorInfoBase' with custom RTTI. E.g.
>
> // Define a new error type implicitly inheriting from TypedErrorInfoBase.
> class MyCustomError : public TypedErrorInfo<MyCustomError> {
> public:
> // Custom error info.
> };
>
> // Define a subclass of MyCustomError.
> class MyCustomSubError : public TypedErrorInfo<MyCustomSubError,
> MyCustomError> {
> public:
> // Extends MyCustomError, adds new members.
> };
>
> 3. A set of utility functions that use the custom RTTI system to inspect
> and handle typed errors. For example 'catchAllTypedErrors' and
> 'handleTypedError' cooperate to handle error instances in a type-safe way:
>
> TypedError foo() {
> if (SomeFailureCondition)
> return make_typed_error<MyCustomError>();
> }
>
> TypedError Err = foo();
>
> catchAllTypedErrors(std::move(Err),
> handleTypedError<MyCustomError>(
> [](std::unique_ptr<MyCustomError> E) {
> // Handle the error.
> return TypedError(); // <- Indicate success from handler.
> }
> )
> );
>
>
> If your initial reaction is "Too much boilerplate!" I understand, but take
> comfort: (1) In the overwhelmingly common case of simply returning errors,
> the usage is identical to std::error_code:
>
> if (TypedError Err = foo())
> return Err;
>
> and (2) the boilerplate for catching errors is usually easily contained in a
> handful of utility functions, and tends not to crowd the rest of your source
> code. My initial experiments with this scheme involved updating many source
> lines, but did not add much code at all beyond the new error classes that
> were introduced.
>
>
> I believe that this scheme addresses many of the shortcomings of
> std::error_code while maintaining the strengths:
>
> 1. Context - Custom error classes enable the user to attach as much
> contextual information as desired.
>
> 2. Difficult to drop - The 'checked' flag in TypedError ensures that it
> can't be dropped, it must be explicitly "handled", even if that only
> involves catching the error and doing nothing.
>
> 3. Debugger friendly - You can set a breakpoint on any custom error class's
> constructor to catch that error being created. Since the error class
> hierarchy is rooted you can break on TypedErrorInfoBase::TypedErrorInfoBase
> to catch any error being raised.
>
> 4. Lightweight - Because TypedError instances are just a pointer and a
> checked-bit, move-constructing it is very cheap. We may also want to
> consider ignoring the 'checked' bit in release mode, at which point
> TypedError should be as cheap as std::error_code.
>
> 5. Explicit - TypedError is represented explicitly in the APIs, the same as
> std::error_code.
>
> 6. Does not require C++ RTTI - The custom RTTI system does not rely on any
> standard C++ RTTI features.
>
> This scheme also has one attribute that I haven't seen in previous error
> handling systems (though my experience in this area is limited): Errors are
> not copyable, due to ownership semantics of TypedError. I think this
> actually neatly captures the idea that there is a chain of responsibility
> for dealing with any given error. Responsibility may be transferred (e.g. by
> returning it to a caller), but it cannot be duplicated as it doesn't
> generally make sense for multiple people to report or attempt to recover
> from the same error.
>
> I've tested this prototype out by threading it through the object-creation
> APIs of libObject and using custom error classes to report errors in MachO
> headers. My initial experience is that this has enabled much richer error
> messages than are possible with std::error_code.
>
> To enable interaction with APIs that still use std::error_code I have added
> a custom ECError class that wraps a std::error_code, and can be converted
> back to a std::error_code using the typedErrorToErrorCode function. For now,
> all custom error code classes should (and do, in the prototype) derive from
> this utility class. In my experiments, this has made it easy to thread
> TypedError selectively through parts of the API. Eventually my hope is that
> TypedError could replace std::error_code for user-facing APIs, at which
> point custom errors would no longer need to derive from ECError, and ECError
> could be relegated to a utility for interacting with other codebases that
> still use std::error_code.
>
> So - I look forward to hearing your thoughts. :)
>
> Cheers,
> Lang.
>
> Attached files:
>
> typed_error.patch - Adds include/llvm/Support/TypedError.h (also adds
> anchor() method to lib/Support/ErrorHandling.cpp).
>
> error_demo.tgz - Stand-alone program demo'ing basic use of the TypedError
> API.
>
> libobject_typed_error_demo.patch - Threads TypedError through the
> binary-file creation methods (createBinary, createObjectFile, etc).
> Proof-of-concept for how TypedError can be integrated into an existing
> system.

>
>
> _______________________________________________
> LLVM Developers mailing list
> llvm...@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>

I like this idea in general. It's a better implementation of what
ErrorOr originally was before we removed the custom error support
because it wasn't used. In fact I would actually support outright
replacing ErrorOr with this if it can be done safely, as I find the
name TypedErrorOr a bit long.

- Michael Spencer

[Rafael Espíndola]

> I like this idea in general. It's a better implementation of what
> ErrorOr originally was before we removed the custom error support
> because it wasn't used. In fact I would actually support outright
> replacing ErrorOr with this if it can be done safely, as I find the
> name TypedErrorOr a bit long.

The main differences are
* This will hopefully be used.
* TypedErrorOr is really a TypedError or T. The situation before was
not that ErrorOr was std::error_code or T.

Since we are adding it, I would also support replacing every use of
ErrorOr with TypedErrorOr and renaming it.

Cheers,
Rafael

[Paweł Bylica]

On Thu, Feb 18, 2016 at 4:36 PM Rafael Espíndola <llvm...@lists.llvm.org> wrote:

> I like this idea in general. It's a better implementation of what
> ErrorOr originally was before we removed the custom error support
> because it wasn't used. In fact I would actually support outright
> replacing ErrorOr with this if it can be done safely, as I find the
> name TypedErrorOr a bit long.


The main differences are
* This will hopefully be used.
* TypedErrorOr is really a TypedError or T. The situation before was
not that ErrorOr was std::error_code or T.

Since we are adding it, I would also support replacing every use of
ErrorOr with TypedErrorOr and renaming it.

Alternative name could be Expected<T> as proposed by Andrei Alexandrescu.

[Lang Hames via llvm-dev]

Hi Michael, Rafael, Pawel,

Apologies for the delayed reply - I was out on vacation last week.

>  In fact I would actually support outright

> replacing ErrorOr with this if it can be done safely, as I find the
> name TypedErrorOr a bit long.

I find the name awkward too. I'll leave these classes as TypedError and TypedErrorOr<T> when I submit the initial patch to llvm-commits, but please feel free to comment on the names in that review. If the proposal is adopted I think the eventual plan should be to move to 'Error' and 'ErrorOr<T>', or Error and Expected<T> (Pawel - I really like that suggestion - thanks!).

> The main differences are
> * This will hopefully be used.
> * TypedErrorOr is really a TypedError or T. The situation before was not that ErrorOr was std::error_code or T.

I don't follow that second point. As I see it, TypedErrorOr<T> and ErrorOr<T> are identical except for two points: TypedError replaces std::error_code, and as a consequence TypedErrorOr is only movable, not copyable.

Cheers,

Lang.

[Rafael Espíndola]

>> The main differences are
>> * This will hopefully be used.
>> * TypedErrorOr is really a TypedError or T. The situation before was not
>> that ErrorOr was std::error_code or T.
>
> I don't follow that second point. As I see it, TypedErrorOr<T> and
> ErrorOr<T> are identical except for two points: TypedError replaces
> std::error_code, and as a consequence TypedErrorOr is only movable, not
> copyable.

That is the case now. Sorry, Mchiael and I were talking about ancient
history when ErrorOr<T> was *not* an std::error_code or T.

[Lang Hames via llvm-dev]

Ahh - that makes sense. Thanks. :)

- Lang.

[Lang Hames via llvm-dev]

Hi All,

An updated version of this proposal has been committed as r263609. Thanks very much for all the discussion and feedback.

Once the dust has settled and any issues that survived pre-commit review are cleared up I'll start writing a more thorough usage document.

Cheers,

Lang.