Add and use cfg(target_has_threads) to enforce no_thread impl usage

[Mark-Simulacrum]

View all comments

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

[rustbot]

Some changes occurred in cfg and check-cfg configuration

cc @Urgau

[Urgau]

We want to prevent --cfg target_has_threads so we need to lint on the none value.

Suggested change

	| (sym::target_has_threads,Some(_))
	| (sym::target_has_threads,None | Some(_))

Can you also add a test for it in tests/ui/cfg/disallowed-cli-cfgs.rs.

View changes since the review

[Mark-Simulacrum]

Hm, interesting. I'll fix this but is it intentional that we allow users to pass e.g. --cfg 'windows="foo"'? That seems a bit confusing / probably unintentional, but from a quick test seems allowed?

IOW, why aren't all the options here restrictive? I guess it's a breaking change; do we normally FCP / search github / something for changes like this that stop allowing a cfg?

[Urgau]

is it intentional that we allow users to pass e.g. --cfg 'windows="foo"'?

Yes, it's intentional, when we introduced the lint I was conservative and only linted against the cfgs we set, to not break people that might do --cfg 'windows="foo"', however il-advised that might be.

But this conservative logic doesn't apply to newly added cfgs, as it's very unlikely that someone is doing some shenanigans on not yet added builtin cfgs.

We could try and lint not matter the value.

I guess it's a breaking change

It's a lint change, and lint changes are not considered breaking changes.

We also document that the explicit_builtin_cfgs_in_flags lint against the builtin cfgs (that includes new ones), and have added other cfgs in that list without doing an FCP.

[Urgau]

r? Urgau

[hanna-kruppe]

Getting the target spec wrong is already unsound (e.g., LLVM can make use of that) so it's a reasonable source of truth.

Unfortunately it seems the target spec field isn't the full truth of what we tell LLVM:

rust/compiler/rustc_codegen_llvm/src/back/write.rs

Lines 214 to 218 in c397dae

	// On the wasm target once the `atomics` feature is enabled that means that
	// we're no longer single-threaded, or otherwise we don't want LLVM to
	// lower atomic operations to single-threaded operations.
	if singlethread && sess.target.is_like_wasm && sess.target_features.contains(&sym::atomics){
	singlethread = false;

[rustbot]

These commits modify compiler targets.
(See the Target Tier Policy.)

[curiousdannii]

So with this, how would you enable threads in wasm32-emscripten?

[Mark-Simulacrum]

So with this, how would you enable threads in wasm32-emscripten?

This shouldn't change anything about behavior, if the target supports threads then it will work. See the previous comment (and fix to the PR since it) around wasm being special in having a target modifier changing the thread support.

@bors try jobs=dist-various-*

[curiousdannii]

This shouldn't change anything about behavior, if the target supports threads then it will work.

wasm32-emscripten's support of threads depends on which CFLAGS (or RUSTFLAGS, I forget which) you use.

[Mark-Simulacrum]

Right, that should be handled by the edit to consider whether the atomics target feature is set or not. This PR really isn't changing any behavior, it's just exposing what was already true, so it shouldn't have any meaningful effect. It may make it easier to see that code is broken (hence the try job as a quick check if there's targets with bad std impls).

[curiousdannii]

Yes, but wasm32-emscripten always says it has atomics, as you pointed out previously.

#156366 (comment)

This PR makes a lot of sense, I'm just not sure it will really improve things for our Emscripten target.

Should there really be a second Emscripten target for threads?

[Mark-Simulacrum]

Yes, but wasm32-emscripten always says it has atomics, as you pointed out previously.

It "has" atomics (i.e., the atomic types are defined), but unless -Ctarget-feature=+atomics is passed, they aren't real atomics. See https://rust.godbolt.org/z/3q9fsPx87 for example (edit to remove the command line argument to see that both functions compile the same without it). Note that we don't currently appear to enforce that said target feature is consistent across std and downstream libraries, which means that setting it is unsound (e.g., std will use non-atomic instructions and no-thread Mutex etc. to manipulate state that downstream code which enables the feature is expecting to be atomic). We'll need to make -Ctarget-feature=+atomics a target modifier but I think it makes sense to do that in a separate PR.

Regardless of that, this PR is an improvement for all targets because it means that we would know at compile-time of std if we get the cfg_select wrong and map a target to use the no-thread Mutex/etc. implementations despite the target actually having threads.

[rust-bors]

☀️ Try build successful (CI)
Build commit: e7f3db1 (e7f3db110a5277f652f7dc4ec3d959043493b0f7)
Base parent: c397dae (c397dae808f70caebab1fc4e11b3edf7e59f58c7)

[Urgau]

Make sense to me. Implementation looks good too.

View changes since this review

[Urgau]

@bors r+

[JonathanBrouwer]

💔 I suspect this PR failed tests as part of a rollup
@bors r-

After fixing the problem, consider running a try job for the failed job before re-approving.

Link to failure: #158815 (comment)

[rust-bors]

This pull request was unapproved.

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

View changes since this unapproval

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[Mark-Simulacrum]

No need for try builds, the failing "test" was a tidy file length check that didn't fail PR CI since that file grew on main after this opened. I split out some content from the relevant file (TargetTuple) in a second commit here which should bring us back under the limit (from ~3001 non-comment+non-blank lines to ~2889).

[Mark-Simulacrum]

@bors r=Urgau

(The code movement commit doesn't seem like it needs review, we can shuffle code again if there's any issues).

[rust-bors]

📌 Commit fe2db9c has been approved by Urgau

It is now in the queue for this repository.

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