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

Miri: enable preemption again #855

Merged
merged 11 commits into from
Jul 1, 2022
33 changes: 18 additions & 15 deletions ci/miri.sh
Original file line number Diff line number Diff line change
Expand Up @@ -3,37 +3,40 @@ set -euxo pipefail
IFS=$'\n\t'
cd "$(dirname "$0")"/..

# We need 'ts' for the per-line timing
sudo apt-get -y install moreutils
echo
Comment on lines +6 to +8
Copy link
Member

Choose a reason for hiding this comment

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

FWIW, passing -- -Z unstable-options --report-time to cargo also works. However, I'm not sure if this is better than ts, as it seems there is no way to color the report in CI.

report-time

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I didn't know about this flag.
However (a) it only works with isolation disabled (since for regular tests it actually measures the time inside Miri), (b) it is harder to read without colors and libtest output in Miri is never colored for $REASONS, (c) the colors might not be appropriate for Miri; 1.3s seems still pretty good to me. ;)

Up to you, I am fine with removing the ts thing again. You have isolation disabled anyway.


export RUSTFLAGS="${RUSTFLAGS:-} -Z randomize-layout"

# disable preemption due to https://github.com/rust-lang/rust/issues/55005
MIRIFLAGS="-Zmiri-strict-provenance -Zmiri-symbolic-alignment-check -Zmiri-disable-isolation -Zmiri-preemption-rate=0" \
# -Zmiri-ignore-leaks is needed because we use detached threads in tests/docs: https://github.com/rust-lang/miri/issues/1371
MIRIFLAGS="-Zmiri-strict-provenance -Zmiri-symbolic-alignment-check -Zmiri-disable-isolation -Zmiri-ignore-leaks" \
cargo miri test \
-p crossbeam-queue \
-p crossbeam-utils
-p crossbeam-utils 2>&1 | ts -i '%.s '

# -Zmiri-ignore-leaks is needed because we use detached threads in tests/docs: https://github.com/rust-lang/miri/issues/1371
# disable preemption due to https://github.com/rust-lang/rust/issues/55005
MIRIFLAGS="-Zmiri-strict-provenance -Zmiri-symbolic-alignment-check -Zmiri-disable-isolation -Zmiri-ignore-leaks -Zmiri-preemption-rate=0" \
MIRIFLAGS="-Zmiri-strict-provenance -Zmiri-symbolic-alignment-check -Zmiri-disable-isolation -Zmiri-ignore-leaks" \
cargo miri test \
-p crossbeam-channel
-p crossbeam-channel 2>&1 | ts -i '%.s '

# -Zmiri-ignore-leaks is needed for https://github.com/crossbeam-rs/crossbeam/issues/579
# -Zmiri-disable-stacked-borrows is needed for https://github.com/crossbeam-rs/crossbeam/issues/545
# disable preemption due to https://github.com/rust-lang/rust/issues/55005
MIRIFLAGS="-Zmiri-check-number-validity -Zmiri-symbolic-alignment-check -Zmiri-disable-isolation -Zmiri-disable-stacked-borrows -Zmiri-ignore-leaks -Zmiri-preemption-rate=0" \
MIRIFLAGS="-Zmiri-symbolic-alignment-check -Zmiri-disable-isolation -Zmiri-disable-stacked-borrows -Zmiri-ignore-leaks" \
cargo miri test \
-p crossbeam-epoch \
-p crossbeam-skiplist
-p crossbeam-skiplist 2>&1 | ts -i '%.s '

# -Zmiri-ignore-leaks is needed for https://github.com/crossbeam-rs/crossbeam/issues/579
# -Zmiri-disable-stacked-borrows is needed for https://github.com/crossbeam-rs/crossbeam/issues/545
# disable preemption due to https://github.com/rust-lang/rust/issues/55005
MIRIFLAGS="-Zmiri-check-number-validity -Zmiri-symbolic-alignment-check -Zmiri-disable-stacked-borrows -Zmiri-ignore-leaks -Zmiri-compare-exchange-weak-failure-rate=0.0 -Zmiri-preemption-rate=0" \
# -Zmiri-compare-exchange-weak-failure-rate=0.0 is needed because some sequential tests (e.g.,
# doctest of Stealer::steal) incorrectly assume that sequential weak CAS will never fail.
# -Zmiri-preemption-rate=0 is needed because this code technically has UB and Miri catches that.
MIRIFLAGS="-Zmiri-symbolic-alignment-check -Zmiri-disable-stacked-borrows -Zmiri-ignore-leaks -Zmiri-compare-exchange-weak-failure-rate=0.0 -Zmiri-preemption-rate=0" \
cargo miri test \
-p crossbeam-deque
-p crossbeam-deque 2>&1 | ts -i '%.s '

# -Zmiri-ignore-leaks is needed for https://github.com/crossbeam-rs/crossbeam/issues/579
# disable preemption due to https://github.com/rust-lang/rust/issues/55005
MIRIFLAGS="-Zmiri-check-number-validity -Zmiri-symbolic-alignment-check -Zmiri-ignore-leaks -Zmiri-preemption-rate=0" \
MIRIFLAGS="-Zmiri-symbolic-alignment-check -Zmiri-ignore-leaks" \
cargo miri test \
-p crossbeam
-p crossbeam 2>&1 | ts -i '%.s '
4 changes: 0 additions & 4 deletions crossbeam-channel/tests/array.rs
Original file line number Diff line number Diff line change
Expand Up @@ -532,16 +532,12 @@ fn drops() {
scope.spawn(|_| {
for _ in 0..steps {
r.recv().unwrap();
#[cfg(miri)]
std::thread::yield_now(); // https://github.com/rust-lang/miri/issues/1388
}
});

scope.spawn(|_| {
for _ in 0..steps {
s.send(DropCounter).unwrap();
#[cfg(miri)]
std::thread::yield_now(); // https://github.com/rust-lang/miri/issues/1388
}
});
})
Expand Down
2 changes: 0 additions & 2 deletions crossbeam-channel/tests/list.rs
Original file line number Diff line number Diff line change
Expand Up @@ -433,8 +433,6 @@ fn drops() {
scope.spawn(|_| {
for _ in 0..steps {
r.recv().unwrap();
#[cfg(miri)]
std::thread::yield_now(); // https://github.com/rust-lang/miri/issues/1388
}
});

Expand Down
2 changes: 1 addition & 1 deletion crossbeam-channel/tests/thread_locals.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
//! Tests that make sure accessing thread-locals while exiting the thread doesn't cause panics.

#![cfg(not(miri))] // error: abnormal termination: the evaluated program aborted execution
#![cfg(not(miri))] // Miri detects that this test is buggy: the destructor of `FOO` uses `std::thread::current()`!

use std::thread;
use std::time::Duration;
Expand Down
43 changes: 20 additions & 23 deletions crossbeam-deque/src/deque.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,8 @@ impl<T> Buffer<T> {
/// Returns a pointer to the task at the specified `index`.
unsafe fn at(&self, index: isize) -> *mut T {
// `self.cap` is always a power of two.
// We do all the loads at `MaybeUninit` because we might realize, after loading, that we
// don't actually have the right to access this memory.
self.ptr.offset(index & (self.cap - 1) as isize)
}

Expand All @@ -62,18 +64,18 @@ impl<T> Buffer<T> {
/// technically speaking a data race and therefore UB. We should use an atomic store here, but
/// that would be more expensive and difficult to implement generically for all types `T`.
/// Hence, as a hack, we use a volatile write instead.
unsafe fn write(&self, index: isize, task: T) {
ptr::write_volatile(self.at(index), task)
unsafe fn write(&self, index: isize, task: MaybeUninit<T>) {
Copy link
Member

Choose a reason for hiding this comment

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

Since the value written to self.at(index) must be valid T (otherwise, the destructor cannot be sure that the value is valid), it may be better to use T instead of MaybeUninit<T> in the argument (and use MaybeUninit::new(task) in the next line).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did this because there are some places that just read the data and write it back immediately. By taking MaybeUninit<T> here we can avoid having to assert that this data is valid.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, that makes sense.

ptr::write_volatile(self.at(index) as *mut MaybeUninit<T>, task)
}

/// Reads a task from the specified `index`.
///
/// This method might be concurrently called with another `write` at the same index, which is
/// technically speaking a data race and therefore UB. We should use an atomic load here, but
/// that would be more expensive and difficult to implement generically for all types `T`.
/// Hence, as a hack, we use a volatile write instead.
unsafe fn read(&self, index: isize) -> T {
ptr::read_volatile(self.at(index))
/// Hence, as a hack, we use a volatile load instead.
unsafe fn read(&self, index: isize) -> MaybeUninit<T> {
ptr::read_volatile(self.at(index) as *mut MaybeUninit<T>)
}
}

Expand Down Expand Up @@ -406,7 +408,7 @@ impl<T> Worker<T> {

// Write `task` into the slot.
unsafe {
buffer.write(b, task);
buffer.write(b, MaybeUninit::new(task));
}

atomic::fence(Ordering::Release);
Expand Down Expand Up @@ -461,7 +463,7 @@ impl<T> Worker<T> {
unsafe {
// Read the popped task.
let buffer = self.buffer.get();
let task = buffer.read(f);
let task = buffer.read(f).assume_init();

// Shrink the buffer if `len - 1` is less than one fourth of the capacity.
if buffer.cap > MIN_CAP && len <= buffer.cap as isize / 4 {
Expand Down Expand Up @@ -509,8 +511,8 @@ impl<T> Worker<T> {
)
.is_err()
{
// Failed. We didn't pop anything.
mem::forget(task.take());
// Failed. We didn't pop anything. Reset to `None`.
task.take();
}

// Restore the back index to the original task.
Expand All @@ -524,7 +526,7 @@ impl<T> Worker<T> {
}
}

task
task.map(|t| unsafe { t.assume_init() })
}
}
}
Expand Down Expand Up @@ -661,12 +663,11 @@ impl<T> Stealer<T> {
.is_err()
{
// We didn't steal this task, forget it.
mem::forget(task);
return Steal::Retry;
}

// Return the stolen task.
Steal::Success(task)
Steal::Success(unsafe { task.assume_init() })
}

/// Steals a batch of tasks and pushes them into another worker.
Expand Down Expand Up @@ -821,7 +822,6 @@ impl<T> Stealer<T> {
.is_err()
{
// We didn't steal this task, forget it and break from the loop.
mem::forget(task);
batch_size = i;
break;
}
Expand Down Expand Up @@ -975,7 +975,6 @@ impl<T> Stealer<T> {
.is_err()
{
// We didn't steal this task, forget it.
mem::forget(task);
return Steal::Retry;
}

Expand All @@ -992,7 +991,6 @@ impl<T> Stealer<T> {
.is_err()
{
// We didn't steal this task, forget it.
mem::forget(task);
return Steal::Retry;
}

Expand Down Expand Up @@ -1037,7 +1035,6 @@ impl<T> Stealer<T> {
.is_err()
{
// We didn't steal this task, forget it and break from the loop.
mem::forget(tmp);
batch_size = i;
break;
}
Expand Down Expand Up @@ -1077,7 +1074,7 @@ impl<T> Stealer<T> {
dest.inner.back.store(dest_b, Ordering::Release);

// Return with success.
Steal::Success(task)
Steal::Success(unsafe { task.assume_init() })
}
}

Expand Down Expand Up @@ -1535,7 +1532,7 @@ impl<T> Injector<T> {
// Read the task.
let slot = (*block).slots.get_unchecked(offset + i);
slot.wait_write();
let task = slot.task.get().read().assume_init();
let task = slot.task.get().read();

// Write it into the destination queue.
dest_buffer.write(dest_b.wrapping_add(i as isize), task);
Expand All @@ -1547,7 +1544,7 @@ impl<T> Injector<T> {
// Read the task.
let slot = (*block).slots.get_unchecked(offset + i);
slot.wait_write();
let task = slot.task.get().read().assume_init();
let task = slot.task.get().read();

// Write it into the destination queue.
dest_buffer.write(dest_b.wrapping_add((batch_size - 1 - i) as isize), task);
Expand Down Expand Up @@ -1689,7 +1686,7 @@ impl<T> Injector<T> {
// Read the task.
let slot = (*block).slots.get_unchecked(offset);
slot.wait_write();
let task = slot.task.get().read().assume_init();
let task = slot.task.get().read();

match dest.flavor {
Flavor::Fifo => {
Expand All @@ -1698,7 +1695,7 @@ impl<T> Injector<T> {
// Read the task.
let slot = (*block).slots.get_unchecked(offset + i + 1);
slot.wait_write();
let task = slot.task.get().read().assume_init();
let task = slot.task.get().read();

// Write it into the destination queue.
dest_buffer.write(dest_b.wrapping_add(i as isize), task);
Expand All @@ -1711,7 +1708,7 @@ impl<T> Injector<T> {
// Read the task.
let slot = (*block).slots.get_unchecked(offset + i + 1);
slot.wait_write();
let task = slot.task.get().read().assume_init();
let task = slot.task.get().read();

// Write it into the destination queue.
dest_buffer.write(dest_b.wrapping_add((batch_size - 1 - i) as isize), task);
Expand Down Expand Up @@ -1744,7 +1741,7 @@ impl<T> Injector<T> {
}
}

Steal::Success(task)
Steal::Success(task.assume_init())
}
}

Expand Down
23 changes: 3 additions & 20 deletions crossbeam-queue/tests/array_queue.rs
Original file line number Diff line number Diff line change
Expand Up @@ -103,8 +103,6 @@ fn len() {
assert_eq!(x, i);
break;
}
#[cfg(miri)]
std::thread::yield_now(); // https://github.com/rust-lang/miri/issues/1388
}
let len = q.len();
assert!(len <= CAP);
Expand All @@ -113,10 +111,7 @@ fn len() {

scope.spawn(|_| {
for i in 0..COUNT {
while q.push(i).is_err() {
#[cfg(miri)]
std::thread::yield_now(); // https://github.com/rust-lang/miri/issues/1388
}
while q.push(i).is_err() {}
let len = q.len();
assert!(len <= CAP);
}
Expand All @@ -143,19 +138,14 @@ fn spsc() {
assert_eq!(x, i);
break;
}
#[cfg(miri)]
std::thread::yield_now(); // https://github.com/rust-lang/miri/issues/1388
}
}
assert!(q.pop().is_none());
});

scope.spawn(|_| {
for i in 0..COUNT {
while q.push(i).is_err() {
#[cfg(miri)]
std::thread::yield_now(); // https://github.com/rust-lang/miri/issues/1388
}
while q.push(i).is_err() {}
}
});
})
Expand Down Expand Up @@ -184,8 +174,6 @@ fn spsc_ring_buffer() {
}
}
}
#[cfg(miri)]
std::thread::yield_now(); // https://github.com/rust-lang/miri/issues/1388
});

scope.spawn(|_| {
Expand Down Expand Up @@ -320,19 +308,14 @@ fn drops() {
scope(|scope| {
scope.spawn(|_| {
for _ in 0..steps {
while q.pop().is_none() {
#[cfg(miri)]
std::thread::yield_now(); // https://github.com/rust-lang/miri/issues/1388
}
while q.pop().is_none() {}
}
});

scope.spawn(|_| {
for _ in 0..steps {
while q.push(DropCounter).is_err() {
DROPS.fetch_sub(1, Ordering::SeqCst);
#[cfg(miri)]
std::thread::yield_now(); // https://github.com/rust-lang/miri/issues/1388
}
}
});
Expand Down
7 changes: 1 addition & 6 deletions crossbeam-queue/tests/seg_queue.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,6 @@ fn spsc() {
assert_eq!(x, i);
break;
}
#[cfg(miri)]
std::thread::yield_now(); // https://github.com/rust-lang/miri/issues/1388
}
}
assert!(q.pop().is_none());
Expand Down Expand Up @@ -155,10 +153,7 @@ fn drops() {
scope(|scope| {
scope.spawn(|_| {
for _ in 0..steps {
while q.pop().is_none() {
#[cfg(miri)]
std::thread::yield_now(); // https://github.com/rust-lang/miri/issues/1388
}
while q.pop().is_none() {}
}
});

Expand Down
5 changes: 3 additions & 2 deletions crossbeam-utils/tests/wait_group.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ fn wait() {
}

#[test]
#[cfg(not(miri))] // this test makes timing assumptions, but Miri is so slow it violates them
fn wait_and_drop() {
let wg = WaitGroup::new();
let (tx, rx) = mpsc::channel();
Expand All @@ -51,8 +52,8 @@ fn wait_and_drop() {
});
}

// At this point, all spawned threads should be sleeping, so we shouldn't get anything from the
// channel.
// At this point, all spawned threads should be in `thread::sleep`, so we shouldn't get anything
// from the channel.
assert!(rx.try_recv().is_err());

wg.wait();
Expand Down