jouleclaw-omni: retire both soundness allows from #329 — alignment and &mut-from-& #373
Loading…
Reference in a new issue
No description provided.
Delete branch "fix/gguf-cast-alignment"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
#329 reached clippy-zero partly by allowing two lints and writing down why. Both notes were honest about what the code did and both reasonings were still wrong. This removes the allows and fixes the code, so
clippy -- -D warningspasses without either.1.
cast_ptr_alignment— eight casts off an mmapThe #329 note said GGUF aligns tensor data in practice. True — but these functions take a plain
&[u8], so nothing in their signature carries that, and misalignedfrom_raw_partsis undefined behaviour the compiler may assume cannot happen, not a slow load.The three sites that looked safest were the least safe to reason about:
contents()on a buffer created withnew_buffer_with_bytes_no_copyis the mmap'd pointer itself, so "Metal allocations are page-aligned" does not cover them.All eight now decode byte-wise through
from_le_bytesoveras_chunks::<N>()— no alignment claim needed,u8has alignment 1. Two casts remain, for destinations that really are fresh Metal allocations; the comments say which side is which.Tests — the rewrite moved every byte index and a wrong one would be silent:
2.
mut_from_ref—&muthanded out from&The #329 note said the method is
unsafe, so the caller carries it. That conflated two 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 holders of a&MetalBuffercan each take a&mut [T]over the same bytes, and aliasing&mutis UB regardless of the GPU.as_mut_sliceandcopy_from_slicenow take&mut self. The second has the same shape and escaped the lint only because it does not hand the slice back.This costs nothing: those methods have no callers anywhere in the workspace and
MetalBufferis used nowhere outside this crate, so there is no caller churn. I had assumed an API decision was needed before checking that.A CI gap this exposed
The metal job ran only
--test metal_kernels, leaving 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. It now runs every target under--features metal: 48 lib tests and the 26 kernel tests.And the sweep caught my own work: the new conversion tests used
3.141_592_7, andapprox_constant— correctness-adjacent, so deliberately not allowed — failed the build until they usedcore::f32::consts::PI.🤖 Generated with Claude Code
`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]>`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]>jouleclaw-omni: read mmap'd GGUF bytes without assuming their alignmentto jouleclaw-omni: retire both soundness allows from #329 — alignment and &mut-from-&