[llvm-commits] [PATCH] Improve error handling, supersede cerr+abort

[Chris Lattner]

On Jun 28, 2009, at 1:55 PM, Török Edwin wrote:
> As discussed here is a first implementation of an error handling API  
> for
> LLVM.
>
> The attached patch introduces the API, and as an example converts 2
> cerr+abort, and a assert(0)+abort to the new API.

Cool, this is something we've needed for a long time.

> The default is to call abort()/exit(1) as till now, but now a client  
> can
> install a custom error handler. If the custom handler returns
> abort()/exit(1) is still called.
> The intention is to also change LLVM to return failure codes instead  
> of
> aborting, but as a first incremental improvement I just introduced the
> API, and tried
> to preserve current behaviour.

Sounds good.

Here are a few specific thoughts on the patch:

+++ b/include/llvm/Support/ErrorHandling.h
@@ -0,0 +1,84 @@
+//===- llvm/Support/ErrorHandling.h - Callbacks for errors ------*- C+ 
+ -*-===//
..
+//
+// This file defines an API for error handling, it supersedes cerr 
+abort(), and
+// cerr+exit() style error handling.
+// Callbacks can be registered for these errors through this API.
+// 
= 
= 
=---------------------------------------------------------------------- 
===//

Please put a line before the last one.  Also, the file should explain  
what it does, it should not refer to historical designs.  Just say  
"this is used to indicate error conditions ... ".

+#ifndef LLVM_SUPPORT_ERRORHANDLING_H
+#define LLVM_SUPPORT_ERRORHANDLING_H
+#include "llvm/Support/raw_ostream.h"
+#include <cstdlib>

Please put a blank line before the #includes.  Also, I think the  
raw_ostream header at least can be replaced with a forward declaration  
of the needed classes.

+  enum ErrorSeverity {
+    WarningSeverity=0,
+    ErrorSeverity,
+    FatalSeverity
+  };

I think that this is over-designed.  The depths of the code generator  
should not be producing warnings, ever.  Things that *can* produce  
warnings (e.g. llvm-mc, tblgen, etc) should do so with their own  
reporting mechanisms, not through this API.  Likewise, I don't think  
there is a need for error vs fatal.  There should be no fatal errors  
in LLVM: in cases where there is an error, it should be detected,  
reported, and recovered from.

+  // Restores default error handling behaviour.
+  // This must not be called between llvm_start_multithreaded() and
+  // llvm_stop_multithreaded().
+  void llvm_remove_error_handler(llvm_error_handler_t handler);

The only use of the argument is an assertion, I think you can just  
remove it.

+  // Call registered error handlers, or the default that prints  
message to
+  // stderr.
+  void llvm_handle_error(const std::string &reason,
+                         enum ErrorSeverity severity);

How about naming this "llvm_report_error", and remove the other  
llvm_report_* functions.

+  // Call this after every assert(0 && "something");.
+  // This makes sure that even if assertions are turned off, error  
handlers
+  // are invoked, and program is aborted.
+  static inline void llvm_unreachable(void) {
+    llvm_report_fatal_error("An impossible code path was reached!");
+  }

We don't need this.  Assertions "can't happen", so assert(0) really is  
unreachable.

+++ b/lib/ExecutionEngine/ExecutionEngine.cpp
-          default: assert(0 && "Invalid long double opcode"); abort();
+          default: assert(0 && "Invalid long double  
opcode");llvm_unreachable();

This can go away.

+          std::string Error;
+          raw_string_ostream ss(Error);
+          ss << "Could not resolve external global address: "
+               << I->getName();
+          llvm_report_fatal_error(ss.str());

This (and the one in JIT/JIT.cpp) can be simplified to:  
llvm_report_error("Could not resolve external global address: " + I- 
 >getName());

Otherwise, this is looking like a great first step!  Thank you for  
working on this, please send in a new adjusted patch.

-Chris