fix error read-read reporting when there's also an unsynchronized non-atomic read (which is fine)

Author: RalfJung

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, &current_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, &current_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, &current_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, &current_clocks.clock)
-                    {
-                        (format!("Atomic Store"), idx, &atomic.write_vector)
-                    } else if let Some(idx) =
-                        Self::find_gt_index(&atomic.read_vector, &current_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.

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 `<unnamed>` and (2) Atomic Load on thread `<unnamed>`
         });

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 `<unnamed>` and (2) Read on thread `<unnamed>`

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();
 }