diff --git a/src/alloc_addresses/mod.rs b/src/alloc_addresses/mod.rs index 612649c90c..2bbb34c9a4 100644 --- a/src/alloc_addresses/mod.rs +++ b/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 }) } } diff --git a/src/alloc_addresses/reuse_pool.rs b/src/alloc_addresses/reuse_pool.rs index fd0b24cc29..77fc9f53f9 100644 --- a/src/alloc_addresses/reuse_pool.rs +++ b/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)> { // 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) })) } } diff --git a/src/concurrency/data_race.rs b/src/concurrency/data_race.rs index 95049b91cb..2281609a04 100644 --- a/src/concurrency/data_race.rs +++ b/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 diff --git a/src/concurrency/init_once.rs b/src/concurrency/init_once.rs index 9dbea08f3e..a01b59c916 100644 --- a/src/concurrency/init_once.rs +++ b/src/concurrency/init_once.rs @@ -41,7 +41,7 @@ pub enum InitOnceStatus { pub(super) struct InitOnce<'mir, 'tcx> { status: InitOnceStatus, waiters: VecDeque>, - 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. diff --git a/src/concurrency/sync.rs b/src/concurrency/sync.rs index 0a42871569..d3cef8bf5f 100644 --- a/src/concurrency/sync.rs +++ b/src/concurrency/sync.rs @@ -69,12 +69,8 @@ struct Mutex { lock_count: usize, /// The queue of threads waiting for this mutex. queue: VecDeque, - /// 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, /// The queue of reader threads waiting for this lock. reader_queue: VecDeque, - /// 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 }) diff --git a/src/concurrency/vector_clock.rs b/src/concurrency/vector_clock.rs index fe719943dc..2cd3d031b1 100644 --- a/src/concurrency/vector_clock.rs +++ b/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 for VClock { } } -impl IndexMut 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); + } }