address comments

Author: Mandragorian

src/shims/unix/sync.rs

@@ -286,6 +286,22 @@ fn condattr_get_clock_id<'tcx>(
     .to_i32()
 }
 
+fn translate_clock_id<'tcx>(
+    ecx: &MiriInterpCx<'tcx>,
+    raw_id: i32,
+) -> InterpResult<'tcx, ClockType> {
+    // 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 {
+        ClockType::Realtime
+    } else if raw_id == ecx.eval_libc_i32("CLOCK_MONOTONIC") {
+        ClockType::Monotonic
+    } else {
+        throw_unsup_format!("unsupported type of clock id: {raw_id}");
+    })
+}
+
 fn condattr_set_clock_id<'tcx>(
     ecx: &mut MiriInterpCx<'tcx>,
     attr_ptr: &OpTy<'tcx>,
@@ -331,16 +347,6 @@ fn cond_id_offset<'tcx>(ecx: &MiriInterpCx<'tcx>) -> InterpResult<'tcx, u64> {
     Ok(offset)
 }
 
-/// Determines whether this clock represents the real-time clock, CLOCK_REALTIME.
-fn is_cond_clock_realtime<'tcx>(ecx: &MiriInterpCx<'tcx>, clock_id: i32) -> bool {
-    // 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.
-    // However, we need to always be able to distinguish this from CLOCK_MONOTONIC.
-    clock_id == ecx.eval_libc_i32("CLOCK_REALTIME")
-        || (clock_id == 0 && clock_id != ecx.eval_libc_i32("CLOCK_MONOTONIC"))
-}
-
 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.
@@ -355,20 +361,30 @@ fn cond_clock_offset<'tcx>(ecx: &MiriInterpCx<'tcx>) -> u64 {
             .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!(
-            is_cond_clock_realtime(ecx, id),
+            matches!(id, ClockType::Realtime),
             "PTHREAD_COND_INITIALIZER is incompatible with our pthread_cond layout: clock is not CLOCK_REALTIME"
         );
     }
 
     offset
 }
 
+#[derive(Debug, Clone, Copy)]
+enum ClockType {
+    Realtime,
+    Monotonic,
+}
+
 #[derive(Debug)]
 /// Additional data that we attach with each cond instance.
 struct AdditionalCondData {
     /// The address of the cond.
     address: u64,
+
+    /// The clock type of the cond.
+    clock_type: ClockType,
 }
 
 fn cond_get_id<'tcx>(
@@ -377,8 +393,14 @@ 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)?, |_| {
-        Ok(Some(Box::new(AdditionalCondData { address })))
+    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_type = translate_clock_id(ecx, raw_id)?;
+        Ok(Some(Box::new(AdditionalCondData { address, clock_type })))
     })?;
 
     // Check that the mutex has not been moved since last use.
@@ -405,20 +427,6 @@ fn cond_get_clock_id<'tcx>(
     .to_i32()
 }
 
-fn cond_set_clock_id<'tcx>(
-    ecx: &mut MiriInterpCx<'tcx>,
-    cond_ptr: &OpTy<'tcx>,
-    clock_id: i32,
-) -> InterpResult<'tcx, ()> {
-    ecx.deref_pointer_and_write(
-        cond_ptr,
-        cond_clock_offset(ecx),
-        Scalar::from_i32(clock_id),
-        ecx.libc_ty_layout("pthread_cond_t"),
-        ecx.machine.layouts.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, ()> {
@@ -827,17 +835,16 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         } else {
             condattr_get_clock_id(this, attr_op)?
         };
+        let clock_type = translate_clock_id(this, clock_id)?;
 
         let cond = this.deref_pointer(cond_op)?;
         let address = cond.ptr().addr().bytes();
         this.condvar_create(
             &cond,
             cond_id_offset(this)?,
-            Some(Box::new(AdditionalCondData { address })),
+            Some(Box::new(AdditionalCondData { address, clock_type })),
         )?;
 
-        cond_set_clock_id(this, cond_op, clock_id)?;
-
         Ok(())
     }
 
@@ -891,7 +898,10 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         let mutex_id = mutex_get_id(this, mutex_op)?;
 
         // Extract the timeout.
-        let clock_id = cond_get_clock_id(this, cond_op)?;
+        let clock_type = this
+            .condvar_get_data::<AdditionalCondData>(id)
+            .expect("additional data should always be present for pthreads")
+            .clock_type;
         let duration = match this
             .read_timespec(&this.deref_pointer_as(abstime_op, this.libc_ty_layout("timespec"))?)?
         {
@@ -902,13 +912,13 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
                 return Ok(());
             }
         };
-        let timeout_clock = if is_cond_clock_realtime(this, clock_id) {
+        let timeout_clock = if matches!(clock_type, ClockType::Realtime) {
             this.check_no_isolation("`pthread_cond_timedwait` with `CLOCK_REALTIME`")?;
             TimeoutClock::RealTime
-        } else if clock_id == this.eval_libc_i32("CLOCK_MONOTONIC") {
+        } else if let ClockType::Monotonic = clock_type {
             TimeoutClock::Monotonic
         } else {
-            throw_unsup_format!("unsupported clock id: {}", clock_id);
+            throw_unsup_format!("unsupported clock id: {clock_type:?}");
         };
 
         this.condvar_wait(
@@ -933,7 +943,6 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
 
         // Destroying an uninit pthread_cond is UB, so check to make sure it's not uninit.
         cond_get_id(this, cond_op)?;
-        cond_get_clock_id(this, cond_op)?;
 
         // This might lead to false positives, see comment in pthread_mutexattr_destroy
         this.write_uninit(&this.deref_pointer_as(cond_op, this.libc_ty_layout("pthread_cond_t"))?)?;