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

Generated WebAssembly unexpectedly requires reference types #128475

Closed
PlasmaPower opened this issue Aug 1, 2024 · 11 comments · Fixed by #128511
Closed

Generated WebAssembly unexpectedly requires reference types #128475

PlasmaPower opened this issue Aug 1, 2024 · 11 comments · Fixed by #128511
Labels
C-bug Category: This is a bug. O-wasm Target: WASM (WebAssembly), http://webassembly.org/ P-high High priority regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@PlasmaPower
Copy link
Contributor

PlasmaPower commented Aug 1, 2024

Code

This can be replicated with a minimal test.rs:

fn main() {}

Compile it to WASM with rustc test.rs -o test.wasm --target wasm32-unknown-unknown

Then, check if the generated WASM uses reference types:

$ wasm-validate --disable-reference-types test.wasm
0000260: error: call_indirect reserved value must be 0

Version it worked on

It most recently worked on: nightly-2024-07-30 (and previous stable rust versions)

On those versions, compiling the above test.rs and then running wasm-validate with --disable-reference-types doesn't fail.

Version with regression

rustc --version --verbose:

rustc 1.82.0-nightly (28a58f2fa 2024-07-31)
binary: rustc
commit-hash: 28a58f2fa7f0c46b8fab8237c02471a915924fe5
commit-date: 2024-07-31
host: x86_64-unknown-linux-gnu
release: 1.82.0-nightly
LLVM version: 19.1.0

@rustbot modify labels: +regression-from-stable-to-nightly -regression-untriaged

This seems to have been caused by #127513

The compatibility note there says:

The WebAssembly target features multivalue and reference-types are now both enabled by default, but generated code is not affected by default

However, the generated code seems to be indeed affected by default. Interestingly, reference types seem to be used unnecessarily, as using wasm2wat to turn the generated test.wasm into wat and then wat2wasm to turn it back into wasm removes the reference types.

As-is, this breaks compatibility with the WebAssembly 1.0 spec.

@PlasmaPower PlasmaPower added C-bug Category: This is a bug. regression-untriaged Untriaged performance or correctness regression. labels Aug 1, 2024
@rustbot rustbot added I-prioritize Issue: Indicates that prioritization has been requested for this issue. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. and removed regression-untriaged Untriaged performance or correctness regression. labels Aug 1, 2024
@jieyouxu jieyouxu added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. O-wasm Target: WASM (WebAssembly), http://webassembly.org/ labels Aug 1, 2024
@nikic
Copy link
Contributor

nikic commented Aug 1, 2024

cc @alexcrichton @daxpedda Looks like this causes issues in practice and we might want to do nikic#1 after all?

@apiraino
Copy link
Contributor

apiraino commented Aug 1, 2024

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-high

@rustbot rustbot added P-high High priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Aug 1, 2024
@daxpedda
Copy link
Contributor

daxpedda commented Aug 1, 2024

I was unable to confirm the presence of any reference type instructions in the generated Wasm, even though I got the same error pointing towards a call_indirect that looks perfectly valid to me.

Interestingly enough, doing the back and forth conversion with wasm2wat and wat2wasm remove the issue, even though the output of both files is exactly the same, apart from the missing debug information.

I'm assuming that this is another example of a tool making use of the target feature section, so this was already anticipated.

Unfortunately (?) this isn't the first time LLVM has enabled features by default, in the past they even did affect code generation. From what I've read in the various WebAssembly specification groups, this was always intended.

In conclusion, I agree this is a form of breakage, it seems to be intended, I believe if Rust wants to do anything about it the solution needs to a bit more comprehensive. E.g. actually disable all target features by default (I don't know if this is feasible with LLVM, would need research).

See #109807 and #119811.

@daxpedda
Copy link
Contributor

daxpedda commented Aug 1, 2024

I just spend some time digging into this error message that wasm2wat spits out.
Locally I've produced exactly the same message, but it really doesn't make any sense to me.

The location pointed out by the error message does indeed contain a call to call_indirect, but the reserved value is 0, so I don't know what the issue is. Further, after using the wasm2wat -> wat2wasm trick, it doesn't produce this error anymore, but the code location contains the exact same bytes.

I also checked if wasm-validate reads the target feature section. While I confirmed it does, I don't believe it validates it from what I can see in the code.

So maybe there is some sort of codegen difference, but I am unable to actually find it.

@hanna-kruppe
Copy link
Contributor

Could it be the encoding difference discussed in the description of this PR? llvm/llvm-project#90792

the way the table index is encoded is different from MVP (u32) vs. reference-types (LEB), which caused different encodings for call_indirect.

@daxpedda
Copy link
Contributor

daxpedda commented Aug 1, 2024

Could it be the encoding difference discussed in the description of this PR? llvm/llvm-project#90792

the way the table index is encoded is different from MVP (u32) vs. reference-types (LEB), which caused different encodings for call_indirect.

Yes, I just confirmed that's the issue.
So it does affect codegen, even if in a minor way!

@alexcrichton
Copy link
Member

Yes the problem is the LEB-encoding of the table index immediate. Using wasm-tools dump on the output of rustc I see:

    0x395 | 11 80 80 80 | call_indirect type_index:0 table_index:0
          | 80 00 80 80
          | 80 80 00

which shows that call_indirect's type and table index are both using a 5-byte encoding of a zero value. This is not valid in MVP WebAssembly which required the table index to be a literal zero byte, not a LEB. Given llvm/llvm-project#90792 I doubt LLVM will do anything here so it sounds like yes, we'll have to solve this in Rust.

I was unable to confirm the presence of any reference type instructions in the generated Wasm

I believe this is due to the the binary encoding of the LEB. This isn't reflected in the text format and most other tools don't handle this well, so it's a subtle error. That's also why convert-to-text-and-back produces a working binary.

I'm assuming that this is another example of a tool making use of the target feature section, so this was already anticipated.

To clarify, I don't believe that this is the case. I believe modifications to the target feature section are unrelated.


Ok so the question now is what to do. I see a few possible routes:

  1. Accept this change. That would entail updating the release notes for the LLVM upgrade PR to indicate that wasm targets are indeed affected. This would probably also detail the workaround of -Ctarget-cpu=mvp. Note that -Ctarget-feature=-reference-types may not be enough given findings in Can not disable sign-ext feature for wasm32 target #109807.
  2. Don't accept this change. That would require something along the lines of Disable Wasm unstable target features according to Rust nikic/rust#1, perhaps modified to use -Ctarget-cpu=mvp then adding on features afterwards.
  3. "Fix" this change. This would require coordinating with upstream LLVM to change how this works. My guess is that given the existence of [WebAssembly] Disable reference types in generic CPU llvm/llvm-project#90792 this seems unlikely to happen.

There's not really a governing body per se around the Rust WebAssembly targets so there's not really a group of people we can point to and say "ok please tell us what to do". In lieu of that I would personally be tempted to go with route (1) because that keeps LLVM/C/C++ aligned with Rust and prevents divergence in the wasm ecosystem. Given the age of the reference-types proposal and the general intention for enabling more features by default over time as popular engines implement them this will also continue to change.

So, concretely, I would propose:

  • Do not change code as a result of this issue, leave it as-is.
  • Update the release notes for LLVM 19 to indicate that WebAssembly targets are affected
  • Update the WebAssembly documentation to explicitly document how to achieve and "MVP binary" by using -Clto and -Ctarget-cpu=mvp.

@wesleywiser
Copy link
Member

Discussed in the compiler team triage meeting. We think @alexcrichton's plan is reasonable at present but if there are additional issues or complications (such as it being more difficult to create an MVP binary than just using -Clto -Ctarget-cpu=mvp), then option 2 (use -Ctarget-cpu=mvp by default) seems to be the way to go.

@alexcrichton
Copy link
Member

Looks like there may be complications. I am endeavouring to add a test to CI which asserts that a particular compiler invocation will generate MVP features. So far roadblocks are:

  • Using -Clto -Ctarget-cpu=mvp is not sufficient. The target-cpu attribute is not rewritten for IR of dependent libraries, like the standard library. That means that -Ctarget-cpu=mvp is only applied to the local crate and the standard library still codegens newer features.
  • One can fix the above issue to additionally use -Ctarget-feature=-reference-types but you'd have to pass -Ctarget-feature for all active features. Not great.
  • Even with -Ctarget-feature=-reference-types there are still bulk-memory instructions in at least memmove for example. These are coming from wasi-libc for the wasm32-wasip1 target which is what rustc tests in CI. This is not applicable to this issue as users typically use wasm32-unknown-unknown for this but would make adding a test difficult.

Trying out the wasm32-unknown-unknown target locally I can't even get something to work it seems. LLVM is producing an invalid binary with -Ctarget-cpu=mvp -Ctarget-feature=-sign-ext alone when there's a memcpy in the binary.

There's... a lot going on here apparently. Given the above I don't think that (2) I described above is a silver bullet by any means, even that seems doubtful to work. I need to investigate more what's going on here with all these features slinging all over the place.

@alexcrichton
Copy link
Member

Testing shows that -Ctarget-cpu=mvp plus Cargo's -Zbuild-std is sufficient. That's more-or-less the same as (1) so I'll take it on myself to document this and get it all written down. Unfortunately that doesn't lend itself well to a test in-repo but that's not necessarily worse than where we are at today.

@alexcrichton
Copy link
Member

I've proposed #128511 to close this issue as "this change is expected".

@saethlin saethlin removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Aug 4, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Aug 22, 2024
…jieyouxu

Document WebAssembly target feature expectations

This commit is a result of the discussion on rust-lang#128475 and incorporates parts of rust-lang#109807 as well. This is all done as a new page of documentation for the `wasm32-unknown-unknown` target which previously did not exist. This new page goes into details about the preexisting target and additionally documents the expectations for WebAssembly features and code generation.

The tl;dr is that LLVM will enable features over time after most engines have had support for awhile. Compiling without features requires `-Ctarget-cpu=mvp` to rustc plus `-Zbuild-std` to Cargo.

Closes rust-lang#109807
Closes rust-lang#119811
Closes rust-lang#128475
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Aug 22, 2024
…jieyouxu

Document WebAssembly target feature expectations

This commit is a result of the discussion on rust-lang#128475 and incorporates parts of rust-lang#109807 as well. This is all done as a new page of documentation for the `wasm32-unknown-unknown` target which previously did not exist. This new page goes into details about the preexisting target and additionally documents the expectations for WebAssembly features and code generation.

The tl;dr is that LLVM will enable features over time after most engines have had support for awhile. Compiling without features requires `-Ctarget-cpu=mvp` to rustc plus `-Zbuild-std` to Cargo.

Closes rust-lang#109807
Closes rust-lang#119811
Closes rust-lang#128475
tgross35 added a commit to tgross35/rust that referenced this issue Aug 23, 2024
…jieyouxu

Document WebAssembly target feature expectations

This commit is a result of the discussion on rust-lang#128475 and incorporates parts of rust-lang#109807 as well. This is all done as a new page of documentation for the `wasm32-unknown-unknown` target which previously did not exist. This new page goes into details about the preexisting target and additionally documents the expectations for WebAssembly features and code generation.

The tl;dr is that LLVM will enable features over time after most engines have had support for awhile. Compiling without features requires `-Ctarget-cpu=mvp` to rustc plus `-Zbuild-std` to Cargo.

Closes rust-lang#109807
Closes rust-lang#119811
Closes rust-lang#128475
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Aug 23, 2024
…jieyouxu

Document WebAssembly target feature expectations

This commit is a result of the discussion on rust-lang#128475 and incorporates parts of rust-lang#109807 as well. This is all done as a new page of documentation for the `wasm32-unknown-unknown` target which previously did not exist. This new page goes into details about the preexisting target and additionally documents the expectations for WebAssembly features and code generation.

The tl;dr is that LLVM will enable features over time after most engines have had support for awhile. Compiling without features requires `-Ctarget-cpu=mvp` to rustc plus `-Zbuild-std` to Cargo.

Closes rust-lang#109807
Closes rust-lang#119811
Closes rust-lang#128475
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Aug 23, 2024
…jieyouxu

Document WebAssembly target feature expectations

This commit is a result of the discussion on rust-lang#128475 and incorporates parts of rust-lang#109807 as well. This is all done as a new page of documentation for the `wasm32-unknown-unknown` target which previously did not exist. This new page goes into details about the preexisting target and additionally documents the expectations for WebAssembly features and code generation.

The tl;dr is that LLVM will enable features over time after most engines have had support for awhile. Compiling without features requires `-Ctarget-cpu=mvp` to rustc plus `-Zbuild-std` to Cargo.

Closes rust-lang#109807
Closes rust-lang#119811
Closes rust-lang#128475
@bors bors closed this as completed in e65a48e Aug 23, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Aug 23, 2024
Rollup merge of rust-lang#128511 - alexcrichton:doc-wasm-features, r=jieyouxu

Document WebAssembly target feature expectations

This commit is a result of the discussion on rust-lang#128475 and incorporates parts of rust-lang#109807 as well. This is all done as a new page of documentation for the `wasm32-unknown-unknown` target which previously did not exist. This new page goes into details about the preexisting target and additionally documents the expectations for WebAssembly features and code generation.

The tl;dr is that LLVM will enable features over time after most engines have had support for awhile. Compiling without features requires `-Ctarget-cpu=mvp` to rustc plus `-Zbuild-std` to Cargo.

Closes rust-lang#109807
Closes rust-lang#119811
Closes rust-lang#128475
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. O-wasm Target: WASM (WebAssembly), http://webassembly.org/ P-high High priority regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants