From f15b5637f22ca2c4357d36206d687f439e585a87 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 24 Oct 2023 07:32:30 +0200 Subject: [PATCH] fix error read-read reporting when there's also an unsynchronized non-atomic read (which is fine) --- src/tools/miri/src/concurrency/data_race.rs | 39 ++++++++----------- .../tests/fail/data_race/read_read_race1.rs | 5 +++ .../tests/fail/data_race/read_read_race2.rs | 5 +++ .../miri/tests/pass/concurrency/simple.rs | 18 +++++++++ 4 files changed, 45 insertions(+), 22 deletions(-) diff --git a/src/tools/miri/src/concurrency/data_race.rs b/src/tools/miri/src/concurrency/data_race.rs index d9bc043225d7..4cab86af8861 100644 --- a/src/tools/miri/src/concurrency/data_race.rs +++ b/src/tools/miri/src/concurrency/data_race.rs @@ -847,13 +847,27 @@ impl VClockAlloc { let mut action = Cow::Borrowed(action); let mut involves_non_atomic = true; let write_clock; - #[rustfmt::skip] let (other_action, other_thread, other_clock) = - if mem_clocks.write.1 > current_clocks.clock[mem_clocks.write.0] { + // 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. + if !is_atomic && + let Some(atomic) = mem_clocks.atomic() && + let Some(idx) = Self::find_gt_index(&atomic.write_vector, ¤t_clocks.clock) + { + (format!("Atomic Store"), idx, &atomic.write_vector) + } else if !is_atomic && + let Some(atomic) = mem_clocks.atomic() && + let Some(idx) = Self::find_gt_index(&atomic.read_vector, ¤t_clocks.clock) + { + (format!("Atomic Load"), idx, &atomic.read_vector) + // Then check races with non-atomic writes/reads. + } else if mem_clocks.write.1 > current_clocks.clock[mem_clocks.write.0] { write_clock = mem_clocks.write(); (mem_clocks.write_type.get_descriptor().to_owned(), mem_clocks.write.0, &write_clock) } else if let Some(idx) = Self::find_gt_index(&mem_clocks.read, ¤t_clocks.clock) { (format!("Read"), idx, &mem_clocks.read) + // Finally, mixed-size races. } else if is_atomic && let Some(atomic) = mem_clocks.atomic() && atomic.size != access_size { // This is only a race if we are not synchronized with all atomic accesses, so find // the one we are not synchronized with. @@ -871,27 +885,8 @@ impl VClockAlloc { "Failed to report data-race for mixed-size access: no race found" ) } - } else if !is_atomic { - if let Some(atomic) = mem_clocks.atomic() { - if let Some(idx) = Self::find_gt_index(&atomic.write_vector, ¤t_clocks.clock) - { - (format!("Atomic Store"), idx, &atomic.write_vector) - } else if let Some(idx) = - Self::find_gt_index(&atomic.read_vector, ¤t_clocks.clock) - { - (format!("Atomic Load"), idx, &atomic.read_vector) - } else { - unreachable!( - "Failed to report data-race for non-atomic operation: no race found" - ) - } - } else { - unreachable!( - "Failed to report data-race for non-atomic operation: no atomic component" - ) - } } else { - unreachable!("Failed to report data-race for atomic operation") + unreachable!("Failed to report data-race") }; // Load elaborated thread information about the racing thread actions. 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 index 295121f5765a..eebfbc74d40a 100644 --- a/src/tools/miri/tests/fail/data_race/read_read_race1.rs +++ b/src/tools/miri/tests/fail/data_race/read_read_race1.rs @@ -15,6 +15,11 @@ fn main() { }); 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) Read on thread `` and (2) Atomic Load on thread `` }); 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 index bf180a1543e8..230b429e2878 100644 --- a/src/tools/miri/tests/fail/data_race/read_read_race2.rs +++ b/src/tools/miri/tests/fail/data_race/read_read_race2.rs @@ -10,10 +10,15 @@ fn main() { 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 `` and (2) Read on thread `` diff --git a/src/tools/miri/tests/pass/concurrency/simple.rs b/src/tools/miri/tests/pass/concurrency/simple.rs index 556e0a24769d..ec549a998bae 100644 --- a/src/tools/miri/tests/pass/concurrency/simple.rs +++ b/src/tools/miri/tests/pass/concurrency/simple.rs @@ -62,6 +62,23 @@ fn panic_named() { .unwrap_err(); } +// This is not a data race! +fn shared_readonly() { + use std::sync::Arc; + + let x = Arc::new(42i32); + let h = thread::spawn({ + let x = Arc::clone(&x); + move || { + assert_eq!(*x, 42); + } + }); + + assert_eq!(*x, 42); + + h.join().unwrap(); +} + fn main() { create_and_detach(); create_and_join(); @@ -71,6 +88,7 @@ fn main() { create_nested_and_join(); create_move_in(); create_move_out(); + shared_readonly(); panic(); panic_named(); }