Instant + Duration produce wrong result on aarch64-apple-darwin

[wangrunji0408]

I tried this code on M1 macOS:

use std::time::{Duration, Instant};
fn main() {
    let t0 = Instant::now();
    let t1 = t0 + Duration::from_nanos(50);
    let d = t1 - t0;
    dbg!(t0, t1, d);
    assert_eq!(t0 + d, t1);
}

It's expected to succeed.

Instead, this happened:

[src/main.rs:6] t0 = Instant {
    t: 147092181000,
}
[src/main.rs:6] t1 = Instant {
    t: 147092181001,
}
[src/main.rs:6] d = 41ns
thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `Instant { t: 147092181000 }`,
 right: `Instant { t: 147092181001 }`', src/main.rs:7:5

It seems that there is something wrong with the Duration -> Instant casting in Rust std:

rust/library/std/src/sys/unix/time.rs

Lines 227 to 232 in 1d6f242

	fn checked_dur2intervals(dur: &Duration) -> Option<u64> {
	let nanos =
	dur.as_secs().checked_mul(NSEC_PER_SEC)?.checked_add(dur.subsec_nanos() as u64)?;
	let info = info();
	Some(mul_div_u64(nanos, info.denom as u64, info.numer as u64))
	}

This problem does not exist on x86_64-apple-darwin, because info.denom = info.numer = 1.

Meta

rustc --version --verbose:

rustc 1.56.1 (59eed8a2a 2021-11-01)
binary: rustc
commit-hash: 59eed8a2aac0230a8b53e89d4e99d55912ba6b35
commit-date: 2021-11-01
host: aarch64-apple-darwin
release: 1.56.1
LLVM version: 13.0.0

[hkratz]

@rustbot label +T-libs +O-macos +O-arm

[csmoe]

Seems related to the time resolution difference on Apple M1, cc https://eclecticlight.co/2020/09/08/changing-the-clock-in-apple-silicon-macs/

[hkratz]

The reason for this behavior is that Instant uses platform-specific resolution while Duration uses nanosecond resolution. For Macos x86-64 and Linux this does not matter because on those platforms the resolution of Instant is also nanoseconds.

Instant resolution on my M1 is 125 / 3 ≈ 41.67 ns. So when you add 50 ns to t0 you are adding 1 to its internal representation (= ~41.67 ns). When the duration is calculated it gets rounded down to d = ⌊1 * 125 / 3⌋ = 41 ns. t0 + d = t0 because 41 ns is less than the minimum resolution.

[wangrunji0408]

The reason for this behavior is that Instant uses platform-specific resolution while Duration uses nanosecond resolution. For Macos x86-64 and Linux this does not matter because on those platforms the resolution of Instant is also nanoseconds.

Instant resolution on my M1 is 125 / 3 ≈ 41.67 ns. So when you add 50 ns to t0 you are adding 1 to its internal representation (= ~41.67 ns). When the duration is calculated it gets rounded down to d = ⌊1 * 125 / 3⌋ = 41 ns. t0 + d = t0 because 41 ns is less than the minimum resolution.

So is it considered a bug? Should we round 41 ns up to 1?

[hkratz]

Thought about this some more.

We allow those operations:

• Duration = Instant - Instant
• Instant = Instant + Duration
• bool = Instant == Instant

That can't be changed, so simple arithmetic identities such as i0 + (i1 - i0) == i1 should hold if there is no overflow. In particular because the current behavior leads to real bugs.

The only way to achieve this is IMHO to change the internal representation for Instant on MacOS to nanosecond resolution because otherwise the resolution of the machine timer influences Instant/Duration arithmetic.

cc @thomcc What do you think?

[thomcc]

We should do this, but I'm not sure we can do it everywhere — clock_gettime_nsec_np (and the other clock_gettime functions) is only available in Darwin 16 (macOS 10.12, iOS 10) and newer, which is newer than our supported versions.

We can certainly do it on macOS+aarch64 targets though, since those should always have it.

[BGR360]

This seems pretty severe. I'm a novice contributor, but I do have an M1 device if there's anything I can do to aid the development of a fix here.

Also, should this get some sort of I-prioritize doohickey?

[the8472]

The only way to achieve this is IMHO to change the internal representation for Instant on MacOS to nanosecond resolution because otherwise the resolution of the machine timer influences Instant/Duration arithmetic.

But the documentation says:

An Instant is a wrapper around system-specific types and it may behave differently depending on the underlying operating system. For example, the following snippet is fine on Linux but panics on macOS:

So the consequence would seem that we can any range and precision limitations from the underlying type and thus rounding errors are part of the API? I don't see the benefit of it, but it's documented.

[hkratz]

It is documented for Instant, yes. But we do allow arithmetic operations (+/-) between Duration and Instant leading to those problems, that - at minimum - violate the principle of the least surprise. It is exacerbated by the fact that Linux, Windows and macOS x86-64 (de facto because numer and denom are both 1) Instants all use nanosecond resolution.

We should just use clock_gettime_nsec_np(CLOCK_UPTIME_RAW)¹ on macOS aarch64 as @thomcc suggested, in particular because the use of mach_absolute_time() is already discouraged. We don't specify which version of iOS we support, so no idea if we could use it for ios as well.

Footnotes

1. Which is currently implemented as (mach_absolute_time() * tb_info.numer) / tb_info.denom (source) apparently totally unconcerned about overflow, so I assume that numer will stay small. ↩

[lyinch]

It tried to use clock_gettime_nsec_np(CLOCK_UPTIME_RAW) but this didn't fix the issue. The changes:

https://github.com/lyinch/rust/blob/issue-91417-fix/library/std/src/sys/unix/time.rs#L154
https://github.com/lyinch/rust/blob/issue-91417-fix/library/std/src/time/tests.rs#L17

I also added a test which still fails:

failures:

---- time::tests::macos_resolution_regression stdout ----
[library/std/src/time/tests.rs:21] t0 = Instant {
    t: 511866740696750,
}
[library/std/src/time/tests.rs:21] t1 = Instant {
    t: 511866740696751,
}
[library/std/src/time/tests.rs:21] d = 41ns
thread 'time::tests::macos_resolution_regression' panicked at 'assertion failed: `(left == right)`
  left: `Instant { t: 511866740696750 }`,
 right: `Instant { t: 511866740696751 }`', library/std/src/time/tests.rs:22:5


failures:
    time::tests::macos_resolution_regression

I'm not sure how to correctly get CLOCK_UPTIME_RAW from Rust, so I looked the value up in my local time.h file and used 8 :

typedef enum {
_CLOCK_REALTIME __CLOCK_AVAILABILITY = 0,
#define CLOCK_REALTIME _CLOCK_REALTIME
_CLOCK_MONOTONIC __CLOCK_AVAILABILITY = 6,
#define CLOCK_MONOTONIC _CLOCK_MONOTONIC
#if !defined(_POSIX_C_SOURCE) || defined(_DARWIN_C_SOURCE)
_CLOCK_MONOTONIC_RAW __CLOCK_AVAILABILITY = 4,
#define CLOCK_MONOTONIC_RAW _CLOCK_MONOTONIC_RAW
_CLOCK_MONOTONIC_RAW_APPROX __CLOCK_AVAILABILITY = 5,
#define CLOCK_MONOTONIC_RAW_APPROX _CLOCK_MONOTONIC_RAW_APPROX
_CLOCK_UPTIME_RAW __CLOCK_AVAILABILITY = 8,
#define CLOCK_UPTIME_RAW _CLOCK_UPTIME_RAW
_CLOCK_UPTIME_RAW_APPROX __CLOCK_AVAILABILITY = 9,
#define CLOCK_UPTIME_RAW_APPROX _CLOCK_UPTIME_RAW_APPROX
#endif
_CLOCK_PROCESS_CPUTIME_ID __CLOCK_AVAILABILITY = 12,
#define CLOCK_PROCESS_CPUTIME_ID _CLOCK_PROCESS_CPUTIME_ID
_CLOCK_THREAD_CPUTIME_ID __CLOCK_AVAILABILITY = 16
#define CLOCK_THREAD_CPUTIME_ID _CLOCK_THREAD_CPUTIME_ID
} clockid_t;

Note that this enum uses

#if !defined(_DARWIN_FEATURE_CLOCK_GETTIME) || _DARWIN_FEATURE_CLOCK_GETTIME != 0
#if __DARWIN_C_LEVEL >= 199309L
#if __has_feature(enumerator_attributes)
#define __CLOCK_AVAILABILITY __OSX_AVAILABLE(10.12) __IOS_AVAILABLE(10.0) __TVOS_AVAILABLE(10.0) __WATCHOS_AVAILABLE(3.0)
#else
#define __CLOCK_AVAILABILITY
#endif

and possibly: https://gcc.gnu.org/onlinedocs/gcc/Enumerator-Attributes.html

[hkratz]

@lyinch That can't work, because you are still adjusting the value returned by clock_gettime_nsec_np(CLOCK_UPTIME_RAW) with mach_timebase_info numerator and denominator in https://github.com/lyinch/rust/blob/issue-91417-fix/library/std/src/sys/unix/time.rs#L159-L172

Also this can only be done for macOS aarch64 where it is always available.

It just occurred to me that we don't even need to use clock_gettime_nsec_np, we can just use the standard UNIX implementation (https://github.com/lyinch/rust/blob/aebc9040ebf41d35b646e4ea9aedb86df09cacdb/library/std/src/sys/unix/time.rs#L268) for macOS aarch64 except that we need to use CLOCK_UPTIME_RAW. Defining a local const for CLOCK_UPTIME_RAW to 8 is fine is fine for testing, but it should become a PR to https://github.com/rust-lang/libc before merging.

[lyinch]

The test passes now with your proposed changes: lyinch@f9f0c97