Merge pull request #4499 from RalfJung/clockid

centralize clockid_t interpretation

Author: RalfJung

src/tools/miri/src/concurrency/thread.rs

@@ -375,7 +375,7 @@ impl Timeout {
 }
 
 /// The clock to use for the timeout you are asking for.
-#[derive(Debug, Copy, Clone)]
+#[derive(Debug, Copy, Clone, PartialEq)]
 pub enum TimeoutClock {
     Monotonic,
     RealTime,

src/tools/miri/src/shims/time.rs

@@ -17,73 +17,71 @@ pub fn system_time_to_duration<'tcx>(time: &SystemTime) -> InterpResult<'tcx, Du
 
 impl<'tcx> EvalContextExt<'tcx> for crate::MiriInterpCx<'tcx> {}
 pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
-    fn clock_gettime(
-        &mut self,
-        clk_id_op: &OpTy<'tcx>,
-        tp_op: &OpTy<'tcx>,
-        dest: &MPlaceTy<'tcx>,
-    ) -> InterpResult<'tcx> {
+    fn parse_clockid(&self, clk_id: Scalar) -> Option<TimeoutClock> {
         // This clock support is deliberately minimal because a lot of clock types have fiddly
         // properties (is it possible for Miri to be suspended independently of the host?). If you
         // have a use for another clock type, please open an issue.
+        let this = self.eval_context_ref();
 
-        let this = self.eval_context_mut();
-
-        this.assert_target_os_is_unix("clock_gettime");
-        let clockid_t_size = this.libc_ty_layout("clockid_t").size;
-
-        let clk_id = this.read_scalar(clk_id_op)?.to_int(clockid_t_size)?;
-        let tp = this.deref_pointer_as(tp_op, this.libc_ty_layout("timespec"))?;
-
-        let absolute_clocks;
-        let mut relative_clocks;
+        // Portable names that exist everywhere.
+        if clk_id == this.eval_libc("CLOCK_REALTIME") {
+            return Some(TimeoutClock::RealTime);
+        } else if clk_id == this.eval_libc("CLOCK_MONOTONIC") {
+            return Some(TimeoutClock::Monotonic);
+        }
 
+        // Some further platform-specific names we support.
         match this.tcx.sess.target.os.as_ref() {
             "linux" | "freebsd" | "android" => {
-                // Linux, Android, and FreeBSD have two main kinds of clocks. REALTIME clocks return the actual time since the
-                // Unix epoch, including effects which may cause time to move backwards such as NTP.
                 // Linux further distinguishes regular and "coarse" clocks, but the "coarse" version
-                // is just specified to be "faster and less precise", so we implement both the same way.
-                absolute_clocks = vec![
-                    this.eval_libc("CLOCK_REALTIME").to_int(clockid_t_size)?,
-                    this.eval_libc("CLOCK_REALTIME_COARSE").to_int(clockid_t_size)?,
-                ];
-                // The second kind is MONOTONIC clocks for which 0 is an arbitrary time point, but they are
-                // never allowed to go backwards. We don't need to do any additional monotonicity
-                // enforcement because std::time::Instant already guarantees that it is monotonic.
-                relative_clocks = vec![
-                    this.eval_libc("CLOCK_MONOTONIC").to_int(clockid_t_size)?,
-                    this.eval_libc("CLOCK_MONOTONIC_COARSE").to_int(clockid_t_size)?,
-                ];
+                // is just specified to be "faster and less precise", so we treat it like normal
+                // clocks.
+                if clk_id == this.eval_libc("CLOCK_REALTIME_COARSE") {
+                    return Some(TimeoutClock::RealTime);
+                } else if clk_id == this.eval_libc("CLOCK_MONOTONIC_COARSE") {
+                    return Some(TimeoutClock::Monotonic);
+                }
             }
             "macos" => {
-                absolute_clocks = vec![this.eval_libc("CLOCK_REALTIME").to_int(clockid_t_size)?];
-                relative_clocks = vec![this.eval_libc("CLOCK_MONOTONIC").to_int(clockid_t_size)?];
                 // `CLOCK_UPTIME_RAW` supposed to not increment while the system is asleep... but
                 // that's not really something a program running inside Miri can tell, anyway.
                 // We need to support it because std uses it.
-                relative_clocks.push(this.eval_libc("CLOCK_UPTIME_RAW").to_int(clockid_t_size)?);
-            }
-            "solaris" | "illumos" => {
-                // The REALTIME clock returns the actual time since the Unix epoch.
-                absolute_clocks = vec![this.eval_libc("CLOCK_REALTIME").to_int(clockid_t_size)?];
-                // MONOTONIC, in the other hand, is the high resolution, non-adjustable
-                // clock from an arbitrary time in the past.
-                // Note that the man page mentions HIGHRES but it is just
-                // an alias of MONOTONIC and the libc crate does not expose it anyway.
-                // https://docs.oracle.com/cd/E23824_01/html/821-1465/clock-gettime-3c.html
-                relative_clocks = vec![this.eval_libc("CLOCK_MONOTONIC").to_int(clockid_t_size)?];
+                if clk_id == this.eval_libc("CLOCK_UPTIME_RAW") {
+                    return Some(TimeoutClock::Monotonic);
+                }
             }
-            target => throw_unsup_format!("`clock_gettime` is not supported on target OS {target}"),
+            _ => {}
         }
 
-        let duration = if absolute_clocks.contains(&clk_id) {
-            this.check_no_isolation("`clock_gettime` with `REALTIME` clocks")?;
-            system_time_to_duration(&SystemTime::now())?
-        } else if relative_clocks.contains(&clk_id) {
-            this.machine.monotonic_clock.now().duration_since(this.machine.monotonic_clock.epoch())
-        } else {
-            return this.set_last_error_and_return(LibcError("EINVAL"), dest);
+        None
+    }
+
+    fn clock_gettime(
+        &mut self,
+        clk_id_op: &OpTy<'tcx>,
+        tp_op: &OpTy<'tcx>,
+        dest: &MPlaceTy<'tcx>,
+    ) -> InterpResult<'tcx> {
+        let this = self.eval_context_mut();
+
+        this.assert_target_os_is_unix("clock_gettime");
+
+        let clk_id = this.read_scalar(clk_id_op)?;
+        let tp = this.deref_pointer_as(tp_op, this.libc_ty_layout("timespec"))?;
+
+        let duration = match this.parse_clockid(clk_id) {
+            Some(TimeoutClock::RealTime) => {
+                this.check_no_isolation("`clock_gettime` with `REALTIME` clocks")?;
+                system_time_to_duration(&SystemTime::now())?
+            }
+            Some(TimeoutClock::Monotonic) =>
+                this.machine
+                    .monotonic_clock
+                    .now()
+                    .duration_since(this.machine.monotonic_clock.epoch()),
+            None => {
+                return this.set_last_error_and_return(LibcError("EINVAL"), dest);
+            }
         };
 
         let tv_sec = duration.as_secs();

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

@@ -228,26 +228,14 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         let abs_time_flag = flags == abs_time;
 
         let clock_id_place = this.project_field(ut, FieldIdx::from_u32(2))?;
-        let clock_id = this.read_scalar(&clock_id_place)?.to_i32()?;
-        let timeout_clock = this.translate_umtx_time_clock_id(clock_id)?;
+        let clock_id = this.read_scalar(&clock_id_place)?;
+        let Some(timeout_clock) = this.parse_clockid(clock_id) else {
+            throw_unsup_format!("unsupported clock")
+        };
+        if timeout_clock == TimeoutClock::RealTime {
+            this.check_no_isolation("`_umtx_op` with `CLOCK_REALTIME`")?;
+        }
 
         interp_ok(Some(UmtxTime { timeout: duration, abs_time: abs_time_flag, timeout_clock }))
     }
-
-    /// Translate raw FreeBSD clockid to a Miri TimeoutClock.
-    /// FIXME: share this code with the pthread and clock_gettime shims.
-    fn translate_umtx_time_clock_id(&mut self, raw_id: i32) -> InterpResult<'tcx, TimeoutClock> {
-        let this = self.eval_context_mut();
-
-        let timeout = if raw_id == this.eval_libc_i32("CLOCK_REALTIME") {
-            // RealTime clock can't be used in isolation mode.
-            this.check_no_isolation("`_umtx_op` with `CLOCK_REALTIME` timeout")?;
-            TimeoutClock::RealTime
-        } else if raw_id == this.eval_libc_i32("CLOCK_MONOTONIC") {
-            TimeoutClock::Monotonic
-        } else {
-            throw_unsup_format!("unsupported clock id {raw_id}");
-        };
-        interp_ok(timeout)
-    }
 }

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

@@ -297,14 +297,13 @@ fn condattr_clock_offset<'tcx>(ecx: &MiriInterpCx<'tcx>) -> InterpResult<'tcx, u
 fn condattr_get_clock_id<'tcx>(
     ecx: &MiriInterpCx<'tcx>,
     attr_ptr: &OpTy<'tcx>,
-) -> InterpResult<'tcx, i32> {
+) -> InterpResult<'tcx, Scalar> {
     ecx.deref_pointer_and_read(
         attr_ptr,
         condattr_clock_offset(ecx)?,
         ecx.libc_ty_layout("pthread_condattr_t"),
         ecx.machine.layouts.i32,
-    )?
-    .to_i32()
+    )
 }
 
 fn condattr_set_clock_id<'tcx>(
@@ -321,20 +320,6 @@ fn condattr_set_clock_id<'tcx>(
     )
 }
 
-/// Translates the clock from what is stored in pthread_condattr_t to our enum.
-fn condattr_translate_clock_id<'tcx>(
-    ecx: &MiriInterpCx<'tcx>,
-    raw_id: i32,
-) -> InterpResult<'tcx, ClockId> {
-    interp_ok(if raw_id == ecx.eval_libc_i32("CLOCK_REALTIME") {
-        ClockId::Realtime
-    } else if raw_id == ecx.eval_libc_i32("CLOCK_MONOTONIC") {
-        ClockId::Monotonic
-    } else {
-        throw_unsup_format!("unsupported clock id: {raw_id}");
-    })
-}
-
 // # pthread_cond_t
 // We store some data directly inside the type, ignoring the platform layout:
 // - init: u32
@@ -363,22 +348,16 @@ fn cond_init_offset<'tcx>(ecx: &MiriInterpCx<'tcx>) -> InterpResult<'tcx, Size>
     interp_ok(offset)
 }
 
-#[derive(Debug, Clone, Copy)]
-enum ClockId {
-    Realtime,
-    Monotonic,
-}
-
 #[derive(Debug, Clone)]
 struct PthreadCondvar {
     condvar_ref: CondvarRef,
-    clock: ClockId,
+    clock: TimeoutClock,
 }
 
 fn cond_create<'tcx>(
     ecx: &mut MiriInterpCx<'tcx>,
     cond_ptr: &OpTy<'tcx>,
-    clock: ClockId,
+    clock: TimeoutClock,
 ) -> InterpResult<'tcx, PthreadCondvar> {
     let cond = ecx.deref_pointer_as(cond_ptr, ecx.libc_ty_layout("pthread_cond_t"))?;
     let data = PthreadCondvar { condvar_ref: CondvarRef::new(), clock };
@@ -407,7 +386,10 @@ where
                 throw_unsup_format!("unsupported static initializer used for `pthread_cond_t`");
             }
             // This used the static initializer. The clock there is always CLOCK_REALTIME.
-            interp_ok(PthreadCondvar { condvar_ref: CondvarRef::new(), clock: ClockId::Realtime })
+            interp_ok(PthreadCondvar {
+                condvar_ref: CondvarRef::new(),
+                clock: TimeoutClock::RealTime,
+            })
         },
     )
 }
@@ -742,11 +724,9 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
     ) -> InterpResult<'tcx, Scalar> {
         let this = self.eval_context_mut();
 
-        let clock_id = this.read_scalar(clock_id_op)?.to_i32()?;
-        if clock_id == this.eval_libc_i32("CLOCK_REALTIME")
-            || clock_id == this.eval_libc_i32("CLOCK_MONOTONIC")
-        {
-            condattr_set_clock_id(this, attr_op, clock_id)?;
+        let clock_id = this.read_scalar(clock_id_op)?;
+        if this.parse_clockid(clock_id).is_some() {
+            condattr_set_clock_id(this, attr_op, clock_id.to_i32()?)?;
         } else {
             let einval = this.eval_libc_i32("EINVAL");
             return interp_ok(Scalar::from_i32(einval));
@@ -764,7 +744,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
 
         let clock_id = condattr_get_clock_id(this, attr_op)?;
         this.write_scalar(
-            Scalar::from_i32(clock_id),
+            clock_id,
             &this.deref_pointer_as(clk_id_op, this.libc_ty_layout("clockid_t"))?,
         )?;
 
@@ -799,13 +779,16 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         let attr = this.read_pointer(attr_op)?;
         // Default clock if `attr` is null, and on macOS where there is no clock attribute.
         let clock_id = if this.ptr_is_null(attr)? || this.tcx.sess.target.os == "macos" {
-            this.eval_libc_i32("CLOCK_REALTIME")
+            this.eval_libc("CLOCK_REALTIME")
         } else {
             condattr_get_clock_id(this, attr_op)?
         };
-        let clock_id = condattr_translate_clock_id(this, clock_id)?;
+        let Some(clock) = this.parse_clockid(clock_id) else {
+            // This is UB since this situation cannot arise when using pthread_condattr_setclock.
+            throw_ub_format!("pthread_cond_init: invalid attributes (unsupported clock)")
+        };
 
-        cond_create(this, cond_op, clock_id)?;
+        cond_create(this, cond_op, clock)?;
 
         interp_ok(())
     }
@@ -870,18 +853,14 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
                 return interp_ok(());
             }
         };
-        let timeout_clock = match data.clock {
-            ClockId::Realtime => {
-                this.check_no_isolation("`pthread_cond_timedwait` with `CLOCK_REALTIME`")?;
-                TimeoutClock::RealTime
-            }
-            ClockId::Monotonic => TimeoutClock::Monotonic,
-        };
+        if data.clock == TimeoutClock::RealTime {
+            this.check_no_isolation("`pthread_cond_timedwait` with `CLOCK_REALTIME`")?;
+        }
 
         this.condvar_wait(
             data.condvar_ref,
             mutex_ref,
-            Some((timeout_clock, TimeoutAnchor::Absolute, duration)),
+            Some((data.clock, TimeoutAnchor::Absolute, duration)),
             Scalar::from_i32(0),
             this.eval_libc("ETIMEDOUT"), // retval_timeout
             dest.clone(),