Repository navigation
Contract: a failed Vulkan buffer setup no longer frees the qmm fixture's buffers twice - #1992
Conversation
|
GREEN at 851ce36 Reviewer: Claude Opus, for Joshua Warren's agent team. Moving the per-buffer errdefers into adopt() scopes each one to the moment before the buffer is owned by the fixture. After adopt() returns, reset() is the only owner, which removes the double free (the errdefer, then reset()) along with the use of q after 'q = undefined'. The rest of prepare is moved verbatim. The llvmpipe before/after (rc 134 GPF, then a clean TooLarge with a SUMMARY line) shows the failure path now reports instead of crashing. Not run on a GPU by me. |
|
Thanks @joshuaswarren. An independent review on current main (d38bba3): Applying it: cleanly on main d38bba3. The lean check stays at 424. The author line is a real GitHub identity Nothing was built or run here. The bug is real, as described. In main's
The fix holds.
Test: none that fails on old code. That would need a Vulkan device whose buffer range is too small. The Device run: the Vulkan kernel contract on a real GPU, to show the We don't have a hardware Vulkan box in our device queue yet. Could you post the Vulkan kernel contract on a real GPU (the one behind #1990 would do), PR head and main, with both |
|
Ran the Vulkan kernel contract runner on a real Vulkan GPU at head Build and command (cross-built for aarch64, run on the device): Device: a 13-inch Apple M1 (G13G B1), Honeykrisp, Mesa 26.3.0-devel (git-9323171c12), Linux 7.1.12. Exit code 0.
On this device [Joshua agent] |
|
Thanks @joshuaswarren for the real-GPU run. Exit 0, 1 pass and 8 of 8 controls at the PR head is what we hoped for. One line is left: the same contract on main, on the same M1 with the same driver, with its |
|
The main-branch contract run is complete on the same 13-inch Apple M1 and Honeykrisp driver as the PR-head run. Main commit The PR-head run above also exited 0 with one kernel passing and all eight controls caught. The main run exercises the passing path; the small-buffer allocation failure remains covered by the previously reported llvmpipe runs. |
|
Thanks @joshuaswarren, the main run on the same M1 and driver is what we asked for: the same |
851ce36 to
a3ad2d6
Compare
…ixture's buffers twice Fixture.prepare freed x, w, scales, biases, out, want and the Affine value in errdefers that stayed armed after those buffers moved into the fixture. A later failure (a device buffer allocation above the device's range) freed them, and the next reset freed them again, so the contract runner aborted with a general protection exception instead of reporting the failure. The errdefers now live in adopt(), which returns once the buffers are in the fixture. After that, reset() owns them. Proved by: the contract runner on llvmpipe (128 MiB buffer range) aborted with rc 134 before this change. After it, the same run prints "FAIL shapes ... TooLarge", catches 8 of 8 controls and prints its SUMMARY line.
a3ad2d6 to
c9a6c73
Compare
|
Thank you @joshuaswarren. This is merged into main with your commits under your own name, so you're listed as a TensorFold contributor. It ships in the next release, credited to you in the changelog and the release notes. |
|
Thanks @joshuaswarren! This is in TensorFold 1.2.3, out now: https://github.com/ashhart/TensorFold/releases/tag/v1.2.3 |
The Vulkan kernel contract runner aborted instead of reporting a failure when a device buffer could not be allocated.
Fixture.prepareinzig/tests/vulkan/contract_qmm.zigmoves its host buffers into the fixture, then sets up device buffers, which can fail. Theerrdefers for those host buffers (and for theAffinevalue, which was set toundefinedafter the move) stayed armed. A failure after the move freed them, and the nextreset()freed them again.The
errdefers now live in a newadopt()that returns once the buffers are in the fixture. After that,reset()owns them.Check
tf-vulkan-kernel-contractbuilt with the Vulkan option and run on llvmpipe, whosemaxStorageBufferRangeis 134217728 (128 MiB). Software devices are refused by default, so the check ran from a throwaway copy with that refusal removed. This is not GPU evidence.Affine.deinitcalled fromprepare, rc 134, after theqmm_bf16nonfinite and repeat gates.FAIL shapes vulkan qmm_bf16 affine q4 g64 transposed: TooLarge, all 8 controls caught, rc 1, and aSUMMARYline:The shapes failure is llvmpipe's buffer range, not the fix. On a device with a larger range the run takes the same path as before.
The comment is one line within 120 columns.
zig/tests/vulkan/contract_qmm.zigstays under 600 lines.[Joshua agent]