Special-case emscripten without atomics to take no-threads path#156366
Special-case emscripten without atomics to take no-threads path#156366juntyr wants to merge 1 commit into
Conversation
rustbot
added
S-waiting-on-review
|
rustbot has assigned @Mark-Simulacrum. Use Why was this reviewer chosen?The reviewer was selected based on:
|
juntyr
force-pushed
the
emscripten-no-threads-sync
branch
from
3eabd84 to
aeabcda
Compare
Does Emscripten without atomics mean Emscripten without threads? It's not obvious to me that is true. |
|
Or the reverse - atomics could be enabled but pthreads disabled. If Emscripten pthreads needs Also for a Rust staticlib there might not be any link args? |
@sbc100 Could you answer this one? |
|
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 Based on 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. 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? |
|
@hoodmane you might be more knowledgeable here; I am unfortunately also currently very low on time |
|
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. |
rust-bors
Bot
added
S-waiting-on-bors
… r=Mark-Simulacrum Special-case emscripten without atomics to take no-threads path Fixes rust-lang#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?
|
This is the only PR in the rollup that mentions |
rust-bors
Bot
added
S-waiting-on-author
|
This pull request was unapproved. This PR was contained in a rollup (#158790), which was unapproved. |
… r=Urgau Add and use cfg(target_has_threads) to enforce no_thread impl usage The standard library has fallback code for targets without threads (e.g., using a Cell-based Mutex and similar). Today there's no enforcement in std that those targets truly don't have threads which makes that code potentially unsound. This will let us add a static assertion that the target spec agrees that the target is non-threaded. Getting the target spec wrong is already unsound (e.g., LLVM can make use of that) so it's a reasonable source of truth. This pulls in the atomics target feature into the target code and makes the field itself private to encourage going via the method. For now the cfg is added as unstable but if we have a use case for user code to use this, happy to cut a tracking issue and make it a regular unstable feature. Setting a compiler reviewer since the std changes are pretty trivial, and I think anyone can review those reasonably. cc rust-lang#156366 which prompted looking into this r? compiler
… r=Urgau Add and use cfg(target_has_threads) to enforce no_thread impl usage The standard library has fallback code for targets without threads (e.g., using a Cell-based Mutex and similar). Today there's no enforcement in std that those targets truly don't have threads which makes that code potentially unsound. This will let us add a static assertion that the target spec agrees that the target is non-threaded. Getting the target spec wrong is already unsound (e.g., LLVM can make use of that) so it's a reasonable source of truth. This pulls in the atomics target feature into the target code and makes the field itself private to encourage going via the method. For now the cfg is added as unstable but if we have a use case for user code to use this, happy to cut a tracking issue and make it a regular unstable feature. Setting a compiler reviewer since the std changes are pretty trivial, and I think anyone can review those reasonably. cc rust-lang#156366 which prompted looking into this r? compiler
… r=Urgau Add and use cfg(target_has_threads) to enforce no_thread impl usage The standard library has fallback code for targets without threads (e.g., using a Cell-based Mutex and similar). Today there's no enforcement in std that those targets truly don't have threads which makes that code potentially unsound. This will let us add a static assertion that the target spec agrees that the target is non-threaded. Getting the target spec wrong is already unsound (e.g., LLVM can make use of that) so it's a reasonable source of truth. This pulls in the atomics target feature into the target code and makes the field itself private to encourage going via the method. For now the cfg is added as unstable but if we have a use case for user code to use this, happy to cut a tracking issue and make it a regular unstable feature. Setting a compiler reviewer since the std changes are pretty trivial, and I think anyone can review those reasonably. cc rust-lang#156366 which prompted looking into this r? compiler
Rollup merge of #158782 - Mark-Simulacrum:expose-no-threads, r=Urgau Add and use cfg(target_has_threads) to enforce no_thread impl usage The standard library has fallback code for targets without threads (e.g., using a Cell-based Mutex and similar). Today there's no enforcement in std that those targets truly don't have threads which makes that code potentially unsound. This will let us add a static assertion that the target spec agrees that the target is non-threaded. Getting the target spec wrong is already unsound (e.g., LLVM can make use of that) so it's a reasonable source of truth. This pulls in the atomics target feature into the target code and makes the field itself private to encourage going via the method. For now the cfg is added as unstable but if we have a use case for user code to use this, happy to cut a tracking issue and make it a regular unstable feature. Setting a compiler reviewer since the std changes are pretty trivial, and I think anyone can review those reasonably. cc #156366 which prompted looking into this r? compiler
6 participants
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?