Auto merge of #3823 - RalfJung:sync, r=RalfJung

simplify synchronization object creation logic

Author: bors

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

@@ -26,27 +26,6 @@ pub(super) struct InitOnce {
     clock: VClock,
 }
 
-impl<'tcx> EvalContextExtPriv<'tcx> for crate::MiriInterpCx<'tcx> {}
-trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> {
-    /// Provides the closure with the next InitOnceId. Creates that InitOnce if the closure returns None,
-    /// otherwise returns the value from the closure.
-    #[inline]
-    fn init_once_get_or_create<F>(&mut self, existing: F) -> InterpResult<'tcx, InitOnceId>
-    where
-        F: FnOnce(&mut MiriInterpCx<'tcx>, InitOnceId) -> InterpResult<'tcx, Option<InitOnceId>>,
-    {
-        let this = self.eval_context_mut();
-        let next_index = this.machine.sync.init_onces.next_index();
-        if let Some(old) = existing(this, next_index)? {
-            Ok(old)
-        } else {
-            let new_index = this.machine.sync.init_onces.push(Default::default());
-            assert_eq!(next_index, new_index);
-            Ok(new_index)
-        }
-    }
-}
-
 impl<'tcx> EvalContextExt<'tcx> for crate::MiriInterpCx<'tcx> {}
 pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
     fn init_once_get_or_create_id(
@@ -56,9 +35,8 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         offset: u64,
     ) -> InterpResult<'tcx, InitOnceId> {
         let this = self.eval_context_mut();
-        this.init_once_get_or_create(|ecx, next_id| {
-            ecx.get_or_create_id(next_id, lock_op, lock_layout, offset)
-        })
+        this.get_or_create_id(lock_op, lock_layout, offset, |ecx| &mut ecx.machine.sync.init_onces)?
+            .ok_or_else(|| err_ub_format!("init_once has invalid ID").into())
     }
 
     #[inline]

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

@@ -164,100 +164,50 @@ pub struct SynchronizationObjects {
 impl<'tcx> EvalContextExtPriv<'tcx> for crate::MiriInterpCx<'tcx> {}
 pub(super) trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> {
     /// Lazily initialize the ID of this Miri sync structure.
-    /// ('0' indicates uninit.)
+    /// If memory stores '0', that indicates uninit and we generate a new instance.
+    /// Returns `None` if memory stores a non-zero invalid ID.
+    ///
+    /// `get_objs` must return the `IndexVec` that stores all the objects of this type.
     #[inline]
-    fn get_or_create_id<Id: SyncId>(
+    fn get_or_create_id<Id: SyncId + Idx, T: Default>(
         &mut self,
-        next_id: Id,
         lock_op: &OpTy<'tcx>,
         lock_layout: TyAndLayout<'tcx>,
         offset: u64,
+        get_objs: impl for<'a> Fn(&'a mut MiriInterpCx<'tcx>) -> &'a mut IndexVec<Id, T>,
     ) -> InterpResult<'tcx, Option<Id>> {
         let this = self.eval_context_mut();
         let value_place =
             this.deref_pointer_and_offset(lock_op, offset, lock_layout, this.machine.layouts.u32)?;
+        let next_index = get_objs(this).next_index();
 
         // Since we are lazy, this update has to be atomic.
         let (old, success) = this
             .atomic_compare_exchange_scalar(
                 &value_place,
                 &ImmTy::from_uint(0u32, this.machine.layouts.u32),
-                Scalar::from_u32(next_id.to_u32()),
+                Scalar::from_u32(next_index.to_u32()),
                 AtomicRwOrd::Relaxed, // deliberately *no* synchronization
                 AtomicReadOrd::Relaxed,
                 false,
             )?
             .to_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(Id::from_u32(old.to_u32().expect("layout is u32")))
-        })
-    }
-
-    /// Provides the closure with the next MutexId. Creates that mutex if the closure returns None,
-    /// otherwise returns the value from the closure.
-    #[inline]
-    fn mutex_get_or_create<F>(&mut self, existing: F) -> InterpResult<'tcx, MutexId>
-    where
-        F: FnOnce(&mut MiriInterpCx<'tcx>, MutexId) -> InterpResult<'tcx, Option<MutexId>>,
-    {
-        let this = self.eval_context_mut();
-        let next_index = this.machine.sync.mutexes.next_index();
-        if let Some(old) = existing(this, next_index)? {
-            if this.machine.sync.mutexes.get(old).is_none() {
-                throw_ub_format!("mutex has invalid ID");
-            }
-            Ok(old)
-        } else {
-            let new_index = this.machine.sync.mutexes.push(Default::default());
+            // We set the in-memory ID to `next_index`, now also create this object in the machine
+            // state.
+            let new_index = get_objs(this).push(T::default());
             assert_eq!(next_index, new_index);
-            Ok(new_index)
-        }
-    }
-
-    /// Provides the closure with the next RwLockId. Creates that RwLock if the closure returns None,
-    /// otherwise returns the value from the closure.
-    #[inline]
-    fn rwlock_get_or_create<F>(&mut self, existing: F) -> InterpResult<'tcx, RwLockId>
-    where
-        F: FnOnce(&mut MiriInterpCx<'tcx>, RwLockId) -> InterpResult<'tcx, Option<RwLockId>>,
-    {
-        let this = self.eval_context_mut();
-        let next_index = this.machine.sync.rwlocks.next_index();
-        if let Some(old) = existing(this, next_index)? {
-            if this.machine.sync.rwlocks.get(old).is_none() {
-                throw_ub_format!("rwlock has invalid ID");
-            }
-            Ok(old)
+            Some(new_index)
         } else {
-            let new_index = this.machine.sync.rwlocks.push(Default::default());
-            assert_eq!(next_index, new_index);
-            Ok(new_index)
-        }
-    }
-
-    /// Provides the closure with the next CondvarId. Creates that Condvar if the closure returns None,
-    /// otherwise returns the value from the closure.
-    #[inline]
-    fn condvar_get_or_create<F>(&mut self, existing: F) -> InterpResult<'tcx, CondvarId>
-    where
-        F: FnOnce(&mut MiriInterpCx<'tcx>, CondvarId) -> InterpResult<'tcx, Option<CondvarId>>,
-    {
-        let this = self.eval_context_mut();
-        let next_index = this.machine.sync.condvars.next_index();
-        if let Some(old) = existing(this, next_index)? {
-            if this.machine.sync.condvars.get(old).is_none() {
-                throw_ub_format!("condvar has invalid ID");
+            let id = Id::from_u32(old.to_u32().expect("layout is u32"));
+            if get_objs(this).get(id).is_none() {
+                // The in-memory ID is invalid.
+                None
+            } else {
+                Some(id)
             }
-            Ok(old)
-        } else {
-            let new_index = this.machine.sync.condvars.push(Default::default());
-            assert_eq!(next_index, new_index);
-            Ok(new_index)
-        }
+        })
     }
 
     fn condvar_reacquire_mutex(
@@ -293,9 +243,8 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         offset: u64,
     ) -> InterpResult<'tcx, MutexId> {
         let this = self.eval_context_mut();
-        this.mutex_get_or_create(|ecx, next_id| {
-            ecx.get_or_create_id(next_id, lock_op, lock_layout, offset)
-        })
+        this.get_or_create_id(lock_op, lock_layout, offset, |ecx| &mut ecx.machine.sync.mutexes)?
+            .ok_or_else(|| err_ub_format!("mutex has invalid ID").into())
     }
 
     fn rwlock_get_or_create_id(
@@ -305,9 +254,8 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         offset: u64,
     ) -> InterpResult<'tcx, RwLockId> {
         let this = self.eval_context_mut();
-        this.rwlock_get_or_create(|ecx, next_id| {
-            ecx.get_or_create_id(next_id, lock_op, lock_layout, offset)
-        })
+        this.get_or_create_id(lock_op, lock_layout, offset, |ecx| &mut ecx.machine.sync.rwlocks)?
+            .ok_or_else(|| err_ub_format!("rwlock has invalid ID").into())
     }
 
     fn condvar_get_or_create_id(
@@ -317,9 +265,8 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         offset: u64,
     ) -> InterpResult<'tcx, CondvarId> {
         let this = self.eval_context_mut();
-        this.condvar_get_or_create(|ecx, next_id| {
-            ecx.get_or_create_id(next_id, lock_op, lock_layout, offset)
-        })
+        this.get_or_create_id(lock_op, lock_layout, offset, |ecx| &mut ecx.machine.sync.condvars)?
+            .ok_or_else(|| err_ub_format!("condvar has invalid ID").into())
     }
 
     #[inline]