Add no_std and WASM support with full backward compatibility #28
Conversation
|
@loadingalias This looks really good, thanks! Reviewing now. Would you mind updating the README with the various features so people can figure out how to build each of the targets? 👍 |
There was a problem hiding this comment.
Pull Request Overview
This PR adds comprehensive no_std and WASM support to the crc-fast library while maintaining full backward compatibility. The changes include:
- Adding configurable feature flags (std, alloc, cache, ffi, panic-handler)
- Implementing dual synchronization paths (std vs spin crate for no_std)
- Enhancing FFI error handling with proper error codes and thread-local error tracking
- Fixing undefined behavior and memory leaks in algorithm.rs and software fallback
Reviewed Changes
Copilot reviewed 33 out of 36 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/wasm_tests.rs | New comprehensive WASM compatibility test suite with 13 tests |
| tests/no_std_tests.rs | New no_std test suite validating core functionality without std |
| Cargo.toml | Added spin and hashbrown dependencies, restructured features for no_std support |
| src/lib.rs | Added no_std support with panic handler, conditional imports for alloc/std |
| src/ffi.rs | Enhanced with error handling system, fallible conversions, better null safety |
| src/feature_detection.rs | Dual-path implementation for runtime (std) vs compile-time (no_std) detection |
| src/cache.rs | Refactored to support both std::sync and spin synchronization primitives |
| src/algorithm.rs | Fixed UB with DataRegion struct for safe overlapping SIMD reads |
| src/arch/* | Updated SIMD intrinsic imports from std::arch to core::arch |
| .github/workflows/tests.yml | Added CI jobs for embedded targets and WASM platforms |
Comments suppressed due to low confidence (1)
src/cache.rs:69
- Fixed trailing whitespace in doc comment.
/// * `poly` - Polynomial value for the CRC algorithm
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| //! Tests that the library works in WebAssembly. These tests run natively | ||
| //! (test framework requires std) but exercise code paths used in WASM. | ||
| //! | ||
| //! Run tests: cargo test --test wasm_test |
There was a problem hiding this comment.
Corrected test file name from 'wasm_test' to 'wasm_tests' in comment.
| //! Tests the library works without std. The test framework requires std, | ||
| //! but this exercises all no_std code paths. | ||
| //! | ||
| //! Run tests: cargo test --test real_no_std_test |
There was a problem hiding this comment.
Corrected test command from 'real_no_std_test' to 'no_std_tests' in comment.
| const VERSION: &CStr = | ||
| match CStr::from_bytes_with_nul(concat!(env!("CARGO_PKG_VERSION"), "\0").as_bytes()) { | ||
| Ok(version) => version, | ||
| Err(_) => panic!("package version contains null bytes??"), | ||
| // Fallback to "unknown" if version string is malformed | ||
| Err(_) => c"unknown", | ||
| }; |
There was a problem hiding this comment.
The fallback to 'unknown' doesn't set an error code. Consider calling set_last_error(CrcFastError::StringConversionError) in the Err arm for consistency with other functions that document error behavior.
| bench = true | ||
|
|
||
| [dependencies] | ||
| crc = "3" |
There was a problem hiding this comment.
When default-features = false is specified, the 'alloc' feature may not be available. Consider removing the features array or documenting why this specific combination is needed, as it could cause build failures if the digest crate's alloc feature depends on its default features.
| crc = "3" | |
| crc = "3" | |
| # We use digest with default-features = false and features = ["alloc"] to enable heap allocation support in no_std environments. | |
| # This configuration is safe because the alloc feature in digest does not depend on its default features as of digest v0.10. |
| /// Data region descriptor for overlapping SIMD reads in CRC processing | ||
| struct DataRegion<'a> { | ||
| full_data: &'a [u8], | ||
| offset: usize, | ||
| remaining: usize, | ||
| } |
There was a problem hiding this comment.
The DataRegion struct lacks documentation about its invariants. The function documentation mentions requirements (offset >= CRC_CHUNK_SIZE, remaining in 1..=15), but these should also be documented on the struct itself as safety-critical invariants.
|
I've addressed the README.md update and tried to keep it minimal and clear. I also added a couple tiny updates to docs/comments for clarity as per the Copilot overview. Cool feature. Haha. Let me know if it's good to go! |
|
@loadingalias Looks great, with one caveat. I'm working on some small doc improvements, updating the Makefile, etc... The caveat is that gating Do you have an opinion on whether it's ok to include in |
|
My proposed changes are here, specifically this FFI commit. |
|
Rebased onto main & resolved conflicts. Had to fix a few build failures that were introduced in the recent commits (missing struct fields in no_std detection, unconditional imports on unsupported architectures). All sorted now. Cherry-picked your FFI commit as well - makes sense for backward compatibility. |
This commit adds comprehensive no_std compatibility while maintaining 100% backward compatibility with existing std-based code. Key changes: - Add optional `std` feature (enabled by default) - Add `alloc` and `cache` features for heap allocation in no_std - Use spin crate for no_std synchronization (Once, RwLock, Mutex) - Use hashbrown for no_std HashMap when cache feature enabled - Fix UB in algorithm.rs (potential out-of-bounds read) - Fix Miri memory leaks in software fallback caching - Add dual-path feature detection (runtime with std, compile-time without) - Add comprehensive test coverage (tests/no_std_tests.rs, tests/wasm_tests.rs) - Add CI workflows for embedded (ARM, RISC-V) and WASM targets - Update all modules to use core/alloc primitives where appropriate Tested on: - Embedded: thumbv7em, thumbv8m, riscv32 - WASM: wasm32-unknown-unknown, wasm32-wasip1, wasm32-wasip2 - All existing x86/x86_64/aarch64 targets - Miri validation passes (no memory leaks)
- Add comprehensive Features section to README explaining all feature flags - Document no_std and WASM build instructions with examples - Add safety invariant documentation to DataRegion struct - Address PR review feedback for improved documentation clarity
Gating it behind the `ffi` feature flag would be a breaking change for 3rd party applications which depend on a build-from-source workflow. Also improve docs while we’re in here.
- Add conditional import of get_arch_ops (only for supported architectures) - Remove feature = "std" guard from fusion module import (fusion works in no_std) - Add missing has_crc and has_sse42 fields to ArchCapabilities in no_std feature detection Fixes build failures for: - PowerPC targets (software fallback) - Embedded ARM targets (thumbv7em, thumbv8m, etc.) - WASM targets (wasm32-unknown-unknown, wasm32-wasip1, wasm32-wasip2)
- Reorganize Features section to show default features separately - Document that ffi is now in default features (backward compatibility) - Note that ffi will become optional in v2.0
|
Hey, man. I'm a big fan of what you're doing here. I'd like to remain involved. I was wondering if there was a way for us to chat ad-hoc about the crate, the direction, the goals? Maybe a one on one or a GH Discussion we could open or something? I hate to pollute these Issues with frivolous chatter; it's a bad habit. I'm sorry. Haha. I'm just really, really invested it and care about it. Let me know. Cheers! |
|
@loadingalias Just opened up Discussions so we can take it there. Love to hear what else you'd like to do with this project. 😄 |
2 participants
The Problem
The
crc-fastlibrary is currently std-only, which prevents its use in embedded systems (bare metal, RTOS), WebAssembly environments, and other no_std contexts where the standard library is unavailable.This limits adoption in:
Additionally, the software fallback implementation had memory leaks detected by Miri, and algorithm.rs contained undefined behavior (potential out-of-bounds read).
The Solution
Add comprehensive no_std compatibility while maintaining 100% backward compatibility with existing std-based code. All existing APIs and functionality remain unchanged when using default features.
Changes
Core Architecture:
stdfeature (enabled by default) to preserve existing behaviorallocfeature for heap allocation in no_std environmentscachefeature for optional caching in no_std (useshashbrownHashMap)core::andalloc::primitives instead ofstd::Synchronization:
spincrate for no_std synchronization primitives (Once,RwLock,Mutex)Bug Fixes:
algorithm.rs(potential out-of-bounds read on line 74)Testing:
tests/no_std_tests.rs- 13 comprehensive tests for no_std functionalitytests/wasm_tests.rs- 13 comprehensive tests for WASM compatibilitythumbv7em-none-eabihf,thumbv8m.main-none-eabihf,riscv32imac-unknown-none-elfwasm32-unknown-unknown,wasm32-wasip1,wasm32-wasip2Tricky Areas:
src/cache.rs(lines 25-150): Dual implementation using either std or spin synchronizationsrc/feature_detection.rs(lines 50-120): Runtime vs compile-time feature detection pathssrc/algorithm.rs(line 74): Bounds checking fix for the UB issuesrc/arch/software.rs(lines 200-350): Memory leak fixes in table cachingPlanned version bump
MINORNotes
This PR is ready for review. All 193 tests pass locally and in CI across all platforms (x86, x86_64, aarch64, embedded ARM, RISC-V, WASM).
The
stdfeature is enabled by default, so this change is completely transparent to existing users. New users can opt into no_std by specifyingdefault-features = falsein their Cargo.toml.Testing matrix expanded to include:
*** WASM64 works, but it's not stable in the nightly toolchain yet. Disabled in CI.
Feature combinations tested:
no_stdalone (minimal, no heap)no_std+alloc(with heap allocation)no_std+cache(with caching enabled)std+alloc+panic-handler)