Auto merge of #3495 - RalfJung:data-race-clocks, r=RalfJung

data_race: make the release/acquire API more clear

src/tools/miri/src/alloc_addresses/mod.rs

@@ -13,7 +13,7 @@ use rustc_data_structures::fx::{FxHashMap, FxHashSet};
 use rustc_span::Span;
 use rustc_target::abi::{Align, HasDataLayout, Size};
 
-use crate::*;
+use crate::{concurrency::VClock, *};
 
 use self::reuse_pool::ReusePool;
 
@@ -171,8 +171,10 @@ trait EvalContextExtPriv<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
                     memory_kind,
                     ecx.get_active_thread(),
                 ) {
-                    if let Some(data_race) = &ecx.machine.data_race {
-                        data_race.validate_lock_acquire(&clock, ecx.get_active_thread());
+                    if let Some(clock) = clock
+                        && let Some(data_race) = &ecx.machine.data_race
+                    {
+                        data_race.acquire_clock(&clock, ecx.get_active_thread());
                     }
                     reuse_addr
                 } else {
@@ -369,15 +371,13 @@ impl<'mir, 'tcx> MiriMachine<'mir, 'tcx> {
         // Also remember this address for future reuse.
         let thread = self.threads.get_active_thread_id();
         global_state.reuse.add_addr(rng, addr, size, align, kind, thread, || {
-            let mut clock = concurrency::VClock::default();
             if let Some(data_race) = &self.data_race {
-                data_race.validate_lock_release(
-                    &mut clock,
-                    thread,
-                    self.threads.active_thread_ref().current_span(),
-                );
+                data_race
+                    .release_clock(thread, self.threads.active_thread_ref().current_span())
+                    .clone()
+            } else {
+                VClock::default()
             }
-            clock
         })
     }
 }

src/tools/miri/src/alloc_addresses/reuse_pool.rs

@@ -78,6 +78,7 @@ impl ReusePool {
         subpool.insert(pos, (addr, size, thread, clock));
     }
 
+    /// Returns the address to use and optionally a clock we have to synchronize with.
     pub fn take_addr(
         &mut self,
         rng: &mut impl Rng,
@@ -85,7 +86,7 @@ impl ReusePool {
         align: Align,
         kind: MemoryKind,
         thread: ThreadId,
-    ) -> Option<(u64, VClock)> {
+    ) -> Option<(u64, Option<VClock>)> {
         // Determine whether we'll even attempt a reuse. As above, we don't do reuse for stack addresses.
         if kind == MemoryKind::Stack || !rng.gen_bool(self.address_reuse_rate) {
             return None;
@@ -122,6 +123,7 @@ impl ReusePool {
         let (chosen_addr, chosen_size, chosen_thread, clock) = subpool.remove(idx);
         debug_assert!(chosen_size >= size && chosen_addr % align.bytes() == 0);
         debug_assert!(cross_thread_reuse || chosen_thread == thread);
-        Some((chosen_addr, clock))
+        // No synchronization needed if we reused from the current thread.
+        Some((chosen_addr, if chosen_thread == thread { None } else { Some(clock) }))
     }
 }

src/tools/miri/src/concurrency/data_race.rs

@@ -547,9 +547,9 @@ impl MemoryCellClocks {
     ) -> Result<(), DataRace> {
         trace!("Unsynchronized read with vectors: {:#?} :: {:#?}", self, thread_clocks);
         if !current_span.is_dummy() {
-            thread_clocks.clock[index].span = current_span;
+            thread_clocks.clock.index_mut(index).span = current_span;
         }
-        thread_clocks.clock[index].set_read_type(read_type);
+        thread_clocks.clock.index_mut(index).set_read_type(read_type);
         if self.write_was_before(&thread_clocks.clock) {
             let race_free = if let Some(atomic) = self.atomic() {
                 // We must be ordered-after all atomic accesses, reads and writes.
@@ -577,7 +577,7 @@ impl MemoryCellClocks {
     ) -> Result<(), DataRace> {
         trace!("Unsynchronized write with vectors: {:#?} :: {:#?}", self, thread_clocks);
         if !current_span.is_dummy() {
-            thread_clocks.clock[index].span = current_span;
+            thread_clocks.clock.index_mut(index).span = current_span;
         }
         if self.write_was_before(&thread_clocks.clock) && self.read <= thread_clocks.clock {
             let race_free = if let Some(atomic) = self.atomic() {
@@ -1701,49 +1701,34 @@ impl GlobalState {
         format!("thread `{thread_name}`")
     }
 
-    /// Acquire a lock, express that the previous call of
-    /// `validate_lock_release` must happen before this.
+    /// Acquire the given clock into the given thread, establishing synchronization with
+    /// the moment when that clock snapshot was taken via `release_clock`.
     /// As this is an acquire operation, the thread timestamp is not
     /// incremented.
-    pub fn validate_lock_acquire(&self, lock: &VClock, thread: ThreadId) {
-        let (_, mut clocks) = self.load_thread_state_mut(thread);
+    pub fn acquire_clock(&self, lock: &VClock, thread: ThreadId) {
+        let (_, mut clocks) = self.thread_state_mut(thread);
         clocks.clock.join(lock);
     }
 
-    /// Release a lock handle, express that this happens-before
-    /// any subsequent calls to `validate_lock_acquire`.
-    /// For normal locks this should be equivalent to `validate_lock_release_shared`
-    /// since an acquire operation should have occurred before, however
-    /// for futex & condvar operations this is not the case and this
-    /// operation must be used.
-    pub fn validate_lock_release(&self, lock: &mut VClock, thread: ThreadId, current_span: Span) {
-        let (index, mut clocks) = self.load_thread_state_mut(thread);
-        lock.clone_from(&clocks.clock);
-        clocks.increment_clock(index, current_span);
-    }
-
-    /// Release a lock handle, express that this happens-before
-    /// any subsequent calls to `validate_lock_acquire` as well
-    /// as any previous calls to this function after any
-    /// `validate_lock_release` calls.
-    /// For normal locks this should be equivalent to `validate_lock_release`.
-    /// This function only exists for joining over the set of concurrent readers
-    /// in a read-write lock and should not be used for anything else.
-    pub fn validate_lock_release_shared(
-        &self,
-        lock: &mut VClock,
-        thread: ThreadId,
-        current_span: Span,
-    ) {
-        let (index, mut clocks) = self.load_thread_state_mut(thread);
-        lock.join(&clocks.clock);
+    /// Returns the `release` clock of the given thread.
+    /// Other threads can acquire this clock in the future to establish synchronization
+    /// with this program point.
+    pub fn release_clock(&self, thread: ThreadId, current_span: Span) -> Ref<'_, VClock> {
+        // We increment the clock each time this happens, to ensure no two releases
+        // can be confused with each other.
+        let (index, mut clocks) = self.thread_state_mut(thread);
         clocks.increment_clock(index, current_span);
+        drop(clocks);
+        // To return a read-only view, we need to release the RefCell
+        // and borrow it again.
+        let (_index, clocks) = self.thread_state(thread);
+        Ref::map(clocks, |c| &c.clock)
     }
 
     /// Load the vector index used by the given thread as well as the set of vector clocks
     /// used by the thread.
     #[inline]
-    fn load_thread_state_mut(&self, thread: ThreadId) -> (VectorIdx, RefMut<'_, ThreadClockSet>) {
+    fn thread_state_mut(&self, thread: ThreadId) -> (VectorIdx, RefMut<'_, ThreadClockSet>) {
         let index = self.thread_info.borrow()[thread]
             .vector_index
             .expect("Loading thread state for thread with no assigned vector");
@@ -1752,6 +1737,18 @@ impl GlobalState {
         (index, clocks)
     }
 
+    /// Load the vector index used by the given thread as well as the set of vector clocks
+    /// used by the thread.
+    #[inline]
+    fn thread_state(&self, thread: ThreadId) -> (VectorIdx, Ref<'_, ThreadClockSet>) {
+        let index = self.thread_info.borrow()[thread]
+            .vector_index
+            .expect("Loading thread state for thread with no assigned vector");
+        let ref_vector = self.vector_clocks.borrow();
+        let clocks = Ref::map(ref_vector, |vec| &vec[index]);
+        (index, clocks)
+    }
+
     /// Load the current vector clock in use and the current set of thread clocks
     /// in use for the vector.
     #[inline]
@@ -1759,10 +1756,7 @@ impl GlobalState {
         &self,
         thread_mgr: &ThreadManager<'_, '_>,
     ) -> (VectorIdx, Ref<'_, ThreadClockSet>) {
-        let index = self.current_index(thread_mgr);
-        let ref_vector = self.vector_clocks.borrow();
-        let clocks = Ref::map(ref_vector, |vec| &vec[index]);
-        (index, clocks)
+        self.thread_state(thread_mgr.get_active_thread_id())
     }
 
     /// Load the current vector clock in use and the current set of thread clocks
@@ -1772,10 +1766,7 @@ impl GlobalState {
         &self,
         thread_mgr: &ThreadManager<'_, '_>,
     ) -> (VectorIdx, RefMut<'_, ThreadClockSet>) {
-        let index = self.current_index(thread_mgr);
-        let ref_vector = self.vector_clocks.borrow_mut();
-        let clocks = RefMut::map(ref_vector, |vec| &mut vec[index]);
-        (index, clocks)
+        self.thread_state_mut(thread_mgr.get_active_thread_id())
     }
 
     /// Return the current thread, should be the same

src/tools/miri/src/concurrency/init_once.rs

@@ -41,7 +41,7 @@ pub enum InitOnceStatus {
 pub(super) struct InitOnce<'mir, 'tcx> {
     status: InitOnceStatus,
     waiters: VecDeque<InitOnceWaiter<'mir, 'tcx>>,
-    data_race: VClock,
+    clock: VClock,
 }
 
 impl<'mir, 'tcx> VisitProvenance for InitOnce<'mir, 'tcx> {
@@ -61,10 +61,8 @@ trait EvalContextExtPriv<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
         let current_thread = this.get_active_thread();
 
         if let Some(data_race) = &this.machine.data_race {
-            data_race.validate_lock_acquire(
-                &this.machine.threads.sync.init_onces[id].data_race,
-                current_thread,
-            );
+            data_race
+                .acquire_clock(&this.machine.threads.sync.init_onces[id].clock, current_thread);
         }
     }
 
@@ -176,7 +174,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
 
         // Each complete happens-before the end of the wait
         if let Some(data_race) = &this.machine.data_race {
-            data_race.validate_lock_release(&mut init_once.data_race, current_thread, current_span);
+            init_once.clock.clone_from(&data_race.release_clock(current_thread, current_span));
         }
 
         // Wake up everyone.
@@ -202,7 +200,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
 
         // Each complete happens-before the end of the wait
         if let Some(data_race) = &this.machine.data_race {
-            data_race.validate_lock_release(&mut init_once.data_race, current_thread, current_span);
+            init_once.clock.clone_from(&data_race.release_clock(current_thread, current_span));
         }
 
         // Wake up one waiting thread, so they can go ahead and try to init this.

src/tools/miri/src/concurrency/sync.rs

@@ -69,12 +69,8 @@ struct Mutex {
     lock_count: usize,
     /// The queue of threads waiting for this mutex.
     queue: VecDeque<ThreadId>,
-    /// Data race handle. This tracks the happens-before
-    /// relationship between each mutex access. It is
-    /// released to during unlock and acquired from during
-    /// locking, and therefore stores the clock of the last
-    /// thread to release this mutex.
-    data_race: VClock,
+    /// Mutex clock. This tracks the moment of the last unlock.
+    clock: VClock,
 }
 
 declare_id!(RwLockId);
@@ -91,7 +87,7 @@ struct RwLock {
     writer_queue: VecDeque<ThreadId>,
     /// The queue of reader threads waiting for this lock.
     reader_queue: VecDeque<ThreadId>,
-    /// Data race handle for writers. Tracks the happens-before
+    /// Data race clock for writers. Tracks the happens-before
     /// ordering between each write access to a rwlock and is updated
     /// after a sequence of concurrent readers to track the happens-
     /// before ordering between the set of previous readers and
@@ -99,8 +95,8 @@ struct RwLock {
     /// Contains the clock of the last thread to release a writer
     /// lock or the joined clock of the set of last threads to release
     /// shared reader locks.
-    data_race: VClock,
-    /// Data race handle for readers. This is temporary storage
+    clock_unlocked: VClock,
+    /// Data race clock for readers. This is temporary storage
     /// for the combined happens-before ordering for between all
     /// concurrent readers and the next writer, and the value
     /// is stored to the main data_race variable once all
@@ -110,7 +106,7 @@ struct RwLock {
     /// add happens-before orderings between shared reader
     /// locks.
     /// This is only relevant when there is an active reader.
-    data_race_reader: VClock,
+    clock_current_readers: VClock,
 }
 
 declare_id!(CondvarId);
@@ -132,8 +128,8 @@ struct Condvar {
     /// between a cond-var signal and a cond-var
     /// wait during a non-spurious signal event.
     /// Contains the clock of the last thread to
-    /// perform a futex-signal.
-    data_race: VClock,
+    /// perform a condvar-signal.
+    clock: VClock,
 }
 
 /// The futex state.
@@ -145,7 +141,7 @@ struct Futex {
     /// during a non-spurious wake event.
     /// Contains the clock of the last thread to
     /// perform a futex-wake.
-    data_race: VClock,
+    clock: VClock,
 }
 
 /// A thread waiting on a futex.
@@ -346,7 +342,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
         }
         mutex.lock_count = mutex.lock_count.checked_add(1).unwrap();
         if let Some(data_race) = &this.machine.data_race {
-            data_race.validate_lock_acquire(&mutex.data_race, thread);
+            data_race.acquire_clock(&mutex.clock, thread);
         }
     }
 
@@ -373,11 +369,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
                 // The mutex is completely unlocked. Try transferring ownership
                 // to another thread.
                 if let Some(data_race) = &this.machine.data_race {
-                    data_race.validate_lock_release(
-                        &mut mutex.data_race,
-                        current_owner,
-                        current_span,
-                    );
+                    mutex.clock.clone_from(&data_race.release_clock(current_owner, current_span));
                 }
                 this.mutex_dequeue_and_lock(id);
             }
@@ -448,7 +440,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
         let count = rwlock.readers.entry(reader).or_insert(0);
         *count = count.checked_add(1).expect("the reader counter overflowed");
         if let Some(data_race) = &this.machine.data_race {
-            data_race.validate_lock_acquire(&rwlock.data_race, reader);
+            data_race.acquire_clock(&rwlock.clock_unlocked, reader);
         }
     }
 
@@ -474,20 +466,16 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
         }
         if let Some(data_race) = &this.machine.data_race {
             // Add this to the shared-release clock of all concurrent readers.
-            data_race.validate_lock_release_shared(
-                &mut rwlock.data_race_reader,
-                reader,
-                current_span,
-            );
+            rwlock.clock_current_readers.join(&data_race.release_clock(reader, current_span));
         }
 
         // The thread was a reader. If the lock is not held any more, give it to a writer.
         if this.rwlock_is_locked(id).not() {
             // All the readers are finished, so set the writer data-race handle to the value
-            //  of the union of all reader data race handles, since the set of readers
-            //  happen-before the writers
+            // of the union of all reader data race handles, since the set of readers
+            // happen-before the writers
             let rwlock = &mut this.machine.threads.sync.rwlocks[id];
-            rwlock.data_race.clone_from(&rwlock.data_race_reader);
+            rwlock.clock_unlocked.clone_from(&rwlock.clock_current_readers);
             this.rwlock_dequeue_and_lock_writer(id);
         }
         true
@@ -511,7 +499,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
         let rwlock = &mut this.machine.threads.sync.rwlocks[id];
         rwlock.writer = Some(writer);
         if let Some(data_race) = &this.machine.data_race {
-            data_race.validate_lock_acquire(&rwlock.data_race, writer);
+            data_race.acquire_clock(&rwlock.clock_unlocked, writer);
         }
     }
 
@@ -530,11 +518,9 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
             trace!("rwlock_writer_unlock: {:?} unlocked by {:?}", id, expected_writer);
             // Release memory to next lock holder.
             if let Some(data_race) = &this.machine.data_race {
-                data_race.validate_lock_release(
-                    &mut rwlock.data_race,
-                    current_writer,
-                    current_span,
-                );
+                rwlock
+                    .clock_unlocked
+                    .clone_from(&*data_race.release_clock(current_writer, current_span));
             }
             // The thread was a writer.
             //
@@ -611,11 +597,11 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
 
         // Each condvar signal happens-before the end of the condvar wake
         if let Some(data_race) = data_race {
-            data_race.validate_lock_release(&mut condvar.data_race, current_thread, current_span);
+            condvar.clock.clone_from(&*data_race.release_clock(current_thread, current_span));
         }
         condvar.waiters.pop_front().map(|waiter| {
             if let Some(data_race) = data_race {
-                data_race.validate_lock_acquire(&condvar.data_race, waiter.thread);
+                data_race.acquire_clock(&condvar.clock, waiter.thread);
             }
             (waiter.thread, waiter.lock)
         })
@@ -645,14 +631,14 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
 
         // Each futex-wake happens-before the end of the futex wait
         if let Some(data_race) = data_race {
-            data_race.validate_lock_release(&mut futex.data_race, current_thread, current_span);
+            futex.clock.clone_from(&*data_race.release_clock(current_thread, current_span));
         }
 
         // Wake up the first thread in the queue that matches any of the bits in the bitset.
         futex.waiters.iter().position(|w| w.bitset & bitset != 0).map(|i| {
             let waiter = futex.waiters.remove(i).unwrap();
             if let Some(data_race) = data_race {
-                data_race.validate_lock_acquire(&futex.data_race, waiter.thread);
+                data_race.acquire_clock(&futex.clock, waiter.thread);
             }
             waiter.thread
         })

src/tools/miri/src/concurrency/vector_clock.rs

@@ -4,7 +4,7 @@ use smallvec::SmallVec;
 use std::{
     cmp::Ordering,
     fmt::Debug,
-    ops::{Index, IndexMut, Shr},
+    ops::{Index, Shr},
 };
 
 use super::data_race::NaReadType;
@@ -92,7 +92,7 @@ impl VTimestamp {
     }
 
     #[inline]
-    pub fn set_read_type(&mut self, read_type: NaReadType) {
+    pub(super) fn set_read_type(&mut self, read_type: NaReadType) {
         self.time_and_read_type = Self::encode_time_and_read_type(self.time(), read_type);
     }
 
@@ -138,7 +138,7 @@ pub struct VClock(SmallVec<[VTimestamp; SMALL_VECTOR]>);
 impl VClock {
     /// Create a new vector-clock containing all zeros except
     /// for a value at the given index
-    pub fn new_with_index(index: VectorIdx, timestamp: VTimestamp) -> VClock {
+    pub(super) fn new_with_index(index: VectorIdx, timestamp: VTimestamp) -> VClock {
         let len = index.index() + 1;
         let mut vec = smallvec::smallvec![VTimestamp::ZERO; len];
         vec[index.index()] = timestamp;
@@ -147,12 +147,18 @@ impl VClock {
 
     /// Load the internal timestamp slice in the vector clock
     #[inline]
-    pub fn as_slice(&self) -> &[VTimestamp] {
+    pub(super) fn as_slice(&self) -> &[VTimestamp] {
+        debug_assert!(!self.0.last().is_some_and(|t| t.time() == 0));
         self.0.as_slice()
     }
 
+    #[inline]
+    pub(super) fn index_mut(&mut self, index: VectorIdx) -> &mut VTimestamp {
+        self.0.as_mut_slice().get_mut(index.to_u32() as usize).unwrap()
+    }
+
     /// Get a mutable slice to the internal vector with minimum `min_len`
-    /// elements, to preserve invariants this vector must modify
+    /// elements. To preserve invariants, the caller must modify
     /// the `min_len`-1 nth element to a non-zero value
     #[inline]
     fn get_mut_with_min_len(&mut self, min_len: usize) -> &mut [VTimestamp] {
@@ -166,7 +172,7 @@ impl VClock {
     /// Increment the vector clock at a known index
     /// this will panic if the vector index overflows
     #[inline]
-    pub fn increment_index(&mut self, idx: VectorIdx, current_span: Span) {
+    pub(super) fn increment_index(&mut self, idx: VectorIdx, current_span: Span) {
         let idx = idx.index();
         let mut_slice = self.get_mut_with_min_len(idx + 1);
         let idx_ref = &mut mut_slice[idx];
@@ -190,28 +196,36 @@ impl VClock {
         }
     }
 
-    /// Set the element at the current index of the vector
-    pub fn set_at_index(&mut self, other: &Self, idx: VectorIdx) {
+    /// Set the element at the current index of the vector. May only increase elements.
+    pub(super) fn set_at_index(&mut self, other: &Self, idx: VectorIdx) {
+        let new_timestamp = other[idx];
+        // Setting to 0 is different, since the last element cannot be 0.
+        if new_timestamp.time() == 0 {
+            if idx.index() >= self.0.len() {
+                // This index does not even exist yet in our clock. Just do nothing.
+                return;
+            }
+            // This changes an existing element. Since it can only increase, that
+            // can never make the last element 0.
+        }
+
         let mut_slice = self.get_mut_with_min_len(idx.index() + 1);
+        let mut_timestamp = &mut mut_slice[idx.index()];
 
-        let prev_span = mut_slice[idx.index()].span;
+        let prev_span = mut_timestamp.span;
 
-        mut_slice[idx.index()] = other[idx];
+        assert!(*mut_timestamp <= new_timestamp, "set_at_index: may only increase the timestamp");
+        *mut_timestamp = new_timestamp;
 
-        let span = &mut mut_slice[idx.index()].span;
+        let span = &mut mut_timestamp.span;
         *span = span.substitute_dummy(prev_span);
     }
 
     /// Set the vector to the all-zero vector
     #[inline]
-    pub fn set_zero_vector(&mut self) {
+    pub(super) fn set_zero_vector(&mut self) {
         self.0.clear();
     }
-
-    /// Return if this vector is the all-zero vector
-    pub fn is_zero_vector(&self) -> bool {
-        self.0.is_empty()
-    }
 }
 
 impl Clone for VClock {
@@ -407,13 +421,6 @@ impl Index<VectorIdx> for VClock {
     }
 }
 
-impl IndexMut<VectorIdx> for VClock {
-    #[inline]
-    fn index_mut(&mut self, index: VectorIdx) -> &mut VTimestamp {
-        self.0.as_mut_slice().get_mut(index.to_u32() as usize).unwrap()
-    }
-}
-
 /// Test vector clock ordering operations
 ///  data-race detection is tested in the external
 ///  test suite
@@ -553,4 +560,15 @@ mod tests {
             "Invalid alt (>=):\n l: {l:?}\n r: {r:?}"
         );
     }
+
+    #[test]
+    fn set_index_to_0() {
+        let mut clock1 = from_slice(&[0, 1, 2, 3]);
+        let clock2 = from_slice(&[0, 2, 3, 4, 0, 5]);
+        // Naively, this would extend clock1 with a new index and set it to 0, making
+        // the last index 0. Make sure that does not happen.
+        clock1.set_at_index(&clock2, VectorIdx(4));
+        // This must not have made the last element 0.
+        assert!(clock1.0.last().unwrap().time() != 0);
+    }
 }