WebAssembly / binaryen

Optimizer and compiler/toolchain library for WebAssembly
Apache License 2.0
7.55k stars 745 forks source link

Asyncify doesn't work with optimization wasm-opt -O #4760

Open dsyer opened 2 years ago

dsyer commented 2 years ago

The sample example input.wat from @kripken 's blog fails to compile for me, so maybe I'm using the wrong tool:

$ wat2wasm input.wat > input.wasm
input.wat:3:4: error: imports must occur before all non-import definitions
  (import "spectest" "print" (func $print (param i32)))
   ^^^^^^
...

I can fix that by simply moving the import definitions to the top. But then the suggested optimization fails too:

$ wasm-opt input.wat -O --asyncify --print
[parse exception: bad global.get name (at 14:15)]
Fatal: error parsing wasm

The blog is old and wasm-opt seems not to support compiling .wat to .wasm so maybe that's the problem:

$ wat2wasm input.wat > input.wasm
$ wasm-opt input.wasm -O --asyncify -o output.wasm
$ wasm2wat output.wasm > output.wat

But now that doesn't work with wasm-shell:

$ wasm-shell output.wat
BUILDING MODULE [line: 1]
[parse exception: expected list (at 7:4)]

I have no idea how to fix that, but I know I can run the .wasm from Node.js:

> const file = fs.readFileSync('./output.wasm');
> let wasm = await WebAssembly.instantiate(file, { "spectest": { "print": console.log } });
1
3
2
1
3
undefined

It runs but it looks like the vanilla "call main twice" behaviour. I can make it work without the -O flag:

$ wat2wasm input.wat > input.wasm
$ wasm-opt input.wasm --asyncify -o output.wasm
$ wasm2wat output.wasm > output.wat
$ node
> const file = fs.readFileSync('./output.wasm');
> let wasm = await WebAssembly.instantiate(file, { "spectest": { "print": console.log } });
1
2
3

The issue seems similar to #4484 but not directly the same since that one seems to be to do with the emcc toolchain. Also, the blog is old and I would have just put it down to bitrot if it were not for the fact that there are unit tests in Binaryen (e.g. asyncify-pure.wat) that seem to have the same issues. So maybe I'm just missing something?

Update: if you optimize in a second pass after asyncify, that appears to be working:

$ wat2wasm input.wat > input.wasm
$ wasm-opt input.wasm --asyncify -o output.wasm
$ wasm-opt output.wasm -Os -o optimized.wasm
$ node
> const file = fs.readFileSync('./optrimized.wasm');
> let wasm = await WebAssembly.instantiate(file, { "spectest": { "print": console.log } });
1
2
3
kripken commented 2 years ago

There are known incompatibilities between wat2wasm and wasm2wat from the wabt project, and wasm-opt from this project. The main one is support for the "stacky" form of wat, which wabt supports but not binaryen. Looks like you found an ordering issue with imports that I wasn't aware of - not sure offhand if that's a bug and if so, in which project. In any case, you can use wasm-as and wasm-dis from this project if you want something that definitely works with wasm-opt.

The other issue is, I think, the same as #4484: optimizing with asyncify has the risk of inlining making us alter the "runtime" code. Running asyncify without optimizations is one way to avoid that, as you found. We should add a "no-inline" flag as mentioned in #4484 as a better solution, probably.

dsyer commented 2 years ago

Great, so wasm-as works without the 2-phase optimization, but not with the same input.wat as wat2wasm. It must be that wat2wasm generates different bytecode that is more amenable to being inlined. Also the problem with wasm-opt barfing on input.wat also goes away if you use the input that works with wasm-as. So wasm-opt does actually support .wat input - it just doesn't give you very useful error messages if you get the order of the imports and global declarations "wrong". Finally, wasm-shell doesn't work with the output of wasm2wat (that's probably the "stacky" thing you mentioned), but it also doesn't really tell you what went wrong. FWIW if you use wasm-dis instead of wasm2wat it does work at the end with wasm-shell, even with the wabt-generated output.wasm.

kripken commented 2 years ago

Yes, and sorry about the error messages not being great. @tlively is working on a new parser actually, which will support the full text format, and it will also have better errors.