Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check results of all runtime function calls #6389

Merged
merged 7 commits into from
Nov 8, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions src/runtime/HalideBuffer.h
Original file line number Diff line number Diff line change
Expand Up @@ -240,15 +240,19 @@ class Buffer {
"Call device_free explicitly if you want to drop dirty device-side data. "
"Call copy_to_host explicitly if you want the data copied to the host allocation "
"before the device allocation is freed.");
int result = 0;
if (dev_ref_count && dev_ref_count->ownership == BufferDeviceOwnership::WrappedNative) {
buf.device_interface->detach_native(nullptr, &buf);
result = buf.device_interface->detach_native(nullptr, &buf);
} else if (dev_ref_count && dev_ref_count->ownership == BufferDeviceOwnership::AllocatedDeviceAndHost) {
buf.device_interface->device_and_host_free(nullptr, &buf);
result = buf.device_interface->device_and_host_free(nullptr, &buf);
} else if (dev_ref_count && dev_ref_count->ownership == BufferDeviceOwnership::Cropped) {
buf.device_interface->device_release_crop(nullptr, &buf);
result = buf.device_interface->device_release_crop(nullptr, &buf);
} else if (dev_ref_count == nullptr || dev_ref_count->ownership == BufferDeviceOwnership::Allocated) {
buf.device_interface->device_free(nullptr, &buf);
result = buf.device_interface->device_free(nullptr, &buf);
}
// No reasonable way to return the error, but we can at least assert-fail in debug builds.
assert((result == 0) && "device_interface call returned a nonzero result in Buffer::decref()");
(void)result;
}
if (dev_ref_count) {
if (dev_ref_count->ownership == BufferDeviceOwnership::Cropped) {
Expand Down
5 changes: 4 additions & 1 deletion src/runtime/cache.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,10 @@ WEAK bool CacheEntry::init(const uint8_t *cache_key, size_t cache_key_size,

WEAK void CacheEntry::destroy() {
for (uint32_t i = 0; i < tuple_count; i++) {
halide_device_free(nullptr, &buf[i]);
if (halide_device_free(nullptr, &buf[i]) != 0) {
// Just log a debug message, there's not much we can do in response here
debug(nullptr) << "CacheEntry::destroy: halide_device_free failed\n";
}
halide_free(nullptr, get_pointer_to_header(buf[i].host));
}
halide_free(nullptr, metadata_storage);
Expand Down
4 changes: 2 additions & 2 deletions src/runtime/device_interface.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ WEAK int copy_to_host_already_locked(void *user_context, struct halide_buffer_t
return halide_error_code_copy_to_host_failed;
}
buf->set_device_dirty(false);
halide_msan_annotate_buffer_is_initialized(user_context, buf);
(void)halide_msan_annotate_buffer_is_initialized(user_context, buf); // ignore errors

return result;
}
Expand Down Expand Up @@ -264,7 +264,7 @@ WEAK int halide_device_free(void *user_context, struct halide_buffer_t *buf) {
* error. Used when freeing as a destructor on an error. */
WEAK void halide_device_free_as_destructor(void *user_context, void *obj) {
struct halide_buffer_t *buf = (struct halide_buffer_t *)obj;
halide_device_free(user_context, buf);
(void)halide_device_free(user_context, buf); // ignore errors
}

/** Allocate host and device memory to back a halide_buffer_t. Ideally this
Expand Down
21 changes: 17 additions & 4 deletions src/runtime/halide_buffer_t.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,10 @@ halide_buffer_t *_halide_buffer_crop(void *user_context,
dst->device_interface = nullptr;
dst->device = 0;
if (src->device_interface) {
src->device_interface->device_crop(user_context, src, dst);
if (src->device_interface->device_crop(user_context, src, dst) != 0) {
// No way to report it: just ignore it
// debug(user_context) << "_halide_buffer_crop: device_crop failed\n";
}
}
return dst;
}
Expand All @@ -168,6 +171,7 @@ halide_buffer_t *_halide_buffer_crop(void *user_context,
HALIDE_BUFFER_HELPER_ATTRS
int _halide_buffer_retire_crop_after_extern_stage(void *user_context,
void *obj) {
int result;
halide_buffer_t **buffers = (halide_buffer_t **)obj;
halide_buffer_t *crop = buffers[0];
halide_buffer_t *parent = buffers[1];
Expand All @@ -178,15 +182,24 @@ int _halide_buffer_retire_crop_after_extern_stage(void *user_context,
// stage. It only represents the cropped region, so we
// can't just give it to the parent.
if (crop->device_dirty()) {
crop->device_interface->copy_to_host(user_context, crop);
result = crop->device_interface->copy_to_host(user_context, crop);
if (result != 0) {
return result;
}
}
result = crop->device_interface->device_free(user_context, crop);
if (result != 0) {
return result;
}
crop->device_interface->device_free(user_context, crop);
} else {
// We are a crop of an existing device allocation.
if (crop->device_dirty()) {
parent->set_device_dirty();
}
crop->device_interface->device_release_crop(user_context, crop);
result = crop->device_interface->device_release_crop(user_context, crop);
if (result != 0) {
return result;
}
}
}
if (crop->host_dirty()) {
Expand Down
10 changes: 8 additions & 2 deletions src/runtime/matlab.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -507,12 +507,18 @@ WEAK int halide_matlab_call_pipeline(void *user_context,

if (arg_metadata->kind == halide_argument_kind_output_buffer) {
halide_buffer_t *buf = (halide_buffer_t *)args[i];
halide_copy_to_host(user_context, buf);
if ((result = halide_copy_to_host(user_context, buf)) != 0) {
error(user_context) << "halide_matlab_call_pipeline: halide_copy_to_host failed.\n";
return result;
}
}
if (arg_metadata->kind == halide_argument_kind_input_buffer ||
arg_metadata->kind == halide_argument_kind_output_buffer) {
halide_buffer_t *buf = (halide_buffer_t *)args[i];
halide_device_free(user_context, buf);
if ((result = halide_device_free(user_context, buf)) != 0) {
error(user_context) << "halide_matlab_call_pipeline: halide_device_free failed.\n";
return result;
}
}
}

Expand Down
8 changes: 4 additions & 4 deletions src/runtime/msan.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ WEAK void annotate_helper(void *uc, const device_copy &c, int d, int64_t off) {

if (d == -1) {
const void *from = (void *)(c.src + off);
halide_msan_annotate_memory_is_initialized(uc, from, c.chunk_size);
(void)halide_msan_annotate_memory_is_initialized(uc, from, c.chunk_size); // ignore errors
} else {
for (uint64_t i = 0; i < c.extent[d]; i++) {
annotate_helper(uc, c, d - 1, off);
Expand All @@ -50,7 +50,7 @@ WEAK void check_helper(void *uc, const device_copy &c, int d, int64_t off, const

if (d == -1) {
const void *from = (void *)(c.src + off);
halide_msan_check_memory_is_initialized(uc, from, c.chunk_size, buf_name);
(void)halide_msan_check_memory_is_initialized(uc, from, c.chunk_size, buf_name); // ignore errors
} else {
for (uint64_t i = 0; i < c.extent[d]; i++) {
check_helper(uc, c, d - 1, off, buf_name);
Expand Down Expand Up @@ -97,8 +97,8 @@ WEAK int halide_msan_check_buffer_is_initialized(void *user_context, halide_buff
return 0;
}

halide_msan_check_memory_is_initialized(user_context, (void *)b, sizeof(*b), buf_name);
halide_msan_check_memory_is_initialized(user_context, (void *)b->dim, b->dimensions * sizeof(b->dim[0]), buf_name);
(void)halide_msan_check_memory_is_initialized(user_context, (void *)b, sizeof(*b), buf_name); // ignore errors
(void)halide_msan_check_memory_is_initialized(user_context, (void *)b->dim, b->dimensions * sizeof(b->dim[0]), buf_name); // ignore errors

Halide::Runtime::Internal::device_copy c = Halide::Runtime::Internal::make_host_to_device_copy(b);
if (c.chunk_size == 0) {
Expand Down
2 changes: 1 addition & 1 deletion src/runtime/tracing.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -385,7 +385,7 @@ WEAK int halide_shutdown_trace() {

namespace {
WEAK __attribute__((destructor)) void halide_trace_cleanup() {
halide_shutdown_trace();
(void)halide_shutdown_trace(); // ignore errors
}
} // namespace
}
5 changes: 4 additions & 1 deletion src/runtime/write_debug_image.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,10 @@ WEAK extern "C" int32_t halide_debug_to_file(void *user_context, const char *fil
return -1;
}

halide_copy_to_host(user_context, buf);
int result = halide_copy_to_host(user_context, buf);
if (result != 0) {
return result;
}

ScopedFile f(filename, "wb");
if (!f.open()) {
Expand Down
23 changes: 20 additions & 3 deletions test/generator/buffer_copy_aottest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,22 @@ using namespace Halide::Runtime;
int main(int argc, char **argv) {
// Test simple host to host buffer copy.

// Note that only way halide_buffer_copy() could possibly fail is if
// the allocation of the images failed (which would simply crash us),
// so checking the return code is arguably redundant in this context
// (but testing to verify that is part of a good test).

{
Buffer<int> input(128, 128);
input.fill([&](int x, int y) { return x + 10 * y; });
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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

if (result != 0) {
printf("halide_buffer_copy() failed\n");
exit(-1);
}

Buffer<int> in_crop = input.cropped(0, 32, 64).cropped(1, 32, 64);
out.for_each_value([&](int a, int b) {
Expand Down Expand Up @@ -44,7 +53,11 @@ int main(int argc, char **argv) {
out.set_min(32, 32);
Buffer<int> in_crop = input.cropped(0, 32, 64).cropped(1, 32, 64);

halide_buffer_copy(nullptr, in_crop, dev, out);
int result = halide_buffer_copy(nullptr, in_crop, dev, out);
if (result != 0) {
printf("halide_buffer_copy() failed\n");
exit(-1);
}

out.copy_to_host();

Expand Down Expand Up @@ -72,7 +85,11 @@ int main(int argc, char **argv) {
in_crop.set_host_dirty(false);
in_crop.set_device_dirty();

halide_buffer_copy(nullptr, in_crop, nullptr, out);
int result = halide_buffer_copy(nullptr, in_crop, nullptr, out);
if (result != 0) {
printf("halide_buffer_copy() failed\n");
exit(-1);
}

in_crop.copy_to_host();

Expand Down
7 changes: 6 additions & 1 deletion test/generator/gpu_object_lifetime_aottest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,12 @@ int main(int argc, char **argv) {
{
Buffer<int> copy(raw_buf);
}
halide_device_free(nullptr, &raw_buf);
// Note that a nonzero result should be impossible here (in theory)
int result = halide_device_free(nullptr, &raw_buf);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

if (result != 0) {
printf("Error! halide_device_free() returned: %d\n", result);
return -1;
}
}

// Test coverage for Halide::Runtime::Buffer construction from halide_buffer_t, taking ownership
Expand Down
6 changes: 5 additions & 1 deletion test/generator/memory_profiler_mandelbrot_aottest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,11 @@ 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);
Copy link
Member

@abadams abadams Nov 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

launcher_task unconditionally returns zero, so this call also unconditionally returns zero, and the check is just confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commented.

// Note that launcher_task() always returns zero, thus halide_do_par_for()
// should always return zero, but since this is a test, let's verify that.
int result = halide_do_par_for(nullptr, launcher_task, 0, num_launcher_tasks, nullptr);
assert(result == 0);
(void)result;

halide_profiler_state *state = halide_profiler_get_state();
assert(state != nullptr);
Expand Down
9 changes: 7 additions & 2 deletions test/generator/user_context_insanity_aottest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,15 @@ 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);
// Note that launcher_task() always returns zero, thus halide_do_par_for()
// should always return zero, but since this is a test, let's verify that.
int result = halide_do_par_for(nullptr, launcher_task, 0, num_launcher_tasks, nullptr);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same concern applies here. There's no way for this call to return a non-zero value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commented.

assert(result == 0);
(void)result;

for (int i = 0; i < num_launcher_tasks; ++i)
for (int i = 0; i < num_launcher_tasks; ++i) {
assert(got_context[i] == true);
}

printf("Success!\n");
return 0;
Expand Down
5 changes: 4 additions & 1 deletion tools/RunGenMain.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -520,7 +520,10 @@ int main(int argc, char **argv) {

// This is a single-purpose binary to benchmark this filter, so we
// shouldn't be eagerly returning device memory.
halide_reuse_device_allocations(nullptr, true);
int result = halide_reuse_device_allocations(nullptr, true);
if (result != 0) {
std::cerr << "halide_reuse_device_allocations() returned an error: " << result << "\n";
}

if (benchmark) {
if (benchmarks_flag_value.empty()) {
Expand Down