Repository navigation
Check every Vulkan result instead of running on past failures - #3
yujiteshima merged 1 commit into
Conversation
52 vk* call sites, 26 of which return a VkResult, and none were checked. A
failed call left an unusable handle in place and execution carried on, which
showed up in three ways:
- the VM died. With no Vulkan driver present, vkCreateInstance failed, the
null instance went to vkEnumeratePhysicalDevices, and the process aborted
(exit 134) with no Ruby-level error. Same shape in create_buffer: when no
memory type was both host-visible and host-coherent the search fell back
to index 0, and map_buffer then returned NULL straight into a write loop.
- the error named the wrong cause. vkCreateComputePipelines leaves the
handle VK_NULL_HANDLE on failure, which ensure_pipeline read as "the .spv
is missing" -- telling the user to run `make -C shader` for shaders that
were already built and had been rejected by the driver.
- the numbers were quietly wrong. An unchecked vkQueueSubmit meant a lost
device never signalled its fence, vkWaitForFences returned at once, and
the caller read the unwritten buffer back as ordinary Floats.
Adds VK_CHECK / gpu_check, raising a RuntimeError naming the call and the
VkResult. gpu_init and dispatch_compute now take mrb_state so they can raise;
map_buffer too, and it no longer returns NULL. Pipeline state is recorded per
pipeline (PIPE_MISSING_SPV vs PIPE_CREATE_FAILED) so the two cases give
opposite advice. A failed gpu_init sets init_failed rather than being retried
on every later operation, which would leak a fresh context each time.
Since mrb_raise unwinds, buffers are now wrapped before the dispatch that
fills them -- the GC can only free what it owns. That also covers the case
where GPU::SFloat.cast is handed a non-numeric element. narray_sum's partial
buffer is wrapped for the same reason, so destroy_buffer is now only the
finalizer and create_buffer's own unwind path.
Two silent-truncation guards in the same family: GPU::SFloat.new now rejects
a size past UINT32_MAX rather than wrapping it into a different length, and a
dispatch past maxComputeWorkGroupCount is rejected rather than left to the
driver -- lavapipe runs it and returns the right answer, so the limit is easy
to miss in testing, while a hardware-bounded driver may not. GPU.info gains
:max_workgroups so callers and bug reports can see the bound.
Verified on Mesa lavapipe: 46 tests pass (2 new), plus the new
test/shader_error_test.rb in both modes. It is a separate file because it has
to break the process-wide GPU context. Checked by hand that the no-device path
now raises and leaves the VM alive where it previously aborted.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JAgXRCCQ43jwXzeXKS6hG4
Verified on real hardwareRan this branch (so #2 + #3) on a physical GPU. Environment: Apple M5 GPU via MoltenVK, Vulkan 1.1.334, macOS 15 (Darwin 25.5.0), mruby 4.0.0 (
The skip is The crash is really fixedMeasured the headline case both ways, same script, same machine, driver hidden with begin
p GPU::SFloat.new(8).to_a
rescue => e
puts "rescued: #{e.class}: #{e.message}"
end
puts "VM still alive"On On this branch: Independently reproduced before reading this PR, incidentally — I hit the same Merge orderConfirmed the stacking is real: this branch contains #2's FFT work, so #2 first, then this. Still openRaspberry Pi 5 / V3D. The board is powered down; I will run both branches there before merging. That is also where the 🤖 Generated with Claude Code |
Stacked on #2 — the base is that PR's branch, so the diff here is just this one commit. Merge #2 first; GitHub will retarget this to
mainautomatically. See the note at the end for why the two cannot merge in either order.The problem
52
vk*call sites, 26 of which return aVkResult, and none were checked. A failed call left an unusable handle in place and execution carried on. That showed up in three ways, in increasing order of nastiness.1. The VM died with no Ruby-level error. With no Vulkan driver present,
vkCreateInstancefailed, the null instance went tovkEnumeratePhysicalDevices, and the process aborted — measured, exit 134 (SIGABRT), nothing catchable. The same shape existed increate_buffer: the memory-type search defaulted to index 0 when nothing was both host-visible and host-coherent, somap_bufferreturned NULL straight into a write loop.2. The error named the wrong cause.
vkCreateComputePipelinesdocuments that it sets the handle toVK_NULL_HANDLEon failure — whichensure_pipelineread as "the.spvis missing". A shader the driver had rejected was reported asRunmake -C shaderfirst., sending the user to re-run a build step that had already run. This is not hypothetical for #1: if V3DV rejectsbitfieldReverseinfft_stage, that is exactly the message the Pi user would have got.3. The numbers were quietly wrong. An unchecked
vkQueueSubmitmeant a lost device never signalled its fence,vkWaitForFencesreturned immediately, and the caller read the unwritten buffer back as ordinaryFloats. For the streaming spectrum in #1, that is a threshold decision made on data that was never computed.The change
VK_CHECK/gpu_checkraise aRuntimeErrornaming the call and theVkResult:The result-code table is written out by hand rather than pulled from the SDK's
vk_enum_string_helper.h, to keep the only link dependency the loader.gpu_init,dispatch_computeandmap_buffernow takemrb_stateso they can raise.map_bufferno longer returns NULL.PIPE_MISSING_SPVvsPIPE_CREATE_FAILED), so the two cases give opposite advice.gpu_initsetsinit_failedinstead of being retried on every later operation, which would leak a fresh context each time.Unwinding
mrb_raiselongjmps, so anything already allocated has to be released before the check.create_bufferreleases each earlier step before handing the failure togpu_check, anddispatch_compute's record/submit/wait sequence funnels into one exit that frees the fence, command buffer and descriptor set and then raises.The subtler consequence: buffers are now wrapped before the dispatch that fills them, because the GC can only free what it owns. That also closes the pre-existing leak when
GPU::SFloat.castis handed a non-numeric element andmrb_as_floatraises mid-loop.narray_sum's partial buffer is wrapped for the same reason, sodestroy_bufferis now only the finalizer pluscreate_buffer's own unwind.Two silent-truncation guards in the same family
GPU::SFloat.newrejects a size pastUINT32_MAX. Previously the cast touint32_twrapped:GPU::SFloat.new(2**32 + 10)returned a 10-element array.maxComputeWorkGroupCountis rejected rather than left to the driver. Worth being precise here: on lavapipe the oversized dispatch runs and returns the correct answer — I checked — which is exactly why the limit is easy to miss in testing while a driver bounded by a hardware register may behave differently.GPU.infogains:max_workgroupsso callers and bug reports can see the bound.Verified
On Mesa lavapipe, building mruby 3.x with the gem (no warnings):
test/narray_test.rb(2 new).test/shader_error_test.rb(new) passes in both modes. It is a separate file because it has to break the process-wide GPU context — running it alongside the main suite would leave every later test without pipelines. CONTRIBUTING.md documents how to run it.Not run on Pi 5 / V3D or a discrete GPU, same as #2.
Why this is stacked rather than parallel
dispatch_computegained anmrb_stateparameter, and #2's FFT code calls it from 12 sites. Whichever of the two lands second would fail to compile against the other, so they cannot merge in either order. I confirmed this commit is otherwise self-contained by cherry-picking it ontomainalone: two trivial context conflicts, and it builds clean and passes 30/30 tests — but that stripped version knows nothing aboutrfft, so it is not a substitute for merging in order.🤖 Generated with Claude Code
https://claude.ai/code/session_01JAgXRCCQ43jwXzeXKS6hG4
Generated by Claude Code