wasmtime / PR #9565 Refactor `vm::Instance` for better `S... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #9565 Refactor `vm::Instance` for better `S...

Wasmtime GitHub notifications bot (Nov 05 2024 at 20:00):

fitzgen opened PR #9565 from fitzgen:instance-and-store-split to bytecodealliance:main:

At various times, for example in libcalls, we need to go from a raw vmctx pointer that Wasm gave us to both an &mut Instance and an &mut Store{Opaque,Context}. This is a pretty fundamental unsafe aspect of implementing a language runtimne in Rust, but we can tighten things up a bit and make them a little safer than they currently are. This commit is such an attempt and is notably tackling the issue of creating multiple store borrows after we have an instance borrow.

This commit makes the following changes:

Instance doesn't expose a method to get the raw *mut dyn VMStore pointer or otherwise create a store borrow.

You cannot construct an Instance directly from a vmctx pointer.

There is now an InstanceAndStore type that represents unique access to both an Instance and a Store.

You can (with unsafe) create an InstanceAndStore from a raw vmctx pointer. This generally only happens inside a couple "bottlenecks", not all throughout the codebase, like the shared plumbing code for libcalls.

The InstanceAndStore can be unpacked into a mutable borrow of an Instance and a mutable borrow of a Store. This unpacking takes holds a mutable borrow of the original InstanceAndStore, so double borrows of the store are no longer a concern, so long as you don't use unsafe to create new InstanceAndStores for the same vmctx.

All Instance methods and functions that previously would unsafely turn the Instance's internal store pointer into a store borrow to perform some action now take a store argument, and this store is threaded around as necessary.

Altogether, I feel that we've ended up with an architecture that is a safety improvement over where we were previously, and will help us properly avoid Rust UB.

For what its worth, I do not think this is the end state. I foresee at least two additional follow ups that I unfortunately do not currently have time for:

Create ComponentInstanceAndStore, which is the same thing that this commit did but for components. I started on this but ran out of fuel while trying to update macros for all the component shims and transcoders.

Stop dealing with &mut vm::Tables and &mut vm::Globals and all that in the runtime. Instead just deal with indices, similar to how things are structured in the host API level. This refactoring was not previously possible due to (1) the wasmtime versus wasmtime-runtime crate split and (2) the lack of StoreOpaques threaded through the VM internals. The first blocker was addressed a few months ago, this commit removes the second blocker. This will still be a pretty large refactoring though, but I think ultimately will be worth it.

Wasmtime GitHub notifications bot (Nov 05 2024 at 20:00):

fitzgen requested wasmtime-core-reviewers for a review on PR #9565.

Wasmtime GitHub notifications bot (Nov 05 2024 at 20:00):

fitzgen requested alexcrichton for a review on PR #9565.

Wasmtime GitHub notifications bot (Nov 05 2024 at 20:43):

fitzgen updated PR #9565.

Wasmtime GitHub notifications bot (Nov 05 2024 at 20:49):

fitzgen updated PR #9565.

Wasmtime GitHub notifications bot (Nov 05 2024 at 20:51):

alexcrichton submitted PR review:

Looks great :+1:

One comment about the 'static though on the dyn VMStore type though.

Wasmtime GitHub notifications bot (Nov 05 2024 at 20:51):

alexcrichton created PR review comment:

Can the 'static here be removed? In theory that's a "lie" because we don't constrain T: 'static in the Store<T>.

Wasmtime GitHub notifications bot (Nov 05 2024 at 20:56):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Nov 05 2024 at 20:56):

fitzgen created PR review comment:

Unfortunately, I don't think it can be or else the borrow is invariant and the InstanceAndStore is unusable after calling unpack once, which isn't acceptable for Caller::with, which needs to access the pair multiple times. It miiight have been an issue in certain libcalls too, I can't quite remember.

Morally, I see this as a "smaller lie" than our current status quo where we just whip up a borrow from a raw pointer whenever we feel like it...

I can try poking at it again, but if I can't figure out a way to remove the 'static, do you think this is a blocker to landing this?

Wasmtime GitHub notifications bot (Nov 05 2024 at 21:01):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Nov 05 2024 at 21:01):

alexcrichton created PR review comment:

Hm this is surprising. I'm worried about labeling this 'static when it wasn't since that to me isn't a failure condition we worried about before (I think at least?). Mind trying to remove it and see what happens?

I'm also surprised if InstanceAndStore is unusable after one call, as that probably means we have this whole tying typed incorrectly anyway...

Wasmtime GitHub notifications bot (Nov 05 2024 at 21:15):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Nov 05 2024 at 21:15):

fitzgen created PR review comment:

I guess we could maybe get rid of InstanceAndStore and just have

impl Instance {
    pub(crate) unsafe fn from_vmctx<R>(vmctx: *mut VMContext, f: impl FnOnce(&mut StoreOpaque, &mut Instance) -> R) -> R {
        // ...
    }
}

(And another version for StoreInner<T>)

Wasmtime GitHub notifications bot (Nov 05 2024 at 21:16):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Nov 05 2024 at 21:16):

fitzgen created PR review comment:

This would require moving all the VMStore trait methods to StoreOpaque

Wasmtime GitHub notifications bot (Nov 05 2024 at 21:40):

fitzgen updated PR #9565.

Wasmtime GitHub notifications bot (Nov 05 2024 at 21:52):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Nov 05 2024 at 22:08):

fitzgen merged PR #9565.

Last updated: Nov 22 2024 at 16:03 UTC