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

[wasi-common]: Lack of virtual fs support in snapshot0 #1252

Open
kubkon opened this issue Mar 7, 2020 · 7 comments
Open

[wasi-common]: Lack of virtual fs support in snapshot0 #1252

kubkon opened this issue Mar 7, 2020 · 7 comments
Labels
wasi:impl Issues pertaining to WASI implementation in Wasmtime wasi:tests Issues pertaining to WASI tests in Wasmtime

Comments

@kubkon
Copy link
Member

kubkon commented Mar 7, 2020

Apologies that this didn't click earlier, but is it intentional that we do not support virtual fs in snapshot0 aka wasi_unstable? #701 landed (hooray!) but after careful re-examination I've noticed it never touched any of the old snapshot code. It's not a deal breaker per se unless we perceive anybody in the future wanting to use virtual fs with the old WASI ABI.

The thing that really worries me though is that I've noticed both snapshots (wasi_unstable and wasi_snapshot_preview1) really diverge and in places where they supposedly shouldn't. Virtual fs is one of those places (but there are more!). My question here is, given that wiggle is hopefully round the corner (I plan to have a fully working port in the coming week), @pchickey and I can start figuring out a way of polyfilling syscall logic to different ABI snapshots which would make the out-of-sync issue essentially disappear (to a degree ofc!).

Also, I remember having a discussion about this with @alexcrichton sometime in the past. I think we ought to really figure out a way of testing all supported by Wasmtime WASI snapshots to avoid this type of errors in the future.

Oh man, what started as a simple question, ended up as an essay. Apologies!

cc @iximeow

@kubkon kubkon added wasi:impl Issues pertaining to WASI implementation in Wasmtime wasi:tests Issues pertaining to WASI tests in Wasmtime labels Mar 7, 2020
@kubkon
Copy link
Member Author

kubkon commented Mar 7, 2020

Also, while we're here. @sunfishcode @alexcrichton @pchickey do you guys think we should introduce wiggle in both snapshots, or target only the current one (wasi_snapshot_preview1), and after we figure out the polyfill mechanics, backport to wasi_unstable?

@iximeow
Copy link
Contributor

iximeow commented Mar 9, 2020

On the topic of virtual fs in in snapshot0 specifically.. I simply didn't think about it. I can't imagine a reason snapshot0 should not be able to use snapshot0. @pchickey mentioned that lucet-wasi has been on snapshot0 for a bit while wiggle has gotten built out, but at one point while I was building it out I definitely had it using virtual files, so I'm highly suspicious of my understanding of.. something, somewhere.

@kubkon
Copy link
Member Author

kubkon commented Mar 9, 2020

Right, OK. I thought I'd double check with you before doing any further refactoring etc. So now the real question is whether we want to backport its functionality as-is to snapshot0 (and this involves a lot of nasty copy-pasting here and there), or if we can wait a little bit for wiggle to make entrance, figure out the polyfill mechanism between different snapshots, and then backporting is given to us for free. Thoughts? :-)

@pchickey
Copy link
Contributor

pchickey commented Mar 9, 2020

The path I was thinking we'd follow here is:

  1. Land wiggle support in snapshot 1 only. Keep the snapshot 0 tree intact, including using the old wig tool. Since its entirely different code, this should be pretty easy, right?

  2. Teach wiggle about the witx polyfill feature for calculating type & func call equivalency. For polyfilled modules, let wiggle_generate spit out a mod snapshot_0 { mod types { <ts> } <funcs> } where we fill in equivalent types with aliases (e.g. type Foo = super::super::snapshot_1::types::Foo), non-equivalent types with their definition. Fill in the equivalent function calls with the same code we generate in snapshot 1 - validate the inputs, and call the snapshot 1 trait method. The non-equivalent function calls will call a snapshot 0 trait method. The user is responsible for implementing these non-automatic methods in the snapshot 0 trait just like they are the snapshot 1 trait.

  3. Land that wiggle feature in wasi-common, deleting snapshot 0 subtree and wig.

@kubkon
Copy link
Member Author

kubkon commented Mar 9, 2020

This plan sounds good to me! @sunfishcode I'd love to hear your thoughts on this as well!

@sunfishcode
Copy link
Member

That sounds reasonable to me! And of course, it continues to make sense to factor out code into yanix/winx and into modules which aren't per-snapshot (currently we just have sandboxed_tty_writer) so that for the cases where we do have to use manual code, we can share as much as possible.

@kubkon
Copy link
Member Author

kubkon commented Mar 9, 2020

Just an FYI that after #1253 lands, I'll restart my efforts at porting wiggle into wasi-common. If everything goes well, I'm really hopeful to have it fully working by the end of this week at the latest. This would mean we could start looking at polyfills soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasi:impl Issues pertaining to WASI implementation in Wasmtime wasi:tests Issues pertaining to WASI tests in Wasmtime
Projects
None yet
Development

No branches or pull requests

4 participants