From 1981b6fdb81a317d5e4be357b57378727358d86b Mon Sep 17 00:00:00 2001 From: Marcos Date: Wed, 6 May 2026 12:03:37 -0300 Subject: [PATCH] chore(scheduler): polish bundle (5 deferred MEDIUM/LOW from PR #15) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #16. Bundles all five deferred polish items from the Agent R review of PR #15: - MEDIUM 1: `tests/mock_matmul.rs::alloc_operands` now uses `checked_mul` (mirroring `expected_a_bytes` / `expected_b_bytes` / `expected_out_bytes`) instead of bare `*`, so future contributors copying the helper inherit the overflow-safe idiom. - MEDIUM 2: `OUTPUT_ELEM_BYTES` flips from `pub(crate)` to `pub` and is now imported in the integration test instead of being redeclared as a local `const = 4` that could silently drift. - MEDIUM 3: `matmul_q4_k_returns_not_implemented` no longer silently `return`s when `/dev/null` is unavailable; it creates a fresh tempfile under `std::env::temp_dir()` so the assertion always runs and a CI summary can't hide a vacuously passing test. Best-effort cleanup at the end. - LOW 4: `build.rs` gains an inline comment explaining why `bindgen.layout_tests(false)` is set — enabling layout assertions would require linking libclang at `cargo test` time; the cross-mirror unit test in `lib.rs` covers the only bound struct (`spanker_version`) instead. - LOW 5: `bindgen` build-dep pinned to `=0.69.5` exactly (was `0.69`) with a comment noting that bindgen patch releases have historically shifted generated-code layout. Verification: - `cargo build --workspace` clean - `cargo test --workspace --all-targets` 47/47 (unchanged from baseline: runtime 3, runtime/version_smoke 1, scheduler 15, scheduler/topology_mock 9, ggml-spanker lib 14, ggml-spanker tests/mock_matmul 5) - `cargo test --workspace --doc` 3/3 doctests pass - `cargo clippy --workspace --all-targets -- -D warnings` clean - `cargo fmt --check --all` clean No new features, no scheduler changes, no API breakage. Bumping `OUTPUT_ELEM_BYTES` from `pub(crate)` to `pub` is the only public API surface change and is purely additive. Authored by Agent 3 (Software Stack — Spanker). Signed-off-by: Marcos --- src/backends/ggml/Cargo.toml | 10 ++++++- src/backends/ggml/build.rs | 12 ++++++++ src/backends/ggml/src/lib.rs | 6 +++- src/backends/ggml/src/sail.rs | 39 ++++++++++++++++++-------- src/backends/ggml/tests/mock_matmul.rs | 28 ++++++++++++++---- 5 files changed, 76 insertions(+), 19 deletions(-) diff --git a/src/backends/ggml/Cargo.toml b/src/backends/ggml/Cargo.toml index 60ae47c..6cf6728 100644 --- a/src/backends/ggml/Cargo.toml +++ b/src/backends/ggml/Cargo.toml @@ -33,4 +33,12 @@ thiserror = { workspace = true } # here yet because doing so would require cloning GGML's build # graph; the Q4_K layout is cross-checked separately in # `tests/q4_k_layout.rs` via a standalone C-side static_assert. -bindgen = { version = "0.69", default-features = false, features = ["runtime"] } +# +# The `=0.69.5` exact-version pin is intentional: bindgen patch +# releases have historically shifted the layout of generated +# code (alignment of `#[repr(C)]` shims, derive ordering, etc.). +# Pinning the patch keeps the binding output stable across CI +# rebuilds and across developer machines without relying on a +# committed Cargo.lock alone (Cargo.lock is the source of truth +# for binaries; libraries pinning here is belt-and-suspenders). +bindgen = { version = "=0.69.5", default-features = false, features = ["runtime"] } diff --git a/src/backends/ggml/build.rs b/src/backends/ggml/build.rs index f759d88..5d67e3b 100644 --- a/src/backends/ggml/build.rs +++ b/src/backends/ggml/build.rs @@ -48,6 +48,18 @@ fn main() { .derive_default(true) .derive_debug(true) .derive_copy(true) + // Suppress bindgen's compile-time size/align assertions. + // Enabling them would require the test runner to link + // against libclang at `cargo test` time (the generated + // assertions reference C-side `sizeof`/`alignof` values), + // which is heavier than this crate's UAPI surface + // justifies. The cross-mirror test + // `bindgen_uapi_constants_match_runtime_mirror` in + // `src/lib.rs` covers `size_of::()` + // (the only struct currently bound) at unit-test time + // instead. If WORK_SUBMIT or another non-trivial struct + // is later added to the UAPI header, revisit this and + // either flip layout_tests back on or extend the mirror. .layout_tests(false) .generate() .expect("bindgen failed to generate spanker UAPI bindings"); diff --git a/src/backends/ggml/src/lib.rs b/src/backends/ggml/src/lib.rs index dc29139..0fe736f 100644 --- a/src/backends/ggml/src/lib.rs +++ b/src/backends/ggml/src/lib.rs @@ -73,7 +73,11 @@ pub const Q4_K_BLOCK_BYTES: usize = 144; /// Bytes per element in the f32-shaped output the device returns /// for a Q4_K matmul. The device dequantizes on the way out, so /// `out` is `m × n` f32 values regardless of the input encoding. -pub(crate) const OUTPUT_ELEM_BYTES: usize = 4; +/// +/// Public so integration tests under `tests/` can compute the same +/// expected output-buffer size the production helpers use, without +/// re-declaring a local `const = 4` that could silently drift. +pub const OUTPUT_ELEM_BYTES: usize = 4; /// Errors returned by this crate. /// diff --git a/src/backends/ggml/src/sail.rs b/src/backends/ggml/src/sail.rs index d475c2c..51fce39 100644 --- a/src/backends/ggml/src/sail.rs +++ b/src/backends/ggml/src/sail.rs @@ -70,19 +70,32 @@ mod tests { /// it with the corresponding success-path assertions — do not /// just delete it. /// - /// Uses `/dev/null` for the underlying handle: it always - /// exists, opens read+write, and the test never reaches an - /// ioctl call (the stub short-circuits with `NotImplemented` - /// before any device traffic). + /// Uses a freshly-created temp file under `std::env::temp_dir()` + /// for the underlying handle. The stub short-circuits with + /// `NotImplemented` before issuing any ioctl, so the file's + /// type doesn't matter — only that `open(2)` succeeds. + /// Previous revisions used `/dev/null`, which is read-only in + /// some hermetic sandboxes and caused the test to silently + /// `return` (passing in CI summaries while never asserting + /// anything). A temp file removes that silent-skip path. #[test] fn matmul_q4_k_returns_not_implemented() { - // /dev/null may not be writable as a regular file in - // every sandbox. If we can't open it, skip — running this - // assertion is best-effort until a hermetic harness lands. - let Ok(ctl) = SpankerControl::open_path("/dev/null") else { - eprintln!("/dev/null unavailable for SailMatmul stub test; skipping"); - return; - }; + // Unique-per-process path so concurrent test runs don't + // collide. `cargo test` runs each integration binary in + // its own process, but unit tests inside one binary share + // a PID — `line!()` keeps this filename distinct from any + // sibling test that might adopt the same pattern later. + let tmp_path = std::env::temp_dir().join(format!( + "spanker-sail-stub-{}-{}.tmp", + std::process::id(), + line!() + )); + // `create` truncates if the file already exists from a + // previous run that was killed before cleanup ran. + std::fs::File::create(&tmp_path).expect("temp file should be creatable"); + + let ctl = SpankerControl::open_path(&tmp_path) + .expect("freshly-created temp file should open r+w"); let sail = SailMatmul::new(ctl); let err = sail .matmul_q4_k(&[], &[], &mut [], 0, 0, 0) @@ -91,6 +104,10 @@ mod tests { matches!(err, Error::NotImplemented), "expected NotImplemented, got {err:?}" ); + + // Best-effort cleanup; a leaked file in /tmp is harmless + // but tidiness is cheap. + let _ = std::fs::remove_file(&tmp_path); } #[test] diff --git a/src/backends/ggml/tests/mock_matmul.rs b/src/backends/ggml/tests/mock_matmul.rs index 93f1737..e3613bb 100644 --- a/src/backends/ggml/tests/mock_matmul.rs +++ b/src/backends/ggml/tests/mock_matmul.rs @@ -9,15 +9,31 @@ //! `axi4_mem_model` cocotb harness; until that lands the mock is //! the source of truth for transaction shape. -use ggml_spanker::{Error, MatmulInt4, MockSail, Transaction, Q4_K_BLOCK_BYTES, QK_K}; - -const OUTPUT_ELEM_BYTES: usize = 4; // f32 dequant on the device side +use ggml_spanker::{ + Error, MatmulInt4, MockSail, Transaction, OUTPUT_ELEM_BYTES, Q4_K_BLOCK_BYTES, QK_K, +}; fn alloc_operands(m: usize, k: usize, n: usize) -> (Vec, Vec, Vec) { + // `checked_mul` mirrors the production helpers + // (`expected_a_bytes`, `expected_b_bytes`, `expected_out_bytes`) + // so a future contributor copying this allocator inherits the + // overflow-safe idiom rather than the bare `*` shortcut. let blocks = k / QK_K; - let a = vec![0u8; m * blocks * Q4_K_BLOCK_BYTES]; - let b = vec![0u8; n * blocks * Q4_K_BLOCK_BYTES]; - let out = vec![0u8; m * n * OUTPUT_ELEM_BYTES]; + let a_len = m + .checked_mul(blocks) + .and_then(|v| v.checked_mul(Q4_K_BLOCK_BYTES)) + .expect("A bytes overflow usize"); + let b_len = n + .checked_mul(blocks) + .and_then(|v| v.checked_mul(Q4_K_BLOCK_BYTES)) + .expect("B bytes overflow usize"); + let out_len = m + .checked_mul(n) + .and_then(|v| v.checked_mul(OUTPUT_ELEM_BYTES)) + .expect("OUT bytes overflow usize"); + let a = vec![0u8; a_len]; + let b = vec![0u8; b_len]; + let out = vec![0u8; out_len]; (a, b, out) }