Skip to content
This repository was archived by the owner on Jul 12, 2024. It is now read-only.

wasm-opt fails to run on our binary #136

Open
tanishiking opened this issue May 21, 2024 · 5 comments
Open

wasm-opt fails to run on our binary #136

tanishiking opened this issue May 21, 2024 · 5 comments

Comments

@tanishiking
Copy link
Owner

tanishiking commented May 21, 2024

$ bin/wasm-opt --version
wasm-opt version 117 (version_117-203-g5999c996c)

wasm-opt build on 2024-05-24 WebAssembly/binaryen@5999c99

$ 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
Copy link
Owner Author

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
Copy link
Owner Author

So, we should use writeInstr here instead

buf.vec(element.init) { expr =>
writeExpr(buf, expr)
}

@tanishiking
Copy link
Owner Author

tanishiking commented May 21, 2024

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
Copy link
Collaborator

sjrd commented May 21, 2024

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
Copy link
Owner Author

tanishiking commented May 21, 2024

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 WebAssembly/binaryen#6407


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

tanishiking added a commit to tanishiking/binaryen that referenced this issue May 21, 2024
The parser was incorrectly handling the parsing of declarative element segments (those with the `elemkind` of `declarative`).
According to the spec, the `init` of the declarative element should be a `vec(expr)`.
https://webassembly.github.io/spec/core/binary/modules.html#element-section
However, binaryen was simple reading a sing `u32` value for each
expression instead of parsing a expression.

This commit updates the `WasmBinaryReader::readElementSegments` function
to correctly parse the expressions for declarative element segments by
calling `readExpression` instead of `getU32LEB`.

Resolves the parsing exception:
"[parse exception: bad section size, started at ... not being equal to new position ...]"

Related discussion: tanishiking/scala-wasm#136
@tanishiking tanishiking changed the title wasm-opt fails to run on binary wasm-opt fails to run on our binary May 21, 2024
tanishiking added a commit to tanishiking/binaryen that referenced this issue May 23, 2024
The parser was incorrectly handling the parsing of declarative element segments (those with the `elemkind` of `declarative`).
According to the spec, the `init` of the declarative element should be a `vec(expr)`.
https://webassembly.github.io/spec/core/binary/modules.html#element-section
However, binaryen was simple reading a sing `u32` value for each
expression instead of parsing a expression.

This commit updates the `WasmBinaryReader::readElementSegments` function
to correctly parse the expressions for declarative element segments by
calling `readExpression` instead of `getU32LEB`.

Resolves the parsing exception:
"[parse exception: bad section size, started at ... not being equal to new position ...]"

Related discussion: tanishiking/scala-wasm#136
tanishiking added a commit to tanishiking/binaryen that referenced this issue May 23, 2024
The parser was incorrectly handling the parsing of declarative element segments (those with the `elemkind` of `declarative`).
According to the spec, the `init` of the declarative element should be a `vec(expr)` if the prefix is 7.
https://webassembly.github.io/spec/core/binary/modules.html#element-section
However, binaryen was simply reading a single `u32LEB` value for each
expression instead of parsing a expression regardless `usesExpressions = true`.

This commit updates the `WasmBinaryReader::readElementSegments` function
to correctly parse the expressions for declarative element segments by
calling `readExpression` instead of `getU32LEB` if `usesExpressions = true`.

Resolves the parsing exception:
"[parse exception: bad section size, started at ... not being equal to new position ...]"

Related discussion: tanishiking/scala-wasm#136
tanishiking added a commit to tanishiking/binaryen that referenced this issue May 31, 2024
The parser was incorrectly handling the parsing of declarative element segments whose `init` is a `vec(expr)`.
https://webassembly.github.io/spec/core/binary/modules.html#element-section
Binry parser was simply reading a single `u32LEB` value for `init`
instead of parsing a expression regardless `usesExpressions = true`.

This commit updates the `WasmBinaryReader::readElementSegments` function
to correctly parse the expressions for declarative element segments by
calling `readExpression` instead of `getU32LEB` when `usesExpressions = true`.

Resolves the parsing exception:
"[parse exception: bad section size, started at ... not being equal to new position ...]"

Related discussion: tanishiking/scala-wasm#136
tlively pushed a commit to WebAssembly/binaryen that referenced this issue Jun 3, 2024
The parser was incorrectly handling the parsing of declarative element segments whose `init` is a `vec(expr)`.
https://webassembly.github.io/spec/core/binary/modules.html#element-section
Binry parser was simply reading a single `u32LEB` value for `init`
instead of parsing a expression regardless `usesExpressions = true`.

This commit updates the `WasmBinaryReader::readElementSegments` function
to correctly parse the expressions for declarative element segments by
calling `readExpression` instead of `getU32LEB` when `usesExpressions = true`.

Resolves the parsing exception:
"[parse exception: bad section size, started at ... not being equal to new position ...]"

Related discussion: tanishiking/scala-wasm#136
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants