-
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.
Miri does not understand |
The Miri subtree was changed cc @rust-lang/miri |
First time adding anything to MIRI.
It was fun to see some MIRI code but I understand if the additional code to MIRI for supporting Let me also thank you all again for your reviews and mentoring me on how to get this PR up to spec. |
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.
Thanks for working on the Miri shims! However, they don't quite match what we'd usually expect. If you need more Miri-specific help, please ask on Zulip.
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 also add a test in tests/pass-dep/libc/libc-time.rs
for the new shim. In particular, it seems there are two codepaths (relative and absolute?), they should both be tested.
Also, please add a call to sleep_until
in tests/pass/shims/time.rs
.
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.
Should be done.
- I'm a little unsure about the
libc-time.rs
test as I've added ause std::time::{Duration, Instant};
while the test hardly had any imports before. - I've added a test for
nanosleep
since there was none there. - All tests where locally tested by sabotaging them.
This comment has been minimized.
This comment has been minimized.
(The build fails on Windows because you're using |
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Jonas Böttiger <jonasboettiger@icloud.com> - use the internal platform specific Instant::CLOCK_ID - skip allow(unused) on on platform that uses it such that it can not become dead code - sleep_until: remove leftover links
- 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.
The job Click to see the possible cause of the failure (guessed by this bot)
|
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).