Skip to content

Contract: a failed Vulkan buffer setup no longer frees the qmm fixture's buffers twice - #1992

Merged
ashhart merged 1 commit into
ashhart:mainfrom
joshuaswarren:contract-prepare-ownership
Nov 5, 2026
Merged

ashhart merged 1 commit into
ashhart:mainfrom
joshuaswarren:contract-prepare-ownership

Conversation

@joshuaswarren

Copy link
Copy Markdown
Contributor

The Vulkan kernel contract runner aborted instead of reporting a failure when a device buffer could not be allocated.

Fixture.prepare in zig/tests/vulkan/contract_qmm.zig moves its host buffers into the fixture, then sets up device buffers, which can fail. The errdefers for those host buffers (and for the Affine value, which was set to undefined after the move) stayed armed. A failure after the move freed them, and the next reset() freed them again.

The errdefers now live in a new adopt() that returns once the buffers are in the fixture. After that, reset() owns them.

Check

tf-vulkan-kernel-contract built with the Vulkan option and run on llvmpipe, whose maxStorageBufferRange is 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.

  • Before: general protection exception in Affine.deinit called from prepare, rc 134, after the qmm_bf16 nonfinite and repeat gates.
  • After: FAIL shapes vulkan qmm_bf16 affine q4 g64 transposed: TooLarge, all 8 controls caught, rc 1, and a SUMMARY line:
SUMMARY llvmpipe (LLVM 15.0.6, 256 bits) 1 kernels: 0 pass gates 1-4, 1 fail, 0 known findings; 8 controls: 8 caught, 0 missed; 0 limit failures; gates 71145 ms

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.zig stays under 600 lines.

[Joshua agent]

@joshuaswarren

Copy link
Copy Markdown
Contributor Author

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.

@ashhart

ashhart commented Nov 3, 2026

Copy link
Copy Markdown
Owner

Thanks @joshuaswarren. An independent review on current main (d38bba3):
Verdict: LAND after one run on a Vulkan GPU.

Applying it: cleanly on main d38bba3. The lean check stays at 424. The author line is a real GitHub identity
(@joshuaswarren, noreply address). zig/tests/vulkan/contract_qmm.zig goes from 429 to 432 lines.

Nothing was built or run here.

The bug is real, as described. In main's Fixture.prepare:

  • errdefer q.deinit() and six errdefer f.a.free(...) stay armed after the buffers move into f.
  • q = undefined follows the move, so a later failure (input snapshot allocation, vk.Buffer.init, upload,
    pipeline.bind) runs deinit on an undefined value. That is the general protection fault the author reports.
  • The other six buffers are freed by the errdefers and again by the next reset(), a double free.

The fix holds.

  • The allocation and the move go into a new adopt(). Its errdefers fire only on its own errors (fill, the
    allocations, reference, NonFiniteReference), before anything is stored in f. f's fields are still the
    empty slices reset() left, so nothing is freed twice.
  • Once adopt() returns, f owns every buffer and reset() frees each once. f.made counts only buffers that
    were created, so a failed Buffer.init is not destroyed.
  • Passing runs are unchanged: the device setup moves verbatim, in the same order, with the same bindings and rig.
    Only q = undefined is dropped, and nothing reads q afterwards.
  • No other Vulkan fixture has the pattern. contract_qmm.zig is the only one with errdefers.

Test: none that fails on old code. That would need a Vulkan device whose buffer range is too small. The
author's llvmpipe run (rc 134 before; a FAIL ... TooLarge line, 8 of 8 controls caught and a SUMMARY line
after) is the evidence, and it is not GPU evidence.

Device run: the Vulkan kernel contract on a real GPU, to show the SUMMARY line and exit status match main's.

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 SUMMARY lines and exit codes? It lands on that.

@joshuaswarren

Copy link
Copy Markdown
Contributor Author

Ran the Vulkan kernel contract runner on a real Vulkan GPU at head 851ce36c (clean tree).

Build and command (cross-built for aarch64, run on the device):

zig build tf-vulkan-kernel-contract -Dvulkan=true -Dtarget=aarch64-linux-gnu -Doptimize=safe
VK_DRIVER_FILES=<Honeykrisp ICD json> ./tf-vulkan-kernel-contract

Device: a 13-inch Apple M1 (G13G B1), Honeykrisp, Mesa 26.3.0-devel (git-9323171c12), Linux 7.1.12. Exit code 0.

SUMMARY Apple M1 (G13G B1) 1 kernels: 1 pass gates 1-4, 0 fail, 0 known findings; 8 controls: 8 caught, 0 missed; 0 limit failures; gates 9481 ms

qmm_bf16 passes all five gates (limits, nonfinite, repeat, shapes, sums: 15 shapes within the float64 bound, worst 0.990 of the bound). The FAIL lines in the output are the 8 planted controls, each red at the gate it was built to break.

On this device maxStorageBufferRange is 2147483647, so the prepare failure path that aborted on llvmpipe (128 MiB range) is not reached here. The before and after on llvmpipe are in the PR description.

[Joshua agent]

@ashhart

ashhart commented Nov 4, 2026

Copy link
Copy Markdown
Owner

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 SUMMARY line and exit code. That shows the passing path is unchanged, and then this lands.

@joshuaswarren

Copy link
Copy Markdown
Contributor Author

The main-branch contract run is complete on the same 13-inch Apple M1 and Honeykrisp driver as the PR-head run. Main commit b85fd1f461adff1bee83a38286bf7abf45378cc0, runner SHA256 e09cf68cf126210ef8477dbaa312e39407821c9f5f572fa742fb89568b87e9a9, Mesa 26.3.0-devel (git-9323171c12). Exit code: 0.

SUMMARY Apple M1 (G13G B1) 1 kernels: 1 pass gates 1-4, 0 fail, 0 known findings; 8 controls: 8 caught, 0 missed; 0 limit failures; gates 9556 ms

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.

@ashhart

ashhart commented Nov 4, 2026

Copy link
Copy Markdown
Owner

Thanks @joshuaswarren, the main run on the same M1 and driver is what we asked for: the same SUMMARY and exit 0 on
both, so the passing path is unchanged, and the llvmpipe runs cover the failure path. This is ready to land.

@ashhart
ashhart force-pushed the contract-prepare-ownership branch from 851ce36 to a3ad2d6 Compare November 5, 2026 01:06
…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.
@ashhart
ashhart force-pushed the contract-prepare-ownership branch from a3ad2d6 to c9a6c73 Compare November 5, 2026 02:51
@ashhart
ashhart merged commit fb08950 into ashhart:main Nov 5, 2026
0 of 7 checks passed
@ashhart

ashhart commented Nov 5, 2026

Copy link
Copy Markdown
Owner

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.

@ashhart

ashhart commented Nov 5, 2026

Copy link
Copy Markdown
Owner

Thanks @joshuaswarren! This is in TensorFold 1.2.3, out now: https://github.com/ashhart/TensorFold/releases/tag/v1.2.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants