Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cranelift: Bitwise compare fuzzgen results #4855

Merged
merged 1 commit into from
Sep 2, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions cranelift/codegen/src/data_value.rs
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,25 @@ impl DataValue {
ty,
)
}

/// Performs a bitwise comparison over the contents of [DataValue].
///
/// Returns true if all bits are equal.
///
/// This behaviour is different from PartialEq for NaN floats.
pub fn bitwise_eq(&self, other: &DataValue) -> bool {
match (self, other) {
// We need to bit compare the floats to ensure that we produce the correct values
// on NaN's. The test suite expects to assert the precise bit pattern on NaN's or
// works around it in the tests themselves.
(DataValue::F32(a), DataValue::F32(b)) => a.bits() == b.bits(),
(DataValue::F64(a), DataValue::F64(b)) => a.bits() == b.bits(),

// We don't need to worry about F32x4 / F64x2 Since we compare V128 which is already the
// raw bytes anyway
(a, b) => a == b,
}
}
}

/// Record failures to cast [DataValue].
Expand Down
19 changes: 5 additions & 14 deletions cranelift/reader/src/run_command.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,20 +54,11 @@ impl RunCommand {
actual: &Vec<DataValue>,
expected: &Vec<DataValue>,
) -> bool {
let are_equal = actual
.into_iter()
.zip(expected.into_iter())
.all(|(a, b)| match (a, b) {
// We need to bit compare the floats to ensure that we produce the correct values
// on NaN's. The test suite expects to assert the precise bit pattern on NaN's or
// works around it in the tests themselves.
(DataValue::F32(a), DataValue::F32(b)) => a.bits() == b.bits(),
(DataValue::F64(a), DataValue::F64(b)) => a.bits() == b.bits(),

// We don't need to worry about F32x4 / F64x2 Since we compare V128 which is already the
// raw bytes anyway
(a, b) => a == b,
});
let are_equal = actual.len() == expected.len()
&& actual
.into_iter()
.zip(expected.into_iter())
.all(|(a, b)| a.bitwise_eq(b));
Comment on lines +58 to +61
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For future reference, it's possible to write this more tersely. Iterator::zip will implicitly call into_iter() on whatever you pass to it, so you can just use .zip(expected). And the implementation of into_iter() on &Vec is the same as calling iter() (they both return references to the elements of the vector) so you can use the shorter name. When I was playing with this locally I found that those two changes meant this part of the expression would fit on one line with cargo fmts defaults. 😁

This is not remotely important, so I'm merging without that change and please don't bother writing another PR for it. This version is just fine as-is. But I thought I'd note the alternative.


match compare {
Comparison::Equals => are_equal,
Expand Down
13 changes: 7 additions & 6 deletions fuzz/fuzz_targets/cranelift-fuzzgen.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,12 @@ enum RunResult {
Error(Box<dyn std::error::Error>),
}

impl RunResult {
pub fn unwrap(self) -> Vec<DataValue> {
match self {
RunResult::Success(d) => d,
_ => panic!("Expected RunResult::Success in unwrap but got: {:?}", self),
impl PartialEq for RunResult {
fn eq(&self, other: &Self) -> bool {
if let (RunResult::Success(l), RunResult::Success(r)) = (self, other) {
l.len() == r.len() && l.iter().zip(r).all(|(l, r)| l.bitwise_eq(r))
} else {
false
}
}
}
Expand Down Expand Up @@ -123,6 +124,6 @@ fuzz_target!(|testcase: TestCase| {
_ => panic!("host failed: {:?}", host_res),
}

assert_eq!(int_res.unwrap(), host_res.unwrap());
assert_eq!(int_res, host_res);
}
});