jouleclaw-omni: retire both soundness allows from #329 — alignment and &mut-from-& #373

Merged

dcharlot merged 2 commits from fix/gguf-cast-alignment into main 2026-10-21 05:39:47 -04:00

Conversation 0 Commits 2 Files changed 5 +188 -69

2026-10-20

Claude

MetalBuffer: a method that writes to the buffer takes &mut self ...

`clippy::mut_from_ref` flagged

    pub unsafe fn as_mut_slice<T: bytemuck::Pod>(&self) -> &mut [T]

which hands out a mutable slice from a shared reference. #329 allowed the
lint, reasoning that the method is `unsafe` so the caller carries the
requirement. That conflated two different requirements. The documented
one -- "must ensure GPU is not accessing this buffer" -- is genuinely the
caller's. The one the signature creates is not: two callers holding a
`&MetalBuffer` can each take a `&mut [T]` over the same bytes, and
aliasing `&mut` is undefined behaviour on the Rust side whatever the GPU
is doing.

Both now take `&mut self`: `as_mut_slice`, and `copy_from_slice`, which
has the same shape and was not flagged only because it does not hand the
slice back. The doc comments say which half of the contract the borrow
settles and which is still the caller's.

This costs nothing. `MetalBuffer::as_slice`, `as_mut_slice` and
`copy_from_slice` have no callers anywhere in the workspace, and
`MetalBuffer` itself is used nowhere outside jouleclaw-omni, so there is
no caller churn to review -- which is why this landed here rather than
waiting for an API decision. I had assumed otherwise before checking.

With this and the alignment commit, both soundness allows added in #329
are gone and `clippy -- -D warnings` passes without either.

One thing the sweep caught in my own work: the new conversion tests used
3.141_592_7 as a literal, and `approx_constant` -- which I deliberately
did not allow, being correctness-adjacent -- failed the build until they
used `core::f32::consts::PI`.

Co-Authored-By: Claude Opus 5 <[email protected]>

fdce8f2faa

• View at this point in history

2026-10-20 17:43:22 -04:00

Claude

jouleclaw-omni: read mmap'd GGUF bytes without assuming their alignment ...

`clippy::cast_ptr_alignment` flagged eight casts from `*const u8` to
more-strictly-aligned pointers. The previous commit allowed the lint
crate-wide with a note that GGUF aligns tensor data in practice. That
note was true and the reasoning was still wrong: these functions take
plain `&[u8]` slices and a `metal::Buffer`, and neither carries the
alignment the cast asserts.

    let src_f32 = std::slice::from_raw_parts(src.as_ptr().cast::<f32>(), src.len() / 4);

`src` is an mmap'd region at whatever offset the file chose. Misaligned
`from_raw_parts` is undefined behaviour, not a slow load, and the
compiler is entitled to assume it cannot happen.

The three that looked safest were the least safe to reason about:
`contents()` on a buffer built with `new_buffer_with_bytes_no_copy` is
the mmap'd pointer itself, so "it's a Metal allocation, those are page
aligned" does not hold for them either.

All eight now decode byte-wise through `from_le_bytes` over
`as_chunks::<N>()`, which needs no unsafe alignment claim at all --
`u8` has alignment 1. The two staging loops keep one cast each, for the
destination, which really is a fresh Metal allocation; the comment says
which side is which and why. The crate-level allow is gone, and
`clippy -- -D warnings` passes without it.

Four unit tests, because the rewrite moved every byte index and a wrong
one would be silent:

- the bf16 and f32 buffer walks must agree with the scalar converters
  element for element, over zero, ±1, pi, +inf, a NaN and f16's max;
- conversion from an odd offset on BOTH sides, which is the case the old
  code could not express without UB;
- a trailing partial element is dropped rather than read past the end.

The metal CI job also now runs every target under `--features metal`.
It ran only `--test metal_kernels`, which left the crate's 46
metal-gated lib tests unrun -- a feature's unit tests being as invisible
to CI as its code was is the same mistake one level down. That job now
runs 48 lib tests and the 26 kernel tests.

Still outstanding from the same sweep: `mut_from_ref` on
`MetalBuffer::as_mut_slice(&self)`, which hands out a `&mut` from a `&`.

Co-Authored-By: Claude Opus 5 <[email protected]>

e4edd51f49

• View at this point in history

2026-10-20 17:09:37 -04:00