From 424bc60a1378ceac6ff59734633d466dd912ab8f Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 11 Nov 2024 08:02:28 +0100 Subject: [PATCH 1/2] sync support: dont implicitly clone inside the general sync machinery --- src/tools/miri/src/concurrency/sync.rs | 38 +++++++++++--------- src/tools/miri/src/shims/unix/macos/sync.rs | 16 +++++++-- src/tools/miri/src/shims/unix/sync.rs | 39 +++++++++++++-------- src/tools/miri/src/shims/windows/sync.rs | 9 +++-- 4 files changed, 64 insertions(+), 38 deletions(-) diff --git a/src/tools/miri/src/concurrency/sync.rs b/src/tools/miri/src/concurrency/sync.rs index 331557ab3f882..ef4034cc0c11e 100644 --- a/src/tools/miri/src/concurrency/sync.rs +++ b/src/tools/miri/src/concurrency/sync.rs @@ -221,12 +221,16 @@ impl<'tcx> EvalContextExt<'tcx> for crate::MiriInterpCx<'tcx> {} pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { /// Helper for lazily initialized `alloc_extra.sync` data: /// this forces an immediate init. - fn lazy_sync_init( - &mut self, + /// Return a reference to the data in the machine state. + fn lazy_sync_init<'a, T: 'static>( + &'a mut self, primitive: &MPlaceTy<'tcx>, init_offset: Size, data: T, - ) -> InterpResult<'tcx> { + ) -> InterpResult<'tcx, &'a T> + where + 'tcx: 'a, + { let this = self.eval_context_mut(); let (alloc, offset, _) = this.ptr_get_alloc_id(primitive.ptr(), 0)?; @@ -239,7 +243,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { &init_field, AtomicWriteOrd::Relaxed, )?; - interp_ok(()) + interp_ok(this.get_alloc_extra(alloc)?.get_sync::(offset).unwrap()) } /// Helper for lazily initialized `alloc_extra.sync` data: @@ -248,15 +252,17 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { /// and stores that in `alloc_extra.sync`. /// - Otherwise, calls `new_data` to initialize the primitive. /// - /// The return value is a *clone* of the stored data, so if you intend to mutate it - /// better wrap everything into an `Rc`. - fn lazy_sync_get_data( - &mut self, + /// Return a reference to the data in the machine state. + fn lazy_sync_get_data<'a, T: 'static>( + &'a mut self, primitive: &MPlaceTy<'tcx>, init_offset: Size, missing_data: impl FnOnce() -> InterpResult<'tcx, T>, new_data: impl FnOnce(&mut MiriInterpCx<'tcx>) -> InterpResult<'tcx, T>, - ) -> InterpResult<'tcx, T> { + ) -> InterpResult<'tcx, &'a T> + where + 'tcx: 'a, + { let this = self.eval_context_mut(); // Check if this is already initialized. Needs to be atomic because we can race with another @@ -280,17 +286,15 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { // or else it has been moved illegally. let (alloc, offset, _) = this.ptr_get_alloc_id(primitive.ptr(), 0)?; let (alloc_extra, _machine) = this.get_alloc_extra_mut(alloc)?; - if let Some(data) = alloc_extra.get_sync::(offset) { - interp_ok(data.clone()) - } else { + // Due to borrow checker reasons, we have to do the lookup twice. + if alloc_extra.get_sync::(offset).is_none() { let data = missing_data()?; - alloc_extra.sync.insert(offset, Box::new(data.clone())); - interp_ok(data) + alloc_extra.sync.insert(offset, Box::new(data)); } + interp_ok(alloc_extra.get_sync::(offset).unwrap()) } else { let data = new_data(this)?; - this.lazy_sync_init(primitive, init_offset, data.clone())?; - interp_ok(data) + this.lazy_sync_init(primitive, init_offset, data) } } @@ -326,7 +330,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { #[inline] /// Get the id of the thread that currently owns this lock. - fn mutex_get_owner(&mut self, mutex_ref: &MutexRef) -> ThreadId { + fn mutex_get_owner(&self, mutex_ref: &MutexRef) -> ThreadId { mutex_ref.0.borrow().owner.unwrap() } diff --git a/src/tools/miri/src/shims/unix/macos/sync.rs b/src/tools/miri/src/shims/unix/macos/sync.rs index 3f3f54fdca8a3..f66a57ae7061c 100644 --- a/src/tools/miri/src/shims/unix/macos/sync.rs +++ b/src/tools/miri/src/shims/unix/macos/sync.rs @@ -22,10 +22,13 @@ enum MacOsUnfairLock { impl<'tcx> EvalContextExtPriv<'tcx> for crate::MiriInterpCx<'tcx> {} trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> { - fn os_unfair_lock_get_data( - &mut self, + fn os_unfair_lock_get_data<'a>( + &'a mut self, lock_ptr: &OpTy<'tcx>, - ) -> InterpResult<'tcx, MacOsUnfairLock> { + ) -> InterpResult<'tcx, &'a MacOsUnfairLock> + where + 'tcx: 'a, + { let this = self.eval_context_mut(); let lock = this.deref_pointer(lock_ptr)?; this.lazy_sync_get_data( @@ -68,6 +71,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { ); return interp_ok(()); }; + let mutex_ref = mutex_ref.clone(); if this.mutex_is_locked(&mutex_ref) { if this.mutex_get_owner(&mutex_ref) == this.active_thread() { @@ -97,6 +101,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { this.write_scalar(Scalar::from_bool(false), dest)?; return interp_ok(()); }; + let mutex_ref = mutex_ref.clone(); if this.mutex_is_locked(&mutex_ref) { // Contrary to the blocking lock function, this does not check for @@ -119,6 +124,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { "attempted to unlock an os_unfair_lock not owned by the current thread".to_owned() )); }; + let mutex_ref = mutex_ref.clone(); // Now, unlock. if this.mutex_unlock(&mutex_ref)?.is_none() { @@ -147,6 +153,8 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { "called os_unfair_lock_assert_owner on an os_unfair_lock not owned by the current thread".to_owned() )); }; + let mutex_ref = mutex_ref.clone(); + if !this.mutex_is_locked(&mutex_ref) || this.mutex_get_owner(&mutex_ref) != this.active_thread() { @@ -167,6 +175,8 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { // The lock is poisoned, who knows who owns it... we'll pretend: someone else. return interp_ok(()); }; + let mutex_ref = mutex_ref.clone(); + if this.mutex_is_locked(&mutex_ref) && this.mutex_get_owner(&mutex_ref) == this.active_thread() { diff --git a/src/tools/miri/src/shims/unix/sync.rs b/src/tools/miri/src/shims/unix/sync.rs index 013d62be4c9df..e9d738d9ac010 100644 --- a/src/tools/miri/src/shims/unix/sync.rs +++ b/src/tools/miri/src/shims/unix/sync.rs @@ -185,7 +185,10 @@ fn mutex_create<'tcx>( fn mutex_get_data<'tcx, 'a>( ecx: &'a mut MiriInterpCx<'tcx>, mutex_ptr: &OpTy<'tcx>, -) -> InterpResult<'tcx, PthreadMutex> { +) -> InterpResult<'tcx, &'a PthreadMutex> +where + 'tcx: 'a, +{ let mutex = ecx.deref_pointer(mutex_ptr)?; ecx.lazy_sync_get_data( &mutex, @@ -259,10 +262,13 @@ fn rwlock_init_offset<'tcx>(ecx: &MiriInterpCx<'tcx>) -> InterpResult<'tcx, Size interp_ok(offset) } -fn rwlock_get_data<'tcx>( - ecx: &mut MiriInterpCx<'tcx>, +fn rwlock_get_data<'tcx, 'a>( + ecx: &'a mut MiriInterpCx<'tcx>, rwlock_ptr: &OpTy<'tcx>, -) -> InterpResult<'tcx, PthreadRwLock> { +) -> InterpResult<'tcx, &'a PthreadRwLock> +where + 'tcx: 'a, +{ let rwlock = ecx.deref_pointer(rwlock_ptr)?; ecx.lazy_sync_get_data( &rwlock, @@ -389,10 +395,13 @@ fn cond_create<'tcx>( interp_ok(data) } -fn cond_get_data<'tcx>( - ecx: &mut MiriInterpCx<'tcx>, +fn cond_get_data<'tcx, 'a>( + ecx: &'a mut MiriInterpCx<'tcx>, cond_ptr: &OpTy<'tcx>, -) -> InterpResult<'tcx, PthreadCondvar> { +) -> InterpResult<'tcx, &'a PthreadCondvar> +where + 'tcx: 'a, +{ let cond = ecx.deref_pointer(cond_ptr)?; ecx.lazy_sync_get_data( &cond, @@ -498,7 +507,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { ) -> InterpResult<'tcx> { let this = self.eval_context_mut(); - let mutex = mutex_get_data(this, mutex_op)?; + let mutex = mutex_get_data(this, mutex_op)?.clone(); let ret = if this.mutex_is_locked(&mutex.mutex_ref) { let owner_thread = this.mutex_get_owner(&mutex.mutex_ref); @@ -535,7 +544,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { fn pthread_mutex_trylock(&mut self, mutex_op: &OpTy<'tcx>) -> InterpResult<'tcx, Scalar> { let this = self.eval_context_mut(); - let mutex = mutex_get_data(this, mutex_op)?; + let mutex = mutex_get_data(this, mutex_op)?.clone(); interp_ok(Scalar::from_i32(if this.mutex_is_locked(&mutex.mutex_ref) { let owner_thread = this.mutex_get_owner(&mutex.mutex_ref); @@ -561,7 +570,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { fn pthread_mutex_unlock(&mut self, mutex_op: &OpTy<'tcx>) -> InterpResult<'tcx, Scalar> { let this = self.eval_context_mut(); - let mutex = mutex_get_data(this, mutex_op)?; + let mutex = mutex_get_data(this, mutex_op)?.clone(); if let Some(_old_locked_count) = this.mutex_unlock(&mutex.mutex_ref)? { // The mutex was locked by the current thread. @@ -590,7 +599,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { // Reading the field also has the side-effect that we detect double-`destroy` // since we make the field unint below. - let mutex = mutex_get_data(this, mutex_op)?; + let mutex = mutex_get_data(this, mutex_op)?.clone(); if this.mutex_is_locked(&mutex.mutex_ref) { throw_ub_format!("destroyed a locked mutex"); @@ -822,8 +831,8 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { ) -> InterpResult<'tcx> { let this = self.eval_context_mut(); - let data = cond_get_data(this, cond_op)?; - let mutex_ref = mutex_get_data(this, mutex_op)?.mutex_ref; + let data = *cond_get_data(this, cond_op)?; + let mutex_ref = mutex_get_data(this, mutex_op)?.mutex_ref.clone(); this.condvar_wait( data.id, @@ -846,8 +855,8 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { ) -> InterpResult<'tcx> { let this = self.eval_context_mut(); - let data = cond_get_data(this, cond_op)?; - let mutex_ref = mutex_get_data(this, mutex_op)?.mutex_ref; + let data = *cond_get_data(this, cond_op)?; + let mutex_ref = mutex_get_data(this, mutex_op)?.mutex_ref.clone(); // Extract the timeout. let duration = match this diff --git a/src/tools/miri/src/shims/windows/sync.rs b/src/tools/miri/src/shims/windows/sync.rs index b03dedea146c4..a394e0430bcd0 100644 --- a/src/tools/miri/src/shims/windows/sync.rs +++ b/src/tools/miri/src/shims/windows/sync.rs @@ -20,10 +20,13 @@ trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> { // Windows sync primitives are pointer sized. // We only use the first 4 bytes for the id. - fn init_once_get_data( - &mut self, + fn init_once_get_data<'a>( + &'a mut self, init_once_ptr: &OpTy<'tcx>, - ) -> InterpResult<'tcx, WindowsInitOnce> { + ) -> InterpResult<'tcx, &'a WindowsInitOnce> + where + 'tcx: 'a, + { let this = self.eval_context_mut(); let init_once = this.deref_pointer(init_once_ptr)?; From ad16dc4b878f1caa7b755887c395f95b114f3002 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 11 Nov 2024 08:04:38 +0100 Subject: [PATCH 2/2] typo --- src/tools/miri/src/shims/unix/sync.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/tools/miri/src/shims/unix/sync.rs b/src/tools/miri/src/shims/unix/sync.rs index e9d738d9ac010..416cf020dcc99 100644 --- a/src/tools/miri/src/shims/unix/sync.rs +++ b/src/tools/miri/src/shims/unix/sync.rs @@ -598,7 +598,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let this = self.eval_context_mut(); // Reading the field also has the side-effect that we detect double-`destroy` - // since we make the field unint below. + // since we make the field uninit below. let mutex = mutex_get_data(this, mutex_op)?.clone(); if this.mutex_is_locked(&mutex.mutex_ref) { @@ -706,7 +706,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let this = self.eval_context_mut(); // Reading the field also has the side-effect that we detect double-`destroy` - // since we make the field unint below. + // since we make the field uninit below. let id = rwlock_get_data(this, rwlock_op)?.id; if this.rwlock_is_locked(id) { @@ -893,7 +893,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let this = self.eval_context_mut(); // Reading the field also has the side-effect that we detect double-`destroy` - // since we make the field unint below. + // since we make the field uninit below. let id = cond_get_data(this, cond_op)?.id; if this.condvar_is_awaited(id) { throw_ub_format!("destroying an awaited conditional variable");