From 24c19b87ab220f2f0cd8480dd32cfad98e9dc016 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 7 Aug 2024 14:50:34 +0200 Subject: [PATCH] miri: no longer complain about read-read races --- src/tools/miri/src/concurrency/data_race.rs | 23 +++-------- .../tests/fail/data_race/read_read_race1.rs | 30 -------------- .../fail/data_race/read_read_race1.stderr | 22 ---------- .../tests/fail/data_race/read_read_race2.rs | 30 -------------- .../fail/data_race/read_read_race2.stderr | 22 ---------- .../miri/tests/pass/concurrency/data_race.rs | 40 ++++++++++++++++++- 6 files changed, 44 insertions(+), 123 deletions(-) delete mode 100644 src/tools/miri/tests/fail/data_race/read_read_race1.rs delete mode 100644 src/tools/miri/tests/fail/data_race/read_read_race1.stderr delete mode 100644 src/tools/miri/tests/fail/data_race/read_read_race2.rs delete mode 100644 src/tools/miri/tests/fail/data_race/read_read_race2.stderr diff --git a/src/tools/miri/src/concurrency/data_race.rs b/src/tools/miri/src/concurrency/data_race.rs index 9df0d95f1f275..69ba396a58b69 100644 --- a/src/tools/miri/src/concurrency/data_race.rs +++ b/src/tools/miri/src/concurrency/data_race.rs @@ -498,7 +498,7 @@ impl MemoryCellClocks { Ok(()) } - /// Detect data-races with an atomic read, caused by a non-atomic access that does + /// Detect data-races with an atomic read, caused by a non-atomic write that does /// not happen-before the atomic-read. fn atomic_read_detect( &mut self, @@ -509,12 +509,8 @@ impl MemoryCellClocks { trace!("Atomic read with vectors: {:#?} :: {:#?}", self, thread_clocks); let atomic = self.atomic_access(thread_clocks, access_size)?; atomic.read_vector.set_at_index(&thread_clocks.clock, index); - // Make sure the last non-atomic write and all non-atomic reads were before this access. - if self.write_was_before(&thread_clocks.clock) && self.read <= thread_clocks.clock { - Ok(()) - } else { - Err(DataRace) - } + // Make sure the last non-atomic write was before this access. + if self.write_was_before(&thread_clocks.clock) { Ok(()) } else { Err(DataRace) } } /// Detect data-races with an atomic write, either with a non-atomic read or with @@ -551,11 +547,9 @@ impl MemoryCellClocks { } thread_clocks.clock.index_mut(index).set_read_type(read_type); if self.write_was_before(&thread_clocks.clock) { + // We must be ordered-after all atomic writes. let race_free = if let Some(atomic) = self.atomic() { - // We must be ordered-after all atomic accesses, reads and writes. - // This ensures we don't mix atomic and non-atomic accesses. atomic.write_vector <= thread_clocks.clock - && atomic.read_vector <= thread_clocks.clock } else { true }; @@ -955,9 +949,7 @@ impl VClockAlloc { let mut other_size = None; // if `Some`, this was a size-mismatch race let write_clock; let (other_access, other_thread, other_clock) = - // First check the atomic-nonatomic cases. If it looks like multiple - // cases apply, this one should take precedence, else it might look like - // we are reporting races between two non-atomic reads. + // First check the atomic-nonatomic cases. if !access.is_atomic() && let Some(atomic) = mem_clocks.atomic() && let Some(idx) = Self::find_gt_index(&atomic.write_vector, &active_clocks.clock) @@ -1005,10 +997,7 @@ impl VClockAlloc { assert!(!involves_non_atomic); Some("overlapping unsynchronized atomic accesses must use the same access size") } else if access.is_read() && other_access.is_read() { - assert!(involves_non_atomic); - Some( - "overlapping atomic and non-atomic accesses must be synchronized, even if both are read-only", - ) + panic!("there should be no same-size read-read races") } else { None }; diff --git a/src/tools/miri/tests/fail/data_race/read_read_race1.rs b/src/tools/miri/tests/fail/data_race/read_read_race1.rs deleted file mode 100644 index 02aa4e4b716e3..0000000000000 --- a/src/tools/miri/tests/fail/data_race/read_read_race1.rs +++ /dev/null @@ -1,30 +0,0 @@ -//@compile-flags: -Zmiri-preemption-rate=0.0 -// Avoid accidental synchronization via address reuse inside `thread::spawn`. -//@compile-flags: -Zmiri-address-reuse-cross-thread-rate=0 - -use std::sync::atomic::{AtomicU16, Ordering}; -use std::thread; - -// Make sure races between atomic and non-atomic reads are detected. -// This seems harmless but C++ does not allow them, so we can't allow them for now either. -// This test coverse the case where the non-atomic access come first. -fn main() { - let a = AtomicU16::new(0); - - thread::scope(|s| { - s.spawn(|| { - let ptr = &a as *const AtomicU16 as *mut u16; - unsafe { ptr.read() }; - }); - s.spawn(|| { - thread::yield_now(); - - // We also put a non-atomic access here, but that should *not* be reported. - let ptr = &a as *const AtomicU16 as *mut u16; - unsafe { ptr.read() }; - // Then do the atomic access. - a.load(Ordering::SeqCst); - //~^ ERROR: Data race detected between (1) non-atomic read on thread `unnamed-1` and (2) atomic load on thread `unnamed-2` - }); - }); -} diff --git a/src/tools/miri/tests/fail/data_race/read_read_race1.stderr b/src/tools/miri/tests/fail/data_race/read_read_race1.stderr deleted file mode 100644 index 37ef46335d414..0000000000000 --- a/src/tools/miri/tests/fail/data_race/read_read_race1.stderr +++ /dev/null @@ -1,22 +0,0 @@ -error: Undefined Behavior: Data race detected between (1) non-atomic read on thread `unnamed-ID` and (2) atomic load on thread `unnamed-ID` at ALLOC. (2) just happened here - --> $DIR/read_read_race1.rs:LL:CC - | -LL | a.load(Ordering::SeqCst); - | ^^^^^^^^^^^^^^^^^^^^^^^^ Data race detected between (1) non-atomic read on thread `unnamed-ID` and (2) atomic load on thread `unnamed-ID` at ALLOC. (2) just happened here - | -help: and (1) occurred earlier here - --> $DIR/read_read_race1.rs:LL:CC - | -LL | unsafe { ptr.read() }; - | ^^^^^^^^^^ - = help: overlapping atomic and non-atomic accesses must be synchronized, even if both are read-only - = help: see https://doc.rust-lang.org/nightly/std/sync/atomic/index.html#memory-model-for-atomic-accesses for more information about the Rust memory model - = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior - = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information - = note: BACKTRACE (of the first span) on thread `unnamed-ID`: - = note: inside closure at $DIR/read_read_race1.rs:LL:CC - -note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace - -error: aborting due to 1 previous error - diff --git a/src/tools/miri/tests/fail/data_race/read_read_race2.rs b/src/tools/miri/tests/fail/data_race/read_read_race2.rs deleted file mode 100644 index 3b94c9143f3b2..0000000000000 --- a/src/tools/miri/tests/fail/data_race/read_read_race2.rs +++ /dev/null @@ -1,30 +0,0 @@ -//@compile-flags: -Zmiri-preemption-rate=0.0 -// Avoid accidental synchronization via address reuse inside `thread::spawn`. -//@compile-flags: -Zmiri-address-reuse-cross-thread-rate=0 - -use std::sync::atomic::{AtomicU16, Ordering}; -use std::thread; - -// Make sure races between atomic and non-atomic reads are detected. -// This seems harmless but C++ does not allow them, so we can't allow them for now either. -// This test coverse the case where the atomic access come first. -fn main() { - let a = AtomicU16::new(0); - - thread::scope(|s| { - s.spawn(|| { - // We also put a non-atomic access here, but that should *not* be reported. - let ptr = &a as *const AtomicU16 as *mut u16; - unsafe { ptr.read() }; - // Then do the atomic access. - a.load(Ordering::SeqCst); - }); - s.spawn(|| { - thread::yield_now(); - - let ptr = &a as *const AtomicU16 as *mut u16; - unsafe { ptr.read() }; - //~^ ERROR: Data race detected between (1) atomic load on thread `unnamed-1` and (2) non-atomic read on thread `unnamed-2` - }); - }); -} diff --git a/src/tools/miri/tests/fail/data_race/read_read_race2.stderr b/src/tools/miri/tests/fail/data_race/read_read_race2.stderr deleted file mode 100644 index e0cabf62a2504..0000000000000 --- a/src/tools/miri/tests/fail/data_race/read_read_race2.stderr +++ /dev/null @@ -1,22 +0,0 @@ -error: Undefined Behavior: Data race detected between (1) atomic load on thread `unnamed-ID` and (2) non-atomic read on thread `unnamed-ID` at ALLOC. (2) just happened here - --> $DIR/read_read_race2.rs:LL:CC - | -LL | unsafe { ptr.read() }; - | ^^^^^^^^^^ Data race detected between (1) atomic load on thread `unnamed-ID` and (2) non-atomic read on thread `unnamed-ID` at ALLOC. (2) just happened here - | -help: and (1) occurred earlier here - --> $DIR/read_read_race2.rs:LL:CC - | -LL | a.load(Ordering::SeqCst); - | ^^^^^^^^^^^^^^^^^^^^^^^^ - = help: overlapping atomic and non-atomic accesses must be synchronized, even if both are read-only - = help: see https://doc.rust-lang.org/nightly/std/sync/atomic/index.html#memory-model-for-atomic-accesses for more information about the Rust memory model - = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior - = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information - = note: BACKTRACE (of the first span) on thread `unnamed-ID`: - = note: inside closure at $DIR/read_read_race2.rs:LL:CC - -note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace - -error: aborting due to 1 previous error - diff --git a/src/tools/miri/tests/pass/concurrency/data_race.rs b/src/tools/miri/tests/pass/concurrency/data_race.rs index d31420380a565..3abc9a1b3fa4a 100644 --- a/src/tools/miri/tests/pass/concurrency/data_race.rs +++ b/src/tools/miri/tests/pass/concurrency/data_race.rs @@ -1,7 +1,7 @@ //@compile-flags: -Zmiri-disable-weak-memory-emulation -Zmiri-preemption-rate=0 -use std::sync::atomic::{fence, AtomicUsize, Ordering}; -use std::thread::spawn; +use std::sync::atomic::*; +use std::thread::{self, spawn}; #[derive(Copy, Clone)] struct EvilSend(pub T); @@ -112,9 +112,45 @@ pub fn test_simple_release() { } } +// This test coverse the case where the non-atomic access come first. +fn test_read_read_race1() { + let a = AtomicU16::new(0); + + thread::scope(|s| { + s.spawn(|| { + let ptr = &a as *const AtomicU16 as *mut u16; + unsafe { ptr.read() }; + }); + s.spawn(|| { + thread::yield_now(); + + a.load(Ordering::SeqCst); + }); + }); +} + +// This test coverse the case where the atomic access come first. +fn test_read_read_race2() { + let a = AtomicU16::new(0); + + thread::scope(|s| { + s.spawn(|| { + a.load(Ordering::SeqCst); + }); + s.spawn(|| { + thread::yield_now(); + + let ptr = &a as *const AtomicU16 as *mut u16; + unsafe { ptr.read() }; + }); + }); +} + pub fn main() { test_fence_sync(); test_multiple_reads(); test_rmw_no_block(); test_simple_release(); + test_read_read_race1(); + test_read_read_race2(); }