Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add wasm32v1-none target (compiler-team/#791) #131487

Merged
merged 4 commits into from
Oct 23, 2024

Conversation

graydon
Copy link
Contributor

@graydon graydon commented Oct 10, 2024

This is a preliminary implementation of the MCP discussed in compiler-team#791. It's not especially "major" but you know, process! Anyway it adds a new wasm32v1-none target which just pins down a set of wasm features. I think this is close to the consensus that emerged when discussing it on Zulip so I figured I'd sketch to see how hard it is. Turns out not very.

@rustbot
Copy link
Collaborator

rustbot commented Oct 10, 2024

r? @Nadrieril

rustbot has assigned @Nadrieril.
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

@rustbot rustbot added 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 Oct 10, 2024
@rust-log-analyzer

This comment has been minimized.

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

For tier 2-related bits, some other locations you'll want to modify are:

@Nadrieril
Copy link
Member

r? @alexcrichton ?

@rustbot rustbot assigned alexcrichton and unassigned Nadrieril Oct 11, 2024
@graydon graydon marked this pull request as ready for review October 16, 2024 23:57
@rustbot
Copy link
Collaborator

rustbot commented Oct 16, 2024

Some changes occurred in src/doc/rustc/src/platform-support

cc @Noratrieb

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

@rust-log-analyzer

This comment has been minimized.

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Oct 17, 2024
Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Oops sorry I wrote these comments last week but forgot to hit submit...

Otherwise though the MCP has now finished so with this it looks good to me and would be happy to approve.

src/doc/rustc/src/platform-support/wasm32v1-none.md Outdated Show resolved Hide resolved
$ rustc -target wasm32v1-none
```

The difference is in how the `core` and `alloc` crates are compiled for distribution with the toolchain, and whether it works on _stable_ Rust toolchains or requires _nightly_ ones. Again referring back to the [`wasm32-unknown-unknown` document](./wasm32-unknown-unknown.md), note that to disable all post-MVP proposals on that target one _actually_ has to compile with this:
Copy link
Member

Choose a reason for hiding this comment

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

If you're feeling up for it (optional as I'm happy to do this later too), mind updating the docs in wasm32-unknown-unknown.md with respect to this new target? For example the documentation can indicate how to disable default features both with -Zbuild-std but also referring to this target primarily. The -Zbuild-std bits aren't really need any more unless you really want the standard library of wasm32-unknown-unknown which is probably unlikely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add a note, but I think the build-std part needs to stay if you're targeting MVP and not MVP+mutable-globals as the wasm32v1 target supplies. Or if you need std.

@@ -1803,6 +1803,7 @@ supported_targets! {

("wasm32-unknown-emscripten", wasm32_unknown_emscripten),
("wasm32-unknown-unknown", wasm32_unknown_unknown),
("wasm32v1-none", wasm32v1_none),
Copy link

@leighmcculloch leighmcculloch Oct 22, 2024

Choose a reason for hiding this comment

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

Will the way to cfg on this specific target be to specify:

#[cfg(all(target_family = "wasm", target_os = "none"))]

or

#[cfg(all(target_arch = "wasm32", target_os = "none"))]

If in the future a wasm32v2-none is released, how will it be distinguished in a cfg?

Or will the target_arch be wasm32v1 and in the future wasm32v2?

Copy link
Contributor

Choose a reason for hiding this comment

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

You can just differentiate them through the target features.

Copy link

@leighmcculloch leighmcculloch Oct 22, 2024

Choose a reason for hiding this comment

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

Could you provide an example?

At the moment differentiating using target_feature is unreliable, because it's common for target features to become released but their cfg to be still marked as unstable and is therefore not usable in stable rust, as has been the case with:

Copy link
Member

Choose a reason for hiding this comment

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

FWIW the documentation in this PR has an answer to your original question:

It's recommended to conditionally compile code for this target with:

#[cfg(all(target_family = "wasm", target_os = "none"))]

Otherwise I would also agree with @CryZe. All other native platforms have a constant target_arch string with a variable target_feature set corresponding to levels of support for various features.

The situation you're referring to where multivalue and reference-types aren't stable right now isn't an issue for this target. This'll be a new target when it lands and users should also be able to see multivalue and reference-types when they get to use it since that PR should land relatively soon as well.

Stabilization of wasm features has lagged behind for awhile and this is basically good motivation to ensure we keep up in the future. For example another option is blocking a hypothetical wasm32v2-none target until all of the features it enables are landed and stable in Rust. (or, for example, blocking this PR on #131080 but to be clear I'm not advocating we do that)

Copy link

@leighmcculloch leighmcculloch Oct 22, 2024

Choose a reason for hiding this comment

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

Will the target_env be set to v1 or v2?

For example, the following cfg to unambiguously distinguish the target:

#[cfg(all(target_arch = "wasm32", target_env = "v1", target_os = "none"))]

Copy link
Member

Choose a reason for hiding this comment

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

As Alex said: The incentive structure that encourages people to push for stabilizing a target feature name that is well-defined, so that we can e.g. evaluate it for its ABI impacts, is intentional.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think what @leighmcculloch is looking for is a way to make a cfg check that specifically identifies the condition of "being limited to the features in wasm32v1-none". Like he wants to not detect any specific new features, but detect the absence of all new features, including features not-yet-invented. Because that's the only case we want to compile for. We want to emit an error if the user tries to build for a target that enables any further wasm features. Which -- even if the current set of feature names all stabilize -- he can't write code to detect because they don't exist yet.

If there were -- speaking hypothetically -- some way to say cfg(target_triple="wasm32v1-none") or even cfg(target_subarch="wasm32v1") that would do the trick. Or anything else that pins to this subarch version and not future versions. I'm not sure if there's a good way to make that happen with the tools currently available though.

Copy link
Member

@workingjubilee workingjubilee Oct 23, 2024

Choose a reason for hiding this comment

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

I believe there is nonesuch in "ways to conveniently do that", yes.

The answer, however, is cfg(target_feature = "nontrapping-fptoint"), or the cfg(not()) version.

The reason that will work is that feature was specifically added for Rust, and it is part of the v2 definition, so "post v1" Rust targets simply will not ever exclude it.

Copy link
Member

Choose a reason for hiding this comment

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

Unless you're asking for "and no 'manual' use of any target_feature anywhere, either", which is:

  • not about the target tuple
  • much closer to "just plain impossible" due to source-level target_feature(enable = "feature") being possible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mhm. Thanks. I'm sure we'll figure out some not-so-bad guardrails that handle the majority of cases.

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Oct 23, 2024

📌 Commit b0f0282 has been approved by alexcrichton

It is now in the queue for this repository.

@bors 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 Oct 23, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 23, 2024
Rollup of 5 pull requests

Successful merges:

 - rust-lang#131043 (Refactor change detection for rustdoc and download-rustc)
 - rust-lang#131181 (Compiletest: Custom differ)
 - rust-lang#131487 (Add wasm32v1-none target (compiler-team/rust-lang#791))
 - rust-lang#132054 (do not remove `.cargo` directory)
 - rust-lang#132058 (CI: rfl: use rust-next temporary commit)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 03cb7de into rust-lang:master Oct 23, 2024
6 checks passed
@rustbot rustbot added this to the 1.84.0 milestone Oct 23, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Oct 23, 2024
Rollup merge of rust-lang#131487 - graydon:wasm32v1-none, r=alexcrichton

Add wasm32v1-none target (compiler-team/rust-lang#791)

This is a preliminary implementation of the MCP discussed in [compiler-team#791](rust-lang/compiler-team#791). It's not especially "major" but you know, process! Anyway it adds a new wasm32v1-none target which just pins down a set of wasm features. I think this is close to the consensus that emerged when discussing it on Zulip so I figured I'd sketch to see how hard it is. Turns out not very.
@danielhjacobs
Copy link

danielhjacobs commented Oct 28, 2024

We're thinking of switching to this target in our code-base once it reaches stable, but another developer had questions.

  • is std:: accessible (even if it only re-exports stuff from core::)
  • is std::sync:Mutex available
  • are functional parts like std::path::Path or std::time::Duration available

and, if any/all of these are false:

  • can we manually, easily re-enable std to function exactly the same as it does on wasm32-unknown-unknown

The code-base, for reference, is https://github.com/ruffle-rs/ruffle. I'll also add that we prefer to do everything with stable Rust.

@danielhjacobs
Copy link

In testing, I got these errors:

error[E0463]: can't find crate for `core`
  |
  = note: the `wasm32-unknown-unknown` target may not be installed
  = help: consider downloading the target with `rustup target add wasm32-unknown-unknown`
  = help: consider building the standard library from source with `cargo build -Zbuild-std`

For more information about this error, try `rustc --explain E0463`.
error: could not compile `cfg-if` (lib) due to 1 previous error
warning: build failed, waiting for other jobs to finish...
error[E0463]: can't find crate for `std`
  |
  = note: the `wasm32-unknown-unknown` target may not be installed
  = help: consider downloading the target with `rustup target add wasm32-unknown-unknown`
  = help: consider building the standard library from source with `cargo build -Zbuild-std`

@alexcrichton
Copy link
Member

@danielhjacobs there's no std, so no Mutex or Path or Duration. There's no means by which to re-enable std for this target unless you're using -Zbuild-std, at which point you can also use wasm32-unknown-unknown.

For the build error, I think that's still something using --target wasm32-unknown-unknown which needs to switch to wasm32v1-none, but if you need std to work then this target won't work for you

@danielhjacobs
Copy link

Hi Alex. That's unfortunate, we were hoping to keep Ruffle working on Pale Moon and on old versions of Safari before the reference types proposal was supported. We can't do that without pinning to Rust 1.81.0 or using nightly Rust, which we don't want to do. rustwasm/wasm-bindgen#4213 will cause wasm-bindgen to at least generate JS glue code that doesn't use those features, but the statement that "For the reference-types feature the encoding of immediates in the call_indirect, a commonly used instruction by the WebAssembly backend, has changed. Validators and parsers which don't understand the reference-types proposal will no longer accept modules produced by LLVM due to this change in encoding of immediates." makes me wary Ruffle will work on those browsers even after that wasm-bindgen PR is out in a new release.

@danielhjacobs
Copy link

danielhjacobs commented Oct 28, 2024

I assume there's no chance to get a target like this but which builds std?

Note that I think I fixed the references to wasm32-unknown-unknown and now I'm getting these errors:

message.txt

@leighmcculloch
Copy link

error[E0463]: can't find crate for core

Does this mean core is also unavailable on this target?

@danielhjacobs
Copy link

Does this mean core is also unavailable on this target?

No, that one was probably my mistake. After installing this target, I failed to update https://github.com/ruffle-rs/ruffle/blob/e81bbe858f07fe8c52e1e30f660eeaea440708c5/web/packages/core/tools/build_wasm.ts. The latest attachment of error messages are the errors I got after changing that file to use this target instead.

@workingjubilee
Copy link
Member

I assume there's no chance to get a target like this but which builds std?

Probably Not. We do not really want to entertain another target that has a standard library as busted as wasm32-unknown-unknown's is.

but the statement that "For the reference-types feature the encoding of immediates in the call_indirect, a commonly used instruction by the WebAssembly backend, has changed. Validators and parsers which don't understand the reference-types proposal will no longer accept modules produced by LLVM due to this change in encoding of immediates." makes me wary Ruffle will work on those browsers even after that wasm-bindgen PR is out in a new release.

Using this target, plus the relevant wasm tooling with that "don't enable externref automatically" PR merged, should generate correct code for your use-case that is recognized by Pale Moon and old Safari.

The errors in the log you showed are from using crates with features enabled that cause them to not support no_std. As the once_cell crate supports using it on no_std, and I did not see any direct dependencies on it in your tree's Cargo.tomls, it is the result of other crates you depend on.

@danielhjacobs
Copy link

I'm not sure how we'd be able to accommodate the loss of std::sync in the web directory: https://github.com/search?q=repo%3Aruffle-rs%2Fruffle+std%3A%3Async+path%3Aweb%2F&type=code

@workingjubilee
Copy link
Member

I opened this issue: ruffle-rs/ruffle#18412

I will continue the discussion there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc 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. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.