Auto merge of #3663 - RalfJung:timeouts, r=RalfJung

don't panic if time computaton overflows

Let the thread blocking system handle timeout computation, and on overflows we just set the timeout to 1h.

Author: bors

src/clock.rs

@@ -20,14 +20,20 @@ enum InstantKind {
 }
 
 impl Instant {
-    pub fn checked_add(&self, duration: Duration) -> Option<Instant> {
+    /// Will try to add `duration`, but if that overflows it may add less.
+    pub fn add_lossy(&self, duration: Duration) -> Instant {
         match self.kind {
-            InstantKind::Host(instant) =>
-                instant.checked_add(duration).map(|i| Instant { kind: InstantKind::Host(i) }),
-            InstantKind::Virtual { nanoseconds } =>
-                nanoseconds
-                    .checked_add(duration.as_nanos())
-                    .map(|nanoseconds| Instant { kind: InstantKind::Virtual { nanoseconds } }),
+            InstantKind::Host(instant) => {
+                // If this overflows, try adding just 1h and assume that will not overflow.
+                let i = instant
+                    .checked_add(duration)
+                    .unwrap_or_else(|| instant.checked_add(Duration::from_secs(3600)).unwrap());
+                Instant { kind: InstantKind::Host(i) }
+            }
+            InstantKind::Virtual { nanoseconds } => {
+                let n = nanoseconds.saturating_add(duration.as_nanos());
+                Instant { kind: InstantKind::Virtual { nanoseconds: n } }
+            }
         }
     }
 
@@ -63,8 +69,9 @@ pub struct Clock {
 #[derive(Debug)]
 enum ClockKind {
     Host {
-        /// The "time anchor" for this machine's monotone clock.
-        time_anchor: StdInstant,
+        /// The "epoch" for this machine's monotone clock:
+        /// the moment we consider to be time = 0.
+        epoch: StdInstant,
     },
     Virtual {
         /// The "current virtual time".
@@ -76,7 +83,7 @@ impl Clock {
     /// Create a new clock based on the availability of communication with the host.
     pub fn new(communicate: bool) -> Self {
         let kind = if communicate {
-            ClockKind::Host { time_anchor: StdInstant::now() }
+            ClockKind::Host { epoch: StdInstant::now() }
         } else {
             ClockKind::Virtual { nanoseconds: 0.into() }
         };
@@ -111,10 +118,10 @@ impl Clock {
         }
     }
 
-    /// Return the `anchor` instant, to convert between monotone instants and durations relative to the anchor.
-    pub fn anchor(&self) -> Instant {
+    /// Return the `epoch` instant (time = 0), to convert between monotone instants and absolute durations.
+    pub fn epoch(&self) -> Instant {
         match &self.kind {
-            ClockKind::Host { time_anchor } => Instant { kind: InstantKind::Host(*time_anchor) },
+            ClockKind::Host { epoch } => Instant { kind: InstantKind::Host(*epoch) },
             ClockKind::Virtual { .. } => Instant { kind: InstantKind::Virtual { nanoseconds: 0 } },
         }
     }

src/concurrency/sync.rs

@@ -1,5 +1,6 @@
 use std::collections::{hash_map::Entry, VecDeque};
 use std::ops::Not;
+use std::time::Duration;
 
 use rustc_data_structures::fx::FxHashMap;
 use rustc_index::{Idx, IndexVec};
@@ -623,7 +624,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         &mut self,
         condvar: CondvarId,
         mutex: MutexId,
-        timeout: Option<Timeout>,
+        timeout: Option<(TimeoutClock, TimeoutAnchor, Duration)>,
         retval_succ: Scalar,
         retval_timeout: Scalar,
         dest: MPlaceTy<'tcx>,
@@ -704,7 +705,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         &mut self,
         addr: u64,
         bitset: u32,
-        timeout: Option<Timeout>,
+        timeout: Option<(TimeoutClock, TimeoutAnchor, Duration)>,
         retval_succ: Scalar,
         retval_timeout: Scalar,
         dest: MPlaceTy<'tcx>,

src/concurrency/thread.rs

@@ -407,7 +407,7 @@ impl VisitProvenance for Frame<'_, Provenance, FrameExtra<'_>> {
 
 /// The moment in time when a blocked thread should be woken up.
 #[derive(Debug)]
-pub enum Timeout {
+enum Timeout {
     Monotonic(Instant),
     RealTime(SystemTime),
 }
@@ -421,6 +421,34 @@ impl Timeout {
                 time.duration_since(SystemTime::now()).unwrap_or(Duration::ZERO),
         }
     }
+
+    /// Will try to add `duration`, but if that overflows it may add less.
+    fn add_lossy(&self, duration: Duration) -> Self {
+        match self {
+            Timeout::Monotonic(i) => Timeout::Monotonic(i.add_lossy(duration)),
+            Timeout::RealTime(s) => {
+                // If this overflows, try adding just 1h and assume that will not overflow.
+                Timeout::RealTime(
+                    s.checked_add(duration)
+                        .unwrap_or_else(|| s.checked_add(Duration::from_secs(3600)).unwrap()),
+                )
+            }
+        }
+    }
+}
+
+/// The clock to use for the timeout you are asking for.
+#[derive(Debug, Copy, Clone)]
+pub enum TimeoutClock {
+    Monotonic,
+    RealTime,
+}
+
+/// Whether the timeout is relative or absolute.
+#[derive(Debug, Copy, Clone)]
+pub enum TimeoutAnchor {
+    Relative,
+    Absolute,
 }
 
 /// A set of threads.
@@ -995,13 +1023,30 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
     fn block_thread(
         &mut self,
         reason: BlockReason,
-        timeout: Option<Timeout>,
+        timeout: Option<(TimeoutClock, TimeoutAnchor, Duration)>,
         callback: impl UnblockCallback<'tcx> + 'tcx,
     ) {
         let this = self.eval_context_mut();
-        if !this.machine.communicate() && matches!(timeout, Some(Timeout::RealTime(..))) {
-            panic!("cannot have `RealTime` callback with isolation enabled!")
-        }
+        let timeout = timeout.map(|(clock, anchor, duration)| {
+            let anchor = match clock {
+                TimeoutClock::RealTime => {
+                    assert!(
+                        this.machine.communicate(),
+                        "cannot have `RealTime` timeout with isolation enabled!"
+                    );
+                    Timeout::RealTime(match anchor {
+                        TimeoutAnchor::Absolute => SystemTime::UNIX_EPOCH,
+                        TimeoutAnchor::Relative => SystemTime::now(),
+                    })
+                }
+                TimeoutClock::Monotonic =>
+                    Timeout::Monotonic(match anchor {
+                        TimeoutAnchor::Absolute => this.machine.clock.epoch(),
+                        TimeoutAnchor::Relative => this.machine.clock.now(),
+                    }),
+            };
+            anchor.add_lossy(duration)
+        });
         this.machine.threads.block_thread(reason, timeout, callback);
     }
 

src/lib.rs

@@ -132,8 +132,8 @@ pub use crate::concurrency::{
     init_once::{EvalContextExt as _, InitOnceId},
     sync::{CondvarId, EvalContextExt as _, MutexId, RwLockId, SynchronizationObjects},
     thread::{
-        BlockReason, EvalContextExt as _, StackEmptyCallback, ThreadId, ThreadManager, Timeout,
-        UnblockCallback,
+        BlockReason, EvalContextExt as _, StackEmptyCallback, ThreadId, ThreadManager,
+        TimeoutAnchor, TimeoutClock, UnblockCallback,
     },
 };
 pub use crate::diagnostics::{

src/shims/time.rs

@@ -78,7 +78,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
             this.check_no_isolation("`clock_gettime` with `REALTIME` clocks")?;
             system_time_to_duration(&SystemTime::now())?
         } else if relative_clocks.contains(&clk_id) {
-            this.machine.clock.now().duration_since(this.machine.clock.anchor())
+            this.machine.clock.now().duration_since(this.machine.clock.epoch())
         } else {
             let einval = this.eval_libc("EINVAL");
             this.set_last_error(einval)?;
@@ -246,7 +246,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
 
         // QueryPerformanceCounter uses a hardware counter as its basis.
         // Miri will emulate a counter with a resolution of 1 nanosecond.
-        let duration = this.machine.clock.now().duration_since(this.machine.clock.anchor());
+        let duration = this.machine.clock.now().duration_since(this.machine.clock.epoch());
         let qpc = i64::try_from(duration.as_nanos()).map_err(|_| {
             err_unsup_format!("programs running longer than 2^63 nanoseconds are not supported")
         })?;
@@ -282,7 +282,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
 
         // This returns a u64, with time units determined dynamically by `mach_timebase_info`.
         // We return plain nanoseconds.
-        let duration = this.machine.clock.now().duration_since(this.machine.clock.anchor());
+        let duration = this.machine.clock.now().duration_since(this.machine.clock.epoch());
         let res = u64::try_from(duration.as_nanos()).map_err(|_| {
             err_unsup_format!("programs running longer than 2^64 nanoseconds are not supported")
         })?;
@@ -323,16 +323,10 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
                 return Ok(-1);
             }
         };
-        // If adding the duration overflows, let's just sleep for an hour. Waking up early is always acceptable.
-        let now = this.machine.clock.now();
-        let timeout_time = now
-            .checked_add(duration)
-            .unwrap_or_else(|| now.checked_add(Duration::from_secs(3600)).unwrap());
-        let timeout_time = Timeout::Monotonic(timeout_time);
 
         this.block_thread(
             BlockReason::Sleep,
-            Some(timeout_time),
+            Some((TimeoutClock::Monotonic, TimeoutAnchor::Relative, duration)),
             callback!(
                 @capture<'tcx> {}
                 @unblock = |_this| { panic!("sleeping thread unblocked before time is up") }
@@ -351,12 +345,10 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         let timeout_ms = this.read_scalar(timeout)?.to_u32()?;
 
         let duration = Duration::from_millis(timeout_ms.into());
-        let timeout_time = this.machine.clock.now().checked_add(duration).unwrap();
-        let timeout_time = Timeout::Monotonic(timeout_time);
 
         this.block_thread(
             BlockReason::Sleep,
-            Some(timeout_time),
+            Some((TimeoutClock::Monotonic, TimeoutAnchor::Relative, duration)),
             callback!(
                 @capture<'tcx> {}
                 @unblock = |_this| { panic!("sleeping thread unblocked before time is up") }

src/shims/unix/linux/sync.rs

@@ -1,5 +1,3 @@
-use std::time::SystemTime;
-
 use crate::*;
 
 /// Implementation of the SYS_futex syscall.
@@ -84,15 +82,9 @@ pub fn futex<'tcx>(
             }
 
             let timeout = this.deref_pointer_as(&args[3], this.libc_ty_layout("timespec"))?;
-            let timeout_time = if this.ptr_is_null(timeout.ptr())? {
+            let timeout = if this.ptr_is_null(timeout.ptr())? {
                 None
             } else {
-                let realtime = op & futex_realtime == futex_realtime;
-                if realtime {
-                    this.check_no_isolation(
-                        "`futex` syscall with `op=FUTEX_WAIT` and non-null timeout with `FUTEX_CLOCK_REALTIME`",
-                    )?;
-                }
                 let duration = match this.read_timespec(&timeout)? {
                     Some(duration) => duration,
                     None => {
@@ -102,23 +94,22 @@ pub fn futex<'tcx>(
                         return Ok(());
                     }
                 };
-                Some(if wait_bitset {
+                let timeout_clock = if op & futex_realtime == futex_realtime {
+                    this.check_no_isolation(
+                        "`futex` syscall with `op=FUTEX_WAIT` and non-null timeout with `FUTEX_CLOCK_REALTIME`",
+                    )?;
+                    TimeoutClock::RealTime
+                } else {
+                    TimeoutClock::Monotonic
+                };
+                let timeout_anchor = if wait_bitset {
                     // FUTEX_WAIT_BITSET uses an absolute timestamp.
-                    if realtime {
-                        Timeout::RealTime(SystemTime::UNIX_EPOCH.checked_add(duration).unwrap())
-                    } else {
-                        Timeout::Monotonic(
-                            this.machine.clock.anchor().checked_add(duration).unwrap(),
-                        )
-                    }
+                    TimeoutAnchor::Absolute
                 } else {
                     // FUTEX_WAIT uses a relative timestamp.
-                    if realtime {
-                        Timeout::RealTime(SystemTime::now().checked_add(duration).unwrap())
-                    } else {
-                        Timeout::Monotonic(this.machine.clock.now().checked_add(duration).unwrap())
-                    }
-                })
+                    TimeoutAnchor::Relative
+                };
+                Some((timeout_clock, timeout_anchor, duration))
             };
             // There may be a concurrent thread changing the value of addr
             // and then invoking the FUTEX_WAKE syscall. It is critical that the
@@ -172,7 +163,7 @@ pub fn futex<'tcx>(
                 this.futex_wait(
                     addr_usize,
                     bitset,
-                    timeout_time,
+                    timeout,
                     Scalar::from_target_isize(0, this), // retval_succ
                     Scalar::from_target_isize(-1, this), // retval_timeout
                     dest.clone(),

src/shims/unix/sync.rs

@@ -1,5 +1,4 @@
 use std::sync::atomic::{AtomicBool, Ordering};
-use std::time::SystemTime;
 
 use rustc_target::abi::Size;
 
@@ -849,19 +848,19 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
                 return Ok(());
             }
         };
-        let timeout_time = if is_cond_clock_realtime(this, clock_id) {
+        let timeout_clock = if is_cond_clock_realtime(this, clock_id) {
             this.check_no_isolation("`pthread_cond_timedwait` with `CLOCK_REALTIME`")?;
-            Timeout::RealTime(SystemTime::UNIX_EPOCH.checked_add(duration).unwrap())
+            TimeoutClock::RealTime
         } else if clock_id == this.eval_libc_i32("CLOCK_MONOTONIC") {
-            Timeout::Monotonic(this.machine.clock.anchor().checked_add(duration).unwrap())
+            TimeoutClock::Monotonic
         } else {
             throw_unsup_format!("unsupported clock id: {}", clock_id);
         };
 
         this.condvar_wait(
             id,
             mutex_id,
-            Some(timeout_time),
+            Some((timeout_clock, TimeoutAnchor::Absolute, duration)),
             Scalar::from_i32(0),
             this.eval_libc("ETIMEDOUT"), // retval_timeout
             dest.clone(),

src/shims/windows/sync.rs

@@ -157,11 +157,11 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         };
         let size = Size::from_bytes(size);
 
-        let timeout_time = if timeout_ms == this.eval_windows_u32("c", "INFINITE") {
+        let timeout = if timeout_ms == this.eval_windows_u32("c", "INFINITE") {
             None
         } else {
             let duration = Duration::from_millis(timeout_ms.into());
-            Some(Timeout::Monotonic(this.machine.clock.now().checked_add(duration).unwrap()))
+            Some((TimeoutClock::Monotonic, TimeoutAnchor::Relative, duration))
         };
 
         // See the Linux futex implementation for why this fence exists.
@@ -177,7 +177,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
             this.futex_wait(
                 addr,
                 u32::MAX, // bitset
-                timeout_time,
+                timeout,
                 Scalar::from_i32(1), // retval_succ
                 Scalar::from_i32(0), // retval_timeout
                 dest.clone(),