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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

wasm-opt: Exported global cannot be mutable #886

Closed
Urhengulas opened this issue Jul 29, 2020 · 32 comments 路 May be fixed by #887
Closed

wasm-opt: Exported global cannot be mutable #886

Urhengulas opened this issue Jul 29, 2020 · 32 comments 路 May be fixed by #887

Comments

@Urhengulas
Copy link

Urhengulas commented Jul 29, 2020

馃悰 Bug description

I am using wasm-pack and wasm-bingen to build a javascript package with rust.

Since today when I am running wasm-pack build I am getting following error:

[INFO]: Checking for the Wasm target...
[INFO]: Compiling to Wasm...
    Finished release [optimized + debuginfo] target(s) in 13.59s
[INFO]: Installing wasm-bindgen...
[INFO]: Optimizing wasm binaries with `wasm-opt`...
[wasm-validator error in module] unexpected true: Exported global cannot be mutable, on 
global$0

# --- omitted .wast output ---

Fatal: error in validating input
Error: failed to execute `wasm-opt`: exited with exit code: 1
  full command: "~/.cache/.wasm-pack/wasm-opt-4d7a65327e9363b7/wasm-opt" "$REPO/rav1e_js/pkg/rav1e_js_bg.wasm" "-o" "$REPO/rav1e_js/pkg/rav1e_js_bg.wasm-opt.wasm" "-O"
To disable `wasm-opt`, add `wasm-opt = false` to your package metadata in your `Cargo.toml`.

It was still working on master yesterday (see CI), but doesn't anymore today with the same commit (see CI). (The important step is "Build").

馃 Expected Behavior

Build package and optimize it with wasm-opt as it did before.

(Running wasm-pack build --dev works, but I'd like to have the optimization).

馃憻 Steps to reproduce

Trying a few things I figured out that I can make wasm-opt pass when I am removing all exported (non-static) methods on the exported structs (which is abviously not desireable).

On urhengulas/rav1e@wasm-opt-fix I removed all the methods and it builds and optimizes fine:

鉃  rav1e git:(wasm-opt-fix) 鉁 cd rav1e_js && wasm-pack build
[INFO]: Checking for the Wasm target...
[INFO]: Compiling to Wasm...
    Finished release [optimized + debuginfo] target(s) in 0.05s
[INFO]: License key is set in Cargo.toml but no LICENSE file(s) were found; Please add the LICENSE file(s) to your project directory
[INFO]: Installing wasm-bindgen...
[INFO]: Optimizing wasm binaries with `wasm-opt`...
[INFO]: :-) Done in 0.88s
[INFO]: :-) Your wasm pkg is ready to publish at /home/urhengulas/Documents/github.com/xiph/rav1e/rav1e_js/pkg.

But, as soon I am adding a method (e.g. fn Frame.debug(&self) -> Self in rav1e_js/src/frame.rs) back, I am getting the error from the bug description.

馃實 Your environment

Include the relevant details of your environment.

鉃  rustc --version
rustc 1.45.0 (5c1f21c3b 2020-07-13)
鉃  wasm-pack --version
wasm-pack 0.9.1
鉃  ~/.cache/.wasm-pack/wasm-opt-4d7a65327e9363b7/wasm-opt --version
wasm-opt version_90

Thx

Thanks to everyone taking the time and having a look at this issue!

@EverlastingBugstopper
Copy link
Collaborator

EverlastingBugstopper commented Jul 29, 2020

saw this same issue the other day in some e2e tests

@vtavernier
Copy link

vtavernier commented Jul 29, 2020

For me this issue showed up in CI. The last build that was correct before the failed one today was on July 21st. The nightly compiler from that version already exhibited the bug so it seems the cause is some dependency somewhere triggering this behavior which is not supported by wasm-opt, but I haven't been able to trace which one yet.

@Urhengulas
Copy link
Author

Urhengulas commented Jul 29, 2020

Thanks for the input @EverlastingBugstopper, @vtavernier.


I've created a bit more atomic example:

// src/lib.rs
use wasm_bindgen::prelude::*;

#[wasm_bindgen]
pub struct Boat {
    length: u32,
}

#[wasm_bindgen]
impl Boat {
    // works
    pub fn new(length: u32) -> Self {
        Self { length }
    }

    // doesn't work
    pub fn float(&self) -> String {
        String::from("floating... 鉀")
    }

    // works
    pub fn volume(&self) -> u32 {
        self.length * 2
    }
}

// doesn't work
#[wasm_bindgen]
pub fn string() -> String {
    String::from("wasm-opt")
}

So it seems returning a String fails wasm-opt.

It is also failing, when returning some structs, but for others not. I will investigate what the failing ones have in common.

@vtavernier
Copy link

vtavernier commented Jul 29, 2020

For me the bug was introduced by wasm-bindgen 0.2.66 (0.2.65 is fine).

@Urhengulas
Copy link
Author

Urhengulas commented Jul 30, 2020

The problem seems to be with the non-MVP webassembly feature mutable-global. Adding --enable-mutable-globals to the wasm-opt command seems to fix it (see WebAssembly/binaryen#3006 (comment)).

My workaround is now:

  1. disable wasm-opt in your Cargo.toml ...
[package.metadata.wasm-pack.profile.release]
wasm-opt = false
  1. ... run wasm-pack ...
$ wasm-pack build
  1. ... and optimize manually
$ /path/to/wasm-opt pkg/repo_bg.wasm -o pkg/repo_bg.wasm -O --enable-mutable-globals

(My /path/to/wasm-opt is ~/.cache/.wasm-pack/wasm-opt-4d7a65327e9363b7/wasm-opt.)


Edit: Originally I recommended to use wasm-opt [...] -O -all, but this creates new errors.

Urhengulas added a commit to Urhengulas/wasm-pack that referenced this issue Jul 30, 2020
Urhengulas added a commit to Urhengulas/wasm-pack that referenced this issue Aug 1, 2020
Urhengulas added a commit to Urhengulas/rav1e that referenced this issue Aug 1, 2020
Add '--dev'. This is needed, because of
rustwasm/wasm-pack#886.
Urhengulas added a commit to Urhengulas/rav1e that referenced this issue Aug 1, 2020
Add `--dev` to `wasm-pack build`. This is needed, because of
rustwasm/wasm-pack#886.
@ACollectionOfAtoms
Copy link

ACollectionOfAtoms commented Aug 1, 2020

Ran into the same issue while doing the wasm game of life tutorial: https://rustwasm.github.io/docs/book/game-of-life/implementing.html

@0xd4d
Copy link
Contributor

0xd4d commented Aug 2, 2020

@Urhengulas It's possible to specify wasm-opt arguments, so you should be able to do this:

[package.metadata.wasm-pack.profile.release]
wasm-opt = ["-Oz", "--enable-mutable-globals"]

Change -Oz to whatever optimization level you need.

@Urhengulas
Copy link
Author

Urhengulas commented Aug 3, 2020

@Urhengulas It's possible to specify wasm-opt arguments, so you should be able to do this:

[package.metadata.wasm-pack.profile.release]
wasm-opt = ["-Oz", "--enable-mutable-globals"]

Change -Oz to whatever optimization level you need.

Thanks @0xd4d. This surely is the better workaround.

lu-zero pushed a commit to xiph/rav1e that referenced this issue Aug 3, 2020
Add `--dev` to `wasm-pack build`. This is needed, because of
rustwasm/wasm-pack#886.
segeljakt added a commit to cda-group/arc that referenced this issue Aug 4, 2020
The arc-script online REPL currently fails to compile with the error:

    Exported global cannot be mutable

It is a known error:

    rustwasm/wasm-pack#886

This commit is a fix for the error + a small cleanup of unused code.
segeljakt added a commit to cda-group/arc that referenced this issue Aug 4, 2020
The arc-script online REPL currently fails to compile with the error:

    Exported global cannot be mutable

It is a known error:

    rustwasm/wasm-pack#886

This commit is a fix for the error + a small cleanup of unused code.
@xtuc
Copy link
Member

xtuc commented Aug 5, 2020

Mutable globals are now phase 4 in the WebAssembly CG, it should be enabled by default in wasm-opt

@kripken
Copy link

kripken commented Aug 5, 2020

@xtuc I don't think we have a clear answer on that. Discussions have been going on about what the defaults should be for various tools including wasm-opt and LLVM and we should discuss those more on https://github.com/WebAssembly/tool-conventions cc @sbc100

The main risk I see is that we turn a feature on by default, but not all VMs support it yet, and if the developer doesn't test everywhere then they can end up shipping code that only works in some places.

@sbc100
Copy link

sbc100 commented Aug 5, 2020

In this particular case I think we wasm-opt should be driven by the target-features section of the input file. So perhaps the problem is that llvm/wasm-ld doesn't know to add mutable-globals to the target-features of the output binary.

While I agree that using/adding new features during wasm-opt would be bad, I don't think limiting the input feature set is something wasm-opt should be doing. If my upstream toolchain is choosing to emit mutable-globals wasm-opt should honor that. The job of wasm-opt is not to limit features in its input, right?

@xtuc
Copy link
Member

xtuc commented Aug 5, 2020

My understanding of the target-features section is that it's used by the toolchain but doesn't necessarily reflect what the target browser supports. In JS we used to transpile down the unsupported features given some user preference.

@sbc100
Copy link

sbc100 commented Aug 5, 2020

target-features is used by the toolchain to specific the feature set that the output is expecting. If llvm's output includes the use of given feature that feature should appear in the target-features section. If it doesn't, that is a bug IIUC.

If you want to transpile those features away in order to support older runtimes that seems fine too.

In this case it seems clear that this is the problem: llvm is producing code that depends a feature but that information not being correctly transmitted to wasm-opt.

@EverlastingBugstopper
Copy link
Collaborator

EverlastingBugstopper commented Aug 5, 2020

I don't have much context here on what exactly the problem is but I will say that whatever changed broke pretty much every standard Hello World project in the Rust-Wasm ecosystem and that doesn't feel great. Wherever this change was introduced needs to have some sort of integration test with some common tutorials so that releases don't break documentation across the entire ecosystem.

If a breaking change must happen (which I'd argue it probably doesn't) then there should be advance notice, some warning messages in terminal output before a breaking change (similar to how Rust itself deprecates things), and work put in to update documentation across the ecosystem that does not break with default settings.

edit: i think we might want to revisit whether wasm-pack should even run wasm-opt by default instead of having it be opt in until it has stabilized - these breaking changes are not fun to work with.

@jacogr
Copy link

jacogr commented Aug 5, 2020

For me this ends up slightly more problematic - due to React Native support, I'm also compiling from the generated wasm to asm.js (same binaryen toolset, different binary), and cannot find a working option to make that work at all. (So -all works for wasm-opt and does seem to work on wasm2js, however the output fails all tests)

There is def. a mismatch with this tooling and the binaryen tooling that has "some" impacts.

TL;DR Can (based on suggestions here) get the wasm part compiled, bundled and optimized, however have no luck generating output to an asm.js bundle from the compiled WASM now being generated. For now fixed to the last know working .25 and all is well.

@Urhengulas
Copy link
Author

Urhengulas commented Aug 7, 2020

@EverlastingBugstopper: i think we might want to revisit whether wasm-pack should even run wasm-opt by default instead of having it be opt in until it has stabilized - these breaking changes are not fun to work with.

I agree. Maybe mirroring the cargo build behaviour would be an option. This is wasm-pack build builds in debug mode [what --dev currently does ([optimized + debuginfo], no wasm-opt)] and wasm-pack build --release builds the optimized wasm and also optimizes with wasm-opt (like it currently already does).

jmmv added a commit to endbasic/endbasic that referenced this issue Aug 8, 2020
Do this by allowing mutable globals, which is the current recommendation
per rustwasm/wasm-pack#886.
@Niedzwiedzw
Copy link

Niedzwiedzw commented Dec 9, 2020

just stumbled upon this, what's the solution? update some dependency?

@zimond
Copy link

zimond commented Dec 10, 2020

@Niedzwiedzw currently my solution is to use a fork of wasm-pack, as it's not maintained now. Or you could directly use wasm-bindgen-cli to completely remove wasm-pack dependency.

tie-rack added a commit to tie-rack/pnglitch that referenced this issue Dec 14, 2020
MKolman added a commit to MKolman/AdventOfCode2020 that referenced this issue Dec 21, 2020
dbrgn pushed a commit to threema-ch/compose-area that referenced this issue Dec 24, 2020
wasm-bindgen is currently locked to 0.2.65 due to
rustwasm/wasm-pack#886
dbrgn pushed a commit to threema-ch/compose-area that referenced this issue Dec 24, 2020
wasm-bindgen is currently locked to 0.2.65 due to
rustwasm/wasm-pack#886
vibhoothi pushed a commit to rust-av/rav1e_js that referenced this issue Dec 30, 2020
Add `--dev` to `wasm-pack build`. This is needed, because of
rustwasm/wasm-pack#886.
@arilotter
Copy link

arilotter commented Jan 4, 2021

This issue may be fixed by rustwasm/wasm-bindgen#2391 , which prevents wasm-bindgen from exporting mutable globals for compatibility reasons.

madonoharu added a commit to madonoharu/fleethub that referenced this issue Jan 15, 2021
@arilotter
Copy link

arilotter commented Jan 27, 2021

rustwasm/wasm-bindgen#2391 has been merged & released in wasm-bindgen 0.2.70. I believe this fixes the issue, and this ticket can now be closed.

sarcaustech added a commit to sarcaustech/unifac-wasm that referenced this issue Feb 2, 2021
- Building adapter crate
- Requires wasm-bindgen ^0.2.70 due to bug in toolchain [1]
- Requires local installation of sarcaustech/unifac [2] with no
  parallelization (Rayon). Used with commit 4711916

[1] rustwasm/wasm-pack#886
[2] https://github.com/sarcaustech/unifac
rudsvar added a commit to HotDrink/hotdrink-rs that referenced this issue Mar 16, 2021
sharksforarms added a commit to sharksforarms/deku that referenced this issue Apr 6, 2021
Removed wasm-pack config now that issue is fixed:
rustwasm/wasm-pack#886
@felipellrocha
Copy link

felipellrocha commented Aug 25, 2021

Is this fixed? I just ran into it trying to compile the threads example: https://rustwasm.github.io/docs/wasm-bindgen/examples/raytrace.html

madonoharu added a commit to madonoharu/fleethub that referenced this issue Oct 2, 2021
jmmv added a commit to endbasic/endbasic that referenced this issue Oct 24, 2021
rustwasm/wasm-pack#886 was resolved a while
ago, so we can remove this as long as we have a sufficiently-moderm
wasm-bindgen.
jmmv added a commit to endbasic/endbasic that referenced this issue Oct 25, 2021
rustwasm/wasm-pack#886 was resolved a while
ago, so we can remove this as long as we have a sufficiently-moderm
wasm-bindgen.
jmmv added a commit to endbasic/endbasic that referenced this issue Oct 26, 2021
rustwasm/wasm-pack#886 was resolved a while
ago, so we can remove this as long as we have a sufficiently-moderm
wasm-bindgen.
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

Successfully merging a pull request may close this issue.