wasmtime / PR #9277 Refactor the `wasmtime::runtime::vm::... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #9277 Refactor the `wasmtime::runtime::vm::...

Wasmtime GitHub notifications bot (Sep 18 2024 at 17:42):

fitzgen requested alexcrichton for a review on PR #9277.

Wasmtime GitHub notifications bot (Sep 18 2024 at 17:42):

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

Wasmtime GitHub notifications bot (Sep 18 2024 at 17:42):

fitzgen opened PR #9277 from fitzgen:refactor-store-trait to bytecodealliance:main:

This dates back from the wasmtime vs wasmtime-runtime crate split. Now that those crates merged, we can simplify some bits and remove unnecessary abstractions and indirections.

Wasmtime GitHub notifications bot (Sep 18 2024 at 17:44):

fitzgen updated PR #9277.

Wasmtime GitHub notifications bot (Sep 18 2024 at 17:47):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Sep 18 2024 at 17:47):

fitzgen created PR review comment:

Note: This can't use the DerefMut impl, and must instead explicitly call store_opaque_mut(), because mutable borrows are invariant over type parameters.

Wasmtime GitHub notifications bot (Sep 18 2024 at 17:49):

fitzgen updated PR #9277.

Wasmtime GitHub notifications bot (Sep 18 2024 at 18:06):

fitzgen edited PR review comment.

Wasmtime GitHub notifications bot (Sep 18 2024 at 19:09):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Sep 18 2024 at 19:09):

alexcrichton created PR review comment:

I'm not sure I understand this, can you expand on this a bit? Is this something where we're accidentally handing out 'static? Or something is acidentally required to be 'static when it shouldn't be? Should the deref impls become impl Deref for dyn (Store + '_) or something like that?

Wasmtime GitHub notifications bot (Sep 18 2024 at 20:50):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Sep 18 2024 at 20:50):

fitzgen created PR review comment:

Here is the error, fwiw:
error: lifetime may not live long enough
   --> crates/wasmtime/src/runtime/vm/table.rs:622:13
    |
554 |         store: &mut dyn VMStore,
    |                - let's call the lifetime of this reference `'1`
...
622 |             store.unwrap_gc_store_mut(),
    |             ^^^^^ requires that `'1` must outlive `'static`
    |
    = note: requirement occurs because of a mutable reference to `dyn VMStore`
    = note: mutable references are invariant over their type parameter
    = help: see <https://doc.rust-lang.org/nomicon/subtyping.html> for more information about variance
My understanding is:

We have a store: &mut dyn VMStore

We are implicitly reborrowing it, in an attempt to get a shorter lifetime, by using the DerefMut impl at this call site

However, because mutable references are invariant over their type parameter, we don't actually end up with a shorter lifetime, but the same lifetime

And then for reasons I don't fully understand, that ends up forcing a 'static constraint. I think because that is the only way that we could unify those two lifetimes? Not sure.

FWIW, making the reborrow explicit with (&mut *store).unwrap_gc_store_mut() results in the same error message, but pointing at the explicit reborrow instead of the method invocation.

I don't know if adding a T: 'static on the impl would solve the problem or not. But also we don't require that for Store::new so I don't think it is really an option.

Quite possible I am misunderstanding something here, but at the end of the day: without the .store_opaque_mut() we get a compiler error, with it we don't.

Wasmtime GitHub notifications bot (Sep 18 2024 at 21:07):

fitzgen merged PR #9277.

Last updated: Nov 22 2024 at 17:03 UTC