On Aug 19, 2009, at 7:29 PM, Óscar Fuentes wrote: Eli Friedman <eli.friedman at gmail.com> writes:> Besides, in my experience all LLVM asserts I see are Type mismatches > for > instructions and similar cases, i.e., errors about my compiler > producing > wrong LLVM "code" through the API, plus some occasional bug on LLVM, > sometimes about one layer passing the wrong thing to the next layer, > sometimes the bug being the existence of the assert check itself.Failures like this are common in development, but should not occur in production quality code. They mean that your compiler is broken and you should fix it. The Verifier pass is one way to catch some classes of malformed IR before you toss it in to poor unsuspecting optimizers.> And finally, as Aaron points out, a robust library reports failures, > but > shouldn't kill the process.Again, an assertion failure is no different than dereferencing a dangling or garbage pointer. This is a non-recoverable error. llvm_unreachable is the same class of problem. It is semantically the same as assert(0) - an unrecoverable failure. To say it again: an assert is not recoverable an assert is not recoverable an assert is not recoverable Remember that assertions happen when an invariant is broken. Invariants, by definition, don't vary. Assertions are also disabled in certain build modes, and this is important because the whole idea is that they do checking in debug builds that disappear in production builds. It is really not feasible or desirable for them to cause a "recoverable error", even ignoring the fact that there is no good recovery mechanism. I want to reiterate what Eli said, it is worth repeating: "> So what should we use to validate input, such as constructing an > APFloat from a string? llvm_report_error? Or should I change my string > parsing function into a classmethod that returns pointer to an APFloat > or null on failure?Nothing in ADT should be using llvm_report_error; if something is expected to fail, it should return an error code, and otherwise it should assert." In other words, we should define APIs that have strict invariants on the callers. If an API naturally has a failure mode, it should report it by returning an error code. Only in situations where it is really really really unreasonable to do this should the error be reported with llvm_report_error. The cases that fall into this category are things like malformed inline asm constraints etc. It is very difficult for the front-end to fully validate all of these constraints, and so the code generator currently has to report some errors. To be fully clear, I'm not a big fan of the current llvm_report_error approach (though it is definitely better than what we had before!). In my ideal little world, when the backend invokes llvm_report_error, it should *guarantee* that it can continue on and return back. For example, in an inline asm error, I think the code generator should report the error with llvm_report_error and then discard the invalid inline asm and continue code generation. This approach gives clients flexibility over how to handle the problem: if they want to immediately abort they can (and this should be the default behavior). However, if the code generator is embedded into a larger app, the app should be able to install a handler, do code generation and when it completes, it could check for an error and report it however desired. Right now the only reasonable implementation of an error handler ends in exit(1) which is not optimal. -Chris
> To be fully clear, I'm not a big fan of the current llvm_report_error > approach (though it is definitely better than what we had before!). > In my ideal little world, when the backend invokes llvm_report_error, > it should *guarantee* that it can continue on and return back. For > example, in an inline asm error, I think the code generator should > report the error with llvm_report_error and then discard the invalid > inline asm and continue code generation.On one hand, the same thing could occur with any malformed construct... a type mismatch in the values fed to CreateCall, for instance, wouldn't corrupt memory; the error could be reported and the CallInst discarded. On the other hand, of course, if the client is expected to check for that stuff, it would slow things down to check for it again on each CreateXXX call, so I certainly see the logic in keeping that the way it is. I think it would be a big improvement for the asserts in those cases to print out what values it expected and what values it got, say by dumping the offending Value or Type objects to the console along with the line in Instructions.cpp where the assert happens. Would y'all look kindly on a patch for that after the release? I think it would speed up every client development project. I know as I develop my compiler, I would love to have a way to print out that information if, and only if, something is wrong with the values I'm trying to feed CreateXXX.> > This approach gives clients flexibility over how to handle the > problem: if they want to immediately abort they can (and this should > be the default behavior). However, if the code generator is embedded > into a larger app, the app should be able to install a handler, do > code generation and when it completes, it could check for an error and > report it however desired. Right now the only reasonable > implementation of an error handler ends in exit(1) which is not optimal. > > -Chris > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev >
On 2009-08-20 16:50, Kenneth Uildriks wrote:>> To be fully clear, I'm not a big fan of the current llvm_report_error >> approach (though it is definitely better than what we had before!). >> In my ideal little world, when the backend invokes llvm_report_error, >> it should *guarantee* that it can continue on and return back. For >> example, in an inline asm error, I think the code generator should >> report the error with llvm_report_error and then discard the invalid >> inline asm and continue code generation. >> > > On one hand, the same thing could occur with any malformed > construct... a type mismatch in the values fed to CreateCall, for > instance, wouldn't corrupt memory; the error could be reported and the > CallInst discarded. On the other hand, of course, if the client is > expected to check for that stuff, it would slow things down to check > for it again on each CreateXXX call, so I certainly see the logic in > keeping that the way it is. > > I think it would be a big improvement for the asserts in those cases > to print out what values it expected and what values it got, say by > dumping the offending Value or Type objects to the console along with > the line in Instructions.cpp where the assert happens. Would y'all > look kindly on a patch for that after the release? I think it would > speed up every client development project. I know as I develop my > compiler, I would love to have a way to print out that information if, > and only if, something is wrong with the values I'm trying to feed > CreateXXX. >Maybe those asserts could be replaced with an optional failure reporting code? By default it would assert, as currently, and if enabled it can report the exact error to you. Regarding the corrupted memory issue: we have LLVMContext now, just because LLVM/an LLVM module is corrupted in one Context it shouldn't prevent the use of LLVM in other contexts. I think the cases where memory corruption outside LLVMContext triggers an assert is rare. Perhaps asserts could be turned into something else if the user desires via a compile-time switch? Thoughts? P.S.: after 2.6 is released of course Best regards, --Edwin