address comments

Author: Mandragorian

src/shims/unix/sync.rs

@@ -286,19 +286,16 @@ fn condattr_get_clock_id<'tcx>(
     .to_i32()
 }
 
-fn translate_clock_id<'tcx>(
-    ecx: &MiriInterpCx<'tcx>,
-    raw_id: i32,
-) -> InterpResult<'tcx, ClockType> {
+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 {
-        ClockType::Realtime
+        ClockId::Realtime
     } else if raw_id == ecx.eval_libc_i32("CLOCK_MONOTONIC") {
-        ClockType::Monotonic
+        ClockId::Monotonic
     } else {
-        throw_unsup_format!("unsupported type of clock id: {raw_id}");
+        throw_unsup_format!("unsupported clock id: {raw_id}");
     })
 }
 
@@ -363,7 +360,7 @@ fn cond_clock_offset<'tcx>(ecx: &MiriInterpCx<'tcx>) -> u64 {
         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, ClockType::Realtime),
+            matches!(id, ClockId::Realtime),
             "PTHREAD_COND_INITIALIZER is incompatible with our pthread_cond layout: clock is not CLOCK_REALTIME"
         );
     }
@@ -372,7 +369,7 @@ fn cond_clock_offset<'tcx>(ecx: &MiriInterpCx<'tcx>) -> u64 {
 }
 
 #[derive(Debug, Clone, Copy)]
-enum ClockType {
+enum ClockId {
     Realtime,
     Monotonic,
 }
@@ -383,8 +380,8 @@ struct AdditionalCondData {
     /// The address of the cond.
     address: u64,
 
-    /// The clock type of the cond.
-    clock_type: ClockType,
+    /// The clock id of the cond.
+    clock_id: ClockId,
 }
 
 fn cond_get_id<'tcx>(
@@ -399,8 +396,8 @@ fn cond_get_id<'tcx>(
         } 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 })))
+        let clock_id = translate_clock_id(ecx, raw_id)?;
+        Ok(Some(Box::new(AdditionalCondData { address, clock_id })))
     })?;
 
     // Check that the mutex has not been moved since last use.
@@ -835,14 +832,14 @@ 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 clock_id = 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, clock_type })),
+            Some(Box::new(AdditionalCondData { address, clock_id })),
         )?;
 
         Ok(())
@@ -898,10 +895,10 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         let mutex_id = mutex_get_id(this, mutex_op)?;
 
         // Extract the timeout.
-        let clock_type = this
+        let clock_id = this
             .condvar_get_data::<AdditionalCondData>(id)
             .expect("additional data should always be present for pthreads")
-            .clock_type;
+            .clock_id;
         let duration = match this
             .read_timespec(&this.deref_pointer_as(abstime_op, this.libc_ty_layout("timespec"))?)?
         {
@@ -912,13 +909,12 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
                 return Ok(());
             }
         };
-        let timeout_clock = if matches!(clock_type, ClockType::Realtime) {
-            this.check_no_isolation("`pthread_cond_timedwait` with `CLOCK_REALTIME`")?;
-            TimeoutClock::RealTime
-        } else if let ClockType::Monotonic = clock_type {
-            TimeoutClock::Monotonic
-        } else {
-            throw_unsup_format!("unsupported clock id: {clock_type:?}");
+        let timeout_clock = match clock_id {
+            ClockId::Realtime => {
+                this.check_no_isolation("`pthread_cond_timedwait` with `CLOCK_REALTIME`")?;
+                TimeoutClock::RealTime
+            }
+            ClockId::Monotonic => TimeoutClock::Monotonic,
         };
 
         this.condvar_wait(
@@ -934,16 +930,16 @@ 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();
 
         let id = cond_get_id(this, cond_op)?;
         if this.condvar_is_awaited(id) {
             throw_ub_format!("destroying an awaited conditional variable");
         }
 
-        // Destroying an uninit pthread_cond is UB, so check to make sure it's not uninit.
-        cond_get_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"))?)?;
         // FIXME: delete interpreter state associated with this condvar.