perf(fse): elide bounds check on init_state + update_state decode reads

.github/copilot-instructions.md

@@ -13,3 +13,27 @@ Pure Rust zstd implementation. Focus: dictionary compression improvements and pe
 - **Clippy:** Must pass `cargo clippy -p structured-zstd --features hash,std,dict_builder -- -D warnings` (`rustc-dep-of-std` is excluded — it's an internal feature for Rust stdlib builds only; `fuzz_exports` is excluded — fuzzing-specific entry points are validated separately from the regular lint gate)
 - Performance-critical code: benchmark before/after any changes
 - **Do not flag `use std::vec` (or `use alloc::vec`) as an unused import.** This crate is `no_std` with an `std` feature; the std prelude is not implicitly in scope, and the `vec![..]` macro resolves through the same path as the module. The import is required wherever the macro is used. Removing it fails the build with `cannot find macro 'vec' in this scope`.
+
+## no-std-First Design Rules
+
+**`structured-zstd` MUST compile and work under `no_std + alloc`.** These rules govern all library code (`src/lib.rs` and its submodules). They are a flat contract — there are no opt-out crate tiers and no per-crate carve-outs; every primitive choice is judged by the rules below.
+
+1. The crate MUST support `no_std + alloc` builds. This is both a CI gate and a design constraint applied during review.
+2. Choose primitives in this order: `core::*` → `alloc::*` → external `no_std + alloc` crate → `std::*` behind `#[cfg(feature = "std")]` → unconditional `std::*` (last resort, requires explicit justification in the PR description).
+3. `Cargo.toml` MUST expose a `std` feature in `[features]` and include it in `default` so that `--no-default-features` produces a `no_std` build. `src/lib.rs` MUST open with `#![cfg_attr(not(feature = "std"), no_std)]` and (when the crate uses heap types) `extern crate alloc;`. A separate `alloc` feature is OPTIONAL — this crate uses `extern crate alloc;` unconditionally and does not gate it behind a feature, because `alloc` is always required. Other crates that need to support pure-`core` no-alloc builds MAY add an `alloc` feature and gate `extern crate alloc;` on it.
+4. CI MUST include a `no-std-check` job that builds with `--no-default-features` (and any feature combinations that should stay no-std-clean, e.g. `--features hash`) and treats warnings as errors. The strongest variant uses a no-std-only target (e.g. `thumbv7em-none-eabihf`) to catch transitive `std` leaks at the linker; a `clippy -D warnings` run on the host target against `--no-default-features` is acceptable provided the crate has no transitive deps that smuggle `std::*` re-exports through unused-import lints. This crate uses the host-clippy variant; future crates SHOULD prefer the embedded target unless they have a documented reason to deviate.
+5. Public API surface (traits, function signatures, returned types) MUST use only `core` / `alloc` types. `std::*` in implementation modules is allowed only when gated behind `#[cfg(feature = "std")]` AND only when the public surface remains alloc-friendly.
+6. `std::collections::HashMap` / `HashSet` are FORBIDDEN. Use `hashbrown::HashMap` / `HashSet`, or `rustc_hash::FxHashMap` for internal-ID keys.
+7. `std::sync::Mutex` / `RwLock` in new code requires explicit justification. Prefer `parking_lot::Mutex` / `RwLock` on hot paths in std-gated code. Use `spin::Mutex` for genuinely no-std code paths and only for very short critical sections.
+8. `std::sync::OnceLock` is FORBIDDEN for fallible init. Use `once_cell::sync::OnceCell::get_or_try_init` or `once_cell::race::OnceBox`.
+9. `thread_local!` is FORBIDDEN in library code. Replace with caller-managed scratch parameters or atomic-pointer patterns.
+10. `std::io::Error` MUST NOT appear in public APIs. Define a crate-local error enum; `From<std::io::Error>` impls live behind `#[cfg(feature = "std")]`.
+11. `std::time::Instant` and `std::time::SystemTime` MUST NOT appear in public APIs. Use a caller-provided clock trait or a `#[cfg(feature = "std")]`-gated convenience wrapper.
+12. `std::thread::*` is FORBIDDEN in library code. Threading must be the caller's responsibility.
+13. `core::*` MUST be used in source whenever the type is available there. `std::*` re-exports of `core` types (e.g. `std::sync::atomic::AtomicU64`) are forbidden — they break the build for no-std targets even when binary-identical for std targets.
+14. Performance does NOT excuse picking `std::*` over a faster `no_std`-ready primitive. Community alternatives (`hashbrown`, `parking_lot`, `smallvec`, `rustc_hash`, `bytes`) are normally faster than their `std::*` counterparts on hot paths — pick them on merit, not as a fallback.
+15. Adding a new unconditional `use std::*` to library code is a one-way ratchet. Reviewers MUST reject such additions unless the PR explicitly justifies why the std primitive cannot be replaced by a no-std-ready alternative or hidden behind `#[cfg(feature = "std")]`.
+16. The `no-std-check` CI job MUST stay green (or, if in transition with `continue-on-error: true`, its compile-error count MUST be monotonically non-decreasing per PR — every PR either keeps the count flat or reduces it; raising it is a regression and blocks merge).
+17. Test code (`#[cfg(test)]`), benches (`benches/`), and binaries (`src/bin/`) MAY use `std::*` freely. Only library code in `src/lib.rs` and its submodules is governed by these rules.
+18. Doc comments and rustdoc `# Examples` blocks on `no_std`-capable APIs MUST NOT depend on `std::*` types if the API itself does not. Doctest examples requiring `std` MUST be gated `#[cfg(feature = "std")]`.
+19. Adding a transitive dependency that pulls `std` into the otherwise no-std-clean build counts as a violation of rule 15 and requires the same approval path.

zstd/src/decoding/errors.rs

@@ -1001,6 +1001,15 @@ impl From<GetBitsError> for FSETableError {
 pub enum FSEDecoderError {
     GetBitsError(GetBitsError),
     TableIsUninitialized,
+    /// Externally constructed `FSETable` violates the
+    /// `decode.len() == 1 << accuracy_log` shape invariant. Only
+    /// reachable under `feature = "fuzz_exports"`, where fuzz
+    /// harnesses can set the `FSETable.decode` / `accuracy_log`
+    /// fields directly and skip `build_decoding_table`.
+    InvalidTableShape {
+        decode_len: usize,
+        accuracy_log: u8,
+    },
 }
 
 #[cfg(feature = "std")]
@@ -1020,6 +1029,19 @@ impl core::fmt::Display for FSEDecoderError {
             FSEDecoderError::TableIsUninitialized => {
                 write!(f, "Tried to use an uninitialized table!")
             }
+            FSEDecoderError::InvalidTableShape {
+                decode_len,
+                accuracy_log,
+            } => match 1usize.checked_shl((*accuracy_log).into()) {
+                Some(expected) => write!(
+                    f,
+                    "FSETable shape invariant violated: decode.len() = {decode_len}, expected 1 << accuracy_log = {expected} (accuracy_log = {accuracy_log})",
+                ),
+                None => write!(
+                    f,
+                    "FSETable shape invariant violated: decode.len() = {decode_len}, accuracy_log = {accuracy_log} overflows 1 << accuracy_log for usize",
+                ),
+            },
         }
     }
 }

zstd/src/fse/fse_decoder.rs

@@ -34,28 +34,153 @@ impl<'t> FSEDecoder<'t> {
         if self.table.accuracy_log == 0 {
             return Err(FSEDecoderError::TableIsUninitialized);
         }
+        // Externally-constructible-table guard: when
+        // `feature = "fuzz_exports"` is on, `FSETable.decode` /
+        // `FSETable.accuracy_log` are settable from outside the crate,
+        // so a fuzz harness can hand the decoder a mis-shaped table
+        // that skips `build_decoding_table`'s invariants. Validate the
+        // table-shape invariant `decode.len() == 1 << accuracy_log`
+        // up-front and surface as a typed `InvalidTableShape` error
+        // (distinct from `TableIsUninitialized` to keep fuzz triage
+        // unambiguous) — without this, `read_entry`'s bounds-checked
+        // indexing under the same cfg would panic on a malformed
+        // table, which fuzz harnesses cannot distinguish from a
+        // legitimate decoder failure. `checked_shl` covers the
+        // pathological case where `accuracy_log >= usize::BITS`.
+        #[cfg(feature = "fuzz_exports")]
+        {
+            let accuracy_log = self.table.accuracy_log;
+            let decode_len = self.table.decode.len();
+            let expected = 1usize.checked_shl(accuracy_log.into()).ok_or(
+                FSEDecoderError::InvalidTableShape {
+                    decode_len,
+                    accuracy_log,
+                },
+            )?;
+            if decode_len != expected {
+                return Err(FSEDecoderError::InvalidTableShape {
+                    decode_len,
+                    accuracy_log,
+                });
+            }
+        }
         let new_state = bits.get_bits(self.table.accuracy_log);
-        self.state = self.table.decode[new_state as usize];
+        // SAFETY: `accuracy_log` bits read from the bitstream produce
+        // `new_state < (1 << accuracy_log) = table_size = decode.len()`.
+        // `build_decoding_table` ensures the table is sized exactly
+        // `1 << accuracy_log` entries. The bounds check that the
+        // checked indexing would emit is provably redundant. Under
+        // `feature = "fuzz_exports"` `read_entry` falls back to the
+        // bounds-checked path — see comment on `read_entry`.
+        self.state = self.read_entry(new_state as usize);
 
         Ok(())
     }
 
     /// Advance the internal state to decode the next symbol in the bitstream.
+    ///
+    /// # Panics
+    ///
+    /// Panics if called on an `FSEDecoder` whose backing `FSETable` has
+    /// not been built yet (empty `decode` vec). `FSEDecoder::new`
+    /// produces such a decoder with a zero-default `state`; the
+    /// well-behaved pipeline is `new` → `init_state` → `update_state*`,
+    /// and `init_state` returns `Err` on an uninitialized table. This
+    /// assertion converts what would otherwise be UB (from the
+    /// unchecked indexing in `read_entry`) into a clear fail-fast
+    /// panic that surfaces the API misuse immediately instead of
+    /// leaving the bitstream and decode state silently desynchronised.
     pub fn update_state(&mut self, bits: &mut BitReaderReversed<'_>) {
+        // Public-API safety guard: `FSEDecoder::new` builds a decoder
+        // with a zero-default `state` (Entry { new_state: 0, num_bits:
+        // 0, symbol: 0 }) regardless of whether the table was actually
+        // populated. A caller that constructs the decoder and then
+        // calls `update_state` BEFORE a successful `init_state` would
+        // hit `read_entry(0)` → `get_unchecked(0)` on an empty
+        // `decode` vec — UB in release mode, since `debug_assert!` is
+        // stripped. Fail-fast with `assert!` instead of silently
+        // returning so that misuse surfaces immediately rather than
+        // leaving the bitstream advanced by some bits but the decode
+        // state stuck at the zero-default Entry — a corruption mode
+        // that the caller has no way to diagnose. The well-behaved
+        // decode pipeline always pairs `new` → `init_state` →
+        // `update_state*`, so this branch is strongly biased "not
+        // taken" and the predictor amortises it to zero cost on the
+        // hot path. The corresponding `update_state_fast` is
+        // `pub(crate)` with controlled callers, so it relies on the
+        // documented precondition instead of paying for a per-call
+        // check.
+        assert!(
+            !self.table.decode.is_empty(),
+            concat!(
+                "FSEDecoder::update_state called on an uninitialized table; ",
+                "call init_state successfully before any update_state* call",
+            ),
+        );
         let num_bits = self.state.num_bits;
         let add = bits.get_bits(num_bits);
         let next_state = usize::from(self.state.new_state) + add as usize;
-        self.state = self.table.decode[next_state];
+        // SAFETY: same invariant as `update_state_fast` below —
+        // `new_state` and `num_bits` were paired by
+        // `calc_baseline_and_numbits` during table construction such
+        // that `new_state + (1 << num_bits) - 1 < table_size =
+        // decode.len()`. `add < 1 << num_bits` by definition of the
+        // `num_bits`-wide read, so `next_state < decode.len()`.
+        self.state = self.read_entry(next_state);
+    }
+
+    /// Read `decode[idx]` — bounds-checked under `fuzz_exports`, unchecked
+    /// otherwise. The call sites all hold the FSE invariant `idx <
+    /// decode.len()` by construction (`init_state` reads
+    /// `accuracy_log` bits, `update_state*` derive `next_state` from
+    /// `Entry.new_state + add` where `calc_baseline_and_numbits`
+    /// guarantees `new_state + (1 << num_bits) - 1 < table_size`).
+    /// Under `fuzz_exports` external code can construct a mis-shaped
+    /// table that violates the invariant — fall back to checked
+    /// indexing so a fuzz harness sees a panic rather than UB, even
+    /// when the fuzz binary is built in release mode (which makes
+    /// `debug_assert!` a no-op and is the default for `cargo fuzz`).
+    #[inline(always)]
+    fn read_entry(&self, idx: usize) -> Entry {
+        #[cfg(feature = "fuzz_exports")]
+        {
+            self.table.decode[idx]
+        }
+        #[cfg(not(feature = "fuzz_exports"))]
+        // SAFETY: see comments at the individual call sites — `idx` is
+        // invariant-bounded by the FSE table-build / state-transition
+        // contract. LLVM cannot prove this on its own because the
+        // invariant spans `build_decoding_table` and decode call sites.
+        unsafe {
+            *self.table.decode.get_unchecked(idx)
+        }
     }
 
     /// Advance the internal state **without** an individual refill check.
     ///
-    /// The caller **must** guarantee that enough bits are available in the bit
-    /// reader (e.g. via [`BitReaderReversed::ensure_bits`] with a budget that
-    /// covers this and any other unchecked reads in the same batch).
+    /// # Preconditions (caller-enforced)
+    ///
+    /// 1. **Bit budget:** enough bits MUST be available in the bit
+    ///    reader (e.g. via [`BitReaderReversed::ensure_bits`] with a
+    ///    budget that covers this and any other unchecked reads in the
+    ///    same batch).
+    /// 2. **State initialisation:** [`init_state`] MUST have returned
+    ///    `Ok` on this decoder before any `update_state_fast` call.
+    ///    Calling `update_state_fast` on a fresh `FSEDecoder::new`
+    ///    output (which holds a zero-default `state` and may reference
+    ///    an empty `decode` vec) would resolve to
+    ///    `read_entry(0).get_unchecked(0)` on an empty slice — UB.
+    ///    The empty-table guard in [`update_state`] is intentionally
+    ///    omitted here to keep the per-sequence fast path branch-free;
+    ///    the only call site (`decode_and_execute_sequences`) always
+    ///    succeeds `init_state` before entering the per-sequence loop,
+    ///    so the precondition holds by construction.
     ///
     /// This is the "fast path" used in the interleaved sequence decode loop
     /// where a single refill check covers all three FSE state updates.
+    ///
+    /// [`init_state`]: Self::init_state
+    /// [`update_state`]: Self::update_state
     #[inline(always)]
     pub(crate) fn update_state_fast(&mut self, bits: &mut BitReaderReversed<'_>) {
         let num_bits = self.state.num_bits;
@@ -68,8 +193,10 @@ impl<'t> FSEDecoder<'t> {
         // `add < 2.pow(num_bits)` by construction of `BitReaderReversed::get_bits_unchecked`.
         // Therefore `next_state < self.table.decode.len()` and the indexed read
         // is in bounds; LLVM cannot prove this invariant on its own because it
-        // spans the table-build and decode call sites.
-        self.state = unsafe { *self.table.decode.get_unchecked(next_state) };
+        // spans the table-build and decode call sites. Under
+        // `feature = "fuzz_exports"` `read_entry` falls back to bounds-checked
+        // indexing — see comment on `read_entry`.
+        self.state = self.read_entry(next_state);
     }
 }