From 1a7f6d504a12f826b59c2cee3067fe02701aef04 Mon Sep 17 00:00:00 2001 From: Andy Wang Date: Sat, 7 May 2022 21:30:15 +0100 Subject: [PATCH 1/4] Use proper atomic rmw for {mutex, rwlock, cond, srwlock}_get_or_create_id --- src/data_race.rs | 29 ++++++++++++------- src/shims/posix/sync.rs | 61 +++++++++++++++++++++++++++------------ src/shims/windows/sync.rs | 21 ++++++++++---- src/sync.rs | 21 ++++++++++++++ 4 files changed, 97 insertions(+), 35 deletions(-) diff --git a/src/data_race.rs b/src/data_race.rs index d249d28d03..b7ccaa3530 100644 --- a/src/data_race.rs +++ b/src/data_race.rs @@ -441,21 +441,33 @@ impl MemoryCellClocks { /// Evaluation context extensions. impl<'mir, 'tcx: 'mir> EvalContextExt<'mir, 'tcx> for MiriEvalContext<'mir, 'tcx> {} pub trait EvalContextExt<'mir, 'tcx: 'mir>: MiriEvalContextExt<'mir, 'tcx> { - /// Atomic variant of read_scalar_at_offset. - fn read_scalar_at_offset_atomic( + /// Calculates the MPlaceTy given the offset and layout of an access on an operand + fn offset_and_layout_to_place( &self, op: &OpTy<'tcx, Tag>, offset: u64, layout: TyAndLayout<'tcx>, - atomic: AtomicReadOp, - ) -> InterpResult<'tcx, ScalarMaybeUninit> { + ) -> InterpResult<'tcx, MPlaceTy<'tcx, Tag>> { let this = self.eval_context_ref(); let op_place = this.deref_operand(op)?; let offset = Size::from_bytes(offset); - // Ensure that the following read at an offset is within bounds. + // Ensure that the access is within bounds. assert!(op_place.layout.size >= offset + layout.size); let value_place = op_place.offset(offset, MemPlaceMeta::None, layout, this)?; + Ok(value_place) + } + + /// Atomic variant of read_scalar_at_offset. + fn read_scalar_at_offset_atomic( + &self, + op: &OpTy<'tcx, Tag>, + offset: u64, + layout: TyAndLayout<'tcx>, + atomic: AtomicReadOp, + ) -> InterpResult<'tcx, ScalarMaybeUninit> { + let this = self.eval_context_ref(); + let value_place = this.offset_and_layout_to_place(op, offset, layout)?; this.read_scalar_atomic(&value_place, atomic) } @@ -469,12 +481,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: MiriEvalContextExt<'mir, 'tcx> { atomic: AtomicWriteOp, ) -> InterpResult<'tcx> { let this = self.eval_context_mut(); - let op_place = this.deref_operand(op)?; - let offset = Size::from_bytes(offset); - - // Ensure that the following read at an offset is within bounds. - assert!(op_place.layout.size >= offset + layout.size); - let value_place = op_place.offset(offset, MemPlaceMeta::None, layout, this)?; + let value_place = this.offset_and_layout_to_place(op, offset, layout)?; this.write_scalar_atomic(value.into(), &value_place, atomic) } diff --git a/src/shims/posix/sync.rs b/src/shims/posix/sync.rs index ea940df1c6..f8c680c0e8 100644 --- a/src/shims/posix/sync.rs +++ b/src/shims/posix/sync.rs @@ -112,15 +112,23 @@ fn mutex_get_or_create_id<'mir, 'tcx: 'mir>( ecx: &mut MiriEvalContext<'mir, 'tcx>, mutex_op: &OpTy<'tcx, Tag>, ) -> InterpResult<'tcx, MutexId> { - let id = mutex_get_id(ecx, mutex_op)?.to_u32()?; - if id == 0 { - // 0 is a default value and also not a valid mutex id. Need to allocate - // a new mutex. + let value_place = ecx.offset_and_layout_to_place(mutex_op, 4, ecx.machine.layouts.u32)?; + let (old, success) = ecx + .atomic_compare_exchange_scalar( + &value_place, + &ImmTy::from_uint(0u32, ecx.machine.layouts.u32), + ecx.mutex_next_id().to_u32_scalar().into(), + AtomicRwOp::Relaxed, + AtomicReadOp::Relaxed, + false, + )? + .to_scalar_pair()?; + + if success.to_bool().expect("compare_exchange's second return value is a bool") { let id = ecx.mutex_create(); - mutex_set_id(ecx, mutex_op, id.to_u32_scalar())?; Ok(id) } else { - Ok(MutexId::from_u32(id)) + Ok(MutexId::from_u32(old.to_u32().expect("layout is u32"))) } } @@ -156,15 +164,23 @@ fn rwlock_get_or_create_id<'mir, 'tcx: 'mir>( ecx: &mut MiriEvalContext<'mir, 'tcx>, rwlock_op: &OpTy<'tcx, Tag>, ) -> InterpResult<'tcx, RwLockId> { - let id = rwlock_get_id(ecx, rwlock_op)?.to_u32()?; - if id == 0 { - // 0 is a default value and also not a valid rwlock id. Need to allocate - // a new read-write lock. + let value_place = ecx.offset_and_layout_to_place(rwlock_op, 4, ecx.machine.layouts.u32)?; + let (old, success) = ecx + .atomic_compare_exchange_scalar( + &value_place, + &ImmTy::from_uint(0u32, ecx.machine.layouts.u32), + ecx.rwlock_next_id().to_u32_scalar().into(), + AtomicRwOp::Relaxed, + AtomicReadOp::Relaxed, + false, + )? + .to_scalar_pair()?; + + if success.to_bool().expect("compare_exchange's second return value is a bool") { let id = ecx.rwlock_create(); - rwlock_set_id(ecx, rwlock_op, id.to_u32_scalar())?; Ok(id) } else { - Ok(RwLockId::from_u32(id)) + Ok(RwLockId::from_u32(old.to_u32().expect("layout is u32"))) } } @@ -228,15 +244,24 @@ fn cond_get_or_create_id<'mir, 'tcx: 'mir>( ecx: &mut MiriEvalContext<'mir, 'tcx>, cond_op: &OpTy<'tcx, Tag>, ) -> InterpResult<'tcx, CondvarId> { - let id = cond_get_id(ecx, cond_op)?.to_u32()?; - if id == 0 { - // 0 is a default value and also not a valid conditional variable id. - // Need to allocate a new id. + let value_place = ecx.offset_and_layout_to_place(cond_op, 4, ecx.machine.layouts.u32)?; + + let (old, success) = ecx + .atomic_compare_exchange_scalar( + &value_place, + &ImmTy::from_uint(0u32, ecx.machine.layouts.u32), + ecx.condvar_next_id().to_u32_scalar().into(), + AtomicRwOp::Relaxed, + AtomicReadOp::Relaxed, + false, + )? + .to_scalar_pair()?; + + if success.to_bool().expect("compare_exchange's second return value is a bool") { let id = ecx.condvar_create(); - cond_set_id(ecx, cond_op, id.to_u32_scalar())?; Ok(id) } else { - Ok(CondvarId::from_u32(id)) + Ok(CondvarId::from_u32(old.to_u32().expect("layout is u32"))) } } diff --git a/src/shims/windows/sync.rs b/src/shims/windows/sync.rs index 78458dc6c9..ccd65aac90 100644 --- a/src/shims/windows/sync.rs +++ b/src/shims/windows/sync.rs @@ -7,15 +7,24 @@ fn srwlock_get_or_create_id<'mir, 'tcx: 'mir>( ecx: &mut MiriEvalContext<'mir, 'tcx>, lock_op: &OpTy<'tcx, Tag>, ) -> InterpResult<'tcx, RwLockId> { - let id = ecx.read_scalar_at_offset(lock_op, 0, ecx.machine.layouts.u32)?.to_u32()?; - if id == 0 { - // 0 is a default value and also not a valid rwlock id. Need to allocate - // a new rwlock. + let value_place = ecx.offset_and_layout_to_place(lock_op, 0, ecx.machine.layouts.u32)?; + + let (old, success) = ecx + .atomic_compare_exchange_scalar( + &value_place, + &ImmTy::from_uint(0u32, ecx.machine.layouts.u32), + ecx.rwlock_next_id().to_u32_scalar().into(), + AtomicRwOp::AcqRel, + AtomicReadOp::Acquire, + false, + )? + .to_scalar_pair()?; + + if success.to_bool().expect("compare_exchange's second return value is a bool") { let id = ecx.rwlock_create(); - ecx.write_scalar_at_offset(lock_op, 0, id.to_u32_scalar(), ecx.machine.layouts.u32)?; Ok(id) } else { - Ok(RwLockId::from_u32(id)) + Ok(RwLockId::from_u32(old.to_u32().expect("layout is u32"))) } } diff --git a/src/sync.rs b/src/sync.rs index ac1687a22e..8c5b8ebfec 100644 --- a/src/sync.rs +++ b/src/sync.rs @@ -208,6 +208,13 @@ trait EvalContextExtPriv<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx // situations. impl<'mir, 'tcx: 'mir> EvalContextExt<'mir, 'tcx> for crate::MiriEvalContext<'mir, 'tcx> {} pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx> { + #[inline] + /// Peek the id of the next mutex + fn mutex_next_id(&self) -> MutexId { + let this = self.eval_context_ref(); + this.machine.threads.sync.mutexes.next_index() + } + #[inline] /// Create state for a new mutex. fn mutex_create(&mut self) -> MutexId { @@ -290,6 +297,13 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx this.block_thread(thread); } + #[inline] + /// Peek the id of the next read write lock + fn rwlock_next_id(&self) -> RwLockId { + let this = self.eval_context_ref(); + this.machine.threads.sync.rwlocks.next_index() + } + #[inline] /// Create state for a new read write lock. fn rwlock_create(&mut self) -> RwLockId { @@ -438,6 +452,13 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx this.block_thread(writer); } + #[inline] + /// Peek the id of the next Condvar + fn condvar_next_id(&self) -> CondvarId { + let this = self.eval_context_ref(); + this.machine.threads.sync.condvars.next_index() + } + #[inline] /// Create state for a new conditional variable. fn condvar_create(&mut self) -> CondvarId { From a5db2c32e5f0b29c730451a80efc0b4751ca208e Mon Sep 17 00:00:00 2001 From: Andy Wang Date: Thu, 12 May 2022 20:31:40 +0100 Subject: [PATCH 2/4] Refactor to hide *_next_id functions --- src/shims/posix/sync.rs | 113 +++++++++++++++++++++----------------- src/shims/windows/sync.rs | 37 +++++++------ src/sync.rs | 72 +++++++++++++++++------- 3 files changed, 133 insertions(+), 89 deletions(-) diff --git a/src/shims/posix/sync.rs b/src/shims/posix/sync.rs index f8c680c0e8..2e5da5b537 100644 --- a/src/shims/posix/sync.rs +++ b/src/shims/posix/sync.rs @@ -113,23 +113,27 @@ fn mutex_get_or_create_id<'mir, 'tcx: 'mir>( mutex_op: &OpTy<'tcx, Tag>, ) -> InterpResult<'tcx, MutexId> { let value_place = ecx.offset_and_layout_to_place(mutex_op, 4, ecx.machine.layouts.u32)?; - let (old, success) = ecx - .atomic_compare_exchange_scalar( - &value_place, - &ImmTy::from_uint(0u32, ecx.machine.layouts.u32), - ecx.mutex_next_id().to_u32_scalar().into(), - AtomicRwOp::Relaxed, - AtomicReadOp::Relaxed, - false, - )? - .to_scalar_pair()?; - - if success.to_bool().expect("compare_exchange's second return value is a bool") { - let id = ecx.mutex_create(); - Ok(id) - } else { - Ok(MutexId::from_u32(old.to_u32().expect("layout is u32"))) - } + + ecx.mutex_get_or_create(|ecx, next_id| { + let (old, success) = ecx + .atomic_compare_exchange_scalar( + &value_place, + &ImmTy::from_uint(0u32, ecx.machine.layouts.u32), + next_id.to_u32_scalar().into(), + AtomicRwOp::Relaxed, + AtomicReadOp::Relaxed, + false, + )? + .to_scalar_pair() + .expect("compare_exchange returns a scalar pair"); + + Ok(if success.to_bool().expect("compare_exchange's second return value is a bool") { + // Caller of the closure needs to allocate next_id + None + } else { + Some(MutexId::from_u32(old.to_u32().expect("layout is u32"))) + }) + }) } // pthread_rwlock_t is between 32 and 56 bytes, depending on the platform. @@ -165,23 +169,27 @@ fn rwlock_get_or_create_id<'mir, 'tcx: 'mir>( rwlock_op: &OpTy<'tcx, Tag>, ) -> InterpResult<'tcx, RwLockId> { let value_place = ecx.offset_and_layout_to_place(rwlock_op, 4, ecx.machine.layouts.u32)?; - let (old, success) = ecx - .atomic_compare_exchange_scalar( - &value_place, - &ImmTy::from_uint(0u32, ecx.machine.layouts.u32), - ecx.rwlock_next_id().to_u32_scalar().into(), - AtomicRwOp::Relaxed, - AtomicReadOp::Relaxed, - false, - )? - .to_scalar_pair()?; - - if success.to_bool().expect("compare_exchange's second return value is a bool") { - let id = ecx.rwlock_create(); - Ok(id) - } else { - Ok(RwLockId::from_u32(old.to_u32().expect("layout is u32"))) - } + + ecx.rwlock_get_or_create(|ecx, next_id| { + let (old, success) = ecx + .atomic_compare_exchange_scalar( + &value_place, + &ImmTy::from_uint(0u32, ecx.machine.layouts.u32), + next_id.to_u32_scalar().into(), + AtomicRwOp::Relaxed, + AtomicReadOp::Relaxed, + false, + )? + .to_scalar_pair() + .expect("compare_exchange returns a scalar pair"); + + Ok(if success.to_bool().expect("compare_exchange's second return value is a bool") { + // Caller of the closure needs to allocate next_id + None + } else { + Some(RwLockId::from_u32(old.to_u32().expect("layout is u32"))) + }) + }) } // pthread_condattr_t @@ -246,23 +254,26 @@ fn cond_get_or_create_id<'mir, 'tcx: 'mir>( ) -> InterpResult<'tcx, CondvarId> { let value_place = ecx.offset_and_layout_to_place(cond_op, 4, ecx.machine.layouts.u32)?; - let (old, success) = ecx - .atomic_compare_exchange_scalar( - &value_place, - &ImmTy::from_uint(0u32, ecx.machine.layouts.u32), - ecx.condvar_next_id().to_u32_scalar().into(), - AtomicRwOp::Relaxed, - AtomicReadOp::Relaxed, - false, - )? - .to_scalar_pair()?; - - if success.to_bool().expect("compare_exchange's second return value is a bool") { - let id = ecx.condvar_create(); - Ok(id) - } else { - Ok(CondvarId::from_u32(old.to_u32().expect("layout is u32"))) - } + ecx.condvar_get_or_create(|ecx, next_id| { + let (old, success) = ecx + .atomic_compare_exchange_scalar( + &value_place, + &ImmTy::from_uint(0u32, ecx.machine.layouts.u32), + next_id.to_u32_scalar().into(), + AtomicRwOp::Relaxed, + AtomicReadOp::Relaxed, + false, + )? + .to_scalar_pair() + .expect("compare_exchange returns a scalar pair"); + + Ok(if success.to_bool().expect("compare_exchange's second return value is a bool") { + // Caller of the closure needs to allocate next_id + None + } else { + Some(CondvarId::from_u32(old.to_u32().expect("layout is u32"))) + }) + }) } fn cond_get_clock_id<'mir, 'tcx: 'mir>( diff --git a/src/shims/windows/sync.rs b/src/shims/windows/sync.rs index ccd65aac90..ff10b3b6aa 100644 --- a/src/shims/windows/sync.rs +++ b/src/shims/windows/sync.rs @@ -9,23 +9,26 @@ fn srwlock_get_or_create_id<'mir, 'tcx: 'mir>( ) -> InterpResult<'tcx, RwLockId> { let value_place = ecx.offset_and_layout_to_place(lock_op, 0, ecx.machine.layouts.u32)?; - let (old, success) = ecx - .atomic_compare_exchange_scalar( - &value_place, - &ImmTy::from_uint(0u32, ecx.machine.layouts.u32), - ecx.rwlock_next_id().to_u32_scalar().into(), - AtomicRwOp::AcqRel, - AtomicReadOp::Acquire, - false, - )? - .to_scalar_pair()?; - - if success.to_bool().expect("compare_exchange's second return value is a bool") { - let id = ecx.rwlock_create(); - Ok(id) - } else { - Ok(RwLockId::from_u32(old.to_u32().expect("layout is u32"))) - } + ecx.rwlock_get_or_create(|ecx, next_id| { + let (old, success) = ecx + .atomic_compare_exchange_scalar( + &value_place, + &ImmTy::from_uint(0u32, ecx.machine.layouts.u32), + next_id.to_u32_scalar().into(), + AtomicRwOp::Relaxed, + AtomicReadOp::Relaxed, + false, + )? + .to_scalar_pair() + .expect("compare_exchange returns a scalar pair"); + + Ok(if success.to_bool().expect("compare_exchange's second return value is a bool") { + // Caller of the closure needs to allocate next_id + None + } else { + Some(RwLockId::from_u32(old.to_u32().expect("layout is u32"))) + }) + }) } impl<'mir, 'tcx> EvalContextExt<'mir, 'tcx> for crate::MiriEvalContext<'mir, 'tcx> {} diff --git a/src/sync.rs b/src/sync.rs index 8c5b8ebfec..fb69c67ecc 100644 --- a/src/sync.rs +++ b/src/sync.rs @@ -208,13 +208,6 @@ trait EvalContextExtPriv<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx // situations. impl<'mir, 'tcx: 'mir> EvalContextExt<'mir, 'tcx> for crate::MiriEvalContext<'mir, 'tcx> {} pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx> { - #[inline] - /// Peek the id of the next mutex - fn mutex_next_id(&self) -> MutexId { - let this = self.eval_context_ref(); - this.machine.threads.sync.mutexes.next_index() - } - #[inline] /// Create state for a new mutex. fn mutex_create(&mut self) -> MutexId { @@ -222,6 +215,21 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx this.machine.threads.sync.mutexes.push(Default::default()) } + #[inline] + /// Provides the closure with the next MutexId. Creates that mutex if the closure returns None, + /// otherwise returns the value from the closure + fn mutex_get_or_create(&mut self, existing: F) -> InterpResult<'tcx, MutexId> + where + F: FnOnce(&mut MiriEvalContext<'mir, 'tcx>, MutexId) -> InterpResult<'tcx, Option>, + { + let this = self.eval_context_mut(); + if let Some(old) = existing(this, this.machine.threads.sync.mutexes.next_index())? { + Ok(old) + } else { + Ok(self.mutex_create()) + } + } + #[inline] /// Get the id of the thread that currently owns this lock. fn mutex_get_owner(&mut self, id: MutexId) -> ThreadId { @@ -297,13 +305,6 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx this.block_thread(thread); } - #[inline] - /// Peek the id of the next read write lock - fn rwlock_next_id(&self) -> RwLockId { - let this = self.eval_context_ref(); - this.machine.threads.sync.rwlocks.next_index() - } - #[inline] /// Create state for a new read write lock. fn rwlock_create(&mut self) -> RwLockId { @@ -311,6 +312,24 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx this.machine.threads.sync.rwlocks.push(Default::default()) } + #[inline] + /// Provides the closure with the next RwLockId. Creates that RwLock if the closure returns None, + /// otherwise returns the value from the closure + fn rwlock_get_or_create(&mut self, existing: F) -> InterpResult<'tcx, RwLockId> + where + F: FnOnce( + &mut MiriEvalContext<'mir, 'tcx>, + RwLockId, + ) -> InterpResult<'tcx, Option>, + { + let this = self.eval_context_mut(); + if let Some(old) = existing(this, this.machine.threads.sync.rwlocks.next_index())? { + Ok(old) + } else { + Ok(self.rwlock_create()) + } + } + #[inline] /// Check if locked. fn rwlock_is_locked(&self, id: RwLockId) -> bool { @@ -452,13 +471,6 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx this.block_thread(writer); } - #[inline] - /// Peek the id of the next Condvar - fn condvar_next_id(&self) -> CondvarId { - let this = self.eval_context_ref(); - this.machine.threads.sync.condvars.next_index() - } - #[inline] /// Create state for a new conditional variable. fn condvar_create(&mut self) -> CondvarId { @@ -466,6 +478,24 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx this.machine.threads.sync.condvars.push(Default::default()) } + #[inline] + /// Provides the closure with the next CondvarId. Creates that Condvar if the closure returns None, + /// otherwise returns the value from the closure + fn condvar_get_or_create(&mut self, existing: F) -> InterpResult<'tcx, CondvarId> + where + F: FnOnce( + &mut MiriEvalContext<'mir, 'tcx>, + CondvarId, + ) -> InterpResult<'tcx, Option>, + { + let this = self.eval_context_mut(); + if let Some(old) = existing(this, this.machine.threads.sync.condvars.next_index())? { + Ok(old) + } else { + Ok(self.condvar_create()) + } + } + #[inline] /// Is the conditional variable awaited? fn condvar_is_awaited(&mut self, id: CondvarId) -> bool { From 10d978c180217f09c576822a312bc7353adfc17c Mon Sep 17 00:00:00 2001 From: Andy Wang Date: Thu, 12 May 2022 21:06:17 +0100 Subject: [PATCH 3/4] Inline _create() calls and add assertions --- src/sync.rs | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/src/sync.rs b/src/sync.rs index fb69c67ecc..0eebe4f654 100644 --- a/src/sync.rs +++ b/src/sync.rs @@ -223,10 +223,13 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx F: FnOnce(&mut MiriEvalContext<'mir, 'tcx>, MutexId) -> InterpResult<'tcx, Option>, { let this = self.eval_context_mut(); - if let Some(old) = existing(this, this.machine.threads.sync.mutexes.next_index())? { + let next_index = this.machine.threads.sync.mutexes.next_index(); + if let Some(old) = existing(this, next_index)? { Ok(old) } else { - Ok(self.mutex_create()) + let new_index = this.machine.threads.sync.mutexes.push(Default::default()); + assert_eq!(next_index, new_index); + Ok(new_index) } } @@ -323,10 +326,13 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx ) -> InterpResult<'tcx, Option>, { let this = self.eval_context_mut(); - if let Some(old) = existing(this, this.machine.threads.sync.rwlocks.next_index())? { + let next_index = this.machine.threads.sync.rwlocks.next_index(); + if let Some(old) = existing(this, next_index)? { Ok(old) } else { - Ok(self.rwlock_create()) + let new_index = this.machine.threads.sync.rwlocks.push(Default::default()); + assert_eq!(next_index, new_index); + Ok(new_index) } } @@ -489,10 +495,13 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx ) -> InterpResult<'tcx, Option>, { let this = self.eval_context_mut(); - if let Some(old) = existing(this, this.machine.threads.sync.condvars.next_index())? { + let next_index = this.machine.threads.sync.condvars.next_index(); + if let Some(old) = existing(this, next_index)? { Ok(old) } else { - Ok(self.condvar_create()) + let new_index = this.machine.threads.sync.condvars.push(Default::default()); + assert_eq!(next_index, new_index); + Ok(new_index) } } From 9e38dc4d499a6c405e0685d71235495699a34209 Mon Sep 17 00:00:00 2001 From: Andy Wang Date: Fri, 13 May 2022 18:42:53 +0100 Subject: [PATCH 4/4] Move and rename offset_and_layout_to_place to deref_operand_and_offset --- src/data_race.rs | 27 +++++---------------------- src/helpers.rs | 25 +++++++++++++++++-------- src/shims/posix/sync.rs | 6 +++--- src/shims/windows/sync.rs | 2 +- 4 files changed, 26 insertions(+), 34 deletions(-) diff --git a/src/data_race.rs b/src/data_race.rs index b7ccaa3530..b8656627e6 100644 --- a/src/data_race.rs +++ b/src/data_race.rs @@ -73,9 +73,9 @@ use rustc_middle::{mir, ty::layout::TyAndLayout}; use rustc_target::abi::Size; use crate::{ - AllocId, AllocRange, ImmTy, Immediate, InterpResult, MPlaceTy, MemPlaceMeta, MemoryKind, - MiriEvalContext, MiriEvalContextExt, MiriMemoryKind, OpTy, Pointer, RangeMap, Scalar, - ScalarMaybeUninit, Tag, ThreadId, VClock, VTimestamp, VectorIdx, + AllocId, AllocRange, HelpersEvalContextExt, ImmTy, Immediate, InterpResult, MPlaceTy, + MemoryKind, MiriEvalContext, MiriEvalContextExt, MiriMemoryKind, OpTy, Pointer, RangeMap, + Scalar, ScalarMaybeUninit, Tag, ThreadId, VClock, VTimestamp, VectorIdx, }; pub type AllocExtra = VClockAlloc; @@ -441,23 +441,6 @@ impl MemoryCellClocks { /// Evaluation context extensions. impl<'mir, 'tcx: 'mir> EvalContextExt<'mir, 'tcx> for MiriEvalContext<'mir, 'tcx> {} pub trait EvalContextExt<'mir, 'tcx: 'mir>: MiriEvalContextExt<'mir, 'tcx> { - /// Calculates the MPlaceTy given the offset and layout of an access on an operand - fn offset_and_layout_to_place( - &self, - op: &OpTy<'tcx, Tag>, - offset: u64, - layout: TyAndLayout<'tcx>, - ) -> InterpResult<'tcx, MPlaceTy<'tcx, Tag>> { - let this = self.eval_context_ref(); - let op_place = this.deref_operand(op)?; - let offset = Size::from_bytes(offset); - - // Ensure that the access is within bounds. - assert!(op_place.layout.size >= offset + layout.size); - let value_place = op_place.offset(offset, MemPlaceMeta::None, layout, this)?; - Ok(value_place) - } - /// Atomic variant of read_scalar_at_offset. fn read_scalar_at_offset_atomic( &self, @@ -467,7 +450,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: MiriEvalContextExt<'mir, 'tcx> { atomic: AtomicReadOp, ) -> InterpResult<'tcx, ScalarMaybeUninit> { let this = self.eval_context_ref(); - let value_place = this.offset_and_layout_to_place(op, offset, layout)?; + let value_place = this.deref_operand_and_offset(op, offset, layout)?; this.read_scalar_atomic(&value_place, atomic) } @@ -481,7 +464,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: MiriEvalContextExt<'mir, 'tcx> { atomic: AtomicWriteOp, ) -> InterpResult<'tcx> { let this = self.eval_context_mut(); - let value_place = this.offset_and_layout_to_place(op, offset, layout)?; + let value_place = this.deref_operand_and_offset(op, offset, layout)?; this.write_scalar_atomic(value.into(), &value_place, atomic) } diff --git a/src/helpers.rs b/src/helpers.rs index 5b820218a9..ba5ebd3026 100644 --- a/src/helpers.rs +++ b/src/helpers.rs @@ -597,18 +597,31 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx } } - fn read_scalar_at_offset( + /// Calculates the MPlaceTy given the offset and layout of an access on an operand + fn deref_operand_and_offset( &self, op: &OpTy<'tcx, Tag>, offset: u64, layout: TyAndLayout<'tcx>, - ) -> InterpResult<'tcx, ScalarMaybeUninit> { + ) -> InterpResult<'tcx, MPlaceTy<'tcx, Tag>> { let this = self.eval_context_ref(); let op_place = this.deref_operand(op)?; let offset = Size::from_bytes(offset); - // Ensure that the following read at an offset is within bounds + + // Ensure that the access is within bounds. assert!(op_place.layout.size >= offset + layout.size); let value_place = op_place.offset(offset, MemPlaceMeta::None, layout, this)?; + Ok(value_place) + } + + fn read_scalar_at_offset( + &self, + op: &OpTy<'tcx, Tag>, + offset: u64, + layout: TyAndLayout<'tcx>, + ) -> InterpResult<'tcx, ScalarMaybeUninit> { + let this = self.eval_context_ref(); + let value_place = this.deref_operand_and_offset(op, offset, layout)?; this.read_scalar(&value_place.into()) } @@ -620,11 +633,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx layout: TyAndLayout<'tcx>, ) -> InterpResult<'tcx, ()> { let this = self.eval_context_mut(); - let op_place = this.deref_operand(op)?; - let offset = Size::from_bytes(offset); - // Ensure that the following read at an offset is within bounds - assert!(op_place.layout.size >= offset + layout.size); - let value_place = op_place.offset(offset, MemPlaceMeta::None, layout, this)?; + let value_place = this.deref_operand_and_offset(op, offset, layout)?; this.write_scalar(value, &value_place.into()) } diff --git a/src/shims/posix/sync.rs b/src/shims/posix/sync.rs index 2e5da5b537..56d4969847 100644 --- a/src/shims/posix/sync.rs +++ b/src/shims/posix/sync.rs @@ -112,7 +112,7 @@ fn mutex_get_or_create_id<'mir, 'tcx: 'mir>( ecx: &mut MiriEvalContext<'mir, 'tcx>, mutex_op: &OpTy<'tcx, Tag>, ) -> InterpResult<'tcx, MutexId> { - let value_place = ecx.offset_and_layout_to_place(mutex_op, 4, ecx.machine.layouts.u32)?; + let value_place = ecx.deref_operand_and_offset(mutex_op, 4, ecx.machine.layouts.u32)?; ecx.mutex_get_or_create(|ecx, next_id| { let (old, success) = ecx @@ -168,7 +168,7 @@ fn rwlock_get_or_create_id<'mir, 'tcx: 'mir>( ecx: &mut MiriEvalContext<'mir, 'tcx>, rwlock_op: &OpTy<'tcx, Tag>, ) -> InterpResult<'tcx, RwLockId> { - let value_place = ecx.offset_and_layout_to_place(rwlock_op, 4, ecx.machine.layouts.u32)?; + let value_place = ecx.deref_operand_and_offset(rwlock_op, 4, ecx.machine.layouts.u32)?; ecx.rwlock_get_or_create(|ecx, next_id| { let (old, success) = ecx @@ -252,7 +252,7 @@ fn cond_get_or_create_id<'mir, 'tcx: 'mir>( ecx: &mut MiriEvalContext<'mir, 'tcx>, cond_op: &OpTy<'tcx, Tag>, ) -> InterpResult<'tcx, CondvarId> { - let value_place = ecx.offset_and_layout_to_place(cond_op, 4, ecx.machine.layouts.u32)?; + let value_place = ecx.deref_operand_and_offset(cond_op, 4, ecx.machine.layouts.u32)?; ecx.condvar_get_or_create(|ecx, next_id| { let (old, success) = ecx diff --git a/src/shims/windows/sync.rs b/src/shims/windows/sync.rs index ff10b3b6aa..6a6b2269e6 100644 --- a/src/shims/windows/sync.rs +++ b/src/shims/windows/sync.rs @@ -7,7 +7,7 @@ fn srwlock_get_or_create_id<'mir, 'tcx: 'mir>( ecx: &mut MiriEvalContext<'mir, 'tcx>, lock_op: &OpTy<'tcx, Tag>, ) -> InterpResult<'tcx, RwLockId> { - let value_place = ecx.offset_and_layout_to_place(lock_op, 0, ecx.machine.layouts.u32)?; + let value_place = ecx.deref_operand_and_offset(lock_op, 0, ecx.machine.layouts.u32)?; ecx.rwlock_get_or_create(|ecx, next_id| { let (old, success) = ecx