wasmtime / PR #1717 Automate borrow checking in wiggle · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #1717 Automate borrow checking in wiggle

Wasmtime GitHub notifications bot (May 15 2020 at 21:07):

pchickey opened PR #1717 from pch/wiggle_auto_borrow_checking to master:

WIP

Wasmtime GitHub notifications bot (May 15 2020 at 21:07):

pchickey updated PR #1717 from pch/wiggle_auto_borrow_checking to master:

WIP

Wasmtime GitHub notifications bot (May 18 2020 at 18:45):

pchickey updated PR #1717 from pch/wiggle_auto_borrow_checking to master:

WIP

Wasmtime GitHub notifications bot (May 19 2020 at 02:06):

pchickey updated PR #1717 from pch/wiggle_auto_borrow_checking to master:

WIP

Wasmtime GitHub notifications bot (May 19 2020 at 02:08):

pchickey updated PR #1717 from pch/wiggle_auto_borrow_checking to master:

WIP

Wasmtime GitHub notifications bot (May 19 2020 at 02:13):

pchickey updated PR #1717 from pch/wiggle_auto_borrow_checking to master:

WIP

Wasmtime GitHub notifications bot (May 20 2020 at 00:35):

pchickey updated PR #1717 from pch/wiggle_auto_borrow_checking to master:

WIP

Wasmtime GitHub notifications bot (May 20 2020 at 01:13):

pchickey updated PR #1717 from pch/wiggle_auto_borrow_checking to master:

WIP

Wasmtime GitHub notifications bot (May 20 2020 at 16:44):

pchickey updated PR #1717 from pch/wiggle_auto_borrow_checking to master:

WIP

Wasmtime GitHub notifications bot (May 20 2020 at 19:29):

pchickey updated PR #1717 from pch/wiggle_auto_borrow_checking to master:

WIP

Wasmtime GitHub notifications bot (May 20 2020 at 19:55):

pchickey updated PR #1717 from pch/wiggle_auto_borrow_checking to master:

WIP

Wasmtime GitHub notifications bot (May 21 2020 at 00:18):

pchickey edited PR #1717 from pch/wiggle_auto_borrow_checking to master:

This PR re-designs the Wiggle library to automate borrow-checking of slices and strs that reside in WebAssembly memory. This reduces the burden of writing unsafe on Wiggle users, and places all unsafe at the boundary between the Wasm runtime and Wiggle, rather than requiring it inside the implementation of the Wiggle traits.

In short, the GuestPtr::as_raw methods have been replaced with runtime borrow-checked smart pointers. GuestPtr::as_raw gave raw pointers into guest memory and require the user to manually thread around a GuestBorrows instance and use unsafe to turn those raw pointers into references.

This PR deletes the GuestBorrows type and instead defines a BorrowChecker type that understands both borrows and drops (unborrows). Instead of having the user pass the BorrowChecker to all of the sites where it is needed, it gets passed to the Wiggle ABI-level function once (this, along with construction, is taken care of by wig) and each GuestPtr has an internal shared ref to it. The user doesn't ever handle borrowing or unborrowing directly - those methods are private to the crate and are taken care of by GuestPtr, plus the GuestSlice and GuestStr drop impls. Each pointer makes the borrow checker accessible via GuestPtr::borrow_checker(&self) -> &BorrowChecker, which is useful for inspecting BorrowChecker::has_outstanding_borrows(&self) -> bool.

The GuestPtr::as_raw methods are replaced by GuestPtr::as_slice on GuestPtr<[T]> and GuestPtr::as_str on GuestPtr<str>. These new methods return a GuestSlice<T> and GuestStr, respectively. These are just basic "smart pointers" in that they contain a &mut ref to the memory that is borrowed, impl Deref and DerefMut, and have a Drop impl which tells the BorrowChecker that the memory is no longer borrowed.

Additionally, each GuestPtr::read and write verifies that the target of that read or write is not borrowed, which is safer than Wiggle was before, where the user had to manually borrow any GuestPtr (with no way to unborrow it afterwards!) before reading or writing it while another borrow was outstanding.

In addition to the changes in wiggle, this PR updates wig and wasi-common for the new interfaces.

Many thanks to @alexcrichton who helped me with the design and implementation of this PR.

Wasmtime GitHub notifications bot (May 21 2020 at 00:18):

pchickey requested alexcrichton for a review on PR #1717.

Wasmtime GitHub notifications bot (May 21 2020 at 00:18):

pchickey requested alexcrichton and kubkon for a review on PR #1717.

Wasmtime GitHub notifications bot (May 21 2020 at 16:26):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (May 21 2020 at 16:26):

kubkon created PR Review Comment:

Is that necessary? Could we scope it instead to do auto-dropping?

Wasmtime GitHub notifications bot (May 21 2020 at 16:26):

kubkon created PR Review Comment:

I know it's not saving us much, but just FWIW, I think this one-liner should also work:
trace!("     | old_path='{}'", &*old_path.as_str()?);

Wasmtime GitHub notifications bot (May 21 2020 at 16:26):

kubkon created PR Review Comment:

Is BorrowCheckerOOM meant to signify an out-of-memory error? If that's the case, I'd suggest we be more verbose and spell the full thing out here: BorrowCheckerOutOfMemory. It hopefully will be less confusing for the newcomers.

Wasmtime GitHub notifications bot (May 21 2020 at 16:26):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (May 21 2020 at 16:26):

kubkon created PR Review Comment:

So the cool thing about the previous approach was that we didn't have to do iterate twice over the same array, which we unfortunately do now. I haven't look in-depth into GuestSlice, so I'm not sure how to go about managing lifetimes here yet, but would it be possible to store io::IoSliceMut in the first pass rather than do the conversion from GuestSlice to &mut [u8] here?

Wasmtime GitHub notifications bot (May 21 2020 at 16:26):

kubkon created PR Review Comment:

Given that the invariant is we have only one instance of BorrowChecker per Wasm memory, would it make sense to somehow orchestrate the use of Once here to enforce that? Or would that be an overkill?

Wasmtime GitHub notifications bot (May 21 2020 at 16:26):

kubkon created PR Review Comment:

I might have missed it, but do we actually handle the case here where, if we handed out all available handles, but then the user unborrows some region, we actually still trip with BorrowCheckerOOM on the next call to self.new_handle() since the unborrowed handle is never returned back to the pool? Or do we actually take care of it and I just missed it?

Wasmtime GitHub notifications bot (May 21 2020 at 16:26):

kubkon created PR Review Comment:

Oh and btw, it might also be good to add this scenario as a test case.

Wasmtime GitHub notifications bot (May 21 2020 at 16:27):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (May 21 2020 at 18:33):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (May 21 2020 at 18:33):

alexcrichton created PR Review Comment:

I think guest_slices could presumably be Vec<IoSliceMut> here, right? I don't think there's any intrinsic reason a second iteration is required?

Wasmtime GitHub notifications bot (May 21 2020 at 18:34):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (May 21 2020 at 18:34):

kubkon created PR Review Comment:

Yep, that'd be ideal.

Wasmtime GitHub notifications bot (May 21 2020 at 18:36):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (May 21 2020 at 18:36):

alexcrichton created PR Review Comment:

FWIW this makes sizeof(GuestPtr<T>) to be 4 words large (2 pointers for mem, 1 for bc, one for pointer).

It might be best to package up with:
pub struct BikeshedMe<'a> {
    mem: &'a (dyn GuestMemory + 'a),
    bc: &'a BorrowChecker,
}

pub struct GuestPtr<'a, T> {
    data: &'a BikeshedMe<'a>,
    pointer: T::Pointer,
}
(and have GuestPtr::new take &BikeshedMe)

Wasmtime GitHub notifications bot (May 21 2020 at 18:38):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (May 21 2020 at 18:38):

kubkon created PR Review Comment:

Oh, that's a cool suggestion! :+1:

Wasmtime GitHub notifications bot (May 21 2020 at 18:41):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (May 21 2020 at 18:48):

pchickey submitted PR Review.

Wasmtime GitHub notifications bot (May 21 2020 at 18:48):

pchickey created PR Review Comment:

this doesn't turn out to be necessary at all, idk what I was thinking. thanks for catching it.

Wasmtime GitHub notifications bot (May 21 2020 at 18:53):

pchickey submitted PR Review.

Wasmtime GitHub notifications bot (May 21 2020 at 18:53):

pchickey created PR Review Comment:

This is the trickiest thing I had to deal with with these changes. For safety, we need the GuestSlice to stay alive for at least as long as any &mut [u8] we get from it (Deref trait enforces this). I couldn't find any other mechanism to collect all of the GuestSlices (creating them does the borrow check) and then use them as &mut [u8] without iterating through them twice. I agree its not great, but the number of vectors hopefully won't be big enough that this becomes a big performance hit. If it does become a problem, we could perhaps find a faster mechanism that involves some manual unsafe, but I'd prefer not to cross that bridge yet.

Wasmtime GitHub notifications bot (May 21 2020 at 18:54):

pchickey submitted PR Review.

Wasmtime GitHub notifications bot (May 21 2020 at 18:54):

pchickey created PR Review Comment:

We need it in the next line as a &str, too

Wasmtime GitHub notifications bot (May 21 2020 at 18:54):

pchickey edited PR Review Comment.

Wasmtime GitHub notifications bot (May 21 2020 at 18:55):

pchickey submitted PR Review.

Wasmtime GitHub notifications bot (May 21 2020 at 18:55):

pchickey created PR Review Comment:

BorrowCheckerOutOfHandles is the right way to spell it, thanks

Wasmtime GitHub notifications bot (May 21 2020 at 18:57):

pchickey submitted PR Review.

Wasmtime GitHub notifications bot (May 21 2020 at 18:57):

pchickey created PR Review Comment:

I believe Once is for one-time-globally, we actually do want to create a new one of these for each wasm instance we create, and at an even smaller scale for each hostcall if we can guarantee that is equivalent (no recursive calls, basically)

Wasmtime GitHub notifications bot (May 21 2020 at 19:01):

pchickey submitted PR Review.

Wasmtime GitHub notifications bot (May 21 2020 at 19:01):

pchickey created PR Review Comment:

Yes, this datastructure isnt smart enough to recycle handles, because I didn't want to incur the bookkeeping cost for what should only happen in a pathological scenario of user code - if you were to { borrow(r1); loop { h2 = borrow(r2); unborrow(h2) } } you'd eventually run out of handles. I can add a test that shows this bad behavior is present, with a note that we might want to try to fix it.

If we recycled all unborrowed handles, it would be impossible to run out because the bound of usize handles is at least as big as the 32 bit memory space described by Region.

Wasmtime GitHub notifications bot (May 21 2020 at 19:02):

pchickey submitted PR Review.

Wasmtime GitHub notifications bot (May 21 2020 at 19:02):

pchickey created PR Review Comment:

Good point, I prefer your second solution that makes it a method on GuestMemory - the same code is responsible for constructing and passing each.

Wasmtime GitHub notifications bot (May 21 2020 at 19:04):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (May 21 2020 at 19:04):

kubkon created PR Review Comment:

Ah, right, makes sense. I forgot we can have multiple Wasm memories at once.

Wasmtime GitHub notifications bot (May 21 2020 at 19:06):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (May 21 2020 at 19:06):

kubkon created PR Review Comment:

Yep, that's a good point on the number of handles. I noticed this since we've faced the same problem with the FdPool in wasi-common, there handles being actual WASI file descriptors. Anyhow, a test case demonstrating the behaviour plus a comment sounds great to me!

Wasmtime GitHub notifications bot (May 21 2020 at 19:11):

pchickey submitted PR Review.

Wasmtime GitHub notifications bot (May 21 2020 at 19:11):

pchickey created PR Review Comment:

I wrote a test for this (requires 2^64 iterations) and it ran for more than 60 seconds before I killed it, so I don't want to include it in the test suite. I'll leave a description of the behavior as a comment.

Wasmtime GitHub notifications bot (May 21 2020 at 19:23):

pchickey updated PR #1717 from pch/wiggle_auto_borrow_checking to master:

This PR re-designs the Wiggle library to automate borrow-checking of slices and strs that reside in WebAssembly memory. This reduces the burden of writing unsafe on Wiggle users, and places all unsafe at the boundary between the Wasm runtime and Wiggle, rather than requiring it inside the implementation of the Wiggle traits.

In short, the GuestPtr::as_raw methods have been replaced with runtime borrow-checked smart pointers. GuestPtr::as_raw gave raw pointers into guest memory and require the user to manually thread around a GuestBorrows instance and use unsafe to turn those raw pointers into references.

This PR deletes the GuestBorrows type and instead defines a BorrowChecker type that understands both borrows and drops (unborrows). Instead of having the user pass the BorrowChecker to all of the sites where it is needed, it gets passed to the Wiggle ABI-level function once (this, along with construction, is taken care of by wig) and each GuestPtr has an internal shared ref to it. The user doesn't ever handle borrowing or unborrowing directly - those methods are private to the crate and are taken care of by GuestPtr, plus the GuestSlice and GuestStr drop impls. Each pointer makes the borrow checker accessible via GuestPtr::borrow_checker(&self) -> &BorrowChecker, which is useful for inspecting BorrowChecker::has_outstanding_borrows(&self) -> bool.

The GuestPtr::as_raw methods are replaced by GuestPtr::as_slice on GuestPtr<[T]> and GuestPtr::as_str on GuestPtr<str>. These new methods return a GuestSlice<T> and GuestStr, respectively. These are just basic "smart pointers" in that they contain a &mut ref to the memory that is borrowed, impl Deref and DerefMut, and have a Drop impl which tells the BorrowChecker that the memory is no longer borrowed.

Additionally, each GuestPtr::read and write verifies that the target of that read or write is not borrowed, which is safer than Wiggle was before, where the user had to manually borrow any GuestPtr (with no way to unborrow it afterwards!) before reading or writing it while another borrow was outstanding.

In addition to the changes in wiggle, this PR updates wig and wasi-common for the new interfaces.

Many thanks to @alexcrichton who helped me with the design and implementation of this PR.

Wasmtime GitHub notifications bot (May 21 2020 at 19:41):

pchickey updated PR #1717 from pch/wiggle_auto_borrow_checking to master:

This PR re-designs the Wiggle library to automate borrow-checking of slices and strs that reside in WebAssembly memory. This reduces the burden of writing unsafe on Wiggle users, and places all unsafe at the boundary between the Wasm runtime and Wiggle, rather than requiring it inside the implementation of the Wiggle traits.

In short, the GuestPtr::as_raw methods have been replaced with runtime borrow-checked smart pointers. GuestPtr::as_raw gave raw pointers into guest memory and require the user to manually thread around a GuestBorrows instance and use unsafe to turn those raw pointers into references.

This PR deletes the GuestBorrows type and instead defines a BorrowChecker type that understands both borrows and drops (unborrows). Instead of having the user pass the BorrowChecker to all of the sites where it is needed, it gets passed to the Wiggle ABI-level function once (this, along with construction, is taken care of by wig) and each GuestPtr has an internal shared ref to it. The user doesn't ever handle borrowing or unborrowing directly - those methods are private to the crate and are taken care of by GuestPtr, plus the GuestSlice and GuestStr drop impls. Each pointer makes the borrow checker accessible via GuestPtr::borrow_checker(&self) -> &BorrowChecker, which is useful for inspecting BorrowChecker::has_outstanding_borrows(&self) -> bool.

The GuestPtr::as_raw methods are replaced by GuestPtr::as_slice on GuestPtr<[T]> and GuestPtr::as_str on GuestPtr<str>. These new methods return a GuestSlice<T> and GuestStr, respectively. These are just basic "smart pointers" in that they contain a &mut ref to the memory that is borrowed, impl Deref and DerefMut, and have a Drop impl which tells the BorrowChecker that the memory is no longer borrowed.

Additionally, each GuestPtr::read and write verifies that the target of that read or write is not borrowed, which is safer than Wiggle was before, where the user had to manually borrow any GuestPtr (with no way to unborrow it afterwards!) before reading or writing it while another borrow was outstanding.

In addition to the changes in wiggle, this PR updates wig and wasi-common for the new interfaces.

Many thanks to @alexcrichton who helped me with the design and implementation of this PR.

Wasmtime GitHub notifications bot (May 21 2020 at 20:14):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (May 21 2020 at 20:14):

kubkon created PR Review Comment:

Oh sorry, I thought you’d spoof the test by doing a borrow, followed by an unborrow, and then manually fast forward next available handle to the limit and assert this fails whereas it shouldn’t since there was at least one unborrow registered. Having said that I reckon a comment will do nicely for now.

Wasmtime GitHub notifications bot (May 21 2020 at 20:15):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (May 21 2020 at 21:22):

pchickey updated PR #1717 from pch/wiggle_auto_borrow_checking to master:

This PR re-designs the Wiggle library to automate borrow-checking of slices and strs that reside in WebAssembly memory. This reduces the burden of writing unsafe on Wiggle users, and places all unsafe at the boundary between the Wasm runtime and Wiggle, rather than requiring it inside the implementation of the Wiggle traits.

In short, the GuestPtr::as_raw methods have been replaced with runtime borrow-checked smart pointers. GuestPtr::as_raw gave raw pointers into guest memory and require the user to manually thread around a GuestBorrows instance and use unsafe to turn those raw pointers into references.

This PR deletes the GuestBorrows type and instead defines a BorrowChecker type that understands both borrows and drops (unborrows). Instead of having the user pass the BorrowChecker to all of the sites where it is needed, it gets passed to the Wiggle ABI-level function once (this, along with construction, is taken care of by wig) and each GuestPtr has an internal shared ref to it. The user doesn't ever handle borrowing or unborrowing directly - those methods are private to the crate and are taken care of by GuestPtr, plus the GuestSlice and GuestStr drop impls. Each pointer makes the borrow checker accessible via GuestPtr::borrow_checker(&self) -> &BorrowChecker, which is useful for inspecting BorrowChecker::has_outstanding_borrows(&self) -> bool.

The GuestPtr::as_raw methods are replaced by GuestPtr::as_slice on GuestPtr<[T]> and GuestPtr::as_str on GuestPtr<str>. These new methods return a GuestSlice<T> and GuestStr, respectively. These are just basic "smart pointers" in that they contain a &mut ref to the memory that is borrowed, impl Deref and DerefMut, and have a Drop impl which tells the BorrowChecker that the memory is no longer borrowed.

Additionally, each GuestPtr::read and write verifies that the target of that read or write is not borrowed, which is safer than Wiggle was before, where the user had to manually borrow any GuestPtr (with no way to unborrow it afterwards!) before reading or writing it while another borrow was outstanding.

In addition to the changes in wiggle, this PR updates wig and wasi-common for the new interfaces.

Many thanks to @alexcrichton who helped me with the design and implementation of this PR.

Wasmtime GitHub notifications bot (May 21 2020 at 22:48):

pchickey updated PR #1717 from pch/wiggle_auto_borrow_checking to master:

This PR re-designs the Wiggle library to automate borrow-checking of slices and strs that reside in WebAssembly memory. This reduces the burden of writing unsafe on Wiggle users, and places all unsafe at the boundary between the Wasm runtime and Wiggle, rather than requiring it inside the implementation of the Wiggle traits.

In short, the GuestPtr::as_raw methods have been replaced with runtime borrow-checked smart pointers. GuestPtr::as_raw gave raw pointers into guest memory and require the user to manually thread around a GuestBorrows instance and use unsafe to turn those raw pointers into references.

This PR deletes the GuestBorrows type and instead defines a BorrowChecker type that understands both borrows and drops (unborrows). Instead of having the user pass the BorrowChecker to all of the sites where it is needed, it gets passed to the Wiggle ABI-level function once (this, along with construction, is taken care of by wig) and each GuestPtr has an internal shared ref to it. The user doesn't ever handle borrowing or unborrowing directly - those methods are private to the crate and are taken care of by GuestPtr, plus the GuestSlice and GuestStr drop impls. Each pointer makes the borrow checker accessible via GuestPtr::borrow_checker(&self) -> &BorrowChecker, which is useful for inspecting BorrowChecker::has_outstanding_borrows(&self) -> bool.

The GuestPtr::as_raw methods are replaced by GuestPtr::as_slice on GuestPtr<[T]> and GuestPtr::as_str on GuestPtr<str>. These new methods return a GuestSlice<T> and GuestStr, respectively. These are just basic "smart pointers" in that they contain a &mut ref to the memory that is borrowed, impl Deref and DerefMut, and have a Drop impl which tells the BorrowChecker that the memory is no longer borrowed.

Additionally, each GuestPtr::read and write verifies that the target of that read or write is not borrowed, which is safer than Wiggle was before, where the user had to manually borrow any GuestPtr (with no way to unborrow it afterwards!) before reading or writing it while another borrow was outstanding.

In addition to the changes in wiggle, this PR updates wig and wasi-common for the new interfaces.

Many thanks to @alexcrichton who helped me with the design and implementation of this PR.

Wasmtime GitHub notifications bot (May 22 2020 at 16:35):

pchickey merged PR #1717.

Last updated: Dec 23 2024 at 12:05 UTC