unix/sync: cleanup

1 files changed, 29 insertions, 81 deletions

diff --git a/src/tools/miri/src/shims/unix/sync.rs b/src/tools/miri/src/shims/unix/sync.rs
index 3535eacb447..b038ac33df8 100644
--- a/src/tools/miri/src/shims/unix/sync.rs
+++ b/src/tools/miri/src/shims/unix/sync.rs
@@ -50,15 +50,6 @@ fn mutexattr_set_kind<'tcx>(
 /// in `pthread_mutexattr_settype` function.
 const PTHREAD_MUTEX_NORMAL_FLAG: i32 = 0x8000000;
 
-fn is_mutex_kind_default<'tcx>(ecx: &MiriInterpCx<'tcx>, kind: i32) -> InterpResult<'tcx, bool> {
-    Ok(kind == ecx.eval_libc_i32("PTHREAD_MUTEX_DEFAULT"))
-}
-
-fn is_mutex_kind_normal<'tcx>(ecx: &MiriInterpCx<'tcx>, kind: i32) -> InterpResult<'tcx, bool> {
-    let mutex_normal_kind = ecx.eval_libc_i32("PTHREAD_MUTEX_NORMAL");
-    Ok(kind == (mutex_normal_kind | PTHREAD_MUTEX_NORMAL_FLAG))
-}
-
 /// The mutex kind.
 #[derive(Debug, Clone, Copy)]
 pub enum MutexKind {
@@ -78,7 +69,7 @@ pub struct AdditionalMutexData {
     pub address: u64,
 }
 
-// pthread_mutex_t is between 24 and 48 bytes, depending on the platform.
+// pthread_mutex_t is between 4 and 48 bytes, depending on the platform.
 // We ignore the platform layout and store our own fields:
 // - id: u32
 
@@ -131,7 +122,7 @@ fn mutex_create<'tcx>(
 ) -> InterpResult<'tcx> {
     let mutex = ecx.deref_pointer(mutex_ptr)?;
     let address = mutex.ptr().addr().bytes();
-    let kind = translate_kind(ecx, kind)?;
+    let kind = mutex_translate_kind(ecx, kind)?;
     let data = Box::new(AdditionalMutexData { address, kind });
     ecx.mutex_create(&mutex, mutex_id_offset(ecx)?, Some(data))?;
     Ok(())
@@ -151,7 +142,7 @@ fn mutex_get_id<'tcx>(
     let id = ecx.mutex_get_or_create_id(&mutex, mutex_id_offset(ecx)?, |ecx| {
         // This is called if a static initializer was used and the lock has not been assigned
         // an ID yet. We have to determine the mutex kind from the static initializer.
-        let kind = kind_from_static_initializer(ecx, &mutex)?;
+        let kind = mutex_kind_from_static_initializer(ecx, &mutex)?;
 
         Ok(Some(Box::new(AdditionalMutexData { kind, address })))
     })?;
@@ -168,12 +159,12 @@ fn mutex_get_id<'tcx>(
 }
 
 /// Returns the kind of a static initializer.
-fn kind_from_static_initializer<'tcx>(
+fn mutex_kind_from_static_initializer<'tcx>(
     ecx: &MiriInterpCx<'tcx>,
     mutex: &MPlaceTy<'tcx>,
 ) -> InterpResult<'tcx, MutexKind> {
-    // Only linux has static initializers other than PTHREAD_MUTEX_DEFAULT.
     let kind = match &*ecx.tcx.sess.target.os {
+        // Only linux has static initializers other than PTHREAD_MUTEX_DEFAULT.
         "linux" => {
             let offset = if ecx.pointer_size().bytes() == 8 { 16 } else { 12 };
             let kind_place =
@@ -184,13 +175,16 @@ fn kind_from_static_initializer<'tcx>(
         os => throw_unsup_format!("`pthread_mutex` is not supported on {os}"),
     };
 
-    translate_kind(ecx, kind)
+    mutex_translate_kind(ecx, kind)
 }
 
-fn translate_kind<'tcx>(ecx: &MiriInterpCx<'tcx>, kind: i32) -> InterpResult<'tcx, MutexKind> {
-    Ok(if is_mutex_kind_default(ecx, kind)? {
+fn mutex_translate_kind<'tcx>(
+    ecx: &MiriInterpCx<'tcx>,
+    kind: i32,
+) -> InterpResult<'tcx, MutexKind> {
+    Ok(if kind == ecx.eval_libc_i32("PTHREAD_MUTEX_DEFAULT") {
         MutexKind::Default
-    } else if is_mutex_kind_normal(ecx, kind)? {
+    } else if kind == (ecx.eval_libc_i32("PTHREAD_MUTEX_NORMAL") | PTHREAD_MUTEX_NORMAL_FLAG) {
         MutexKind::Normal
     } else if kind == ecx.eval_libc_i32("PTHREAD_MUTEX_ERRORCHECK") {
         MutexKind::ErrorCheck
@@ -201,7 +195,7 @@ fn translate_kind<'tcx>(ecx: &MiriInterpCx<'tcx>, kind: i32) -> InterpResult<'tc
     })
 }
 
-// pthread_rwlock_t is between 32 and 56 bytes, depending on the platform.
+// pthread_rwlock_t is between 4 and 56 bytes, depending on the platform.
 // We ignore the platform layout and store our own fields:
 // - id: u32
 
@@ -286,11 +280,11 @@ fn condattr_get_clock_id<'tcx>(
     .to_i32()
 }
 
-fn translate_clock_id<'tcx>(ecx: &MiriInterpCx<'tcx>, raw_id: i32) -> InterpResult<'tcx, ClockId> {
-    // To ensure compatibility with PTHREAD_COND_INITIALIZER on all platforms,
-    // we can't just compare with CLOCK_REALTIME: on Solarish, PTHREAD_COND_INITIALIZER
-    // makes the clock 0 but CLOCK_REALTIME is 3.
-    Ok(if raw_id == ecx.eval_libc_i32("CLOCK_REALTIME") || raw_id == 0 {
+fn cond_translate_clock_id<'tcx>(
+    ecx: &MiriInterpCx<'tcx>,
+    raw_id: i32,
+) -> InterpResult<'tcx, ClockId> {
+    Ok(if raw_id == ecx.eval_libc_i32("CLOCK_REALTIME") {
         ClockId::Realtime
     } else if raw_id == ecx.eval_libc_i32("CLOCK_MONOTONIC") {
         ClockId::Monotonic
@@ -313,10 +307,9 @@ fn condattr_set_clock_id<'tcx>(
     )
 }
 
-// pthread_cond_t.
+// pthread_cond_t can be only 4 bytes in size, depending on the platform.
 // We ignore the platform layout and store our own fields:
 // - id: u32
-// - clock: i32
 
 fn cond_id_offset<'tcx>(ecx: &MiriInterpCx<'tcx>) -> InterpResult<'tcx, u64> {
     let offset = match &*ecx.tcx.sess.target.os {
@@ -344,30 +337,6 @@ fn cond_id_offset<'tcx>(ecx: &MiriInterpCx<'tcx>) -> InterpResult<'tcx, u64> {
     Ok(offset)
 }
 
-fn cond_clock_offset<'tcx>(ecx: &MiriInterpCx<'tcx>) -> u64 {
-    // macOS doesn't have a clock attribute, but to keep the code uniform we store
-    // a clock ID in the pthread_cond_t anyway. There's enough space.
-    let offset = 8;
-
-    // Sanity-check this against PTHREAD_COND_INITIALIZER (but only once):
-    // the clock must start out as CLOCK_REALTIME.
-    static SANITY: AtomicBool = AtomicBool::new(false);
-    if !SANITY.swap(true, Ordering::Relaxed) {
-        let static_initializer = ecx.eval_path(&["libc", "PTHREAD_COND_INITIALIZER"]);
-        let id_field = static_initializer
-            .offset(Size::from_bytes(offset), ecx.machine.layouts.i32, ecx)
-            .unwrap();
-        let id = ecx.read_scalar(&id_field).unwrap().to_i32().unwrap();
-        let id = translate_clock_id(ecx, id).expect("static initializer should be valid");
-        assert!(
-            matches!(id, ClockId::Realtime),
-            "PTHREAD_COND_INITIALIZER is incompatible with our pthread_cond layout: clock is not CLOCK_REALTIME"
-        );
-    }
-
-    offset
-}
-
 #[derive(Debug, Clone, Copy)]
 enum ClockId {
     Realtime,
@@ -390,14 +359,9 @@ fn cond_get_id<'tcx>(
 ) -> InterpResult<'tcx, CondvarId> {
     let cond = ecx.deref_pointer(cond_ptr)?;
     let address = cond.ptr().addr().bytes();
-    let id = ecx.condvar_get_or_create_id(&cond, cond_id_offset(ecx)?, |ecx| {
-        let raw_id = if ecx.tcx.sess.target.os == "macos" {
-            ecx.eval_libc_i32("CLOCK_REALTIME")
-        } else {
-            cond_get_clock_id(ecx, cond_ptr)?
-        };
-        let clock_id = translate_clock_id(ecx, raw_id)?;
-        Ok(Some(Box::new(AdditionalCondData { address, clock_id })))
+    let id = ecx.condvar_get_or_create_id(&cond, cond_id_offset(ecx)?, |_ecx| {
+        // This used the static initializer. The clock there is always CLOCK_REALTIME.
+        Ok(Some(Box::new(AdditionalCondData { address, clock_id: ClockId::Realtime })))
     })?;
 
     // Check that the mutex has not been moved since last use.
@@ -411,19 +375,6 @@ fn cond_get_id<'tcx>(
     Ok(id)
 }
 
-fn cond_get_clock_id<'tcx>(
-    ecx: &MiriInterpCx<'tcx>,
-    cond_ptr: &OpTy<'tcx>,
-) -> InterpResult<'tcx, i32> {
-    ecx.deref_pointer_and_read(
-        cond_ptr,
-        cond_clock_offset(ecx),
-        ecx.libc_ty_layout("pthread_cond_t"),
-        ecx.machine.layouts.i32,
-    )?
-    .to_i32()
-}
-
 impl<'tcx> EvalContextExt<'tcx> for crate::MiriInterpCx<'tcx> {}
 pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
     fn pthread_mutexattr_init(&mut self, attr_op: &OpTy<'tcx>) -> InterpResult<'tcx, ()> {
@@ -624,15 +575,14 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
     fn pthread_mutex_destroy(&mut self, mutex_op: &OpTy<'tcx>) -> InterpResult<'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.
         let id = mutex_get_id(this, mutex_op)?;
 
         if this.mutex_is_locked(id) {
             throw_ub_format!("destroyed a locked mutex");
         }
 
-        // Destroying an uninit pthread_mutex is UB, so check to make sure it's not uninit.
-        mutex_get_id(this, mutex_op)?;
-
         // This might lead to false positives, see comment in pthread_mutexattr_destroy
         this.write_uninit(
             &this.deref_pointer_as(mutex_op, this.libc_ty_layout("pthread_mutex_t"))?,
@@ -734,15 +684,14 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
     fn pthread_rwlock_destroy(&mut self, rwlock_op: &OpTy<'tcx>) -> InterpResult<'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.
         let id = rwlock_get_id(this, rwlock_op)?;
 
         if this.rwlock_is_locked(id) {
             throw_ub_format!("destroyed a locked rwlock");
         }
 
-        // Destroying an uninit pthread_rwlock is UB, so check to make sure it's not uninit.
-        rwlock_get_id(this, rwlock_op)?;
-
         // This might lead to false positives, see comment in pthread_mutexattr_destroy
         this.write_uninit(
             &this.deref_pointer_as(rwlock_op, this.libc_ty_layout("pthread_rwlock_t"))?,
@@ -832,7 +781,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         } else {
             condattr_get_clock_id(this, attr_op)?
         };
-        let clock_id = translate_clock_id(this, clock_id)?;
+        let clock_id = cond_translate_clock_id(this, clock_id)?;
 
         let cond = this.deref_pointer(cond_op)?;
         let address = cond.ptr().addr().bytes();
@@ -930,11 +879,10 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
     }
 
     fn pthread_cond_destroy(&mut self, cond_op: &OpTy<'tcx>) -> InterpResult<'tcx, ()> {
-        //NOTE: Destroying an uninit pthread_cond is UB. Make sure it's not uninit,
-        // by accessing at least once all of its fields that we use.
-
         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.
         let id = cond_get_id(this, cond_op)?;
         if this.condvar_is_awaited(id) {
             throw_ub_format!("destroying an awaited conditional variable");