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

Stream: git-wasmtime

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

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

github-actions[bot] commented on Issue #1717:

Subscribe to Label Action

cc @kubkon

<details>
This issue or pull request has been labeled: "wasi"

Thus the following users have been cc'd because of the following labels:

kubkon: wasi

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

Learn more.
</details>

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

alexcrichton commented on Issue #1717:

Looking pretty good! As you mentioned on Zulip I agree the main thing missing here is how we expect the "assert there are no active borrows" to work.

A possible idea I think is to remove the lifetime from MemoryManager. That way an application would create one instance of a MemoryManager as a global context. I think we'd have to make the constructor unsafe as well and document you can't create multiple of them. Afterwards each wiggle-generated call would take two parameters, one being the MemoryManager (a borrow) and one being the GuestMemory implementation. The GuestPtr type would then internally contain &(&MemoryManager, &GuestMemory) (or some equivalent thereof).

The main caveat would be documenting the unsafety of the constructor, which I think would amount to:

You can only create one per wasm memory.

You must call an "assert no active borrows" whenever wasm is called.

I think with that the invariants we have are:

Whenever you enter a wiggle-generated function, you provide proof of "I have the one memory manager". This means that structure is tracking all active borrows into guest memory.

Whenever you go back to wasm, you've asserted you're calling the method to make sure there's no active borrows.

Otherwise inside of the wiggle-generated function all wasm borrows are channeled through MemoryManager, which does normal management with RAII to ensure everything works out.

I think that should cover our use case? We could even make 'assert no active borrows' a function that takes no parameters, updating some thread-local state, if it's too onerous to thread that around.

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

kubkon commented on Issue #1717:

I like @alexcrichton's take on this, although manually asserting sounds like a bit of a pain and I'm worried it might be easy to forget. But I'm usually a pessimist so perhaps this won't be that much of a problem after all.

Wasmtime GitHub notifications bot (May 15 2020 at 23:51):

pchickey commented on Issue #1717:

Thanks for taking a look, I like your ideas on how to restructure this. That boils MemoryManager down to just a RefCell<BorrowChecker>, so maybe we'll just call it the latter. I'll implement this on Monday :)

Wasmtime GitHub notifications bot (May 18 2020 at 12:06):

alexcrichton commented on Issue #1717:

It might actually be possible to just take &BorrowChecker in that case and have the RefCell and/or mutability be interior to BorrowChecker too?

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

pchickey commented on Issue #1717:

Rewrote the changes with this redesign in mind. Still need to:

go through and audit the safety story. I think for this to work correctly GuestPtr::{read, write} has to borrow, read/write, then unborrow; plus I'm not sure if the array borrows are done correctly.

the BorrowChecker datastructure itself is probably nowhere near optimal - hashmap of a u32 key isnt the best, should reset the starting key to zero when the map is emptied so we don't have to deal with index overflow or finding unused keys

string the changes throughout wasi-common and the wasmtime interface code

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

pchickey commented on Issue #1717:

I think by checking that the region is unborrowed in GuestPtr::{read, write}, we ensure that we don't read or write anything that may be aliased with a mutable ref. This does add a significant amount of overhead to each read and write operation. We should look into a more efficient datastructure at some point, but I'm not going to invest time in that now.

Tomorrow I'll update the docs and thread it through the downstream libraries.

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

pchickey commented on Issue #1717:

I believe I addressed all code review, GuestMemory::borrow_checker() now gives the BorrowChecker, rather than passing it separately.

Last updated: Nov 22 2024 at 16:03 UTC