From 9d8ef1160747a4d033f21803770641f2deb32b25 Mon Sep 17 00:00:00 2001 From: The8472 Date: Fri, 15 Oct 2021 23:55:23 +0200 Subject: [PATCH 1/4] make Instant::{duration_since, elapsed, sub} saturating and remove workarounds This removes all mutex/atomics based workarounds for non-monotonic clocks and makes the previously panicking methods saturating instead. Effectively this moves the monotonization from `Instant` construction to the comparisons. This has some observable effects, especially on platforms without monotonic clocks: * Incorrectly ordered Instant comparisons no longer panic. This may hide some programming errors until someone actually looks at the resulting `Duration` * `checked_duration_since` will now return `None` in more cases. Previously it only happened when one compared instants obtained in the wrong order or manually created ones. Now it also does on backslides. The upside is reduced complexity and lower overhead of `Instant::now`. --- library/std/src/sys/hermit/time.rs | 8 -- library/std/src/sys/itron/time.rs | 9 -- library/std/src/sys/sgx/time.rs | 8 -- library/std/src/sys/unix/time.rs | 19 ---- library/std/src/sys/unsupported/time.rs | 8 -- library/std/src/sys/wasi/time.rs | 8 -- library/std/src/sys/windows/time.rs | 8 -- library/std/src/time.rs | 85 ++++++----------- library/std/src/time/monotonic.rs | 116 ------------------------ library/std/src/time/tests.rs | 31 +------ 10 files changed, 30 insertions(+), 270 deletions(-) delete mode 100644 library/std/src/time/monotonic.rs diff --git a/library/std/src/sys/hermit/time.rs b/library/std/src/sys/hermit/time.rs index c02de17c1fc3a..27173de630729 100644 --- a/library/std/src/sys/hermit/time.rs +++ b/library/std/src/sys/hermit/time.rs @@ -115,14 +115,6 @@ impl Instant { Instant { t: time } } - pub const fn zero() -> Instant { - Instant { t: Timespec::zero() } - } - - pub fn actually_monotonic() -> bool { - true - } - pub fn checked_sub_instant(&self, other: &Instant) -> Option { self.t.sub_timespec(&other.t).ok() } diff --git a/library/std/src/sys/itron/time.rs b/library/std/src/sys/itron/time.rs index 6a992ad1d3c75..25f13ee441aca 100644 --- a/library/std/src/sys/itron/time.rs +++ b/library/std/src/sys/itron/time.rs @@ -14,15 +14,6 @@ impl Instant { } } - pub const fn zero() -> Instant { - Instant(0) - } - - pub fn actually_monotonic() -> bool { - // There are ways to change the system time - false - } - pub fn checked_sub_instant(&self, other: &Instant) -> Option { self.0.checked_sub(other.0).map(|ticks| { // `SYSTIM` is measured in microseconds diff --git a/library/std/src/sys/sgx/time.rs b/library/std/src/sys/sgx/time.rs index e2f6e6dba695d..db4cf2804bf13 100644 --- a/library/std/src/sys/sgx/time.rs +++ b/library/std/src/sys/sgx/time.rs @@ -25,14 +25,6 @@ impl Instant { pub fn checked_sub_duration(&self, other: &Duration) -> Option { Some(Instant(self.0.checked_sub(*other)?)) } - - pub fn actually_monotonic() -> bool { - false - } - - pub const fn zero() -> Instant { - Instant(Duration::from_secs(0)) - } } impl SystemTime { diff --git a/library/std/src/sys/unix/time.rs b/library/std/src/sys/unix/time.rs index 824283ef6c41e..59ddd1aa92f81 100644 --- a/library/std/src/sys/unix/time.rs +++ b/library/std/src/sys/unix/time.rs @@ -154,14 +154,6 @@ mod inner { Instant { t: unsafe { mach_absolute_time() } } } - pub const fn zero() -> Instant { - Instant { t: 0 } - } - - pub fn actually_monotonic() -> bool { - true - } - pub fn checked_sub_instant(&self, other: &Instant) -> Option { let diff = self.t.checked_sub(other.t)?; let info = info(); @@ -296,17 +288,6 @@ mod inner { Instant { t: now(libc::CLOCK_MONOTONIC) } } - pub const fn zero() -> Instant { - Instant { t: Timespec::zero() } - } - - pub fn actually_monotonic() -> bool { - (cfg!(target_os = "linux") && cfg!(target_arch = "x86_64")) - || (cfg!(target_os = "linux") && cfg!(target_arch = "x86")) - || (cfg!(target_os = "linux") && cfg!(target_arch = "aarch64")) - || cfg!(target_os = "fuchsia") - } - pub fn checked_sub_instant(&self, other: &Instant) -> Option { self.t.sub_timespec(&other.t).ok() } diff --git a/library/std/src/sys/unsupported/time.rs b/library/std/src/sys/unsupported/time.rs index 8aaf1777f2427..6d67b538a96bf 100644 --- a/library/std/src/sys/unsupported/time.rs +++ b/library/std/src/sys/unsupported/time.rs @@ -13,14 +13,6 @@ impl Instant { panic!("time not implemented on this platform") } - pub const fn zero() -> Instant { - Instant(Duration::from_secs(0)) - } - - pub fn actually_monotonic() -> bool { - false - } - pub fn checked_sub_instant(&self, other: &Instant) -> Option { self.0.checked_sub(other.0) } diff --git a/library/std/src/sys/wasi/time.rs b/library/std/src/sys/wasi/time.rs index db0ddecf0c629..088585654b948 100644 --- a/library/std/src/sys/wasi/time.rs +++ b/library/std/src/sys/wasi/time.rs @@ -25,14 +25,6 @@ impl Instant { Instant(current_time(wasi::CLOCKID_MONOTONIC)) } - pub const fn zero() -> Instant { - Instant(Duration::from_secs(0)) - } - - pub fn actually_monotonic() -> bool { - true - } - pub fn checked_sub_instant(&self, other: &Instant) -> Option { self.0.checked_sub(other.0) } diff --git a/library/std/src/sys/windows/time.rs b/library/std/src/sys/windows/time.rs index 91e4f7654840d..a04908b541cdb 100644 --- a/library/std/src/sys/windows/time.rs +++ b/library/std/src/sys/windows/time.rs @@ -41,14 +41,6 @@ impl Instant { perf_counter::PerformanceCounterInstant::now().into() } - pub fn actually_monotonic() -> bool { - false - } - - pub const fn zero() -> Instant { - Instant { t: Duration::from_secs(0) } - } - pub fn checked_sub_instant(&self, other: &Instant) -> Option { // On windows there's a threshold below which we consider two timestamps // equivalent due to measurement error. For more details + doc link, diff --git a/library/std/src/time.rs b/library/std/src/time.rs index 2d2b96c8bcec7..0d6ace7536bf4 100644 --- a/library/std/src/time.rs +++ b/library/std/src/time.rs @@ -31,7 +31,6 @@ #![stable(feature = "time", since = "1.3.0")] -mod monotonic; #[cfg(test)] mod tests; @@ -125,6 +124,23 @@ pub use core::time::FromFloatSecsError; /// > structure cannot represent the new point in time. /// /// [`add`]: Instant::add +/// +/// ## Monotonicity +/// +/// On all platforms `Instant` will try to use an OS API that guarantees monotonic behavior +/// if available, which is the case for all [tier 1] platforms. +/// In practice such guarantees are – under rare circumstances – broken by hardware, virtualization +/// or operating system bugs. To work around these bugs and platforms not offering monotonic clocks +/// [`duration_since`], [`elapsed`] and [`sub`] saturate to zero. [`checked_duration_since`] can +/// be used to detect and handle situations where monotonicity is violated, or `Instant`s are +/// subtracted in the wrong order. +/// +/// [tier 1]: https://doc.rust-lang.org/rustc/platform-support.html +/// [`duration_since`]: Instant::duration_since +/// [`elapsed`]: Instant::elapsed +/// [`sub`]: Instant::sub +/// [`checked_duration_since`]: Instant::checked_duration_since +/// #[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] #[stable(feature = "time2", since = "1.8.0")] pub struct Instant(time::Instant); @@ -247,59 +263,12 @@ impl Instant { #[must_use] #[stable(feature = "time2", since = "1.8.0")] pub fn now() -> Instant { - let os_now = time::Instant::now(); - - // And here we come upon a sad state of affairs. The whole point of - // `Instant` is that it's monotonically increasing. We've found in the - // wild, however, that it's not actually monotonically increasing for - // one reason or another. These appear to be OS and hardware level bugs, - // and there's not really a whole lot we can do about them. Here's a - // taste of what we've found: - // - // * #48514 - OpenBSD, x86_64 - // * #49281 - linux arm64 and s390x - // * #51648 - windows, x86 - // * #56560 - windows, x86_64, AWS - // * #56612 - windows, x86, vm (?) - // * #56940 - linux, arm64 - // * https://bugzilla.mozilla.org/show_bug.cgi?id=1487778 - a similar - // Firefox bug - // - // It seems that this just happens a lot in the wild. - // We're seeing panics across various platforms where consecutive calls - // to `Instant::now`, such as via the `elapsed` function, are panicking - // as they're going backwards. Placed here is a last-ditch effort to try - // to fix things up. We keep a global "latest now" instance which is - // returned instead of what the OS says if the OS goes backwards. - // - // To hopefully mitigate the impact of this, a few platforms are - // excluded as "these at least haven't gone backwards yet". - // - // While issues have been seen on arm64 platforms the Arm architecture - // requires that the counter monotonically increases and that it must - // provide a uniform view of system time (e.g. it must not be possible - // for a core to receive a message from another core with a time stamp - // and observe time going backwards (ARM DDI 0487G.b D11.1.2). While - // there have been a few 64bit SoCs that have bugs which cause time to - // not monoticially increase, these have been fixed in the Linux kernel - // and we shouldn't penalize all Arm SoCs for those who refuse to - // update their kernels: - // SUN50I_ERRATUM_UNKNOWN1 - Allwinner A64 / Pine A64 - fixed in 5.1 - // FSL_ERRATUM_A008585 - Freescale LS2080A/LS1043A - fixed in 4.10 - // HISILICON_ERRATUM_161010101 - Hisilicon 1610 - fixed in 4.11 - // ARM64_ERRATUM_858921 - Cortex A73 - fixed in 4.12 - if time::Instant::actually_monotonic() { - return Instant(os_now); - } - - Instant(monotonic::monotonize(os_now)) + Instant(time::Instant::now()) } - /// Returns the amount of time elapsed from another instant to this one. - /// - /// # Panics + /// Returns the amount of time elapsed from another instant to this one, + /// or zero duration if that instant is later than this one. /// - /// This function will panic if `earlier` is later than `self`. /// /// # Examples /// @@ -311,16 +280,22 @@ impl Instant { /// sleep(Duration::new(1, 0)); /// let new_now = Instant::now(); /// println!("{:?}", new_now.duration_since(now)); + /// println!("{:?}", now.duration_since(new_now)); // 0ns /// ``` #[must_use] #[stable(feature = "time2", since = "1.8.0")] pub fn duration_since(&self, earlier: Instant) -> Duration { - self.0.checked_sub_instant(&earlier.0).expect("supplied instant is later than self") + self.checked_duration_since(earlier).unwrap_or_default() } /// Returns the amount of time elapsed from another instant to this one, /// or None if that instant is later than this one. /// + /// Due to [monotonicity bugs], even under correct logical ordering of the passed `Instant`s, + /// this method can return `None`. + /// + /// [monotonicity bugs]: Instant#monotonicity + /// /// # Examples /// /// ```no_run @@ -362,12 +337,6 @@ impl Instant { /// Returns the amount of time elapsed since this instant was created. /// - /// # Panics - /// - /// This function may panic if the current time is earlier than this - /// instant, which is something that can happen if an `Instant` is - /// produced synthetically. - /// /// # Examples /// /// ```no_run diff --git a/library/std/src/time/monotonic.rs b/library/std/src/time/monotonic.rs deleted file mode 100644 index 64f16245c2b16..0000000000000 --- a/library/std/src/time/monotonic.rs +++ /dev/null @@ -1,116 +0,0 @@ -use crate::sys::time; - -#[inline] -pub(super) fn monotonize(raw: time::Instant) -> time::Instant { - inner::monotonize(raw) -} - -#[cfg(any(all(target_has_atomic = "64", not(target_has_atomic = "128")), target_arch = "aarch64"))] -pub mod inner { - use crate::sync::atomic::AtomicU64; - use crate::sync::atomic::Ordering::*; - use crate::sys::time; - use crate::time::Duration; - - pub(in crate::time) const ZERO: time::Instant = time::Instant::zero(); - - // bits 30 and 31 are never used since the nanoseconds part never exceeds 10^9 - const UNINITIALIZED: u64 = 0b11 << 30; - static MONO: AtomicU64 = AtomicU64::new(UNINITIALIZED); - - #[inline] - pub(super) fn monotonize(raw: time::Instant) -> time::Instant { - monotonize_impl(&MONO, raw) - } - - #[inline] - pub(in crate::time) fn monotonize_impl(mono: &AtomicU64, raw: time::Instant) -> time::Instant { - let delta = raw.checked_sub_instant(&ZERO).unwrap(); - let secs = delta.as_secs(); - // occupies no more than 30 bits (10^9 seconds) - let nanos = delta.subsec_nanos() as u64; - - // This wraps around every 136 years (2^32 seconds). - // To detect backsliding we use wrapping arithmetic and declare forward steps smaller - // than 2^31 seconds as expected and everything else as a backslide which will be - // monotonized. - // This could be a problem for programs that call instants at intervals greater - // than 68 years. Interstellar probes may want to ensure that actually_monotonic() is true. - let packed = (secs << 32) | nanos; - let updated = mono.fetch_update(Relaxed, Relaxed, |old| { - (old == UNINITIALIZED || packed.wrapping_sub(old) < u64::MAX / 2).then_some(packed) - }); - match updated { - Ok(_) => raw, - Err(newer) => { - // Backslide occurred. We reconstruct monotonized time from the upper 32 bit of the - // passed in value and the 64bits loaded from the atomic - let seconds_lower = newer >> 32; - let mut seconds_upper = secs & 0xffff_ffff_0000_0000; - if secs & 0xffff_ffff > seconds_lower { - // Backslide caused the lower 32bit of the seconds part to wrap. - // This must be the case because the seconds part is larger even though - // we are in the backslide branch, i.e. the seconds count should be smaller or equal. - // - // We assume that backslides are smaller than 2^32 seconds - // which means we need to add 1 to the upper half to restore it. - // - // Example: - // most recent observed time: 0xA1_0000_0000_0000_0000u128 - // bits stored in AtomicU64: 0x0000_0000_0000_0000u64 - // backslide by 1s - // caller time is 0xA0_ffff_ffff_0000_0000u128 - // -> we can fix up the upper half time by adding 1 << 32 - seconds_upper = seconds_upper.wrapping_add(0x1_0000_0000); - } - let secs = seconds_upper | seconds_lower; - let nanos = newer as u32; - ZERO.checked_add_duration(&Duration::new(secs, nanos)).unwrap() - } - } - } -} - -#[cfg(all(target_has_atomic = "128", not(target_arch = "aarch64")))] -pub mod inner { - use crate::sync::atomic::AtomicU128; - use crate::sync::atomic::Ordering::*; - use crate::sys::time; - use crate::time::Duration; - - const ZERO: time::Instant = time::Instant::zero(); - static MONO: AtomicU128 = AtomicU128::new(0); - - #[inline] - pub(super) fn monotonize(raw: time::Instant) -> time::Instant { - let delta = raw.checked_sub_instant(&ZERO).unwrap(); - // Split into seconds and nanos since Duration doesn't have a - // constructor that takes a u128 - let secs = delta.as_secs() as u128; - let nanos = delta.subsec_nanos() as u128; - let timestamp: u128 = secs << 64 | nanos; - let timestamp = MONO.fetch_max(timestamp, Relaxed).max(timestamp); - let secs = (timestamp >> 64) as u64; - let nanos = timestamp as u32; - ZERO.checked_add_duration(&Duration::new(secs, nanos)).unwrap() - } -} - -#[cfg(not(any(target_has_atomic = "64", target_has_atomic = "128")))] -pub mod inner { - use crate::cmp; - use crate::sys::time; - use crate::sys_common::mutex::StaticMutex; - - #[inline] - pub(super) fn monotonize(os_now: time::Instant) -> time::Instant { - static LOCK: StaticMutex = StaticMutex::new(); - static mut LAST_NOW: time::Instant = time::Instant::zero(); - unsafe { - let _lock = LOCK.lock(); - let now = cmp::max(LAST_NOW, os_now); - LAST_NOW = now; - now - } - } -} diff --git a/library/std/src/time/tests.rs b/library/std/src/time/tests.rs index 7279925a6d0be..d1a69ff8697c6 100644 --- a/library/std/src/time/tests.rs +++ b/library/std/src/time/tests.rs @@ -90,10 +90,9 @@ fn instant_math_is_associative() { } #[test] -#[should_panic] -fn instant_duration_since_panic() { +fn instant_duration_since_saturates() { let a = Instant::now(); - let _ = (a - Duration::SECOND).duration_since(a); + assert_eq!((a - Duration::SECOND).duration_since(a), Duration::ZERO); } #[test] @@ -109,6 +108,7 @@ fn instant_checked_duration_since_nopanic() { #[test] fn instant_saturating_duration_since_nopanic() { let a = Instant::now(); + #[allow(deprecated, deprecated_in_future)] let ret = (a - Duration::SECOND).saturating_duration_since(a); assert_eq!(ret, Duration::ZERO); } @@ -192,31 +192,6 @@ fn since_epoch() { assert!(a < hundred_twenty_years); } -#[cfg(all(target_has_atomic = "64", not(target_has_atomic = "128")))] -#[test] -fn monotonizer_wrapping_backslide() { - use super::monotonic::inner::{monotonize_impl, ZERO}; - use core::sync::atomic::AtomicU64; - - let reference = AtomicU64::new(0); - - let time = match ZERO.checked_add_duration(&Duration::from_secs(0xffff_ffff)) { - Some(time) => time, - None => { - // platform cannot represent u32::MAX seconds so it won't have to deal with this kind - // of overflow either - return; - } - }; - - let monotonized = monotonize_impl(&reference, time); - let expected = ZERO.checked_add_duration(&Duration::from_secs(1 << 32)).unwrap(); - assert_eq!( - monotonized, expected, - "64bit monotonizer should handle overflows in the seconds part" - ); -} - macro_rules! bench_instant_threaded { ($bench_name:ident, $thread_count:expr) => { #[bench] From bda2693e9bcbf4246d1ada67886d9457054e3535 Mon Sep 17 00:00:00 2001 From: The 8472 Date: Fri, 7 Jan 2022 10:50:15 +0100 Subject: [PATCH 2/4] Add caveat about the monotonicity guarantee by linking to the later section --- library/std/src/time.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/library/std/src/time.rs b/library/std/src/time.rs index 0d6ace7536bf4..e8512f0cbaa14 100644 --- a/library/std/src/time.rs +++ b/library/std/src/time.rs @@ -49,8 +49,8 @@ pub use core::time::FromFloatSecsError; /// A measurement of a monotonically nondecreasing clock. /// Opaque and useful only with [`Duration`]. /// -/// Instants are always guaranteed to be no less than any previously measured -/// instant when created, and are often useful for tasks such as measuring +/// Instants are always guaranteed, barring [platform bugs], to be no less than any previously +/// measured instant when created, and are often useful for tasks such as measuring /// benchmarks or timing how long an operation takes. /// /// Note, however, that instants are **not** guaranteed to be **steady**. In other @@ -83,6 +83,8 @@ pub use core::time::FromFloatSecsError; /// } /// ``` /// +/// [platform bugs]: Instant#monotonicity +/// /// # OS-specific behaviors /// /// An `Instant` is a wrapper around system-specific types and it may behave From 376d955a32e781c988af3a1cd2f7056f5da28cb5 Mon Sep 17 00:00:00 2001 From: The 8472 Date: Fri, 7 Jan 2022 10:51:39 +0100 Subject: [PATCH 3/4] Add panic docs describing old, current and possible future behavior --- library/std/src/time.rs | 34 +++++++++++++++++++++++++++++++--- 1 file changed, 31 insertions(+), 3 deletions(-) diff --git a/library/std/src/time.rs b/library/std/src/time.rs index e8512f0cbaa14..4a65d612a6208 100644 --- a/library/std/src/time.rs +++ b/library/std/src/time.rs @@ -133,9 +133,12 @@ pub use core::time::FromFloatSecsError; /// if available, which is the case for all [tier 1] platforms. /// In practice such guarantees are – under rare circumstances – broken by hardware, virtualization /// or operating system bugs. To work around these bugs and platforms not offering monotonic clocks -/// [`duration_since`], [`elapsed`] and [`sub`] saturate to zero. [`checked_duration_since`] can -/// be used to detect and handle situations where monotonicity is violated, or `Instant`s are -/// subtracted in the wrong order. +/// [`duration_since`], [`elapsed`] and [`sub`] saturate to zero. In older rust versions this +/// lead to a panic instead. [`checked_duration_since`] can be used to detect and handle situations +/// where monotonicity is violated, or `Instant`s are subtracted in the wrong order. +/// +/// This workaround obscures programming errors where earlier and later instants are accidentally +/// swapped. For this reason future rust versions may reintroduce panics. /// /// [tier 1]: https://doc.rust-lang.org/rustc/platform-support.html /// [`duration_since`]: Instant::duration_since @@ -271,6 +274,13 @@ impl Instant { /// Returns the amount of time elapsed from another instant to this one, /// or zero duration if that instant is later than this one. /// + /// # Panics + /// + /// Previous rust versions panicked when `earlier` was later than `self`. Currently this + /// method saturates. Future versions may reintroduce the panic in some circumstances. + /// See [Monotonicity]. + /// + /// [Monotonicity]: Instant#monotonicity /// /// # Examples /// @@ -339,6 +349,14 @@ impl Instant { /// Returns the amount of time elapsed since this instant was created. /// + /// # Panics + /// + /// Previous rust versions panicked when self was earlier than the current time. Currently this + /// method returns a Duration of zero in that case. Future versions may reintroduce the panic. + /// See [Monotonicity]. + /// + /// [Monotonicity]: Instant#monotonicity + /// /// # Examples /// /// ```no_run @@ -413,6 +431,16 @@ impl SubAssign for Instant { impl Sub for Instant { type Output = Duration; + /// Returns the amount of time elapsed from another instant to this one, + /// or zero duration if that instant is later than this one. + /// + /// # Panics + /// + /// Previous rust versions panicked when `other` was later than `self`. Currently this + /// method saturates. Future versions may reintroduce the panic in some circumstances. + /// See [Monotonicity]. + /// + /// [Monotonicity]: Instant#monotonicity fn sub(self, other: Instant) -> Duration { self.duration_since(other) } From 37a1fc542fbf8c11356d4395f28d4ab7f936945d Mon Sep 17 00:00:00 2001 From: Josh Triplett Date: Wed, 9 Feb 2022 12:17:38 -0800 Subject: [PATCH 4/4] Capitalize "Rust" Co-authored-by: Mark Rousskov --- library/std/src/time.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/std/src/time.rs b/library/std/src/time.rs index 4a65d612a6208..df8a726e64ecb 100644 --- a/library/std/src/time.rs +++ b/library/std/src/time.rs @@ -133,7 +133,7 @@ pub use core::time::FromFloatSecsError; /// if available, which is the case for all [tier 1] platforms. /// In practice such guarantees are – under rare circumstances – broken by hardware, virtualization /// or operating system bugs. To work around these bugs and platforms not offering monotonic clocks -/// [`duration_since`], [`elapsed`] and [`sub`] saturate to zero. In older rust versions this +/// [`duration_since`], [`elapsed`] and [`sub`] saturate to zero. In older Rust versions this /// lead to a panic instead. [`checked_duration_since`] can be used to detect and handle situations /// where monotonicity is violated, or `Instant`s are subtracted in the wrong order. ///