-
Notifications
You must be signed in to change notification settings - Fork 29
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
NonZero (rotate_left & rotate_right) Proof #202
base: main
Are you sure you want to change the base?
NonZero (rotate_left & rotate_right) Proof #202
Conversation
library/core/src/num/nonzero.rs
Outdated
mod macro_nonzero_check_rotate_right { | ||
use super::*; | ||
macro_rules! nonzero_check_rotate_right { | ||
($t:ty, $nonzero_type:ty, $nonzero_check_rotate_right_for:ident) => { | ||
#[kani::proof] | ||
pub fn $nonzero_check_rotate_right_for() { | ||
let int_x: $t = kani::any(); | ||
let n: u32 = kani::any(); | ||
kani::assume(int_x != 0); // x must be non-zero | ||
|
||
// Ensure that n is within a valid range for rotating | ||
kani::assume(n < (core::mem::size_of::<$t>() as u32 * 8)); | ||
kani::assume(n >= 0); | ||
|
||
unsafe { | ||
let x = <$nonzero_type>::new_unchecked(int_x); | ||
|
||
// Perform rotate_right | ||
let result = x.rotate_right(n); | ||
|
||
// Ensure the result is still non-zero | ||
assert!(result.get() != 0); | ||
} | ||
|
||
|
||
} | ||
}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you perhaps just use a single macro with one extra parameter to cover both rotate_left
and rotate_right
? Or, possibly even simpler: just make the macro call both as all requirements are the same for both functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have merge the 2 rotate function into 1 macro
library/core/src/num/nonzero.rs
Outdated
kani::assume(int_x != 0); // x must be non-zero | ||
|
||
// Ensure that n is within a valid range for rotating | ||
// kani::assume(n < (std::mem::size_of::<$t>() as u32 * 8)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this commented out (yet still left in here)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed this comment
library/core/src/num/nonzero.rs
Outdated
// kani::assume(n < (std::mem::size_of::<$t>() as u32 * 8)); | ||
// need to use core::mem instead of std::mem | ||
kani::assume(n < (core::mem::size_of::<$t>() as u32 * 8)); | ||
kani::assume(n >= 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is trivially true given n
is unsigned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed this assume
library/core/src/num/nonzero.rs
Outdated
// Ensure that n is within a valid range for rotating | ||
// kani::assume(n < (std::mem::size_of::<$t>() as u32 * 8)); | ||
// need to use core::mem instead of std::mem | ||
kani::assume(n < (core::mem::size_of::<$t>() as u32 * 8)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this assumption required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After talked with Carolyn, I understand that there's no need to have assume here. Already removed it
library/core/src/num/nonzero.rs
Outdated
// Ensure the result is still non-zero | ||
assert!(result.get() != 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about asserting that rotate_left(rotate_right(x)) == x?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have merged rotated_right and rotate_left with this logic
Working on #71 (Safety of NonZero)
We are looking for feedback on our proof for rotate_left & rotate_right.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses.