From 0a2768c6e7c3279b1a34bab8c813b5fa75e3e8cd Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Thu, 21 Jan 2021 12:17:27 -0800 Subject: [PATCH] Refactor instantiation to be more async-friendly Instantiation right now uses a recursive `instantiate` function since it was relatively easy to write that way, but this is unfortunately not factored in a way friendly to the async implementation in #2434. This commit refactors the function to instead use an iterative loop and refactors code in such a way that it should be easy to rebase #2434 on top of this change. The main goal is to make the body of `Instance::new` as small as possible since it needs to be duplicated with `Instance::new_async`. --- crates/wasmtime/src/instance.rs | 625 ++++++++++++++++++-------------- crates/wasmtime/src/module.rs | 4 + 2 files changed, 365 insertions(+), 264 deletions(-) diff --git a/crates/wasmtime/src/instance.rs b/crates/wasmtime/src/instance.rs index fed2bb29cec7..e2f292bdb26e 100644 --- a/crates/wasmtime/src/instance.rs +++ b/crates/wasmtime/src/instance.rs @@ -1,3 +1,4 @@ +use crate::trampoline::StoreInstanceHandle; use crate::types::matching; use crate::{ Engine, Export, Extern, Func, Global, InstanceType, Memory, Module, Store, Table, Trap, @@ -16,227 +17,6 @@ use wasmtime_runtime::{ VMTableImport, }; -/// Performs all low-level steps necessary for instantiation. -/// -/// This function will take all the arguments and attempt to do everything -/// necessary to instantiate the referenced instance. The trickiness of this -/// function stems from the implementation of the module-linking proposal where -/// we're handling nested instances, interleaved imports/aliases, etc. That's -/// all an internal implementation here ideally though! -/// -/// * `store` - the store we're instantiating into -/// * `compiled_module` - the module that we're instantiating -/// * `all_modules` - the list of all modules that were part of the compilation -/// of `compiled_module`. This is only applicable in the module linking -/// proposal, otherwise this will just be a list containing `compiled_module` -/// itself. -/// * `type` - the type tables produced during compilation which -/// `compiled_module`'s metadata references. -/// * `define_import` - this function, like the name implies, defines an import -/// into the provided builder. The expected entity that it's defining is also -/// passed in for the top-level case where type-checking is performed. This is -/// fallible because type checks may fail. -fn instantiate( - store: &Store, - module: &Module, - define_import: &mut dyn FnMut( - &str, - Option<&str>, - &EntityIndex, - &mut ImportsBuilder<'_>, - ) -> Result<()>, -) -> Result { - store.bump_instance_count()?; - - let compiled_module = module.compiled_module(); - let env_module = compiled_module.module(); - - let mut imports = ImportsBuilder::new(store, module); - for initializer in env_module.initializers.iter() { - match initializer { - // Definition of an import depends on how our parent is providing - // imports, so we delegate to our custom closure. This will resolve - // to fetching from the import list for the top-level module and - // otherwise fetching from each nested instance's argument list for - // submodules. - Initializer::Import { index, name, field } => { - define_import(name, field.as_deref(), index, &mut imports) - .with_context(|| format!("incompatible import type for `{}`", name))?; - } - - // Here we lookup our instance handle, find the right export, - // and then push that item into our own index space. We eschew - // type-checking since only valid modules should reach this point. - Initializer::AliasInstanceExport { instance, export } => { - let export = &imports.instances[*instance][export]; - let item = unsafe { Extern::from_wasmtime_export(export, store) }; - imports.push_extern(&item); - } - - // Oh boy a recursive instantiation! - // - // We use our local index space of modules to find the module to - // instantiate and argument lookup is defined as looking inside of - // `args`. Like above with aliases all type checking is eschewed - // because only valid modules should reach this point. - // - // Note that it's thought that due to the acyclic nature of - // instantiation this can't loop to blow the native stack, and - // validation should in theory ensure this has a bounded depth. - // Despite this we may need to change this to a loop instead of - // recursion one day. - Initializer::Instantiate { module, args } => { - let handle = instantiate( - store, - &imports.modules[*module], - &mut |name, field, _, builder| { - debug_assert!(field.is_none()); - let index = args.get(name).expect("should be present after validation"); - match *index { - EntityIndex::Global(i) => { - builder.globals.push(imports.globals[i]); - } - EntityIndex::Function(i) => { - builder.functions.push(imports.functions[i]); - } - EntityIndex::Table(i) => { - builder.tables.push(imports.tables[i]); - } - EntityIndex::Memory(i) => { - builder.memories.push(imports.memories[i]); - } - EntityIndex::Module(i) => { - builder.modules.push(imports.modules[i].clone()); - } - EntityIndex::Instance(i) => { - builder.instances.push(imports.instances[i].clone()); - } - } - Ok(()) - }, - )?; - imports.instances.push(handle); - } - - // A new module is being defined, and the source of this module is - // our module's list of closed-over-modules. - // - // This is used for outer aliases. - Initializer::DefineModule(upvar_index) => { - imports - .modules - .push(module.module_upvar(*upvar_index).clone()); - } - - // A new module is defined, created from a set of compiled - // artifacts. The new module value will be created with the - // specified artifacts being closed over as well as the specified - // set of module values in our index/upvar index spaces being closed - // over. - // - // This is used for defining submodules. - Initializer::CreateModule { - artifact_index, - artifacts, - modules, - } => { - let submodule = - module.create_submodule(*artifact_index, artifacts, modules, &imports.modules); - imports.modules.push(submodule); - } - } - } - - // Register the module just before instantiation to ensure we have a - // trampoline registered for every signature and to preserve the module's - // compiled JIT code within the `Store`. - store.register_module(module); - - let config = store.engine().config(); - let instance = unsafe { - let instance = compiled_module.instantiate( - imports.build(), - &store.lookup_shared_signature(module.types()), - config.memory_creator.as_ref().map(|a| a as _), - store.interrupts(), - Box::new(()), - store.externref_activations_table() as *const VMExternRefActivationsTable as *mut _, - store.stack_map_registry() as *const StackMapRegistry as *mut _, - )?; - - // 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.features.bulk_memory, - &compiled_module.data_initializers(), - ) - .map_err(|e| -> Error { - match e { - InstantiationError::Trap(trap) => Trap::from_runtime(store, trap).into(), - other => other.into(), - } - })?; - - instance - }; - - let start_func = instance.handle.module().start_func; - - // If a start function is present, invoke it. Make sure we use all the - // trap-handling configuration in `store` as well. - if let Some(start) = start_func { - let f = match instance - .handle - .lookup_by_declaration(&EntityIndex::Function(start)) - { - wasmtime_runtime::Export::Function(f) => f, - _ => unreachable!(), // valid modules shouldn't hit this - }; - let vmctx_ptr = instance.handle.vmctx_ptr(); - unsafe { - super::func::invoke_wasm_and_catch_traps(vmctx_ptr, store, || { - mem::transmute::< - *const VMFunctionBody, - unsafe extern "C" fn(*mut VMContext, *mut VMContext), - >(f.anyfunc.as_ref().func_ptr.as_ptr())( - f.anyfunc.as_ref().vmctx, vmctx_ptr - ) - })?; - } - } - - let exports = instance - .handle - .module() - .exports - .iter() - .map(|(name, index)| { - // Note that instances and modules are not handled by - // `wasmtime_runtime`, they're handled by us in this crate. That - // means we need to handle that here, otherwise we defer to the - // instance to load the values. - let item = match index { - EntityIndex::Instance(i) => { - wasmtime_runtime::Export::Instance(imports.instances[*i].clone()) - } - EntityIndex::Module(i) => { - wasmtime_runtime::Export::Module(Box::new(imports.modules[*i].clone())) - } - index => instance.handle.lookup_by_declaration(index), - }; - (name.clone(), item) - }) - .collect(); - Ok(Rc::new(exports)) -} - /// An instantiated WebAssembly module. /// /// This type represents the instantiation of a [`Module`]. Once instantiated @@ -314,29 +94,15 @@ impl Instance { /// [issue]: https://github.com/bytecodealliance/wasmtime/issues/727 /// [`ExternType`]: crate::ExternType pub fn new(store: &Store, module: &Module, imports: &[Extern]) -> Result { - if !Engine::same(store.engine(), module.engine()) { - bail!("cross-`Engine` instantiation is not currently supported"); - } - - // Perform some pre-flight checks before we get into the meat of - // instantiation. - let expected = module.compiled_module().module().imports().count(); - if expected != imports.len() { - bail!("expected {} imports, found {}", expected, imports.len()); - } - for import in imports { - if !import.comes_from_same_store(store) { - bail!("cross-`Store` instantiation is not currently supported"); + let mut i = Instantiator::new(store, module, imports)?; + loop { + if let Some((instance, items)) = i.step()? { + i.start_raw(&instance)?; + if let Some(items) = items { + break Ok(Instance::from_wasmtime(&items, store)); + } } } - - let mut imports = imports.iter(); - let items = instantiate(store, module, &mut |_name, _field, idx, builder| { - let import = imports.next().expect("already checked the length"); - builder.define_extern(idx, &import) - })?; - - Ok(Instance::from_wasmtime(&items, store)) } pub(crate) fn from_wasmtime(handle: &RuntimeInstance, store: &Store) -> Instance { @@ -417,42 +183,374 @@ impl Instance { } } +struct Instantiator<'a> { + in_progress: Vec>, + cur: ImportsBuilder<'a>, + store: &'a Store, +} + struct ImportsBuilder<'a> { + src: ImportSource<'a>, functions: PrimaryMap, tables: PrimaryMap, memories: PrimaryMap, globals: PrimaryMap, instances: PrimaryMap, modules: PrimaryMap, + initializer: usize, + module: Module, +} - module: &'a wasmtime_environ::Module, - matcher: matching::MatchCx<'a>, +enum ImportSource<'a> { + Runtime(&'a [Extern]), + Outer { initializer: usize }, } -impl<'a> ImportsBuilder<'a> { - fn new(store: &'a Store, module: &'a Module) -> ImportsBuilder<'a> { - let types = module.types(); - let module = module.compiled_module().module(); - ImportsBuilder { - module, - matcher: matching::MatchCx { store, types }, - functions: PrimaryMap::with_capacity(module.num_imported_funcs), - tables: PrimaryMap::with_capacity(module.num_imported_tables), - memories: PrimaryMap::with_capacity(module.num_imported_memories), - globals: PrimaryMap::with_capacity(module.num_imported_globals), - instances: PrimaryMap::with_capacity(module.instances.len()), - modules: PrimaryMap::with_capacity(module.modules.len()), +impl<'a> Instantiator<'a> { + /// Creates a new instantiation context used to process all the initializer + /// directives of a module. + /// + /// This doesn't do much work itself beyond setting things up. + fn new(store: &'a Store, module: &Module, imports: &'a [Extern]) -> Result> { + if !Engine::same(store.engine(), module.engine()) { + bail!("cross-`Engine` instantiation is not currently supported"); + } + + // Perform some pre-flight checks before we get into the meat of + // instantiation. + let expected = module.compiled_module().module().imports().count(); + if expected != imports.len() { + bail!("expected {} imports, found {}", expected, imports.len()); + } + for import in imports { + if !import.comes_from_same_store(store) { + bail!("cross-`Store` instantiation is not currently supported"); + } + } + + Ok(Instantiator { + in_progress: Vec::new(), + cur: ImportsBuilder::new(module, ImportSource::Runtime(imports)), + store, + }) + } + + /// Processes the next initializer for the next instance being created + /// without running any wasm code. + /// + /// This function will process module initializers, handling recursive + /// instantiations of modules for module linking if necessary as well. This + /// does not actually execute any WebAssembly code, which means that it + /// will return whenever an instance is created (because its `start` + /// function may need to be executed). + /// + /// If this function returns `None`, then it simply needs to be called + /// again to execute the next initializer. Otherwise this function has two + /// return values: + /// + /// * The first is the raw handle to the instance that was just created. + /// This instance must have its start function executed by the caller. + /// * The second is an optional list of items to get wrapped up in an + /// `Instance`. This is only `Some` for the outermost instance that was + /// created. If this is `None` callers need to keep calling this function + /// since the instance created was simply for a recursive instance + /// defined here. + fn step(&mut self) -> Result)>> { + if self.cur.initializer == 0 { + self.store.bump_instance_count()?; + } + + // Read the current module's initializer and move forward the + // initializer pointer as well. + self.cur.initializer += 1; + match self + .cur + .module + .env_module() + .initializers + .get(self.cur.initializer - 1) + { + Some(Initializer::Import { index, name, field }) => { + match &mut self.cur.src { + // If imports are coming from the runtime-provided list + // (e.g. the root module being instantiated) then we + // need to typecheck each item here before recording it. + // + // Note the `unwrap` here should be ok given the validation + // above in `Instantiation::new`. + ImportSource::Runtime(list) => { + let (head, remaining) = list.split_first().unwrap(); + *list = remaining; + let expected_ty = + self.cur.module.compiled_module().module().type_of(*index); + matching::MatchCx { + types: self.cur.module.types(), + store: self.store, + } + .extern_(&expected_ty, head) + .with_context(|| { + let extra = match field { + Some(name) => format!("::{}", name), + None => String::new(), + }; + format!("incompatible import type for `{}{}`", name, extra) + })?; + self.cur.push(head); + } + + // Otherwise if arguments are coming from our outer + // instance due to a recursive instantiation then we + // look in the previous initializer's mapping of + // arguments to figure out where to load the item from. + // Note that no typechecking is necessary here due to + // validation. + ImportSource::Outer { initializer } => { + debug_assert!(field.is_none()); + let outer = self.in_progress.last().unwrap(); + let args = match &outer.module.env_module().initializers[*initializer] { + Initializer::Instantiate { args, .. } => args, + _ => unreachable!(), + }; + let index = args.get(name).expect("should be present after validation"); + match *index { + EntityIndex::Global(i) => { + self.cur.globals.push(outer.globals[i]); + } + EntityIndex::Function(i) => { + self.cur.functions.push(outer.functions[i]); + } + EntityIndex::Table(i) => { + self.cur.tables.push(outer.tables[i]); + } + EntityIndex::Memory(i) => { + self.cur.memories.push(outer.memories[i]); + } + EntityIndex::Module(i) => { + self.cur.modules.push(outer.modules[i].clone()); + } + EntityIndex::Instance(i) => { + self.cur.instances.push(outer.instances[i].clone()); + } + } + } + } + } + + // Here we lookup our instance handle, find the right export, + // and then push that item into our own index space. We eschew + // type-checking since only valid modules should reach this point. + Some(Initializer::AliasInstanceExport { instance, export }) => { + let export = &self.cur.instances[*instance][export]; + let item = unsafe { Extern::from_wasmtime_export(export, self.store) }; + self.cur.push(&item); + } + + // A recursive instantiation of an instance. + // + // The `module` argument is used to create an import builder + // object, and we specify that the source of imports for the builder is + // this initializer's position so we can look at the `args` payload + // later. + // + // Once that's set up we save off `self.cur` into + // `self.in_progress` and start the instantiation of the child + // instance on the next execution of this function. + Some(Initializer::Instantiate { module, args: _ }) => { + let module = &self.cur.modules[*module]; + let imports = ImportsBuilder::new( + module, + ImportSource::Outer { + initializer: self.cur.initializer - 1, + }, + ); + let prev = mem::replace(&mut self.cur, imports); + self.in_progress.push(prev); + } + + // A new module is being defined, and the source of this module is + // our module's list of closed-over-modules. + // + // This is used for outer aliases. + Some(Initializer::DefineModule(upvar_index)) => { + self.cur + .modules + .push(self.cur.module.module_upvar(*upvar_index).clone()); + } + + // A new module is defined, created from a set of compiled + // artifacts. The new module value will be created with the + // specified artifacts being closed over as well as the specified + // set of module values in our index/upvar index spaces being closed + // over. + // + // This is used for defining submodules. + Some(Initializer::CreateModule { + artifact_index, + artifacts, + modules, + }) => { + let submodule = self.cur.module.create_submodule( + *artifact_index, + artifacts, + modules, + &self.cur.modules, + ); + self.cur.modules.push(submodule); + } + + // All initializers have been processed, which means we're ready to + // perform the actual raw instantiation with the raw import values. + // Once that's done if there's an in-progress module we record the + // instance in the index space. Otherwise this is the final module + // and we return the items out. + // + // Note that in all cases we return the raw instance handle to get + // the start function executed by the outer context. + None => { + let instance = self.instantiate_raw(&self.cur.module, &self.cur)?; + let items = self.runtime_instance(&instance, &self.cur); + let items = match self.in_progress.pop() { + Some(imports) => { + self.cur = imports; + self.cur.instances.push(items); + None + } + None => Some(items), + }; + return Ok(Some((instance, items))); + } + } + + Ok(None) + } + + fn instantiate_raw( + &self, + module: &Module, + imports: &ImportsBuilder, + ) -> Result { + let compiled_module = module.compiled_module(); + + // Register the module just before instantiation to ensure we have a + // trampoline registered for every signature and to preserve the module's + // compiled JIT code within the `Store`. + self.store.register_module(module); + + let config = self.store.engine().config(); + unsafe { + let instance = compiled_module.instantiate( + imports.build(), + &self.store.lookup_shared_signature(module.types()), + config.memory_creator.as_ref().map(|a| a as _), + self.store.interrupts(), + Box::new(()), + self.store.externref_activations_table() as *const VMExternRefActivationsTable + as *mut _, + self.store.stack_map_registry() as *const StackMapRegistry as *mut _, + )?; + + // 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 = self.store.add_instance(instance); + instance + .initialize( + config.features.bulk_memory, + &compiled_module.data_initializers(), + ) + .map_err(|e| -> Error { + match e { + InstantiationError::Trap(trap) => { + Trap::from_runtime(self.store, trap).into() + } + other => other.into(), + } + })?; + + Ok(instance) } } - fn define_extern(&mut self, expected: &EntityIndex, actual: &Extern) -> Result<()> { - let expected_ty = self.module.type_of(*expected); - self.matcher.extern_(&expected_ty, actual)?; - self.push_extern(actual); + fn start_raw(&self, instance: &StoreInstanceHandle) -> Result<()> { + let start_func = instance.handle.module().start_func; + + // If a start function is present, invoke it. Make sure we use all the + // trap-handling configuration in `store` as well. + if let Some(start) = start_func { + let f = match instance + .handle + .lookup_by_declaration(&EntityIndex::Function(start)) + { + wasmtime_runtime::Export::Function(f) => f, + _ => unreachable!(), // valid modules shouldn't hit this + }; + let vmctx_ptr = instance.handle.vmctx_ptr(); + unsafe { + super::func::invoke_wasm_and_catch_traps(vmctx_ptr, &instance.store, || { + mem::transmute::< + *const VMFunctionBody, + unsafe extern "C" fn(*mut VMContext, *mut VMContext), + >(f.anyfunc.as_ref().func_ptr.as_ptr())( + f.anyfunc.as_ref().vmctx, vmctx_ptr + ) + })?; + } + } Ok(()) } - fn push_extern(&mut self, item: &Extern) { + fn runtime_instance( + &self, + instance: &StoreInstanceHandle, + imports: &ImportsBuilder, + ) -> RuntimeInstance { + let exports = instance + .handle + .module() + .exports + .iter() + .map(|(name, index)| { + // Note that instances and modules are not handled by + // `wasmtime_runtime`, they're handled by us in this crate. That + // means we need to handle that here, otherwise we defer to the + // instance to load the values. + let item = match index { + EntityIndex::Instance(i) => { + wasmtime_runtime::Export::Instance(imports.instances[*i].clone()) + } + EntityIndex::Module(i) => { + wasmtime_runtime::Export::Module(Box::new(imports.modules[*i].clone())) + } + index => instance.handle.lookup_by_declaration(index), + }; + (name.clone(), item) + }) + .collect(); + Rc::new(exports) + } +} + +impl<'a> ImportsBuilder<'a> { + fn new(module: &Module, src: ImportSource<'a>) -> ImportsBuilder<'a> { + let raw = module.compiled_module().module(); + ImportsBuilder { + src, + functions: PrimaryMap::with_capacity(raw.num_imported_funcs), + tables: PrimaryMap::with_capacity(raw.num_imported_tables), + memories: PrimaryMap::with_capacity(raw.num_imported_memories), + globals: PrimaryMap::with_capacity(raw.num_imported_globals), + instances: PrimaryMap::with_capacity(raw.instances.len()), + modules: PrimaryMap::with_capacity(raw.modules.len()), + module: module.clone(), + initializer: 0, + } + } + + fn push(&mut self, item: &Extern) { match item { Extern::Func(i) => { self.functions.push(i.vmimport()); @@ -467,7 +565,6 @@ impl<'a> ImportsBuilder<'a> { self.memories.push(i.vmimport()); } Extern::Instance(i) => { - debug_assert!(Store::same(i.store(), self.matcher.store)); self.instances.push(i.items.clone()); } Extern::Module(m) => { @@ -476,7 +573,7 @@ impl<'a> ImportsBuilder<'a> { } } - fn build(&mut self) -> Imports<'_> { + fn build(&self) -> Imports<'_> { Imports { tables: self.tables.values().as_slice(), globals: self.globals.values().as_slice(), diff --git a/crates/wasmtime/src/module.rs b/crates/wasmtime/src/module.rs index 8fcd94a3bd4b..0bcf8f568657 100644 --- a/crates/wasmtime/src/module.rs +++ b/crates/wasmtime/src/module.rs @@ -527,6 +527,10 @@ impl Module { &self.inner.module } + pub(crate) fn env_module(&self) -> &wasmtime_environ::Module { + self.compiled_module().module() + } + pub(crate) fn types(&self) -> &Arc { &self.inner.types }