Pass Fd by reference

[sunfishcode]

This is a draft PR partially illustrating the idea mentioned in #1202 for passing Fd handles by reference.

This provides I/O safety within the WASI implementation code, though it depends on being careful around calling the Fd::from_raw function. It also simplifies the FdPool, by taking advantage of the new EntryTable.

This doesn't build at the moment, because it doesn't change the wiggle code, but it shows how the rest of the code would look.

• Fd is passed by reference when it's an argument in host code.
• Fd no longer needs to implement Copy or Clone.
• File descriptors within structs -- types::SubscriptionU::FdRead and types::SubscriptionU::FdWrite still need to be stored as u32; it can be converted to an Fd by calling the unsafe from_raw

In Wiggle, would it be better to special-case Handle to implement pass-by-reference behavior, or add a new TypePassedBy kind in witx?

[kubkon]

OK, so the only thing missing is storing handles as part of a struct and passing this struct around. Personally, I think I'm leaning towards the first suggestion of always passing handles by ref.

[kubkon]

On second thought, this is going to be more tricky than I originally anticipated. wiggle currently is built with types that take an explicit lifetime in mind, so ideally we'd have something like Fd<'a>. In other words, if there was a way for us to hide the ref inside the type, that would be ideal. To make structs hold a ref in wiggle, I guess we'd need to make handles a special-case indeed, and this might perhaps lead to some overly complicated code? I'm just putting it out there. @pchickey @alexcrichton what do y'all reckon?

[alexcrichton]

I think I may be missing the motivation here, it sounds like we have u32 values from wasm, Fd, and Entry. Today Entry is already passed by reference, and this PR looks to be switching Fd, which is currently a thin newtype around u32, to also be passed by reference. Why is Fd being changed to be passed by reference?

I feel like a better eventuality may be something where if you take a handle type in a function it's automatically looked up, e.g. for (type $fd (handle)) that translates to this in the trait:

trait WasiSnapshot {
    fn lookup_fd(&self, idx: u32) -> Result<&Entry>;
}

(or something like that)

And that way APIs don't ever actually deal with Fd or u32, that all happens in the wiggle layer. Even in APIs that return handle we could change those to return Result<Entry> in Rust and that's automatically converted to a file descriptor under the hood.

I guess I'm sort of confused why we want more types passed by reference when we already have a type passed by reference, Entry?

[kubkon]

Hmm, those are some really good points, thanks @alexcrichton! I too don’t want to make this overly complicated and convoluted, and indeed this somewhat starts to look like it. However, one motivation for handles being non-Copy and non-Clone, and passed by ref, would be to signal that they are something more than just an “index” into a capabilities table. I guess you could think of it as RawFd vs File. Now I’m not saying this is the way to go, but personally I’d welcome it if we managed to come with a wrapper type for handles that could only be passed by reference unless reconstructed using unsafe primitives such as from_raw and as_raw. So that would be the main motivation the way I understand it. Perhaps @sunfishcode could offer some details or his point of view about all this.

[sunfishcode]

@alexcrichton I like that idea. If it's feasible to have wiggle generate the get_entry calls, so that the hand-maintained code just gets an &Entry parameter instead of a Fd, that achieves the same goal as passing Fd by reference, but in a simpler way.

[pchickey]

I like the idea of looking up witx handles to get a ref to some other Rust type, and passing that to the trait method. I've been fielding requests for wiggle to be able to convert to and from types defined in the user's code in other contexts. So I wonder if we can come up with a generalized way for wiggle users to specify that a witx type needs to be transformed to/from some externally-defined Rust type before consumption / after production.

The other big use case that has come up is in Fastly's HTTP code, we have a rich (as in thiserror) structured error enum that we want to implement the trait method with. If we return that Err from the trait method, we would like to call another Ctx method to log it appropriately and convert it to a "flat" witx error enum (as in errno). That use case is similar to the GuestErrorType trait we already require. I would like to figure out some way for that mechanism to work for handles, and external enum definitions like Frank has, as well.

This is a pretty complicated design space to figure out so if anyone wants to have a zoom call to chat about it more, lets.

[kubkon]

This might be a bit more premature to ask, but I guess in a trait suggested by @alexcrichton we'd expect something like this:

trait WasiSnapshot {
    type Entry;
    fn lookup_fd(&self, idx: u32) -> Result<&Self::Entry>;
}

in the sense that we wouldn't tie some specific type to the trait and the lookup method, or would we actually? I'm just trying to get a better sense of how we'd like to achieve this.

[alexcrichton]

That's my rough thinking yeah, although it may not literally work out as such. The idea though would be that each implementation would define what an Entry is, and the methods that operate on all the entries would get that object in the trait. That way lucet/wasmtime could have entirely different representations if they really wanted to (but they both use wasi-common so it likely wouldn't matter)

This may be a bit pie-in-the-sky though, so don't feel like anything needs to be blocked on it.

[github-actions]

Subscribe to Label Action

This issue or pull request has been labeled: "wasi"

Users Subscribed to "wasi"

• @kubkon

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

[sunfishcode]

Closing this PR to pursue other approaches as discussed above.