tanishiking / scala-wasm

Experimental WasmGC backend for Scala.js | merging into the upstream Scala.js repo https://github.com/scala-js/scala-js/pull/4988
41 stars 3 forks source link

wasm-opt fails to run on our binary #136

Open tanishiking opened 6 months ago

tanishiking commented 6 months ago
$ bin/wasm-opt --version
wasm-opt version 117 (version_117-203-g5999c996c)

wasm-opt build on 2024-05-24 https://github.com/WebAssembly/binaryen/commit/5999c996c36abeba912599b5fba83d0b2989ed4c

$ bin/wasm-opt sample/target/scala-2.12/sample-fastopt/main.wasm
[parse exception: bad section size, started at 7633 plus payload 61 not being equal to new position 7663 (at 0:7663)]
Fatal: error parsing wasm (try --debug for more info)
$ wasm-objdump -h sample/target/scala-2.12/sample-fastopt/main.wasm

main.wasm:      file format wasm 0x1
0000011: error: expected valid field type (got -0x8)
000090f: error: invalid global type: 0xffffffe4
0001738: error: invalid global type: 0xffffffe4
0001e22: error: expected valid local type
00050b6: warning: invalid function index: 186

Sections:

     Type start=0x0000000d end=0x000008d6 (size=0x000008c9) count: 136
   Import start=0x000008db end=0x000015f2 (size=0x00000d17) count: 112
 Function start=0x000015f7 end=0x0000172b (size=0x00000134) count: 186
      Tag start=0x00001730 end=0x00001731 (size=0x00000001) count: 0
   Global start=0x00001736 end=0x00001dbf (size=0x00000689) count: 63
   Export start=0x00001dc4 end=0x00001dc5 (size=0x00000001) count: 0
    Start start=0x00001dca end=0x00001dcc (size=0x00000002) start: 291
     Elem start=0x00001dd1 end=0x00001e0e (size=0x0000003d) count: 1
DataCount start=0x00001e13 end=0x00001e14 (size=0x00000001) count: 1
     Code start=0x00001e19 end=0x000038d5 (size=0x00001abc) count: 186
     Data start=0x000038da end=0x00003f2e (size=0x00000654) count: 1
   Custom start=0x00003f33 end=0x00006116 (size=0x000021e3) "name"
   Custom start=0x0000611b end=0x0000613c (size=0x00000021) "sourceMappingURL"

Since 0x00001dd1 = 7633, it looks like binaryen fails to parse Elem Section (more precisely, binaryen couldn't find the enough content of 61 bytes in the elem section) and failed at the validation https://github.com/WebAssembly/binaryen/blob/5999c996c36abeba912599b5fba83d0b2989ed4c/src/wasm/wasm-binary.cpp#L1829-L1834

$ wasm-objdump -s --section=elem sample/target/scala-2.12/sample-fastopt/main.wasm

...

Contents of section Elem:
0001dd1: 0107 7011 d270 0bd2 710b d273 0bd2 740b  ..p..p..q..s..t.
0001de1: d275 0bd2 7a0b d27b 0bd2 7c0b d27d 0bd2  .u..z..{..|..}..
0001df1: 7e0b d27f 0bd2 8001 0bd2 8101 0bd2 8201  ~...............
0001e01: 0bd2 8701 0bd2 8901 0bd2 a202 0b         .............

It seems binaryen stopped reading at

0001dd1: 0107 7011 d270 0bd2 710b d273 0bd2 740b  ..p..p..q..s..t.
0001de1: d275 0bd2 7a0b d27b 0bd2 7c0b d27d

?

tanishiking commented 6 months ago

https://webassembly.github.io/spec/core/binary/modules.html#element-section

$ bin/wasm-opt --debug sample/target/scala-2.12/sample-fastopt/main.wasm
...
== readElementSegments
<==
getInt8: 1 (at 7633) # number of elem
getU32LEB: 1 ==>
<==
getInt8: 7 (at 7634) # elem flag (declarative)
getU32LEB: 7 ==>
<==
getInt8: 112 (at 7635) # reftype
getU32LEB: 112 ==>
<==
getInt8: 17 (at 7636) # length of expr
getU32LEB: 17 ==>
<==
getInt8: 210 (at 7637) # ref.func
getInt8: 112 (at 7638) # funcidx
getU32LEB: 14418 ==>
<==
getInt8: 11 (at 7639) # 0x0B why ???
getU32LEB: 11 ==>
<==
getInt8: 210 (at 7640)
getInt8: 113 (at 7641)
getU32LEB: 14546 ==>
<==
getInt8: 11 (at 7642)
getU32LEB: 11 ==>
<==
getInt8: 210 (at 7643)
getInt8: 115 (at 7644)
getU32LEB: 14802 ==>
<==
getInt8: 11 (at 7645)
getU32LEB: 11 ==>
<==
getInt8: 210 (at 7646)
getInt8: 116 (at 7647)
getU32LEB: 14930 ==>
<==
getInt8: 11 (at 7648)
getU32LEB: 11 ==>
<==
getInt8: 210 (at 7649)
getInt8: 117 (at 7650)
getU32LEB: 15058 ==>
<==
getInt8: 11 (at 7651)
getU32LEB: 11 ==>
<==
getInt8: 210 (at 7652)
getInt8: 122 (at 7653)
getU32LEB: 15698 ==>
<==
getInt8: 11 (at 7654)
getU32LEB: 11 ==>
<==
getInt8: 210 (at 7655)
getInt8: 123 (at 7656)
getU32LEB: 15826 ==>
<==
getInt8: 11 (at 7657)
getU32LEB: 11 ==>
<==
getInt8: 210 (at 7658)
getInt8: 124 (at 7659)
getU32LEB: 15954 ==>
<==
getInt8: 11 (at 7660)
getU32LEB: 11 ==>
<==
getInt8: 210 (at 7661)
getInt8: 125 (at 7662)
getU32LEB: 16082 ==>
[parse exception: bad section size, started at 7633 plus payload 61 not being equal to new position 7663 (at 0:7663)]
Assertion failed: (*this && "Cannot print an empty name"), function print, file name.cpp, line 44.
zsh: abort      /Users/tanishiking/ghq/github.com/WebAssembly/binaryen/bin/wasm-opt --debug

11 (0x0B) is the opcode for end, which should appear only once in the end of the expr, but it seems like we insert 0x0B for each instr?

Expressions are encoded by their instruction sequence terminated with an explicit https://webassembly.github.io/spec/core/binary/modules.html#element-section

It seems like we insert the

tanishiking commented 6 months ago

So, we should use writeInstr here instead

https://github.com/tanishiking/scala-wasm/blob/fb3bcb2aa731516c52ec9ae2bc1a95cd1f9a24e6/wasm/src/main/scala/org/scalajs/linker/backend/webassembly/BinaryWriter.scala#L231-L233

tanishiking commented 6 months ago

Hmm, not actually, we follow the spec right, and binaryen parser seems to be a culprit 🤔

Screenshot 2024-05-21 at 16 51 33

https://webassembly.github.io/spec/core/binary/modules.html#element-section

sjrd commented 6 months ago

I wouldn't be surprised if binaryen is confused about things only found in GC-heavy usage of Wasm. It was designed for users of the linear memory model first.

tanishiking commented 6 months ago

Could fix the issue with the following diff,

diff --git a/src/wasm/wasm-binary.cpp b/src/wasm/wasm-binary.cpp
index ec06692a4..2aa5e7c11 100644
--- a/src/wasm/wasm-binary.cpp
+++ b/src/wasm/wasm-binary.cpp
@@ -3354,7 +3354,7 @@ void WasmBinaryReader::readElementSegments() {
       [[maybe_unused]] auto type = getU32LEB();
       auto num = getU32LEB();
       for (Index i = 0; i < num; i++) {
-        getU32LEB();
+        readExpression();
       }
       continue;
     }

but we hit the different problem anyway 😄

$ bin/wasm-opt sample/target/scala-2.12/sample-fastopt/main.wasm
[parse exception: control flow inputs are not supported yet (at 0:8437)]
Fatal: error parsing wasm (try --debug for more info)

https://github.com/WebAssembly/binaryen/blob/5999c996c36abeba912599b5fba83d0b2989ed4c/src/wasm/wasm-binary.cpp#L2113-L2115

probably this https://github.com/WebAssembly/binaryen/issues/6407


I'll send an above patch to binaryen, and call it a day for now