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

Use sys::unix::locks::futex* on wasm+atomics. #96206

Merged
merged 2 commits into from
Apr 20, 2022

Conversation

m-ou-se
Copy link
Member

@m-ou-se m-ou-se commented Apr 19, 2022

This removes the wasm-specific lock implementations and instead re-uses the implementations from sys::unix.

Tracking issue: #93740

cc @alexcrichton

@m-ou-se m-ou-se added A-concurrency Area: Concurrency O-wasm Target: WASM (WebAssembly), http://webassembly.org/ T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Apr 19, 2022
@rust-highfive

This comment was marked as off-topic.

@rust-highfive
Copy link
Collaborator

r? @joshtriplett

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 19, 2022
@alexcrichton
Copy link
Member

Seems reasonable to me! One question I would have though is that I believe that the wasm standard library in atomics mode is not checked on CI, so are the futex locks expected to be used outside of unix platforms? I might otherwise worry about Unix-specific features creeping into the unix-specific futex files which accidentally break wasm and we'd go awhile without figuring that out. If, however, the futex locks are used on a variety of platforms on CI that would probably reduce the likelihood of platform-specific features creeping in.

@m-ou-se
Copy link
Member Author

m-ou-se commented Apr 19, 2022

I might otherwise worry about Unix-specific features creeping into the unix-specific futex files which accidentally break wasm and we'd go awhile without figuring that out.

There's no standard futex API across the different Unix platforms, so there's not really any unix-specific futex features we could start using.

Breaking things through reorganizing files and modules is an issue though, as proven by how the wasm+atomics build was actually broken before this PR. ^^'

@m-ou-se
Copy link
Member Author

m-ou-se commented Apr 19, 2022

We should probably start testing wasm+atomics in CI, as that's the only way to really solve that issue. I don't think this PR is making the situation worse. (If anything, it improves things by bringing wasm+atomics back into a buildable state.)

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Ok sounds like this at least isn't making the situation any worse, so seems reasonable to me!

@m-ou-se
Copy link
Member Author

m-ou-se commented Apr 19, 2022

@bors r=alexcrichton

@bors
Copy link
Contributor

bors commented Apr 19, 2022

📌 Commit 8f2913c has been approved by alexcrichton

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 19, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Apr 20, 2022
…chton

Use sys::unix::locks::futex* on wasm+atomics.

This removes the wasm-specific lock implementations and instead re-uses the implementations from sys::unix.

Tracking issue: rust-lang#93740

cc `@alexcrichton`
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 20, 2022
Rollup of 6 pull requests

Successful merges:

 - rust-lang#93313 (Check if call return type is visibly uninhabited when building MIR)
 - rust-lang#96160 (Miri/interpreter debugging tweaks)
 - rust-lang#96167 (Replace sys/unix/weak AtomicUsize with AtomicPtr)
 - rust-lang#96168 (Improve AddrParseError description)
 - rust-lang#96206 (Use sys::unix::locks::futex* on wasm+atomics.)
 - rust-lang#96234 (remove_dir_all_recursive: treat ELOOP the same as ENOTDIR)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 41235ef into rust-lang:master Apr 20, 2022
@rustbot rustbot added this to the 1.62.0 milestone Apr 20, 2022
@m-ou-se m-ou-se deleted the wasm-futex-locks branch April 20, 2022 21:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-concurrency Area: Concurrency O-wasm Target: WASM (WebAssembly), http://webassembly.org/ S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

6 participants