Auto merge of rust-lang#2302 - RalfJung:datarace, r=RalfJung

avoid copying thread manager state in data race detector

When doing rust-lang/miri#2047 I did not realize that there is some redundant state here that we can now remove from the data race detector.

Also this removes the vector clocks from the data race errors since those don't really help diagnose the problem.

Author: bors

src/concurrency/data_race.rs

@@ -9,7 +9,7 @@
 //! Note that this implementation does not take into account of C++20's memory model revision to SC accesses
 //! and fences introduced by P0668 (<https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0668r5.html>).
 //! This implementation is not fully correct under the revised C++20 model and may generate behaviours C++20
-//! disallows.
+//! disallows (<https://github.com/rust-lang/miri/issues/2301>).
 //!
 //! Rust follows the C++20 memory model (except for the Consume ordering and some operations not performable through C++'s
 //! std::atomic<T> API). It is therefore possible for this implementation to generate behaviours never observable when the
@@ -82,10 +82,12 @@ use rustc_const_eval::interpret::{
 };
 use rustc_data_structures::fx::FxHashMap;
 
-use crate::{AtomicReadOp, AtomicRwOp, AtomicWriteOp, Tag, VClock, VTimestamp, VectorIdx};
+use crate::{
+    AtomicReadOrd, AtomicRwOrd, AtomicWriteOrd, Tag, ThreadManager, VClock, VTimestamp, VectorIdx,
+};
 
 use super::{
-    data_race::{GlobalState, ThreadClockSet},
+    data_race::{GlobalState as DataRaceState, ThreadClockSet},
     range_object_map::{AccessType, RangeObjectMap},
 };
 
@@ -149,7 +151,7 @@ impl StoreBufferAlloc {
     /// before without data race, we can determine that the non-atomic access fully happens
     /// after all the prior atomic accesses so the location no longer needs to exhibit
     /// any weak memory behaviours until further atomic accesses.
-    pub fn memory_accessed(&self, range: AllocRange, global: &GlobalState) {
+    pub fn memory_accessed(&self, range: AllocRange, global: &DataRaceState) {
         if !global.ongoing_action_data_race_free() {
             let mut buffers = self.store_buffers.borrow_mut();
             let access_type = buffers.access_type(range);
@@ -236,17 +238,18 @@ impl<'mir, 'tcx: 'mir> StoreBuffer {
     }
 
     /// Reads from the last store in modification order
-    fn read_from_last_store(&self, global: &GlobalState) {
+    fn read_from_last_store(&self, global: &DataRaceState, thread_mgr: &ThreadManager<'_, '_>) {
         let store_elem = self.buffer.back();
         if let Some(store_elem) = store_elem {
-            let (index, clocks) = global.current_thread_state();
+            let (index, clocks) = global.current_thread_state(thread_mgr);
             store_elem.load_impl(index, &clocks);
         }
     }
 
     fn buffered_read(
         &self,
-        global: &GlobalState,
+        global: &DataRaceState,
+        thread_mgr: &ThreadManager<'_, '_>,
         is_seqcst: bool,
         rng: &mut (impl rand::Rng + ?Sized),
         validate: impl FnOnce() -> InterpResult<'tcx>,
@@ -257,7 +260,7 @@ impl<'mir, 'tcx: 'mir> StoreBuffer {
         let store_elem = {
             // The `clocks` we got here must be dropped before calling validate_atomic_load
             // as the race detector will update it
-            let (.., clocks) = global.current_thread_state();
+            let (.., clocks) = global.current_thread_state(thread_mgr);
             // Load from a valid entry in the store buffer
             self.fetch_store(is_seqcst, &clocks, &mut *rng)
         };
@@ -268,18 +271,19 @@ impl<'mir, 'tcx: 'mir> StoreBuffer {
         // requires access to ThreadClockSet.clock, which is updated by the race detector
         validate()?;
 
-        let (index, clocks) = global.current_thread_state();
+        let (index, clocks) = global.current_thread_state(thread_mgr);
         let loaded = store_elem.load_impl(index, &clocks);
         Ok(loaded)
     }
 
     fn buffered_write(
         &mut self,
         val: ScalarMaybeUninit<Tag>,
-        global: &GlobalState,
+        global: &DataRaceState,
+        thread_mgr: &ThreadManager<'_, '_>,
         is_seqcst: bool,
     ) -> InterpResult<'tcx> {
-        let (index, clocks) = global.current_thread_state();
+        let (index, clocks) = global.current_thread_state(thread_mgr);
 
         self.store_impl(val, index, &clocks.clock, is_seqcst);
         Ok(())
@@ -428,8 +432,11 @@ pub(super) trait EvalContextExt<'mir, 'tcx: 'mir>:
         {
             let range = alloc_range(base_offset, place.layout.size);
             if alloc_buffers.is_overlapping(range)
-                && !alloc_clocks
-                    .race_free_with_atomic(range, this.machine.data_race.as_ref().unwrap())
+                && !alloc_clocks.race_free_with_atomic(
+                    range,
+                    this.machine.data_race.as_ref().unwrap(),
+                    &this.machine.threads,
+                )
             {
                 throw_unsup_format!(
                     "racy imperfectly overlapping atomic access is not possible in the C++20 memory model, and not supported by Miri's weak memory emulation"
@@ -443,41 +450,41 @@ pub(super) trait EvalContextExt<'mir, 'tcx: 'mir>:
         &mut self,
         new_val: ScalarMaybeUninit<Tag>,
         place: &MPlaceTy<'tcx, Tag>,
-        atomic: AtomicRwOp,
+        atomic: AtomicRwOrd,
         init: ScalarMaybeUninit<Tag>,
     ) -> InterpResult<'tcx> {
         let this = self.eval_context_mut();
         let (alloc_id, base_offset, ..) = this.ptr_get_alloc_id(place.ptr)?;
         if let (
             crate::AllocExtra { weak_memory: Some(alloc_buffers), .. },
-            crate::Evaluator { data_race: Some(global), .. },
+            crate::Evaluator { data_race: Some(global), threads, .. },
         ) = this.get_alloc_extra_mut(alloc_id)?
         {
-            if atomic == AtomicRwOp::SeqCst {
-                global.sc_read();
-                global.sc_write();
+            if atomic == AtomicRwOrd::SeqCst {
+                global.sc_read(threads);
+                global.sc_write(threads);
             }
             let range = alloc_range(base_offset, place.layout.size);
             let buffer = alloc_buffers.get_or_create_store_buffer_mut(range, init)?;
-            buffer.read_from_last_store(global);
-            buffer.buffered_write(new_val, global, atomic == AtomicRwOp::SeqCst)?;
+            buffer.read_from_last_store(global, threads);
+            buffer.buffered_write(new_val, global, threads, atomic == AtomicRwOrd::SeqCst)?;
         }
         Ok(())
     }
 
     fn buffered_atomic_read(
         &self,
         place: &MPlaceTy<'tcx, Tag>,
-        atomic: AtomicReadOp,
+        atomic: AtomicReadOrd,
         latest_in_mo: ScalarMaybeUninit<Tag>,
         validate: impl FnOnce() -> InterpResult<'tcx>,
     ) -> InterpResult<'tcx, ScalarMaybeUninit<Tag>> {
         let this = self.eval_context_ref();
         if let Some(global) = &this.machine.data_race {
             let (alloc_id, base_offset, ..) = this.ptr_get_alloc_id(place.ptr)?;
             if let Some(alloc_buffers) = this.get_alloc_extra(alloc_id)?.weak_memory.as_ref() {
-                if atomic == AtomicReadOp::SeqCst {
-                    global.sc_read();
+                if atomic == AtomicReadOrd::SeqCst {
+                    global.sc_read(&this.machine.threads);
                 }
                 let mut rng = this.machine.rng.borrow_mut();
                 let buffer = alloc_buffers.get_or_create_store_buffer(
@@ -486,7 +493,8 @@ pub(super) trait EvalContextExt<'mir, 'tcx: 'mir>:
                 )?;
                 let loaded = buffer.buffered_read(
                     global,
-                    atomic == AtomicReadOp::SeqCst,
+                    &this.machine.threads,
+                    atomic == AtomicReadOrd::SeqCst,
                     &mut *rng,
                     validate,
                 )?;
@@ -504,18 +512,18 @@ pub(super) trait EvalContextExt<'mir, 'tcx: 'mir>:
         &mut self,
         val: ScalarMaybeUninit<Tag>,
         dest: &MPlaceTy<'tcx, Tag>,
-        atomic: AtomicWriteOp,
+        atomic: AtomicWriteOrd,
         init: ScalarMaybeUninit<Tag>,
     ) -> InterpResult<'tcx> {
         let this = self.eval_context_mut();
         let (alloc_id, base_offset, ..) = this.ptr_get_alloc_id(dest.ptr)?;
         if let (
             crate::AllocExtra { weak_memory: Some(alloc_buffers), .. },
-            crate::Evaluator { data_race: Some(global), .. },
+            crate::Evaluator { data_race: Some(global), threads, .. },
         ) = this.get_alloc_extra_mut(alloc_id)?
         {
-            if atomic == AtomicWriteOp::SeqCst {
-                global.sc_write();
+            if atomic == AtomicWriteOrd::SeqCst {
+                global.sc_write(threads);
             }
 
             // UGLY HACK: in write_scalar_atomic() we don't know the value before our write,
@@ -535,7 +543,7 @@ pub(super) trait EvalContextExt<'mir, 'tcx: 'mir>:
                 buffer.buffer.pop_front();
             }
 
-            buffer.buffered_write(val, global, atomic == AtomicWriteOp::SeqCst)?;
+            buffer.buffered_write(val, global, threads, atomic == AtomicWriteOrd::SeqCst)?;
         }
 
         // Caller should've written to dest with the vanilla scalar write, we do nothing here
@@ -548,21 +556,21 @@ pub(super) trait EvalContextExt<'mir, 'tcx: 'mir>:
     fn perform_read_on_buffered_latest(
         &self,
         place: &MPlaceTy<'tcx, Tag>,
-        atomic: AtomicReadOp,
+        atomic: AtomicReadOrd,
         init: ScalarMaybeUninit<Tag>,
     ) -> InterpResult<'tcx> {
         let this = self.eval_context_ref();
 
         if let Some(global) = &this.machine.data_race {
-            if atomic == AtomicReadOp::SeqCst {
-                global.sc_read();
+            if atomic == AtomicReadOrd::SeqCst {
+                global.sc_read(&this.machine.threads);
             }
             let size = place.layout.size;
             let (alloc_id, base_offset, ..) = this.ptr_get_alloc_id(place.ptr)?;
             if let Some(alloc_buffers) = this.get_alloc_extra(alloc_id)?.weak_memory.as_ref() {
                 let buffer = alloc_buffers
                     .get_or_create_store_buffer(alloc_range(base_offset, size), init)?;
-                buffer.read_from_last_store(global);
+                buffer.read_from_last_store(global, &this.machine.threads);
             }
         }
         Ok(())

src/concurrency/weak_memory.rs

@@ -70,7 +70,7 @@ pub use crate::shims::tls::{EvalContextExt as _, TlsData};
 pub use crate::shims::EvalContextExt as _;
 
 pub use crate::concurrency::data_race::{
-    AtomicFenceOp, AtomicReadOp, AtomicRwOp, AtomicWriteOp,
+    AtomicFenceOrd, AtomicReadOrd, AtomicRwOrd, AtomicWriteOrd,
     EvalContextExt as DataRaceEvalContextExt,
 };
 pub use crate::diagnostics::{

src/lib.rs

@@ -647,7 +647,12 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'mir, 'tcx> {
             None
         };
         let race_alloc = if let Some(data_race) = &ecx.machine.data_race {
-            Some(data_race::AllocExtra::new_allocation(data_race, alloc.size(), kind))
+            Some(data_race::AllocExtra::new_allocation(
+                data_race,
+                &ecx.machine.threads,
+                alloc.size(),
+                kind,
+            ))
         } else {
             None
         };
@@ -756,7 +761,12 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'mir, 'tcx> {
         range: AllocRange,
     ) -> InterpResult<'tcx> {
         if let Some(data_race) = &alloc_extra.data_race {
-            data_race.read(alloc_id, range, machine.data_race.as_ref().unwrap())?;
+            data_race.read(
+                alloc_id,
+                range,
+                machine.data_race.as_ref().unwrap(),
+                &machine.threads,
+            )?;
         }
         if let Some(stacked_borrows) = &alloc_extra.stacked_borrows {
             stacked_borrows.borrow_mut().memory_read(
@@ -782,7 +792,12 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'mir, 'tcx> {
         range: AllocRange,
     ) -> InterpResult<'tcx> {
         if let Some(data_race) = &mut alloc_extra.data_race {
-            data_race.write(alloc_id, range, machine.data_race.as_mut().unwrap())?;
+            data_race.write(
+                alloc_id,
+                range,
+                machine.data_race.as_mut().unwrap(),
+                &machine.threads,
+            )?;
         }
         if let Some(stacked_borrows) = &mut alloc_extra.stacked_borrows {
             stacked_borrows.get_mut().memory_written(
@@ -811,7 +826,12 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'mir, 'tcx> {
             register_diagnostic(NonHaltingDiagnostic::FreedAlloc(alloc_id));
         }
         if let Some(data_race) = &mut alloc_extra.data_race {
-            data_race.deallocate(alloc_id, range, machine.data_race.as_mut().unwrap())?;
+            data_race.deallocate(
+                alloc_id,
+                range,
+                machine.data_race.as_mut().unwrap(),
+                &machine.threads,
+            )?;
         }
         if let Some(stacked_borrows) = &mut alloc_extra.stacked_borrows {
             stacked_borrows.get_mut().memory_deallocated(