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

Build failure with Rust 1.82.0 and bundled wasm-opt #904

Open
eric-seppanen opened this issue Nov 4, 2024 · 15 comments
Open

Build failure with Rust 1.82.0 and bundled wasm-opt #904

eric-seppanen opened this issue Nov 4, 2024 · 15 comments

Comments

@eric-seppanen
Copy link

After upgrading from Rust 1.81 to 1.82 I discovered that a particular wasm/trunk project no longer builds. It always fails with:

[parse exception: invalid code after misc prefix: 17 (at 0:20646)]
Fatal: error parsing wasm (try --debug for more info)
2024-11-04T23:06:47.941140Z ERROR ❌ error
error from build pipeline

Caused by:
    0: HTML build pipeline failed (1 errors), showing first
    1: error from asset pipeline
    2: running wasm-opt
    3: wasm-opt call to executable '/home/___/.cache/trunk/wasm-opt-version_116/bin/wasm-opt' with args: '["--output=.../wasm_test_2024_b/target/wasm-opt/release/wasm_test_2024_bg.wasm", "-Oz", ".../wasm_test_2024_b/dist/.stage/wasm_test_2024-616af1a08af380ae_bg.wasm"]' returned a bad status: exit status: 1
2024-11-04T23:06:47.941182Z ERROR error from build pipeline
2024-11-04T23:06:47.941184Z  INFO   1: HTML build pipeline failed (1 errors), showing first
2024-11-04T23:06:47.941186Z  INFO   2: error from asset pipeline
2024-11-04T23:06:47.941187Z  INFO   3: running wasm-opt
2024-11-04T23:06:47.941189Z  INFO   4: wasm-opt call to executable '/home/___/.cache/trunk/wasm-opt-version_116/bin/wasm-opt' with args: '["--output=.../wasm_test_2024_b/target/wasm-opt/release/wasm_test_2024_bg.wasm", "-Oz", ".../wasm_test_2024_b/dist/.stage/wasm_test_2024-616af1a08af380ae_bg.wasm"]' returned a bad status: exit status: 1

I built a minimized project that demonstrates the issue here: https://github.com/eric-seppanen/wasm_test_2024

This seems to be a wasm-opt issue, but since wasm-opt is bundled with trunk by default, perhaps it would be a good idea to ship a newer version?

@eric-seppanen
Copy link
Author

I investigated a few versions of wasm-opt and found that any version >= 117 works, but they all require an additional CLI flag --enable-bulk-memory.

I'm not sure how to make this happen when using trunk.

So perhaps this is related to #866?

@ctron
Copy link
Collaborator

ctron commented Nov 5, 2024

It might make sense to update the default versions for the different tools:

trunk/src/tools.rs

Lines 98 to 105 in 306fa0b

pub(crate) fn default_version(&self) -> &str {
match self {
Self::Sass => "1.69.5",
Self::TailwindCss => "3.3.5",
Self::WasmBindgen => "0.2.89",
Self::WasmOpt => "version_116",
}
}

Also #901 might help.

@eric-seppanen
Copy link
Author

After some more experimentation, This problem seems specific to wasm-bindgen/wasm-bindgen-cli 0.2.95 (0.2.93 works; 0.2.94 was yanked but I think it's also broken). But the problem is only triggered by a build with rust 1.82.0.

I don't think it matters what Trunk's default wasm-bindgen version is-- the version of wasm-bindgen-cli has to match the version of the wasm-bindgen library crate. It looks like Trunk does this matching automatically, so the failure appears when my project uses wasm-bindgen >= 0.2.94.

@kunhualqk
Copy link

I have the same problem

Finished `release` profile [optimized] target(s) in 6.89s

[parse exception: invalid code after misc prefix: 17 (at 0:564976)]
Fatal: error parsing wasm (try --debug for more info)
2024-11-08T02:11:23.262311Z ERROR ❌ error
error from build pipeline

rustc 1.82.0 (f6e511eec 2024-10-15)
trunk 0.21.4

@ctron
Copy link
Collaborator

ctron commented Nov 8, 2024

As you're using trunk 0.21.4, you should be able to use the new data-wasm-opt-param flag. If you find some reasonable settings that "just work", we could add them as a default, as we now also have the version available internally.

@ctron
Copy link
Collaborator

ctron commented Nov 8, 2024

Or maybe we need to bump the default version of wasm-opt.

@eric-seppanen
Copy link
Author

Also note that this will be fixed in the next release of wasm-bindgen, which won't emit table.fill instructions by default when it encounters the wasm metadata output by rustc 1.82.

@isosphere
Copy link

As you're using trunk 0.21.4, you should be able to use the new data-wasm-opt-param flag. If you find some reasonable settings that "just work", we could add them as a default, as we now also have the version available internally.

I had this issue and resolved it for myself with the following:

I admit I do not know what bulk memory or table.fill instructions are, perhaps there are consequences to this that I am unaware of.

@ctron
Copy link
Collaborator

ctron commented Nov 12, 2024

Thanks for the summary. That is super helpful. Now that we have internal APIs for the tools in place, would it make sense to auto-enable --enable-bulk-memory in case the wasm-opt version 119 or higher?

@eric-seppanen
Copy link
Author

Note that rustc doesn't enable bulk memory by default, so to me the safest default would be to track whatever rustc is doing. Enabling bulk memory instructions by default could affect compatibility with existing runtimes.

The reason we're having problems with rust 1.82.0 is because of a bug in wasm-bindgen 0.2.94-0.2.95: wasm-bindgen notices that rustc has enabled new target features, and accidentally enables bulk memory on its output as a side-effect. This will no longer be the case in wasm-bindgen 0.2.96.

@eric-seppanen
Copy link
Author

eric-seppanen commented Nov 12, 2024

What should happen in the long term? A few possibilities I can think of:

  1. By default enable the most common set of target features when calling wasm-opt. This is hard to do as this set is changing across rustc versions and there are runtime compatibility hazards in enabling the wrong thing.
  2. Require trunk users to specify a set of wasm-opt flags that match their runtime capabilities and their rustc output. This gives users the power to solve their own problems, but many users won't know what settings to use, which makes the tools harder to use correctly.
  3. Automatically detect what rustc is doing, and try to configure wasm-opt the same way. Ideally wasm-opt would be doing this itself1, but if that's not going to happen then this might be a kindness to users who just want things to work without needing a deep dive into webassembly proposals and rustc target-features. Perhaps this is possible by reading the "target-features" section of the wasm output?

Is wasm-opt necessary for trunk to work? Perhaps if wasm-opt is going to need such careful configuration it should be an optional step in the build process (for users that don't want the deep dive).

Footnotes

  1. wasm-opt has a deprecated flag --detect-features but I can't find any documentation on whether this functionality ever existed or still exists. Maybe more research is needed here?

@troelsfr
Copy link

Thanks for this thread! It's super helpful.

Is it possible to specify default wasm-opt version through the configuration file? From a quick look at the documentation and code it appears not, but maybe I missed something.

For others, this might be helpful: I managed to get things working by cloning the trunk, then patching trunk/src/tools.rs changing the default version to 119 and adding data-wasm-opt-params="--enable-bulk-memory" as proposed by @isosphere.

One thing I noted is data-wasm-opt-params="--enable-bulk-memory" seems to not work if you've set data-wasm-opt="s".

@ctron
Copy link
Collaborator

ctron commented Nov 19, 2024

I guess the answer to all of this is: PRs welcome 😉

@hmacias-avaya
Copy link
Contributor

hmacias-avaya commented Nov 19, 2024

@troelsfr the wasm-opt version is configurable already through Trunk.toml https://github.com/trunk-rs/trunk/blob/4c9d85f6f04a31a272c71db8df12e142c76311fb/Trunk.toml

I'm using:

[tools]
wasm_opt = "version_119"

and this on my html:

<link data-trunk rel="rust" data-bin="some-app-name" data-wasm-opt="z"
        data-wasm-opt-params="--enable-bulk-memory" />

@troelsfr
Copy link

Sorry for not reading the docs careful enough! And thanks a lot for pointing me to the right place!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants