Stream: git-wasmtime

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


view this post on Zulip 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:

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:

  1. 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.

  2. 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.

<!--
Please make sure you include the following information:

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

view this post on Zulip Wasmtime GitHub notifications bot (Nov 05 2024 at 20:00):

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

view this post on Zulip Wasmtime GitHub notifications bot (Nov 05 2024 at 20:00):

fitzgen requested alexcrichton for a review on PR #9565.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 05 2024 at 20:43):

fitzgen updated PR #9565.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 05 2024 at 20:49):

fitzgen updated PR #9565.

view this post on Zulip 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.

view this post on Zulip 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>.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 05 2024 at 20:56):

fitzgen submitted PR review.

view this post on Zulip 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?

view this post on Zulip Wasmtime GitHub notifications bot (Nov 05 2024 at 21:01):

alexcrichton submitted PR review.

view this post on Zulip 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...

view this post on Zulip Wasmtime GitHub notifications bot (Nov 05 2024 at 21:15):

fitzgen submitted PR review.

view this post on Zulip 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>)

view this post on Zulip Wasmtime GitHub notifications bot (Nov 05 2024 at 21:16):

fitzgen submitted PR review.

view this post on Zulip 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

view this post on Zulip Wasmtime GitHub notifications bot (Nov 05 2024 at 21:40):

fitzgen updated PR #9565.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 05 2024 at 21:52):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 05 2024 at 22:08):

fitzgen merged PR #9565.


Last updated: Dec 23 2024 at 12:05 UTC