Use sys::unix::locks::futex* on wasm+atomics. by m-ou-se · Pull Request #96206 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation8 Commits2 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

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

Tracking issue: #93740

cc @alexcrichton

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

This comment was marked as off-topic.

r? @joshtriplett

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

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.

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. ^^'

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.)

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!

📌 Commit 8f2913c has been approved by alexcrichton

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

Labels