Remove the need to have a Store for an InstancePre (#5683)

* Remove the need to have a `Store` for an `InstancePre`

This commit relaxes a requirement of the `InstancePre` API, notably its
construction via `Linker::instantiate_pre`. Previously this function
required a `Store<T>` to be present to be able to perform type-checking
on the contents of the linker, and now this requirement has been
removed.

Items stored within a linker are either a `HostFunc`, which has type
information inside of it, or an `Extern`, which doesn't have type
information inside of it. Due to the usage of `Extern` this is why a
`Store` was required during the `InstancePre` construction process, it's
used to extract the type of an `Extern`. This commit implements a
solution where the type information of an `Extern` is stored alongside
the `Extern` itself, meaning that the `InstancePre` construction process
no longer requires a `Store<T>`.

One caveat of this implementation is that some items, such as tables and
memories, technically have a "dynamic type" where during type checking
their current size is consulted to match against the minimum size
required of an import. This no longer works when using
`Linker::instantiate_pre` as the current size used is the one when it
was inserted into the linker rather than the one available at
instantiation time. It's hoped, however, that this is a relatively
esoteric use case that doesn't impact many real-world users.

Additionally note that this is an API-breaking change. Not only is the
`Store` argument removed from `Linker::instantiate_pre`, but some other
methods such as `Linker::define` grew a `Store` argument as the type
needs to be extracted when an item is inserted into a linker.

Closes #5675

* Fix the C API

* Fix benchmark compilation

* Add C API docs

* Update crates/wasmtime/src/linker.rs

Co-authored-by: Andrew Brown <[email protected]>

---------

Co-authored-by: Andrew Brown <[email protected]>

Author: alexcrichton

benches/instantiation.rs

@@ -46,7 +46,7 @@ fn bench_sequential(c: &mut Criterion, path: &Path) {
             let mut linker = Linker::new(&engine);
             wasmtime_wasi::add_to_linker(&mut linker, |cx| cx).unwrap();
             let pre = linker
-                .instantiate_pre(&mut store(&engine), &module)
+                .instantiate_pre(&module)
                 .expect("failed to pre-instantiate");
             (engine, pre)
         });
@@ -77,7 +77,7 @@ fn bench_parallel(c: &mut Criterion, path: &Path) {
             wasmtime_wasi::add_to_linker(&mut linker, |cx| cx).unwrap();
             let pre = Arc::new(
                 linker
-                    .instantiate_pre(&mut store(&engine), &module)
+                    .instantiate_pre(&module)
                     .expect("failed to pre-instantiate"),
             );
             (engine, pre)

crates/c-api/include/wasmtime/linker.h

@@ -59,6 +59,7 @@ WASM_API_EXTERN void wasmtime_linker_allow_shadowing(wasmtime_linker_t* linker,
  * \brief Defines a new item in this linker.
  *
  * \param linker the linker the name is being defined in.
+ * \param store the store that the `item` is owned by.
  * \param module the module name the item is defined under.
  * \param module_len the byte length of `module`
  * \param name the field name the item is defined under
@@ -73,6 +74,7 @@ WASM_API_EXTERN void wasmtime_linker_allow_shadowing(wasmtime_linker_t* linker,
  */
 WASM_API_EXTERN wasmtime_error_t* wasmtime_linker_define(
     wasmtime_linker_t *linker,
+    wasmtime_context_t *store,
     const char *module,
     size_t module_len,
     const char *name,

crates/c-api/src/linker.rs

@@ -1,6 +1,6 @@
 use crate::{
     bad_utf8, handle_result, wasm_engine_t, wasm_functype_t, wasm_trap_t, wasmtime_error_t,
-    wasmtime_extern_t, wasmtime_module_t, CStoreContextMut,
+    wasmtime_extern_t, wasmtime_module_t, CStoreContext, CStoreContextMut,
 };
 use std::ffi::c_void;
 use std::mem::MaybeUninit;
@@ -41,6 +41,7 @@ macro_rules! to_str {
 #[no_mangle]
 pub unsafe extern "C" fn wasmtime_linker_define(
     linker: &mut wasmtime_linker_t,
+    store: CStoreContext<'_>,
     module: *const u8,
     module_len: usize,
     name: *const u8,
@@ -51,7 +52,7 @@ pub unsafe extern "C" fn wasmtime_linker_define(
     let module = to_str!(module, module_len);
     let name = to_str!(name, name_len);
     let item = item.to_extern();
-    handle_result(linker.define(module, name, item), |_linker| ())
+    handle_result(linker.define(&store, module, name, item), |_linker| ())
 }
 
 #[no_mangle]

crates/fuzzing/src/oracles.rs

@@ -544,45 +544,40 @@ pub fn table_ops(
         // test case.
         const MAX_GCS: usize = 5;
 
-        linker
-            .define(
-                "",
-                "gc",
-                // NB: use `Func::new` so that this can still compile on the old x86
-                // backend, where `IntoFunc` isn't implemented for multi-value
-                // returns.
-                Func::new(
-                    &mut store,
-                    FuncType::new(
-                        vec![],
-                        vec![ValType::ExternRef, ValType::ExternRef, ValType::ExternRef],
-                    ),
-                    {
-                        let num_dropped = num_dropped.clone();
-                        let expected_drops = expected_drops.clone();
-                        let num_gcs = num_gcs.clone();
-                        move |mut caller: Caller<'_, StoreLimits>, _params, results| {
-                            log::info!("table_ops: GC");
-                            if num_gcs.fetch_add(1, SeqCst) < MAX_GCS {
-                                caller.gc();
-                            }
-
-                            let a = ExternRef::new(CountDrops(num_dropped.clone()));
-                            let b = ExternRef::new(CountDrops(num_dropped.clone()));
-                            let c = ExternRef::new(CountDrops(num_dropped.clone()));
-
-                            log::info!("table_ops: make_refs() -> ({:p}, {:p}, {:p})", a, b, c);
-
-                            expected_drops.fetch_add(3, SeqCst);
-                            results[0] = Some(a).into();
-                            results[1] = Some(b).into();
-                            results[2] = Some(c).into();
-                            Ok(())
-                        }
-                    },
-                ),
-            )
-            .unwrap();
+        // NB: use `Func::new` so that this can still compile on the old x86
+        // backend, where `IntoFunc` isn't implemented for multi-value
+        // returns.
+        let func = Func::new(
+            &mut store,
+            FuncType::new(
+                vec![],
+                vec![ValType::ExternRef, ValType::ExternRef, ValType::ExternRef],
+            ),
+            {
+                let num_dropped = num_dropped.clone();
+                let expected_drops = expected_drops.clone();
+                let num_gcs = num_gcs.clone();
+                move |mut caller: Caller<'_, StoreLimits>, _params, results| {
+                    log::info!("table_ops: GC");
+                    if num_gcs.fetch_add(1, SeqCst) < MAX_GCS {
+                        caller.gc();
+                    }
+
+                    let a = ExternRef::new(CountDrops(num_dropped.clone()));
+                    let b = ExternRef::new(CountDrops(num_dropped.clone()));
+                    let c = ExternRef::new(CountDrops(num_dropped.clone()));
+
+                    log::info!("table_ops: make_refs() -> ({:p}, {:p}, {:p})", a, b, c);
+
+                    expected_drops.fetch_add(3, SeqCst);
+                    results[0] = Some(a).into();
+                    results[1] = Some(b).into();
+                    results[2] = Some(c).into();
+                    Ok(())
+                }
+            },
+        );
+        linker.define(&store, "", "gc", func).unwrap();
 
         linker
             .func_wrap("", "take_refs", {
@@ -624,37 +619,29 @@ pub fn table_ops(
             })
             .unwrap();
 
-        linker
-            .define(
-                "",
-                "make_refs",
-                // NB: use `Func::new` so that this can still compile on the old
-                // x86 backend, where `IntoFunc` isn't implemented for
-                // multi-value returns.
-                Func::new(
-                    &mut store,
-                    FuncType::new(
-                        vec![],
-                        vec![ValType::ExternRef, ValType::ExternRef, ValType::ExternRef],
-                    ),
-                    {
-                        let num_dropped = num_dropped.clone();
-                        let expected_drops = expected_drops.clone();
-                        move |_caller, _params, results| {
-                            log::info!("table_ops: make_refs");
-                            expected_drops.fetch_add(3, SeqCst);
-                            results[0] =
-                                Some(ExternRef::new(CountDrops(num_dropped.clone()))).into();
-                            results[1] =
-                                Some(ExternRef::new(CountDrops(num_dropped.clone()))).into();
-                            results[2] =
-                                Some(ExternRef::new(CountDrops(num_dropped.clone()))).into();
-                            Ok(())
-                        }
-                    },
-                ),
-            )
-            .unwrap();
+        // NB: use `Func::new` so that this can still compile on the old
+        // x86 backend, where `IntoFunc` isn't implemented for
+        // multi-value returns.
+        let func = Func::new(
+            &mut store,
+            FuncType::new(
+                vec![],
+                vec![ValType::ExternRef, ValType::ExternRef, ValType::ExternRef],
+            ),
+            {
+                let num_dropped = num_dropped.clone();
+                let expected_drops = expected_drops.clone();
+                move |_caller, _params, results| {
+                    log::info!("table_ops: make_refs");
+                    expected_drops.fetch_add(3, SeqCst);
+                    results[0] = Some(ExternRef::new(CountDrops(num_dropped.clone()))).into();
+                    results[1] = Some(ExternRef::new(CountDrops(num_dropped.clone()))).into();
+                    results[2] = Some(ExternRef::new(CountDrops(num_dropped.clone()))).into();
+                    Ok(())
+                }
+            },
+        );
+        linker.define(&store, "", "make_refs", func).unwrap();
 
         let instance = linker.instantiate(&mut store, &module).unwrap();
         let run = instance.get_func(&mut store, "run").unwrap();

crates/fuzzing/src/oracles/dummy.rs

@@ -8,12 +8,9 @@ pub fn dummy_linker<'module, T>(store: &mut Store<T>, module: &Module) -> Result
     let mut linker = Linker::new(store.engine());
     linker.allow_shadowing(true);
     for import in module.imports() {
+        let extern_ = dummy_extern(store, import.ty())?;
         linker
-            .define(
-                import.module(),
-                import.name(),
-                dummy_extern(store, import.ty())?,
-            )
+            .define(&store, import.module(), import.name(), extern_)
             .unwrap();
     }
     Ok(linker)

crates/wasmtime/src/component/instance.rs

@@ -1,6 +1,7 @@
 use crate::component::func::HostFunc;
 use crate::component::{Component, ComponentNamedList, Func, Lift, Lower, TypedFunc};
 use crate::instance::OwnedImports;
+use crate::linker::DefinitionType;
 use crate::store::{StoreOpaque, Stored};
 use crate::{AsContextMut, Module, StoreContextMut};
 use anyhow::{anyhow, Context, Result};
@@ -439,13 +440,13 @@ impl<'a> Instantiator<'a> {
         }
 
         let val = unsafe { crate::Extern::from_wasmtime_export(export, store) };
+        let ty = DefinitionType::from(store, &val);
         crate::types::matching::MatchCx {
-            store,
             engine: store.engine(),
             signatures: module.signatures(),
             types: module.types(),
         }
-        .extern_(&expected, &val)
+        .definition(&expected, &ty)
         .expect("unexpected typecheck failure");
     }
 }

crates/wasmtime/src/externals.rs

@@ -138,16 +138,6 @@ impl Extern {
             Extern::Table(t) => store.store_data().contains(t.0),
         }
     }
-
-    pub(crate) fn desc(&self) -> &'static str {
-        match self {
-            Extern::Func(_) => "function",
-            Extern::Table(_) => "table",
-            Extern::Memory(_) => "memory",
-            Extern::SharedMemory(_) => "shared memory",
-            Extern::Global(_) => "global",
-        }
-    }
 }
 
 impl From<Func> for Extern {
@@ -233,8 +223,8 @@ impl Global {
     /// )?;
     ///
     /// let mut linker = Linker::new(&engine);
-    /// linker.define("", "i32-const", i32_const)?;
-    /// linker.define("", "f64-mut", f64_mut)?;
+    /// linker.define(&store, "", "i32-const", i32_const)?;
+    /// linker.define(&store, "", "f64-mut", f64_mut)?;
     ///
     /// let instance = linker.instantiate(&mut store, &module)?;
     /// // ...

crates/wasmtime/src/instance.rs

@@ -1,4 +1,4 @@
-use crate::linker::Definition;
+use crate::linker::{Definition, DefinitionType};
 use crate::store::{InstanceId, StoreOpaque, Stored};
 use crate::types::matching;
 use crate::{
@@ -157,7 +157,10 @@ impl Instance {
                 bail!("cross-`Store` instantiation is not currently supported");
             }
         }
-        typecheck(store, module, imports, |cx, ty, item| cx.extern_(ty, item))?;
+        typecheck(module, imports, |cx, ty, item| {
+            let item = DefinitionType::from(store, item);
+            cx.definition(ty, &item)
+        })?;
         let mut owned_imports = OwnedImports::new(module);
         for import in imports {
             owned_imports.push(import, store);
@@ -679,19 +682,8 @@ impl<T> InstancePre<T> {
     /// This method is unsafe as the `T` of the `InstancePre<T>` is not
     /// guaranteed to be the same as the `T` within the `Store`, the caller must
     /// verify that.
-    pub(crate) unsafe fn new(
-        store: &mut StoreOpaque,
-        module: &Module,
-        items: Vec<Definition>,
-    ) -> Result<InstancePre<T>> {
-        for import in items.iter() {
-            if !import.comes_from_same_store(store) {
-                bail!("cross-`Store` instantiation is not currently supported");
-            }
-        }
-        typecheck(store, module, &items, |cx, ty, item| {
-            cx.definition(ty, item)
-        })?;
+    pub(crate) unsafe fn new(module: &Module, items: Vec<Definition>) -> Result<InstancePre<T>> {
+        typecheck(module, &items, |cx, ty, item| cx.definition(ty, &item.ty()))?;
 
         let host_funcs = items
             .iter()
@@ -813,7 +805,6 @@ fn pre_instantiate_raw(
 }
 
 fn typecheck<I>(
-    store: &mut StoreOpaque,
     module: &Module,
     imports: &[I],
     check: impl Fn(&matching::MatchCx<'_>, &EntityType, &I) -> Result<()>,
@@ -826,8 +817,7 @@ fn typecheck<I>(
     let cx = matching::MatchCx {
         signatures: module.signatures(),
         types: module.types(),
-        store: store,
-        engine: store.engine(),
+        engine: module.engine(),
     };
     for ((name, field, expected_ty), actual) in env_module.imports().zip(imports) {
         check(&cx, &expected_ty, actual)