-
Notifications
You must be signed in to change notification settings - Fork 13.5k
Specialize sleep_until implementation for unix (except mac) #141829
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
r? @cuviper |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This can be done without touching all pal's, ill be doing that tomorrow, now its bed time 😴 edit: I was mistaken, tidy does not allow #[cfg(...)] in |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
- In contradiction to nanosleep clock_nanosleep returns the error directly and does not require a call to `os::errno()`. - The last argument to clock_nanosleep can be NULL removing the need for mutating the timespec. - Missed an `allow(unused)` that could be made conditional.
This is intended to support the std's new sleep_until. Only the clocks REALTIME and MONOTONIC are supported. The first because it was trivial to add the second as its needed for sleep_until. Only passing no flags or passing only TIMER_ABSTIME is supported. If an unsupported flags or clocks are passed this implementation panics.
This comment has been minimized.
This comment has been minimized.
rebased to:
edit: I just realized rebasing all the time makes this rather hard to review. I'll rebase/squash at the end, for now I'll focus on short descriptive commits. |
src/tools/miri/src/shims/time.rs
Outdated
@@ -396,14 +393,14 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { | |||
// No flags set, the timespec should be interperted as a duration | |||
// to sleep for | |||
TimeoutAnchor::Relative | |||
} else if flag == this.eval_libc("TIMER_ABSTIME").to_int(int_size) { | |||
} else if flags == this.eval_libc("TIMER_ABSTIME").to_i32()? { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} else if flags == this.eval_libc("TIMER_ABSTIME").to_i32()? { | |
} else if flags == this.eval_libc_i32("TIMER_ABSTIME") { |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
test_nanosleep(); | ||
test_clock_nanosleep_absolute(); | ||
test_clock_nanosleep_relative(); | ||
test_localtime_r_epoch(); | ||
test_localtime_r_gmt(); | ||
test_localtime_r_pst(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why you reordered an existing test here?^^
Since the other tests are all about getting the current time, seems best to have the sleep tests all together in a separate "section", e.g. at the end of main
, separated from the existing tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why you reordered an existing test here?^^
I really need to start paying a little more attention to the diffs I make
Since the other tests are all about getting the current time, seems best to have the sleep tests all together in a separate "section", e.g. at the end of main, separated from the existing tests.
Good idea
let mut ts = timespec_now(libc::CLOCK_MONOTONIC); | ||
ts.tv_nsec += 1_000_000_000 / 10; | ||
ts | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs logic to deal with nsec
growing to more than 1s
unsafe { timespec.assume_init() } | ||
} | ||
|
||
fn test_clock_nanosleep_absolute() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These functions also need the cfg
to not be built on macOS.
You can test this locally even if you are not on macOS:
./x test miri --target aarch64-apple-darwin -- time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can test this locally even if you are not on macOS:
that's super useful thanks!
@rustbot author |
Reminder, once the PR becomes ready for a review, use |
@rustbot ready Apologies for the unnecessary rebase. Changes:
|
ts.tv_nsec = ts.tv_nsec % SECOND; | ||
ts.tv_sec = ts.tv_nsec / SECOND; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't do the right thing. You need to swap these two lines. With your version of the code, tv_sec
will always end up being 0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, also this should be tv_sec += ...
!
} | ||
|
||
/// Helper function to get the current time for testing relative sleeps | ||
fn timespec_now(clock: libc::clockid_t) -> libc::timespec { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please put the 2 helper functions above the functions that use them.
related tracking issue: #113752
Supersedes #118480 for the reasons see: #113752 (comment)
Replaces the generic catch all implementation with target_os specific ones for: linux/netbsd/freebsd/android/solaris/illumos etc. Other platforms like wasi, macos/ios/tvos/watchos and windows will follow in later separate PR's (once this is merged).