Merge #855 #858

855: Miri: enable preemption again r=taiki-e a=RalfJung Miri has a [work-around](rust-lang/miri#2248) for rust-lang/rust#55005 now. Also, "check-number-validity" is on by default these days. And I am not sure why "compare-exchange-weak-failure-rate=0.0" was set so let's see what happens when we remove it. :) 858: Ignore clippy::let_unit_value lint r=taiki-e a=taiki-e Co-authored-by: Ralf Jung <[email protected]> Co-authored-by: Taiki Endo <[email protected]>
crossbeam-rs · Jul 1, 2022 · 4b949c2 · 4b949c2
3 parents f12133c + 93148ad + e718229
commit 4b949c2
Show file tree

Hide file tree

Showing 12 changed files with 70 additions and 109 deletions.
diff --git a/ci/miri.sh b/ci/miri.sh
@@ -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
+
 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-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-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  '
diff --git a/crossbeam-channel/tests/array.rs b/crossbeam-channel/tests/array.rs
@@ -377,7 +377,7 @@ fn spsc() {
 #[test]
 fn mpmc() {
     #[cfg(miri)]
-    const COUNT: usize = 100;
+    const COUNT: usize = 50;
     #[cfg(not(miri))]
     const COUNT: usize = 25_000;
     const THREADS: usize = 4;
@@ -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
                 }
             });
         })

diff --git a/crossbeam-channel/tests/golang.rs b/crossbeam-channel/tests/golang.rs
@@ -959,7 +959,7 @@ mod chan_test {
     #[test]
     fn test_chan() {
         #[cfg(miri)]
-        const N: i32 = 20;
+        const N: i32 = 12;
         #[cfg(not(miri))]
         const N: i32 = 200;
 
@@ -1489,7 +1489,7 @@ mod chan_test {
     fn test_multi_consumer() {
         const NWORK: usize = 23;
         #[cfg(miri)]
-        const NITER: usize = 100;
+        const NITER: usize = 50;
         #[cfg(not(miri))]
         const NITER: usize = 271828;
 

diff --git a/crossbeam-channel/tests/list.rs b/crossbeam-channel/tests/list.rs
@@ -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
                 }
             });
 

diff --git a/crossbeam-channel/tests/mpsc.rs b/crossbeam-channel/tests/mpsc.rs
@@ -339,25 +339,22 @@ mod channel_tests {
 
     #[test]
     fn stress_shared() {
-        #[cfg(miri)]
-        const AMT: u32 = 100;
-        #[cfg(not(miri))]
-        const AMT: u32 = 10000;
-        const NTHREADS: u32 = 8;
+        let amt: u32 = if cfg!(miri) { 100 } else { 10_000 };
+        let nthreads: u32 = if cfg!(miri) { 4 } else { 8 };
         let (tx, rx) = channel::<i32>();
 
         let t = thread::spawn(move || {
-            for _ in 0..AMT * NTHREADS {
+            for _ in 0..amt * nthreads {
                 assert_eq!(rx.recv().unwrap(), 1);
             }
             assert!(rx.try_recv().is_err());
         });
 
-        let mut ts = Vec::with_capacity(NTHREADS as usize);
-        for _ in 0..NTHREADS {
+        let mut ts = Vec::with_capacity(nthreads as usize);
+        for _ in 0..nthreads {
             let tx = tx.clone();
             let t = thread::spawn(move || {
-                for _ in 0..AMT {
+                for _ in 0..amt {
                     tx.send(1).unwrap();
                 }
             });

diff --git a/crossbeam-channel/tests/thread_locals.rs b/crossbeam-channel/tests/thread_locals.rs
@@ -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;

diff --git a/crossbeam-channel/tests/zero.rs b/crossbeam-channel/tests/zero.rs
@@ -403,7 +403,7 @@ fn drops() {
     #[cfg(not(miri))]
     const RUNS: usize = 100;
     #[cfg(miri)]
-    const STEPS: usize = 500;
+    const STEPS: usize = 100;
     #[cfg(not(miri))]
     const STEPS: usize = 10_000;
 

diff --git a/crossbeam-deque/src/deque.rs b/crossbeam-deque/src/deque.rs
@@ -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)
     }
 
@@ -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>) {
+        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>)
     }
 }
 
@@ -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);
@@ -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 {
@@ -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.
@@ -524,7 +526,7 @@ impl<T> Worker<T> {
                         }
                     }
 
-                    task
+                    task.map(|t| unsafe { t.assume_init() })
                 }
             }
         }
@@ -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.
@@ -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;
                     }
@@ -975,7 +975,6 @@ impl<T> Stealer<T> {
                         .is_err()
                 {
                     // We didn't steal this task, forget it.
-                    mem::forget(task);
                     return Steal::Retry;
                 }
 
@@ -992,7 +991,6 @@ impl<T> Stealer<T> {
                     .is_err()
                 {
                     // We didn't steal this task, forget it.
-                    mem::forget(task);
                     return Steal::Retry;
                 }
 
@@ -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;
                     }
@@ -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() })
     }
 }
 
@@ -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);
@@ -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);
@@ -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 => {
@@ -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);
@@ -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);
@@ -1744,7 +1741,7 @@ impl<T> Injector<T> {
                 }
             }
 
-            Steal::Success(task)
+            Steal::Success(task.assume_init())
         }
     }