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

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


On Tue, Feb 9, 2016 at 11:12 AM, Rafael Espíndola <
rafael.espindola at gmail.com> wrote:

> On 3 February 2016 at 02:33, Lang Hames via llvm-dev
> <llvm-dev at 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20160209/9c69fd2c/attachment.html>