Check results of all runtime function calls #6389
Conversation
This cherry-picks just the changes to callsites internal to Halide (and tests) from #6388. (It doesn't attempt to annotate runtime functions to enforce checking the results.)
|
The AOT tests and the app don't override halide_error, so the return code is statically known to be zero and shouldn't be checked. |
src/runtime/write_debug_image.cpp
Outdated
| halide_copy_to_host(user_context, buf); | ||
| if (halide_copy_to_host(user_context, buf) != 0) { | ||
| halide_error(user_context, "halide_copy_to_host failed.\n"); | ||
| return -1; |
There was a problem hiding this comment.
This should possibly return the error code instead of -1
There was a problem hiding this comment.
And actually halide_error was probably already called if halide_copy_to_host failed, so no need to call it again.
There was a problem hiding this comment.
Good catch, done
Yes, but people learning Halide will copy-n-paste from our tests and apps, and checking result codes should be best practice everywhere. |
| @@ -16,7 +16,11 @@ int main(int argc, char **argv) { | |||
| Buffer<int> out(64, 64); | |||
| out.set_min(32, 32); | |||
|
|
|||
| halide_buffer_copy(nullptr, input, nullptr, out); | |||
| int result = halide_buffer_copy(nullptr, input, nullptr, out); | |||
There was a problem hiding this comment.
The only way this could possibly fail is if the allocation of a 64x64 image failed above, but we already wrote into that allocation in the buffer constructor so it would have crashed already. I think we may disagree, but I don't think it's good practice to check return codes when failure is impossible in this context. It's just confusing for anyone reading the code. So I wouldn't want to see this in user code.
That said, this is a test to ensure that halide_buffer_copy does the right thing, and returning a non-zero value in a situation where it should be impossible for it to return a non-zero value is a good test.
| @@ -162,7 +162,11 @@ int main(int argc, char **argv) { | |||
| { | |||
| Buffer<int> copy(raw_buf); | |||
| } | |||
| halide_device_free(nullptr, &raw_buf); | |||
| int result = halide_device_free(nullptr, &raw_buf); | |||
There was a problem hiding this comment.
Failure should be impossible in this specific context again. The point of this test is to create a trace of debug events so that we can check for lifetime issues. I guess this is a secondary test of if the buffer becomes malformed or something.
| @@ -82,7 +82,9 @@ int main(int argc, char **argv) { | |||
| printf("argmin expected value\n stack peak: %d\n", argmin_stack_peak); | |||
| printf("\n"); | |||
|
|
|||
| halide_do_par_for(nullptr, launcher_task, 0, num_launcher_tasks, nullptr); | |||
There was a problem hiding this comment.
launcher_task unconditionally returns zero, so this call also unconditionally returns zero, and the check is just confusing.
| @@ -39,10 +39,13 @@ int main(int argc, char **argv) { | |||
|
|
|||
| // Hijack halide's runtime to run a bunch of instances of this function | |||
| // in parallel. | |||
| halide_do_par_for(nullptr, launcher_task, 0, num_launcher_tasks, nullptr); | |||
| int result = halide_do_par_for(nullptr, launcher_task, 0, num_launcher_tasks, nullptr); | |||
There was a problem hiding this comment.
Same concern applies here. There's no way for this call to return a non-zero value.
apps/lens_blur/process.cpp
Outdated
| @@ -21,7 +21,7 @@ int main(int argc, char **argv) { | |||
| // Let the Halide runtime hold onto GPU allocations for | |||
| // intermediates and reuse them instead of eagerly freeing | |||
| // them. cuMemAlloc/cuMemFree is slower than the algorithm! | |||
| halide_reuse_device_allocations(nullptr, true); | |||
| (void)halide_reuse_device_allocations(nullptr, true); // ignore errors | |||
There was a problem hiding this comment.
This can only possibly return non-zero if the second arg is false. Turning on reuse of device allocations can't fail, and no errors are possible. It just configures future behavior.
There was a problem hiding this comment.
This can only possibly return non-zero if the second arg is false
That may be true of the current implementation, but I don't see that mentioned in the documentation for this call. But since this isn't a test, I'll just drop this change.
|
A possible middle ground for tests with pedagogical value is a comment stating "There's no way for this call to fail in this context, but it's good practice to check return codes and people may copy-paste this code into some other context." That could encourage good practices without confusing readers with apparently pointless checks. |
This cherry-picks just the changes to callsites internal to Halide (and tests) from #6388. (It doesn't attempt to annotate runtime functions to enforce checking the results.)