Fix non-associativity of Instant math on aarch64-apple-darwin targets

[maniwani]

This is a duplicate of #94100 (since the original author is unresponsive), which resolves #91417.

On aarch64-apple-darwin targets, the internal resolution of Instant is lower than that of Duration, so math between them becomes non-associative with small-enough durations.

This PR makes this target use the standard Unix implementation (where Instant has 1ns resolution), but with CLOCK_UPTIME_RAW so it still returns the same values as mach_absolute_time¹.

(Edit: I need someone to confirm that this still works, I do not have access to an M1 device.)

Footnotes

1. https://www.manpagez.com/man/3/clock_gettime/ ↩

[rustbot]

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• Stabilizing library features
• Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
• Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
• Changing public documentation in ways that create new stability guarantees
• Changing observable runtime behavior of library APIs

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (or someone else) soon.

Please see the contribution instructions for more information.

[thomcc]

I have a good amount of familiarity with these targets (and can test on a fairly wide range of them), so I'd like to take this. I'm a little hesitant about this change, but the current situation is admittedly fairly undesirable. I'll probably get to the review this weekend.

r? thomcc

[thomcc]

This doesn't compile on aarch64-apple-darwin.

error[E0308]: mismatched types
   --> library/std/src/sys/unix/time.rs:293:39
    |
293 |             const clock_id: clock_t = libc::CLOCK_UPTIME_RAW;
    |                                       ^^^^^^^^^^^^^^^^^^^^^^ expected `i32`, found `u32`

error[E0308]: mismatched types
   --> library/std/src/sys/unix/time.rs:323:43
    |
323 |             SystemTime { t: Timespec::now(libc::CLOCK_REALTIME) }
    |                             ------------- ^^^^^^^^^^^^^^^^^^^^ expected `i32`, found `u32`
    |                             |
    |                             arguments to this function are incorrect
    |
note: associated function defined here
   --> library/std/src/sys/unix/time.rs:333:16
    |
333 |         pub fn now(clock: clock_t) -> Timespec {
    |                ^^^ --------------
help: you can convert a `u32` to an `i32` and panic if the converted value doesn't fit
    |
323 |             SystemTime { t: Timespec::now(libc::CLOCK_REALTIME.try_into().unwrap()) }
    |                                                               ++++++++++++++++++++

error[E0308]: mismatched types
    --> library/std/src/sys/unix/time.rs:362:46
     |
362  |             cvt(unsafe { libc::clock_gettime(clock, t.as_mut_ptr()) }).unwrap();
     |                          ------------------- ^^^^^ expected `u32`, found `i32`
     |                          |
     |                          arguments to this function are incorrect
     |
note: function defined here
    --> /Users/thom/.cargo/registry/src/github.com-1ecc6299db9ec823/libc-0.2.135/src/unix/bsd/apple/mod.rs:5092:12
     |
5092 |     pub fn clock_gettime(clk_id: ::clockid_t, tp: *mut ::timespec) -> ::c_int;
     |            ^^^^^^^^^^^^^
help: you can convert an `i32` to a `u32` and panic if the converted value doesn't fit
     |
362  |             cvt(unsafe { libc::clock_gettime(clock.try_into().unwrap(), t.as_mut_ptr()) }).unwrap();
     |                                                   ++++++++++++++++++++

@rustbot author

[maniwani]

I think the problem is the type alias used here.

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

Lines 315 to 318 in e4d6307

	#[cfg(not(any(target_os = "dragonfly", target_os = "espidf", target_os = "horizon")))]
	pub type clock_t = libc::c_int;
	#[cfg(any(target_os = "dragonfly", target_os = "espidf", target_os = "horizon"))]
	pub type clock_t = libc::c_ulong;

Is there any reason to even have these? libc seems to have a clockid_t alias for every platform already.

[thomcc]

If it's in libc on every platform there's no reason to have it in std::sys

[thomcc]

Hmm, it seems that miri doesn't emulate clock_gettime on macOS. @RalfJung, any opinions as to how you'd like us to handle this?

[thomcc]

This looks good to me. Seems to work on M1 too. libc::CLOCK_UPTIME_RAW is the same as mach_absolute_time effectively, which we already use, so I don't see a need to deliberate over this like we have on some other platforms. Thanks.

@bors r+

(Also, I like your username and avatar)

[bors]

📌 Commit 015ab65 has been approved by thomcc

It is now in the queue for this repository.

[thomcc]

Ah, wait, this is failing. I forgot that we need to resolve the situation with miri.

@bors r-

[thomcc]

Probably the best solution is to continue using the x86_64 code on aarch64 when under miri). Pretty soon we should be able to globally improve the handling of this (assuming the min macOS version bump goes through).

[RalfJung]

Yeah if further changes are planned soon that might be easiest. Is the plan to use clock_gettime on all macos targets? We already have clock_gettime for Linux so if the macos version is similar it should be easy to add. Am 14. November 2022 06:50:40 UTC schrieb Thom Chiovoloni ***@***.***>:

…

Probably the best solution is to continue using the x86_64 code on aarch64 when under miri). Pretty soon we should be able to globally improve the handling of this (assuming the min macOS version bump goes through). -- Reply to this email directly or view it on GitHub: #103594 (comment) You are receiving this because you were mentioned. Message ID: ***@***.***>

[RalfJung]

I guess that would be just adding a not(miri) here. Please also open an issue at https://github.com/rust-lang/miri/issues to track removing this Miri hack.

[maniwani]

Will do.

[thomcc]

Is the plan to use clock_gettime on all macos targets?

Plausibly. I'd probably prefer using clock_gettime_nsec_np given that what we actually want is nanoseconds, and apple's docs recommend that you prefer it if possible. But clock_gettime is an easier migration path.

[RalfJung]

The 'np' makes me thing that will require a dedicated implementation. ;) But it's fine, these clock APIs are usually not a lot of work. (Unlike synchronization APIs...)

[thomcc]

The 'np' makes me thing that will require a dedicated implementation

Yes, most likely. But yeah, I don't expect it to be very complex.

[thomcc]

LGTM

@bors r+

[bors]

📌 Commit f4f5159 has been approved by thomcc

It is now in the queue for this repository.