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

[Rafael Espíndola 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'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,
Rafael