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

[Török Edwin]

On 2009-07-01 07:28, Chris Lattner wrote:
> 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.
>   

Fixed, none of the includes is needed now.

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

On 2009-07-01 08:12, Chris Lattner wrote:
> On Jun 30, 2009, at 9:59 PM, Eli Friedman wrote:
>
>   
>> On Tue, Jun 30, 2009 at 9:28 PM, Chris Lattner<clattner at apple.com>  
>> wrote:
>>     
>>> I think that this is over-designed.  The depths of the code generator
>>> should not be producing warnings, ever.
>>>       
>> Instcombine currently can produce warnings; are you suggesting we
>> should get rid of that?
>>     
>
> Yes, without loc info, it is pretty useless.  Working with end users  
> using llvm-gcc, every time they get that warning, they assume it is a  
> bug in the compiler, not a bug in their code.

I agree that warnings without location info are useless.
However if program is compiled with debug info, we might be able to
recover *some* location
info.

I'll just remove report_warning for now, and see if we can come up with
something better in the future.
Perhaps what I really want are debug messages, that can all be turned
on/off via a single flag.

For example, after reading your FAQ on calling conventions mismatches
I'd love to have a debug flag that tells me
whenever I have a *possible* mismatch, even though it may be unreachable
code.
Such a warning should never be shown to an end-user, but it may be
useful during debugging / or for frontend authors to see what went wrong,
when something does go wrong.

Similarly when DAGCombiner replaces code with Undef it may be useful to
show that in some debug message.
Currently it does show that a node is replaced with undef, but it may
get lost in the other debug messages, so having a separate flag for that
could have saved some time when debugging PR4254 for example.
It would also be easier to breakpoint on a single function that is
reporting undefined behaviour.

So maybe something like this in Support/Debug.h:
DEBUG_REPORT_UNDEF(Value* value, Instruction *Use);
DEBUG_REPORT_UNDEF(SDNode *Node);

And they could be all controlled by -debug-only=possible-undef. What do
you think?

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

Ok, I removed the enum.

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

I'm fine with a single error reporting mechanism, with a default action
of exit(1), not abort().
Otherwise people could complain that llvm-gcc crashed on invalid code :)

I assume bugpoint is able to reduce bugs just as well if exit(1) is
called, am I right?

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

What if assertions are turned off? Should we just remove the calls to
abort() after each assert(0)?
I thought the abort()s were there after the assert(0) so that the
compiler knows that the codepaths
never return even with assertions turned off.

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

Ok, I removed both the abort and llvm_unreachable.

> +          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());
>   

Thanks, fixed.

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

Thanks for the review, attached is a new patch.
I marked llvm_report_error as NORETURN, since the compiler can no longer
see that since the implementation is out of line.


Best regards,
--Edwin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: error_handling2.patch
Type: text/x-diff
Size: 6512 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20090701/72ed2928/attachment.patch>