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

Implement non-deterministc mode #683

Merged
merged 21 commits into from
Apr 10, 2019
Merged

Conversation

Aaron1011
Copy link
Member

Part of #653

This allows us to properly implement getrandom(),
which unlocks the default HashMap type (e.g. HashMap<K, V>)
with RandomState)

This commit adds a new '-Zmiri-seed=' option. When present,
this option takes a 64-bit hex value, which is used as the seed
to an internal PRNG. This PRNG is used to implement the 'getrandom()'
syscall.

When '-Zmiri-seed' is not passed, 'getrandom()' will be disabled.

Part of rust-lang#653

This allows us to properly implement getrandom(),
which unlocks the default HashMap type (e.g. HashMap<K, V>)
with RandomState)

This commit adds a new '-Zmiri-seed=<seed>' option. When present,
this option takes a 64-bit hex value, which is used as the seed
to an internal PRNG. This PRNG is used to implement the 'getrandom()'
syscall.

When '-Zmiri-seed' is not passed, 'getrandom()' will be disabled.
Copy link
Contributor

@oli-obk oli-obk left a comment

Choose a reason for hiding this comment

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

Just some nits. This is awesome!

src/fn_call.rs Show resolved Hide resolved
tests/compile-fail/getrandom.rs Outdated Show resolved Hide resolved
src/bin/miri.rs Outdated Show resolved Hide resolved
src/fn_call.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
tests/run-pass/hashmap.rs Outdated Show resolved Hide resolved
@RalfJung
Copy link
Member

RalfJung commented Apr 8, 2019

This is pretty cool!

To make the test suite pass with your changes to the HashMap tests, you'd have to also implement this for macOS and Windows.

Windows says

error[E0080]: constant evaluation error: can't call foreign function: SystemFunction036
 --> C:/Users/appveyor/.rustup/toolchains/master/lib/rustlib/src/rust/src/libstd/sys/windows/rand.rs:8:9
  |
8 | /         c::RtlGenRandom(&mut v as *mut _ as *mut u8,
9 | |                         mem::size_of_val(&v) as c::ULONG)
  | |_________________________________________________________^ can't call foreign function: SystemFunction036
  |
  = note: inside call to `std::sys::windows::rand::hashmap_random_keys` at C:/Users/appveyor/.rustup/toolchains/master/lib/rustlib/src/rust/src/libstd/collections/hash/map.rs:3181:23

and macOS says

error[E0080]: constant evaluation error: can't call foreign function: open
   --> /Users/travis/.rustup/toolchains/master/lib/rustlib/src/rust/src/libstd/sys/unix/fs.rs:467:13
    |
467 |             open64(path.as_ptr(), flags, opts.mode as c_int)
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ can't call foreign function: open
    |
    = note: inside call to closure at /Users/travis/.rustup/toolchains/master/lib/rustlib/src/rust/src/libstd/sys/unix/mod.rs:143:19

Alternatively, you could make the affected tests linux-only. Then we should open two issues to track the missing support for Windows and macOS.

@Aaron1011
Copy link
Member Author

I've implemented random number generation for Windows.

For now, I've left the test disabled for OS X. Implementing random number generation on OS X will be trickier, since we'll need to handle attempted reads from /dev/urandom (also, I don't have a Mac to test on).

@Aaron1011
Copy link
Member Author

@oli-obk @RalfJung I've made the changes you requested

let map_normal: HashMap<i32, i32> = HashMap::new();
test_map(map_normal);
} else {
test_map(_map);
Copy link
Member

Choose a reason for hiding this comment

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

Can you move the let _map into this conditional branch to avoid the unused variable on other platforms? Then you can also avoid the _ in the name.

Copy link
Member Author

Choose a reason for hiding this comment

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

I wanted to test creation of both types of maps on all platforms, even though only one has the full set of tests run on it.

Copy link
Member

Choose a reason for hiding this comment

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

TBH I don't see the point, this code just looks weird. Why test creation on both but not the rest of the test?

The purpose of this is mostly to test Miri, not to test the HashMap. For that, HashBrown actually runs its test suite in Miri. I don't see a good reason to use multiple different hash functions here.

Copy link
Member Author

Choose a reason for hiding this comment

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

My intention was to ensure that Miri's test coverage didn't regress (e.g. ensure that we catch any regressions with creating a HashMap with a custom hasher).

I'll clean it up.

src/fn_call.rs Outdated Show resolved Hide resolved
src/fn_call.rs Outdated Show resolved Hide resolved
@Aaron1011
Copy link
Member Author

@RalfJung: I've addressed your comments

src/fn_call.rs Outdated Show resolved Hide resolved
src/fn_call.rs Outdated Show resolved Hide resolved
src/fn_call.rs Outdated Show resolved Hide resolved
@oli-obk
Copy link
Contributor

oli-obk commented Apr 9, 2019

just some nits then this looks ready to go to me

@Aaron1011
Copy link
Member Author

@oli-obk Fixed

src/fn_call.rs Outdated Show resolved Hide resolved
RalfJung and others added 2 commits April 9, 2019 23:33
Co-Authored-By: Aaron1011 <aa1ronham@gmail.com>
@Aaron1011
Copy link
Member Author

@RalfJung Fixed

@RalfJung
Copy link
Member

Awesome, thanks for bearing with our nitpicking :)

@RalfJung RalfJung merged commit 2dc6e8b into rust-lang:master Apr 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants