make lazy_sync_get_data also take a closure to initialize if needed

Author: RalfJung

src/concurrency/sync.rs

@@ -201,12 +201,11 @@ pub fn lazy_sync_init<'tcx, T: 'static + Copy>(
     init_offset: Size,
     data: T,
 ) -> InterpResult<'tcx> {
-    let init_field = primitive.offset(init_offset, ecx.machine.layouts.u32, ecx)?;
-
     let (alloc, offset, _) = ecx.ptr_get_alloc_id(primitive.ptr(), 0)?;
     let (alloc_extra, _machine) = ecx.get_alloc_extra_mut(alloc)?;
     alloc_extra.sync.insert(offset, Box::new(data));
     // Mark this as "initialized".
+    let init_field = primitive.offset(init_offset, ecx.machine.layouts.u32, ecx)?;
     ecx.write_scalar_atomic(
         Scalar::from_u32(LAZY_INIT_COOKIE),
         &init_field,
@@ -217,18 +216,19 @@ pub fn lazy_sync_init<'tcx, T: 'static + Copy>(
 
 /// Helper for lazily initialized `alloc_extra.sync` data:
 /// Checks if the primitive is initialized, and return its associated data if so.
-/// Otherwise, return None.
+/// Otherwise, calls `new_data` to initialize the primitive.
 pub fn lazy_sync_get_data<'tcx, T: 'static + Copy>(
     ecx: &mut MiriInterpCx<'tcx>,
     primitive: &MPlaceTy<'tcx>,
     init_offset: Size,
     name: &str,
-) -> InterpResult<'tcx, Option<T>> {
-    let init_field = primitive.offset(init_offset, ecx.machine.layouts.u32, ecx)?;
+    new_data: impl FnOnce(&mut MiriInterpCx<'tcx>) -> InterpResult<'tcx, T>,
+) -> InterpResult<'tcx, T> {
     // Check if this is already initialized. Needs to be atomic because we can race with another
     // thread initializing. Needs to be an RMW operation to ensure we read the *latest* value.
     // So we just try to replace MUTEX_INIT_COOKIE with itself.
     let init_cookie = Scalar::from_u32(LAZY_INIT_COOKIE);
+    let init_field = primitive.offset(init_offset, ecx.machine.layouts.u32, ecx)?;
     let (_init, success) = ecx
         .atomic_compare_exchange_scalar(
             &init_field,
@@ -239,6 +239,7 @@ pub fn lazy_sync_get_data<'tcx, T: 'static + Copy>(
             /* can_fail_spuriously */ false,
         )?
         .to_scalar_pair();
+
     if success.to_bool()? {
         // If it is initialized, it must be found in the "sync primitive" table,
         // or else it has been moved illegally.
@@ -247,9 +248,11 @@ pub fn lazy_sync_get_data<'tcx, T: 'static + Copy>(
         let data = alloc_extra
             .get_sync::<T>(offset)
             .ok_or_else(|| err_ub_format!("`{name}` can't be moved after first use"))?;
-        interp_ok(Some(*data))
+        interp_ok(*data)
     } else {
-        interp_ok(None)
+        let data = new_data(ecx)?;
+        lazy_sync_init(ecx, primitive, init_offset, data)?;
+        interp_ok(data)
     }
 }
 

src/shims/unix/sync.rs

@@ -190,19 +190,11 @@ fn mutex_get_data<'tcx, 'a>(
     mutex_ptr: &OpTy<'tcx>,
 ) -> InterpResult<'tcx, MutexData> {
     let mutex = ecx.deref_pointer(mutex_ptr)?;
-
-    if let Some(data) =
-        lazy_sync_get_data::<MutexData>(ecx, &mutex, mutex_init_offset(ecx)?, "pthread_mutex_t")?
-    {
-        interp_ok(data)
-    } else {
-        // Not yet initialized. This must be a static initializer, figure out the kind
-        // from that. We don't need to worry about races since we are the interpreter
-        // and don't let any other tread take a step.
+    lazy_sync_get_data(ecx, &mutex, mutex_init_offset(ecx)?, "pthread_mutex_t", |ecx| {
         let kind = mutex_kind_from_static_initializer(ecx, &mutex)?;
-        // And then create the mutex like this.
-        mutex_create(ecx, mutex_ptr, kind)
-    }
+        let id = ecx.machine.sync.mutex_create();
+        interp_ok(MutexData { id, kind })
+    })
 }
 
 /// Returns the kind of a static initializer.
@@ -271,13 +263,7 @@ fn rwlock_get_data<'tcx>(
     rwlock_ptr: &OpTy<'tcx>,
 ) -> InterpResult<'tcx, RwLockData> {
     let rwlock = ecx.deref_pointer(rwlock_ptr)?;
-    let init_offset = rwlock_init_offset(ecx)?;
-
-    if let Some(data) =
-        lazy_sync_get_data::<RwLockData>(ecx, &rwlock, init_offset, "pthread_rwlock_t")?
-    {
-        interp_ok(data)
-    } else {
+    lazy_sync_get_data(ecx, &rwlock, rwlock_init_offset(ecx)?, "pthread_rwlock_t", |ecx| {
         if !bytewise_equal_atomic_relaxed(
             ecx,
             &rwlock,
@@ -286,10 +272,8 @@ fn rwlock_get_data<'tcx>(
             throw_unsup_format!("unsupported static initializer used for `pthread_rwlock_t`");
         }
         let id = ecx.machine.sync.rwlock_create();
-        let data = RwLockData { id };
-        lazy_sync_init(ecx, &rwlock, init_offset, data)?;
-        interp_ok(data)
-    }
+        interp_ok(RwLockData { id })
+    })
 }
 
 // # pthread_condattr_t
@@ -405,21 +389,18 @@ fn cond_get_data<'tcx>(
     cond_ptr: &OpTy<'tcx>,
 ) -> InterpResult<'tcx, CondData> {
     let cond = ecx.deref_pointer(cond_ptr)?;
-    let init_offset = cond_init_offset(ecx)?;
-
-    if let Some(data) = lazy_sync_get_data::<CondData>(ecx, &cond, init_offset, "pthread_cond_t")? {
-        interp_ok(data)
-    } else {
-        // This used the static initializer. The clock there is always CLOCK_REALTIME.
+    lazy_sync_get_data(ecx, &cond, cond_init_offset(ecx)?, "pthread_cond_t", |ecx| {
         if !bytewise_equal_atomic_relaxed(
             ecx,
             &cond,
             &ecx.eval_path(&["libc", "PTHREAD_COND_INITIALIZER"]),
         )? {
             throw_unsup_format!("unsupported static initializer used for `pthread_cond_t`");
         }
-        cond_create(ecx, cond_ptr, ClockId::Realtime)
-    }
+        // This used the static initializer. The clock there is always CLOCK_REALTIME.
+        let id = ecx.machine.sync.condvar_create();
+        interp_ok(CondData { id, clock: ClockId::Realtime })
+    })
 }
 
 impl<'tcx> EvalContextExt<'tcx> for crate::MiriInterpCx<'tcx> {}

src/shims/windows/sync.rs

@@ -3,7 +3,7 @@ use std::time::Duration;
 use rustc_target::abi::Size;
 
 use crate::concurrency::init_once::InitOnceStatus;
-use crate::concurrency::sync::{lazy_sync_get_data, lazy_sync_init};
+use crate::concurrency::sync::lazy_sync_get_data;
 use crate::*;
 
 #[derive(Copy, Clone)]
@@ -16,23 +16,20 @@ 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_id(&mut self, init_once_ptr: &OpTy<'tcx>) -> InterpResult<'tcx, InitOnceId> {
+    fn init_once_get_data(
+        &mut self,
+        init_once_ptr: &OpTy<'tcx>,
+    ) -> InterpResult<'tcx, InitOnceData> {
         let this = self.eval_context_mut();
 
         let init_once = this.deref_pointer(init_once_ptr)?;
         let init_offset = Size::ZERO;
 
-        if let Some(data) =
-            lazy_sync_get_data::<InitOnceData>(this, &init_once, init_offset, "INIT_ONCE")?
-        {
-            interp_ok(data.id)
-        } else {
+        lazy_sync_get_data(this, &init_once, init_offset, "INIT_ONCE", |this| {
             // TODO: check that this is still all-zero.
             let id = this.machine.sync.init_once_create();
-            let data = InitOnceData { id };
-            lazy_sync_init(this, &init_once, init_offset, data)?;
-            interp_ok(id)
-        }
+            interp_ok(InitOnceData { id })
+        })
     }
 
     /// Returns `true` if we were succssful, `false` if we would block.
@@ -74,7 +71,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
     ) -> InterpResult<'tcx> {
         let this = self.eval_context_mut();
 
-        let id = this.init_once_get_id(init_once_op)?;
+        let id = this.init_once_get_data(init_once_op)?.id;
         let flags = this.read_scalar(flags_op)?.to_u32()?;
         let pending_place = this.deref_pointer(pending_op)?;
         let context = this.read_pointer(context_op)?;
@@ -120,7 +117,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
     ) -> InterpResult<'tcx, Scalar> {
         let this = self.eval_context_mut();
 
-        let id = this.init_once_get_id(init_once_op)?;
+        let id = this.init_once_get_data(init_once_op)?.id;
         let flags = this.read_scalar(flags_op)?.to_u32()?;
         let context = this.read_pointer(context_op)?;