Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: upstream wasi virt walrus ops #248

Merged

Conversation

vados-cosmonic
Copy link
Contributor

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

Copy link
Collaborator

@guybedford guybedford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a great start, these seem like useful conveniences to me.

src/module/functions/mod.rs Outdated Show resolved Hide resolved
src/module/functions/mod.rs Outdated Show resolved Hide resolved
src/module/functions/mod.rs Outdated Show resolved Hide resolved
@vados-cosmonic vados-cosmonic force-pushed the feat/upstream-wasi-virt-walrus-ops branch 2 times, most recently from 1bc49ea to db6c171 Compare October 11, 2023 03:02
Copy link
Collaborator

@guybedford guybedford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @vados-cosmonic, this is mostly seeming pretty good to me.

src/module/exports.rs Outdated Show resolved Hide resolved
src/module/imports.rs Outdated Show resolved Hide resolved
src/module/imports.rs Outdated Show resolved Hide resolved
src/module/imports.rs Outdated Show resolved Hide resolved
src/module/memories.rs Show resolved Hide resolved
@vados-cosmonic vados-cosmonic marked this pull request as ready for review October 12, 2023 14:08
@vados-cosmonic
Copy link
Contributor Author

vados-cosmonic commented Oct 12, 2023

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 vados-cosmonic force-pushed the feat/upstream-wasi-virt-walrus-ops branch from 74966a4 to fe21e67 Compare October 12, 2023 14:09
@vados-cosmonic vados-cosmonic force-pushed the feat/upstream-wasi-virt-walrus-ops branch 2 times, most recently from 563e8d9 to ce36cd7 Compare October 13, 2023 15:55
@vados-cosmonic
Copy link
Contributor Author

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

Copy link
Collaborator

@guybedford guybedford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thank you. Just a note re naming and I think we should keep stubbing to being a user side implementation detail.

src/module/functions/mod.rs Outdated Show resolved Hide resolved
src/module/functions/mod.rs Outdated Show resolved Hide resolved
src/module/functions/mod.rs Outdated Show resolved Hide resolved
@vados-cosmonic vados-cosmonic force-pushed the feat/upstream-wasi-virt-walrus-ops branch from cbcc22d to ad31f36 Compare October 13, 2023 18:46
@vados-cosmonic
Copy link
Contributor Author

vados-cosmonic commented Oct 13, 2023

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).

WASI-virt contains functions that are helpful for manipulating modules
and dealing with exports/imports, which would be helpful to an even
wider group if upstreamed here to walrus.

This commit copies and upstreams some operations that were introduced
in WASI-virt for wider use via walrus.

See also: bytecodealliance/WASI-Virt#20

Signed-off-by: Victor Adossi <[email protected]>
@vados-cosmonic vados-cosmonic force-pushed the feat/upstream-wasi-virt-walrus-ops branch from ad31f36 to 4331cb2 Compare October 13, 2023 18:56
@guybedford
Copy link
Collaborator

@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 vados-cosmonic force-pushed the feat/upstream-wasi-virt-walrus-ops branch from 1bd3c52 to 45cbc48 Compare October 14, 2023 05:45
@vados-cosmonic
Copy link
Contributor Author

vados-cosmonic commented Oct 14, 2023

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

  • get_local (that's now local.get, right?)
  • s-expressions in WAT being invalid (maybe the syntax without making the body a proper s-expression was/is supported then/now, but the easy fix was to add some parens)
  • atomic.notify becoming memory.atomic.notify

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.

@vados-cosmonic vados-cosmonic force-pushed the feat/upstream-wasi-virt-walrus-ops branch from ccef318 to 47e193e Compare October 14, 2023 06:08
The excellent pre-existing test suite contains a lot of WAT files as
test-cases that seem to have older syntax in them.

The breakage is introduced because the current test suite runs stable,
latest and nightly versions of Rust while conducting tests. While the
rest of the Wasm toolchain (binaryen, wabt, etc) are hard-coded to versions that should
stay compatible with the WAT as they were written, the upstreaming of
various changes to Rust itself seems to be causing failures when using
the existing WAT for tests.

This PR updates the WAT (mostly operation renaming) that have changed
in order to get tests running again.

Signed-off-by: Victor Adossi <[email protected]>
@vados-cosmonic vados-cosmonic force-pushed the feat/upstream-wasi-virt-walrus-ops branch from 4cc888f to 7af98d7 Compare October 14, 2023 06:38
@guybedford
Copy link
Collaborator

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!

@guybedford guybedford merged commit 632d220 into rustwasm:main Oct 14, 2023
8 checks passed
@vados-cosmonic vados-cosmonic deleted the feat/upstream-wasi-virt-walrus-ops branch October 14, 2023 21:14
@vados-cosmonic
Copy link
Contributor Author

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants