Device runtime "wrap native" routines should check for invalid device buffers and return an error

[slomp]

t looks like we don't check the "validity" of the native buffer in the various wrap-native implementations:

Halide/src/runtime/vulkan.cpp

Lines 1337 to 1347 in 9864bd4

	WEAK int halide_vulkan_wrap_vk_buffer(void *user_context, struct halide_buffer_t *buf, uint64_t vk_buffer) {
	halide_debug_assert(user_context, buf->device == 0);
	if (buf->device != 0) {
	error(user_context) << "Vulkan: Unable to wrap buffer ... invalid device pointer!\n";
	return halide_error_code_device_wrap_native_failed;
	}
	buf->device = vk_buffer;
	buf->device_interface = &vulkan_device_interface;
	buf->device_interface->impl->use_module();
	return halide_error_code_success;
	}

Halide/src/runtime/metal.cpp

Lines 1230 to 1247 in 9864bd4

	WEAK int halide_metal_wrap_buffer(void *user_context, struct halide_buffer_t *buf, uint64_t buffer) {
	if (buf->device != 0) {
	error(user_context) << "halide_metal_wrap_buffer: device field is already non-zero.";
	return halide_error_code_generic_error;
	}
	device_handle *handle = (device_handle *)malloc(sizeof(device_handle));
	if (handle == nullptr) {
	error(user_context) << "halide_metal_wrap_buffer: malloc failed making device handle.";
	return halide_error_code_out_of_memory;
	}
	handle->buf = (mtl_buffer *)buffer;
	handle->offset = 0;
	buf->device = (uint64_t)handle;
	buf->device_interface = &metal_device_interface;
	buf->device_interface->impl->use_module();
	return halide_error_code_success;
	}

Halide/src/runtime/cuda.cpp

Lines 1227 to 1246 in 9864bd4

	WEAK int halide_cuda_wrap_device_ptr(void *user_context, struct halide_buffer_t *buf, uint64_t device_ptr) {
	halide_abort_if_false(user_context, buf->device == 0);
	if (buf->device != 0) {
	error(user_context) << "halide_cuda_wrap_device_ptr: device field is already non-zero";
	return halide_error_code_generic_error;
	}
	buf->device = device_ptr;
	buf->device_interface = &cuda_device_interface;
	buf->device_interface->impl->use_module();
	#ifdef DEBUG_RUNTIME
	if (auto result = validate_device_pointer(user_context, buf);
	result != halide_error_code_success) {
	buf->device_interface->impl->release_module();
	buf->device = 0;
	buf->device_interface = nullptr;
	return result;
	}
	#endif
	return halide_error_code_success;
	}

Halide/src/runtime/opencl.cpp

Lines 1231 to 1257 in 9864bd4

	WEAK int halide_opencl_wrap_cl_mem(void *user_context, struct halide_buffer_t *buf, uint64_t mem) {
	halide_abort_if_false(user_context, buf->device == 0);
	if (buf->device != 0) {
	error(user_context) << "halide_opencl_wrap_cl_mem: device field is already non-zero.";
	return halide_error_code_generic_error;
	}
	device_handle *dev_handle = (device_handle *)malloc(sizeof(device_handle));
	if (dev_handle == nullptr) {
	return halide_error_code_out_of_memory;
	}
	dev_handle->mem = (cl_mem)mem;
	dev_handle->offset = 0;
	buf->device = (uint64_t)dev_handle;
	buf->device_interface = &opencl_device_interface;
	buf->device_interface->impl->use_module();
	#ifdef DEBUG_RUNTIME
	auto result = validate_device_pointer(user_context, buf);
	if (result) {
	free((device_handle *)buf->device);
	buf->device = 0;
	buf->device_interface->impl->release_module();
	buf->device_interface = nullptr;
	return result;
	}
	#endif
	return halide_error_code_success;
	}

In D3D12 there's a check (a rather overkill one):

Halide/src/runtime/d3d12compute.cpp

Lines 3476 to 3477 in 9864bd4

	ID3D12Resource *pResource = reinterpret_cast<ID3D12Resource *>(d3d12_resource);
	halide_abort_if_false(user_context, (pResource != nullptr));

In fact, some of the interface comments even mention the call can fail if an invalid device pointer is passed:

Halide/src/runtime/HalideRuntimeCuda.h

Lines 40 to 47 in 9864bd4

	/** Set the underlying cuda device poiner for a buffer. The device
	* pointer should be allocated using cuMemAlloc or similar and must
	* have an extent large enough to cover that specified by the
	* halide_buffer_t extent fields. The dev field of the halide_buffer_t
	* must be NULL when this routine is called. This call can fail due to
	* being passed an invalid device pointer. The device and host dirty
	* bits are left unmodified. */
	extern int halide_cuda_wrap_device_ptr(void *user_context, struct halide_buffer_t *buf, uint64_t device_ptr);

Halide/src/runtime/HalideRuntimeMetal.h

Lines 41 to 48 in 9864bd4

	/** Set the underlying MTLBuffer for a halide_buffer_t. This memory should be
	* allocated using newBufferWithLength:options or similar and must
	* have an extent large enough to cover that specified by the halide_buffer_t
	* extent fields. The dev field of the halide_buffer_t must be NULL when this
	* routine is called. This call can fail due to running out of memory
	* or being passed an invalid buffer. The device and host dirty bits
	* are left unmodified. */
	extern int halide_metal_wrap_buffer(void *user_context, struct halide_buffer_t *buf, uint64_t buffer);

Halide/src/runtime/HalideRuntimeOpenCL.h

Lines 82 to 89 in 9864bd4

	/** Set the underlying cl_mem for a halide_buffer_t. This memory should be
	* allocated using clCreateBuffer or similar and must have an extent
	* large enough to cover that specified by the halide_buffer_t extent
	* fields. The dev field of the halide_buffer_t must be NULL when this
	* routine is called. This call can fail due to running out of memory
	* or being passed an invalid device pointer. The device and host
	* dirty bits are left unmodified. */
	extern int halide_opencl_wrap_cl_mem(void *user_context, struct halide_buffer_t *buf, uint64_t device_ptr);

(Vulkan does not expose the wrap/unwrap symbols in HalideRuntimeVulkan.h)

[slomp]

What's the proper way to report the error back to the caller?

• return an error code and leave it up to the caller?
• call halide_error (knowing that the default handler will terminate the process)?

[steven-johnson]

What's the proper way to report the error back to the caller?

• return an error code and leave it up to the caller?
• call halide_error (knowing that the default handler will terminate the process)?

You must do both in runtime code, since you don't know if the caller has overridden halide_error() or not (it may be relying on it to abort, or it may be relying on it not to abort so that it can check the eventual error code)

[slomp]

Something like this, then?

if (proverbial_hits_the_fan) {
    halide_error(user_context, message);
    return halide_error_code_xyzw;
}