Skip to content
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

Specialize sleep_until implementation #118480

Draft
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

dvdsk
Copy link
Contributor

@dvdsk dvdsk commented Nov 30, 2023

Draft (I need the CI tests, which will probably fail)
related tracking issue: #113752

Replaces the generic catch all implementation with target_os specific ones for: linux/netbsd/freebsd/android/solaris/illumos, wasi, macos/ios/tvos/watchos and windows.

Points of attention:

  • Adds extern "C" calls to mach_wait_until and mach_timebase_info for macos etc. We could also import them from the mach2 crate, but then we depend on mach2 and we need to modify mach2 to work as an std dep (adding a feature rustc-dep-of-std).
  • The implementations need access to the target_os specific details of the Instant type. For that I added an into_inner and made some fields public. Is there a neater way?
  • All untested, I only have access to an linux install. I hope the CI catches any issues.

@rustbot
Copy link
Collaborator

rustbot commented Nov 30, 2023

r? @cuviper

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added O-hermit Operating System: Hermit O-itron Operating System: ITRON O-SGX Target: SGX O-unix Operating system: Unix-like O-wasi Operating system: Wasi, Webassembly System Interface O-windows Operating system: Windows S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Nov 30, 2023
@dvdsk
Copy link
Contributor Author

dvdsk commented Nov 30, 2023

Not ready for review, however I need CI to run.

@dvdsk dvdsk marked this pull request as ready for review November 30, 2023 16:03
@cuviper
Copy link
Member

cuviper commented Nov 30, 2023

We don't have a configured way to try full CI, but you can copy specific jobs from auto to pr here:

Then ./x.py run src/tools/expand-yaml-anchors to update the real config, and push that as a temporary commit.

Copy link
Contributor

@joboet joboet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this on my local macOS machine. While the implementation works, the test currently fails because its success margin is too small.

library/std/tests/thread.rs Outdated Show resolved Hide resolved
@@ -943,11 +943,7 @@ pub fn sleep(dur: Duration) {
/// ```
#[unstable(feature = "thread_sleep_until", issue = "113752")]
pub fn sleep_until(deadline: Instant) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The section on platform-specific behaviour still needs updating.

@bors
Copy link
Contributor

bors commented Dec 7, 2023

☔ The latest upstream changes (presumably #116565) made this pull request unmergeable. Please resolve the merge conflicts.

@dvdsk dvdsk force-pushed the sleep_until_os_specific_impl branch from 558a3e4 to 7ace020 Compare December 24, 2023 14:10
@dvdsk dvdsk force-pushed the sleep_until_os_specific_impl branch from 7ace020 to 1e36e08 Compare December 24, 2023 14:44
@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Dec 24, 2023
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@dvdsk dvdsk force-pushed the sleep_until_os_specific_impl branch from 5470a7c to a0f50b6 Compare December 24, 2023 16:44
@dvdsk dvdsk marked this pull request as draft December 24, 2023 16:47
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Jan 13, 2024

☔ The latest upstream changes (presumably #117285) made this pull request unmergeable. Please resolve the merge conflicts.

@cuviper
Copy link
Member

cuviper commented Jan 25, 2024

I'm clearing this from my queue while it's in draft, but you can use ready when you are.

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 25, 2024
@Dylan-DPC
Copy link
Member

@dvdsk any updates on this pr? thanks

@dvdsk
Copy link
Contributor Author

dvdsk commented Feb 18, 2024

@dvdsk any updates on this pr? thanks

I had to put this on the back burner while moving. If I remember correctly only the windows test was not working. Got some hours today, lets see if I can change that. Then this should be ready to merge.

So, kinda stuck on getting the std tests working on a windows VM (something complaining about llvm...). Once I have some time im gonna ask someone to have a look with me.

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Feb 19, 2024

☔ The latest upstream changes (presumably #121185) made this pull request unmergeable. Please resolve the merge conflicts.

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Feb 24, 2024

☔ The latest upstream changes (presumably #121549) made this pull request unmergeable. Please resolve the merge conflicts.

The CI changes where ment for testing, that didnt work out. I do not
really get the library/stdarch and tools/cargo bit. Trying to reset that
to master with this commit
@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-llvm-16 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
GITHUB_ENV=/home/runner/work/_temp/_runner_file_commands/set_env_b540bac1-8b60-47eb-8fc2-e28f3e7d7285
GITHUB_EVENT_NAME=pull_request
GITHUB_EVENT_PATH=/home/runner/work/_temp/_github_workflow/event.json
GITHUB_GRAPHQL_URL=https://api.github.com/graphql
GITHUB_HEAD_REF=sleep_until_os_specific_impl
GITHUB_JOB=pr
GITHUB_PATH=/home/runner/work/_temp/_runner_file_commands/add_path_b540bac1-8b60-47eb-8fc2-e28f3e7d7285
GITHUB_REF=refs/pull/118480/merge
GITHUB_REF_NAME=118480/merge
GITHUB_REF_PROTECTED=false
---
#12 writing image sha256:ef441547c275d83673303c9b52f1dd2b3ba0d71a300c19095605d599afddc15a done
#12 naming to docker.io/library/rust-ci done
#12 DONE 10.0s
##[endgroup]
Setting extra environment values for docker:  --env ENABLE_GCC_CODEGEN=1 --env GCC_EXEC_PREFIX=/usr/lib/gcc/
[CI_JOB_NAME=x86_64-gnu-llvm-16]
##[group]Clock drift check
  local time: Sun Feb 25 20:35:24 UTC 2024
  network time: Sun, 25 Feb 2024 20:35:24 GMT
  network time: Sun, 25 Feb 2024 20:35:24 GMT
##[endgroup]
sccache: Starting the server...
##[group]Configure the build
configure: processing command line
configure: 
configure: build.configure-args := ['--build=x86_64-unknown-linux-gnu', '--llvm-root=/usr/lib/llvm-16', '--enable-llvm-link-shared', '--set', 'rust.thin-lto-import-instr-limit=10', '--set', 'change-id=99999999', '--enable-verbose-configure', '--enable-sccache', '--disable-manage-submodules', '--enable-locked-deps', '--enable-cargo-native-static', '--set', 'rust.codegen-units-std=1', '--set', 'dist.compression-profile=balanced', '--dist-compression-formats=xz', '--set', 'build.optimized-compiler-builtins', '--disable-dist-src', '--release-channel=nightly', '--enable-debug-assertions', '--enable-overflow-checks', '--enable-llvm-assertions', '--set', 'rust.verify-llvm-ir', '--set', 'rust.codegen-backends=llvm,cranelift,gcc', '--set', 'llvm.static-libstdcpp', '--enable-new-symbol-mangling']
configure: target.x86_64-unknown-linux-gnu.llvm-config := /usr/lib/llvm-16/bin/llvm-config
configure: llvm.link-shared     := True
configure: rust.thin-lto-import-instr-limit := 10
configure: change-id            := 99999999
---
   Compiling addr2line v0.21.0
error[E0433]: failed to resolve: could not find `unix` in `sys`
   --> library/std/src/sys/pal/unix/time.rs:322:24
    |
322 |     pub(in crate::sys::unix) fn into_timespec(self) -> Timespec {
    |                        ^^^^ could not find `unix` in `sys`
error[E0433]: failed to resolve: could not find `unix` in `sys`
  --> library/std/src/sys/pal/unix/time.rs:22:20
   |
   |
22 | pub(in crate::sys::unix) struct Nanoseconds(pub(in crate::sys::unix) u32);
   |                    ^^^^ could not find `unix` in `sys`
error[E0433]: failed to resolve: could not find `unix` in `sys`
  --> library/std/src/sys/pal/unix/time.rs:22:64
   |
   |
22 | pub(in crate::sys::unix) struct Nanoseconds(pub(in crate::sys::unix) u32);
   |                                                                ^^^^ could not find `unix` in `sys`
error[E0433]: failed to resolve: could not find `unix` in `sys`
  --> library/std/src/sys/pal/unix/time.rs:31:24
   |
   |
31 |     pub(in crate::sys::unix) tv_sec: i64,
   |                        ^^^^ could not find `unix` in `sys`
error[E0433]: failed to resolve: could not find `unix` in `sys`
  --> library/std/src/sys/pal/unix/time.rs:32:24
   |
   |
32 |     pub(in crate::sys::unix) tv_nsec: Nanoseconds,
   |                        ^^^^ could not find `unix` in `sys`
For more information about this error, try `rustc --explain E0433`.
error: could not compile `std` (lib) due to 5 previous errors
Build completed unsuccessfully in 0:00:14
  local time: Sun Feb 25 20:36:12 UTC 2024

@bors
Copy link
Contributor

bors commented Mar 2, 2024

☔ The latest upstream changes (presumably #121904) made this pull request unmergeable. Please resolve the merge conflicts.

}
}

#[cfg(any(target_os = "macos", target_os = "ios", target_os = "tvos", target_os = "watchos"))]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

visionOS was added in #121419.

Suggested change
#[cfg(any(target_os = "macos", target_os = "ios", target_os = "tvos", target_os = "watchos"))]
#[cfg(any(target_os = "macos", target_os = "ios", target_os = "tvos", target_os = "watchos", target_os = "visionos"))]

Same goes for the other cfgs.

let info = info.assume_init();
let ticks = nanos * (info.denom as u64) / (info.numer as u64);

mach_wait_until(ticks);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually check the return type of mach_wait_until.

Suggested change
mach_wait_until(ticks);
let ret = mach_wait_until(ticks);

Comment on lines +288 to +295
#[cfg(any(
target_os = "freebsd",
target_os = "netbsd",
target_os = "linux",
target_os = "android",
target_os = "solaris",
target_os = "illumos",
))]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add target_os = "dragonfly", target_os = "hurd" and target_os = "fuchsia"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc O-hermit Operating System: Hermit O-itron Operating System: ITRON O-SGX Target: SGX O-unix Operating system: Unix-like O-wasi Operating system: Wasi, Webassembly System Interface O-windows Operating system: Windows S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants