update to wasmtime 3.0.0 #65

JakeChampion · 2023-01-04T17:37:23Z

wasmtime 0.38.0 removed it's module linking implementation to make room for the upcoming support for the component model. bytecodealliance/wasmtime#3958

This PR removes all the module linking functionality from wizer and updates to wasmtime 0.38.0

This PR also changes from using Trap::new to using the anyhow! macro, Trap::new was removed in wasmtime 3.0.0 bytecodealliance/wasmtime#5149

I've not worked much (if at all) in the wasmtime or wizer codebases, I may have done many things which are not correct here, hopefully it's along the right tracks 🤞

fitzgen

LGTM, thanks! Just one test that was deleted that shouldn't be.

fitzgen · 2023-01-05T20:44:02Z

tests/tests.rs

-#[test]
-fn multi_memory() -> Result<()> {
-    run_wat(
-        &[],
-        42,
-        r#"
-(module
-  (memory $m1 1)
-  (memory $m2 1)
-  (func (export "wizer.initialize")
-    i32.const 0
-    i32.const 41
-    i32.store (memory $m1) offset=1337
-    i32.const 0
-    i32.const 1
-    i32.store (memory $m2) offset=1337)
-  (func (export "run") (result i32)
-    i32.const 0
-    i32.load (memory $m1) offset=1337
-    i32.const 0
-    i32.load (memory $m2) offset=1337
-    i32.add))
-"#,
-    )
-}


This test shouldn't be deleted, we should still support multi-memory Wasm.

I've re-added it now, the test fails but I unfortunately don't know how to make it pass.

failures: ---- multi_memory stdout ---- Error: unknown operator or unexpected token --> <anon>:8:16 | 8 | i32.store (memory $m1) offset=1337 | ^ failures: multi_memory

It looks like the WAT syntax for multi-memory has changed so that should just be

i32.store $m1 offset=1337

now.

Thanks, I made that change to both the store calls and both the load calls however I believe it fails to generate valid wasm. The error that is generated is

Error: unknown memory 1337 (at offset 75)

This error comes from store call for the $m2 memory.

If I change the calls to use different offsets from each other then the test passes but I'm not sure if that is just avoiding a potential bug that I've introduced within this PR by updating wasmtime.

If useful, this is the wat which fails to validate:

(module (memory $m1 1) (memory $m2 1) (func (export "wizer.initialize") i32.const 0 i32.const 41 i32.store $m1 offset=1337 i32.const 0 i32.const 1 i32.store $m2 offset=1337) (func (export "run") (result i32) i32.const 0 i32.load $m1 offset=1337 i32.const 0 i32.load $m2 offset=1337 i32.add))

And this is the wat which passes the test:

(module (memory $m1 1) (memory $m2 1) (func (export "wizer.initialize") i32.const 0 i32.const 41 i32.store $m1 offset=1337 i32.const 0 i32.const 1 i32.store $m2 offset=1) (func (export "run") (result i32) i32.const 0 i32.load $m1 offset=1337 i32.const 0 i32.load $m2 offset=1 i32.add))

Huh. That looks correct by my reading of the parsing code but it looks like it is trying to treat the offset (static index into the memory) as a memory index (thing that decides which memory we are accessing).

Actually IDs should work too.

In fact, if I copy/paste the failing example and run our validator on it, it succeeds:

$ cat ~/scratch/multi-memory.wat (module (memory $m1 1) (memory $m2 1) (func (export "wizer.initialize") i32.const 0 i32.const 41 i32.store $m1 offset=1337 i32.const 0 i32.const 1 i32.store $m2 offset=1337) (func (export "run") (result i32) i32.const 0 i32.load $m1 offset=1337 i32.const 0 i32.load $m2 offset=1337 i32.add)) $ wasm-tools validate --features multi-memory ~/scratch/multi-memory.wat $ echo $? 0

Are you sure that you tested that exact version of the WAT and not accidentally some in between version while editing and poking at the test case?

I tested that exact version, yep, I've pushed it to GH now, here is the failing CI logs -- https://github.com/bytecodealliance/wizer/actions/runs/3876213375/jobs/6609775823

To get more information from that failing test, I ran this locally:
❯ RUST_LOG=debug cargo test --package wizer --test tests -- multi_memory --exact --nocapture
Which then output the message:

Error: unknown memory 1337 (at offset 75)

I'm not sure why it is failing though

Hmm maybe wasm-tools have changed since wasmtime 3.0 and 4.0? If you rebase your 4.0 work on the latest version of this branch, do you get the same failure?

I rebased on wasmtime 4 branch and the same test failure happens.

I went to see what the prewizened wasm looked like after parsing the wat 56bb567 (#65) , and this is what it is:

=== PreWizened Wasm ========================================================== (module (type (;0;) (func)) (type (;1;) (func (result i32))) (func (;0;) (type 0) i32.const 0 i32.const 41 i32.store offset=1337 i32.const 0 i32.const 1 i32.store (memory 1337) offset=1) (func (;1;) (type 1) (result i32) i32.const 0 i32.load offset=1337 i32.const 0 i32.load (memory 1337) offset=1 i32.add) (memory $m1 1) (memory $m2 1) (export "wizer.initialize" (func 0)) (export "run" (func 1))) ===========================================================================

tests/tests.rs

alexcrichton · 2023-04-14T22:27:45Z

I've sent #75 which is a PR to this PR which I think should get the tests working.

wasmtime 0.36.0 removed it's module linking implementation to make room for the upcoming support for the component model. bytecodealliance/wasmtime#3958 This commit removes all the module linking functionality from wizer and updates to wasmtime 0.38.0

this update required changes from the removed `Trap::new` to anyhow! macro, Trap::new was removed in bytecodealliance/wasmtime#5149

Bring these up-to-date with the latest.

These dependencies are far behind the latest versions so this fast-forwards them up to versions where larger changes are required. This updates them enough to perform some minor rewrites here and there but nothing major is changing.

This involved deleting quite a bit more module-linking related code in `wizer`.

…mory This commit further updates the `wasmparser` dependency across some versions with minor renamings and structuring of the API. This pulls in enough support to get the `multi_memory` test passing.

JakeChampion requested a review from fitzgen January 4, 2023 17:37

JakeChampion changed the title ~~update to wasmtime 0.38.0~~ update to wasmtime 3.0.0 Jan 4, 2023

JakeChampion force-pushed the jake/wasmtime branch 3 times, most recently from 62af22b to 40d59ea Compare January 4, 2023 18:04

fitzgen approved these changes Jan 5, 2023

View reviewed changes

JakeChampion commented Jan 6, 2023

View reviewed changes

tests/tests.rs Outdated Show resolved Hide resolved

JakeChampion force-pushed the jake/wasmtime branch 3 times, most recently from 9e02d39 to 52c4070 Compare January 9, 2023 01:42

alexcrichton mentioned this pull request Apr 14, 2023

Update wasmparser on the 3.0.0 update branch #75

Merged

Jake Champion and others added 9 commits April 15, 2023 01:01

update to wasmtime 0.38.0

8bdd1ad

wasmtime 0.36.0 removed it's module linking implementation to make room for the upcoming support for the component model. bytecodealliance/wasmtime#3958 This commit removes all the module linking functionality from wizer and updates to wasmtime 0.38.0

update to wasmtime 3.0.0

4caa57d

this update required changes from the removed `Trap::new` to anyhow! macro, Trap::new was removed in bytecodealliance/wasmtime#5149

Update tests.rs

05b7a18

print out the pre-wizened wasm when debugging

ed4426a

Update wat/wasmprinter deps

309df09

Bring these up-to-date with the latest.

Update wasmparser/wasm-encoder deps incrementally

2e71749

These dependencies are far behind the latest versions so this fast-forwards them up to versions where larger changes are required. This updates them enough to perform some minor rewrites here and there but nothing major is changing.

Update to wasmparser without module linking

2b227b6

This involved deleting quite a bit more module-linking related code in `wizer`.

Update wasmparser to a version with the latest encoding of multi-me…

509dd59

…mory This commit further updates the `wasmparser` dependency across some versions with minor renamings and structuring of the API. This pulls in enough support to get the `multi_memory` test passing.

Remove module linking support from fuzzer

f2f15f3

JakeChampion force-pushed the jake/wasmtime branch from 9f527bf to f2f15f3 Compare April 15, 2023 00:02

JakeChampion merged commit 8425b0b into main Apr 15, 2023

JakeChampion deleted the jake/wasmtime branch April 15, 2023 00:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update to wasmtime 3.0.0 #65

update to wasmtime 3.0.0 #65

JakeChampion commented Jan 4, 2023 •

edited

Loading

fitzgen left a comment

fitzgen Jan 5, 2023

JakeChampion Jan 6, 2023

fitzgen Jan 6, 2023

JakeChampion Jan 9, 2023

fitzgen Jan 9, 2023

fitzgen Jan 9, 2023

fitzgen Jan 9, 2023

JakeChampion Jan 9, 2023

fitzgen Jan 9, 2023

JakeChampion Jan 10, 2023 •

edited

Loading

alexcrichton commented Apr 14, 2023

update to wasmtime 3.0.0 #65

update to wasmtime 3.0.0 #65

Conversation

JakeChampion commented Jan 4, 2023 • edited Loading

fitzgen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JakeChampion Jan 10, 2023 • edited Loading

Choose a reason for hiding this comment

alexcrichton commented Apr 14, 2023

JakeChampion commented Jan 4, 2023 •

edited

Loading

JakeChampion Jan 10, 2023 •

edited

Loading