rustwasm / walrus

Walrus is a WebAssembly transformation library 🌊🐘
https://docs.rs/walrus
Apache License 2.0
385 stars 62 forks source link

feat: upstream wasi virt walrus ops #248

Closed vados-cosmonic closed 11 months ago

vados-cosmonic commented 11 months ago

This PR upstreams a few functions from WASI-Virt (see previous discussion) that might be helpful to the wider walrus user ecosystem.

vados-cosmonic commented 11 months ago

Thanks for all the work reviewing this, improving the function/usage surface here has certainly taken more work than I expected.

I've just rebased & squashed the changes down to one commit

vados-cosmonic commented 11 months ago

Hey @guybedford I've done a little bit more on this -- split up the functions and added some basic tests, if you wouldn't mind taking another look

vados-cosmonic commented 11 months ago

Addressed all comments and squashed + rebased! Thanks for the reviews -- I think you should be good to merge at will

[EDIT] - One more thing, removed the Clone additions -- they're not necessary for this PR and someone else with a legitimate need should add them (I realized that I needed to be moveing the FunctionBuilders into the closure rather than trying to copy the types, since FunctionBuilder uses that &mut ModuleTypes).

guybedford commented 11 months ago

@vados-cosmonic this looks great to merge to me as well. I'm not sure what is up with the recent CI break, given nothing has changed on that side. If you have any ideas do let me know, I think we should ensure CI is running before landing.

vados-cosmonic commented 11 months ago

Hey @guybedford I think I found the problem -- the WAT files used in the tests seem to be outdated in a few ways:

For reference, the errors in CI look like this:

running 5 tests
test tests_invalid_multi_value_0_wat ... FAILED
test tests_invalid_multi_value_1_wat ... FAILED
test tests_invalid_if_with_no_else_wat ... ok
test tests_invalid_multi_value_2_wat ... ok
test tests_invalid_multi_value_3_wat ... ok

failures:

---- tests_invalid_multi_value_0_wat stdout ----
thread 'tests_invalid_multi_value_0_wat' panicked at crates/tests-utils/src/lib.rs:107:23:
got an error: unknown operator or unexpected token
     --> tests/invalid/multi-value-0.wat:5:5
      |
    5 |     get_local 0))
      |     ^
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- tests_invalid_multi_value_1_wat stdout ----
thread 'tests_invalid_multi_value_1_wat' panicked at crates/tests-utils/src/lib.rs:107:23:
got an error: unknown operator or unexpected token
     --> tests/invalid/multi-value-1.wat:4:5
      |
    4 |     get_local 0
      |     ^

failures:
    tests_invalid_multi_value_0_wat
    tests_invalid_multi_value_1_wat

It seems to be simply a WAT format/issue... I'm not sure how this could have possibly been in here so long without causing tests to fail..

The failing tests were written in 2019, so I'm hoping it's actually this simple of a fix.

I'm going around and updating the WAT as best I can, will collect them into one separate commit so they're easy to review!

[EDIT] I was wondering to myself how this stuff could be changing since the GHA config was using hard coded versions of the toolchain (binaryen, wabt, etc)... I think this is actually due to changes in Rust underneath as stuff is getting upstreamed -- different colloquial release names (stable/beta/nightly) are used in the GHA config, so these tests are definitely becoming a moving target.

[EDIT2] I'm probably going to move these changes to their own PR. It remains to be seen whether we should hard-code the rust versions in the matrix (since the current way means that tests will break randomly)... but at the very least, these WAT changes should be kept separate.

guybedford commented 11 months ago

I'm going to merge this, but before we release I would still like to see how the Wasi-VIRT rebase looks like on top of this work to see if there's any final API feedback we need to make in follow-up PRs.

Thanks for getting this over the line!

vados-cosmonic commented 11 months ago

Hey @guybedford sure -- I'll have a PR up to WASI-Virt and try and use these new functions so we have an idea of how different it will look!