Stream: git-wasmtime

Topic: wasmtime / PR #1202 It's wiggle time!

Wasmtime GitHub notifications bot (Mar 11 2020 at 18:26):

kubkon edited PR #1202 from wiggle-time to master:

It's [wiggle] time!

This PR is the beginning of a journey which aims at replacing wig crate with wiggle. @pchickey and myself have been working on wiggle for over a month now, and we feel we got to the point where it could successfully be used in wasi-common, wasmtime and lucet. We also agreed that if any functionality is still missing, we should iterate in-tree if possible. Anyhow, this is a draft PR so things will invariably change (a lot!), but I wanted to get it out there sooner rather than later so that I can get some meaningful feedback early on and work on addressing that when things are still plastic.

[wiggle]: https://github.com/kubkon/wiggle

Wasmtime GitHub notifications bot (Mar 12 2020 at 15:22):

kubkon updated PR #1202 from wiggle-time to master:

It's [wiggle] time!

This PR is the beginning of a journey which aims at replacing wig crate with wiggle. @pchickey and myself have been working on wiggle for over a month now, and we feel we got to the point where it could successfully be used in wasi-common, wasmtime and lucet. We also agreed that if any functionality is still missing, we should iterate in-tree if possible. Anyhow, this is a draft PR so things will invariably change (a lot!), but I wanted to get it out there sooner rather than later so that I can get some meaningful feedback early on and work on addressing that when things are still plastic.

[wiggle]: https://github.com/kubkon/wiggle

Wasmtime GitHub notifications bot (Mar 13 2020 at 09:26):

kubkon updated PR #1202 from wiggle-time to master:

It's [wiggle] time!

This PR is the beginning of a journey which aims at replacing wig crate with wiggle. @pchickey and myself have been working on wiggle for over a month now, and we feel we got to the point where it could successfully be used in wasi-common, wasmtime and lucet. We also agreed that if any functionality is still missing, we should iterate in-tree if possible. Anyhow, this is a draft PR so things will invariably change (a lot!), but I wanted to get it out there sooner rather than later so that I can get some meaningful feedback early on and work on addressing that when things are still plastic.

[wiggle]: https://github.com/kubkon/wiggle

Wasmtime GitHub notifications bot (Mar 13 2020 at 09:45):

kubkon updated PR #1202 from wiggle-time to master:

It's [wiggle] time!

This PR is the beginning of a journey which aims at replacing wig crate with wiggle. @pchickey and myself have been working on wiggle for over a month now, and we feel we got to the point where it could successfully be used in wasi-common, wasmtime and lucet. We also agreed that if any functionality is still missing, we should iterate in-tree if possible. Anyhow, this is a draft PR so things will invariably change (a lot!), but I wanted to get it out there sooner rather than later so that I can get some meaningful feedback early on and work on addressing that when things are still plastic.

[wiggle]: https://github.com/kubkon/wiggle

Wasmtime GitHub notifications bot (Mar 13 2020 at 15:18):

kubkon updated PR #1202 from wiggle-time to master:

It's [wiggle] time!

This PR is the beginning of a journey which aims at replacing wig crate with wiggle. @pchickey and myself have been working on wiggle for over a month now, and we feel we got to the point where it could successfully be used in wasi-common, wasmtime and lucet. We also agreed that if any functionality is still missing, we should iterate in-tree if possible. Anyhow, this is a draft PR so things will invariably change (a lot!), but I wanted to get it out there sooner rather than later so that I can get some meaningful feedback early on and work on addressing that when things are still plastic.

[wiggle]: https://github.com/kubkon/wiggle

Wasmtime GitHub notifications bot (Mar 17 2020 at 10:49):

kubkon updated PR #1202 from wiggle-time to master:

It's [wiggle] time!

This PR is the beginning of a journey which aims at replacing wig crate with wiggle. @pchickey and myself have been working on wiggle for over a month now, and we feel we got to the point where it could successfully be used in wasi-common, wasmtime and lucet. We also agreed that if any functionality is still missing, we should iterate in-tree if possible. Anyhow, this is a draft PR so things will invariably change (a lot!), but I wanted to get it out there sooner rather than later so that I can get some meaningful feedback early on and work on addressing that when things are still plastic.

[wiggle]: https://github.com/kubkon/wiggle

Wasmtime GitHub notifications bot (Mar 18 2020 at 12:26):

kubkon updated PR #1202 from wiggle-time to master:

It's [wiggle] time!

This PR is the beginning of a journey which aims at replacing wig crate with wiggle. @pchickey and myself have been working on wiggle for over a month now, and we feel we got to the point where it could successfully be used in wasi-common, wasmtime and lucet. We also agreed that if any functionality is still missing, we should iterate in-tree if possible. Anyhow, this is a draft PR so things will invariably change (a lot!), but I wanted to get it out there sooner rather than later so that I can get some meaningful feedback early on and work on addressing that when things are still plastic.

[wiggle]: https://github.com/kubkon/wiggle

Wasmtime GitHub notifications bot (Mar 18 2020 at 16:01):

kubkon updated PR #1202 from wiggle-time to master:

It's [wiggle] time!

This PR is the beginning of a journey which aims at replacing wig crate with wiggle. @pchickey and myself have been working on wiggle for over a month now, and we feel we got to the point where it could successfully be used in wasi-common, wasmtime and lucet. We also agreed that if any functionality is still missing, we should iterate in-tree if possible. Anyhow, this is a draft PR so things will invariably change (a lot!), but I wanted to get it out there sooner rather than later so that I can get some meaningful feedback early on and work on addressing that when things are still plastic.

[wiggle]: https://github.com/kubkon/wiggle

Wasmtime GitHub notifications bot (Mar 18 2020 at 16:14):

kubkon updated PR #1202 from wiggle-time to master:

It's [wiggle] time!

This PR is the beginning of a journey which aims at replacing wig crate with wiggle. @pchickey and myself have been working on wiggle for over a month now, and we feel we got to the point where it could successfully be used in wasi-common, wasmtime and lucet. We also agreed that if any functionality is still missing, we should iterate in-tree if possible. Anyhow, this is a draft PR so things will invariably change (a lot!), but I wanted to get it out there sooner rather than later so that I can get some meaningful feedback early on and work on addressing that when things are still plastic.

[wiggle]: https://github.com/kubkon/wiggle

Wasmtime GitHub notifications bot (Mar 18 2020 at 20:51):

kubkon updated PR #1202 from wiggle-time to master:

It's [wiggle] time!

This PR is the beginning of a journey which aims at replacing wig crate with wiggle. @pchickey and myself have been working on wiggle for over a month now, and we feel we got to the point where it could successfully be used in wasi-common, wasmtime and lucet. We also agreed that if any functionality is still missing, we should iterate in-tree if possible. Anyhow, this is a draft PR so things will invariably change (a lot!), but I wanted to get it out there sooner rather than later so that I can get some meaningful feedback early on and work on addressing that when things are still plastic.

[wiggle]: https://github.com/kubkon/wiggle

Wasmtime GitHub notifications bot (Mar 18 2020 at 21:08):

kubkon updated PR #1202 from wiggle-time to master:

It's [wiggle] time!

This PR is the beginning of a journey which aims at replacing wig crate with wiggle. @pchickey and myself have been working on wiggle for over a month now, and we feel we got to the point where it could successfully be used in wasi-common, wasmtime and lucet. We also agreed that if any functionality is still missing, we should iterate in-tree if possible. Anyhow, this is a draft PR so things will invariably change (a lot!), but I wanted to get it out there sooner rather than later so that I can get some meaningful feedback early on and work on addressing that when things are still plastic.

[wiggle]: https://github.com/kubkon/wiggle

Wasmtime GitHub notifications bot (Mar 18 2020 at 21:12):

kubkon updated PR #1202 from wiggle-time to master:

It's [wiggle] time!

This PR is the beginning of a journey which aims at replacing wig crate with wiggle. @pchickey and myself have been working on wiggle for over a month now, and we feel we got to the point where it could successfully be used in wasi-common, wasmtime and lucet. We also agreed that if any functionality is still missing, we should iterate in-tree if possible. Anyhow, this is a draft PR so things will invariably change (a lot!), but I wanted to get it out there sooner rather than later so that I can get some meaningful feedback early on and work on addressing that when things are still plastic.

[wiggle]: https://github.com/kubkon/wiggle

Wasmtime GitHub notifications bot (Mar 18 2020 at 22:54):

kubkon updated PR #1202 from wiggle-time to master:

It's [wiggle] time!

This PR is the beginning of a journey which aims at replacing wig crate with wiggle. @pchickey and myself have been working on wiggle for over a month now, and we feel we got to the point where it could successfully be used in wasi-common, wasmtime and lucet. We also agreed that if any functionality is still missing, we should iterate in-tree if possible. Anyhow, this is a draft PR so things will invariably change (a lot!), but I wanted to get it out there sooner rather than later so that I can get some meaningful feedback early on and work on addressing that when things are still plastic.

[wiggle]: https://github.com/kubkon/wiggle

Wasmtime GitHub notifications bot (Mar 19 2020 at 09:12):

kubkon updated PR #1202 from wiggle-time to master:

It's [wiggle] time!

This PR is the beginning of a journey which aims at replacing wig crate with wiggle. @pchickey and myself have been working on wiggle for over a month now, and we feel we got to the point where it could successfully be used in wasi-common, wasmtime and lucet. We also agreed that if any functionality is still missing, we should iterate in-tree if possible. Anyhow, this is a draft PR so things will invariably change (a lot!), but I wanted to get it out there sooner rather than later so that I can get some meaningful feedback early on and work on addressing that when things are still plastic.

[wiggle]: https://github.com/kubkon/wiggle

Wasmtime GitHub notifications bot (Mar 19 2020 at 09:20):

kubkon updated PR #1202 from wiggle-time to master:

It's [wiggle] time!

This PR is the beginning of a journey which aims at replacing wig crate with wiggle. @pchickey and myself have been working on wiggle for over a month now, and we feel we got to the point where it could successfully be used in wasi-common, wasmtime and lucet. We also agreed that if any functionality is still missing, we should iterate in-tree if possible. Anyhow, this is a draft PR so things will invariably change (a lot!), but I wanted to get it out there sooner rather than later so that I can get some meaningful feedback early on and work on addressing that when things are still plastic.

[wiggle]: https://github.com/kubkon/wiggle

Wasmtime GitHub notifications bot (Mar 19 2020 at 10:43):

kubkon updated PR #1202 from wiggle-time to master:

It's [wiggle] time!

This PR is the beginning of a journey which aims at replacing wig crate with wiggle. @pchickey and myself have been working on wiggle for over a month now, and we feel we got to the point where it could successfully be used in wasi-common, wasmtime and lucet. We also agreed that if any functionality is still missing, we should iterate in-tree if possible. Anyhow, this is a draft PR so things will invariably change (a lot!), but I wanted to get it out there sooner rather than later so that I can get some meaningful feedback early on and work on addressing that when things are still plastic.

[wiggle]: https://github.com/kubkon/wiggle

Wasmtime GitHub notifications bot (Mar 19 2020 at 10:48):

kubkon updated PR #1202 from wiggle-time to master:

It's [wiggle] time!

This PR is the beginning of a journey which aims at replacing wig crate with wiggle. @pchickey and myself have been working on wiggle for over a month now, and we feel we got to the point where it could successfully be used in wasi-common, wasmtime and lucet. We also agreed that if any functionality is still missing, we should iterate in-tree if possible. Anyhow, this is a draft PR so things will invariably change (a lot!), but I wanted to get it out there sooner rather than later so that I can get some meaningful feedback early on and work on addressing that when things are still plastic.

[wiggle]: https://github.com/kubkon/wiggle

Wasmtime GitHub notifications bot (Mar 19 2020 at 11:41):

kubkon updated PR #1202 from wiggle-time to master:

It's [wiggle] time!

This PR is the beginning of a journey which aims at replacing wig crate with wiggle. @pchickey and myself have been working on wiggle for over a month now, and we feel we got to the point where it could successfully be used in wasi-common, wasmtime and lucet. We also agreed that if any functionality is still missing, we should iterate in-tree if possible. Anyhow, this is a draft PR so things will invariably change (a lot!), but I wanted to get it out there sooner rather than later so that I can get some meaningful feedback early on and work on addressing that when things are still plastic.

[wiggle]: https://github.com/kubkon/wiggle

Wasmtime GitHub notifications bot (Mar 19 2020 at 17:20):

kubkon updated PR #1202 from wiggle-time to master:

It's [wiggle] time!

This PR is the beginning of a journey which aims at replacing wig crate with wiggle. @pchickey and myself have been working on wiggle for over a month now, and we feel we got to the point where it could successfully be used in wasi-common, wasmtime and lucet. We also agreed that if any functionality is still missing, we should iterate in-tree if possible. Anyhow, this is a draft PR so things will invariably change (a lot!), but I wanted to get it out there sooner rather than later so that I can get some meaningful feedback early on and work on addressing that when things are still plastic.

[wiggle]: https://github.com/kubkon/wiggle

Wasmtime GitHub notifications bot (Mar 19 2020 at 18:07):

kubkon updated PR #1202 from wiggle-time to master:

It's [wiggle] time!

This PR is the beginning of a journey which aims at replacing wig crate with wiggle. @pchickey and myself have been working on wiggle for over a month now, and we feel we got to the point where it could successfully be used in wasi-common, wasmtime and lucet. We also agreed that if any functionality is still missing, we should iterate in-tree if possible. Anyhow, this is a draft PR so things will invariably change (a lot!), but I wanted to get it out there sooner rather than later so that I can get some meaningful feedback early on and work on addressing that when things are still plastic.

[wiggle]: https://github.com/kubkon/wiggle

Wasmtime GitHub notifications bot (Mar 19 2020 at 18:22):

pchickey submitted PR Review.

Wasmtime GitHub notifications bot (Mar 19 2020 at 18:22):

pchickey created PR Review Comment:

can you just use the Into<u32> implementation?

Wasmtime GitHub notifications bot (Mar 19 2020 at 18:34):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Mar 19 2020 at 18:34):

sunfishcode created PR Review Comment:

I agree it'd be good to have one way to do this, rather than two. Could I suggest preferring .inner() over .into()? Most of the code in wasi-common should treat these as opaque, so it'd be nice for the places where we do need to peek inside to stand out.

Wasmtime GitHub notifications bot (Mar 19 2020 at 18:36):

kubkon updated PR #1202 from wiggle-time to master:

It's [wiggle] time!

This PR is the beginning of a journey which aims at replacing wig crate with wiggle. @pchickey and myself have been working on wiggle for over a month now, and we feel we got to the point where it could successfully be used in wasi-common, wasmtime and lucet. We also agreed that if any functionality is still missing, we should iterate in-tree if possible. Anyhow, this is a draft PR so things will invariably change (a lot!), but I wanted to get it out there sooner rather than later so that I can get some meaningful feedback early on and work on addressing that when things are still plastic.

[wiggle]: https://github.com/kubkon/wiggle

Wasmtime GitHub notifications bot (Mar 19 2020 at 20:06):

kubkon updated PR #1202 from wiggle-time to master:

It's [wiggle] time!

This PR is the beginning of a journey which aims at replacing wig crate with wiggle. @pchickey and myself have been working on wiggle for over a month now, and we feel we got to the point where it could successfully be used in wasi-common, wasmtime and lucet. We also agreed that if any functionality is still missing, we should iterate in-tree if possible. Anyhow, this is a draft PR so things will invariably change (a lot!), but I wanted to get it out there sooner rather than later so that I can get some meaningful feedback early on and work on addressing that when things are still plastic.

[wiggle]: https://github.com/kubkon/wiggle

Wasmtime GitHub notifications bot (Mar 19 2020 at 20:07):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Mar 19 2020 at 20:07):

sunfishcode created PR Review Comment:

fd_pool and entries need to stay in sync with each other. How awkward would it be to have a single RefCell that holds a struct containing both?

Wasmtime GitHub notifications bot (Mar 19 2020 at 20:07):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Mar 19 2020 at 20:07):

sunfishcode created PR Review Comment:

And similar here? If so, we may not even need the contains_key method, which would be nice because it can be tricky to use without look-before-you-leap hazards.

Wasmtime GitHub notifications bot (Mar 19 2020 at 20:07):

sunfishcode created PR Review Comment:

Instead of from, what would you think of naming this method from_raw, to emphasize that this call is not something one should do often.

Wasmtime GitHub notifications bot (Mar 19 2020 at 20:07):

sunfishcode created PR Review Comment:

Implementing this as a trait for WasiCtx is an interesting idea, and it is pretty fun to do ctx.fd_close as the code does above, but does this make it harder to share a WasiCtx between snapshot versions in the same wasm module?

Wasmtime GitHub notifications bot (Mar 19 2020 at 20:07):

sunfishcode created PR Review Comment:

This isn't strictly incorrect (at least while we only have one thread), but it feels awkward. Would it work to do something like this? A little verbose still, but it only does one borrow and one entries lookup.

        let maybe = Ref::map(self.entries.borrow(), |entries| {
            entries.get(&fd)
        });
        if maybe.is_none() {
            return Err(Errno::BadF);
        }
        Ref::map(maybe, |maybe_entry| maybe_entry.unwrap())
    }

?

Wasmtime GitHub notifications bot (Mar 19 2020 at 20:07):

sunfishcode created PR Review Comment:

What does bc stand for?

Wasmtime GitHub notifications bot (Mar 19 2020 at 20:07):

sunfishcode created PR Review Comment:

Is there a path to where these trace! calls could be auto-generated by wiggle?

Wasmtime GitHub notifications bot (Mar 19 2020 at 20:07):

sunfishcode created PR Review Comment:

Can you say more about the unsafe here? I assume wiggle has done the bounds checking before we get here, is that right?

Wasmtime GitHub notifications bot (Mar 19 2020 at 20:07):

sunfishcode created PR Review Comment:

Is it temporary to add this hook in wig here?

Wasmtime GitHub notifications bot (Mar 19 2020 at 20:08):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 19 2020 at 20:08):

kubkon created PR Review Comment:

Ah, excellent observation!

Wasmtime GitHub notifications bot (Mar 19 2020 at 20:09):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 19 2020 at 20:09):

kubkon created PR Review Comment:

Awesome, this is exactly what I wanted to do and couldn't figure out how. Thanks! :-D

Wasmtime GitHub notifications bot (Mar 19 2020 at 20:09):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 19 2020 at 20:09):

kubkon created PR Review Comment:

Right, I've actually tweaked this only so that it builds. I was going to cc you about the fs module and how we'd want to handle things here :-)

Wasmtime GitHub notifications bot (Mar 19 2020 at 20:11):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 19 2020 at 20:11):

kubkon created PR Review Comment:

Excellent question! Thanks for bringing it up! @pchickey and I had some thoughts about this, and as far as I can tell, he would rather avoid adding logging to wiggle (which is where we'd want this to end up actually). I haven't put that much thought to it just yet, hence why I left the traces in wasi-common, but I'm open to suggestions here!

Wasmtime GitHub notifications bot (Mar 19 2020 at 20:11):

sunfishcode created PR Review Comment:

Ah, I missed the part of your earlier comment where you explained this ^_^.

Wasmtime GitHub notifications bot (Mar 19 2020 at 20:11):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Mar 19 2020 at 20:12):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 19 2020 at 20:12):

kubkon created PR Review Comment:

Excellent question! So I think it won't be that difficult. However, I suggest to leave that for a subsequent PR. I'm purposely migrated all impls into one module and directly into the trait impl so that we can have a better idea how to tackle the problem of multiple snapshots/polyfill. Does it make sense?

Wasmtime GitHub notifications bot (Mar 19 2020 at 20:13):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 19 2020 at 20:13):

kubkon created PR Review Comment:

You got me, I was just following @alexcrichton's example in wiggle's tests :-} I'm happy to change it to something more meaningful, say borrows?

Wasmtime GitHub notifications bot (Mar 19 2020 at 20:17):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 19 2020 at 20:17):

kubkon created PR Review Comment:

That's correct, as_raw does the bounds checking. We need unsafe here since we're dereferencing a raw *mut _ pointer, and in this particular case, we're obtaining &str. The signature of this function here is:

fn as_raw(...) -> Result<*mut _, GuestError>

This is unsafe since we cannot really track the references here other than by using GuestBorrows. More on that here.

Wasmtime GitHub notifications bot (Mar 19 2020 at 20:18):

kubkon created PR Review Comment:

Yep, until we figure out where to put the glue macro for wasmtime-wasi/wasmtime, i.e., the one that's autogenerating the Func wrappers, and whatnot.

Wasmtime GitHub notifications bot (Mar 19 2020 at 20:18):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 19 2020 at 20:24):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 19 2020 at 20:24):

kubkon created PR Review Comment:

I'll try to beat everyone to the question here. I'm not sure how to handle this better with the current state of wiggle (which is not to say we can't modify wiggle to accommodate this case). So, I was forced to create two instances of GuestBorrows since it is possible for the two paths that are handed to us from guest to point at the same memory location (one use case being for instance symlink loops, etc.).

I was thinking, we could potentially check for that by comparing the pointers (if they implemented PartialEq), but it still feels clunky. Perhaps we should revisit the concept of immutability in wiggle? That is, differentiate immutable vs mutable borrows? In this case, I reckon it should be fine to borrow the same location immutably multiple times. Thoughts? @alexcrichton what do you reckon?

Wasmtime GitHub notifications bot (Mar 19 2020 at 20:25):

kubkon edited PR Review Comment.

Wasmtime GitHub notifications bot (Mar 19 2020 at 20:27):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 19 2020 at 20:27):

kubkon created PR Review Comment:

@sunfishcode Note that we don't have unsafe's in methods' signatures. I was somewhat worried about this since we're still dealing with file descriptors and other OS handles, but perhaps the entry points to WASI syscalls are OK being safe especially given that WASI fd is now a handle type? What do you think?

Wasmtime GitHub notifications bot (Mar 19 2020 at 20:27):

pchickey created PR Review Comment:

This is a great question. One alternative to creating two different borrowed path strings is to clone two path strings from the wasm memory into Rust owned Strings. I expect, since paths shouldn't be too big, that is the right design choice here, except for the possibility of a DoS attack by creating two 4gb Strings here.

Wasmtime GitHub notifications bot (Mar 19 2020 at 20:27):

pchickey submitted PR Review.

Wasmtime GitHub notifications bot (Mar 19 2020 at 20:28):

pchickey edited PR Review Comment.

Wasmtime GitHub notifications bot (Mar 19 2020 at 20:34):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Mar 19 2020 at 20:34):

sunfishcode created PR Review Comment:

Differentiating between mutable and immutable borrows sounds like a good way to go.

For example, POSIX doesn't say anything about link's arguments aliasing or not, but most functions with output buffers, such as readlink are defined with restrict qualifiers, meaning the output buffer is expected to not overlap with any of the input buffers. This also echos the Rust's borrow checker: you can have many aliasing immutable references, but a mutable reference must be unaliased.

Wasmtime GitHub notifications bot (Mar 19 2020 at 20:59):

pchickey submitted PR Review.

Wasmtime GitHub notifications bot (Mar 19 2020 at 20:59):

pchickey created PR Review Comment:

I've thought about this and I'm fairly convinced the right thing, for now, is to leave a nice comment that outlines the design decisions you just did in this thread.

Wasmtime GitHub notifications bot (Mar 19 2020 at 21:07):

pchickey edited PR Review Comment.

Wasmtime GitHub notifications bot (Mar 19 2020 at 21:19):

pchickey submitted PR Review.

Wasmtime GitHub notifications bot (Mar 19 2020 at 21:19):

pchickey created PR Review Comment:

its short for "borrow checker".

Wasmtime GitHub notifications bot (Mar 19 2020 at 21:28):

alexcrichton created PR Review Comment:

Could there be a log feature of wiggle which generates log statements? That way wasmtime could turn it on but if it's not needed in lucet it could be disabled?

Wasmtime GitHub notifications bot (Mar 19 2020 at 21:28):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Mar 19 2020 at 21:29):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Mar 19 2020 at 21:29):

alexcrichton created PR Review Comment:

I'd actually imagine that it makes it easier to share a WasiCtx between versions because we could implement both traits for one struct!

Wasmtime GitHub notifications bot (Mar 19 2020 at 21:44):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Mar 19 2020 at 21:44):

sunfishcode created PR Review Comment:

Crazy idea that I haven't thought all the way through yet: Would it work if we made Fd work like std::fs::File?

• Make Fd::from_raw unsafe? , similar to from_raw_fd being unsafe.
• Make Fd not implement Copy. Change Fd arguments to &Fd.
• Make Fd implement Drop and have it automatically close. This might be hard though. Is there a way in Rust to make drop private, so that you have to pass it back to the context?

Wasmtime GitHub notifications bot (Mar 19 2020 at 21:53):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Mar 19 2020 at 21:53):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Mar 19 2020 at 21:53):

alexcrichton created PR Review Comment:

FWIW stylistically I'd group sets of statements that are all "participating in the unsafe" together rather than trying to minimize the unsafe block to just one expression.

Wasmtime GitHub notifications bot (Mar 19 2020 at 21:53):

alexcrichton created PR Review Comment:

I'm not sure if this is how it was implemented before, but I'm actually pretty surprised by this. I thought that one of the purposes of GuestBorrows and stuff was to pass wasm buffers directly to syscalls, no?

Wasmtime GitHub notifications bot (Mar 19 2020 at 21:53):

alexcrichton created PR Review Comment:

One of the downsides of this returning RefMut directly is that it runs the risk of making it easy to panic in wasi-common code by accident I think. For example if while you have a RefMut you call back into wasm any reentrancy back into wasi would cause a panic due to a failed borrow.

It may just be worth documenting that for now, but wanted to point out that this may be a panic waiting to happen in the future.

Wasmtime GitHub notifications bot (Mar 19 2020 at 21:53):

alexcrichton created PR Review Comment:

Also, I don't think this function needs to be unsafe?

Wasmtime GitHub notifications bot (Mar 19 2020 at 21:53):

alexcrichton created PR Review Comment:

I think the usage of GuestBorrows here technically may not be correct. There's no real need to do any borrow checking here because there's no long-lived borrows, this is effectively just a "given this guest slice pointer write in these bytes' which is in general a safe operation (like GuestPtr::write is)

Wasmtime GitHub notifications bot (Mar 19 2020 at 21:53):

alexcrichton created PR Review Comment:

As a side note, this is not a safe function in general because the virtual implementation could reenter wasm. I think we'll want to make the virtual traits unsafe since you need to assert you don't reenter wasm or try to reborrow/read/etc from wasm memory in the underlying implementations.

Wasmtime GitHub notifications bot (Mar 19 2020 at 21:53):

alexcrichton created PR Review Comment:

I'd have the same question as before with pread here about passing raw wasm buffers to the underlying implementation.

Wasmtime GitHub notifications bot (Mar 19 2020 at 21:53):

alexcrichton created PR Review Comment:

There's two slices vecs here, but I think there only needs to be one?

Wasmtime GitHub notifications bot (Mar 19 2020 at 21:53):

alexcrichton created PR Review Comment:

I don't actually think that GuestBorrows is really needed here at all because there are no long-lived pointers, I think it's fine to do what this is currently doing, but I might recommend making it a bit more concise with the comment I made above

Wasmtime GitHub notifications bot (Mar 19 2020 at 21:53):

alexcrichton created PR Review Comment:

This idiom is showing up in a few places ("given this slice write this slice"), so it might be good to encapsulate this in a safe function perhaps?

Wasmtime GitHub notifications bot (Mar 19 2020 at 21:53):

alexcrichton created PR Review Comment:

This read is unsafe in combination with the long-lived borrows here, you'll need to add the GuestPtr<Iovec> to the GuestBorrows to make sure this pointer doesn't overlap with previous buffers.

Wasmtime GitHub notifications bot (Mar 19 2020 at 21:53):

alexcrichton created PR Review Comment:

Hm does as_raw or as_array not work with zero-length arrays? I would naively expect this condition to not be necessary.

Wasmtime GitHub notifications bot (Mar 19 2020 at 21:53):

alexcrichton created PR Review Comment:

Could the idiom of unsafely getting a &str and then calling path::get be wrapped up in a safe function to unsafe doesn't have to be used each time a path is resolved?

Wasmtime GitHub notifications bot (Mar 19 2020 at 21:53):

alexcrichton created PR Review Comment:

Technically the FIXME items elsewhere are equally applicable here, there's not really any fundamental reason why these two guest buffers shouldn't be allowed to overlap.

Wasmtime GitHub notifications bot (Mar 19 2020 at 21:57):

pchickey submitted PR Review.

Wasmtime GitHub notifications bot (Mar 19 2020 at 21:57):

pchickey created PR Review Comment:

Unfortunately, this is how it was implemented before. I think we should correct it in a follow-up PR.

Wasmtime GitHub notifications bot (Mar 19 2020 at 21:59):

pchickey submitted PR Review.

Wasmtime GitHub notifications bot (Mar 19 2020 at 21:59):

pchickey created PR Review Comment:

Agreed, this could be a method on GuestPtr<[T]>

Wasmtime GitHub notifications bot (Mar 19 2020 at 21:59):

pchickey edited PR Review Comment.

Wasmtime GitHub notifications bot (Mar 19 2020 at 22:00):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Mar 19 2020 at 22:00):

alexcrichton created PR Review Comment:

Ah ok, in that case I think we can probably avoid the GuestBorrows and collecting slices, since all the writes/reads only happen in one go and there's no need for long-lived borrows.

Wasmtime GitHub notifications bot (Mar 20 2020 at 00:32):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Mar 20 2020 at 00:32):

sunfishcode created PR Review Comment:

The unsafe is there for the raw file descriptor. This is a convention we picked up from std::fs::File::from_raw_fd, which isn't necessarily memory-unsafe, but extends the concept of safety to include I/O safety. Or an alternative interpretation is that file descriptors are pointers into a different address space (they can dangle, alias, be out of bounds, be bogus, etc.), and memory safety rules should apply to that address space too.

That said, I think we're still working out exactly how we want to approach this. See https://github.com/bytecodealliance/wasmtime/pull/1202/files#r395300196 for an example.

Wasmtime GitHub notifications bot (Mar 20 2020 at 00:34):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Mar 20 2020 at 00:34):

sunfishcode created PR Review Comment:

On further consideration, the Drop part makes this unworkable. I'm going to investigate other approaches.

Wasmtime GitHub notifications bot (Mar 20 2020 at 05:02):

pchickey submitted PR Review.

Wasmtime GitHub notifications bot (Mar 20 2020 at 05:02):

pchickey created PR Review Comment:

Hmm, I don't think I understand what you mean here by GuestBorrows not needed here at all. I'm probably missing something but let me outline the things I do believe:

I agree that in this case, the borrow checking is vacuous. But there isn't presently another method besides GuestPtr::as_raw that allows us to get a &str out of this GuestPtr<str>. We could add a method to GuestPtr that returns an owned String, but I fear that is a DoS vector because the guest will control the size of that String, and therefore gets to allocate in Rust's heap. We haven't yet rigorously audited this codebase for DoS vectors, of course, but I'm wary of adding something I know I'll have to forbid actually using in production.

I do not think we should create a shortcut variant of GuestPtr::as_raw to omit borrow checking which is only safe if the user ensures borrows are always short-lived, because I could see that leading to bugs when someone less familiar with the safety properties updates the code. Maybe that is too paranoid?

Wasmtime GitHub notifications bot (Mar 20 2020 at 08:59):

kubkon updated PR #1202 from wiggle-time to master:

It's [wiggle] time!

This PR is the beginning of a journey which aims at replacing wig crate with wiggle. @pchickey and myself have been working on wiggle for over a month now, and we feel we got to the point where it could successfully be used in wasi-common, wasmtime and lucet. We also agreed that if any functionality is still missing, we should iterate in-tree if possible. Anyhow, this is a draft PR so things will invariably change (a lot!), but I wanted to get it out there sooner rather than later so that I can get some meaningful feedback early on and work on addressing that when things are still plastic.

[wiggle]: https://github.com/kubkon/wiggle

Wasmtime GitHub notifications bot (Mar 20 2020 at 09:05):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 20 2020 at 09:05):

kubkon created PR Review Comment:

Good point. Do you think we could do it in a subsequent PR and just leave reminder "TODO" comment now?

Wasmtime GitHub notifications bot (Mar 20 2020 at 09:06):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 20 2020 at 09:06):

kubkon created PR Review Comment:

Yep, @pchickey is right, I've just adapted the "prior art" to wiggle-runtime types. I'm happy to look at this though, but @pchickey might be right that we should perhaps increment in-tree?

Wasmtime GitHub notifications bot (Mar 20 2020 at 09:07):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 20 2020 at 09:07):

kubkon created PR Review Comment:

Cool, I'm fine with that. I have to admit when porting the syscalls to wiggle-runtime I was following the fd_pread example from wiggle's WASI testcase :-)

Wasmtime GitHub notifications bot (Mar 20 2020 at 09:15):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 20 2020 at 09:15):

kubkon created PR Review Comment:

I guess you may be right, however, without the GuestBorrows I cannot obtain a mutable ref to [u8], and there's not other simple way of copying the contents of arg_bytes into the argv_buf pointer currently. The only other way I can see is manually incrementing argv_buf pointer and copying byte-by-byte from arg_bytes. Unless I'm perhaps missing something here?

Wasmtime GitHub notifications bot (Mar 20 2020 at 09:17):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 20 2020 at 09:17):

kubkon created PR Review Comment:

Cool, I'll leave a comment in for now, and we can figure out a fix later. As far as unsafe is concerned, as @sunfishcode pointed out, this is "prior art" and was there to signal the unsafety of using I/O. FWIW I'd leave that in for now and we can figure out how to deal with all the unsafety in a future PR?

Wasmtime GitHub notifications bot (Mar 20 2020 at 09:19):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 20 2020 at 09:19):

kubkon created PR Review Comment:

I like the idea but yeah, Drop will not work like this, unless we made the context object static/global. Having said that your suggestion aligns with my mental model of Fd 100%, so perhaps we could come up with something close to it but not necessarily one-to-one.

Wasmtime GitHub notifications bot (Mar 20 2020 at 09:21):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 20 2020 at 09:21):

kubkon created PR Review Comment:

Exactly as @pchickey explained, obtaining a reference to a slice-type object in Rust from wiggle is currently only possible through as_raw method which requires a GuestBorrows object in scope.

Wasmtime GitHub notifications bot (Mar 20 2020 at 09:23):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 20 2020 at 09:23):

kubkon created PR Review Comment:

When you say "encapsulate in a safe function" do you mean as a helper in wasi-common or a method of GuestPtr<'_, [T]>? I'm partial to the latter approach, and agreed that a method like this would be most useful!

Wasmtime GitHub notifications bot (Mar 20 2020 at 09:23):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 20 2020 at 09:23):

kubkon created PR Review Comment:

Excellent observation, and as I explained above, I've only made enough to make wiggle work with wasi-common. I'm happy to investigate this further though!

Wasmtime GitHub notifications bot (Mar 20 2020 at 09:25):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 20 2020 at 09:25):

kubkon created PR Review Comment:

Hmm, out of curiosity, is the example here wrong then?

Wasmtime GitHub notifications bot (Mar 20 2020 at 09:34):

kubkon updated PR #1202 from wiggle-time to master:

It's [wiggle] time!

This PR is the beginning of a journey which aims at replacing wig crate with wiggle. @pchickey and myself have been working on wiggle for over a month now, and we feel we got to the point where it could successfully be used in wasi-common, wasmtime and lucet. We also agreed that if any functionality is still missing, we should iterate in-tree if possible. Anyhow, this is a draft PR so things will invariably change (a lot!), but I wanted to get it out there sooner rather than later so that I can get some meaningful feedback early on and work on addressing that when things are still plastic.

[wiggle]: https://github.com/kubkon/wiggle

Wasmtime GitHub notifications bot (Mar 20 2020 at 09:38):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 20 2020 at 09:39):

kubkon created PR Review Comment:

Oh, this must be a remnant of my hack to not stifle progress with wasi-common while some required changes to wiggle were in progress. This change is no longer part of this PR, so please ignore it.

Wasmtime GitHub notifications bot (Mar 20 2020 at 09:41):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 20 2020 at 09:41):

kubkon created PR Review Comment:

Oh, I believe what @alexcrichton suggests sounds like _the_ solution to this problem. @pchickey @sunfishcode what do you guys reckon? Oh, and until that lands in wiggle, my suggestion would be to leave the traces as-is in wasi-common. What do you think?

Wasmtime GitHub notifications bot (Mar 20 2020 at 09:50):

kubkon updated PR #1202 from wiggle-time to master:

It's [wiggle] time!

This PR is the beginning of a journey which aims at replacing wig crate with wiggle. @pchickey and myself have been working on wiggle for over a month now, and we feel we got to the point where it could successfully be used in wasi-common, wasmtime and lucet. We also agreed that if any functionality is still missing, we should iterate in-tree if possible. Anyhow, this is a draft PR so things will invariably change (a lot!), but I wanted to get it out there sooner rather than later so that I can get some meaningful feedback early on and work on addressing that when things are still plastic.

[wiggle]: https://github.com/kubkon/wiggle

Wasmtime GitHub notifications bot (Mar 20 2020 at 09:56):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 20 2020 at 09:56):

kubkon created PR Review Comment:

Oh, and I think we should do the same for wasi-common as well. We've got a couple log calls here and there for debugging and what not, and I believe that @pchickey would like to have a version stripped off all altogether for lucet/xqd.

Wasmtime GitHub notifications bot (Mar 20 2020 at 09:56):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 20 2020 at 09:56):

kubkon created PR Review Comment:

Not suggesting we do it immediately in this PR, but we should have it in mind. :-)

Wasmtime GitHub notifications bot (Mar 20 2020 at 10:02):

kubkon updated PR #1202 from wiggle-time to master:

It's [wiggle] time!

This PR is the beginning of a journey which aims at replacing wig crate with wiggle. @pchickey and myself have been working on wiggle for over a month now, and we feel we got to the point where it could successfully be used in wasi-common, wasmtime and lucet. We also agreed that if any functionality is still missing, we should iterate in-tree if possible. Anyhow, this is a draft PR so things will invariably change (a lot!), but I wanted to get it out there sooner rather than later so that I can get some meaningful feedback early on and work on addressing that when things are still plastic.

[wiggle]: https://github.com/kubkon/wiggle

Wasmtime GitHub notifications bot (Mar 20 2020 at 10:14):

kubkon updated PR #1202 from wiggle-time to master:

It's [wiggle] time!

This PR is the beginning of a journey which aims at replacing wig crate with wiggle. @pchickey and myself have been working on wiggle for over a month now, and we feel we got to the point where it could successfully be used in wasi-common, wasmtime and lucet. We also agreed that if any functionality is still missing, we should iterate in-tree if possible. Anyhow, this is a draft PR so things will invariably change (a lot!), but I wanted to get it out there sooner rather than later so that I can get some meaningful feedback early on and work on addressing that when things are still plastic.

[wiggle]: https://github.com/kubkon/wiggle

Wasmtime GitHub notifications bot (Mar 20 2020 at 10:35):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 20 2020 at 10:35):

kubkon created PR Review Comment:

You're right, I'd expect this to work as well. It turns out wiggle_runtime::Region didn't accommodate for zero-length Regions. I've now fixed that in #1366. I'll leave a comment in to remove the check once it lands in wiggle-runtime.

Wasmtime GitHub notifications bot (Mar 20 2020 at 10:38):

kubkon created PR Review Comment:

I've simplified this now, so have a look and lemme know what you reckon!

Wasmtime GitHub notifications bot (Mar 20 2020 at 10:38):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 20 2020 at 10:40):

kubkon updated PR #1202 from wiggle-time to master:

It's [wiggle] time!

This PR is the beginning of a journey which aims at replacing wig crate with wiggle. @pchickey and myself have been working on wiggle for over a month now, and we feel we got to the point where it could successfully be used in wasi-common, wasmtime and lucet. We also agreed that if any functionality is still missing, we should iterate in-tree if possible. Anyhow, this is a draft PR so things will invariably change (a lot!), but I wanted to get it out there sooner rather than later so that I can get some meaningful feedback early on and work on addressing that when things are still plastic.

[wiggle]: https://github.com/kubkon/wiggle

Wasmtime GitHub notifications bot (Mar 20 2020 at 10:58):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 20 2020 at 10:58):

kubkon created PR Review Comment:

BTW @alexcrichton, lemme know what's your plan for this, i.e., what to use instead of RefMut! :-)

Wasmtime GitHub notifications bot (Mar 20 2020 at 10:59):

kubkon updated PR #1202 from wiggle-time to master:

It's [wiggle] time!

This PR is the beginning of a journey which aims at replacing wig crate with wiggle. @pchickey and myself have been working on wiggle for over a month now, and we feel we got to the point where it could successfully be used in wasi-common, wasmtime and lucet. We also agreed that if any functionality is still missing, we should iterate in-tree if possible. Anyhow, this is a draft PR so things will invariably change (a lot!), but I wanted to get it out there sooner rather than later so that I can get some meaningful feedback early on and work on addressing that when things are still plastic.

[wiggle]: https://github.com/kubkon/wiggle

Wasmtime GitHub notifications bot (Mar 20 2020 at 12:35):

sunfishcode created PR Review Comment:

Ok, thinking this through more carefully, maybe we can do this without the Drop part. So make handle types not implement Copy, pass handle arguments by reference rather than by value, and make the raw constructor unsafe. That's not the same as std::fs::File, but it still has some nice properties. We just need to be careful when calling the raw constructor, but there aren't many places where we need to do that outside of the generated wrappers. Does that sound feasible?

If you'd like to defer this to a later PR, that's fine too.

Wasmtime GitHub notifications bot (Mar 20 2020 at 12:35):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Mar 20 2020 at 12:35):

kubkon updated PR #1202 from wiggle-time to master:

It's [wiggle] time!

This PR is the beginning of a journey which aims at replacing wig crate with wiggle. @pchickey and myself have been working on wiggle for over a month now, and we feel we got to the point where it could successfully be used in wasi-common, wasmtime and lucet. We also agreed that if any functionality is still missing, we should iterate in-tree if possible. Anyhow, this is a draft PR so things will invariably change (a lot!), but I wanted to get it out there sooner rather than later so that I can get some meaningful feedback early on and work on addressing that when things are still plastic.

[wiggle]: https://github.com/kubkon/wiggle

Wasmtime GitHub notifications bot (Mar 20 2020 at 12:38):

kubkon created PR Review Comment:

OK, I've refactored both pread and pwrite so that we no longer need temp storage buffer. Instead, we're using slices bound to guest's memory directly (as IoSliceMut and IoSlice respectively), and both ops now essentially consists of two steps SeekFrom::Start(offset), followed by read_vectored(...) and write_vectored(...) respectively.

Wasmtime GitHub notifications bot (Mar 20 2020 at 12:38):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 20 2020 at 12:40):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 20 2020 at 12:40):

kubkon created PR Review Comment:

Sounds good, and yeah, if we could defer to a subsequent PR, that would be perfect IMHO. There's already a substantial amount of tweaks I have to apply and I'm worried more and more will slip away. Is that OK?

Wasmtime GitHub notifications bot (Mar 20 2020 at 12:42):

kubkon updated PR #1202 from wiggle-time to master:

It's [wiggle] time!

This PR is the beginning of a journey which aims at replacing wig crate with wiggle. @pchickey and myself have been working on wiggle for over a month now, and we feel we got to the point where it could successfully be used in wasi-common, wasmtime and lucet. We also agreed that if any functionality is still missing, we should iterate in-tree if possible. Anyhow, this is a draft PR so things will invariably change (a lot!), but I wanted to get it out there sooner rather than later so that I can get some meaningful feedback early on and work on addressing that when things are still plastic.

[wiggle]: https://github.com/kubkon/wiggle

Wasmtime GitHub notifications bot (Mar 20 2020 at 12:43):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 20 2020 at 12:43):

kubkon created PR Review Comment:

I've done just that. Now path::get takes &GuestPtr<'_, str> directly and does borrowing internally. This actually, at the surface at least, gets rid of the double-immutable-borrow problem.

Wasmtime GitHub notifications bot (Mar 20 2020 at 12:56):

kubkon created PR Review Comment:

Hmm, but wouldn't borrow_slice take care of it for us here?

Wasmtime GitHub notifications bot (Mar 20 2020 at 12:56):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 20 2020 at 13:06):

kubkon updated PR #1202 from wiggle-time to master:

It's [wiggle] time!

This PR is the beginning of a journey which aims at replacing wig crate with wiggle. @pchickey and myself have been working on wiggle for over a month now, and we feel we got to the point where it could successfully be used in wasi-common, wasmtime and lucet. We also agreed that if any functionality is still missing, we should iterate in-tree if possible. Anyhow, this is a draft PR so things will invariably change (a lot!), but I wanted to get it out there sooner rather than later so that I can get some meaningful feedback early on and work on addressing that when things are still plastic.

[wiggle]: https://github.com/kubkon/wiggle

Wasmtime GitHub notifications bot (Mar 20 2020 at 13:08):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 20 2020 at 13:08):

kubkon created PR Review Comment:

@pchickey What do you think of having a from_raw method for handles in wiggle?

Wasmtime GitHub notifications bot (Mar 20 2020 at 13:13):

kubkon updated PR #1202 from wiggle-time to master:

It's [wiggle] time!

This PR is the beginning of a journey which aims at replacing wig crate with wiggle. @pchickey and myself have been working on wiggle for over a month now, and we feel we got to the point where it could successfully be used in wasi-common, wasmtime and lucet. We also agreed that if any functionality is still missing, we should iterate in-tree if possible. Anyhow, this is a draft PR so things will invariably change (a lot!), but I wanted to get it out there sooner rather than later so that I can get some meaningful feedback early on and work on addressing that when things are still plastic.

[wiggle]: https://github.com/kubkon/wiggle

Wasmtime GitHub notifications bot (Mar 20 2020 at 14:22):

kubkon updated PR #1202 from wiggle-time to master:

It's [wiggle] time!

This PR is the beginning of a journey which aims at replacing wig crate with wiggle. @pchickey and myself have been working on wiggle for over a month now, and we feel we got to the point where it could successfully be used in wasi-common, wasmtime and lucet. We also agreed that if any functionality is still missing, we should iterate in-tree if possible. Anyhow, this is a draft PR so things will invariably change (a lot!), but I wanted to get it out there sooner rather than later so that I can get some meaningful feedback early on and work on addressing that when things are still plastic.

[wiggle]: https://github.com/kubkon/wiggle

Wasmtime GitHub notifications bot (Mar 20 2020 at 14:23):

kubkon created PR Review Comment:

Ok, I've now added an aux struct called EntryTable which holds both fd_pool and entries, and inside WasiCtx it's behind a single RefCell thus both working in sync.

Wasmtime GitHub notifications bot (Mar 20 2020 at 14:23):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 20 2020 at 14:39):

kubkon updated PR #1202 from wiggle-time to master:

It's [wiggle] time!

This PR is the beginning of a journey which aims at replacing wig crate with wiggle. @pchickey and myself have been working on wiggle for over a month now, and we feel we got to the point where it could successfully be used in wasi-common, wasmtime and lucet. We also agreed that if any functionality is still missing, we should iterate in-tree if possible. Anyhow, this is a draft PR so things will invariably change (a lot!), but I wanted to get it out there sooner rather than later so that I can get some meaningful feedback early on and work on addressing that when things are still plastic.

[wiggle]: https://github.com/kubkon/wiggle

Wasmtime GitHub notifications bot (Mar 20 2020 at 14:40):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 20 2020 at 14:40):

kubkon created PR Review Comment:

Ok, since #1366 landed, I've now removed the check and everything works as expected :+1:

Wasmtime GitHub notifications bot (Mar 20 2020 at 14:40):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Mar 20 2020 at 14:40):

alexcrichton created PR Review Comment:

@pchickey er sorry I don't think I was being very clear! I don't think we should remove the GuestBorrows argument or use owned strings here, I mean that each of these is resolved independently and there's never any overlapping borrows so documenting fixmes and/or hacks isn't necessary because each path is getting sucked into an owned object anyway so borrows never last long enough for runtime borrow-checking to be that relevant.

Wasmtime GitHub notifications bot (Mar 20 2020 at 14:42):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Mar 20 2020 at 14:42):

alexcrichton created PR Review Comment:

Hm ok so I would strongly recommend removing unsafe here. There's already enough unsafe as-is with dealing with raw wasm memory, and this feels like it's overkill.

The unsafety around libstd's I/O was specifically about providing a guarantee that each entity holding a file descriptor owns that file descriptor. That allows safe abstractions around mmap and stuff (in theory). I really think we should not be adding unsafe here lightly, and we may just not have that guarantee for wasi-common.

The unsafety here also doesn't really seem to stop anything in practice, every callsite just wraps a unsafe { ... } block which just makes it more verbose to call these methods rather than providing the "oh I should think about this" speedbump it seems.

Wasmtime GitHub notifications bot (Mar 20 2020 at 14:43):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 20 2020 at 14:43):

kubkon created PR Review Comment:

Ok, this proves to be more difficult than I originally envisioned. If I follow your suggestion, than we end up with a lifetime mismatch since the first maybe will contain weirdness of the like &Option<&Entry>, and the compiler cannot assign approriate lifetimes to the final Ref<'_, Entry>. Perhaps when we revisit the use of RefCell in wasi-common altogether, this will no longer be an issue?

Wasmtime GitHub notifications bot (Mar 20 2020 at 14:44):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Mar 20 2020 at 14:44):

alexcrichton created PR Review Comment:

Oh no nevermind my interpretation is wrong, I think this should be fine.

Wasmtime GitHub notifications bot (Mar 20 2020 at 14:47):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Mar 20 2020 at 14:47):

alexcrichton created PR Review Comment:

Oh I should also mention, I think the only real alternative to returning RefMut<Entry> is to return Rc<Entry>. That will likely be required anyway if we want to make this multithreaded at some point (probably don't want to hold locks for entireties of syscalls). I don't know of a great way to otherwise handle this. For now it seems fine to document how you need to be careful about holding a borrow for too long.

Wasmtime GitHub notifications bot (Mar 20 2020 at 14:48):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Mar 20 2020 at 14:48):

alexcrichton created PR Review Comment:

Oh sorry my point is that persisting the same GuestBorrows for the entirety of this method I believe is not necessary. Each write can use an independent instance of GuestBorrows since there's no need to prevent overlap in this method.

Wasmtime GitHub notifications bot (Mar 20 2020 at 14:49):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 20 2020 at 14:49):

kubkon created PR Review Comment:

Hmm, right, I think I get it. Rc should still allow for obtaining &mut of the wrapped type provided there is only one at play at any one time, right? And then in the multithreaded case, we'd just swap it for Arc?

Wasmtime GitHub notifications bot (Mar 20 2020 at 14:56):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Mar 20 2020 at 14:56):

alexcrichton created PR Review Comment:

I've posted https://github.com/bytecodealliance/wasmtime/pull/1367, and my point here is that if that's used then there's no need for GuestBorrows in this method and no need for this method to be unsafe.

Wasmtime GitHub notifications bot (Mar 20 2020 at 14:58):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Mar 20 2020 at 14:58):

alexcrichton created PR Review Comment:

FWIW I've posted https://github.com/bytecodealliance/wasmtime/pull/1367 to cover this.

Wasmtime GitHub notifications bot (Mar 20 2020 at 14:58):

alexcrichton created PR Review Comment:

In terms of semantics, I'm not sure if pread is specified to change the file cursor or not, but I believe this implementation does? Might be good to double check...

Wasmtime GitHub notifications bot (Mar 20 2020 at 14:58):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 20 2020 at 14:58):

kubkon created PR Review Comment:

That's perfect! It'll clean up the code nicely! :sunglasses:

Wasmtime GitHub notifications bot (Mar 20 2020 at 14:59):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 20 2020 at 14:59):

kubkon created PR Review Comment:

Oh crap, you're right, it shouldn't! I forgot to rewind the cursor! Thanks for bringing it up!

Wasmtime GitHub notifications bot (Mar 20 2020 at 15:02):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Mar 20 2020 at 15:02):

alexcrichton created PR Review Comment:

I don't believe that &mut is needed anywhere actually? For example all methods on I/O files are implemented for &T so you have to do something like (&entry.file).read(...) which is a bit wonky, but there shouldn't be a fundamental need to ever mutate an Entry.

If there is though then a RefCell can always be used, and the borrows would need to be scoped in duration.

Wasmtime GitHub notifications bot (Mar 20 2020 at 15:02):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Mar 20 2020 at 15:02):

alexcrichton created PR Review Comment:

Yes that should be fine

Wasmtime GitHub notifications bot (Mar 20 2020 at 15:10):

kubkon updated PR #1202 from wiggle-time to master:

It's [wiggle] time!

This PR is the beginning of a journey which aims at replacing wig crate with wiggle. @pchickey and myself have been working on wiggle for over a month now, and we feel we got to the point where it could successfully be used in wasi-common, wasmtime and lucet. We also agreed that if any functionality is still missing, we should iterate in-tree if possible. Anyhow, this is a draft PR so things will invariably change (a lot!), but I wanted to get it out there sooner rather than later so that I can get some meaningful feedback early on and work on addressing that when things are still plastic.

[wiggle]: https://github.com/kubkon/wiggle

Wasmtime GitHub notifications bot (Mar 20 2020 at 15:10):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 20 2020 at 15:10):

kubkon created PR Review Comment:

Fixed! Thanks for spotting that one!

Wasmtime GitHub notifications bot (Mar 20 2020 at 15:11):

kubkon created PR Review Comment:

I left the comment in virtfs.rs module.

Wasmtime GitHub notifications bot (Mar 20 2020 at 15:11):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 20 2020 at 15:16):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 20 2020 at 15:16):

kubkon created PR Review Comment:

You're absolutely right, we could easily operate on refs since we have impl on &T for I/O in Rust. That's cool. I'll make this change here already to clean up the code as much as possible (i.e., removing the muts). In terms of Rc, I'm guessing we'd still want to wrap the map holding the Entrys in an Rc?

Wasmtime GitHub notifications bot (Mar 20 2020 at 15:18):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 20 2020 at 15:18):

kubkon created PR Review Comment:

Right, we do since we still need to able to mutate EntryTable and not Entry!

Wasmtime GitHub notifications bot (Mar 20 2020 at 15:18):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Mar 20 2020 at 15:18):

alexcrichton created PR Review Comment:

The map will want to be in a RefCell for sure (was Rc a typo?) because we're modifying it behind a shared reference, and then each entry would be in its own Rc to get handed out without holding a long-lived borrow.

Wasmtime GitHub notifications bot (Mar 20 2020 at 15:19):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 20 2020 at 15:19):

kubkon created PR Review Comment:

Sorry, yeah, meant RefCell.

Right, makes sense!

Wasmtime GitHub notifications bot (Mar 20 2020 at 15:49):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 20 2020 at 15:49):

kubkon created PR Review Comment:

I was wrong actually. We do need to be able to mutate Entry in two places: fd_fdstat_set_rights and fd_fdstat_set_flags.

Wasmtime GitHub notifications bot (Mar 20 2020 at 16:17):

kubkon updated PR #1202 from wiggle-time to master:

It's [wiggle] time!

This PR is the beginning of a journey which aims at replacing wig crate with wiggle. @pchickey and myself have been working on wiggle for over a month now, and we feel we got to the point where it could successfully be used in wasi-common, wasmtime and lucet. We also agreed that if any functionality is still missing, we should iterate in-tree if possible. Anyhow, this is a draft PR so things will invariably change (a lot!), but I wanted to get it out there sooner rather than later so that I can get some meaningful feedback early on and work on addressing that when things are still plastic.

[wiggle]: https://github.com/kubkon/wiggle

Wasmtime GitHub notifications bot (Mar 20 2020 at 16:19):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 20 2020 at 16:19):

kubkon created PR Review Comment:

@sunfishcode would you agree at removing the unsafe here?

Wasmtime GitHub notifications bot (Mar 20 2020 at 16:29):

sunfishcode created PR Review Comment:

Yes. I've been exploring the "pass Fd by reference" idea more, and I'm sufficiently confident that that will provide a way for get_entry and get_entry_mut to be safe functions, even considering I/O safety, so I think we can leave the unsafe off for now because that's where they'll end up anyway.

Wasmtime GitHub notifications bot (Mar 20 2020 at 16:29):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Mar 20 2020 at 16:47):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 20 2020 at 16:47):

kubkon created PR Review Comment:

@alexcrichton Would you be OK if we parked the & vs &mut cleanup in wasi-common? Changing to & in most cases while absolutely correct will require some more work with virtfs module (at quick glance, not everything is behind a RefCell or alike to be able to mutate the interior behind &.

Wasmtime GitHub notifications bot (Mar 20 2020 at 16:48):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Mar 20 2020 at 16:48):

alexcrichton created PR Review Comment:

Oh sorry yeah, I think cleaning up later is fine, but it'd be nice to remove the unsafe before landing

Wasmtime GitHub notifications bot (Mar 20 2020 at 16:49):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 20 2020 at 16:49):

kubkon created PR Review Comment:

Absolutely, I'm on it :+1:

Wasmtime GitHub notifications bot (Mar 20 2020 at 16:53):

kubkon updated PR #1202 from wiggle-time to master:

It's [wiggle] time!

This PR is the beginning of a journey which aims at replacing wig crate with wiggle. @pchickey and myself have been working on wiggle for over a month now, and we feel we got to the point where it could successfully be used in wasi-common, wasmtime and lucet. We also agreed that if any functionality is still missing, we should iterate in-tree if possible. Anyhow, this is a draft PR so things will invariably change (a lot!), but I wanted to get it out there sooner rather than later so that I can get some meaningful feedback early on and work on addressing that when things are still plastic.

[wiggle]: https://github.com/kubkon/wiggle

Wasmtime GitHub notifications bot (Mar 20 2020 at 16:55):

kubkon requested alexcrichton for a review on PR #1202.

Wasmtime GitHub notifications bot (Mar 20 2020 at 16:55):

kubkon requested alexcrichton, and sunfishcode for a review on PR #1202.

Wasmtime GitHub notifications bot (Mar 20 2020 at 16:55):

kubkon requested alexcrichton, pchickey, and sunfishcode for a review on PR #1202.

Wasmtime GitHub notifications bot (Mar 20 2020 at 17:13):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Mar 20 2020 at 17:13):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Mar 20 2020 at 17:13):

alexcrichton created PR Review Comment:

This looks like it may be buggy? I think environ_buf_offset may not be needed here because we're already offseting environ_buf on each iteration of the loop?

Wasmtime GitHub notifications bot (Mar 20 2020 at 17:26):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Mar 20 2020 at 17:26):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Mar 20 2020 at 17:26):

sunfishcode created PR Review Comment:

Ok, if it gets awkward, then I agree it makes sense to leave the code as-is for now.

Wasmtime GitHub notifications bot (Mar 20 2020 at 18:14):

kubkon updated PR #1202 from wiggle-time to master:

It's [wiggle] time!

This PR is the beginning of a journey which aims at replacing wig crate with wiggle. @pchickey and myself have been working on wiggle for over a month now, and we feel we got to the point where it could successfully be used in wasi-common, wasmtime and lucet. We also agreed that if any functionality is still missing, we should iterate in-tree if possible. Anyhow, this is a draft PR so things will invariably change (a lot!), but I wanted to get it out there sooner rather than later so that I can get some meaningful feedback early on and work on addressing that when things are still plastic.

[wiggle]: https://github.com/kubkon/wiggle

Wasmtime GitHub notifications bot (Mar 20 2020 at 18:15):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 20 2020 at 18:15):

kubkon created PR Review Comment:

@alexcrichton Right on, thanks for spotting that one! I believe I've fixed it in 5e75830, but if you could double check, that'd be awesome!

Wasmtime GitHub notifications bot (Mar 20 2020 at 20:16):

kubkon updated PR #1202 from wiggle-time to master:

It's [wiggle] time!

This PR is the beginning of a journey which aims at replacing wig crate with wiggle. @pchickey and myself have been working on wiggle for over a month now, and we feel we got to the point where it could successfully be used in wasi-common, wasmtime and lucet. We also agreed that if any functionality is still missing, we should iterate in-tree if possible. Anyhow, this is a draft PR so things will invariably change (a lot!), but I wanted to get it out there sooner rather than later so that I can get some meaningful feedback early on and work on addressing that when things are still plastic.

[wiggle]: https://github.com/kubkon/wiggle

Wasmtime GitHub notifications bot (Mar 20 2020 at 20:40):

pchickey submitted PR Review.

Wasmtime GitHub notifications bot (Mar 20 2020 at 20:54):

kubkon merged PR #1202.

Last updated: Nov 22 2024 at 16:03 UTC