Ensure Miri passes on x86_64 and x86
#26
Conversation
`miri` detected a variety of issues on x86_64. This commit resolves them. It also excludes a few tests, such as those that use large data sets but have full underlying code coverage in other smaller tests, and tests that have isolation issues, such as file I/O, but also have code coverage in other tests. It doesn’t include `aarch64` support, since `miri` seems to be missing support for some required intrinsics, so that work is still TBD. All 103 supported tests pass.
There was a problem hiding this comment.
Pull Request Overview
This pull request implements runtime CPU feature detection and adds optimizations for memory management and test coverage. The main goal is to make the library more robust by detecting CPU capabilities at runtime rather than compile-time, and to add proper Miri test support.
- Runtime feature detection for AArch64 and x86 architectures to gracefully fallback when required CPU features are unavailable
- Memory optimization through caching mechanisms for custom CRC algorithms and FFI string allocations
- Enhanced test coverage with Miri support by adding skip annotations for time-intensive tests and creating non-stress variants
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/lib.rs | Adds runtime CPU feature detection for CRC32 ISCSI and ISO-HDLC calculators, plus Miri test annotations |
| src/ffi.rs | Implements string caching to reduce memory leaks and fixes stable key pointer handling |
| src/crc32/fusion/x86/mod.rs | Adds runtime feature detection guards to x86 fusion tests |
| src/cache.rs | Refactors stress test to support Miri execution with a lighter non-stress variant |
| src/arch/software.rs | Implements caching for custom CRC algorithm objects and removes conditional compilation |
| src/arch/mod.rs | Simplifies software fallback logic and adds Miri test annotations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Use all features of the CRC definition.
onethumb
changed the title
Miri passes on x86_64 and x86Miri passes on x86_64 and x86
|
Hey, I'm going to work through the no-std update this week and I'll take a look at the Miri issue. Interestingly, I run Miri extensively in the crate using this lib - and all my tests (over 600 plus mutant kills) are passing under Miri. I run it across Linux/Windows x86-64 and ARM64, plus the Linux musl static builds and MacOS without any issue. I've also been running the sanitizers with no issues. Finally, I've run the Kani-Verifier over the pieces I thought were sticky and didn't get a hit. Let me know if you have any leads for me to look into while I update for no-std compat, or anything you're hoping to see with respect to it! |
|
Yeah, there weren't very many issues Miri surfaced, anyway, but those that it did were almost entirely (maybe entirely?) in this crate's local test suite, so your crate wouldn't have even seen them. I'll bet that's what's happening. I do wish I knew how to get it to work against |
Miri won’t allow Command::new() due to isolation restrictions.
|
Miri is now part of the |
|
Hey, give me a few days. I'm going to knock the Miri investigation down and no-std compat down. Talk soon! |
| use std::process::Command; | ||
|
|
||
| #[test] | ||
| #[cfg_attr(miri, ignore)] // Miri doesn't allow this due to isolation restrictions |
There was a problem hiding this comment.
You can set MIRIFLAGS=-Zmiri-disable-isolation if you want to run such tests anyway.
| #[cfg(target_arch = "aarch64")] | ||
| { | ||
| use std::arch::is_aarch64_feature_detected; | ||
| if is_aarch64_feature_detected!("aes") && is_aarch64_feature_detected!("crc") { |
There was a problem hiding this comment.
As far as I can tell, this is the meat of the change: Adding runtime checks for CPU features before calling into the specialized routines. Looks good.
|
I'm pretty sure I found and fixed the only UB that existed. I'll push the PR today w/ no_std support and full wasm/no_std testing structure. I THINK the only UB was in algorithm.rs where the DataRegion was trying to read from memory before the slice. I THINK. I'm getting clean Miri; CI is passing. Hopefully, this was the issue. PR Incoming... |
3 participants
The Problem
Miri detected a variety of issues on
x86_64(andx86), such as #19The Solution
Fix each of the issues on
x86_64andx86. Does not addressaarch64(see Notes, below).Changes
Undefined Behaviorissues such as Miri detected possible UB with CPU feature flags for x86 #19.Stacked Borrows, as well.Planned version bump
PATCHLinks
TODO
Mirito the automated test & release workflows.Notes
Mirionaarch64platforms, it looks like it doesn't support some critical intrinsics (the error messages suggest these aren't bugs, but Miri limitations). This confuses me, since Miri claims to support all of Rust's Tier 1 targets, andPMULL2support stabilized in1.70.0. I'll keep investigating, but if anyone knows how I can addaarch64support, I'd love to. Thanks!