Special-case emscripten without atomics to take no-threads path

[juntyr]

Fixes #156350

Emscripten's pthread stub, which is used when compiling Emscripten without pthreads/atomics, has an unsound mutex implementation. As a quick fix, explicitly make Emscripten without atomics take the no-threads path.

@hoodmane Do we support Emscripten tests in CI here? How should we test this?

[rustbot]

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: @ChrisDenton, libs
• @ChrisDenton, libs expanded to 8 candidates
• Random selection from Mark-Simulacrum, nia-e

[Mark-Simulacrum]

As a quick fix, explicitly make Emscripten without atomics take the no-threads path

Does Emscripten without atomics mean Emscripten without threads? It's not obvious to me that is true.

[curiousdannii]

Or the reverse - atomics could be enabled but pthreads disabled.

If Emscripten pthreads needs RUSTFLAGS="-Clink-arg=-pthread" then is it even something that could be tested with cfg_select!?

Also for a Rust staticlib there might not be any link args?

[juntyr]

As a quick fix, explicitly make Emscripten without atomics take the no-threads path

Does Emscripten without atomics mean Emscripten without threads? It's not obvious to me that is true.

@sbc100 Could you answer this one?

[Mark-Simulacrum]

It's not obvious to me what the right behavior here is. I'm not very happy that we have no-threads targets as an implicit catch-all in std's implementation, that seems very dangerous since the implementation is unsound if someone forgets to note that a target does in fact have threads. It seems like that ought to condition on something like cfg(not(target_has_threads)) from the singlethread target option...

Based on rustc --print target-spec-json --target wasm32-unknown-emscripten -Zunstable-options, it looks like wasm32-unknown-emscripten does currently set singlethread to true, so I think from that perspective this is reasonable. But it also seems like we declare that target to have atomics:

$ rustc --print cfg --target wasm32-unknown-emscripten -Zunstable-options
target_has_atomic
target_has_atomic="16"
target_has_atomic="32"
target_has_atomic="64"
target_has_atomic="8"
...

So even without the target feature (which doesn't appear to be present in the list), it seems like gating on atomics is a pretty weird thing to do.

target_feature="bulk-memory"
target_feature="crt-static"
target_feature="exception-handling"
target_feature="multivalue"
target_feature="mutable-globals"
target_feature="nontrapping-fptoint"
target_feature="reference-types"
target_feature="sign-ext"

cc @hoodmane @juntyr (https://doc.rust-lang.org/nightly/rustc/platform-support/wasm32-unknown-emscripten.html#target-maintainers), do you have an opinion here? It seems like that's the only currently present emscripten target, in which case we could adjust the cfg here to just be "emscripten" perhaps?

[Mark-Simulacrum]

Just another poke of the target maintainers: @hoodmane @juntyr, can you take a look at this PR?

[juntyr]

@hoodmane you might be more knowledgeable here; I am unfortunately also currently very low on time

[hoodmane]

I agree it's not obvious that no threads implies no atomics but no atomics definitely implies no threads. So I think this is an improvement as is, but might not be the best possible improvement.

[Mark-Simulacrum]

@bors r+

OK, I'm going to go ahead and approve this for now. In parallel I've put up #158782 adding a compile-time assertion that no_threads code is only used on single-threaded (according to target spec) targets. It should pass on our current (single) emscripten target.

[rust-bors]

📌 Commit aeabcda has been approved by Mark-Simulacrum

It is now in the queue for this repository.

🌲 The tree is currently closed for pull requests below priority 1000. This pull request will be tested once the tree is reopened.

[jhpratt]

This is the only PR in the rollup that mentions CondVar in any way, so I presume this is the cause.

@bors r- #158790 (comment)

[rust-bors]

This pull request was unapproved.

This PR was contained in a rollup (#158790), which was unapproved.

View changes since this unapproval