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

a bit of refactoring in "sync"

- Use `Box<dyn Any>` to keep the "extra data" local to the module implementing the primitive
- Pass around places, not pointers

Cc `@Mandragorian` -- sorry I couldn't resist and did the `Any` thing ;)

Author: bors

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

@@ -1,7 +1,6 @@
 use std::collections::VecDeque;
 
 use rustc_index::Idx;
-use rustc_middle::ty::layout::TyAndLayout;
 
 use super::sync::EvalContextExtPriv as _;
 use super::vector_clock::VClock;
@@ -30,14 +29,12 @@ impl<'tcx> EvalContextExt<'tcx> for crate::MiriInterpCx<'tcx> {}
 pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
     fn init_once_get_or_create_id(
         &mut self,
-        lock_op: &OpTy<'tcx>,
-        lock_layout: TyAndLayout<'tcx>,
+        lock: &MPlaceTy<'tcx>,
         offset: u64,
     ) -> InterpResult<'tcx, InitOnceId> {
         let this = self.eval_context_mut();
         this.get_or_create_id(
-            lock_op,
-            lock_layout,
+            lock,
             offset,
             |ecx| &mut ecx.machine.sync.init_onces,
             |_| Ok(Default::default()),

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

@@ -1,10 +1,11 @@
+use std::any::Any;
 use std::collections::{hash_map::Entry, VecDeque};
 use std::ops::Not;
 use std::time::Duration;
 
 use rustc_data_structures::fx::FxHashMap;
 use rustc_index::{Idx, IndexVec};
-use rustc_middle::ty::layout::TyAndLayout;
+use rustc_target::abi::Size;
 
 use super::init_once::InitOnce;
 use super::vector_clock::VClock;
@@ -66,27 +67,6 @@ pub(super) use declare_id;
 
 declare_id!(MutexId);
 
-/// The mutex kind.
-#[derive(Debug, Clone, Copy)]
-#[non_exhaustive]
-pub enum MutexKind {
-    Invalid,
-    Normal,
-    Default,
-    Recursive,
-    ErrorCheck,
-}
-
-#[derive(Debug)]
-/// Additional data that may be used by shim implementations.
-pub struct AdditionalMutexData {
-    /// The mutex kind, used by some mutex implementations like pthreads mutexes.
-    pub kind: MutexKind,
-
-    /// The address of the mutex.
-    pub address: u64,
-}
-
 /// The mutex state.
 #[derive(Default, Debug)]
 struct Mutex {
@@ -100,18 +80,11 @@ struct Mutex {
     clock: VClock,
 
     /// Additional data that can be set by shim implementations.
-    data: Option<AdditionalMutexData>,
+    data: Option<Box<dyn Any>>,
 }
 
 declare_id!(RwLockId);
 
-#[derive(Debug)]
-/// Additional data that may be used by shim implementations.
-pub struct AdditionalRwLockData {
-    /// The address of the rwlock.
-    pub address: u64,
-}
-
 /// The read-write lock state.
 #[derive(Default, Debug)]
 struct RwLock {
@@ -146,7 +119,7 @@ struct RwLock {
     clock_current_readers: VClock,
 
     /// Additional data that can be set by shim implementations.
-    data: Option<AdditionalRwLockData>,
+    data: Option<Box<dyn Any>>,
 }
 
 declare_id!(CondvarId);
@@ -206,21 +179,21 @@ pub(super) trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> {
     #[inline]
     fn get_or_create_id<Id: SyncId + Idx, T>(
         &mut self,
-        lock_op: &OpTy<'tcx>,
-        lock_layout: TyAndLayout<'tcx>,
+        lock: &MPlaceTy<'tcx>,
         offset: u64,
         get_objs: impl for<'a> Fn(&'a mut MiriInterpCx<'tcx>) -> &'a mut IndexVec<Id, T>,
         create_obj: impl for<'a> FnOnce(&'a mut MiriInterpCx<'tcx>) -> InterpResult<'tcx, 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 offset = Size::from_bytes(offset);
+        assert!(lock.layout.size >= offset + this.machine.layouts.u32.size);
+        let id_place = lock.offset(offset, this.machine.layouts.u32, this)?;
         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,
+                &id_place,
                 &ImmTy::from_uint(0u32, this.machine.layouts.u32),
                 Scalar::from_u32(next_index.to_u32()),
                 AtomicRwOrd::Relaxed, // deliberately *no* synchronization
@@ -258,18 +231,18 @@ pub(super) trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> {
     /// - `obj` must be the new sync object.
     fn create_id<Id: SyncId + Idx, T>(
         &mut self,
-        lock_op: &OpTy<'tcx>,
-        lock_layout: TyAndLayout<'tcx>,
+        lock: &MPlaceTy<'tcx>,
         offset: u64,
         get_objs: impl for<'a> Fn(&'a mut MiriInterpCx<'tcx>) -> &'a mut IndexVec<Id, T>,
         obj: T,
     ) -> InterpResult<'tcx, 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 offset = Size::from_bytes(offset);
+        assert!(lock.layout.size >= offset + this.machine.layouts.u32.size);
+        let id_place = lock.offset(offset, this.machine.layouts.u32, this)?;
 
         let new_index = get_objs(this).push(obj);
-        this.write_scalar(Scalar::from_u32(new_index.to_u32()), &value_place)?;
+        this.write_scalar(Scalar::from_u32(new_index.to_u32()), &id_place)?;
         Ok(new_index)
     }
 
@@ -302,15 +275,13 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
     /// Eagerly create and initialize a new mutex.
     fn mutex_create(
         &mut self,
-        lock_op: &OpTy<'tcx>,
-        lock_layout: TyAndLayout<'tcx>,
+        lock: &MPlaceTy<'tcx>,
         offset: u64,
-        data: Option<AdditionalMutexData>,
+        data: Option<Box<dyn Any>>,
     ) -> InterpResult<'tcx, MutexId> {
         let this = self.eval_context_mut();
         this.create_id(
-            lock_op,
-            lock_layout,
+            lock,
             offset,
             |ecx| &mut ecx.machine.sync.mutexes,
             Mutex { data, ..Default::default() },
@@ -321,17 +292,15 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
     /// `initialize_data` must return any additional data that a user wants to associate with the mutex.
     fn mutex_get_or_create_id(
         &mut self,
-        lock_op: &OpTy<'tcx>,
-        lock_layout: TyAndLayout<'tcx>,
+        lock: &MPlaceTy<'tcx>,
         offset: u64,
         initialize_data: impl for<'a> FnOnce(
             &'a mut MiriInterpCx<'tcx>,
-        ) -> InterpResult<'tcx, Option<AdditionalMutexData>>,
+        ) -> InterpResult<'tcx, Option<Box<dyn Any>>>,
     ) -> InterpResult<'tcx, MutexId> {
         let this = self.eval_context_mut();
         this.get_or_create_id(
-            lock_op,
-            lock_layout,
+            lock,
             offset,
             |ecx| &mut ecx.machine.sync.mutexes,
             |ecx| initialize_data(ecx).map(|data| Mutex { data, ..Default::default() }),
@@ -340,28 +309,25 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
     }
 
     /// Retrieve the additional data stored for a mutex.
-    fn mutex_get_data<'a>(&'a mut self, id: MutexId) -> Option<&'a AdditionalMutexData>
+    fn mutex_get_data<'a, T: 'static>(&'a mut self, id: MutexId) -> Option<&'a T>
     where
         'tcx: 'a,
     {
         let this = self.eval_context_ref();
-        this.machine.sync.mutexes[id].data.as_ref()
+        this.machine.sync.mutexes[id].data.as_deref().and_then(|p| p.downcast_ref::<T>())
     }
 
     fn rwlock_get_or_create_id(
         &mut self,
-        lock_op: &OpTy<'tcx>,
-        lock_layout: TyAndLayout<'tcx>,
+        lock: &MPlaceTy<'tcx>,
         offset: u64,
         initialize_data: impl for<'a> FnOnce(
             &'a mut MiriInterpCx<'tcx>,
-        )
-            -> InterpResult<'tcx, Option<AdditionalRwLockData>>,
+        ) -> InterpResult<'tcx, Option<Box<dyn Any>>>,
     ) -> InterpResult<'tcx, RwLockId> {
         let this = self.eval_context_mut();
         this.get_or_create_id(
-            lock_op,
-            lock_layout,
+            lock,
             offset,
             |ecx| &mut ecx.machine.sync.rwlocks,
             |ecx| initialize_data(ecx).map(|data| RwLock { data, ..Default::default() }),
@@ -370,24 +336,22 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
     }
 
     /// Retrieve the additional data stored for a rwlock.
-    fn rwlock_get_data<'a>(&'a mut self, id: RwLockId) -> Option<&'a AdditionalRwLockData>
+    fn rwlock_get_data<'a, T: 'static>(&'a mut self, id: RwLockId) -> Option<&'a T>
     where
         'tcx: 'a,
     {
         let this = self.eval_context_ref();
-        this.machine.sync.rwlocks[id].data.as_ref()
+        this.machine.sync.rwlocks[id].data.as_deref().and_then(|p| p.downcast_ref::<T>())
     }
 
     fn condvar_get_or_create_id(
         &mut self,
-        lock_op: &OpTy<'tcx>,
-        lock_layout: TyAndLayout<'tcx>,
+        lock: &MPlaceTy<'tcx>,
         offset: u64,
     ) -> InterpResult<'tcx, CondvarId> {
         let this = self.eval_context_mut();
         this.get_or_create_id(
-            lock_op,
-            lock_layout,
+            lock,
             offset,
             |ecx| &mut ecx.machine.sync.condvars,
             |_| Ok(Default::default()),

src/tools/miri/src/lib.rs

@@ -133,10 +133,7 @@ pub use crate::concurrency::{
     cpu_affinity::MAX_CPUS,
     data_race::{AtomicFenceOrd, AtomicReadOrd, AtomicRwOrd, AtomicWriteOrd, EvalContextExt as _},
     init_once::{EvalContextExt as _, InitOnceId},
-    sync::{
-        AdditionalMutexData, AdditionalRwLockData, CondvarId, EvalContextExt as _, MutexId,
-        MutexKind, RwLockId, SynchronizationObjects,
-    },
+    sync::{CondvarId, EvalContextExt as _, MutexId, RwLockId, SynchronizationObjects},
     thread::{
         BlockReason, EvalContextExt as _, StackEmptyCallback, ThreadId, ThreadManager,
         TimeoutAnchor, TimeoutClock, UnblockCallback,

src/tools/miri/src/shims/unix/macos/sync.rs

@@ -14,12 +14,13 @@ use crate::*;
 
 impl<'tcx> EvalContextExtPriv<'tcx> for crate::MiriInterpCx<'tcx> {}
 trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> {
-    fn os_unfair_lock_getid(&mut self, lock_op: &OpTy<'tcx>) -> InterpResult<'tcx, MutexId> {
+    fn os_unfair_lock_getid(&mut self, lock_ptr: &OpTy<'tcx>) -> InterpResult<'tcx, MutexId> {
         let this = self.eval_context_mut();
+        let lock = this.deref_pointer(lock_ptr)?;
         // os_unfair_lock holds a 32-bit value, is initialized with zero and
         // must be assumed to be opaque. Therefore, we can just store our
         // internal mutex ID in the structure without anyone noticing.
-        this.mutex_get_or_create_id(lock_op, this.libc_ty_layout("os_unfair_lock"), 0, |_| Ok(None))
+        this.mutex_get_or_create_id(&lock, 0, |_| Ok(None))
     }
 }