Rename halide_assert -> halide_abort_if_false #6382
Conversation
A crashing bug got mistakenly inserted because a new contributor (reasonably) assumed that the `halide_assert()` macro in our runtime code was like a C `assert()` (i.e., something that would vanish in optimized builds). This is not the case; it is a check that happens in all build modes and always triggers an `abort()` if it fires. We should remove any ambiguity about it, so this proposes to rename it to somethingmore like the Google/Abseil-style CHECK() macro, to make it stand out more. (We may want to do a followup to verify that all of the uses really are unrecoverable errors that aren't better handled by returning an error.)
steven-johnson
changed the title
|
Failures unrelated (see #6386) |
steven-johnson
changed the title
|
Should the original use that caused this problem be an abort even in debug mode? I think maybe it was just wrong to assert at all. In which case, the problem isn't that assert is active in non-debug builds at all. In general I think the only reason to disable asserts in non-debug builds isn't because we should tolerate assert failures there, but we just don't want the performance hit due to assert overhead. So, I question whether this PR and related PRs are needed at all? |
|
The name is still misleading though, because it evokes "assert", so even if no usage changes +1 to the rename. |
I think it's both. An assertion failure / abort() in our runtime should be the absolutely last resort, and only for errors that are truly unrecoverable. The vast majority of existing |
src/CodeGen_LLVM.cpp
Outdated
| @@ -1159,21 +1159,30 @@ void CodeGen_LLVM::optimize_module() { | |||
| #endif | |||
| pb.registerOptimizerLastEPCallback( | |||
| [](ModulePassManager &mpm, OptimizationLevel level) { | |||
| #if LLVM_VERSION >= 140 | |||
There was a problem hiding this comment.
Not sure what's going on in this file?
|
Oh you merged in a fix for trunk llvm branch. Got it. |
A crashing bug got mistakenly inserted because a new contributor (reasonably) assumed that the
halide_assert()macro in our runtime code was like a Cassert()(i.e., something that would vanish in optimized builds).This is not the case; it is a check that happens in all build modes and always triggers an
abort()if it fires. We should remove any ambiguity about it, so this proposes to rename it to somethingmore like the Google/Abseil-style CHECK() macro, to make it stand out more.(We may want to do a followup to verify that all of the uses really are unrecoverable errors that aren't better handled by returning an error.)