diff --git a/crates/api/src/externals.rs b/crates/api/src/externals.rs index 576c92e15e..43639cacc1 100644 --- a/crates/api/src/externals.rs +++ b/crates/api/src/externals.rs @@ -1,7 +1,8 @@ -use crate::trampoline::{generate_global_export, generate_memory_export, generate_table_export}; +use crate::trampoline::{ + generate_global_export, generate_memory_export, generate_table_export, StoreInstanceHandle, +}; use crate::values::{from_checked_anyfunc, into_checked_anyfunc, Val}; -use crate::Mutability; -use crate::{ExternType, GlobalType, MemoryType, TableType, ValType}; +use crate::{ExternType, GlobalType, MemoryType, Mutability, TableType, ValType}; use crate::{Func, Store, Trap}; use anyhow::{anyhow, bail, Result}; use std::slice; @@ -92,21 +93,20 @@ impl Extern { pub(crate) fn from_wasmtime_export( wasmtime_export: wasmtime_runtime::Export, - store: &Store, - instance_handle: InstanceHandle, + instance: StoreInstanceHandle, ) -> Extern { match wasmtime_export { wasmtime_runtime::Export::Function(f) => { - Extern::Func(Func::from_wasmtime_function(f, store, instance_handle)) + Extern::Func(Func::from_wasmtime_function(f, instance)) } wasmtime_runtime::Export::Memory(m) => { - Extern::Memory(Memory::from_wasmtime_memory(m, store, instance_handle)) + Extern::Memory(Memory::from_wasmtime_memory(m, instance)) } wasmtime_runtime::Export::Global(g) => { - Extern::Global(Global::from_wasmtime_global(g, store, instance_handle)) + Extern::Global(Global::from_wasmtime_global(g, instance)) } wasmtime_runtime::Export::Table(t) => { - Extern::Table(Table::from_wasmtime_table(t, store, instance_handle)) + Extern::Table(Table::from_wasmtime_table(t, instance)) } } } @@ -114,9 +114,9 @@ impl Extern { pub(crate) fn comes_from_same_store(&self, store: &Store) -> bool { let my_store = match self { Extern::Func(f) => f.store(), - Extern::Global(g) => &g.store, - Extern::Memory(m) => &m.store, - Extern::Table(t) => &t.store, + Extern::Global(g) => &g.instance.store, + Extern::Memory(m) => &m.instance.store, + Extern::Table(t) => &t.instance.store, }; Store::same(my_store, store) } @@ -163,9 +163,8 @@ impl From for Extern { /// instances are equivalent in their functionality. #[derive(Clone)] pub struct Global { - store: Store, + instance: StoreInstanceHandle, wasmtime_export: wasmtime_runtime::ExportGlobal, - wasmtime_handle: InstanceHandle, } impl Global { @@ -187,11 +186,10 @@ impl Global { if val.ty() != *ty.content() { bail!("value provided does not match the type of this global"); } - let (wasmtime_handle, wasmtime_export) = generate_global_export(store, &ty, val)?; + let (instance, wasmtime_export) = generate_global_export(store, &ty, val)?; Ok(Global { - store: store.clone(), + instance, wasmtime_export, - wasmtime_handle, }) } @@ -246,7 +244,7 @@ impl Global { if val.ty() != ty { bail!("global of type {:?} cannot be set to {:?}", ty, val.ty()); } - if !val.comes_from_same_store(&self.store) { + if !val.comes_from_same_store(&self.instance.store) { bail!("cross-`Store` values are not supported"); } unsafe { @@ -264,13 +262,11 @@ impl Global { pub(crate) fn from_wasmtime_global( wasmtime_export: wasmtime_runtime::ExportGlobal, - store: &Store, - wasmtime_handle: InstanceHandle, + instance: StoreInstanceHandle, ) -> Global { Global { - store: store.clone(), + instance, wasmtime_export, - wasmtime_handle, } } } @@ -292,18 +288,17 @@ impl Global { /// instances are equivalent in their functionality. #[derive(Clone)] pub struct Table { - store: Store, - wasmtime_handle: InstanceHandle, + instance: StoreInstanceHandle, wasmtime_export: wasmtime_runtime::ExportTable, } fn set_table_item( - handle: &InstanceHandle, + instance: &InstanceHandle, table_index: wasm::DefinedTableIndex, item_index: u32, item: wasmtime_runtime::VMCallerCheckedAnyfunc, ) -> Result<()> { - handle + instance .table_set(table_index, item_index, item) .map_err(|()| anyhow!("table element index out of bounds")) } @@ -322,18 +317,17 @@ impl Table { /// Returns an error if `init` does not match the element type of the table. pub fn new(store: &Store, ty: TableType, init: Val) -> Result
{ let item = into_checked_anyfunc(init, store)?; - let (wasmtime_handle, wasmtime_export) = generate_table_export(store, &ty)?; + let (instance, wasmtime_export) = generate_table_export(store, &ty)?; // Initialize entries with the init value. let definition = unsafe { &*wasmtime_export.definition }; - let index = wasmtime_handle.table_index(definition); + let index = instance.table_index(definition); for i in 0..definition.current_elements { - set_table_item(&wasmtime_handle, index, i, item.clone())?; + set_table_item(&instance, index, i, item.clone())?; } Ok(Table { - store: store.clone(), - wasmtime_handle, + instance, wasmtime_export, }) } @@ -345,10 +339,7 @@ impl Table { } fn wasmtime_table_index(&self) -> wasm::DefinedTableIndex { - unsafe { - self.wasmtime_handle - .table_index(&*self.wasmtime_export.definition) - } + unsafe { self.instance.table_index(&*self.wasmtime_export.definition) } } /// Returns the table element value at `index`. @@ -356,8 +347,8 @@ impl Table { /// Returns `None` if `index` is out of bounds. pub fn get(&self, index: u32) -> Option { let table_index = self.wasmtime_table_index(); - let item = self.wasmtime_handle.table_get(table_index, index)?; - Some(from_checked_anyfunc(item, &self.store)) + let item = self.instance.table_get(table_index, index)?; + Some(from_checked_anyfunc(item, &self.instance.store)) } /// Writes the `val` provided into `index` within this table. @@ -368,8 +359,8 @@ impl Table { /// the right type to be stored in this table. pub fn set(&self, index: u32, val: Val) -> Result<()> { let table_index = self.wasmtime_table_index(); - let item = into_checked_anyfunc(val, &self.store)?; - set_table_item(&self.wasmtime_handle, table_index, index, item) + let item = into_checked_anyfunc(val, &self.instance.store)?; + set_table_item(&self.instance, table_index, index, item) } /// Returns the current size of this table. @@ -387,12 +378,11 @@ impl Table { /// error if `init` is not of the right type. pub fn grow(&self, delta: u32, init: Val) -> Result { let index = self.wasmtime_table_index(); - let item = into_checked_anyfunc(init, &self.store)?; - if let Some(len) = self.wasmtime_handle.clone().table_grow(index, delta) { - let mut wasmtime_handle = self.wasmtime_handle.clone(); + let item = into_checked_anyfunc(init, &self.instance.store)?; + if let Some(len) = self.instance.table_grow(index, delta) { for i in 0..delta { let i = len - (delta - i); - set_table_item(&mut wasmtime_handle, index, i, item.clone())?; + set_table_item(&self.instance, index, i, item.clone())?; } Ok(len) } else { @@ -414,7 +404,7 @@ impl Table { src_index: u32, len: u32, ) -> Result<()> { - if !Store::same(&dst_table.store, &src_table.store) { + if !Store::same(&dst_table.instance.store, &src_table.instance.store) { bail!("cross-`Store` table copies are not supported"); } @@ -423,10 +413,10 @@ impl Table { // come from different modules. let dst_table_index = dst_table.wasmtime_table_index(); - let dst_table = dst_table.wasmtime_handle.get_defined_table(dst_table_index); + let dst_table = dst_table.instance.get_defined_table(dst_table_index); let src_table_index = src_table.wasmtime_table_index(); - let src_table = src_table.wasmtime_handle.get_defined_table(src_table_index); + let src_table = src_table.instance.get_defined_table(src_table_index); runtime::Table::copy(dst_table, src_table, dst_index, src_index, len) .map_err(Trap::from_jit)?; @@ -435,12 +425,10 @@ impl Table { pub(crate) fn from_wasmtime_table( wasmtime_export: wasmtime_runtime::ExportTable, - store: &Store, - wasmtime_handle: wasmtime_runtime::InstanceHandle, + instance: StoreInstanceHandle, ) -> Table { Table { - store: store.clone(), - wasmtime_handle, + instance, wasmtime_export, } } @@ -654,8 +642,7 @@ impl Table { /// [open an issue]: https://github.com/bytecodealliance/wasmtime/issues/new #[derive(Clone)] pub struct Memory { - store: Store, - wasmtime_handle: InstanceHandle, + instance: StoreInstanceHandle, wasmtime_export: wasmtime_runtime::ExportMemory, } @@ -683,11 +670,10 @@ impl Memory { /// # } /// ``` pub fn new(store: &Store, ty: MemoryType) -> Memory { - let (wasmtime_handle, wasmtime_export) = + let (instance, wasmtime_export) = generate_memory_export(store, &ty).expect("generated memory"); Memory { - store: store.clone(), - wasmtime_handle, + instance, wasmtime_export, } } @@ -827,22 +813,19 @@ impl Memory { /// ``` pub fn grow(&self, delta: u32) -> Result { let index = self - .wasmtime_handle + .instance .memory_index(unsafe { &*self.wasmtime_export.definition }); - self.wasmtime_handle - .clone() + self.instance .memory_grow(index, delta) .ok_or_else(|| anyhow!("failed to grow memory")) } pub(crate) fn from_wasmtime_memory( wasmtime_export: wasmtime_runtime::ExportMemory, - store: &Store, - wasmtime_handle: wasmtime_runtime::InstanceHandle, + instance: StoreInstanceHandle, ) -> Memory { Memory { - store: store.clone(), - wasmtime_handle, + instance, wasmtime_export, } } diff --git a/crates/api/src/func.rs b/crates/api/src/func.rs index 98d1e54704..ac1838a5e4 100644 --- a/crates/api/src/func.rs +++ b/crates/api/src/func.rs @@ -1,3 +1,5 @@ +use crate::runtime::StoreInner; +use crate::trampoline::StoreInstanceHandle; use crate::{Extern, FuncType, Memory, Store, Trap, Val, ValType}; use anyhow::{bail, ensure, Context as _, Result}; use std::cmp::max; @@ -5,6 +7,7 @@ use std::fmt; use std::mem; use std::panic::{self, AssertUnwindSafe}; use std::ptr; +use std::rc::Weak; use wasmtime_runtime::{Export, InstanceHandle, VMContext, VMFunctionBody}; use wasmtime_runtime::{ExportFunction, VMTrampoline}; @@ -135,8 +138,7 @@ use wasmtime_runtime::{ExportFunction, VMTrampoline}; /// ``` #[derive(Clone)] pub struct Func { - store: Store, - instance: InstanceHandle, + instance: StoreInstanceHandle, export: ExportFunction, trampoline: VMTrampoline, } @@ -176,7 +178,7 @@ macro_rules! getters { // of the closure. Pass the export in so that we can call it. let instance = self.instance.clone(); let export = self.export.clone(); - let max_wasm_stack = self.store.engine().config().max_wasm_stack; + let max_wasm_stack = self.store().engine().config().max_wasm_stack; // ... and then once we've passed the typechecks we can hand out our // object since our `transmute` below should be safe! @@ -236,7 +238,7 @@ impl Func { ty: FuncType, func: impl Fn(Caller<'_>, &[Val], &mut [Val]) -> Result<(), Trap> + 'static, ) -> Self { - let store_clone = store.clone(); + let store_weak = store.weak(); let ty_clone = ty.clone(); // Create our actual trampoline function which translates from a bunch @@ -255,7 +257,7 @@ impl Func { let mut returns = vec![Val::null(); ty_clone.results().len()]; func( Caller { - store: &store_clone, + store: &store_weak, caller_vmctx, }, &args, @@ -281,7 +283,6 @@ impl Func { let (instance, export, trampoline) = crate::trampoline::generate_func_export(&ty, func, store).expect("generated func"); Func { - store: store.clone(), instance, export, trampoline, @@ -483,6 +484,7 @@ impl Func { // Signatures should always be registered in the store's registry of // shared signatures, so we should be able to unwrap safely here. let sig = self + .instance .store .compiler() .signatures() @@ -498,6 +500,7 @@ impl Func { /// Returns the number of parameters that this function takes. pub fn param_arity(&self) -> usize { let sig = self + .instance .store .compiler() .signatures() @@ -509,6 +512,7 @@ impl Func { /// Returns the number of results this function produces. pub fn result_arity(&self) -> usize { let sig = self + .instance .store .compiler() .signatures() @@ -549,7 +553,7 @@ impl Func { if arg.ty() != *ty { bail!("argument type mismatch"); } - if !arg.comes_from_same_store(&self.store) { + if !arg.comes_from_same_store(&self.instance.store) { bail!("cross-`Store` values are not currently supported"); } unsafe { @@ -561,7 +565,7 @@ impl Func { if let Err(error) = unsafe { wasmtime_runtime::catch_traps( self.export.vmctx, - self.store.engine().config().max_wasm_stack, + self.instance.store.engine().config().max_wasm_stack, || { (self.trampoline)( self.export.vmctx, @@ -593,8 +597,7 @@ impl Func { pub(crate) fn from_wasmtime_function( export: wasmtime_runtime::ExportFunction, - store: &Store, - instance: InstanceHandle, + instance: StoreInstanceHandle, ) -> Self { // Each function signature in a module should have a trampoline stored // on that module as well, so unwrap the result here since otherwise @@ -607,7 +610,6 @@ impl Func { instance, export, trampoline, - store: store.clone(), } } @@ -734,7 +736,7 @@ impl Func { } pub(crate) fn store(&self) -> &Store { - &self.store + &self.instance.store } } @@ -970,7 +972,23 @@ pub trait IntoFunc { /// the caller's memory until interface types has been fully standardized and /// implemented. pub struct Caller<'a> { - store: &'a Store, + // Note that this is a `Weak` pointer instead of a `&'a Store`, + // intentionally so. This allows us to break an `Rc` cycle which would + // otherwise look like this: + // + // * A `Store` object ... + // * ... owns all `InstanceHandle` objects ... + // * ... which are created in `Func::wrap` with custom host data ... + // * ... where the custom host data needs to point to `Store` to be stored + // here + // + // This `Rc` cycle means that we would never actually reclaim any memory or + // deallocate any instances. To break this cycle we use a weak pointer here + // which points back to `Store`. A `Caller` should only ever be usable + // when the original `Store` is alive, however, so this should always be an + // upgrade-able pointer. Alternative solutions or other ideas to break this + // cycle would be most welcome! + store: &'a Weak, caller_vmctx: *mut VMContext, } @@ -1004,7 +1022,14 @@ impl Caller<'_> { Some(Export::Memory(m)) => m, _ => return None, }; - let mem = Memory::from_wasmtime_memory(export, self.store, instance); + // Our `Weak` pointer is used only to break a cycle where `Store` + // stores instance handles which have this weak pointer as their + // custom host data. This function should only be invoke-able while + // the `Store` is active, so this upgrade should always succeed. + debug_assert!(self.store.upgrade().is_some()); + let handle = + Store::from_inner(self.store.upgrade()?).existing_instance_handle(instance); + let mem = Memory::from_wasmtime_memory(export, handle); Some(Extern::Memory(mem)) } } @@ -1057,8 +1082,8 @@ macro_rules! impl_into_func { // Double-check ourselves in debug mode, but we control // the `Any` here so an unsafe downcast should also // work. - debug_assert!(state.is::<(F, Store)>()); - let (func, store) = &*(state as *const _ as *const (F, Store)); + debug_assert!(state.is::<(F, Weak)>()); + let (func, store) = &*(state as *const _ as *const (F, Weak)); panic::catch_unwind(AssertUnwindSafe(|| { func( Caller { store, caller_vmctx }, @@ -1102,7 +1127,7 @@ macro_rules! impl_into_func { let mut ret = Vec::new(); R::push(&mut ret); let ty = FuncType::new(_args.into(), ret.into()); - let store_clone = store.clone(); + let store_weak = store.weak(); unsafe { let trampoline = trampoline::<$($args,)* R>; let (instance, export) = crate::trampoline::generate_raw_func_export( @@ -1113,11 +1138,10 @@ macro_rules! impl_into_func { ), trampoline, store, - Box::new((self, store_clone)), + Box::new((self, store_weak)), ) .expect("failed to generate export"); Func { - store: store.clone(), instance, export, trampoline, diff --git a/crates/api/src/instance.rs b/crates/api/src/instance.rs index ec883d923e..02c535e773 100644 --- a/crates/api/src/instance.rs +++ b/crates/api/src/instance.rs @@ -1,12 +1,9 @@ -use crate::externals::{Export, Extern, Global, Memory, Table}; -use crate::func::Func; -use crate::module::Module; -use crate::runtime::{Config, Store}; -use crate::trap::Trap; +use crate::trampoline::StoreInstanceHandle; +use crate::{Export, Extern, Func, Global, Memory, Module, Store, Table, Trap}; use anyhow::{bail, Error, Result}; use std::any::Any; use wasmtime_jit::{CompiledModule, Resolver}; -use wasmtime_runtime::{InstanceHandle, InstantiationError, SignatureRegistry}; +use wasmtime_runtime::{InstantiationError, SignatureRegistry}; struct SimpleResolver<'a> { imports: &'a [Extern], @@ -21,22 +18,35 @@ impl Resolver for SimpleResolver<'_> { } fn instantiate( - config: &Config, + store: &Store, compiled_module: &CompiledModule, imports: &[Extern], sig_registry: &SignatureRegistry, host: Box, -) -> Result { +) -> Result { let mut resolver = SimpleResolver { imports }; unsafe { - let instance = compiled_module - .instantiate( + let config = store.engine().config(); + let instance = compiled_module.instantiate( + &mut resolver, + sig_registry, + config.memory_creator.as_ref().map(|a| a as _), + host, + )?; + + // After we've created the `InstanceHandle` we still need to run + // initialization to set up data/elements/etc. We do this after adding + // the `InstanceHandle` to the store though. This is required for safety + // because the start function (for example) may trap, but element + // initializers may have run which placed elements into other instance's + // tables. This means that from this point on, regardless of whether + // initialization is successful, we need to keep the instance alive. + let instance = store.add_instance(instance); + instance + .initialize( config.validating_config.operator_config.enable_bulk_memory, - &mut resolver, - sig_registry, - config.memory_creator.as_ref().map(|a| a as _), config.max_wasm_stack, - host, + &compiled_module.data_initializers(), ) .map_err(|e| -> Error { match e { @@ -68,7 +78,7 @@ fn instantiate( /// call any code or execute anything! #[derive(Clone)] pub struct Instance { - pub(crate) instance_handle: InstanceHandle, + pub(crate) handle: StoreInstanceHandle, module: Module, } @@ -137,9 +147,8 @@ impl Instance { } let info = module.register_frame_info(); - let config = store.engine().config(); - let instance_handle = instantiate( - config, + let handle = instantiate( + store, module.compiled_module(), imports, store.compiler().signatures(), @@ -147,7 +156,7 @@ impl Instance { )?; Ok(Instance { - instance_handle, + handle, module: module.clone(), }) } @@ -164,15 +173,11 @@ impl Instance { pub fn exports<'instance>( &'instance self, ) -> impl ExactSizeIterator> + 'instance { - let instance_handle = &self.instance_handle; - let store = self.module.store(); - self.instance_handle - .exports() - .map(move |(name, entity_index)| { - let export = instance_handle.lookup_by_declaration(entity_index); - let extern_ = Extern::from_wasmtime_export(export, store, instance_handle.clone()); - Export::new(name, extern_) - }) + self.handle.exports().map(move |(name, entity_index)| { + let export = self.handle.lookup_by_declaration(entity_index); + let extern_ = Extern::from_wasmtime_export(export, self.handle.clone()); + Export::new(name, extern_) + }) } /// Looks up an exported [`Extern`] value by name. @@ -182,12 +187,8 @@ impl Instance { /// /// Returns `None` if there was no export named `name`. pub fn get_export(&self, name: &str) -> Option { - let export = self.instance_handle.lookup(&name)?; - Some(Extern::from_wasmtime_export( - export, - self.module.store(), - self.instance_handle.clone(), - )) + let export = self.handle.lookup(&name)?; + Some(Extern::from_wasmtime_export(export, self.handle.clone())) } /// Looks up an exported [`Func`] value by name. @@ -221,9 +222,4 @@ impl Instance { pub fn get_global(&self, name: &str) -> Option { self.get_export(name)?.into_global() } - - #[doc(hidden)] - pub fn handle(&self) -> &InstanceHandle { - &self.instance_handle - } } diff --git a/crates/api/src/runtime.rs b/crates/api/src/runtime.rs index 716b1324ba..6592cc93df 100644 --- a/crates/api/src/runtime.rs +++ b/crates/api/src/runtime.rs @@ -1,18 +1,18 @@ use crate::externals::MemoryCreator; -use crate::trampoline::MemoryCreatorProxy; +use crate::trampoline::{MemoryCreatorProxy, StoreInstanceHandle}; use anyhow::{bail, Result}; use std::cell::RefCell; use std::cmp::min; use std::fmt; use std::path::Path; -use std::rc::Rc; +use std::rc::{Rc, Weak}; use std::sync::Arc; use wasmparser::{OperatorValidatorConfig, ValidatingParserConfig}; use wasmtime_environ::settings::{self, Configurable}; use wasmtime_environ::{CacheConfig, Tunables}; use wasmtime_jit::{native, CompilationStrategy, Compiler}; use wasmtime_profiling::{JitDumpAgent, NullProfilerAgent, ProfilingAgent, VTuneAgent}; -use wasmtime_runtime::{debug_builtins, RuntimeMemoryCreator, VMInterrupts}; +use wasmtime_runtime::{debug_builtins, InstanceHandle, RuntimeMemoryCreator, VMInterrupts}; // Runtime Environment @@ -550,13 +550,13 @@ impl Engine { /// ocnfiguration (see [`Config`] for more information). #[derive(Clone)] pub struct Store { - // FIXME(#777) should be `Arc` and this type should be thread-safe inner: Rc, } -struct StoreInner { +pub(crate) struct StoreInner { engine: Engine, compiler: RefCell, + instances: RefCell>, } impl Store { @@ -573,10 +573,15 @@ impl Store { inner: Rc::new(StoreInner { engine: engine.clone(), compiler: RefCell::new(compiler), + instances: RefCell::new(Vec::new()), }), } } + pub(crate) fn from_inner(inner: Rc) -> Store { + Store { inner } + } + /// Returns the [`Engine`] that this store is associated with. pub fn engine(&self) -> &Engine { &self.inner.engine @@ -595,6 +600,31 @@ impl Store { self.inner.compiler.borrow_mut() } + pub(crate) unsafe fn add_instance(&self, handle: InstanceHandle) -> StoreInstanceHandle { + self.inner.instances.borrow_mut().push(handle.clone()); + StoreInstanceHandle { + store: self.clone(), + handle, + } + } + + pub(crate) fn existing_instance_handle(&self, handle: InstanceHandle) -> StoreInstanceHandle { + debug_assert!(self + .inner + .instances + .borrow() + .iter() + .any(|i| i.vmctx_ptr() == handle.vmctx_ptr())); + StoreInstanceHandle { + store: self.clone(), + handle, + } + } + + pub(crate) fn weak(&self) -> Weak { + Rc::downgrade(&self.inner) + } + /// Returns whether the stores `a` and `b` refer to the same underlying /// `Store`. /// @@ -703,6 +733,16 @@ impl Default for Store { } } +impl Drop for StoreInner { + fn drop(&mut self) { + for instance in self.instances.get_mut().iter() { + unsafe { + instance.dealloc(); + } + } + } +} + /// A threadsafe handle used to interrupt instances executing within a /// particular `Store`. /// diff --git a/crates/api/src/trampoline/create_handle.rs b/crates/api/src/trampoline/create_handle.rs index 3fa2c68410..428c6c35ac 100644 --- a/crates/api/src/trampoline/create_handle.rs +++ b/crates/api/src/trampoline/create_handle.rs @@ -1,9 +1,10 @@ //! Support for a calling of an imported function. -use crate::runtime::Store; +use crate::trampoline::StoreInstanceHandle; +use crate::Store; use anyhow::Result; use std::any::Any; -use std::collections::{HashMap, HashSet}; +use std::collections::HashMap; use std::sync::Arc; use wasmtime_environ::entity::PrimaryMap; use wasmtime_environ::wasm::DefinedFuncIndex; @@ -18,15 +19,13 @@ pub(crate) fn create_handle( finished_functions: PrimaryMap, trampolines: HashMap, state: Box, -) -> Result { +) -> Result { let imports = Imports::new( - HashSet::new(), PrimaryMap::new(), PrimaryMap::new(), PrimaryMap::new(), PrimaryMap::new(), ); - let data_initializers = Vec::new(); // Compute indices into the shared signature table. let signatures = module @@ -37,24 +36,17 @@ pub(crate) fn create_handle( .collect::>(); unsafe { - Ok(InstanceHandle::new( + let handle = InstanceHandle::new( Arc::new(module), finished_functions.into_boxed_slice(), trampolines, imports, store.memory_creator(), - &data_initializers, signatures.into_boxed_slice(), None, - store - .engine() - .config() - .validating_config - .operator_config - .enable_bulk_memory, state, store.compiler().interrupts().clone(), - store.engine().config().max_wasm_stack, - )?) + )?; + Ok(store.add_instance(handle)) } } diff --git a/crates/api/src/trampoline/func.rs b/crates/api/src/trampoline/func.rs index 0c5f36c625..c54f5f0fcb 100644 --- a/crates/api/src/trampoline/func.rs +++ b/crates/api/src/trampoline/func.rs @@ -1,6 +1,7 @@ //! Support for a calling of an imported function. use super::create_handle::create_handle; +use crate::trampoline::StoreInstanceHandle; use crate::{FuncType, Store, Trap}; use anyhow::{bail, Result}; use std::any::Any; @@ -203,7 +204,7 @@ pub fn create_handle_with_function( ft: &FuncType, func: Box Result<(), Trap>>, store: &Store, -) -> Result<(InstanceHandle, VMTrampoline)> { +) -> Result<(StoreInstanceHandle, VMTrampoline)> { let isa = { let isa_builder = native::builder(); let flag_builder = settings::builder(); @@ -267,7 +268,7 @@ pub unsafe fn create_handle_with_raw_function( trampoline: VMTrampoline, store: &Store, state: Box, -) -> Result { +) -> Result { let isa = { let isa_builder = native::builder(); let flag_builder = settings::builder(); diff --git a/crates/api/src/trampoline/global.rs b/crates/api/src/trampoline/global.rs index db2366fc92..4fa13e42bd 100644 --- a/crates/api/src/trampoline/global.rs +++ b/crates/api/src/trampoline/global.rs @@ -1,12 +1,12 @@ use super::create_handle::create_handle; +use crate::trampoline::StoreInstanceHandle; use crate::Store; use crate::{GlobalType, Mutability, Val}; use anyhow::{bail, Result}; use wasmtime_environ::entity::PrimaryMap; use wasmtime_environ::{wasm, EntityIndex, Module}; -use wasmtime_runtime::InstanceHandle; -pub fn create_global(store: &Store, gt: &GlobalType, val: Val) -> Result { +pub fn create_global(store: &Store, gt: &GlobalType, val: Val) -> Result { let global = wasm::Global { ty: match gt.content().get_wasmtime_type() { Some(t) => t, diff --git a/crates/api/src/trampoline/memory.rs b/crates/api/src/trampoline/memory.rs index b3a2bf6a46..a5fe8f6ffd 100644 --- a/crates/api/src/trampoline/memory.rs +++ b/crates/api/src/trampoline/memory.rs @@ -1,17 +1,19 @@ use super::create_handle::create_handle; use crate::externals::{LinearMemory, MemoryCreator}; +use crate::trampoline::StoreInstanceHandle; use crate::Store; use crate::{Limits, MemoryType}; use anyhow::Result; use wasmtime_environ::entity::PrimaryMap; use wasmtime_environ::{wasm, EntityIndex, MemoryPlan, Module, WASM_PAGE_SIZE}; -use wasmtime_runtime::{ - InstanceHandle, RuntimeLinearMemory, RuntimeMemoryCreator, VMMemoryDefinition, -}; +use wasmtime_runtime::{RuntimeLinearMemory, RuntimeMemoryCreator, VMMemoryDefinition}; use std::sync::Arc; -pub fn create_handle_with_memory(store: &Store, memory: &MemoryType) -> Result { +pub fn create_handle_with_memory( + store: &Store, + memory: &MemoryType, +) -> Result { let mut module = Module::new(); let memory = wasm::Memory { diff --git a/crates/api/src/trampoline/mod.rs b/crates/api/src/trampoline/mod.rs index 563924e413..a3a6aac71a 100644 --- a/crates/api/src/trampoline/mod.rs +++ b/crates/api/src/trampoline/mod.rs @@ -15,14 +15,42 @@ use self::table::create_handle_with_table; use crate::{FuncType, GlobalType, MemoryType, Store, TableType, Trap, Val}; use anyhow::Result; use std::any::Any; -use wasmtime_runtime::{VMContext, VMFunctionBody, VMTrampoline}; +use std::ops::Deref; +use wasmtime_runtime::{InstanceHandle, VMContext, VMFunctionBody, VMTrampoline}; + +/// A wrapper around `wasmtime_runtime::InstanceHandle` which pairs it with the +/// `Store` that it's rooted within. The instance is deallocated when `Store` is +/// deallocated, so this is a safe handle in terms of memory management for the +/// `Store`. +pub struct StoreInstanceHandle { + pub store: Store, + pub handle: InstanceHandle, +} + +impl Clone for StoreInstanceHandle { + fn clone(&self) -> StoreInstanceHandle { + StoreInstanceHandle { + store: self.store.clone(), + // Note should be safe because the lifetime of the instance handle + // is tied to the `Store` which this is paired with. + handle: unsafe { self.handle.clone() }, + } + } +} + +impl Deref for StoreInstanceHandle { + type Target = InstanceHandle; + fn deref(&self) -> &InstanceHandle { + &self.handle + } +} pub fn generate_func_export( ft: &FuncType, func: Box Result<(), Trap>>, store: &Store, ) -> Result<( - wasmtime_runtime::InstanceHandle, + StoreInstanceHandle, wasmtime_runtime::ExportFunction, VMTrampoline, )> { @@ -42,10 +70,7 @@ pub unsafe fn generate_raw_func_export( trampoline: VMTrampoline, store: &Store, state: Box, -) -> Result<( - wasmtime_runtime::InstanceHandle, - wasmtime_runtime::ExportFunction, -)> { +) -> Result<(StoreInstanceHandle, wasmtime_runtime::ExportFunction)> { let instance = func::create_handle_with_raw_function(ft, func, trampoline, store, state)?; match instance.lookup("trampoline").expect("trampoline export") { wasmtime_runtime::Export::Function(f) => Ok((instance, f)), @@ -57,10 +82,7 @@ pub fn generate_global_export( store: &Store, gt: &GlobalType, val: Val, -) -> Result<( - wasmtime_runtime::InstanceHandle, - wasmtime_runtime::ExportGlobal, -)> { +) -> Result<(StoreInstanceHandle, wasmtime_runtime::ExportGlobal)> { let instance = create_global(store, gt, val)?; match instance.lookup("global").expect("global export") { wasmtime_runtime::Export::Global(g) => Ok((instance, g)), @@ -71,10 +93,7 @@ pub fn generate_global_export( pub fn generate_memory_export( store: &Store, m: &MemoryType, -) -> Result<( - wasmtime_runtime::InstanceHandle, - wasmtime_runtime::ExportMemory, -)> { +) -> Result<(StoreInstanceHandle, wasmtime_runtime::ExportMemory)> { let instance = create_handle_with_memory(store, m)?; match instance.lookup("memory").expect("memory export") { wasmtime_runtime::Export::Memory(m) => Ok((instance, m)), @@ -85,10 +104,7 @@ pub fn generate_memory_export( pub fn generate_table_export( store: &Store, t: &TableType, -) -> Result<( - wasmtime_runtime::InstanceHandle, - wasmtime_runtime::ExportTable, -)> { +) -> Result<(StoreInstanceHandle, wasmtime_runtime::ExportTable)> { let instance = create_handle_with_table(store, t)?; match instance.lookup("table").expect("table export") { wasmtime_runtime::Export::Table(t) => Ok((instance, t)), diff --git a/crates/api/src/trampoline/table.rs b/crates/api/src/trampoline/table.rs index eb6c12dd65..4919e0e09c 100644 --- a/crates/api/src/trampoline/table.rs +++ b/crates/api/src/trampoline/table.rs @@ -1,12 +1,12 @@ use super::create_handle::create_handle; +use crate::trampoline::StoreInstanceHandle; use crate::Store; use crate::{TableType, ValType}; use anyhow::{bail, Result}; use wasmtime_environ::entity::PrimaryMap; use wasmtime_environ::{wasm, EntityIndex, Module}; -use wasmtime_runtime::InstanceHandle; -pub fn create_handle_with_table(store: &Store, table: &TableType) -> Result { +pub fn create_handle_with_table(store: &Store, table: &TableType) -> Result { let mut module = Module::new(); let table = wasm::Table { diff --git a/crates/api/src/unix.rs b/crates/api/src/unix.rs index f17eecfe13..22fa392597 100644 --- a/crates/api/src/unix.rs +++ b/crates/api/src/unix.rs @@ -26,6 +26,6 @@ impl InstanceExt for Instance { where H: 'static + Fn(libc::c_int, *const libc::siginfo_t, *const libc::c_void) -> bool, { - self.instance_handle.clone().set_signal_handler(handler); + self.handle.set_signal_handler(handler); } } diff --git a/crates/api/src/values.rs b/crates/api/src/values.rs index a9387a7338..8d1d7f55be 100644 --- a/crates/api/src/values.rs +++ b/crates/api/src/values.rs @@ -224,6 +224,7 @@ pub(crate) fn from_checked_anyfunc( signature: item.type_index, vmctx: item.vmctx, }; - let f = Func::from_wasmtime_function(export, store, instance_handle); + let instance = store.existing_instance_handle(instance_handle); + let f = Func::from_wasmtime_function(export, instance); Val::FuncRef(f) } diff --git a/crates/api/src/windows.rs b/crates/api/src/windows.rs index 962cae7458..5725007d53 100644 --- a/crates/api/src/windows.rs +++ b/crates/api/src/windows.rs @@ -26,6 +26,6 @@ impl InstanceExt for Instance { where H: 'static + Fn(winapi::um::winnt::PEXCEPTION_POINTERS) -> bool, { - self.instance_handle.clone().set_signal_handler(handler); + self.handle.set_signal_handler(handler); } } diff --git a/crates/jit/src/imports.rs b/crates/jit/src/imports.rs index 743b6e512e..d2ef3536fc 100644 --- a/crates/jit/src/imports.rs +++ b/crates/jit/src/imports.rs @@ -2,14 +2,13 @@ use crate::resolver::Resolver; use more_asserts::assert_ge; -use std::collections::HashSet; use std::convert::TryInto; use wasmtime_environ::entity::PrimaryMap; use wasmtime_environ::wasm::{Global, GlobalInit, Memory, Table, TableElementType}; use wasmtime_environ::{EntityIndex, MemoryPlan, MemoryStyle, Module, TablePlan}; use wasmtime_runtime::{ - Export, Imports, InstanceHandle, LinkError, SignatureRegistry, VMFunctionImport, - VMGlobalImport, VMMemoryImport, VMTableImport, + Export, Imports, LinkError, SignatureRegistry, VMFunctionImport, VMGlobalImport, + VMMemoryImport, VMTableImport, }; /// This function allows to match all imports of a `Module` with concrete definitions provided by @@ -21,8 +20,6 @@ pub fn resolve_imports( signatures: &SignatureRegistry, resolver: &mut dyn Resolver, ) -> Result { - let mut dependencies = HashSet::new(); - let mut function_imports = PrimaryMap::with_capacity(module.local.num_imported_funcs); let mut table_imports = PrimaryMap::with_capacity(module.local.num_imported_tables); let mut memory_imports = PrimaryMap::with_capacity(module.local.num_imported_memories); @@ -45,7 +42,6 @@ pub fn resolve_imports( module_name, field_name, signature, import_signature ))); } - dependencies.insert(unsafe { InstanceHandle::from_vmctx(f.vmctx) }); function_imports.push(VMFunctionImport { body: f.address, vmctx: f.vmctx, @@ -73,7 +69,6 @@ pub fn resolve_imports( module_name, field_name, ))); } - dependencies.insert(unsafe { InstanceHandle::from_vmctx(t.vmctx) }); table_imports.push(VMTableImport { from: t.definition, vmctx: t.vmctx, @@ -115,7 +110,6 @@ pub fn resolve_imports( } assert_ge!(m.memory.offset_guard_size, import_memory.offset_guard_size); - dependencies.insert(unsafe { InstanceHandle::from_vmctx(m.vmctx) }); memory_imports.push(VMMemoryImport { from: m.definition, vmctx: m.vmctx, @@ -143,7 +137,6 @@ pub fn resolve_imports( module_name, field_name ))); } - dependencies.insert(unsafe { InstanceHandle::from_vmctx(g.vmctx) }); global_imports.push(VMGlobalImport { from: g.definition }); } (EntityIndex::Global(_), Some(_)) => { @@ -162,7 +155,6 @@ pub fn resolve_imports( } Ok(Imports::new( - dependencies, function_imports, table_imports, memory_imports, diff --git a/crates/jit/src/instantiate.rs b/crates/jit/src/instantiate.rs index c7bcb73df6..e3f1a5eb96 100644 --- a/crates/jit/src/instantiate.rs +++ b/crates/jit/src/instantiate.rs @@ -204,21 +204,11 @@ impl CompiledModule { /// See `InstanceHandle::new` pub unsafe fn instantiate( &self, - is_bulk_memory: bool, resolver: &mut dyn Resolver, sig_registry: &SignatureRegistry, mem_creator: Option<&dyn RuntimeMemoryCreator>, - max_wasm_stack: usize, host_state: Box, ) -> Result { - let data_initializers = self - .data_initializers - .iter() - .map(|init| DataInitializer { - location: init.location.clone(), - data: &*init.data, - }) - .collect::>(); let imports = resolve_imports(&self.module, &sig_registry, resolver)?; InstanceHandle::new( Arc::clone(&self.module), @@ -226,16 +216,24 @@ impl CompiledModule { self.trampolines.clone(), imports, mem_creator, - &data_initializers, self.signatures.clone(), self.dbg_jit_registration.as_ref().map(|r| Rc::clone(&r)), - is_bulk_memory, host_state, self.interrupts.clone(), - max_wasm_stack, ) } + /// Returns data initializers to pass to `InstanceHandle::initialize` + pub fn data_initializers(&self) -> Vec> { + self.data_initializers + .iter() + .map(|init| DataInitializer { + location: init.location.clone(), + data: &*init.data, + }) + .collect() + } + /// Return a reference-counting pointer to a module. pub fn module(&self) -> &Arc { &self.module diff --git a/crates/runtime/src/imports.rs b/crates/runtime/src/imports.rs index 0214ce67b0..14360a7036 100644 --- a/crates/runtime/src/imports.rs +++ b/crates/runtime/src/imports.rs @@ -1,15 +1,10 @@ -use crate::instance::InstanceHandle; use crate::vmcontext::{VMFunctionImport, VMGlobalImport, VMMemoryImport, VMTableImport}; -use std::collections::HashSet; use wasmtime_environ::entity::{BoxedSlice, PrimaryMap}; use wasmtime_environ::wasm::{FuncIndex, GlobalIndex, MemoryIndex, TableIndex}; /// Resolved import pointers. #[derive(Clone)] pub struct Imports { - /// The set of instances that the imports depend on. - pub dependencies: HashSet, - /// Resolved addresses for imported functions. pub functions: BoxedSlice, @@ -26,14 +21,12 @@ pub struct Imports { impl Imports { /// Construct a new `Imports` instance. pub fn new( - dependencies: HashSet, function_imports: PrimaryMap, table_imports: PrimaryMap, memory_imports: PrimaryMap, global_imports: PrimaryMap, ) -> Self { Self { - dependencies, functions: function_imports.into_boxed_slice(), tables: table_imports.into_boxed_slice(), memories: memory_imports.into_boxed_slice(), @@ -44,7 +37,6 @@ impl Imports { /// Construct a new `Imports` instance with no imports. pub fn none() -> Self { Self { - dependencies: HashSet::new(), functions: PrimaryMap::new().into_boxed_slice(), tables: PrimaryMap::new().into_boxed_slice(), memories: PrimaryMap::new().into_boxed_slice(), diff --git a/crates/runtime/src/instance.rs b/crates/runtime/src/instance.rs index ef5e176600..bbf4ebfaba 100644 --- a/crates/runtime/src/instance.rs +++ b/crates/runtime/src/instance.rs @@ -20,7 +20,7 @@ use more_asserts::assert_lt; use std::alloc::{self, Layout}; use std::any::Any; use std::cell::{Cell, RefCell}; -use std::collections::{HashMap, HashSet}; +use std::collections::HashMap; use std::convert::TryFrom; use std::rc::Rc; use std::sync::Arc; @@ -39,7 +39,7 @@ cfg_if::cfg_if! { impl InstanceHandle { /// Set a custom signal handler - pub fn set_signal_handler(&mut self, handler: H) + pub fn set_signal_handler(&self, handler: H) where H: 'static + Fn(libc::c_int, *const libc::siginfo_t, *const libc::c_void) -> bool, { @@ -51,7 +51,7 @@ cfg_if::cfg_if! { impl InstanceHandle { /// Set a custom signal handler - pub fn set_signal_handler(&mut self, handler: H) + pub fn set_signal_handler(&self, handler: H) where H: 'static + Fn(winapi::um::winnt::PEXCEPTION_POINTERS) -> bool, { @@ -66,14 +66,6 @@ cfg_if::cfg_if! { /// This is repr(C) to ensure that the vmctx field is last. #[repr(C)] pub(crate) struct Instance { - /// The number of references to this `Instance`. - refcount: Cell, - - /// `Instance`s from which this `Instance` imports. These won't - /// create reference cycles because wasm instances can't cyclically - /// import from each other. - dependencies: HashSet, - /// The `Module` this `Instance` was instantiated from. module: Arc, @@ -878,13 +870,10 @@ impl InstanceHandle { trampolines: HashMap, imports: Imports, mem_creator: Option<&dyn RuntimeMemoryCreator>, - data_initializers: &[DataInitializer<'_>], vmshared_signatures: BoxedSlice, dbg_jit_registration: Option>, - is_bulk_memory: bool, host_state: Box, interrupts: Arc, - max_wasm_stack: usize, ) -> Result { let tables = create_tables(&module); let memories = create_memories(&module, mem_creator.unwrap_or(&DefaultMemoryCreator {}))?; @@ -909,8 +898,6 @@ impl InstanceHandle { let handle = { let instance = Instance { - refcount: Cell::new(1), - dependencies: imports.dependencies, module, offsets, memories, @@ -983,30 +970,44 @@ impl InstanceHandle { ); *instance.interrupts() = &*instance.interrupts; + // Ensure that our signal handlers are ready for action. + // TODO: Move these calls out of `InstanceHandle`. + traphandlers::init(); + + // Perform infallible initialization in this constructor, while fallible + // initialization is deferred to the `initialize` method. + initialize_passive_elements(instance); + initialize_globals(instance); + + Ok(handle) + } + + /// Finishes the instantiation process started by `Instance::new`. + /// + /// Only safe to call immediately after instantiation. + pub unsafe fn initialize( + &self, + is_bulk_memory: bool, + max_wasm_stack: usize, + data_initializers: &[DataInitializer<'_>], + ) -> Result<(), InstantiationError> { // Check initializer bounds before initializing anything. Only do this // when bulk memory is disabled, since the bulk memory proposal changes // instantiation such that the intermediate results of failed // initializations are visible. if !is_bulk_memory { - check_table_init_bounds(instance)?; - check_memory_init_bounds(instance, data_initializers)?; + check_table_init_bounds(self.instance())?; + check_memory_init_bounds(self.instance(), data_initializers)?; } - // Apply the initializers. - initialize_tables(instance)?; - initialize_passive_elements(instance); - initialize_memories(instance, data_initializers)?; - initialize_globals(instance); + // Apply fallible initializers. Note that this can "leak" state even if + // it fails. + initialize_tables(self.instance())?; + initialize_memories(self.instance(), data_initializers)?; - // Ensure that our signal handlers are ready for action. - // TODO: Move these calls out of `InstanceHandle`. - traphandlers::init(); - - // The WebAssembly spec specifies that the start function is - // invoked automatically at instantiation time. - instance.invoke_start_function(max_wasm_stack)?; - - Ok(handle) + // And finally, invoke the start function. + self.instance().invoke_start_function(max_wasm_stack)?; + Ok(()) } /// Create a new `InstanceHandle` pointing at the instance @@ -1017,7 +1018,6 @@ impl InstanceHandle { /// be a `VMContext` allocated as part of an `Instance`. pub unsafe fn from_vmctx(vmctx: *mut VMContext) -> Self { let instance = (&mut *vmctx).instance(); - instance.refcount.set(instance.refcount.get() + 1); Self { instance: instance as *const Instance as *mut Instance, } @@ -1130,30 +1130,29 @@ impl InstanceHandle { pub(crate) fn instance(&self) -> &Instance { unsafe { &*(self.instance as *const Instance) } } -} -impl Clone for InstanceHandle { - fn clone(&self) -> Self { - let instance = self.instance(); - instance.refcount.set(instance.refcount.get() + 1); - Self { + /// Returns a clone of this instance. + /// + /// This is unsafe because the returned handle here is just a cheap clone + /// of the internals, there's no lifetime tracking around its validity. + /// You'll need to ensure that the returned handles all go out of scope at + /// the same time. + pub unsafe fn clone(&self) -> InstanceHandle { + InstanceHandle { instance: self.instance, } } -} -impl Drop for InstanceHandle { - fn drop(&mut self) { + /// Deallocates memory associated with this instance. + /// + /// Note that this is unsafe because there might be other handles to this + /// `InstanceHandle` elsewhere, and there's nothing preventing usage of + /// this handle after this function is called. + pub unsafe fn dealloc(&self) { let instance = self.instance(); - let count = instance.refcount.get(); - instance.refcount.set(count - 1); - if count == 1 { - let layout = instance.alloc_layout(); - unsafe { - ptr::drop_in_place(self.instance); - alloc::dealloc(self.instance.cast(), layout); - } - } + let layout = instance.alloc_layout(); + ptr::drop_in_place(self.instance); + alloc::dealloc(self.instance.cast(), layout); } } diff --git a/tests/all/func.rs b/tests/all/func.rs index 40616810ed..ea55840564 100644 --- a/tests/all/func.rs +++ b/tests/all/func.rs @@ -39,6 +39,7 @@ fn dtor_runs() { Func::wrap(&store, move || { drop(&a); }); + drop(store); assert_eq!(HITS.load(SeqCst), 1); } @@ -63,7 +64,7 @@ fn dtor_delayed() -> Result<()> { let module = Module::new(&store, &wasm)?; let instance = Instance::new(&module, &[func.into()])?; assert_eq!(HITS.load(SeqCst), 0); - drop(instance); + drop((instance, module, store)); assert_eq!(HITS.load(SeqCst), 1); Ok(()) } diff --git a/tests/all/main.rs b/tests/all/main.rs index 2f819c1a54..95b7d6ba44 100644 --- a/tests/all/main.rs +++ b/tests/all/main.rs @@ -15,4 +15,5 @@ mod memory_creator; mod name; mod stack_overflow; mod traps; +mod use_after_drop; mod wast; diff --git a/tests/all/memory_creator.rs b/tests/all/memory_creator.rs index ed80cc72c9..b53dcd2c82 100644 --- a/tests/all/memory_creator.rs +++ b/tests/all/memory_creator.rs @@ -175,9 +175,9 @@ mod not_for_windows { let tot_pages = *mem_creator.num_total_pages.lock().unwrap(); assert_eq!(tot_pages, 4); - drop(instance1); + drop((instance1, instance2, store, module)); let tot_pages = *mem_creator.num_total_pages.lock().unwrap(); - assert_eq!(tot_pages, 2); + assert_eq!(tot_pages, 0); Ok(()) } diff --git a/tests/all/use_after_drop.rs b/tests/all/use_after_drop.rs new file mode 100644 index 0000000000..b1f45041ce --- /dev/null +++ b/tests/all/use_after_drop.rs @@ -0,0 +1,21 @@ +use anyhow::Result; +use wasmtime::*; + +#[test] +fn use_func_after_drop() -> Result<()> { + let table; + { + let store = Store::default(); + let closed_over_data = String::from("abcd"); + let func = Func::wrap(&store, move || { + assert_eq!(closed_over_data, "abcd"); + }); + let ty = TableType::new(ValType::FuncRef, Limits::new(1, None)); + table = Table::new(&store, ty, Val::AnyRef(AnyRef::Null))?; + table.set(0, func.into())?; + } + let func = table.get(0).unwrap().funcref().unwrap().clone(); + let func = func.get0::<()>()?; + func()?; + Ok(()) +}