Runtime error handling is easy to get wrong, hard to get right

[steven-johnson]

(From a comment on #6924:)

When an error occurs in any Halide runtime code, you must do BOTH of these things:

• call halide_error(), either directly, or indirectly (typically via the error() stream output)
• AND, return a nonzero error code from the original entry point of the runtime call.

It's easy to leave one of these out, but failing to return a nonzero error code means that anything that replaces the error handler with something that doesn't abort won't see the errors (and may crash).

It's way too easy to make this mistake, and way too hard to vet code to verify that all the sites are correct. We need to think about a way to restructure error handling in our runtime code to make this bulletproof. Strawman:

• struct HalideRuntimeError { int result; };
• Compile-time assertions to verify it is a plain old data structure that is the same size as int (ie, bit-compatible)
• Only ctor is of the form HalideRuntimeError(halide_error_code_t value, char *msg = "");
• Calling this ctor implicitly calls halide_error() with the msg (or some useful default if empty)
• Probably have some macro-ish wrapper to allow constructing text via << as we do now?
• Copy ctor is deleted, but move ctor exists
• All internal code in the Runtime that currently calls error() or halide_error() is revised to return one of these to its caller. (All 'toplevel' halide runtime calls will just return the .result field.)
• We start adapting code at the leaf level and work our way back up; might need some temporary scaffolding to allow everything to get updated properly

[steven-johnson]

Note that this issue is more about coming up with a design that is less mistake-prone than the status quo. For reference, however, here is what I believe to be the Best Practice for the status quo:

• ("Public-facing functions" means a Halide Runtime function that is either defined in HalideRuntime.h, or called by Halide-generated code)
• Public-facing functions should always return a value that is a valid halide_error_code_t (rather than some random integer)
• Public-facing functions should always do both of these when an error occurs:
  • call halide_error() or error()
  • return a valid, nonzero halide_error_code_t as the function result
• Don't ever use halide_abort_if_false():
  • Runtime code should only call abort() if it detects a situation in which it is unsafe for the current process to continue executing -- e.g., memory corruption. This should be infrequent enough that we don't need a macro for it, we can just call it explicitly.
  • Validating things like non-null argument should be mostly handled by halide_debug_assert() (which mainly defines an expected invariant, but only enforces in Debug runtimes); validation that need to happen in non-Debug runtimes should be done by explicit C++ of the form if (bad-thing) { error() << "Bad thing Detected"; return halide_error_code_bad_thing; }