Skip to content

Fix the wasm32-unknown-unknown target feature/cfg bug - #161718

Merged
rust-bors[bot] merged 4 commits into
rust-lang:mainfrom
nnethercote:parse_cfg-stuff
Aug 26, 2026
Merged

Fix the wasm32-unknown-unknown target feature/cfg bug#161718
rust-bors[bot] merged 4 commits into
rust-lang:mainfrom
nnethercote:parse_cfg-stuff

Conversation

@nnethercote

Copy link
Copy Markdown
Contributor

Due to some bad ordering of session/config initialization code, cfg(target_has_threads) fails to be set for the wasm32-unknown-unknown platform when -Ctarget-feature=+atomics is specified. This PR fixes the problem. Details in individual commits.

r? @Mark-Simulacrum

Due to some bad ordering of session/config initialization code -- more
about that in subsequent commits -- `cfg(target_has_threads)` fails to
be set for the `wasm32-unknown-unknown` platform when
`-Ctarget-feature=+atomics` is specified. This commit modifies a test to
demonstrate the bug; as written the test passes.
- `parse_check_cfg` has a single call site and is followed by
  a call to `fill_well_known`.

- `parse_cfg` has two call sites and in both cases is followed by
  a call to `build_configuration`.

This commit moves the follow-up calls into the functions, simplifying
`run_compiler`.
Currently it modifies the `Session` and the `Cfg` (and the `Cfg`
afterwards is put into the `Session`). It also takes a `CodegenBackend`.
Those are some heavyweight arguments.

This commit moves the `Session` modifications to the caller so the `&mut
Session` isn't necessary, and passes in the `TargetConfig` instead of
the whole `CodegenBackend`, plus some other small arguments.
`add_configuration` ends up more clearly about modifying the `Cfg`. This
is a step towards untangling session/backend initialization.
@rustbot rustbot added A-run-make Area: port run-make Makefiles to rmake.rs S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 25, 2026
@rustbot

This comment has been minimized.

@nnethercote

Copy link
Copy Markdown
Contributor Author

LLM disclosure: an LLM identified this bug and made some suggestions on how to fix it and test the fix. I wrote all the code and text myself.

Comment thread compiler/rustc_interface/src/interface.rs
@RalfJung

Copy link
Copy Markdown
Member

Ah nice, with my recent refactors here we can now funnel everything through sess.internal_target_features. Good catch!

Currently, `parse_cfg` calls `build_configuration`, which calls
`default_configuration`, which calls
`sess.target.singlethread(&sess.internal_target_features)`. But
`sess.internal_target_features` hasn't been set at this point and is
empty!

This commit moves the setting of `sess.internal_target_features` before
the `parse_cfg` call to fix this ordering bug. This results in the
`cfg(target_has_threads)` being correctly set on
`wasm32-unknown-unknown` when `-Ctarget-feature=+atomics` is specified.

Note: I have plans to make this kind of ordering bug
difficult/impossible in a follow-up (e.g. rust-lang#161432).
@rustbot

rustbot commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator

⚠️ Warning ⚠️

  • There are issue links (such as #123) in the commit messages of the following commits.
    Please move them to the PR description, to avoid spamming the issues with references to the commit, and so this bot can automatically canonicalize them to avoid issues with subtree.

@Urgau Urgau left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I was nerd snipped by the changes to check-cfg.

Looks good to me. r=me with/without the nits or Mark's review

View changes since this review


/// Converts strings provided as `--check-cfg [specs]` into a `CheckCfg`.
pub(crate) fn parse_check_cfg(dcx: DiagCtxtHandle<'_>, specs: Vec<String>) -> CheckCfg {
pub(crate) fn parse_check_cfg(sess: &Session, specs: Vec<String>) -> CheckCfg {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A bit surprising that we take a Session to do some parsing, maybe the functions should be renamed to parse_and_build_...?

@nnethercote nnethercote Aug 25, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The only things used from the Session are .dcx() and .target. I thought about just passing those in, but #161432 will introduce an EarlySession type that just contains dcx, target, host, and opts and that will probably be what gets passed to parse_check_cfg.

@nnethercote

Copy link
Copy Markdown
Contributor Author

@bors r=Urgau

@rust-bors

rust-bors Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

📌 Commit 62578df has been approved by Urgau

It is now in the queue for this repository.

@rust-bors rust-bors Bot 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 Aug 25, 2026
jhpratt added a commit to jhpratt/rust that referenced this pull request Aug 26, 2026
Fix the wasm32-unknown-unknown target feature/cfg bug

Due to some bad ordering of session/config initialization code, `cfg(target_has_threads)` fails to be set for the `wasm32-unknown-unknown` platform when `-Ctarget-feature=+atomics` is specified. This PR fixes the problem. Details in individual commits.

r? @Mark-Simulacrum
rust-bors Bot pushed a commit that referenced this pull request Aug 26, 2026
…uwer

Rollup of 7 pull requests

Successful merges:

 - #157036 (lint against repeated repr attributes)
 - #160183 (panic_unwind: Use global_asm! for IMGREL relocations)
 - #161718 (Fix the wasm32-unknown-unknown target feature/cfg bug)
 - #161673 (Fix broken link to lang_items.rs in unstable book)
 - #161744 (Remove `RawDefPathHash`)
 - #161747 (explicitly state that allocations cannot grow to the left)
 - #161796 (Remove dead parse error recovery (underscores in expressions))
@rust-bors
rust-bors Bot merged commit abfd8e5 into rust-lang:main Aug 26, 2026
13 checks passed
@rustbot rustbot added this to the 1.100.0 milestone Aug 26, 2026
rust-bors Bot pushed a commit that referenced this pull request Aug 26, 2026
Rollup merge of #161718 - nnethercote:parse_cfg-stuff, r=Urgau

Fix the wasm32-unknown-unknown target feature/cfg bug

Due to some bad ordering of session/config initialization code, `cfg(target_has_threads)` fails to be set for the `wasm32-unknown-unknown` platform when `-Ctarget-feature=+atomics` is specified. This PR fixes the problem. Details in individual commits.

r? @Mark-Simulacrum
@rust-timer

Copy link
Copy Markdown
Collaborator

Note

This PR was benchmarked as part of triage of its containing rollup: triage URL.

Finished benchmarking commit (8a6ec0c): comparison URL.

Overall result: ❌ regressions - please read:

Our benchmarks found a performance regression caused by this PR.
This might be an actual regression, but it can also be just noise.

Next Steps:

  • If the regression was expected or you think it can be justified,
    please write a comment with sufficient written justification, and add
    @rustbot label: +perf-regression-triaged to it, to mark the regression as triaged.
  • If you think that you know of a way to resolve the regression, try to create
    a new PR with a fix for the regression.
  • If you do not understand the regression or you think that it is just noise,
    you can ask the @rust-lang/wg-compiler-performance working group for help (members of this group
    were already notified of this PR).

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

mean range count
Regressions ❌
(primary)
0.2% [0.2%, 0.2%] 3
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.2% [0.2%, 0.2%] 3

Max RSS (memory usage)

Results (primary 0.5%, secondary -1.9%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
0.5% [0.5%, 0.5%] 1
Regressions ❌
(secondary)
3.0% [1.8%, 3.6%] 4
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-4.7% [-7.8%, -3.3%] 7
All ❌✅ (primary) 0.5% [0.5%, 0.5%] 1

Cycles

Results (secondary -0.9%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
2.0% [2.0%, 2.0%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.4% [-2.7%, -2.1%] 2
All ❌✅ (primary) - - 0

Binary size

Results (secondary -0.0%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-0.0% [-0.0%, -0.0%] 1
All ❌✅ (primary) - - 0

Bootstrap: 475.812s -> 479.967s (0.87%)
Artifact size: 403.37 MiB -> 402.68 MiB (-0.17%)

@rustbot rustbot added the perf-regression Performance regression. label Aug 26, 2026
@panstromek

Copy link
Copy Markdown
Contributor

Looks like this caused the regression in #161801

This seems quite unexpected, any idea where this is coming from?

@nnethercote
nnethercote deleted the parse_cfg-stuff branch August 26, 2026 20:06
@nnethercote

Copy link
Copy Markdown
Contributor Author

I think the regression isn't real.

  • This PR did some very minor rearrangement of startup code that only runs once.
  • It's a 0.2% regression on three of the libc runs, i.e. very small.
  • I can't reproduce it on my machine with a local build.
  • I can't even reproduce it on my machine using the downloaded artifacts. (I got a 0.007% icount increase, not a 0.2% increase.) I don't remember ever seeing that before.

I'm out of ideas! I don't think it's worth investigating any further.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-run-make Area: port run-make Makefiles to rmake.rs perf-regression Performance regression. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants