From 37ba53c4a4ce401a876503d9a2c39a8ff292fb48 Mon Sep 17 00:00:00 2001 From: James Kominick Date: Mon, 12 Oct 2026 07:20:59 -0400 Subject: [PATCH] fix unit tests on 32-bit targets, add `make tests/32bit` and i686 ci job - check `ExpiringCache` field types instead of comparing `size_of` against `UnboundCache` - assert only consistency for a wrong hash passed to `LruCache::pop_raw_with_hash`, and fix its doc - add `make tests/32bit` and `make tests/32bit/docker` to run the suite on `i686-unknown-linux-gnu` - run `make tests/32bit` in CI Fixes #330 --- .github/workflows/build.yml | 49 +++++++++++++++++++++++++++++ AGENTS.md | 7 +++++ CHANGELOG.md | 13 ++++++++ CONTRIBUTING.md | 3 ++ Makefile | 41 ++++++++++++++++++++++++- docker/i686/Dockerfile | 13 ++++++++ src/stores/expiring.rs | 52 +++++++++++++------------------ src/stores/lru.rs | 61 +++++++++++++++++++++++-------------- 8 files changed, 185 insertions(+), 54 deletions(-) create mode 100644 docker/i686/Dockerfile diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 35f7ed9..c3c5c6e 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -102,6 +102,55 @@ jobs: - run: make clean/docker name: Cleaning Docker services + test-32bit: + + runs-on: ubuntu-latest + + permissions: + contents: read + + steps: + - uses: actions/checkout@v6 + + # `rust-src` is for the trybuild golden that pins a standard-library snippet, see the + # `build` job. + - uses: dtolnay/rust-toolchain@1.96.0 + with: + components: rust-src + targets: i686-unknown-linux-gnu + + # `criterion`'s `alloca` dev-dependency compiles C for the test target, which needs + # the 32-bit libc headers. + - run: sudo apt-get update && sudo apt-get install -y gcc-multilib + name: Installing 32-bit C toolchain + + - name: Cache cargo dir + uses: actions/cache@v4 + env: + cache-name: cache-cargo-dir + with: + path: ~/.cargo + key: ${{ runner.os }}-test-32bit-${{ env.cache-name }}-${{ hashFiles('**/Cargo.lock') }} + restore-keys: | + ${{ runner.os }}-test-32bit-${{ env.cache-name }}- + ${{ runner.os }}-test-32bit- + + - name: Cache target dir + uses: actions/cache@v4 + env: + cache-name: cache-target-dir + with: + path: ./target + key: ${{ runner.os }}-test-32bit-${{ env.cache-name }}-${{ hashFiles('**/Cargo.lock') }} + restore-keys: | + ${{ runner.os }}-test-32bit-${{ env.cache-name }}- + ${{ runner.os }}-test-32bit- + + # Store internals (struct layout, which hash bits hashbrown reads) depend on pointer + # width, see #330. `make tests/32bit/docker` runs the same target locally. + - run: make tests/32bit + name: 'Running `make tests/32bit`' + nightly-routing: runs-on: ubuntu-latest diff --git a/AGENTS.md b/AGENTS.md index 72b102d..f3a365e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -296,6 +296,13 @@ Or use the Makefile (auto-starts Docker): make tests ``` +### 32-bit tests +Struct layout and the hash bits hashbrown reads depend on pointer width (see #330). Run the suite on `i686-unknown-linux-gnu` with: +```bash +make tests/32bit/docker # in a container, no local setup +make tests/32bit # natively: needs `rustup target add i686-unknown-linux-gnu` and gcc-multilib +``` + ### Trybuild golden files The `tests/ui/` directory contains compile-fail tests with `.stderr` golden files. When type names or error messages change, regenerate them with: ```bash diff --git a/CHANGELOG.md b/CHANGELOG.md index 0c354e3..c28bc81 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,19 @@ ## [Unreleased] +### Fixed + +- Fix two unit tests that failed on 32-bit targets (#330). The `ExpiringCache` layout test checks + field types instead of comparing `size_of` against `UnboundCache`, and the + `LruCache::pop_raw_with_hash` wrong-hash test asserts only that the cache stays consistent. No + change to the published crates' behavior. + +### Added + +- `make tests/32bit` runs the test suite on `i686-unknown-linux-gnu`, and CI runs it. + `make tests/32bit/docker` runs the same target in a container, with no local 32-bit toolchain + needed. + ## [4.0.1] - 2026-10-01 ### Changed diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 5f3c584..87d380e 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -40,6 +40,9 @@ make ci make help # Run all tests across all feature combinations make tests +# Run the tests on 32-bit (i686) in Docker; `make tests/32bit` runs them natively +# with `rustup target add i686-unknown-linux-gnu` and gcc-multilib installed +make tests/32bit/docker # Run all examples make examples # Sync README.md from src/lib.rs diff --git a/Makefile b/Makefile index 38c4e37..067d05e 100644 --- a/Makefile +++ b/Makefile @@ -66,7 +66,9 @@ TEST_TARGETS = tests \ tests/redis-runtime-axes \ tests/redis-capability-requires-runtime \ tests/redis-tokio \ - tests/all-features + tests/all-features \ + tests/32bit \ + tests/32bit/docker DOCKER_TARGETS = docker/status docker/redis DOC_TARGETS = docs docs/readme @@ -96,6 +98,10 @@ FMT_CCFLAGS = DOCKER_COMMAND = docker DOCKER_REDIS_CONTAINER_NAME = cached-tests DOCKER_REDIS_CONTAINER_LOCAL_PORT = 6399 +DOCKER_32BIT_IMAGE = cached-tests-i686 + +# Target for the 32-bit test run (`tests/32bit`) +TESTS_32BIT_TARGET = i686-unknown-linux-gnu ################################################################################ # Exported variables @@ -149,6 +155,8 @@ help: ## List all supported Make targets tests/redis-capability-requires-runtime) desc="Assert capability-only redis features fail without a runtime" ;; \ tests/redis-tokio) desc="Run async Redis Tokio tests" ;; \ tests/all-features) desc="Run tests with all features enabled" ;; \ + tests/32bit) desc="Run tests on i686 (needs the rustup target and gcc-multilib)" ;; \ + tests/32bit/docker) desc="Run tests/32bit in a Docker container" ;; \ docker/status) desc="Check whether the Docker engine is available" ;; \ docker/redis) desc="Start the Redis test container" ;; \ docs) desc="Sync generated documentation artifacts" ;; \ @@ -326,6 +334,37 @@ tests/all-features: | docker/redis @echo "[$@]: Running tests (all features)..." $(CARGO_COMMAND) test --all-features -- --nocapture +# 32-bit run of the unit and integration tests. Store internals (struct layout, which +# hash bits hashbrown reads) depend on pointer width, see #330. Needs the +# `TESTS_32BIT_TARGET` rustup target and 32-bit libc headers (`gcc-multilib` on +# Debian/Ubuntu), which `criterion`'s `alloca` dev-dependency compiles against. +# `tests/32bit/docker` runs this target in a container that has both. +tests/32bit: + @rustup target list --installed | grep -qx $(TESTS_32BIT_TARGET) \ + || (>&2 echo 'Missing rustup target, run `rustup target add $(TESTS_32BIT_TARGET)` or `make tests/32bit/docker`'; exit 1) + @printf '#include \n' | cc -m32 -E - > /dev/null 2>&1 \ + || (>&2 echo 'Missing 32-bit libc headers, install gcc-multilib or run `make tests/32bit/docker`'; exit 1) + @echo "[$@]: Running tests on $(TESTS_32BIT_TARGET) (default features)..." + $(CARGO_COMMAND) test --target $(TESTS_32BIT_TARGET) --tests + @echo "[$@]: Running tests on $(TESTS_32BIT_TARGET) (no default features)..." + $(CARGO_COMMAND) test --target $(TESTS_32BIT_TARGET) --no-default-features --tests + +# Runs `tests/32bit` in `DOCKER_32BIT_IMAGE` as the host user, so files written to the +# mounted repo stay owned by you. The cargo home and target dir live under the +# gitignored `local/docker-32bit/` and are reused between runs. The repo is not mounted at +# `/src`: trybuild rewrites the crate dir to `$DIR` in compiler output, and `/src` also +# occurs in the toolchain's `lib/rustlib/src/rust` path, which breaks the UI goldens. +tests/32bit/docker: docker/status + @echo [$@]: Building $(DOCKER_32BIT_IMAGE) image... + $(DOCKER_COMMAND) build -t $(DOCKER_32BIT_IMAGE) docker/i686 + mkdir -p local/docker-32bit/cargo local/docker-32bit/target + $(DOCKER_COMMAND) run --rm --user "$$(id -u):$$(id -g)" \ + -v "$(CURDIR):/workspace/cached" -w /workspace/cached \ + -e HOME=/tmp \ + -e CARGO_HOME=/workspace/cached/local/docker-32bit/cargo \ + -e CARGO_TARGET_DIR=/workspace/cached/local/docker-32bit/target \ + $(DOCKER_32BIT_IMAGE) make tests/32bit + ################################################################################ # Starts a Redis server using `DOCKER_COMMAND` docker/redis: docker/status diff --git a/docker/i686/Dockerfile b/docker/i686/Dockerfile new file mode 100644 index 0000000..4326be5 --- /dev/null +++ b/docker/i686/Dockerfile @@ -0,0 +1,13 @@ +# Image for `make tests/32bit/docker`: the toolchain pinned in rust-toolchain.toml plus the +# i686 target and the 32-bit libc headers that `make tests/32bit` needs. `jq` is for the +# `bin/` scripts that `tests/release_scripts.rs` runs. +FROM rust:1.96.0 + +RUN apt-get update \ + && apt-get install -y --no-install-recommends gcc-multilib jq \ + && rm -rf /var/lib/apt/lists/* + +# Installed as root here because the tests run as the host user, who cannot write to +# RUSTUP_HOME. Keep this in sync with rust-toolchain.toml. +RUN rustup component add rustfmt clippy rust-src \ + && rustup target add wasm32-unknown-unknown i686-unknown-linux-gnu diff --git a/src/stores/expiring.rs b/src/stores/expiring.rs index e217452..c5cf1f3 100644 --- a/src/stores/expiring.rs +++ b/src/stores/expiring.rs @@ -1607,37 +1607,29 @@ mod tests { #[test] fn struct_holds_hashmap_directly_not_an_unbound_cache() { - // `ExpiringCache` used to wrap a full `UnboundCache` (HashMap + two - // `StripedCounter`s, each a `Box<[Slot]>` fat-pointer field, plus a dead - // `on_evict` field) purely for `cache_clear` / `cache_reset` / `cache_size` - // / `cache_reset_metrics`, while every read/write bypassed it entirely. - // Now the struct holds a `HashMap` directly (plus an `initial_capacity` - // hint for `cache_reset`'s shrink behavior). + // `ExpiringCache` used to wrap a full `UnboundCache` purely for + // `cache_clear` / `cache_reset` / `cache_size` / `cache_reset_metrics`, + // while every read/write bypassed it entirely. Now the struct holds a + // `HashMap` directly (plus an `initial_capacity` hint for `cache_reset`'s + // shrink behavior). // - // The prior version of this test hard-coded the expected byte size of - // `Option` / `AtomicU64` / `Option>` and a padding - // slack constant. `#[repr(Rust)]` layout (padding, niche optimization, - // field reordering) is not guaranteed by the language, so that arithmetic - // could drift on a different toolchain or target and flake without any - // real regression. Comparing directly against the sibling `UnboundCache` - // struct (same K/V, built in the same compilation) sidesteps all of that: - // whatever the concrete field layout is on a given target, `ExpiringCache` - // holding its own bare `HashMap` + `initial_capacity` + 3x `AtomicU64` + - // `on_evict` is strictly smaller than `UnboundCache` holding a `HashMap` + - // 2x `StripedCounter` + `initial_capacity` + `on_evict` (one extra - // `AtomicU64` costs less than a second `StripedCounter`). If `ExpiringCache` - // ever again embeds an `UnboundCache` (directly or via a wrapper), its size - // becomes `UnboundCache`'s size *plus* its own extra fields, which flips - // this comparison and fails the assertion on any target. - let expiring = std::mem::size_of::>(); - let unbound = std::mem::size_of::>(); - assert!( - expiring < unbound, - "ExpiringCache ({expiring} bytes) must be smaller than UnboundCache \ - ({unbound} bytes) for the same K/V types; equal-or-larger implies \ - ExpiringCache is once again wrapping a full UnboundCache instead of \ - holding a bare HashMap directly" - ); + // This is checked at compile time rather than by comparing + // `size_of` against `UnboundCache`: struct layout depends on the target + // (e.g. a `Box<[Slot]>` is the same size as an `AtomicU64` on 32-bit, + // see #330). The exhaustive destructure fails to compile if a field is + // added (e.g. an embedded `UnboundCache`), and the type ascription fails + // if `store` stops being a bare `HashMap`. Adding a field to + // `ExpiringCache` means updating this pattern. + let c: ExpiringCache = ExpiringCache::builder().build().unwrap(); + let ExpiringCache { + store, + initial_capacity: _, + hits: _, + misses: _, + evictions: _, + on_evict: _, + } = c; + let _: HashMap = store; } #[test] diff --git a/src/stores/lru.rs b/src/stores/lru.rs index 4f1d1a9..7f3d054 100644 --- a/src/stores/lru.rs +++ b/src/stores/lru.rs @@ -407,8 +407,9 @@ impl LruCache { /// [`pop_raw`](Self::pop_raw) for callers that already hold the key's hash. /// /// `hash` MUST be `self.hash(k)` (i.e. produced by this cache's own hash builder for - /// this key); passing any other value makes the lookup miss and the entry is left in - /// place. Use this when a caller has computed the hash for an earlier probe on the + /// this key). Passing any other value may miss or may still find and remove the entry + /// for `k` (which bits hashbrown reads depends on the target); either way the store and + /// the LRU order stay consistent, since the probe still compares keys. Use this when a caller has computed the hash for an earlier probe on the /// same key -- it skips only the re-hash, the `K: Eq` bucket probe still runs. pub(super) fn pop_raw_with_hash(&mut self, hash: u64, k: &Q) -> Option<(K, V)> where @@ -2871,28 +2872,42 @@ mod tests { } #[test] - fn pop_raw_with_hash_wrong_hash_misses_and_leaves_the_entry() { - // Documented misuse contract: `hash` MUST be `self.hash(k)`. Flipping the top - // bit changes hashbrown's control tag deterministically, so the probe cannot - // match -- the lookup misses and the entry stays put. - let mut c: LruCache = LruCache::new(4); - c.cache_set(1, 10); - c.cache_set(2, 20); - c.cache_set(3, 30); - let before = c.key_order(); - - let wrong = c.hash(&2u32) ^ (1u64 << 63); - assert_eq!( - c.pop_raw_with_hash(wrong, &2u32), - None, - "a hash that is not self.hash(k) must silently miss" - ); - assert_eq!(c.cache_size(), 3); - assert_eq!(c.key_order(), before); - assert_eq!(c.cache_peek(&2), Some(&20)); - assert_store_and_order_agree(&c); + fn pop_raw_with_hash_wrong_hash_leaves_the_cache_consistent() { + // Documented misuse contract: `hash` MUST be `self.hash(k)`. A wrong hash may + // miss or may still find the entry, depending on which hash bits hashbrown reads + // for the probe position and control tag on this target (only the low 32 on + // 32-bit, see #330). The test asserts only what holds everywhere: the result is + // a miss or the entry for `k`, and the store and LRU order stay consistent. + let fresh = || { + let mut c: LruCache = LruCache::new(4); + c.cache_set(1, 10); + c.cache_set(2, 20); + c.cache_set(3, 30); + c + }; + let before = fresh().key_order(); + + for bit in 0..64 { + let mut c = fresh(); + let wrong = c.hash(&2u32) ^ (1u64 << bit); + match c.pop_raw_with_hash(wrong, &2u32) { + None => { + assert_eq!(c.cache_size(), 3, "bit {bit}"); + assert_eq!(c.key_order(), before, "bit {bit}"); + assert_eq!(c.cache_peek(&2), Some(&20), "bit {bit}"); + } + Some(kv) => { + assert_eq!(kv, (2, 20), "bit {bit}"); + assert_eq!(c.cache_size(), 2, "bit {bit}"); + assert_eq!(c.key_order(), vec![3, 1], "bit {bit}"); + assert_eq!(c.cache_peek(&2), None, "bit {bit}"); + } + } + assert_store_and_order_agree(&c); + } - // The correct hash still finds it, and the miss left no damage behind. + // The correct hash always finds it. + let mut c = fresh(); let right = c.hash(&2u32); assert_eq!(c.pop_raw_with_hash(right, &2u32), Some((2, 20))); assert_eq!(c.key_order(), vec![3, 1]);