Skip to content

Commit

Permalink
switch the instruction used for acle crc32 calculation
Browse files Browse the repository at this point in the history 
this is to match the current version of zlib-ng that we're trying to be compatible with
  • Loading branch information
@folkertdev
folkertdev committed Aug 16, 2024
1 parent b7bf6ac commit e66e83c
Show file tree
Hide file tree
Showing 4 changed files with 97 additions and 35 deletions.
57 changes: 51 additions & 6 deletions test-libz-rs-sys/src/deflate.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1984,7 +1984,7 @@ mod fuzz_based_tests {

#[test]
#[cfg_attr(
not(target_arch = "x86_64"),
not(any(target_arch = "x86_64", target_arch = "aarch64")),
ignore = "https://github.com/memorysafety/zlib-rs/issues/91"
)]
#[cfg_attr(miri, ignore = "too slow")]
Expand All @@ -1998,11 +1998,7 @@ mod fuzz_based_tests {
}

#[test]
#[cfg_attr(
target_arch = "aarch64",
ignore = "https://github.com/memorysafety/zlib-rs/issues/91"
)]
#[cfg_attr(miri, ignore)]
#[cfg_attr(miri, ignore = "too slow")]
fn compress_fireworks() {
let mut config = DeflateConfig::default();

Expand Down Expand Up @@ -2177,4 +2173,53 @@ mod fuzz_based_tests {
],
)
}

#[test]
fn hash_calc_difference() {
// exposed an issue in the crc32 acle hash calc where the incorrect instruction was used.

// zlib-ng uses DFLTCC to perform the hash calc, giving different results; we don't support that
let output_s390x = &[
24, 149, 99, 96, 102, 24, 104, 160, 7, 38, 57, 96, 92, 117, 6, 14, 6, 38, 60, 202, 65,
14, 86, 99, 208, 3, 3, 6, 67, 6, 38, 22, 58, 56, 17, 10, 40, 119, 41, 84, 175, 26, 0,
172, 56, 3, 232,
];

let output_other = &[
24, 149, 99, 96, 102, 24, 104, 160, 7, 38, 57, 96, 92, 117, 6, 14, 6, 38, 60, 202, 65,
14, 86, 99, 208, 3, 3, 6, 67, 6, 38, 22, 122, 184, 17, 2, 40, 119, 41, 84, 175, 26, 0,
172, 56, 3, 232,
];

fuzz_based_test(
&[
0, 3, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 46, 0, 0, 0, 0, 0, 8, 0,
0, 0, 0, 0, 0, 0, 0, 39, 0, 8, 0, 2, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 3, 0, 0, 0, 38, 0, 46, 46, 46, 46, 46, 46, 0,
49, 0, 2, 4, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
46, 0, 0, 0, 0, 0, 8, 0, 0, 0, 0, 0, 0, 0, 0, 39, 0, 8, 0, 2, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 3, 0, 0, 0, 38, 0, 46,
46, 46, 46, 46, 46, 0, 49, 0, 2, 4, 0, 0, 8, 0, 0, 0, 0, 0, 0, 38,
],
DeflateConfig {
level: -1,
method: Method::Deflated,
window_bits: 8,
mem_level: 2,
strategy: Strategy::Default,
},
if cfg!(target_arch = "s390x") {
output_s390x
} else {
output_other
},
)
}
}
2 changes: 1 addition & 1 deletion zlib-rs/src/crc32/acle.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ unsafe fn __crc32h(mut crc: u32, data: u16) -> u32 {
/// [Arm's documentation](https://developer.arm.com/architectures/instruction-sets/intrinsics/__crc32w)
#[target_feature(enable = "crc")]
#[cfg_attr(target_arch = "arm", target_feature(enable = "v8"))]
unsafe fn __crc32w(mut crc: u32, data: u32) -> u32 {
pub unsafe fn __crc32w(mut crc: u32, data: u32) -> u32 {
core::arch::asm!("crc32w {crc:w}, {crc:w}, {data:w}", crc = inout(reg) crc, data = in(reg) data);
crc
}
Expand Down
8 changes: 7 additions & 1 deletion zlib-rs/src/deflate.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4066,7 +4066,13 @@ mod test {
// the output is slightly different based on what hashing algorithm is used
match HashCalcVariant::for_compression_level(config.level as usize) {
HashCalcVariant::Crc32 => {
fuzz_based_test(&input, config, &crc32);
// the aarch64 hashing algorithm is different from the standard algorithm, but in
// this case they turn out to give the same output. Beware!
if cfg!(target_arch = "x86") || cfg!(target_arch = "x86_64") {
fuzz_based_test(&input, config, &crc32);
} else {
fuzz_based_test(&input, config, &other);
}
}
HashCalcVariant::Standard | HashCalcVariant::Roll => {
fuzz_based_test(&input, config, &other);
Expand Down
65 changes: 38 additions & 27 deletions zlib-rs/src/deflate/hash_calc.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,14 @@ pub enum HashCalcVariant {

impl HashCalcVariant {
#[cfg(test)]
pub const fn for_compression_level(level: usize) -> Self {
pub fn for_compression_level(level: usize) -> Self {
let max_chain_length = crate::deflate::algorithm::CONFIGURATION_TABLE[level].max_chain;
Self::for_max_chain_length(max_chain_length as usize)
}

/// Use rolling hash for deflate_slow algorithm with level 9. It allows us to
/// properly lookup different hash chains to speed up longest_match search.
pub const fn for_max_chain_length(max_chain_length: usize) -> Self {
pub fn for_max_chain_length(max_chain_length: usize) -> Self {
if max_chain_length > 1024 {
HashCalcVariant::Roll
} else if Crc32HashCalc::is_supported() {
Expand Down Expand Up @@ -136,15 +136,14 @@ impl HashCalc for RollHashCalc {
pub struct Crc32HashCalc;

impl Crc32HashCalc {
pub const fn is_supported() -> bool {
pub fn is_supported() -> bool {
if cfg!(target_arch = "x86") || cfg!(target_arch = "x86_64") {
return true;
}

// zlib-ng no longer special-cases on aarch64
#[cfg(all(target_arch = "aarch64", feature = "std"))]
// return std::arch::is_aarch64_feature_detected!("crc");
return false;
return std::arch::is_aarch64_feature_detected!("crc");

#[allow(unreachable_code)]
false
Expand All @@ -168,7 +167,7 @@ impl HashCalc for Crc32HashCalc {

#[cfg(target_arch = "aarch64")]
fn hash_calc(h: u32, val: u32) -> u32 {
unsafe { crate::crc32::acle::__crc32cw(h, val) }
unsafe { crate::crc32::acle::__crc32w(h, val) }
}

#[cfg(not(any(target_arch = "x86", target_arch = "x86_64", target_arch = "aarch64")))]
Expand All @@ -188,26 +187,34 @@ mod tests {
ignore = "no crc32 hardware support on this platform"
)]
fn crc32_hash_calc() {
assert_eq!(Crc32HashCalc::hash_calc(0, 807411760), 2423125009);
assert_eq!(Crc32HashCalc::hash_calc(0, 540024864), 1452438466);
assert_eq!(Crc32HashCalc::hash_calc(0, 538980384), 435552201);
assert_eq!(Crc32HashCalc::hash_calc(0, 807411760), 2423125009);
assert_eq!(Crc32HashCalc::hash_calc(0, 540024864), 1452438466);
assert_eq!(Crc32HashCalc::hash_calc(0, 538980384), 435552201);
assert_eq!(Crc32HashCalc::hash_calc(0, 807411760), 2423125009);
assert_eq!(Crc32HashCalc::hash_calc(0, 540024864), 1452438466);
assert_eq!(Crc32HashCalc::hash_calc(0, 538980384), 435552201);
assert_eq!(Crc32HashCalc::hash_calc(0, 807411760), 2423125009);
assert_eq!(Crc32HashCalc::hash_calc(0, 540024864), 1452438466);
assert_eq!(Crc32HashCalc::hash_calc(0, 538980384), 435552201);
assert_eq!(Crc32HashCalc::hash_calc(0, 807411760), 2423125009);
assert_eq!(Crc32HashCalc::hash_calc(0, 170926112), 500028708);
assert_eq!(Crc32HashCalc::hash_calc(0, 537538592), 3694129053);
assert_eq!(Crc32HashCalc::hash_calc(0, 538970672), 373925026);
assert_eq!(Crc32HashCalc::hash_calc(0, 538976266), 4149335727);
assert_eq!(Crc32HashCalc::hash_calc(0, 538976288), 1767342659);
assert_eq!(Crc32HashCalc::hash_calc(0, 941629472), 4090502627);
assert_eq!(Crc32HashCalc::hash_calc(0, 775430176), 1744703325);
if cfg!(target_arch = "x86") || cfg!(target_arch = "x86_64") {
assert_eq!(Crc32HashCalc::hash_calc(0, 807411760), 2423125009);
assert_eq!(Crc32HashCalc::hash_calc(0, 540024864), 1452438466);
assert_eq!(Crc32HashCalc::hash_calc(0, 538980384), 435552201);
assert_eq!(Crc32HashCalc::hash_calc(0, 807411760), 2423125009);
assert_eq!(Crc32HashCalc::hash_calc(0, 540024864), 1452438466);
assert_eq!(Crc32HashCalc::hash_calc(0, 538980384), 435552201);
assert_eq!(Crc32HashCalc::hash_calc(0, 807411760), 2423125009);
assert_eq!(Crc32HashCalc::hash_calc(0, 540024864), 1452438466);
assert_eq!(Crc32HashCalc::hash_calc(0, 538980384), 435552201);
assert_eq!(Crc32HashCalc::hash_calc(0, 807411760), 2423125009);
assert_eq!(Crc32HashCalc::hash_calc(0, 540024864), 1452438466);
assert_eq!(Crc32HashCalc::hash_calc(0, 538980384), 435552201);
assert_eq!(Crc32HashCalc::hash_calc(0, 807411760), 2423125009);
assert_eq!(Crc32HashCalc::hash_calc(0, 170926112), 500028708);
assert_eq!(Crc32HashCalc::hash_calc(0, 537538592), 3694129053);
assert_eq!(Crc32HashCalc::hash_calc(0, 538970672), 373925026);
assert_eq!(Crc32HashCalc::hash_calc(0, 538976266), 4149335727);
assert_eq!(Crc32HashCalc::hash_calc(0, 538976288), 1767342659);
assert_eq!(Crc32HashCalc::hash_calc(0, 941629472), 4090502627);
assert_eq!(Crc32HashCalc::hash_calc(0, 775430176), 1744703325);
} else {
assert_eq!(Crc32HashCalc::hash_calc(0, 807411760), 2067507791);
assert_eq!(Crc32HashCalc::hash_calc(0, 540024864), 2086141925);
assert_eq!(Crc32HashCalc::hash_calc(0, 538980384), 716394180);
assert_eq!(Crc32HashCalc::hash_calc(0, 775430176), 1396070634);
assert_eq!(Crc32HashCalc::hash_calc(0, 941629472), 637105634);
}
}

#[test]
Expand All @@ -228,6 +235,10 @@ mod tests {

#[test]
fn standard_hash_calc() {
assert_eq!(StandardHashCalc::hash_calc(0, 721420288), 47872);
assert_eq!(StandardHashCalc::hash_calc(0, 807411760), 65468);
assert_eq!(StandardHashCalc::hash_calc(0, 540024864), 42837);
assert_eq!(StandardHashCalc::hash_calc(0, 538980384), 33760);
assert_eq!(StandardHashCalc::hash_calc(0, 775430176), 8925);
assert_eq!(StandardHashCalc::hash_calc(0, 941629472), 42053);
}
}

0 comments on commit e66e83c

Please sign in to comment.