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>
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 aMemoryManager
as a global context. I think we'd have to make the constructorunsafe
as well and document you can't create multiple of them. Afterwards each wiggle-generated call would take two parameters, one being theMemoryManager
(a borrow) and one being theGuestMemory
implementation. TheGuestPtr
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.
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.
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 aRefCell<BorrowChecker>
, so maybe we'll just call it the latter. I'll implement this on Monday :)
alexcrichton commented on Issue #1717:
It might actually be possible to just take
&BorrowChecker
in that case and have theRefCell
and/or mutability be interior toBorrowChecker
too?
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
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.
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: Dec 23 2024 at 12:05 UTC