Skip to content

Commit 6efdd56

Browse files
committed
WIP. Ensure miri passes on x86_64
`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.
1 parent c0337ca commit 6efdd56

File tree

6 files changed

+171
-108
lines changed

6 files changed

+171
-108
lines changed

src/arch/mod.rs

Lines changed: 11 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -113,13 +113,7 @@ pub(crate) unsafe fn update(state: u64, bytes: &[u8], params: CrcParams) -> u64
113113
32 => algorithm::update::<_, Width32>(state as u32, bytes, params, ops) as u64,
114114
_ => panic!("Unsupported CRC width: {}", params.width),
115115
},
116-
ArchOpsInstance::SoftwareFallback => {
117-
#[cfg(target_arch = "x86")]
118-
crate::arch::x86_software_update(state, bytes, params);
119-
120-
// This should never happen, but just in case
121-
panic!("x86 features missing (SSE4.1 && PCLMULQDQ)");
122-
}
116+
ArchOpsInstance::SoftwareFallback => crate::arch::software::update(state, bytes, params),
123117
}
124118
}
125119

@@ -139,27 +133,8 @@ pub(crate) unsafe fn update(state: u64, bytes: &[u8], params: CrcParams) -> u64
139133
32 => algorithm::update::<_, Width32>(state as u32, bytes, params, ops) as u64,
140134
_ => panic!("Unsupported CRC width: {}", params.width),
141135
},
142-
ArchOpsInstance::SoftwareFallback => x86_software_update(state, bytes, params),
143-
}
144-
}
145-
146-
#[inline(always)]
147-
#[allow(unused)]
148-
#[cfg(any(target_arch = "x86", target_arch = "x86_64"))]
149-
fn x86_software_update(state: u64, bytes: &[u8], params: CrcParams) -> u64 {
150-
if !is_x86_feature_detected!("sse4.1") || !is_x86_feature_detected!("pclmulqdq") {
151-
#[cfg(all(
152-
target_arch = "x86",
153-
any(not(target_feature = "sse4.1"), not(target_feature = "pclmulqdq"))
154-
))]
155-
{
156-
// Use software implementation when no SIMD support is available
157-
crate::arch::software::update(state, bytes, params);
158-
}
136+
ArchOpsInstance::SoftwareFallback => crate::arch::software::update(state, bytes, params),
159137
}
160-
161-
// This should never happen, but just in case
162-
panic!("x86 features missing (SSE4.1 && PCLMULQDQ)");
163138
}
164139

165140
#[inline]
@@ -347,7 +322,10 @@ mod tests {
347322
}
348323
}
349324

325+
/// Skipping for Miri runs due to time constraints, underlying code already covered by other
326+
/// tests.
350327
#[test]
328+
#[cfg_attr(miri, ignore)]
351329
fn test_small_lengths_all() {
352330
// Test each CRC-64 variant
353331
for config in TEST_ALL_CONFIGS {
@@ -358,7 +336,10 @@ mod tests {
358336
}
359337
}
360338

339+
/// Skipping for Miri runs due to time constraints, underlying code already covered by other
340+
/// tests.
361341
#[test]
342+
#[cfg_attr(miri, ignore)]
362343
fn test_medium_lengths() {
363344
// Test each CRC-64 variant
364345
for config in TEST_ALL_CONFIGS {
@@ -369,7 +350,10 @@ mod tests {
369350
}
370351
}
371352

353+
/// Skipping for Miri runs due to time constraints, underlying code already covered by other
354+
/// tests.
372355
#[test]
356+
#[cfg_attr(miri, ignore)]
373357
fn test_large_lengths() {
374358
// Test each CRC-64 variant
375359
for config in TEST_ALL_CONFIGS {

src/arch/software.rs

Lines changed: 42 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -1,29 +1,13 @@
11
// Copyright 2025 Don MacAskill. Licensed under MIT or Apache-2.0.
22

33
//! This module contains a software fallback for unsupported architectures.
4-
//!
5-
//! Software fallback is conditionally compiled based on target architecture:
6-
//! - Always included for non-SIMD architectures (not x86/x86_64/aarch64)
7-
//! - Included for x86 when SSE4.1/PCLMULQDQ may not be available
8-
//! - Included for aarch64 for runtime fallback when AES is not detected
9-
//! - Excluded for x86_64 since SSE4.1/PCLMULQDQ are always available (but included for testing)
10-
11-
#![cfg(any(
12-
// Non-aarch64/x86/x86_64 architectures always need software fallback
13-
not(any(target_arch = "x86", target_arch = "x86_64", target_arch = "aarch64")),
14-
// x86 may not have SSE4.1/PCLMULQDQ support
15-
all(target_arch = "x86", any(not(target_feature = "sse4.1"), not(target_feature = "pclmulqdq"))),
16-
// aarch64 needs software fallback for runtime detection when AES is not available...
17-
// NEON doesn't guarantee AES, so for rare outlier CPUs this might not work 100%...
18-
all(target_arch = "aarch64", not(target_feature = "aes")),
19-
// Include for testing on all architectures
20-
test
21-
))]
224
235
use crate::consts::CRC_64_NVME;
246
use crate::CrcAlgorithm;
257
use crate::CrcParams;
268
use crc::{Algorithm, Table};
9+
use std::collections::HashMap;
10+
use std::sync::{Mutex, OnceLock};
2711

2812
#[allow(unused)]
2913
const RUST_CRC32_AIXM: crc::Crc<u32, Table<16>> =
@@ -96,6 +80,9 @@ const RUST_CRC64_WE: crc::Crc<u64, Table<16>> = crc::Crc::<u64, Table<16>>::new(
9680
#[allow(unused)]
9781
const RUST_CRC64_XZ: crc::Crc<u64, Table<16>> = crc::Crc::<u64, Table<16>>::new(&crc::CRC_64_XZ);
9882

83+
static CUSTOM_CRC32_CACHE: OnceLock<Mutex<HashMap<u32, &'static Algorithm<u32>>>> = OnceLock::new();
84+
static CUSTOM_CRC64_CACHE: OnceLock<Mutex<HashMap<u64, &'static Algorithm<u64>>>> = OnceLock::new();
85+
9986
#[allow(unused)]
10087
// Dispatch function that handles the generic case
10188
pub(crate) fn update(state: u64, data: &[u8], params: CrcParams) -> u64 {
@@ -115,19 +102,25 @@ pub(crate) fn update(state: u64, data: &[u8], params: CrcParams) -> u64 {
115102
CrcAlgorithm::Crc32Mpeg2 => RUST_CRC32_MPEG_2,
116103
CrcAlgorithm::Crc32Xfer => RUST_CRC32_XFER,
117104
CrcAlgorithm::Crc32Custom => {
118-
let algorithm: Algorithm<u32> = Algorithm {
119-
width: params.width,
120-
poly: params.poly as u32,
121-
init: params.init as u32,
122-
refin: params.refin,
123-
refout: params.refout,
124-
xorout: params.xorout as u32,
125-
check: params.check as u32,
126-
residue: 0x00000000, // unused in this context
127-
};
128-
129-
// ugly, but the crc crate is difficult to work with...
130-
let static_algorithm = Box::leak(Box::new(algorithm));
105+
let cache = CUSTOM_CRC32_CACHE.get_or_init(|| Mutex::new(HashMap::new()));
106+
let mut cache = cache.lock().unwrap();
107+
108+
// Create a key from params that uniquely identifies this algorithm
109+
let key = params.poly as u32;
110+
111+
let static_algorithm = cache.entry(key).or_insert_with(|| {
112+
let algorithm = Algorithm {
113+
width: params.width,
114+
poly: params.poly as u32,
115+
init: params.init as u32,
116+
refin: params.refin,
117+
refout: params.refout,
118+
xorout: params.xorout as u32,
119+
check: params.check as u32,
120+
residue: 0x00000000,
121+
};
122+
Box::leak(Box::new(algorithm))
123+
});
131124

132125
crc::Crc::<u32, Table<16>>::new(static_algorithm)
133126
}
@@ -145,19 +138,24 @@ pub(crate) fn update(state: u64, data: &[u8], params: CrcParams) -> u64 {
145138
CrcAlgorithm::Crc64We => RUST_CRC64_WE,
146139
CrcAlgorithm::Crc64Xz => RUST_CRC64_XZ,
147140
CrcAlgorithm::Crc64Custom => {
148-
let algorithm: Algorithm<u64> = Algorithm {
149-
width: params.width,
150-
poly: params.poly,
151-
init: params.init,
152-
refin: params.refin,
153-
refout: params.refout,
154-
xorout: params.xorout,
155-
check: params.check,
156-
residue: 0x0000000000000000, // unused in this context
157-
};
158-
159-
// ugly, but the crc crate is difficult to work with...
160-
let static_algorithm = Box::leak(Box::new(algorithm));
141+
let cache = CUSTOM_CRC64_CACHE.get_or_init(|| Mutex::new(HashMap::new()));
142+
let mut cache = cache.lock().unwrap();
143+
144+
let key = params.poly;
145+
146+
let static_algorithm = cache.entry(key).or_insert_with(|| {
147+
let algorithm = Algorithm {
148+
width: params.width,
149+
poly: params.poly,
150+
init: params.init,
151+
refin: params.refin,
152+
refout: params.refout,
153+
xorout: params.xorout,
154+
check: params.check,
155+
residue: 0x0000000000000000,
156+
};
157+
Box::leak(Box::new(algorithm))
158+
});
161159

162160
crc::Crc::<u64, Table<16>>::new(static_algorithm)
163161
}

src/cache.rs

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1029,14 +1029,27 @@ mod tests {
10291029
);
10301030
}
10311031

1032+
/// Non-stress version of the stress test to allow Miri to evaluate without timing out.
10321033
#[test]
1034+
fn test_cache_memory_allocation_non_stress() {
1035+
cache_memory_allocation_stress(2);
1036+
}
1037+
1038+
/// Skipping for Miri runs due to time constraints, underlying code already covered by other
1039+
/// tests.
1040+
#[test]
1041+
#[cfg_attr(miri, ignore)]
10331042
fn test_cache_memory_allocation_stress() {
1043+
cache_memory_allocation_stress(1000);
1044+
}
1045+
1046+
fn cache_memory_allocation_stress(count: i32) {
10341047
clear_cache();
10351048

10361049
// Test cache behavior under memory allocation stress
10371050
// Create a large number of unique cache entries to stress memory allocation
10381051
let mut created_entries = Vec::new();
1039-
let stress_count = 1000;
1052+
let stress_count = count;
10401053

10411054
// Create many unique cache entries
10421055
for i in 0..stress_count {

src/crc32/fusion/x86/mod.rs

Lines changed: 31 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -199,32 +199,40 @@ mod tests {
199199

200200
#[test]
201201
fn test_crc32_iscsi_check() {
202-
assert_eq!(
203-
crc32_iscsi(0xffffffff, TEST_CHECK_STRING) ^ 0xffffffff,
204-
0xe3069283
205-
);
202+
if is_x86_feature_detected!("sse4.2") && is_x86_feature_detected!("pclmulqdq") {
203+
assert_eq!(
204+
crc32_iscsi(0xffffffff, TEST_CHECK_STRING) ^ 0xffffffff,
205+
0xe3069283
206+
);
207+
}
206208
}
207209

208210
#[test]
209211
fn test_crc32_iscsi_small_all_lengths() {
210-
for len in 1..=255 {
211-
test_crc32_iscsi_random(len);
212+
if is_x86_feature_detected!("sse4.2") && is_x86_feature_detected!("pclmulqdq") {
213+
for len in 1..=255 {
214+
test_crc32_iscsi_random(len);
215+
}
212216
}
213217
}
214218

215219
#[test]
216220
fn test_crc32_iscsi_medium_lengths() {
217-
// Test each length from 256 to 1024, which should fold and include handling remainders
218-
for len in 256..=1024 {
219-
test_crc32_iscsi_random(len);
221+
if is_x86_feature_detected!("sse4.2") && is_x86_feature_detected!("pclmulqdq") {
222+
// Test each length from 256 to 1024, which should fold and include handling remainders
223+
for len in 256..=1024 {
224+
test_crc32_iscsi_random(len);
225+
}
220226
}
221227
}
222228

223229
#[test]
224230
fn test_crc32_iscsi_large_lengths() {
225-
// Test 1 MiB just before, at, and just after the folding boundaries
226-
for len in 1048575..1048577 {
227-
test_crc32_iscsi_random(len);
231+
if is_x86_feature_detected!("sse4.2") && is_x86_feature_detected!("pclmulqdq") {
232+
// Test 1 MiB just before, at, and just after the folding boundaries
233+
for len in 1048575..1048577 {
234+
test_crc32_iscsi_random(len);
235+
}
228236
}
229237
}
230238

@@ -235,7 +243,9 @@ mod tests {
235243

236244
let checksum = RUST_CRC32_ISCSI.checksum(&data);
237245

238-
assert_eq!(crc32_iscsi(0xffffffff, &data) ^ 0xffffffff, checksum);
246+
if is_x86_feature_detected!("sse4.2") && is_x86_feature_detected!("pclmulqdq") {
247+
assert_eq!(crc32_iscsi(0xffffffff, &data) ^ 0xffffffff, checksum);
248+
}
239249

240250
unsafe {
241251
#[cfg(target_arch = "x86_64")]
@@ -277,13 +287,15 @@ mod tests {
277287

278288
let checksum = RUST_CRC32_ISCSI.checksum(&data);
279289

280-
assert_eq!(crc32_iscsi(0xffffffff, &data) ^ 0xffffffff, checksum);
290+
if is_x86_feature_detected!("sse4.2") && is_x86_feature_detected!("pclmulqdq") {
291+
assert_eq!(crc32_iscsi(0xffffffff, &data) ^ 0xffffffff, checksum);
281292

282-
unsafe {
283-
assert_eq!(
284-
crc32_iscsi_sse_v4s3x3(0xffffffff, data.as_ptr(), data.len()) ^ 0xffffffff,
285-
checksum
286-
);
293+
unsafe {
294+
assert_eq!(
295+
crc32_iscsi_sse_v4s3x3(0xffffffff, data.as_ptr(), data.len()) ^ 0xffffffff,
296+
checksum
297+
);
298+
}
287299
}
288300
}
289301
}

src/ffi.rs

Lines changed: 22 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -11,11 +11,13 @@ use crate::CrcAlgorithm;
1111
use crate::CrcParams;
1212
use crate::{get_calculator_target, Digest};
1313
use std::collections::HashMap;
14+
use std::collections::HashSet;
1415
use std::ffi::CStr;
1516
use std::os::raw::c_char;
1617
use std::slice;
17-
use std::sync::Mutex;
18-
use std::sync::OnceLock;
18+
use std::sync::{Mutex, OnceLock};
19+
20+
static STRING_CACHE: OnceLock<Mutex<HashSet<&'static str>>> = OnceLock::new();
1921

2022
// Global storage for stable key pointers to ensure they remain valid across FFI boundary
2123
static STABLE_KEY_STORAGE: OnceLock<Mutex<HashMap<u64, Box<[u64]>>>> = OnceLock::new();
@@ -55,11 +57,12 @@ fn create_stable_key_pointer(keys: &crate::CrcKeysStorage) -> (*const u64, u32)
5557
};
5658

5759
let boxed_keys = key_vec.into_boxed_slice();
58-
let ptr = boxed_keys.as_ptr();
5960
let count = boxed_keys.len() as u32;
6061

6162
storage_map.insert(key_hash, boxed_keys);
6263

64+
let ptr = storage_map.get(&key_hash).expect("just inserted").as_ptr();
65+
6366
(ptr, count)
6467
}
6568

@@ -473,8 +476,7 @@ pub extern "C" fn crc_fast_get_custom_params(
473476

474477
// Get the custom params from the library
475478
let params = CrcParams::new(
476-
// We need to use a static string for the name field
477-
Box::leak(name.to_string().into_boxed_str()),
479+
get_or_leak_string(name), // ✅ Use cached leak
478480
width,
479481
poly,
480482
init,
@@ -536,3 +538,18 @@ unsafe fn convert_to_string(data: *const u8, len: usize) -> String {
536538
Err(_) => panic!("Invalid UTF-8 string"),
537539
}
538540
}
541+
542+
fn get_or_leak_string(s: &str) -> &'static str {
543+
let cache = STRING_CACHE.get_or_init(|| Mutex::new(HashSet::new()));
544+
let mut cache = cache.lock().unwrap();
545+
546+
// Check if we already have this string
547+
if let Some(&cached) = cache.get(s) {
548+
return cached;
549+
}
550+
551+
// Leak it and cache the result
552+
let leaked: &'static str = Box::leak(s.to_string().into_boxed_str());
553+
cache.insert(leaked);
554+
leaked
555+
}

0 commit comments

Comments
 (0)