From a2466b3c232f82a0e13a18e1ed9ddd9ad677bd86 Mon Sep 17 00:00:00 2001 From: Peter Huene Date: Sun, 11 Apr 2021 00:37:27 -0700 Subject: [PATCH 1/8] Move the signature registry into `Engine`. This commit moves the shared signature registry out of `Store` and into `Engine`. This helps eliminate work that was performed whenever a `Module` was instantiated into a `Store`. Now a `Module` is registered with the shared signature registry upon creation, storing the mapping from the module's signature index space to the shared index space. This also refactors the "frame info" registry into a general purpose "module registry" that is used to look up trap information, signature information, and (soon) stack map information. --- crates/runtime/src/externref.rs | 2 +- crates/runtime/src/traphandlers/macos.rs | 2 +- crates/wasmtime/src/config.rs | 8 + crates/wasmtime/src/engine.rs | 100 ++++++- crates/wasmtime/src/func.rs | 111 ++++---- crates/wasmtime/src/instance.rs | 5 +- crates/wasmtime/src/lib.rs | 6 +- crates/wasmtime/src/module.rs | 60 ++++- .../src/{frame_info.rs => module/registry.rs} | 225 ++++++++-------- crates/wasmtime/src/module/serialization.rs | 243 +++++++++++------- crates/wasmtime/src/sig_registry.rs | 155 ----------- crates/wasmtime/src/signatures.rs | 185 +++++++++++++ crates/wasmtime/src/store.rs | 152 +++++------ crates/wasmtime/src/trampoline/func.rs | 10 +- crates/wasmtime/src/trap.rs | 4 +- crates/wasmtime/src/types.rs | 7 +- crates/wasmtime/src/types/matching.rs | 42 +-- 17 files changed, 768 insertions(+), 549 deletions(-) rename crates/wasmtime/src/{frame_info.rs => module/registry.rs} (78%) delete mode 100644 crates/wasmtime/src/sig_registry.rs create mode 100644 crates/wasmtime/src/signatures.rs diff --git a/crates/runtime/src/externref.rs b/crates/runtime/src/externref.rs index cefce0d507..02943c1c01 100644 --- a/crates/runtime/src/externref.rs +++ b/crates/runtime/src/externref.rs @@ -753,7 +753,7 @@ pub struct StackMapRegistry { struct StackMapRegistryInner { /// A map from the highest pc in a module, to its stack maps. /// - /// For details, see the comment above `GlobalFrameInfo::ranges`. + /// For details, see the comment above `GlobalModuleRegistry`. ranges: BTreeMap, } diff --git a/crates/runtime/src/traphandlers/macos.rs b/crates/runtime/src/traphandlers/macos.rs index 0f5e593012..2f92167526 100644 --- a/crates/runtime/src/traphandlers/macos.rs +++ b/crates/runtime/src/traphandlers/macos.rs @@ -25,7 +25,7 @@ //! use a thread-local to store information about how to unwind. Additionally //! this requires that the check of whether a pc is a wasm trap or not is a //! global check rather than a per-thread check. This necessitates the existence -//! of `GlobalFrameInfo` in the `wasmtime` crate. +//! of `GlobalModuleRegistry` in the `wasmtime` crate. //! //! Otherwise this file heavily uses the `mach` Rust crate for type and //! function declarations. Many bits and pieces are copied or translated from diff --git a/crates/wasmtime/src/config.rs b/crates/wasmtime/src/config.rs index 43bd809c84..8351fea426 100644 --- a/crates/wasmtime/src/config.rs +++ b/crates/wasmtime/src/config.rs @@ -319,6 +319,10 @@ impl HostFuncMap { fn async_required(&self) -> bool { self.funcs.values().any(|f| f.1) } + + fn iter(&self) -> impl Iterator { + self.funcs.values().map(|v| &*v.0) + } } macro_rules! generate_wrap_async_host_func { @@ -1318,6 +1322,10 @@ impl Config { for_each_function_signature!(generate_wrap_async_host_func); + pub(crate) fn host_funcs(&self) -> impl Iterator { + self.host_funcs.iter() + } + pub(crate) fn get_host_func(&self, module: &str, name: &str) -> Option<&HostFunc> { self.host_funcs.get(module, name) } diff --git a/crates/wasmtime/src/engine.rs b/crates/wasmtime/src/engine.rs index f7d9c4eaa4..54d8769d19 100644 --- a/crates/wasmtime/src/engine.rs +++ b/crates/wasmtime/src/engine.rs @@ -1,10 +1,31 @@ +use crate::signatures::{SharedSignatures, SignatureRegistry, TrampolineMap}; use crate::Config; use anyhow::Result; -use std::sync::Arc; +use std::collections::HashMap; +use std::sync::{Arc, RwLock}; #[cfg(feature = "cache")] use wasmtime_cache::CacheConfig; +use wasmtime_environ::{ + entity::PrimaryMap, + wasm::{SignatureIndex, WasmFuncType}, +}; use wasmtime_jit::Compiler; -use wasmtime_runtime::{debug_builtins, InstanceAllocator}; +use wasmtime_runtime::{ + debug_builtins, InstanceAllocator, InstanceHandle, VMCallerCheckedAnyfunc, + VMSharedSignatureIndex, VMTrampoline, +}; + +#[derive(Default)] +struct EngineHostFuncs { + anyfuncs: HashMap>, + trampolines: TrampolineMap, +} + +// This is safe for send and sync as it is read-only once the +// engine is constructed and the host functions live with the config, +// which the engine keeps a strong reference to. +unsafe impl Send for EngineHostFuncs {} +unsafe impl Sync for EngineHostFuncs {} /// An `Engine` which is a global context for compilation and management of wasm /// modules. @@ -37,6 +58,16 @@ struct EngineInner { config: Config, compiler: Compiler, allocator: Box, + signatures: RwLock, + host_funcs: EngineHostFuncs, +} + +impl Drop for EngineInner { + fn drop(&mut self) { + let mut signatures = self.signatures.write().unwrap(); + signatures.unregister(self.host_funcs.trampolines.indexes()); + assert!(signatures.is_empty()); + } } impl Engine { @@ -46,11 +77,29 @@ impl Engine { debug_builtins::ensure_exported(); config.validate()?; let allocator = config.build_allocator()?; + let mut signatures = SignatureRegistry::default(); + let mut host_funcs = EngineHostFuncs::default(); + + // Register all the host function signatures + for func in config.host_funcs() { + let sig = signatures.register(func.ty.as_wasm_func_type()); + + // Cloning the instance handle is safe as host functions outlive the engine + host_funcs.anyfuncs.insert( + unsafe { func.instance.clone() }, + Box::new(func.anyfunc(sig)), + ); + + host_funcs.trampolines.insert(sig, func.trampoline); + } + Ok(Engine { inner: Arc::new(EngineInner { config: config.clone(), compiler: config.build_compiler(allocator.as_ref()), allocator, + signatures: RwLock::new(signatures), + host_funcs, }), }) } @@ -79,6 +128,53 @@ impl Engine { Arc::ptr_eq(&a.inner, &b.inner) } + pub(crate) fn register_module_signatures( + &self, + signatures: &PrimaryMap, + trampolines: impl Iterator, + ) -> (SharedSignatures, TrampolineMap) { + self.inner + .signatures + .write() + .unwrap() + .register_module(signatures, trampolines) + } + + pub(crate) fn register_signature(&self, ty: &WasmFuncType) -> VMSharedSignatureIndex { + self.inner.signatures.write().unwrap().register(ty) + } + + pub(crate) fn unregister_signatures( + &self, + indexes: impl Iterator, + ) { + self.inner.signatures.write().unwrap().unregister(indexes); + } + + pub(crate) fn lookup_func_type(&self, index: VMSharedSignatureIndex) -> Option { + self.inner + .signatures + .read() + .unwrap() + .lookup_type(index) + .cloned() + } + + pub(crate) fn host_func_trampolines(&self) -> &TrampolineMap { + &self.inner.host_funcs.trampolines + } + + pub(crate) fn host_func_anyfunc( + &self, + instance: &InstanceHandle, + ) -> Option<&VMCallerCheckedAnyfunc> { + self.inner + .host_funcs + .anyfuncs + .get(instance) + .map(AsRef::as_ref) + } + /// Ahead-of-time (AOT) compiles a WebAssembly module. /// /// The `bytes` provided must be in one of two formats: diff --git a/crates/wasmtime/src/func.rs b/crates/wasmtime/src/func.rs index 82805e2c11..6bb24597e1 100644 --- a/crates/wasmtime/src/func.rs +++ b/crates/wasmtime/src/func.rs @@ -1,4 +1,4 @@ -use crate::{sig_registry::SignatureRegistry, trampoline::StoreInstanceHandle}; +use crate::trampoline::StoreInstanceHandle; use crate::{Config, Extern, FuncType, Store, Trap, Val, ValType}; use anyhow::{bail, Context as _, Result}; use smallvec::{smallvec, SmallVec}; @@ -22,9 +22,9 @@ use wasmtime_runtime::{ /// This differs from `Func` in that it is not associated with a `Store`. /// Host functions are associated with a `Config`. pub(crate) struct HostFunc { - ty: FuncType, - instance: InstanceHandle, - trampoline: VMTrampoline, + pub ty: FuncType, + pub instance: InstanceHandle, + pub trampoline: VMTrampoline, } impl HostFunc { @@ -73,6 +73,23 @@ impl HostFunc { } } + /// Gets a caller-checked anyfunc for this host function given a shared signature index. + /// + /// The shared signature index must have been registered for the signature of + /// this host function. + pub fn anyfunc(&self, sig: VMSharedSignatureIndex) -> VMCallerCheckedAnyfunc { + let mut anyfunc = match self + .instance + .lookup_by_declaration(&EntityIndex::Function(FuncIndex::from_u32(0))) + { + wasmtime_runtime::Export::Function(f) => unsafe { f.anyfunc.as_ref() }.clone(), + _ => unreachable!(), + }; + + anyfunc.type_index = sig; + anyfunc + } + /// Converts a `HostFunc` to a `Func`. /// /// # Safety @@ -88,11 +105,11 @@ impl HostFunc { }; let export = ExportFunction { - anyfunc: std::ptr::NonNull::new_unchecked(store.get_host_anyfunc( - &self.instance, - &self.ty, - self.trampoline, - )), + anyfunc: store + .engine() + .host_func_anyfunc(&self.instance) + .unwrap() + .into(), }; Func { @@ -408,13 +425,9 @@ impl Func { Func::invoke(&store, &ty_clone, caller_vmctx, values_vec, &func) }); - let (instance, trampoline) = crate::trampoline::create_function( - &ty, - func, - store.engine().config(), - Some(&mut store.signatures().borrow_mut()), - ) - .expect("failed to create function"); + let (instance, trampoline) = + crate::trampoline::create_function(&ty, func, store.engine().config(), Some(store)) + .expect("failed to create function"); let idx = EntityIndex::Function(FuncIndex::from_u32(0)); let (instance, export) = match instance.lookup_by_declaration(&idx) { @@ -734,7 +747,7 @@ impl Func { /// # } /// ``` pub fn wrap(store: &Store, func: impl IntoFunc) -> Func { - let (_, instance, trampoline) = func.into_func(Some(&mut store.signatures().borrow_mut())); + let (_, instance, trampoline) = func.into_func(Some(store)); let (instance, export) = unsafe { let idx = EntityIndex::Function(FuncIndex::from_u32(0)); @@ -759,33 +772,35 @@ impl Func { /// Returns the underlying wasm type that this `Func` has. pub fn ty(&self) -> FuncType { - // Signatures should always be registered in the store's registry of + // Signatures should always be registered in the engine's registry of // shared signatures, so we should be able to unwrap safely here. - let signatures = self.instance.store.signatures().borrow(); - let (wft, _) = signatures - .lookup_shared(self.sig_index()) - .expect("signature should be registered"); - - // This is only called with `Export::Function`, and since it's coming - // from wasmtime_runtime itself we should support all the types coming - // out of it, so assert such here. - FuncType::from_wasm_func_type(&wft) + FuncType::from_wasm_func_type( + self.instance + .store + .engine() + .lookup_func_type(self.sig_index()) + .expect("signature should be registered"), + ) } /// Returns the number of parameters that this function takes. pub fn param_arity(&self) -> usize { - let signatures = self.instance.store.signatures().borrow(); - let (sig, _) = signatures - .lookup_shared(self.sig_index()) + let sig = self + .instance + .store + .engine() + .lookup_func_type(self.sig_index()) .expect("signature should be registered"); sig.params.len() } /// Returns the number of results this function produces. pub fn result_arity(&self) -> usize { - let signatures = self.instance.store.signatures().borrow(); - let (sig, _) = signatures - .lookup_shared(self.sig_index()) + let sig = self + .instance + .store + .engine() + .lookup_func_type(self.sig_index()) .expect("signature should be registered"); sig.returns.len() } @@ -907,21 +922,12 @@ impl Func { } pub(crate) unsafe fn from_wasmtime_function(export: &ExportFunction, store: &Store) -> Self { - // Each function signature in a module should have a trampoline stored - // on that module as well, so unwrap the result here since otherwise - // it's a bug in wasmtime. let anyfunc = export.anyfunc.as_ref(); - let trampoline = store - .signatures() - .borrow() - .lookup_shared(anyfunc.type_index) - .expect("failed to retrieve trampoline from module") - .1; Func { instance: store.existing_vmctx(anyfunc.vmctx), export: export.clone(), - trampoline, + trampoline: store.lookup_trampoline(anyfunc.type_index), } } @@ -1542,10 +1548,7 @@ for_each_function_signature!(impl_host_abi); /// as an implementation detail of this crate. pub trait IntoFunc { #[doc(hidden)] - fn into_func( - self, - registry: Option<&mut SignatureRegistry>, - ) -> (FuncType, InstanceHandle, VMTrampoline); + fn into_func(self, store: Option<&Store>) -> (FuncType, InstanceHandle, VMTrampoline); } /// A structure representing the *caller's* context when creating a function @@ -1658,12 +1661,12 @@ macro_rules! impl_into_func { $($args: WasmTy,)* R: WasmRet, { - fn into_func(self, registry: Option<&mut SignatureRegistry>) -> (FuncType, InstanceHandle, VMTrampoline) { + fn into_func(self, store: Option<&Store>) -> (FuncType, InstanceHandle, VMTrampoline) { let f = move |_: Caller<'_>, $($args:$args),*| { self($($args),*) }; - f.into_func(registry) + f.into_func(store) } } @@ -1674,7 +1677,7 @@ macro_rules! impl_into_func { $($args: WasmTy,)* R: WasmRet, { - fn into_func(self, registry: Option<&mut SignatureRegistry>) -> (FuncType, InstanceHandle, VMTrampoline) { + fn into_func(self, store: Option<&Store>) -> (FuncType, InstanceHandle, VMTrampoline) { /// This shim is called by Wasm code, constructs a `Caller`, /// calls the wrapped host function, and returns the translated /// result back to Wasm. @@ -1807,10 +1810,10 @@ macro_rules! impl_into_func { let trampoline = host_trampoline::<$($args,)* R>; - // If not given a registry, use a default signature index that is guaranteed to trap - // if the function is called indirectly without first being associated with a store (a bug condition). - let shared_signature_id = registry - .map(|r| r.register(ty.as_wasm_func_type(), trampoline)) + // If not given a store, use a default signature index that is guaranteed to trap. + // If the function is called indirectly without first being associated with a store (a bug condition). + let shared_signature_id = store + .map(|s| s.register_signature(ty.as_wasm_func_type(), trampoline)) .unwrap_or(VMSharedSignatureIndex::default()); let instance = unsafe { diff --git a/crates/wasmtime/src/instance.rs b/crates/wasmtime/src/instance.rs index 01765d02bd..00cc90fd82 100644 --- a/crates/wasmtime/src/instance.rs +++ b/crates/wasmtime/src/instance.rs @@ -362,6 +362,7 @@ impl<'a> Instantiator<'a> { let expected_ty = self.cur.module.compiled_module().module().type_of(*index); matching::MatchCx { + signatures: self.cur.module.signatures(), types: self.cur.module.types(), store: self.store, } @@ -513,14 +514,12 @@ impl<'a> Instantiator<'a> { unsafe { let engine = self.store.engine(); let allocator = engine.allocator(); - let signatures = self.store.signatures().borrow(); - let signatures = signatures.lookup_table(&self.cur.module); let instance = allocator.allocate(InstanceAllocationRequest { module: compiled_module.module().clone(), finished_functions: compiled_module.finished_functions(), imports: self.cur.build(), - shared_signatures: (&signatures).into(), + shared_signatures: self.cur.module.signatures().into(), host_state: Box::new(()), interrupts: self.store.interrupts(), externref_activations_table: self.store.externref_activations_table() diff --git a/crates/wasmtime/src/lib.rs b/crates/wasmtime/src/lib.rs index d843b9db43..98a86a279e 100644 --- a/crates/wasmtime/src/lib.rs +++ b/crates/wasmtime/src/lib.rs @@ -282,13 +282,12 @@ mod func; mod config; mod engine; mod externals; -mod frame_info; mod instance; mod linker; mod memory; mod module; mod r#ref; -mod sig_registry; +mod signatures; mod store; mod trampoline; mod trap; @@ -298,12 +297,11 @@ mod values; pub use crate::config::*; pub use crate::engine::*; pub use crate::externals::*; -pub use crate::frame_info::{FrameInfo, FrameSymbol}; pub use crate::func::*; pub use crate::instance::Instance; pub use crate::linker::*; pub use crate::memory::*; -pub use crate::module::Module; +pub use crate::module::{FrameInfo, FrameSymbol, Module}; pub use crate::r#ref::ExternRef; pub use crate::store::*; pub use crate::trap::*; diff --git a/crates/wasmtime/src/module.rs b/crates/wasmtime/src/module.rs index 8b460207cf..7e1db3c876 100644 --- a/crates/wasmtime/src/module.rs +++ b/crates/wasmtime/src/module.rs @@ -1,4 +1,7 @@ -use crate::types::{ExportType, ExternType, ImportType}; +use crate::{ + signatures::{SharedSignatures, TrampolineMap}, + types::{ExportType, ExternType, ImportType}, +}; use crate::{Engine, ModuleType}; use anyhow::{bail, Context, Result}; use std::fs; @@ -8,13 +11,36 @@ use wasmparser::Validator; #[cfg(feature = "cache")] use wasmtime_cache::ModuleCacheEntry; use wasmtime_environ::entity::PrimaryMap; -use wasmtime_environ::wasm::ModuleIndex; +use wasmtime_environ::wasm::{ModuleIndex, SignatureIndex}; use wasmtime_jit::{CompilationArtifacts, CompiledModule, TypeTables}; +use wasmtime_runtime::VMSharedSignatureIndex; +mod registry; mod serialization; +pub use registry::{FrameInfo, FrameSymbol, GlobalModuleRegistry, ModuleRegistry}; pub use serialization::SerializedModule; +// A wrapper around registered signatures and trampolines that will automatically +/// unregister the signatures when dropped. +pub(crate) struct ModuleSharedSignatures { + engine: Engine, + signatures: SharedSignatures, + trampolines: TrampolineMap, +} + +impl Drop for ModuleSharedSignatures { + fn drop(&mut self) { + if !self.signatures.is_empty() { + // Use the shared signatures map to unregister as not every registered + // signature will have a trampoline, but every index in the trampoline map + // will be present in the shared signatures map. + self.engine + .unregister_signatures(self.signatures.values().cloned()); + } + } +} + /// A compiled WebAssembly module, ready to be instantiated. /// /// A `Module` is a compiled in-memory representation of an input WebAssembly @@ -102,6 +128,8 @@ struct ModuleInner { /// Type information of this module and all `artifact_upvars` compiled /// modules. types: Arc, + /// Registered shared signature for the module. + signatures: Arc, } impl Module { @@ -318,10 +346,16 @@ impl Module { engine.compiler().isa(), &*engine.config().profiler, )?; - let module = modules.remove(main_module); // Validate the module can be used with the current allocator - engine.allocator().validate(module.module())?; + engine.allocator().validate(modules[main_module].module())?; + + let (signatures, trampolines) = engine.register_module_signatures( + &types.wasm_signatures, + modules.iter().flat_map(|m| m.trampolines().iter().cloned()), + ); + + let module = modules.remove(main_module); Ok(Module { inner: Arc::new(ModuleInner { @@ -330,6 +364,11 @@ impl Module { types: Arc::new(types), artifact_upvars: modules, module_upvars: Vec::new(), + signatures: Arc::new(ModuleSharedSignatures { + engine: engine.clone(), + signatures, + trampolines, + }), }), }) } @@ -416,8 +455,8 @@ impl Module { ) -> Module { Module { inner: Arc::new(ModuleInner { - types: self.types().clone(), - engine: self.engine().clone(), + types: self.inner.types.clone(), + engine: self.inner.engine.clone(), module: self.inner.artifact_upvars[artifact_index].clone(), artifact_upvars: artifact_upvars .iter() @@ -432,6 +471,7 @@ impl Module { wasmtime_environ::ModuleUpvar::Local(i) => modules[i].clone(), }) .collect(), + signatures: self.inner.signatures.clone(), }), } } @@ -448,6 +488,14 @@ impl Module { &self.inner.types } + pub(crate) fn signatures(&self) -> &PrimaryMap { + &self.inner.signatures.signatures + } + + pub(crate) fn shared_signatures(&self) -> &Arc { + &self.inner.signatures + } + /// Looks up the module upvar value at the `index` specified. /// /// Note that this panics if `index` is out of bounds since this should diff --git a/crates/wasmtime/src/frame_info.rs b/crates/wasmtime/src/module/registry.rs similarity index 78% rename from crates/wasmtime/src/frame_info.rs rename to crates/wasmtime/src/module/registry.rs index 8bd326ebdb..161ef408d6 100644 --- a/crates/wasmtime/src/frame_info.rs +++ b/crates/wasmtime/src/module/registry.rs @@ -1,35 +1,30 @@ -use std::collections::BTreeMap; -use std::sync::Arc; -use std::sync::Mutex; -use wasmtime_environ::entity::EntityRef; -use wasmtime_environ::ir; -use wasmtime_environ::wasm::DefinedFuncIndex; -use wasmtime_environ::{FunctionAddressMap, TrapInformation}; -use wasmtime_jit::CompiledModule; +//! Implements a registry of modules for a store. -/// This is a structure that lives within a `Store` and retains information -/// about all modules registered with the `Store` via instantiation. -/// -/// "frame information" here refers to things like determining whether a -/// program counter is a wasm program counter, and additionally mapping program -/// counters to wasm filenames, modules, line numbers, etc. This store of -/// information lives as long as a `Store` lives since modules are never -/// unloaded today. -#[derive(Default)] -pub struct StoreFrameInfo { - /// An internal map that keeps track of backtrace frame information for - /// each module. - /// - /// This map is morally a map of ranges to a map of information for that - /// module. Each module is expected to reside in a disjoint section of - /// contiguous memory. No modules can overlap. - /// - /// The key of this map is the highest address in the module and the value - /// is the module's information, which also contains the start address. - ranges: BTreeMap, +use crate::{module::ModuleSharedSignatures, Module}; +use std::{ + collections::BTreeMap, + sync::{Arc, Mutex}, +}; +use wasmtime_environ::{ + entity::EntityRef, ir, wasm::DefinedFuncIndex, FunctionAddressMap, TrapInformation, +}; +use wasmtime_jit::CompiledModule; +use wasmtime_runtime::{VMSharedSignatureIndex, VMTrampoline}; + +lazy_static::lazy_static! { + static ref GLOBAL_MODULES: Mutex = Default::default(); } -impl StoreFrameInfo { +/// Used for registering modules with a store. +/// +/// The map is from the ending (exclusive) address for the module code to +/// the registered module. +/// +/// The `BTreeMap` is used to quickly locate a module based on a program counter value. +#[derive(Default)] +pub struct ModuleRegistry(BTreeMap); + +impl ModuleRegistry { /// Fetches frame information about a program counter in a backtrace. /// /// Returns an object if this `pc` is known to some previously registered @@ -48,8 +43,8 @@ impl StoreFrameInfo { self.module(pc)?.lookup_trap_info(pc) } - fn module(&self, pc: usize) -> Option<&ModuleFrameInfo> { - let (end, info) = self.ranges.range(pc..).next()?; + fn module(&self, pc: usize) -> Option<&RegisteredModule> { + let (end, info) = self.0.range(pc..).next()?; if pc < info.start || *end < pc { return None; } @@ -57,57 +52,78 @@ impl StoreFrameInfo { Some(info) } - /// Registers a new compiled module's frame information. - pub fn register(&mut self, module: &Arc) { - let (start, end) = module.code().range(); + /// Registers a new module with the registry. + pub fn register(&mut self, module: &Module) -> bool { + let compiled_module = module.compiled_module(); + let (start, end) = compiled_module.code().range(); - // Ignore modules with no code or finished functions - if start == end || module.finished_functions().is_empty() { - return; + // Ignore modules with no code, finished functions, or if the module is already registered + if start == end || compiled_module.finished_functions().is_empty() { + return false; } // The module code range is exclusive for end, so make it inclusive as it // may be a valid PC value let end = end - 1; + if self.0.get(&end).is_some() { + return false; + } + // Assert that this module's code doesn't collide with any other registered modules - if let Some((_, prev)) = self.ranges.range(end..).next() { + if let Some((_, prev)) = self.0.range(end..).next() { assert!(prev.start > end); } - if let Some((prev_end, _)) = self.ranges.range(..=start).next_back() { + + if let Some((prev_end, _)) = self.0.range(..=start).next_back() { assert!(*prev_end < start); } - let prev = self.ranges.insert( + let prev = self.0.insert( end, - ModuleFrameInfo { + RegisteredModule { start, - module: module.clone(), + module: compiled_module.clone(), + signatures: module.shared_signatures().clone(), }, ); assert!(prev.is_none()); - GLOBAL_INFO.lock().unwrap().register(start, end, module); + GLOBAL_MODULES.lock().unwrap().register(start, end, module); + true + } + + /// Looks up a trampoline from a shared signature index. + /// + /// This will search all modules associated with the store for a suitable trampoline + /// given the shared signature index. + pub fn lookup_trampoline(&self, index: VMSharedSignatureIndex) -> Option { + for (_, m) in &self.0 { + if let Some(trampoline) = m.signatures.trampolines.get(index) { + return Some(trampoline); + } + } + + None } } -impl Drop for StoreFrameInfo { +impl Drop for ModuleRegistry { fn drop(&mut self) { - let mut info = GLOBAL_INFO.lock().unwrap(); - for end in self.ranges.keys() { + let mut info = GLOBAL_MODULES.lock().unwrap(); + for end in self.0.keys() { info.unregister(*end); } } } -/// Represents a module's frame information. -#[derive(Clone)] -pub struct ModuleFrameInfo { +struct RegisteredModule { start: usize, module: Arc, + signatures: Arc, } -impl ModuleFrameInfo { +impl RegisteredModule { /// Determines if the related module has unparsed debug information. pub fn has_unparsed_debuginfo(&self) -> bool { self.module.has_unparsed_debuginfo() @@ -214,47 +230,29 @@ impl ModuleFrameInfo { } } -/// This is the dual of `StoreFrameInfo` and is stored globally (as the name -/// implies) rather than simply in one `Store`. +/// This is the global module registry that stores information for all modules +/// that are currently in use by any `Store`. /// /// The purpose of this map is to be called from signal handlers to determine /// whether a program counter is a wasm trap or not. Specifically macOS has /// no contextual information about the thread available, hence the necessity /// for global state rather than using thread local state. /// -/// This is similar to `StoreFrameInfo` except that it has less information and -/// supports removal. Any time anything is registered with a `StoreFrameInfo` -/// it is also automatically registered with the singleton global frame -/// information. When a `StoreFrameInfo` is destroyed then all of its entries -/// are removed from the global frame information. +/// This is similar to `ModuleRegistry` except that it has less information and +/// supports removal. Any time anything is registered with a `ModuleRegistry` +/// it is also automatically registered with the singleton global module +/// registry. When a `ModuleRegistry` is destroyed then all of its entries +/// are removed from the global module registry. #[derive(Default)] -pub struct GlobalFrameInfo { - // The map here behaves the same way as `StoreFrameInfo`. - ranges: BTreeMap, -} +pub struct GlobalModuleRegistry(BTreeMap); -/// This is the equivalent of `ModuleFrameInfo` except it keeps a reference count. -struct GlobalModuleFrameInfo { - module: ModuleFrameInfo, - - /// Note that modules can be instantiated in many stores, so the purpose of - /// this field is to keep track of how many stores have registered a - /// module. Information is only removed from the global store when this - /// reference count reaches 0. - references: usize, -} - -lazy_static::lazy_static! { - static ref GLOBAL_INFO: Mutex = Default::default(); -} - -impl GlobalFrameInfo { +impl GlobalModuleRegistry { /// Returns whether the `pc`, according to globally registered information, /// is a wasm trap or not. pub(crate) fn is_wasm_pc(pc: usize) -> bool { - let info = GLOBAL_INFO.lock().unwrap(); + let info = GLOBAL_MODULES.lock().unwrap(); - match info.ranges.range(pc..).next() { + match info.0.range(pc..).next() { Some((end, info)) => { if pc < info.module.start || *end < pc { return false; @@ -263,7 +261,7 @@ impl GlobalFrameInfo { match info.module.func(pc) { Some((index, offset)) => { let (addr_map, _) = info.module.module.func_info(index); - ModuleFrameInfo::instr_pos(offset, addr_map).is_some() + RegisteredModule::instr_pos(offset, addr_map).is_some() } None => false, } @@ -274,17 +272,15 @@ impl GlobalFrameInfo { /// Registers a new region of code, described by `(start, end)` and with /// the given function information, with the global information. - fn register(&mut self, start: usize, end: usize, module: &Arc) { - let info = self - .ranges - .entry(end) - .or_insert_with(|| GlobalModuleFrameInfo { - module: ModuleFrameInfo { - start, - module: module.clone(), - }, - references: 0, - }); + fn register(&mut self, start: usize, end: usize, module: &Module) { + let info = self.0.entry(end).or_insert_with(|| GlobalRegisteredModule { + module: RegisteredModule { + start, + module: module.compiled_module().clone(), + signatures: module.shared_signatures().clone(), + }, + references: 0, + }); // Note that ideally we'd debug_assert that the information previously // stored, if any, matches the `functions` we were given, but for now we @@ -293,17 +289,28 @@ impl GlobalFrameInfo { info.references += 1; } - /// Unregisters a region of code (keyed by the `end` address) from this + /// Unregisters a region of code (keyed by the `end` address) from the /// global information. fn unregister(&mut self, end: usize) { - let info = self.ranges.get_mut(&end).unwrap(); + let info = self.0.get_mut(&end).unwrap(); info.references -= 1; if info.references == 0 { - self.ranges.remove(&end); + self.0.remove(&end); } } } +/// This is the equivalent of `RegisteredModule` except it keeps a reference count. +struct GlobalRegisteredModule { + module: RegisteredModule, + + /// Note that modules can be instantiated in many stores, so the purpose of + /// this field is to keep track of how many stores have registered a + /// module. Information is only removed from the global registry when this + /// reference count reaches 0. + references: usize, +} + /// Description of a frame in a backtrace for a [`Trap`]. /// /// Whenever a WebAssembly trap occurs an instance of [`Trap`] is created. Each @@ -321,19 +328,6 @@ pub struct FrameInfo { symbols: Vec, } -/// Debug information for a symbol that is attached to a [`FrameInfo`]. -/// -/// When DWARF debug information is present in a wasm file then this structure -/// can be found on a [`FrameInfo`] and can be used to learn about filenames, -/// line numbers, etc, which are the origin of a function in a stack trace. -#[derive(Debug)] -pub struct FrameSymbol { - name: Option, - file: Option, - line: Option, - column: Option, -} - impl FrameInfo { /// Returns the WebAssembly function index for this frame. /// @@ -405,6 +399,19 @@ impl FrameInfo { } } +/// Debug information for a symbol that is attached to a [`FrameInfo`]. +/// +/// When DWARF debug information is present in a wasm file then this structure +/// can be found on a [`FrameInfo`] and can be used to learn about filenames, +/// line numbers, etc, which are the origin of a function in a stack trace. +#[derive(Debug)] +pub struct FrameSymbol { + name: Option, + file: Option, + line: Option, + column: Option, +} + impl FrameSymbol { /// Returns the function name associated with this symbol. /// @@ -463,7 +470,7 @@ fn test_frame_info() -> Result<(), anyhow::Error> { )?; // Create an instance to ensure the frame information is registered. Instance::new(&store, &module, &[])?; - let info = store.frame_info().borrow(); + let modules = store.modules().borrow(); for (i, alloc) in module.compiled_module().finished_functions() { let (start, end) = unsafe { let ptr = (**alloc).as_ptr(); @@ -471,7 +478,7 @@ fn test_frame_info() -> Result<(), anyhow::Error> { (ptr as usize, ptr as usize + len) }; for pc in start..end { - let (frame, _) = info.lookup_frame_info(pc).unwrap(); + let (frame, _) = modules.lookup_frame_info(pc).unwrap(); assert!(frame.func_index() == i.as_u32()); } } diff --git a/crates/wasmtime/src/module/serialization.rs b/crates/wasmtime/src/module/serialization.rs index cc28cdeaf7..1d74aaaf43 100644 --- a/crates/wasmtime/src/module/serialization.rs +++ b/crates/wasmtime/src/module/serialization.rs @@ -1,6 +1,6 @@ //! Implements module serialization. -use super::ModuleInner; +use super::{ModuleInner, ModuleSharedSignatures}; use crate::{Engine, Module, OptLevel}; use anyhow::{anyhow, bail, Context, Result}; use bincode::Options; @@ -10,8 +10,7 @@ use std::fmt; use std::str::FromStr; use std::sync::Arc; use std::{collections::HashMap, fmt::Display}; -use wasmtime_environ::Tunables; -use wasmtime_environ::{isa::TargetIsa, settings}; +use wasmtime_environ::{isa::TargetIsa, settings, Tunables}; use wasmtime_jit::{ CompilationArtifacts, CompilationStrategy, CompiledModule, Compiler, TypeTables, }; @@ -123,55 +122,44 @@ impl From for OptLevel { } } -/// A small helper struct which defines modules are serialized. +/// A small helper struct for serialized module upvars. #[derive(Serialize, Deserialize)] -struct SerializedModuleData<'a> { - /// All compiled artifacts needed by this module, where the last entry in - /// this list is the artifacts for the module itself. - artifacts: Vec>, +struct SerializedModuleUpvar { + /// The module's index into the compilation artifact. + index: usize, + /// Indexes into the list of all compilation artifacts for this module. + artifact_upvars: Vec, /// Closed-over module values that are also needed for this module. - modules: Vec>, - /// The index into the list of type tables that are used for this module's - /// type tables. - type_tables: usize, + module_upvars: Vec, } -impl<'a> SerializedModuleData<'a> { - pub fn new(module: &'a Module) -> (Self, Vec>) { - let mut pushed = HashMap::new(); - let mut tables = Vec::new(); - return (module_data(module, &mut pushed, &mut tables), tables); +impl SerializedModuleUpvar { + pub fn new(module: &Module, artifacts: &[Arc]) -> Self { + // TODO: improve upon the linear searches in the artifact list + let index = artifacts + .iter() + .position(|a| Arc::as_ptr(a) == Arc::as_ptr(&module.inner.module)) + .expect("module should be in artifacts list"); - fn module_data<'a>( - module: &'a Module, - type_tables_pushed: &mut HashMap, - type_tables: &mut Vec>, - ) -> SerializedModuleData<'a> { - // Deduplicate `Arc` using our two parameters to ensure we - // serialize type tables as little as possible. - let ptr = Arc::as_ptr(module.types()); - let type_tables_idx = *type_tables_pushed.entry(ptr as usize).or_insert_with(|| { - type_tables.push(MyCow::Borrowed(module.types())); - type_tables.len() - 1 - }); - SerializedModuleData { - artifacts: module - .inner - .artifact_upvars - .iter() - .map(|i| MyCow::Borrowed(i.compilation_artifacts())) - .chain(Some(MyCow::Borrowed( - module.compiled_module().compilation_artifacts(), - ))) - .collect(), - modules: module - .inner - .module_upvars - .iter() - .map(|i| module_data(i, type_tables_pushed, type_tables)) - .collect(), - type_tables: type_tables_idx, - } + SerializedModuleUpvar { + index, + artifact_upvars: module + .inner + .artifact_upvars + .iter() + .map(|m| { + artifacts + .iter() + .position(|a| Arc::as_ptr(a) == Arc::as_ptr(m)) + .expect("artifact should be in artifacts list") + }) + .collect(), + module_upvars: module + .inner + .module_upvars + .iter() + .map(|m| SerializedModuleUpvar::new(m, artifacts)) + .collect(), } } } @@ -212,14 +200,36 @@ pub struct SerializedModule<'a> { strategy: CompilationStrategy, tunables: Tunables, features: WasmFeatures, - data: SerializedModuleData<'a>, - tables: Vec>, + artifacts: Vec>, + module_upvars: Vec, + types: MyCow<'a, TypeTables>, } impl<'a> SerializedModule<'a> { pub fn new(module: &'a Module) -> Self { - let (data, tables) = SerializedModuleData::new(module); - Self::with_data(module.engine().compiler(), data, tables) + let compiler = module.engine().compiler(); + let artifacts = module + .inner + .artifact_upvars + .iter() + .map(|m| MyCow::Borrowed(m.compilation_artifacts())) + .chain(Some(MyCow::Borrowed( + module.inner.module.compilation_artifacts(), + ))) + .collect::>(); + let module_upvars = module + .inner + .module_upvars + .iter() + .map(|m| SerializedModuleUpvar::new(m, &module.inner.artifact_upvars)) + .collect::>(); + + Self::with_data( + compiler, + artifacts, + module_upvars, + MyCow::Borrowed(module.types()), + ) } pub fn from_artifacts( @@ -229,19 +239,17 @@ impl<'a> SerializedModule<'a> { ) -> Self { Self::with_data( compiler, - SerializedModuleData { - artifacts: artifacts.iter().map(MyCow::Borrowed).collect(), - modules: Vec::new(), - type_tables: 0, - }, - vec![MyCow::Borrowed(types)], + artifacts.iter().map(MyCow::Borrowed).collect(), + Vec::new(), + MyCow::Borrowed(types), ) } fn with_data( compiler: &Compiler, - data: SerializedModuleData<'a>, - tables: Vec>, + artifacts: Vec>, + module_upvars: Vec, + types: MyCow<'a, TypeTables>, ) -> Self { let isa = compiler.isa(); @@ -260,8 +268,9 @@ impl<'a> SerializedModule<'a> { strategy: compiler.strategy(), tunables: compiler.tunables().clone(), features: compiler.features().into(), - data, - tables, + artifacts, + module_upvars, + types, } } @@ -276,45 +285,95 @@ impl<'a> SerializedModule<'a> { self.check_tunables(compiler)?; self.check_features(compiler)?; - let types = self - .tables - .into_iter() - .map(|t| Arc::new(t.unwrap_owned())) - .collect::>(); - let module = mk(engine, &types, self.data)?; + let types = Arc::new(self.types.unwrap_owned()); + let mut modules = CompiledModule::from_artifacts_list( + self.artifacts + .into_iter() + .map(|i| i.unwrap_owned()) + .collect(), + engine.compiler().isa(), + &*engine.config().profiler, + )?; // Validate the module can be used with the current allocator - engine.allocator().validate(module.inner.module.module())?; + engine + .allocator() + .validate(modules.last().unwrap().module())?; - return Ok(module); + let (signatures, trampolines) = engine.register_module_signatures( + &types.wasm_signatures, + modules.iter().flat_map(|m| m.trampolines().iter().cloned()), + ); + + let signatures = Arc::new(ModuleSharedSignatures { + engine: engine.clone(), + signatures, + trampolines, + }); + + let module = modules.pop().unwrap(); + + let module_upvars = self + .module_upvars + .iter() + .map(|m| { + mk( + engine, + &modules, + &types, + m.index, + &m.artifact_upvars, + &m.module_upvars, + &signatures, + ) + }) + .collect::>>()?; + + return Ok(Module { + inner: Arc::new(ModuleInner { + engine: engine.clone(), + types, + module, + artifact_upvars: modules, + module_upvars, + signatures, + }), + }); fn mk( engine: &Engine, - types: &Vec>, - data: SerializedModuleData<'_>, + artifacts: &[Arc], + types: &Arc, + module_index: usize, + artifact_upvars: &[usize], + module_upvars: &[SerializedModuleUpvar], + signatures: &Arc, ) -> Result { - let mut artifacts = CompiledModule::from_artifacts_list( - data.artifacts - .into_iter() - .map(|i| i.unwrap_owned()) - .collect(), - engine.compiler().isa(), - &*engine.config().profiler, - )?; - let inner = ModuleInner { - engine: engine.clone(), - types: types[data.type_tables].clone(), - module: artifacts.pop().unwrap(), - artifact_upvars: artifacts, - module_upvars: data - .modules - .into_iter() - .map(|m| mk(engine, types, m)) - .collect::>>()?, - }; - Ok(Module { - inner: Arc::new(inner), + inner: Arc::new(ModuleInner { + engine: engine.clone(), + types: types.clone(), + module: artifacts[module_index].clone(), + artifact_upvars: artifact_upvars + .iter() + .map(|i| artifacts[*i].clone()) + .collect(), + module_upvars: module_upvars + .into_iter() + .map(|m| { + mk( + engine, + artifacts, + types, + m.index, + &m.artifact_upvars, + &m.module_upvars, + signatures, + ) + }) + .collect::>>()?, + signatures: signatures.clone(), + }), }) } } diff --git a/crates/wasmtime/src/sig_registry.rs b/crates/wasmtime/src/sig_registry.rs deleted file mode 100644 index e5811a8424..0000000000 --- a/crates/wasmtime/src/sig_registry.rs +++ /dev/null @@ -1,155 +0,0 @@ -//! Implement a registry of function signatures, for fast indirect call -//! signature checking. - -use crate::Module; -use std::collections::{hash_map, HashMap}; -use std::convert::TryFrom; -use wasmtime_environ::entity::PrimaryMap; -use wasmtime_environ::wasm::{SignatureIndex, WasmFuncType}; -use wasmtime_runtime::{VMSharedSignatureIndex, VMTrampoline}; - -/// WebAssembly requires that the caller and callee signatures in an indirect -/// call must match. To implement this efficiently, keep a registry of all -/// signatures, shared by all instances, so that call sites can just do an -/// index comparison. -#[derive(Debug, Default)] -pub struct SignatureRegistry { - // Map from a wasm actual function type to the index that it is assigned, - // shared amongst all wasm modules. - wasm2index: HashMap, - - // Map of all known wasm function signatures in this registry. This is - // keyed by `VMSharedSignatureIndex` above. - index_map: Vec, -} - -#[derive(Debug)] -struct Entry { - // The WebAssembly type signature, using wasm types. - wasm: WasmFuncType, - // The native trampoline used to invoke this type signature from `Func`. - // Note that the code memory for this trampoline is not owned by this - // type, but instead it's expected to be owned by the store that this - // registry lives within. - trampoline: Option, -} - -impl SignatureRegistry { - /// Registers all signatures within a module into this registry all at once. - /// - /// This will also internally register trampolines compiled in the module. - pub fn register_module(&mut self, module: &Module) { - // Register a unique index for all types in this module, even if they - // don't have a trampoline. - let signatures = &module.types().wasm_signatures; - for ty in module.compiled_module().module().types.values() { - if let wasmtime_environ::ModuleType::Function(index) = ty { - self.register_one(&signatures[*index], None); - } - } - - // Once we've got a shared index for all types used then also fill in - // any trampolines that the module has compiled as well. - for (index, trampoline) in module.compiled_module().trampolines() { - let shared = self.wasm2index[&signatures[*index]]; - let entry = &mut self.index_map[shared.bits() as usize]; - if entry.trampoline.is_none() { - entry.trampoline = Some(*trampoline); - } - } - } - - /// Register a signature and return its unique index. - pub fn register( - &mut self, - wasm: &WasmFuncType, - trampoline: VMTrampoline, - ) -> VMSharedSignatureIndex { - self.register_one(wasm, Some(trampoline)) - } - - fn register_one( - &mut self, - wasm: &WasmFuncType, - trampoline: Option, - ) -> VMSharedSignatureIndex { - let len = self.wasm2index.len(); - - match self.wasm2index.entry(wasm.clone()) { - hash_map::Entry::Occupied(entry) => { - let ret = *entry.get(); - let entry = &mut self.index_map[ret.bits() as usize]; - // If the entry does not previously have a trampoline, then - // overwrite it with whatever was specified by this function. - if entry.trampoline.is_none() { - entry.trampoline = trampoline; - } - ret - } - hash_map::Entry::Vacant(entry) => { - // Keep `signature_hash` len under 2**32 -- VMSharedSignatureIndex::new(std::u32::MAX) - // is reserved for VMSharedSignatureIndex::default(). - assert!( - len < std::u32::MAX as usize, - "Invariant check: signature_hash.len() < std::u32::MAX" - ); - debug_assert_eq!(len, self.index_map.len()); - let index = VMSharedSignatureIndex::new(u32::try_from(len).unwrap()); - self.index_map.push(Entry { - wasm: wasm.clone(), - trampoline, - }); - entry.insert(index); - index - } - } - } - - /// Looks up a shared index from the wasm signature itself. - pub fn lookup(&self, wasm: &WasmFuncType) -> Option { - self.wasm2index.get(wasm).cloned() - } - - /// Builds a lookup table for a module from the possible module's signature - /// indices to the shared signature index within this registry. - pub fn lookup_table( - &self, - module: &Module, - ) -> PrimaryMap { - // For module-linking using modules this builds up a map that is - // too large. This builds up a map for everything in `TypeTables` but - // that's all the types for all modules in a whole module linking graph, - // which our `module` may not be using. - // - // For all non-module-linking-using modules, though, this is not an - // issue. This is optimizing for the non-module-linking case right now - // and it seems like module linking will likely change to the point that - // this will no longer be an issue in the future. - let signatures = &module.types().wasm_signatures; - let mut map = PrimaryMap::with_capacity(signatures.len()); - for wasm in signatures.values() { - map.push( - self.wasm2index - .get(wasm) - .cloned() - .unwrap_or(VMSharedSignatureIndex::new(u32::MAX)), - ); - } - map - } - - /// Looks up information known about a shared signature index. - /// - /// Note that for this operation to be semantically correct the `idx` must - /// have previously come from a call to `register` of this same object. - pub fn lookup_shared( - &self, - idx: VMSharedSignatureIndex, - ) -> Option<(&WasmFuncType, VMTrampoline)> { - let (wasm, trampoline) = self - .index_map - .get(idx.bits() as usize) - .map(|e| (&e.wasm, e.trampoline))?; - Some((wasm, trampoline?)) - } -} diff --git a/crates/wasmtime/src/signatures.rs b/crates/wasmtime/src/signatures.rs new file mode 100644 index 0000000000..bd68f0d8ce --- /dev/null +++ b/crates/wasmtime/src/signatures.rs @@ -0,0 +1,185 @@ +//! Implement a registry of function signatures, for fast indirect call +//! signature checking. + +use std::collections::{hash_map::Entry, HashMap}; +use std::convert::TryFrom; +use wasmtime_environ::entity::PrimaryMap; +use wasmtime_environ::wasm::{SignatureIndex, WasmFuncType}; +use wasmtime_runtime::{VMSharedSignatureIndex, VMTrampoline}; + +/// Represents a mapping of shared signature index to trampolines. +/// +/// This is used in various places to store trampolines associated with shared +/// signature indexes. +/// +/// As multiple trampolines may exist for a single signature, the map entries +/// are internally reference counted. +#[derive(Default)] +pub struct TrampolineMap(HashMap); + +impl TrampolineMap { + /// Inserts a trampoline into the map. + pub fn insert(&mut self, index: VMSharedSignatureIndex, trampoline: VMTrampoline) { + let entry = match self.0.entry(index) { + Entry::Occupied(e) => e.into_mut(), + Entry::Vacant(e) => e.insert((0, trampoline)), + }; + + // Increment the ref count + entry.0 += 1; + } + + /// Gets a trampoline from the map. + pub fn get(&self, index: VMSharedSignatureIndex) -> Option { + self.0.get(&index).map(|(_, trampoline)| *trampoline) + } + + /// Iterates the shared signature indexes stored in the map. + /// + /// A shared signature index will be returned by the iterator for every + /// trampoline registered for that index, so duplicates may be present. + /// + /// This iterator can be used for deregistering signatures with the + /// signature registry. + pub fn indexes<'a>(&'a self) -> impl Iterator + 'a { + self.0 + .iter() + .flat_map(|(index, (count, _))| std::iter::repeat(*index).take(*count)) + } + + /// Determines if the trampoline map is empty. + pub fn is_empty(&self) -> bool { + self.0.is_empty() + } +} + +/// Represents a map between module signature indexes and +/// shared signature indexes. +pub type SharedSignatures = PrimaryMap; + +#[derive(Debug)] +struct RegistryEntry { + references: usize, + ty: WasmFuncType, +} + +/// WebAssembly requires that the caller and callee signatures in an indirect +/// call must match. To implement this efficiently, keep a registry of all +/// signatures, shared by all instances, so that call sites can just do an +/// index comparison. +#[derive(Debug, Default)] +pub struct SignatureRegistry { + map: HashMap, + entries: Vec>, + free: Vec, +} + +impl SignatureRegistry { + /// Registers a module with the signature registry from the collection of + /// all signatures and trampolines in the module. + pub fn register_module( + &mut self, + signatures: &PrimaryMap, + trampolines: impl Iterator, + ) -> (SharedSignatures, TrampolineMap) { + let mut sigs = SharedSignatures::default(); + let mut map = TrampolineMap::default(); + + for (_, ty) in signatures.iter() { + sigs.push(self.register(ty)); + } + + for (index, trampoline) in trampolines { + let index = self.map[&signatures[index]]; + map.insert(index, trampoline); + } + + (sigs, map) + } + + /// Registers a single signature with the registry. + /// + /// This is used for registering host functions created with the Wasmtime API. + pub fn register(&mut self, ty: &WasmFuncType) -> VMSharedSignatureIndex { + let len = self.map.len(); + + let index = match self.map.entry(ty.clone()) { + Entry::Occupied(e) => *e.get(), + Entry::Vacant(e) => { + let (index, entry) = match self.free.pop() { + Some(index) => (index, &mut self.entries[index.bits() as usize]), + None => { + // Keep `index_map` len under 2**32 -- VMSharedSignatureIndex::new(std::u32::MAX) + // is reserved for VMSharedSignatureIndex::default(). + assert!( + len < std::u32::MAX as usize, + "Invariant check: index_map.len() < std::u32::MAX" + ); + debug_assert_eq!(len, self.entries.len()); + + let index = VMSharedSignatureIndex::new(u32::try_from(len).unwrap()); + self.entries.push(None); + + (index, self.entries.last_mut().unwrap()) + } + }; + + *entry = Some(RegistryEntry { + references: 0, + ty: ty.clone(), + }); + + *e.insert(index) + } + }; + + self.entries[index.bits() as usize] + .as_mut() + .unwrap() + .references += 1; + + index + } + + /// Unregisters a collection of shared indexes from the registry. + pub fn unregister(&mut self, indexes: impl Iterator) { + for index in indexes { + let removed = { + let entry = self.entries[index.bits() as usize].as_mut().unwrap(); + + debug_assert!(entry.references > 0); + entry.references -= 1; + + if entry.references == 0 { + self.map.remove(&entry.ty); + self.free.push(index); + true + } else { + false + } + }; + + if removed { + self.entries[index.bits() as usize] = None; + } + } + } + + /// Looks up a function type from a shared signature index. + pub fn lookup_type(&self, index: VMSharedSignatureIndex) -> Option<&WasmFuncType> { + self.entries + .get(index.bits() as usize) + .and_then(|e| e.as_ref().map(|e| &e.ty)) + } + + /// Determines if the registry is semantically empty. + pub fn is_empty(&self) -> bool { + // If the map is empty, assert that all remaining entries are "free" + if self.map.is_empty() { + assert!(self.free.len() == self.entries.len()); + true + } else { + false + } + } +} diff --git a/crates/wasmtime/src/store.rs b/crates/wasmtime/src/store.rs index 4c78e82c8f..5cbe0119b5 100644 --- a/crates/wasmtime/src/store.rs +++ b/crates/wasmtime/src/store.rs @@ -1,12 +1,11 @@ -use crate::frame_info; -use crate::frame_info::StoreFrameInfo; -use crate::sig_registry::SignatureRegistry; -use crate::trampoline::StoreInstanceHandle; -use crate::{Engine, Func, FuncType, Module, Trap}; +use crate::{ + module::ModuleRegistry, signatures::TrampolineMap, trampoline::StoreInstanceHandle, Engine, + Func, Module, Trap, +}; use anyhow::{bail, Result}; use std::any::{Any, TypeId}; use std::cell::{Cell, RefCell}; -use std::collections::{hash_map::Entry, HashMap, HashSet}; +use std::collections::{hash_map::Entry, HashMap}; use std::convert::TryFrom; use std::fmt; use std::future::Future; @@ -16,12 +15,12 @@ use std::ptr; use std::rc::Rc; use std::sync::Arc; use std::task::{Context, Poll}; -use wasmtime_environ::wasm; +use wasmtime_environ::wasm::WasmFuncType; use wasmtime_jit::{CompiledModule, ModuleCode}; use wasmtime_runtime::{ - Export, InstanceAllocator, InstanceHandle, OnDemandInstanceAllocator, SignalHandler, - StackMapRegistry, TrapInfo, VMCallerCheckedAnyfunc, VMContext, VMExternRef, - VMExternRefActivationsTable, VMInterrupts, VMTrampoline, + InstanceAllocator, InstanceHandle, OnDemandInstanceAllocator, SignalHandler, StackMapRegistry, + TrapInfo, VMContext, VMExternRef, VMExternRefActivationsTable, VMInterrupts, + VMSharedSignatureIndex, VMTrampoline, }; /// Used to associate instances with the store. @@ -72,20 +71,13 @@ pub struct Store { pub(crate) struct StoreInner { engine: Engine, - /// The map of all host functions registered with this store's signature registry - host_funcs: RefCell>>, interrupts: Arc, - signatures: RefCell, instances: RefCell>, signal_handler: RefCell>>>, externref_activations_table: VMExternRefActivationsTable, stack_map_registry: StackMapRegistry, - /// Information about JIT code which allows us to test if a program counter - /// is in JIT code, lookup trap information, etc. - frame_info: RefCell, - /// Set of all compiled modules that we're holding a strong reference to - /// the module's code for. This includes JIT functions, trampolines, etc. - modules: RefCell>, + modules: RefCell, + trampolines: RefCell, // Numbers of resources instantiated in this store. instance_count: Cell, memory_count: Cell, @@ -137,21 +129,19 @@ impl Store { // once-per-thread. Platforms like Unix, however, only require this // once-per-program. In any case this is safe to call many times and // each one that's not relevant just won't do anything. - wasmtime_runtime::init_traps(frame_info::GlobalFrameInfo::is_wasm_pc) + wasmtime_runtime::init_traps(crate::module::GlobalModuleRegistry::is_wasm_pc) .expect("failed to initialize trap handling"); Store { inner: Rc::new(StoreInner { engine: engine.clone(), - host_funcs: RefCell::new(HashMap::new()), interrupts: Arc::new(Default::default()), - signatures: RefCell::new(Default::default()), instances: RefCell::new(Vec::new()), signal_handler: RefCell::new(None), externref_activations_table: VMExternRefActivationsTable::new(), stack_map_registry: StackMapRegistry::default(), - frame_info: Default::default(), - modules: Default::default(), + modules: RefCell::new(ModuleRegistry::default()), + trampolines: RefCell::new(TrampolineMap::default()), instance_count: Default::default(), memory_count: Default::default(), table_count: Default::default(), @@ -181,35 +171,6 @@ impl Store { }) } - pub(crate) fn get_host_anyfunc( - &self, - instance: &InstanceHandle, - ty: &FuncType, - trampoline: VMTrampoline, - ) -> *mut VMCallerCheckedAnyfunc { - let mut funcs = self.inner.host_funcs.borrow_mut(); - - let anyfunc = funcs.entry(unsafe { instance.clone() }).or_insert_with(|| { - let mut anyfunc = match instance - .lookup_by_declaration(&wasm::EntityIndex::Function(wasm::FuncIndex::from_u32(0))) - { - Export::Function(f) => unsafe { f.anyfunc.as_ref() }.clone(), - _ => unreachable!(), - }; - - // Register the function with this store's signature registry - anyfunc.type_index = self - .inner - .signatures - .borrow_mut() - .register(ty.as_wasm_func_type(), trampoline); - - Box::new(anyfunc) - }); - - &mut **anyfunc - } - /// Returns the [`Engine`] that this store is associated with. #[inline] pub fn engine(&self) -> &Engine { @@ -244,52 +205,16 @@ impl Store { } } - pub(crate) fn signatures(&self) -> &RefCell { - &self.inner.signatures - } - pub(crate) fn register_module(&self, module: &Module) { - // With a module being instantiated into this `Store` we need to - // preserve its jit-code. References to this module's code and - // trampolines are not owning-references so it's our responsibility to - // keep it all alive within the `Store`. - // - // If this module is already present in the store then we skip all - // further registration steps. - let first = self - .inner - .modules - .borrow_mut() - .insert(ArcModuleCode(module.compiled_module().code().clone())); - if !first { + // Register the module with the registry + if !self.inner.modules.borrow_mut().register(module) { return; } - // All modules register their JIT code in a store for two reasons - // currently: - // - // * First we only catch signals/traps if the program counter falls - // within the jit code of an instantiated wasm module. This ensures - // we don't catch accidental Rust/host segfaults. - // - // * Second when generating a backtrace we'll use this mapping to - // only generate wasm frames for instruction pointers that fall - // within jit code. - self.inner - .frame_info - .borrow_mut() - .register(module.compiled_module()); - // We need to know about all the stack maps of all instantiated modules // so when performing a GC we know about all wasm frames that we find // on the stack. self.register_stack_maps(module.compiled_module()); - - // Signatures are loaded into our `SignatureRegistry` here - // once-per-module (and once-per-signature). This allows us to create - // a `Func` wrapper for any function in the module, which requires that - // we know about the signature and trampoline for all instances. - self.signatures().borrow_mut().register_module(module); } fn register_stack_maps(&self, module: &CompiledModule) { @@ -304,6 +229,39 @@ impl Store { })); } + // This is used to register a `Func` with the store + pub(crate) fn register_signature( + &self, + ty: &WasmFuncType, + trampoline: VMTrampoline, + ) -> VMSharedSignatureIndex { + let index = self.inner.engine.register_signature(ty); + self.inner + .trampolines + .borrow_mut() + .insert(index, trampoline); + index + } + + pub(crate) fn lookup_trampoline(&self, index: VMSharedSignatureIndex) -> VMTrampoline { + // Look up the trampoline with the store's trampolines (from `Func`). + if let Some(trampoline) = self.inner.trampolines.borrow().get(index) { + return trampoline; + } + + // Look up the trampoline with the registered modules + if let Some(trampoline) = self.inner.modules.borrow().lookup_trampoline(index) { + return trampoline; + } + + // Lastly, check with the engine (for `HostFunc`) + self.inner + .engine + .host_func_trampolines() + .get(index) + .expect("trampoline missing") + } + pub(crate) fn bump_resource_counts(&self, module: &Module) -> Result<()> { let config = self.engine().config(); @@ -363,7 +321,7 @@ impl Store { .borrow() .iter() .any(|i| i.handle.vmctx_ptr() == handle.vmctx_ptr()) - || self.inner.host_funcs.borrow().get(&handle).is_some() + || self.inner.engine.host_func_anyfunc(&handle).is_some() ); StoreInstanceHandle { store: self.clone(), @@ -494,8 +452,8 @@ impl Store { &self.inner.stack_map_registry } - pub(crate) fn frame_info(&self) -> &RefCell { - &self.inner.frame_info + pub(crate) fn modules(&self) -> &RefCell { + &self.inner.modules } /// Notifies that the current Store (and all referenced entities) has been moved over to a @@ -984,6 +942,12 @@ impl Drop for StoreInner { } } } + + let trampolines = self.trampolines.borrow(); + + if !trampolines.is_empty() { + self.engine.unregister_signatures(trampolines.indexes()); + } } } diff --git a/crates/wasmtime/src/trampoline/func.rs b/crates/wasmtime/src/trampoline/func.rs index 9879880900..f2402efa6c 100644 --- a/crates/wasmtime/src/trampoline/func.rs +++ b/crates/wasmtime/src/trampoline/func.rs @@ -1,6 +1,6 @@ //! Support for a calling of an imported function. -use crate::{sig_registry::SignatureRegistry, Config, FuncType, Trap}; +use crate::{Config, FuncType, Store, Trap}; use anyhow::Result; use std::any::Any; use std::cmp; @@ -262,15 +262,15 @@ pub fn create_function( ft: &FuncType, func: Box Result<(), Trap>>, config: &Config, - registry: Option<&mut SignatureRegistry>, + store: Option<&Store>, ) -> Result<(InstanceHandle, VMTrampoline)> { let (module, finished_functions, trampoline, trampoline_state) = create_function_trampoline(config, ft, func)?; - // If there is no signature registry, use the default signature index which is + // If there is no store, use the default signature index which is // guaranteed to trap if there is ever an indirect call on the function (should not happen) - let shared_signature_id = registry - .map(|r| r.register(ft.as_wasm_func_type(), trampoline)) + let shared_signature_id = store + .map(|s| s.register_signature(ft.as_wasm_func_type(), trampoline)) .unwrap_or(VMSharedSignatureIndex::default()); unsafe { diff --git a/crates/wasmtime/src/trap.rs b/crates/wasmtime/src/trap.rs index b50f5353ee..1d59aa564c 100644 --- a/crates/wasmtime/src/trap.rs +++ b/crates/wasmtime/src/trap.rs @@ -161,7 +161,7 @@ impl Trap { maybe_interrupted, } => { let mut code = store - .frame_info() + .modules() .borrow() .lookup_trap_info(pc) .map(|info| info.trap_code) @@ -239,7 +239,7 @@ impl Trap { // (the call instruction) so we subtract one as the lookup. let pc_to_lookup = if Some(pc) == trap_pc { pc } else { pc - 1 }; if let Some((info, has_unparsed_debuginfo)) = - store.frame_info().borrow().lookup_frame_info(pc_to_lookup) + store.modules().borrow().lookup_frame_info(pc_to_lookup) { wasm_trace.push(info); diff --git a/crates/wasmtime/src/types.rs b/crates/wasmtime/src/types.rs index 2a91af473c..0241aee282 100644 --- a/crates/wasmtime/src/types.rs +++ b/crates/wasmtime/src/types.rs @@ -204,8 +204,7 @@ impl ExternType { ) -> ExternType { match ty { EntityType::Function(idx) => { - let sig = &types.wasm_signatures[*idx]; - FuncType::from_wasm_func_type(sig).into() + FuncType::from_wasm_func_type(types.wasm_signatures[*idx].clone()).into() } EntityType::Global(ty) => GlobalType::from_wasmtime_global(ty).into(), EntityType::Memory(ty) => MemoryType::from_wasmtime_memory(ty).into(), @@ -298,8 +297,8 @@ impl FuncType { &self.sig } - pub(crate) fn from_wasm_func_type(sig: &wasm::WasmFuncType) -> FuncType { - FuncType { sig: sig.clone() } + pub(crate) fn from_wasm_func_type(sig: wasm::WasmFuncType) -> FuncType { + Self { sig } } } diff --git a/crates/wasmtime/src/types/matching.rs b/crates/wasmtime/src/types/matching.rs index a767bbb7e1..55a55c1d62 100644 --- a/crates/wasmtime/src/types/matching.rs +++ b/crates/wasmtime/src/types/matching.rs @@ -1,4 +1,4 @@ -use crate::{Extern, Store}; +use crate::{signatures::SharedSignatures, Extern, Store}; use anyhow::{bail, Context, Result}; use wasmtime_environ::wasm::{ EntityType, Global, InstanceTypeIndex, Memory, ModuleTypeIndex, SignatureIndex, Table, @@ -6,6 +6,7 @@ use wasmtime_environ::wasm::{ use wasmtime_jit::TypeTables; pub struct MatchCx<'a> { + pub signatures: &'a SharedSignatures, pub types: &'a TypeTables, pub store: &'a Store, } @@ -70,13 +71,8 @@ impl MatchCx<'_> { } pub fn func(&self, expected: SignatureIndex, actual: &crate::Func) -> Result<()> { - let matches = match self - .store - .signatures() - .borrow() - .lookup(&self.types.wasm_signatures[expected]) - { - Some(idx) => actual.sig_index() == idx, + let matches = match self.signatures.get(expected) { + Some(idx) => actual.sig_index() == *idx, // If our expected signature isn't registered, then there's no way // that `actual` can match it. None => false, @@ -114,15 +110,19 @@ impl MatchCx<'_> { let module = actual.compiled_module().module(); self.imports_match( expected, + actual.signatures(), actual.types(), module.imports().map(|(name, field, ty)| { assert!(field.is_none()); // should be true if module linking is enabled (name, ty) }), )?; - self.exports_match(expected_sig.exports, actual.types(), |name| { - module.exports.get(name).map(|idx| module.type_of(*idx)) - })?; + self.exports_match( + expected_sig.exports, + actual.signatures(), + actual.types(), + |name| module.exports.get(name).map(|idx| module.type_of(*idx)), + )?; Ok(()) } @@ -133,6 +133,7 @@ impl MatchCx<'_> { fn imports_match<'a>( &self, expected: ModuleTypeIndex, + actual_signatures: &SharedSignatures, actual_types: &TypeTables, actual_imports: impl Iterator, ) -> Result<()> { @@ -146,10 +147,11 @@ impl MatchCx<'_> { None => bail!("expected type doesn't import {:?}", name), }; MatchCx { + signatures: actual_signatures, types: actual_types, store: self.store, } - .extern_ty_matches(&actual_ty, expected_ty, self.types) + .extern_ty_matches(&actual_ty, expected_ty, self.signatures, self.types) .with_context(|| format!("module import {:?} incompatible", name))?; } Ok(()) @@ -160,6 +162,7 @@ impl MatchCx<'_> { fn exports_match( &self, expected: InstanceTypeIndex, + actual_signatures: &SharedSignatures, actual_types: &TypeTables, lookup: impl Fn(&str) -> Option, ) -> Result<()> { @@ -169,7 +172,7 @@ impl MatchCx<'_> { for (name, expected) in self.types.instance_signatures[expected].exports.iter() { match lookup(name) { Some(ty) => self - .extern_ty_matches(expected, &ty, actual_types) + .extern_ty_matches(expected, &ty, actual_signatures, actual_types) .with_context(|| format!("export {:?} incompatible", name))?, None => bail!("failed to find export {:?}", name), } @@ -183,6 +186,7 @@ impl MatchCx<'_> { &self, expected: &EntityType, actual_ty: &EntityType, + actual_signatures: &SharedSignatures, actual_types: &TypeTables, ) -> Result<()> { let actual_desc = match actual_ty { @@ -221,7 +225,7 @@ impl MatchCx<'_> { EntityType::Instance(expected) => match actual_ty { EntityType::Instance(actual) => { let sig = &actual_types.instance_signatures[*actual]; - self.exports_match(*expected, actual_types, |name| { + self.exports_match(*expected, actual_signatures, actual_types, |name| { sig.exports.get(name).cloned() })?; Ok(()) @@ -237,15 +241,19 @@ impl MatchCx<'_> { self.imports_match( *expected, + actual_signatures, actual_types, actual_module_sig .imports .iter() .map(|(module, ty)| (module.as_str(), ty.clone())), )?; - self.exports_match(expected_module_sig.exports, actual_types, |name| { - actual_instance_sig.exports.get(name).cloned() - })?; + self.exports_match( + expected_module_sig.exports, + actual_signatures, + actual_types, + |name| actual_instance_sig.exports.get(name).cloned(), + )?; Ok(()) } _ => bail!("expected module, but found {}", actual_desc), From ea72c621f07644609c87252d5fb4454d8d8f0e49 Mon Sep 17 00:00:00 2001 From: Peter Huene Date: Wed, 14 Apr 2021 13:45:56 -0700 Subject: [PATCH 2/8] Remove the stack map registry. This commit removes the stack map registry and instead uses the existing information from the store's module registry to lookup stack maps. A trait is now used to pass the lookup context to the runtime, implemented by `Store` to do the lookup. With this change, module registration in `Store` is now entirely limited to inserting the module into the module registry. --- crates/environ/src/vmoffsets.rs | 18 +- crates/jit/src/instantiate.rs | 11 +- crates/runtime/src/externref.rs | 201 ++---------------- crates/runtime/src/instance.rs | 8 +- crates/runtime/src/instance/allocator.rs | 8 +- .../runtime/src/instance/allocator/pooling.rs | 4 +- .../src/instance/allocator/pooling/uffd.rs | 2 +- crates/runtime/src/libcalls.rs | 8 +- crates/wasmtime/src/func/typed.rs | 2 +- crates/wasmtime/src/instance.rs | 5 +- crates/wasmtime/src/module/registry.rs | 82 ++++++- crates/wasmtime/src/store.rs | 57 ++--- crates/wasmtime/src/trampoline.rs | 6 +- .../wasmtime/src/trampoline/create_handle.rs | 4 +- crates/wasmtime/src/trampoline/func.rs | 4 +- crates/wasmtime/src/values.rs | 2 +- 16 files changed, 158 insertions(+), 264 deletions(-) diff --git a/crates/environ/src/vmoffsets.rs b/crates/environ/src/vmoffsets.rs index e5d6604610..20f3b8a298 100644 --- a/crates/environ/src/vmoffsets.rs +++ b/crates/environ/src/vmoffsets.rs @@ -6,7 +6,7 @@ // struct VMContext { // interrupts: *const VMInterrupts, // externref_activations_table: *mut VMExternRefActivationsTable, -// stack_map_registry: *mut StackMapRegistry, +// stack_map_lookup: *const dyn StackMapLookup, // signature_ids: [VMSharedSignatureIndex; module.num_signature_ids], // imported_functions: [VMFunctionImport; module.num_imported_functions], // imported_tables: [VMTableImport; module.num_imported_tables], @@ -77,7 +77,7 @@ pub struct VMOffsets { // precalculated offsets of various member fields interrupts: u32, externref_activations_table: u32, - stack_map_registry: u32, + stack_map_lookup: u32, signature_ids: u32, imported_functions: u32, imported_tables: u32, @@ -149,7 +149,7 @@ impl From for VMOffsets { num_defined_globals: fields.num_defined_globals, interrupts: 0, externref_activations_table: 0, - stack_map_registry: 0, + stack_map_lookup: 0, signature_ids: 0, imported_functions: 0, imported_tables: 0, @@ -168,13 +168,13 @@ impl From for VMOffsets { .interrupts .checked_add(u32::from(fields.pointer_size)) .unwrap(); - ret.stack_map_registry = ret + ret.stack_map_lookup = ret .externref_activations_table .checked_add(u32::from(fields.pointer_size)) .unwrap(); ret.signature_ids = ret - .stack_map_registry - .checked_add(u32::from(fields.pointer_size)) + .stack_map_lookup + .checked_add(u32::from(fields.pointer_size * 2)) .unwrap(); ret.imported_functions = ret .signature_ids @@ -507,10 +507,10 @@ impl VMOffsets { self.externref_activations_table } - /// The offset of the `*mut StackMapRegistry` member. + /// The offset of the `*const dyn StackMapLookup` member. #[inline] - pub fn vmctx_stack_map_registry(&self) -> u32 { - self.stack_map_registry + pub fn vmctx_stack_map_lookup(&self) -> u32 { + self.stack_map_lookup } /// The offset of the `signature_ids` array. diff --git a/crates/jit/src/instantiate.rs b/crates/jit/src/instantiate.rs index 2d155e3d7b..5527051ecd 100644 --- a/crates/jit/src/instantiate.rs +++ b/crates/jit/src/instantiate.rs @@ -362,11 +362,18 @@ impl CompiledModule { } /// Gets the function information for a given function index. - pub fn func_info(&self, index: DefinedFuncIndex) -> (&FunctionAddressMap, &[TrapInformation]) { + pub fn func_info( + &self, + index: DefinedFuncIndex, + ) -> ( + &FunctionAddressMap, + &[TrapInformation], + &[StackMapInformation], + ) { self.artifacts .funcs .get(index) - .map(|f| (&f.address_map, f.traps.as_ref())) + .map(|f| (&f.address_map, f.traps.as_ref(), f.stack_maps.as_ref())) .expect("defined function should be present") } diff --git a/crates/runtime/src/externref.rs b/crates/runtime/src/externref.rs index 02943c1c01..0606a941e6 100644 --- a/crates/runtime/src/externref.rs +++ b/crates/runtime/src/externref.rs @@ -103,14 +103,12 @@ use std::alloc::Layout; use std::any::Any; use std::cell::{Cell, RefCell, UnsafeCell}; use std::cmp::Ordering; -use std::collections::BTreeMap; use std::collections::HashSet; use std::hash::{Hash, Hasher}; use std::mem; use std::ops::Deref; use std::ptr::{self, NonNull}; -use std::rc::Rc; -use wasmtime_environ::{ir::StackMap, StackMapInformation}; +use wasmtime_environ::ir::StackMap; /// An external reference to some opaque data. /// @@ -596,10 +594,10 @@ impl VMExternRefActivationsTable { pub unsafe fn insert_with_gc( &self, externref: VMExternRef, - stack_maps_registry: &StackMapRegistry, + stack_map_lookup: &dyn StackMapLookup, ) { if let Err(externref) = self.try_insert(externref) { - self.gc_and_insert_slow(externref, stack_maps_registry); + self.gc_and_insert_slow(externref, stack_map_lookup); } } @@ -607,9 +605,9 @@ impl VMExternRefActivationsTable { unsafe fn gc_and_insert_slow( &self, externref: VMExternRef, - stack_maps_registry: &StackMapRegistry, + stack_map_lookup: &dyn StackMapLookup, ) { - gc(stack_maps_registry, self); + gc(stack_map_lookup, self); // Might as well insert right into the hash set, rather than the bump // chunk, since we are already on a slow path and we get de-duplication @@ -743,182 +741,21 @@ impl VMExternRefActivationsTable { } } -/// A registry of stack maps for currently active Wasm modules. -#[derive(Default)] -pub struct StackMapRegistry { - inner: RefCell, +/// Used by the runtime to lookup a stack map from a PC value. +pub trait StackMapLookup { + /// Lookup the stack map at a program counter (PC) value. + fn lookup(&self, pc: usize) -> Option<*const StackMap>; } -#[derive(Default)] -struct StackMapRegistryInner { - /// A map from the highest pc in a module, to its stack maps. - /// - /// For details, see the comment above `GlobalModuleRegistry`. - ranges: BTreeMap, -} +pub(crate) struct EmptyStackMapLookup; -#[derive(Debug)] -struct ModuleStackMaps { - /// The range of PCs that this module covers. Different modules must always - /// have distinct ranges. - range: std::ops::Range, - - /// A map from a PC in this module (that is a GC safepoint) to its - /// associated stack map. If `None` then it means that the PC is the start - /// of a range which has no stack map. - pc_to_stack_map: Vec<(usize, Option>)>, -} - -impl StackMapRegistry { - /// Register the stack maps for a given module. - /// - /// The stack maps should be given as an iterator over a function's PC range - /// in memory (that is, where the JIT actually allocated and emitted the - /// function's code at), and the stack maps and code offsets within that - /// range for each of its GC safepoints. - pub fn register_stack_maps<'a>( - &self, - stack_maps: impl IntoIterator, &'a [StackMapInformation])>, - ) { - let mut min = usize::max_value(); - let mut max = 0; - let mut pc_to_stack_map = vec![]; - let mut last_is_none_marker = true; - - for (range, infos) in stack_maps { - let len = range.end - range.start; - - min = std::cmp::min(min, range.start); - max = std::cmp::max(max, range.end); - - // Add a marker between functions indicating that this function's pc - // starts with no stack map so when our binary search later on finds - // a pc between the start of the function and the function's first - // stack map it doesn't think the previous stack map is our stack - // map. - // - // We skip this if the previous entry pushed was also a `None` - // marker, in which case the starting pc already has no stack map. - // This is also skipped if the first `code_offset` is zero since - // what we'll push applies for the first pc anyway. - if !last_is_none_marker && (infos.is_empty() || infos[0].code_offset > 0) { - pc_to_stack_map.push((range.start, None)); - last_is_none_marker = true; - } - - for info in infos { - assert!((info.code_offset as usize) < len); - pc_to_stack_map.push(( - range.start + (info.code_offset as usize), - Some(Rc::new(info.stack_map.clone())), - )); - last_is_none_marker = false; - } - } - - if pc_to_stack_map.is_empty() { - // Nothing to register. - return; - } - - let module_stack_maps = ModuleStackMaps { - range: min..max, - pc_to_stack_map, - }; - - let mut inner = self.inner.borrow_mut(); - - // Assert that this chunk of ranges doesn't collide with any other known - // chunks. - if let Some((_, prev)) = inner.ranges.range(max..).next() { - assert!(prev.range.start > max); - } - if let Some((prev_end, _)) = inner.ranges.range(..=min).next_back() { - assert!(*prev_end < min); - } - - let old = inner.ranges.insert(max, module_stack_maps); - assert!(old.is_none()); - } - - /// Lookup the stack map for the given PC, if any. - pub fn lookup_stack_map(&self, pc: usize) -> Option> { - let inner = self.inner.borrow(); - let stack_maps = inner.module_stack_maps(pc)?; - - // Do a binary search to find the stack map for the given PC. - // - // Because GC safepoints are technically only associated with a single - // PC, we should ideally only care about `Ok(index)` values returned - // from the binary search. However, safepoints are inserted right before - // calls, and there are two things that can disturb the PC/offset - // associated with the safepoint versus the PC we actually use to query - // for the stack map: - // - // 1. The `backtrace` crate gives us the PC in a frame that will be - // *returned to*, and where execution will continue from, rather than - // the PC of the call we are currently at. So we would need to - // disassemble one instruction backwards to query the actual PC for - // the stack map. - // - // TODO: One thing we *could* do to make this a little less error - // prone, would be to assert/check that the nearest GC safepoint - // found is within `max_encoded_size(any kind of call instruction)` - // our queried PC for the target architecture. - // - // 2. Cranelift's stack maps only handle the stack, not - // registers. However, some references that are arguments to a call - // may need to be in registers. In these cases, what Cranelift will - // do is: - // - // a. spill all the live references, - // b. insert a GC safepoint for those references, - // c. reload the references into registers, and finally - // d. make the call. - // - // Step (c) adds drift between the GC safepoint and the location of - // the call, which is where we actually walk the stack frame and - // collect its live references. - // - // Luckily, the spill stack slots for the live references are still - // up to date, so we can still find all the on-stack roots. - // Furthermore, we do not have a moving GC, so we don't need to worry - // whether the following code will reuse the references in registers - // (which would not have been updated to point to the moved objects) - // or reload from the stack slots (which would have been updated to - // point to the moved objects). - let index = match stack_maps - .pc_to_stack_map - .binary_search_by_key(&pc, |(pc, _stack_map)| *pc) - { - // Exact hit. - Ok(i) => i, - - // `Err(0)` means that the associated stack map would have been the - // first element in the array if this pc had an associated stack - // map, but this pc does not have an associated stack map. This can - // only happen inside a Wasm frame if there are no live refs at this - // pc. - Err(0) => return None, - - Err(n) => n - 1, - }; - - let stack_map = stack_maps.pc_to_stack_map[index].1.as_ref()?.clone(); - Some(stack_map) +impl StackMapLookup for EmptyStackMapLookup { + fn lookup(&self, _pc: usize) -> Option<*const StackMap> { + None } } -impl StackMapRegistryInner { - fn module_stack_maps(&self, pc: usize) -> Option<&ModuleStackMaps> { - let (end, stack_maps) = self.ranges.range(pc..).next()?; - if pc < stack_maps.range.start || *end < pc { - None - } else { - Some(stack_maps) - } - } -} +pub(crate) const EMPTY_STACK_MAP_LOOKUP: EmptyStackMapLookup = EmptyStackMapLookup; #[derive(Debug, Default)] struct DebugOnly { @@ -965,7 +802,7 @@ impl std::ops::DerefMut for DebugOnly { /// Additionally, you must have registered the stack maps for every Wasm module /// that has frames on the stack with the given `stack_maps_registry`. pub unsafe fn gc( - stack_maps_registry: &StackMapRegistry, + stack_map_lookup: &dyn StackMapLookup, externref_activations_table: &VMExternRefActivationsTable, ) { // We borrow the precise stack roots `RefCell` for the whole duration of @@ -1003,7 +840,7 @@ pub unsafe fn gc( if cfg!(debug_assertions) { // Assert that there aren't any Wasm frames on the stack. backtrace::trace(|frame| { - let stack_map = stack_maps_registry.lookup_stack_map(frame.ip() as usize); + let stack_map = stack_map_lookup.lookup(frame.ip() as usize); assert!(stack_map.is_none()); true }); @@ -1048,11 +885,11 @@ pub unsafe fn gc( let pc = frame.ip() as usize; let sp = frame.sp() as usize; - if let Some(stack_map) = stack_maps_registry.lookup_stack_map(pc) { + if let Some(stack_map) = stack_map_lookup.lookup(pc) { debug_assert!(sp != 0, "we should always get a valid SP for Wasm frames"); - for i in 0..(stack_map.mapped_words() as usize) { - if stack_map.get_bit(i) { + for i in 0..((*stack_map).mapped_words() as usize) { + if (*stack_map).get_bit(i) { // Stack maps have one bit per word in the frame, and the // zero^th bit is the *lowest* addressed word in the frame, // i.e. the closest to the SP. So to get the `i`^th word in diff --git a/crates/runtime/src/instance.rs b/crates/runtime/src/instance.rs index bf4234f6b8..33a2583cff 100644 --- a/crates/runtime/src/instance.rs +++ b/crates/runtime/src/instance.rs @@ -3,7 +3,7 @@ //! `InstanceHandle` is a reference-counting handle for an `Instance`. use crate::export::Export; -use crate::externref::{StackMapRegistry, VMExternRefActivationsTable}; +use crate::externref::{StackMapLookup, VMExternRefActivationsTable}; use crate::memory::{Memory, RuntimeMemoryCreator}; use crate::table::{Table, TableElement}; use crate::traphandlers::Trap; @@ -249,9 +249,9 @@ impl Instance { unsafe { self.vmctx_plus_offset(self.offsets.vmctx_externref_activations_table()) } } - /// Return a pointer to the `StackMapRegistry`. - pub fn stack_map_registry(&self) -> *mut *mut StackMapRegistry { - unsafe { self.vmctx_plus_offset(self.offsets.vmctx_stack_map_registry()) } + /// Return a pointer to the `StackMapLookup`. + pub fn stack_map_lookup(&self) -> *mut *const dyn StackMapLookup { + unsafe { self.vmctx_plus_offset(self.offsets.vmctx_stack_map_lookup()) } } /// Return a reference to the vmctx used by compiled wasm code. diff --git a/crates/runtime/src/instance/allocator.rs b/crates/runtime/src/instance/allocator.rs index d236b4723c..14a9d2eff3 100644 --- a/crates/runtime/src/instance/allocator.rs +++ b/crates/runtime/src/instance/allocator.rs @@ -1,4 +1,4 @@ -use crate::externref::{StackMapRegistry, VMExternRefActivationsTable}; +use crate::externref::{StackMapLookup, VMExternRefActivationsTable, EMPTY_STACK_MAP_LOOKUP}; use crate::imports::Imports; use crate::instance::{Instance, InstanceHandle, RuntimeMemoryCreator}; use crate::memory::{DefaultMemoryCreator, Memory}; @@ -57,8 +57,8 @@ pub struct InstanceAllocationRequest<'a> { /// The pointer to the reference activations table to use for the instance. pub externref_activations_table: *mut VMExternRefActivationsTable, - /// The pointer to the stack map registry to use for the instance. - pub stack_map_registry: *mut StackMapRegistry, + /// The pointer to the stack map lookup to use for the instance. + pub stack_map_lookup: Option<*const dyn StackMapLookup>, } /// An link error while instantiating a module. @@ -447,7 +447,7 @@ unsafe fn initialize_vmcontext(instance: &Instance, req: InstanceAllocationReque *instance.interrupts() = req.interrupts; *instance.externref_activations_table() = req.externref_activations_table; - *instance.stack_map_registry() = req.stack_map_registry; + *instance.stack_map_lookup() = req.stack_map_lookup.unwrap_or(&EMPTY_STACK_MAP_LOOKUP); // Initialize shared signatures let mut ptr = instance.signature_ids_ptr(); diff --git a/crates/runtime/src/instance/allocator/pooling.rs b/crates/runtime/src/instance/allocator/pooling.rs index d9604c348f..6b89ce603e 100644 --- a/crates/runtime/src/instance/allocator/pooling.rs +++ b/crates/runtime/src/instance/allocator/pooling.rs @@ -1370,7 +1370,7 @@ mod test { host_state: Box::new(()), interrupts: std::ptr::null(), externref_activations_table: std::ptr::null_mut(), - stack_map_registry: std::ptr::null_mut(), + stack_map_lookup: None, }, ) .expect("allocation should succeed"), @@ -1394,7 +1394,7 @@ mod test { host_state: Box::new(()), interrupts: std::ptr::null(), externref_activations_table: std::ptr::null_mut(), - stack_map_registry: std::ptr::null_mut(), + stack_map_lookup: None, }, ) { Err(InstantiationError::Limit(3)) => {} diff --git a/crates/runtime/src/instance/allocator/pooling/uffd.rs b/crates/runtime/src/instance/allocator/pooling/uffd.rs index 5578ca1746..fb811680bb 100644 --- a/crates/runtime/src/instance/allocator/pooling/uffd.rs +++ b/crates/runtime/src/instance/allocator/pooling/uffd.rs @@ -523,7 +523,7 @@ mod test { host_state: Box::new(()), interrupts: ptr::null(), externref_activations_table: ptr::null_mut(), - stack_map_registry: ptr::null_mut(), + stack_map_lookup: None, }, ) .expect("instance should allocate"), diff --git a/crates/runtime/src/libcalls.rs b/crates/runtime/src/libcalls.rs index 026738a8a1..58eb585312 100644 --- a/crates/runtime/src/libcalls.rs +++ b/crates/runtime/src/libcalls.rs @@ -449,8 +449,8 @@ pub unsafe extern "C" fn wasmtime_activations_table_insert_with_gc( let externref = VMExternRef::clone_from_raw(externref); let instance = (&mut *vmctx).instance(); let activations_table = &**instance.externref_activations_table(); - let registry = &**instance.stack_map_registry(); - activations_table.insert_with_gc(externref, registry); + let stack_map_lookup = &**instance.stack_map_lookup(); + activations_table.insert_with_gc(externref, stack_map_lookup); } /// Perform a Wasm `global.get` for `externref` globals. @@ -466,8 +466,8 @@ pub unsafe extern "C" fn wasmtime_externref_global_get( Some(externref) => { let raw = externref.as_raw(); let activations_table = &**instance.externref_activations_table(); - let registry = &**instance.stack_map_registry(); - activations_table.insert_with_gc(externref, registry); + let stack_map_lookup = &**instance.stack_map_lookup(); + activations_table.insert_with_gc(externref, stack_map_lookup); raw } } diff --git a/crates/wasmtime/src/func/typed.rs b/crates/wasmtime/src/func/typed.rs index c0a1d834da..b0fe8b574f 100644 --- a/crates/wasmtime/src/func/typed.rs +++ b/crates/wasmtime/src/func/typed.rs @@ -207,7 +207,7 @@ unsafe impl WasmTy for Option { unsafe { store .externref_activations_table() - .insert_with_gc(x.inner, store.stack_map_registry()); + .insert_with_gc(x.inner, &*store.stack_map_lookup()); } abi } else { diff --git a/crates/wasmtime/src/instance.rs b/crates/wasmtime/src/instance.rs index 00cc90fd82..a30ef98414 100644 --- a/crates/wasmtime/src/instance.rs +++ b/crates/wasmtime/src/instance.rs @@ -13,7 +13,7 @@ use wasmtime_environ::wasm::{ }; use wasmtime_environ::Initializer; use wasmtime_runtime::{ - Imports, InstanceAllocationRequest, InstantiationError, RuntimeInstance, StackMapRegistry, + Imports, InstanceAllocationRequest, InstantiationError, RuntimeInstance, StackMapLookup, VMContext, VMExternRefActivationsTable, VMFunctionBody, VMFunctionImport, VMGlobalImport, VMMemoryImport, VMTableImport, }; @@ -525,8 +525,7 @@ impl<'a> Instantiator<'a> { externref_activations_table: self.store.externref_activations_table() as *const VMExternRefActivationsTable as *mut _, - stack_map_registry: self.store.stack_map_registry() as *const StackMapRegistry - as *mut _, + stack_map_lookup: Some(self.store.stack_map_lookup() as *const dyn StackMapLookup), })?; // After we've created the `InstanceHandle` we still need to run diff --git a/crates/wasmtime/src/module/registry.rs b/crates/wasmtime/src/module/registry.rs index 161ef408d6..bec0434a91 100644 --- a/crates/wasmtime/src/module/registry.rs +++ b/crates/wasmtime/src/module/registry.rs @@ -43,6 +43,11 @@ impl ModuleRegistry { self.module(pc)?.lookup_trap_info(pc) } + /// Looks up a stack map from a program counter. + pub fn lookup_stack_map<'a>(&'a self, pc: usize) -> Option<&'a ir::StackMap> { + self.module(pc)?.lookup_stack_map(pc) + } + fn module(&self, pc: usize) -> Option<&RegisteredModule> { let (end, info) = self.0.range(pc..).next()?; if pc < info.start || *end < pc { @@ -53,13 +58,13 @@ impl ModuleRegistry { } /// Registers a new module with the registry. - pub fn register(&mut self, module: &Module) -> bool { + pub fn register(&mut self, module: &Module) { let compiled_module = module.compiled_module(); let (start, end) = compiled_module.code().range(); // Ignore modules with no code, finished functions, or if the module is already registered if start == end || compiled_module.finished_functions().is_empty() { - return false; + return; } // The module code range is exclusive for end, so make it inclusive as it @@ -67,7 +72,7 @@ impl ModuleRegistry { let end = end - 1; if self.0.get(&end).is_some() { - return false; + return; } // Assert that this module's code doesn't collide with any other registered modules @@ -90,7 +95,6 @@ impl ModuleRegistry { assert!(prev.is_none()); GLOBAL_MODULES.lock().unwrap().register(start, end, module); - true } /// Looks up a trampoline from a shared signature index. @@ -135,7 +139,7 @@ impl RegisteredModule { /// if no information can be found. pub fn lookup_frame_info(&self, pc: usize) -> Option { let (index, offset) = self.func(pc)?; - let (addr_map, _) = self.module.func_info(index); + let (addr_map, _, _) = self.module.func_info(index); let pos = Self::instr_pos(offset, addr_map); // In debug mode for now assert that we found a mapping for `pc` within @@ -197,13 +201,77 @@ impl RegisteredModule { /// Fetches trap information about a program counter in a backtrace. pub fn lookup_trap_info(&self, pc: usize) -> Option<&TrapInformation> { let (index, offset) = self.func(pc)?; - let (_, traps) = self.module.func_info(index); + let (_, traps, _) = self.module.func_info(index); let idx = traps .binary_search_by_key(&offset, |info| info.code_offset) .ok()?; Some(&traps[idx]) } + /// Looks up a stack map from a program counter + pub fn lookup_stack_map(&self, pc: usize) -> Option<&ir::StackMap> { + let (index, offset) = self.func(pc)?; + let (_, _, stack_maps) = self.module.func_info(index); + + // Do a binary search to find the stack map for the given offset. + // + // Because GC safepoints are technically only associated with a single + // PC, we should ideally only care about `Ok(index)` values returned + // from the binary search. However, safepoints are inserted right before + // calls, and there are two things that can disturb the PC/offset + // associated with the safepoint versus the PC we actually use to query + // for the stack map: + // + // 1. The `backtrace` crate gives us the PC in a frame that will be + // *returned to*, and where execution will continue from, rather than + // the PC of the call we are currently at. So we would need to + // disassemble one instruction backwards to query the actual PC for + // the stack map. + // + // TODO: One thing we *could* do to make this a little less error + // prone, would be to assert/check that the nearest GC safepoint + // found is within `max_encoded_size(any kind of call instruction)` + // our queried PC for the target architecture. + // + // 2. Cranelift's stack maps only handle the stack, not + // registers. However, some references that are arguments to a call + // may need to be in registers. In these cases, what Cranelift will + // do is: + // + // a. spill all the live references, + // b. insert a GC safepoint for those references, + // c. reload the references into registers, and finally + // d. make the call. + // + // Step (c) adds drift between the GC safepoint and the location of + // the call, which is where we actually walk the stack frame and + // collect its live references. + // + // Luckily, the spill stack slots for the live references are still + // up to date, so we can still find all the on-stack roots. + // Furthermore, we do not have a moving GC, so we don't need to worry + // whether the following code will reuse the references in registers + // (which would not have been updated to point to the moved objects) + // or reload from the stack slots (which would have been updated to + // point to the moved objects). + + let index = match stack_maps.binary_search_by_key(&offset, |i| i.code_offset) { + // Exact hit. + Ok(i) => i, + + // `Err(0)` means that the associated stack map would have been the + // first element in the array if this pc had an associated stack + // map, but this pc does not have an associated stack map. This can + // only happen inside a Wasm frame if there are no live refs at this + // pc. + Err(0) => return None, + + Err(i) => i - 1, + }; + + Some(&stack_maps[index].stack_map) + } + fn func(&self, pc: usize) -> Option<(DefinedFuncIndex, u32)> { let (index, start, _) = self.module.func_by_pc(pc)?; Some((index, (pc - start) as u32)) @@ -260,7 +328,7 @@ impl GlobalModuleRegistry { match info.module.func(pc) { Some((index, offset)) => { - let (addr_map, _) = info.module.module.func_info(index); + let (addr_map, _, _) = info.module.module.func_info(index); RegisteredModule::instr_pos(offset, addr_map).is_some() } None => false, diff --git a/crates/wasmtime/src/store.rs b/crates/wasmtime/src/store.rs index 5cbe0119b5..f337b39975 100644 --- a/crates/wasmtime/src/store.rs +++ b/crates/wasmtime/src/store.rs @@ -16,11 +16,11 @@ use std::rc::Rc; use std::sync::Arc; use std::task::{Context, Poll}; use wasmtime_environ::wasm::WasmFuncType; -use wasmtime_jit::{CompiledModule, ModuleCode}; +use wasmtime_jit::ModuleCode; use wasmtime_runtime::{ - InstanceAllocator, InstanceHandle, OnDemandInstanceAllocator, SignalHandler, StackMapRegistry, - TrapInfo, VMContext, VMExternRef, VMExternRefActivationsTable, VMInterrupts, - VMSharedSignatureIndex, VMTrampoline, + InstanceAllocator, InstanceHandle, OnDemandInstanceAllocator, SignalHandler, TrapInfo, + VMContext, VMExternRef, VMExternRefActivationsTable, VMInterrupts, VMSharedSignatureIndex, + VMTrampoline, }; /// Used to associate instances with the store. @@ -75,7 +75,6 @@ pub(crate) struct StoreInner { instances: RefCell>, signal_handler: RefCell>>>, externref_activations_table: VMExternRefActivationsTable, - stack_map_registry: StackMapRegistry, modules: RefCell, trampolines: RefCell, // Numbers of resources instantiated in this store. @@ -139,7 +138,6 @@ impl Store { instances: RefCell::new(Vec::new()), signal_handler: RefCell::new(None), externref_activations_table: VMExternRefActivationsTable::new(), - stack_map_registry: StackMapRegistry::default(), modules: RefCell::new(ModuleRegistry::default()), trampolines: RefCell::new(TrampolineMap::default()), instance_count: Default::default(), @@ -207,26 +205,7 @@ impl Store { pub(crate) fn register_module(&self, module: &Module) { // Register the module with the registry - if !self.inner.modules.borrow_mut().register(module) { - return; - } - - // We need to know about all the stack maps of all instantiated modules - // so when performing a GC we know about all wasm frames that we find - // on the stack. - self.register_stack_maps(module.compiled_module()); - } - - fn register_stack_maps(&self, module: &CompiledModule) { - self.stack_map_registry() - .register_stack_maps(module.stack_maps().map(|(func, stack_maps)| unsafe { - let ptr = (*func).as_ptr(); - let len = (*func).len(); - let start = ptr as usize; - let end = ptr as usize + len; - let range = start..end; - (range, stack_maps) - })); + self.inner.modules.borrow_mut().register(module); } // This is used to register a `Func` with the store @@ -448,10 +427,6 @@ impl Store { } #[inline] - pub(crate) fn stack_map_registry(&self) -> &StackMapRegistry { - &self.inner.stack_map_registry - } - pub(crate) fn modules(&self) -> &RefCell { &self.inner.modules } @@ -471,20 +446,21 @@ impl Store { /// /// It is fine to call this several times: only the first call will have an effect. pub unsafe fn notify_switched_thread(&self) { - wasmtime_runtime::init_traps(frame_info::GlobalFrameInfo::is_wasm_pc) + wasmtime_runtime::init_traps(crate::module::GlobalModuleRegistry::is_wasm_pc) .expect("failed to initialize per-threads traps"); } + #[inline] + pub(crate) fn stack_map_lookup(&self) -> *const dyn wasmtime_runtime::StackMapLookup { + self.inner.as_ref() + } + /// Perform garbage collection of `ExternRef`s. pub fn gc(&self) { // For this crate's API, we ensure that `set_stack_canary` invariants - // are upheld for all host-->Wasm calls, and we register every module - // used with this store in `self.inner.stack_map_registry`. + // are upheld for all host-->Wasm calls. unsafe { - wasmtime_runtime::gc( - &self.inner.stack_map_registry, - &self.inner.externref_activations_table, - ); + wasmtime_runtime::gc(self.inner.as_ref(), &self.inner.externref_activations_table); } } @@ -951,6 +927,13 @@ impl Drop for StoreInner { } } +impl wasmtime_runtime::StackMapLookup for StoreInner { + fn lookup(&self, pc: usize) -> Option<*const wasmtime_environ::ir::StackMap> { + // The address of the stack map is stable for the lifetime of the store + self.modules.borrow().lookup_stack_map(pc).map(|m| m as _) + } +} + /// A threadsafe handle used to interrupt instances executing within a /// particular `Store`. /// diff --git a/crates/wasmtime/src/trampoline.rs b/crates/wasmtime/src/trampoline.rs index 7cb588f7d4..21eac2a134 100644 --- a/crates/wasmtime/src/trampoline.rs +++ b/crates/wasmtime/src/trampoline.rs @@ -19,8 +19,8 @@ use std::sync::Arc; use wasmtime_environ::{entity::PrimaryMap, wasm, Module}; use wasmtime_runtime::{ Imports, InstanceAllocationRequest, InstanceAllocator, InstanceHandle, - OnDemandInstanceAllocator, StackMapRegistry, VMExternRefActivationsTable, VMFunctionBody, - VMFunctionImport, VMSharedSignatureIndex, + OnDemandInstanceAllocator, VMExternRefActivationsTable, VMFunctionBody, VMFunctionImport, + VMSharedSignatureIndex, }; /// A wrapper around `wasmtime_runtime::InstanceHandle` which pairs it with the @@ -77,7 +77,7 @@ fn create_handle( externref_activations_table: store.externref_activations_table() as *const VMExternRefActivationsTable as *mut _, - stack_map_registry: store.stack_map_registry() as *const StackMapRegistry as *mut _, + stack_map_lookup: Some(store.stack_map_lookup()), }, )?; diff --git a/crates/wasmtime/src/trampoline/create_handle.rs b/crates/wasmtime/src/trampoline/create_handle.rs index dde937f81c..182e421499 100644 --- a/crates/wasmtime/src/trampoline/create_handle.rs +++ b/crates/wasmtime/src/trampoline/create_handle.rs @@ -9,7 +9,7 @@ use wasmtime_environ::entity::PrimaryMap; use wasmtime_environ::wasm::DefinedFuncIndex; use wasmtime_environ::Module; use wasmtime_runtime::{ - Imports, InstanceAllocationRequest, InstanceAllocator, StackMapRegistry, + Imports, InstanceAllocationRequest, InstanceAllocator, StackMapLookup, VMExternRefActivationsTable, VMFunctionBody, VMFunctionImport, VMSharedSignatureIndex, }; @@ -43,7 +43,7 @@ pub(crate) fn create_handle( externref_activations_table: store.externref_activations_table() as *const VMExternRefActivationsTable as *mut _, - stack_map_registry: store.stack_map_registry() as *const StackMapRegistry as *mut _, + stack_map_lookup: &store, })?; Ok(store.add_instance(handle, true)) diff --git a/crates/wasmtime/src/trampoline/func.rs b/crates/wasmtime/src/trampoline/func.rs index f2402efa6c..0265c0b3c1 100644 --- a/crates/wasmtime/src/trampoline/func.rs +++ b/crates/wasmtime/src/trampoline/func.rs @@ -283,7 +283,7 @@ pub fn create_function( host_state: Box::new(trampoline_state), interrupts: std::ptr::null(), externref_activations_table: std::ptr::null_mut(), - stack_map_registry: std::ptr::null_mut(), + stack_map_lookup: None, })?, trampoline, )) @@ -315,7 +315,7 @@ pub unsafe fn create_raw_function( host_state, interrupts: std::ptr::null(), externref_activations_table: std::ptr::null_mut(), - stack_map_registry: std::ptr::null_mut(), + stack_map_lookup: None, })?, ) } diff --git a/crates/wasmtime/src/values.rs b/crates/wasmtime/src/values.rs index 4dc6474513..9e10f0aa83 100644 --- a/crates/wasmtime/src/values.rs +++ b/crates/wasmtime/src/values.rs @@ -98,7 +98,7 @@ impl Val { let externref_ptr = x.inner.as_raw(); store .externref_activations_table() - .insert_with_gc(x.inner, store.stack_map_registry()); + .insert_with_gc(x.inner, &*store.stack_map_lookup()); ptr::write(p as *mut *mut u8, externref_ptr) } Val::FuncRef(f) => ptr::write( From 510fc717281327971f35b40879567f1dc9918e9c Mon Sep 17 00:00:00 2001 From: Peter Huene Date: Thu, 15 Apr 2021 20:15:24 -0700 Subject: [PATCH 3/8] Code review feedback. * Make `FunctionInfo` public and `CompiledModule::func_info` return it. * Make the `StackMapLookup` trait unsafe. * Add comments for the purpose of `EngineHostFuncs`. * Rework ownership model of shared signatures: `SignatureCollection` in conjunction with `SignatureRegistry` is now used so that the `Engine`, `Store`, and `Module` don't need to worry about unregistering shared signatures. * Implement `Func::param_arity` and `Func::result_arity` in terms of `Func::ty`. * Make looking up a trampoline with the module registry more efficient by doing a binary search on the function's starting PC value for the owning module and then looking up the trampoline with only that module. * Remove reference to the shared signatures from `GlobalRegisteredModule`. --- crates/jit/src/instantiate.rs | 20 +- crates/runtime/src/externref.rs | 12 +- crates/wasmtime/src/engine.rs | 94 +++---- crates/wasmtime/src/func.rs | 23 +- crates/wasmtime/src/instance.rs | 17 +- crates/wasmtime/src/module.rs | 44 +--- crates/wasmtime/src/module/registry.rs | 118 +++++---- crates/wasmtime/src/module/serialization.rs | 17 +- crates/wasmtime/src/signatures.rs | 257 +++++++++++++------- crates/wasmtime/src/store.rs | 56 ++--- crates/wasmtime/src/trampoline.rs | 2 +- crates/wasmtime/src/trampoline/func.rs | 6 +- crates/wasmtime/src/types/matching.rs | 14 +- 13 files changed, 336 insertions(+), 344 deletions(-) diff --git a/crates/jit/src/instantiate.rs b/crates/jit/src/instantiate.rs index 5527051ecd..3ff67d9c7a 100644 --- a/crates/jit/src/instantiate.rs +++ b/crates/jit/src/instantiate.rs @@ -176,11 +176,13 @@ struct FinishedFunctions(PrimaryMap); unsafe impl Send for FinishedFunctions {} unsafe impl Sync for FinishedFunctions {} +/// Information about a function, such as trap information, address map, +/// and stack maps. #[derive(Serialize, Deserialize, Clone)] -struct FunctionInfo { - traps: Vec, - address_map: FunctionAddressMap, - stack_maps: Vec, +pub struct FunctionInfo { + pub traps: Vec, + pub address_map: FunctionAddressMap, + pub stack_maps: Vec, } /// This is intended to mirror the type tables in `wasmtime_environ`, except that @@ -362,18 +364,10 @@ impl CompiledModule { } /// Gets the function information for a given function index. - pub fn func_info( - &self, - index: DefinedFuncIndex, - ) -> ( - &FunctionAddressMap, - &[TrapInformation], - &[StackMapInformation], - ) { + pub fn func_info(&self, index: DefinedFuncIndex) -> &FunctionInfo { self.artifacts .funcs .get(index) - .map(|f| (&f.address_map, f.traps.as_ref(), f.stack_maps.as_ref())) .expect("defined function should be present") } diff --git a/crates/runtime/src/externref.rs b/crates/runtime/src/externref.rs index 0606a941e6..44a60ce797 100644 --- a/crates/runtime/src/externref.rs +++ b/crates/runtime/src/externref.rs @@ -742,14 +742,22 @@ impl VMExternRefActivationsTable { } /// Used by the runtime to lookup a stack map from a PC value. -pub trait StackMapLookup { +/// +/// # Safety +/// +/// This trait is unsafe as it returns pointers to a stack map without +/// any clear ownership. +/// +/// It is the responsibility of the caller to not have the pointer outlive +/// the stack map lookup trait object. +pub unsafe trait StackMapLookup { /// Lookup the stack map at a program counter (PC) value. fn lookup(&self, pc: usize) -> Option<*const StackMap>; } pub(crate) struct EmptyStackMapLookup; -impl StackMapLookup for EmptyStackMapLookup { +unsafe impl StackMapLookup for EmptyStackMapLookup { fn lookup(&self, _pc: usize) -> Option<*const StackMap> { None } diff --git a/crates/wasmtime/src/engine.rs b/crates/wasmtime/src/engine.rs index 54d8769d19..acafaa5797 100644 --- a/crates/wasmtime/src/engine.rs +++ b/crates/wasmtime/src/engine.rs @@ -1,24 +1,34 @@ -use crate::signatures::{SharedSignatures, SignatureRegistry, TrampolineMap}; +use crate::signatures::{SignatureCollection, SignatureRegistry}; use crate::Config; use anyhow::Result; use std::collections::HashMap; -use std::sync::{Arc, RwLock}; +use std::sync::Arc; #[cfg(feature = "cache")] use wasmtime_cache::CacheConfig; -use wasmtime_environ::{ - entity::PrimaryMap, - wasm::{SignatureIndex, WasmFuncType}, -}; use wasmtime_jit::Compiler; -use wasmtime_runtime::{ - debug_builtins, InstanceAllocator, InstanceHandle, VMCallerCheckedAnyfunc, - VMSharedSignatureIndex, VMTrampoline, -}; +use wasmtime_runtime::{debug_builtins, InstanceAllocator, InstanceHandle, VMCallerCheckedAnyfunc}; -#[derive(Default)] +/// This is used as a Send+Sync wrapper around two data structures relating to +/// host functions defined on `Config`: +/// +/// * `anyfuncs` - this stores a mapping between the host function instance and +/// a `VMCallerCheckedAnyfunc` that can be used as the function's value in Wasmtime's ABI. +/// The address of the anyfunc needs to be stable, thus the boxed value. +/// +/// * `signatures` - this stores the collection of shared signatures registered for every +/// usable host functions with this engine. struct EngineHostFuncs { anyfuncs: HashMap>, - trampolines: TrampolineMap, + signatures: SignatureCollection, +} + +impl EngineHostFuncs { + fn new(registry: &SignatureRegistry) -> Self { + Self { + anyfuncs: HashMap::new(), + signatures: SignatureCollection::new(registry), + } + } } // This is safe for send and sync as it is read-only once the @@ -58,18 +68,10 @@ struct EngineInner { config: Config, compiler: Compiler, allocator: Box, - signatures: RwLock, + signatures: SignatureRegistry, host_funcs: EngineHostFuncs, } -impl Drop for EngineInner { - fn drop(&mut self) { - let mut signatures = self.signatures.write().unwrap(); - signatures.unregister(self.host_funcs.trampolines.indexes()); - assert!(signatures.is_empty()); - } -} - impl Engine { /// Creates a new [`Engine`] with the specified compilation and /// configuration settings. @@ -77,20 +79,20 @@ impl Engine { debug_builtins::ensure_exported(); config.validate()?; let allocator = config.build_allocator()?; - let mut signatures = SignatureRegistry::default(); - let mut host_funcs = EngineHostFuncs::default(); + let registry = SignatureRegistry::new(); + let mut host_funcs = EngineHostFuncs::new(®istry); - // Register all the host function signatures + // Register all the host function signatures with the collection for func in config.host_funcs() { - let sig = signatures.register(func.ty.as_wasm_func_type()); + let sig = host_funcs + .signatures + .register(func.ty.as_wasm_func_type(), func.trampoline); // Cloning the instance handle is safe as host functions outlive the engine host_funcs.anyfuncs.insert( unsafe { func.instance.clone() }, Box::new(func.anyfunc(sig)), ); - - host_funcs.trampolines.insert(sig, func.trampoline); } Ok(Engine { @@ -98,7 +100,7 @@ impl Engine { config: config.clone(), compiler: config.build_compiler(allocator.as_ref()), allocator, - signatures: RwLock::new(signatures), + signatures: registry, host_funcs, }), }) @@ -128,40 +130,12 @@ impl Engine { Arc::ptr_eq(&a.inner, &b.inner) } - pub(crate) fn register_module_signatures( - &self, - signatures: &PrimaryMap, - trampolines: impl Iterator, - ) -> (SharedSignatures, TrampolineMap) { - self.inner - .signatures - .write() - .unwrap() - .register_module(signatures, trampolines) + pub(crate) fn signatures(&self) -> &SignatureRegistry { + &self.inner.signatures } - pub(crate) fn register_signature(&self, ty: &WasmFuncType) -> VMSharedSignatureIndex { - self.inner.signatures.write().unwrap().register(ty) - } - - pub(crate) fn unregister_signatures( - &self, - indexes: impl Iterator, - ) { - self.inner.signatures.write().unwrap().unregister(indexes); - } - - pub(crate) fn lookup_func_type(&self, index: VMSharedSignatureIndex) -> Option { - self.inner - .signatures - .read() - .unwrap() - .lookup_type(index) - .cloned() - } - - pub(crate) fn host_func_trampolines(&self) -> &TrampolineMap { - &self.inner.host_funcs.trampolines + pub(crate) fn host_func_signatures(&self) -> &SignatureCollection { + &self.inner.host_funcs.signatures } pub(crate) fn host_func_anyfunc( diff --git a/crates/wasmtime/src/func.rs b/crates/wasmtime/src/func.rs index 6bb24597e1..0319ee0002 100644 --- a/crates/wasmtime/src/func.rs +++ b/crates/wasmtime/src/func.rs @@ -778,31 +778,20 @@ impl Func { self.instance .store .engine() - .lookup_func_type(self.sig_index()) + .signatures() + .lookup_type(self.sig_index()) .expect("signature should be registered"), ) } /// Returns the number of parameters that this function takes. pub fn param_arity(&self) -> usize { - let sig = self - .instance - .store - .engine() - .lookup_func_type(self.sig_index()) - .expect("signature should be registered"); - sig.params.len() + self.ty().params().len() } /// Returns the number of results this function produces. pub fn result_arity(&self) -> usize { - let sig = self - .instance - .store - .engine() - .lookup_func_type(self.sig_index()) - .expect("signature should be registered"); - sig.returns.len() + self.ty().results().len() } /// Invokes this function with the `params` given, returning the results and @@ -927,7 +916,7 @@ impl Func { Func { instance: store.existing_vmctx(anyfunc.vmctx), export: export.clone(), - trampoline: store.lookup_trampoline(anyfunc.type_index), + trampoline: store.lookup_trampoline(&*anyfunc), } } @@ -1813,7 +1802,7 @@ macro_rules! impl_into_func { // If not given a store, use a default signature index that is guaranteed to trap. // If the function is called indirectly without first being associated with a store (a bug condition). let shared_signature_id = store - .map(|s| s.register_signature(ty.as_wasm_func_type(), trampoline)) + .map(|s| s.signatures().borrow_mut().register(ty.as_wasm_func_type(), trampoline)) .unwrap_or(VMSharedSignatureIndex::default()); let instance = unsafe { diff --git a/crates/wasmtime/src/instance.rs b/crates/wasmtime/src/instance.rs index a30ef98414..cf807aee6a 100644 --- a/crates/wasmtime/src/instance.rs +++ b/crates/wasmtime/src/instance.rs @@ -13,9 +13,9 @@ use wasmtime_environ::wasm::{ }; use wasmtime_environ::Initializer; use wasmtime_runtime::{ - Imports, InstanceAllocationRequest, InstantiationError, RuntimeInstance, StackMapLookup, - VMContext, VMExternRefActivationsTable, VMFunctionBody, VMFunctionImport, VMGlobalImport, - VMMemoryImport, VMTableImport, + Imports, InstanceAllocationRequest, InstantiationError, RuntimeInstance, VMContext, + VMExternRefActivationsTable, VMFunctionBody, VMFunctionImport, VMGlobalImport, VMMemoryImport, + VMTableImport, }; /// An instantiated WebAssembly module. @@ -506,10 +506,9 @@ impl<'a> Instantiator<'a> { fn instantiate_raw(&self) -> Result { let compiled_module = self.cur.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(&self.cur.module); + // Register the module just before instantiation to ensure we keep the module + // properly referenced while in use by the store. + self.store.modules().borrow_mut().register(&self.cur.module); unsafe { let engine = self.store.engine(); @@ -519,13 +518,13 @@ impl<'a> Instantiator<'a> { module: compiled_module.module().clone(), finished_functions: compiled_module.finished_functions(), imports: self.cur.build(), - shared_signatures: self.cur.module.signatures().into(), + shared_signatures: self.cur.module.signatures().as_module_map().into(), host_state: Box::new(()), interrupts: self.store.interrupts(), externref_activations_table: self.store.externref_activations_table() as *const VMExternRefActivationsTable as *mut _, - stack_map_lookup: Some(self.store.stack_map_lookup() as *const dyn StackMapLookup), + stack_map_lookup: Some(std::mem::transmute(self.store.stack_map_lookup())), })?; // After we've created the `InstanceHandle` we still need to run diff --git a/crates/wasmtime/src/module.rs b/crates/wasmtime/src/module.rs index 7e1db3c876..af108d2bac 100644 --- a/crates/wasmtime/src/module.rs +++ b/crates/wasmtime/src/module.rs @@ -1,5 +1,5 @@ use crate::{ - signatures::{SharedSignatures, TrampolineMap}, + signatures::SignatureCollection, types::{ExportType, ExternType, ImportType}, }; use crate::{Engine, ModuleType}; @@ -11,9 +11,8 @@ use wasmparser::Validator; #[cfg(feature = "cache")] use wasmtime_cache::ModuleCacheEntry; use wasmtime_environ::entity::PrimaryMap; -use wasmtime_environ::wasm::{ModuleIndex, SignatureIndex}; +use wasmtime_environ::wasm::ModuleIndex; use wasmtime_jit::{CompilationArtifacts, CompiledModule, TypeTables}; -use wasmtime_runtime::VMSharedSignatureIndex; mod registry; mod serialization; @@ -21,26 +20,6 @@ mod serialization; pub use registry::{FrameInfo, FrameSymbol, GlobalModuleRegistry, ModuleRegistry}; pub use serialization::SerializedModule; -// A wrapper around registered signatures and trampolines that will automatically -/// unregister the signatures when dropped. -pub(crate) struct ModuleSharedSignatures { - engine: Engine, - signatures: SharedSignatures, - trampolines: TrampolineMap, -} - -impl Drop for ModuleSharedSignatures { - fn drop(&mut self) { - if !self.signatures.is_empty() { - // Use the shared signatures map to unregister as not every registered - // signature will have a trampoline, but every index in the trampoline map - // will be present in the shared signatures map. - self.engine - .unregister_signatures(self.signatures.values().cloned()); - } - } -} - /// A compiled WebAssembly module, ready to be instantiated. /// /// A `Module` is a compiled in-memory representation of an input WebAssembly @@ -129,7 +108,7 @@ struct ModuleInner { /// modules. types: Arc, /// Registered shared signature for the module. - signatures: Arc, + signatures: Arc, } impl Module { @@ -350,10 +329,11 @@ impl Module { // Validate the module can be used with the current allocator engine.allocator().validate(modules[main_module].module())?; - let (signatures, trampolines) = engine.register_module_signatures( + let signatures = Arc::new(SignatureCollection::new_for_module( + engine.signatures(), &types.wasm_signatures, modules.iter().flat_map(|m| m.trampolines().iter().cloned()), - ); + )); let module = modules.remove(main_module); @@ -364,11 +344,7 @@ impl Module { types: Arc::new(types), artifact_upvars: modules, module_upvars: Vec::new(), - signatures: Arc::new(ModuleSharedSignatures { - engine: engine.clone(), - signatures, - trampolines, - }), + signatures, }), }) } @@ -488,11 +464,7 @@ impl Module { &self.inner.types } - pub(crate) fn signatures(&self) -> &PrimaryMap { - &self.inner.signatures.signatures - } - - pub(crate) fn shared_signatures(&self) -> &Arc { + pub(crate) fn signatures(&self) -> &Arc { &self.inner.signatures } diff --git a/crates/wasmtime/src/module/registry.rs b/crates/wasmtime/src/module/registry.rs index bec0434a91..3691c38834 100644 --- a/crates/wasmtime/src/module/registry.rs +++ b/crates/wasmtime/src/module/registry.rs @@ -1,6 +1,6 @@ //! Implements a registry of modules for a store. -use crate::{module::ModuleSharedSignatures, Module}; +use crate::{signatures::SignatureCollection, Module}; use std::{ collections::BTreeMap, sync::{Arc, Mutex}, @@ -9,12 +9,17 @@ use wasmtime_environ::{ entity::EntityRef, ir, wasm::DefinedFuncIndex, FunctionAddressMap, TrapInformation, }; use wasmtime_jit::CompiledModule; -use wasmtime_runtime::{VMSharedSignatureIndex, VMTrampoline}; +use wasmtime_runtime::{VMCallerCheckedAnyfunc, VMTrampoline}; lazy_static::lazy_static! { static ref GLOBAL_MODULES: Mutex = Default::default(); } +fn func_by_pc(module: &CompiledModule, pc: usize) -> Option<(DefinedFuncIndex, u32)> { + let (index, start, _) = module.func_by_pc(pc)?; + Some((index, (pc - start) as u32)) +} + /// Used for registering modules with a store. /// /// The map is from the ending (exclusive) address for the module code to @@ -62,7 +67,7 @@ impl ModuleRegistry { let compiled_module = module.compiled_module(); let (start, end) = compiled_module.code().range(); - // Ignore modules with no code, finished functions, or if the module is already registered + // Ignore modules with no code or finished functions if start == end || compiled_module.finished_functions().is_empty() { return; } @@ -71,7 +76,10 @@ impl ModuleRegistry { // may be a valid PC value let end = end - 1; - if self.0.get(&end).is_some() { + // Ensure the module isn't already present in the registry + // This is expected when a module is instantiated multiple times in the same store + if let Some(m) = self.0.get(&end) { + assert_eq!(m.start, start); return; } @@ -89,7 +97,7 @@ impl ModuleRegistry { RegisteredModule { start, module: compiled_module.clone(), - signatures: module.shared_signatures().clone(), + signatures: module.signatures().clone(), }, ); assert!(prev.is_none()); @@ -97,18 +105,10 @@ impl ModuleRegistry { GLOBAL_MODULES.lock().unwrap().register(start, end, module); } - /// Looks up a trampoline from a shared signature index. - /// - /// This will search all modules associated with the store for a suitable trampoline - /// given the shared signature index. - pub fn lookup_trampoline(&self, index: VMSharedSignatureIndex) -> Option { - for (_, m) in &self.0 { - if let Some(trampoline) = m.signatures.trampolines.get(index) { - return Some(trampoline); - } - } - - None + /// Looks up a trampoline from an anyfunc. + pub fn lookup_trampoline(&self, anyfunc: &VMCallerCheckedAnyfunc) -> Option { + let module = self.module(anyfunc.func_ptr.as_ptr() as usize)?; + module.signatures.trampoline(anyfunc.type_index) } } @@ -124,7 +124,7 @@ impl Drop for ModuleRegistry { struct RegisteredModule { start: usize, module: Arc, - signatures: Arc, + signatures: Arc, } impl RegisteredModule { @@ -138,9 +138,9 @@ impl RegisteredModule { /// Returns an object if this `pc` is known to this module, or returns `None` /// if no information can be found. pub fn lookup_frame_info(&self, pc: usize) -> Option { - let (index, offset) = self.func(pc)?; - let (addr_map, _, _) = self.module.func_info(index); - let pos = Self::instr_pos(offset, addr_map); + let (index, offset) = func_by_pc(&self.module, pc)?; + let info = self.module.func_info(index); + let pos = Self::instr_pos(offset, &info.address_map); // In debug mode for now assert that we found a mapping for `pc` within // the function, because otherwise something is buggy along the way and @@ -149,8 +149,8 @@ impl RegisteredModule { debug_assert!(pos.is_some(), "failed to find instruction for {:x}", pc); let instr = match pos { - Some(pos) => addr_map.instructions[pos].srcloc, - None => addr_map.start_srcloc, + Some(pos) => info.address_map.instructions[pos].srcloc, + None => info.address_map.start_srcloc, }; // Use our wasm-relative pc to symbolize this frame. If there's a @@ -193,25 +193,26 @@ impl RegisteredModule { func_index: index.index() as u32, func_name: module.func_names.get(&index).cloned(), instr, - func_start: addr_map.start_srcloc, + func_start: info.address_map.start_srcloc, symbols, }) } /// Fetches trap information about a program counter in a backtrace. pub fn lookup_trap_info(&self, pc: usize) -> Option<&TrapInformation> { - let (index, offset) = self.func(pc)?; - let (_, traps, _) = self.module.func_info(index); - let idx = traps + let (index, offset) = func_by_pc(&self.module, pc)?; + let info = self.module.func_info(index); + let idx = info + .traps .binary_search_by_key(&offset, |info| info.code_offset) .ok()?; - Some(&traps[idx]) + Some(&info.traps[idx]) } /// Looks up a stack map from a program counter pub fn lookup_stack_map(&self, pc: usize) -> Option<&ir::StackMap> { - let (index, offset) = self.func(pc)?; - let (_, _, stack_maps) = self.module.func_info(index); + let (index, offset) = func_by_pc(&self.module, pc)?; + let info = self.module.func_info(index); // Do a binary search to find the stack map for the given offset. // @@ -255,7 +256,10 @@ impl RegisteredModule { // or reload from the stack slots (which would have been updated to // point to the moved objects). - let index = match stack_maps.binary_search_by_key(&offset, |i| i.code_offset) { + let index = match info + .stack_maps + .binary_search_by_key(&offset, |i| i.code_offset) + { // Exact hit. Ok(i) => i, @@ -269,12 +273,7 @@ impl RegisteredModule { Err(i) => i - 1, }; - Some(&stack_maps[index].stack_map) - } - - fn func(&self, pc: usize) -> Option<(DefinedFuncIndex, u32)> { - let (index, start, _) = self.module.func_by_pc(pc)?; - Some((index, (pc - start) as u32)) + Some(&info.stack_maps[index].stack_map) } fn instr_pos(offset: u32, addr_map: &FunctionAddressMap) -> Option { @@ -298,6 +297,17 @@ impl RegisteredModule { } } +// Counterpart to `RegisteredModule`, but stored in the global registry. +struct GlobalRegisteredModule { + start: usize, + module: Arc, + /// Note that modules can be instantiated in many stores, so the purpose of + /// this field is to keep track of how many stores have registered a + /// module. Information is only removed from the global registry when this + /// reference count reaches 0. + references: usize, +} + /// This is the global module registry that stores information for all modules /// that are currently in use by any `Store`. /// @@ -318,18 +328,18 @@ impl GlobalModuleRegistry { /// Returns whether the `pc`, according to globally registered information, /// is a wasm trap or not. pub(crate) fn is_wasm_pc(pc: usize) -> bool { - let info = GLOBAL_MODULES.lock().unwrap(); + let modules = GLOBAL_MODULES.lock().unwrap(); - match info.0.range(pc..).next() { - Some((end, info)) => { - if pc < info.module.start || *end < pc { + match modules.0.range(pc..).next() { + Some((end, entry)) => { + if pc < entry.start || *end < pc { return false; } - match info.module.func(pc) { + match func_by_pc(&entry.module, pc) { Some((index, offset)) => { - let (addr_map, _, _) = info.module.module.func_info(index); - RegisteredModule::instr_pos(offset, addr_map).is_some() + let info = entry.module.func_info(index); + RegisteredModule::instr_pos(offset, &info.address_map).is_some() } None => false, } @@ -342,18 +352,15 @@ impl GlobalModuleRegistry { /// the given function information, with the global information. fn register(&mut self, start: usize, end: usize, module: &Module) { let info = self.0.entry(end).or_insert_with(|| GlobalRegisteredModule { - module: RegisteredModule { - start, - module: module.compiled_module().clone(), - signatures: module.shared_signatures().clone(), - }, + start, + module: module.compiled_module().clone(), references: 0, }); // Note that ideally we'd debug_assert that the information previously // stored, if any, matches the `functions` we were given, but for now we // just do some simple checks to hope it's the same. - assert_eq!(info.module.start, start); + assert_eq!(info.start, start); info.references += 1; } @@ -368,17 +375,6 @@ impl GlobalModuleRegistry { } } -/// This is the equivalent of `RegisteredModule` except it keeps a reference count. -struct GlobalRegisteredModule { - module: RegisteredModule, - - /// Note that modules can be instantiated in many stores, so the purpose of - /// this field is to keep track of how many stores have registered a - /// module. Information is only removed from the global registry when this - /// reference count reaches 0. - references: usize, -} - /// Description of a frame in a backtrace for a [`Trap`]. /// /// Whenever a WebAssembly trap occurs an instance of [`Trap`] is created. Each diff --git a/crates/wasmtime/src/module/serialization.rs b/crates/wasmtime/src/module/serialization.rs index 1d74aaaf43..0d2836889e 100644 --- a/crates/wasmtime/src/module/serialization.rs +++ b/crates/wasmtime/src/module/serialization.rs @@ -1,7 +1,7 @@ //! Implements module serialization. -use super::{ModuleInner, ModuleSharedSignatures}; -use crate::{Engine, Module, OptLevel}; +use super::ModuleInner; +use crate::{signatures::SignatureCollection, Engine, Module, OptLevel}; use anyhow::{anyhow, bail, Context, Result}; use bincode::Options; use serde::{Deserialize, Serialize}; @@ -300,16 +300,11 @@ impl<'a> SerializedModule<'a> { .allocator() .validate(modules.last().unwrap().module())?; - let (signatures, trampolines) = engine.register_module_signatures( + let signatures = Arc::new(SignatureCollection::new_for_module( + engine.signatures(), &types.wasm_signatures, modules.iter().flat_map(|m| m.trampolines().iter().cloned()), - ); - - let signatures = Arc::new(ModuleSharedSignatures { - engine: engine.clone(), - signatures, - trampolines, - }); + )); let module = modules.pop().unwrap(); @@ -347,7 +342,7 @@ impl<'a> SerializedModule<'a> { module_index: usize, artifact_upvars: &[usize], module_upvars: &[SerializedModuleUpvar], - signatures: &Arc, + signatures: &Arc, ) -> Result { Ok(Module { inner: Arc::new(ModuleInner { diff --git a/crates/wasmtime/src/signatures.rs b/crates/wasmtime/src/signatures.rs index bd68f0d8ce..2faf3fa059 100644 --- a/crates/wasmtime/src/signatures.rs +++ b/crates/wasmtime/src/signatures.rs @@ -1,61 +1,108 @@ //! Implement a registry of function signatures, for fast indirect call //! signature checking. -use std::collections::{hash_map::Entry, HashMap}; -use std::convert::TryFrom; +use std::{ + collections::{hash_map::Entry, HashMap}, + sync::RwLock, +}; +use std::{convert::TryFrom, sync::Arc}; use wasmtime_environ::entity::PrimaryMap; use wasmtime_environ::wasm::{SignatureIndex, WasmFuncType}; use wasmtime_runtime::{VMSharedSignatureIndex, VMTrampoline}; -/// Represents a mapping of shared signature index to trampolines. +/// Represents a collection of shared signatures. /// -/// This is used in various places to store trampolines associated with shared -/// signature indexes. +/// This is used to register shared signatures with a shared signature registry. /// -/// As multiple trampolines may exist for a single signature, the map entries -/// are internally reference counted. -#[derive(Default)] -pub struct TrampolineMap(HashMap); +/// The collection will unregister any contained signatures with the registry +/// when dropped. +#[derive(Debug)] +pub struct SignatureCollection { + registry: Arc>, + signatures: PrimaryMap, + trampolines: HashMap, +} -impl TrampolineMap { - /// Inserts a trampoline into the map. - pub fn insert(&mut self, index: VMSharedSignatureIndex, trampoline: VMTrampoline) { - let entry = match self.0.entry(index) { +impl SignatureCollection { + /// Creates a new, empty signature collection given a signature registry. + pub fn new(registry: &SignatureRegistry) -> Self { + Self { + registry: registry.0.clone(), + signatures: PrimaryMap::new(), + trampolines: HashMap::new(), + } + } + + /// Creates a signature collection for a module given the module's signatures + /// and trampolines. + pub fn new_for_module( + registry: &SignatureRegistry, + signatures: &PrimaryMap, + trampolines: impl Iterator, + ) -> Self { + let (signatures, trampolines) = registry + .0 + .write() + .unwrap() + .register_for_module(signatures, trampolines); + + Self { + registry: registry.0.clone(), + signatures, + trampolines, + } + } + + /// Treats the signature collection as a map from a module signature index to + /// registered shared signature indexes. + /// + /// This is used for looking up module shared signature indexes during module + /// instantiation. + pub fn as_module_map(&self) -> &PrimaryMap { + &self.signatures + } + + /// Gets the shared signature index given a module signature index. + pub fn shared_signature(&self, index: SignatureIndex) -> Option { + self.signatures.get(index).copied() + } + + /// Gets a trampoline for a registered signature. + pub fn trampoline(&self, index: VMSharedSignatureIndex) -> Option { + self.trampolines + .get(&index) + .map(|(_, trampoline)| *trampoline) + } + + /// Registers a single function with the collection. + /// + /// Returns the shared signature index for the function. + pub fn register( + &mut self, + ty: &WasmFuncType, + trampoline: VMTrampoline, + ) -> VMSharedSignatureIndex { + let index = self.registry.write().unwrap().register(ty); + + let entry = match self.trampolines.entry(index) { Entry::Occupied(e) => e.into_mut(), Entry::Vacant(e) => e.insert((0, trampoline)), }; // Increment the ref count entry.0 += 1; - } - /// Gets a trampoline from the map. - pub fn get(&self, index: VMSharedSignatureIndex) -> Option { - self.0.get(&index).map(|(_, trampoline)| *trampoline) - } - - /// Iterates the shared signature indexes stored in the map. - /// - /// A shared signature index will be returned by the iterator for every - /// trampoline registered for that index, so duplicates may be present. - /// - /// This iterator can be used for deregistering signatures with the - /// signature registry. - pub fn indexes<'a>(&'a self) -> impl Iterator + 'a { - self.0 - .iter() - .flat_map(|(index, (count, _))| std::iter::repeat(*index).take(*count)) - } - - /// Determines if the trampoline map is empty. - pub fn is_empty(&self) -> bool { - self.0.is_empty() + index } } -/// Represents a map between module signature indexes and -/// shared signature indexes. -pub type SharedSignatures = PrimaryMap; +impl Drop for SignatureCollection { + fn drop(&mut self) { + if !self.signatures.is_empty() || !self.trampolines.is_empty() { + self.registry.write().unwrap().unregister_signatures(self); + } + } +} #[derive(Debug)] struct RegistryEntry { @@ -63,44 +110,37 @@ struct RegistryEntry { ty: WasmFuncType, } -/// WebAssembly requires that the caller and callee signatures in an indirect -/// call must match. To implement this efficiently, keep a registry of all -/// signatures, shared by all instances, so that call sites can just do an -/// index comparison. #[derive(Debug, Default)] -pub struct SignatureRegistry { +struct SignatureRegistryInner { map: HashMap, entries: Vec>, free: Vec, } -impl SignatureRegistry { - /// Registers a module with the signature registry from the collection of - /// all signatures and trampolines in the module. - pub fn register_module( +impl SignatureRegistryInner { + fn register_for_module( &mut self, signatures: &PrimaryMap, trampolines: impl Iterator, - ) -> (SharedSignatures, TrampolineMap) { - let mut sigs = SharedSignatures::default(); - let mut map = TrampolineMap::default(); + ) -> ( + PrimaryMap, + HashMap, + ) { + let mut sigs = PrimaryMap::default(); + let mut map = HashMap::default(); for (_, ty) in signatures.iter() { sigs.push(self.register(ty)); } for (index, trampoline) in trampolines { - let index = self.map[&signatures[index]]; - map.insert(index, trampoline); + map.insert(sigs[index], (1, trampoline)); } (sigs, map) } - /// Registers a single signature with the registry. - /// - /// This is used for registering host functions created with the Wasmtime API. - pub fn register(&mut self, ty: &WasmFuncType) -> VMSharedSignatureIndex { + fn register(&mut self, ty: &WasmFuncType) -> VMSharedSignatureIndex { let len = self.map.len(); let index = match self.map.entry(ty.clone()) { @@ -124,6 +164,10 @@ impl SignatureRegistry { } }; + // The entry should be missing for one just allocated or + // taken from the free list + assert!(entry.is_none()); + *entry = Some(RegistryEntry { references: 0, ty: ty.clone(), @@ -141,45 +185,78 @@ impl SignatureRegistry { index } - /// Unregisters a collection of shared indexes from the registry. - pub fn unregister(&mut self, indexes: impl Iterator) { - for index in indexes { - let removed = { - let entry = self.entries[index.bits() as usize].as_mut().unwrap(); - - debug_assert!(entry.references > 0); - entry.references -= 1; - - if entry.references == 0 { - self.map.remove(&entry.ty); - self.free.push(index); - true - } else { - false - } - }; - - if removed { - self.entries[index.bits() as usize] = None; + fn unregister_signatures(&mut self, collection: &SignatureCollection) { + // If the collection has a populated signatures map, use it to deregister + // This is always 1:1 from entry to registration + if !collection.signatures.is_empty() { + for (_, index) in collection.signatures.iter() { + self.unregister_entry(*index, 1); + } + } else { + // Otherwise, use the trampolines map, which has reference counts related + // to the stored index + for (index, (count, _)) in collection.trampolines.iter() { + self.unregister_entry(*index, *count); } } } - /// Looks up a function type from a shared signature index. - pub fn lookup_type(&self, index: VMSharedSignatureIndex) -> Option<&WasmFuncType> { - self.entries - .get(index.bits() as usize) - .and_then(|e| e.as_ref().map(|e| &e.ty)) - } + fn unregister_entry(&mut self, index: VMSharedSignatureIndex, count: usize) { + let removed = { + let entry = self.entries[index.bits() as usize].as_mut().unwrap(); - /// Determines if the registry is semantically empty. - pub fn is_empty(&self) -> bool { - // If the map is empty, assert that all remaining entries are "free" - if self.map.is_empty() { - assert!(self.free.len() == self.entries.len()); - true - } else { - false + debug_assert!(entry.references >= count); + entry.references -= count; + + if entry.references == 0 { + self.map.remove(&entry.ty); + self.free.push(index); + true + } else { + false + } + }; + + if removed { + self.entries[index.bits() as usize] = None; } } } + +// `SignatureRegistryInner` implements `Drop` in debug builds to assert that +// all signatures have been unregistered for the registry. +#[cfg(debug_assertions)] +impl Drop for SignatureRegistryInner { + fn drop(&mut self) { + assert!( + self.map.is_empty() && self.free.len() == self.entries.len(), + "signature registry not empty" + ); + } +} + +/// Implements a shared signature registry. +/// +/// WebAssembly requires that the caller and callee signatures in an indirect +/// call must match. To implement this efficiently, keep a registry of all +/// signatures, shared by all instances, so that call sites can just do an +/// index comparison. +#[derive(Debug)] +pub struct SignatureRegistry(Arc>); + +impl SignatureRegistry { + /// Creates a new shared signature registry. + pub fn new() -> Self { + Self(Arc::new(RwLock::new(SignatureRegistryInner::default()))) + } + + /// Looks up a function type from a shared signature index. + pub fn lookup_type(&self, index: VMSharedSignatureIndex) -> Option { + self.0 + .read() + .unwrap() + .entries + .get(index.bits() as usize) + .and_then(|e| e.as_ref().map(|e| &e.ty).cloned()) + } +} diff --git a/crates/wasmtime/src/store.rs b/crates/wasmtime/src/store.rs index f337b39975..6b4c65a1fc 100644 --- a/crates/wasmtime/src/store.rs +++ b/crates/wasmtime/src/store.rs @@ -1,6 +1,6 @@ use crate::{ - module::ModuleRegistry, signatures::TrampolineMap, trampoline::StoreInstanceHandle, Engine, - Func, Module, Trap, + module::ModuleRegistry, signatures::SignatureCollection, trampoline::StoreInstanceHandle, + Engine, Func, Module, Trap, }; use anyhow::{bail, Result}; use std::any::{Any, TypeId}; @@ -15,11 +15,10 @@ use std::ptr; use std::rc::Rc; use std::sync::Arc; use std::task::{Context, Poll}; -use wasmtime_environ::wasm::WasmFuncType; use wasmtime_jit::ModuleCode; use wasmtime_runtime::{ InstanceAllocator, InstanceHandle, OnDemandInstanceAllocator, SignalHandler, TrapInfo, - VMContext, VMExternRef, VMExternRefActivationsTable, VMInterrupts, VMSharedSignatureIndex, + VMCallerCheckedAnyfunc, VMContext, VMExternRef, VMExternRefActivationsTable, VMInterrupts, VMTrampoline, }; @@ -76,7 +75,8 @@ pub(crate) struct StoreInner { signal_handler: RefCell>>>, externref_activations_table: VMExternRefActivationsTable, modules: RefCell, - trampolines: RefCell, + // The signatures and trampolines for `Func` objects + signatures: RefCell, // Numbers of resources instantiated in this store. instance_count: Cell, memory_count: Cell, @@ -139,7 +139,7 @@ impl Store { signal_handler: RefCell::new(None), externref_activations_table: VMExternRefActivationsTable::new(), modules: RefCell::new(ModuleRegistry::default()), - trampolines: RefCell::new(TrampolineMap::default()), + signatures: RefCell::new(SignatureCollection::new(engine.signatures())), instance_count: Default::default(), memory_count: Default::default(), table_count: Default::default(), @@ -203,41 +203,31 @@ impl Store { } } - pub(crate) fn register_module(&self, module: &Module) { - // Register the module with the registry - self.inner.modules.borrow_mut().register(module); + pub(crate) fn signatures(&self) -> &RefCell { + &self.inner.signatures } - // This is used to register a `Func` with the store - pub(crate) fn register_signature( - &self, - ty: &WasmFuncType, - trampoline: VMTrampoline, - ) -> VMSharedSignatureIndex { - let index = self.inner.engine.register_signature(ty); - self.inner - .trampolines - .borrow_mut() - .insert(index, trampoline); - index - } - - pub(crate) fn lookup_trampoline(&self, index: VMSharedSignatureIndex) -> VMTrampoline { + pub(crate) fn lookup_trampoline(&self, anyfunc: &VMCallerCheckedAnyfunc) -> VMTrampoline { // Look up the trampoline with the store's trampolines (from `Func`). - if let Some(trampoline) = self.inner.trampolines.borrow().get(index) { + if let Some(trampoline) = self + .inner + .signatures + .borrow() + .trampoline(anyfunc.type_index) + { return trampoline; } // Look up the trampoline with the registered modules - if let Some(trampoline) = self.inner.modules.borrow().lookup_trampoline(index) { + if let Some(trampoline) = self.inner.modules.borrow().lookup_trampoline(anyfunc) { return trampoline; } // Lastly, check with the engine (for `HostFunc`) self.inner .engine - .host_func_trampolines() - .get(index) + .host_func_signatures() + .trampoline(anyfunc.type_index) .expect("trampoline missing") } @@ -451,7 +441,7 @@ impl Store { } #[inline] - pub(crate) fn stack_map_lookup(&self) -> *const dyn wasmtime_runtime::StackMapLookup { + pub(crate) fn stack_map_lookup(&self) -> &dyn wasmtime_runtime::StackMapLookup { self.inner.as_ref() } @@ -918,16 +908,10 @@ impl Drop for StoreInner { } } } - - let trampolines = self.trampolines.borrow(); - - if !trampolines.is_empty() { - self.engine.unregister_signatures(trampolines.indexes()); - } } } -impl wasmtime_runtime::StackMapLookup for StoreInner { +unsafe impl wasmtime_runtime::StackMapLookup for StoreInner { fn lookup(&self, pc: usize) -> Option<*const wasmtime_environ::ir::StackMap> { // The address of the stack map is stable for the lifetime of the store self.modules.borrow().lookup_stack_map(pc).map(|m| m as _) diff --git a/crates/wasmtime/src/trampoline.rs b/crates/wasmtime/src/trampoline.rs index 21eac2a134..1590bf444a 100644 --- a/crates/wasmtime/src/trampoline.rs +++ b/crates/wasmtime/src/trampoline.rs @@ -77,7 +77,7 @@ fn create_handle( externref_activations_table: store.externref_activations_table() as *const VMExternRefActivationsTable as *mut _, - stack_map_lookup: Some(store.stack_map_lookup()), + stack_map_lookup: Some(std::mem::transmute(store.stack_map_lookup())), }, )?; diff --git a/crates/wasmtime/src/trampoline/func.rs b/crates/wasmtime/src/trampoline/func.rs index 0265c0b3c1..0f95703268 100644 --- a/crates/wasmtime/src/trampoline/func.rs +++ b/crates/wasmtime/src/trampoline/func.rs @@ -270,7 +270,11 @@ pub fn create_function( // If there is no store, use the default signature index which is // guaranteed to trap if there is ever an indirect call on the function (should not happen) let shared_signature_id = store - .map(|s| s.register_signature(ft.as_wasm_func_type(), trampoline)) + .map(|s| { + s.signatures() + .borrow_mut() + .register(ft.as_wasm_func_type(), trampoline) + }) .unwrap_or(VMSharedSignatureIndex::default()); unsafe { diff --git a/crates/wasmtime/src/types/matching.rs b/crates/wasmtime/src/types/matching.rs index 55a55c1d62..7405194c12 100644 --- a/crates/wasmtime/src/types/matching.rs +++ b/crates/wasmtime/src/types/matching.rs @@ -1,4 +1,4 @@ -use crate::{signatures::SharedSignatures, Extern, Store}; +use crate::{signatures::SignatureCollection, Extern, Store}; use anyhow::{bail, Context, Result}; use wasmtime_environ::wasm::{ EntityType, Global, InstanceTypeIndex, Memory, ModuleTypeIndex, SignatureIndex, Table, @@ -6,7 +6,7 @@ use wasmtime_environ::wasm::{ use wasmtime_jit::TypeTables; pub struct MatchCx<'a> { - pub signatures: &'a SharedSignatures, + pub signatures: &'a SignatureCollection, pub types: &'a TypeTables, pub store: &'a Store, } @@ -71,8 +71,8 @@ impl MatchCx<'_> { } pub fn func(&self, expected: SignatureIndex, actual: &crate::Func) -> Result<()> { - let matches = match self.signatures.get(expected) { - Some(idx) => actual.sig_index() == *idx, + let matches = match self.signatures.shared_signature(expected) { + Some(idx) => actual.sig_index() == idx, // If our expected signature isn't registered, then there's no way // that `actual` can match it. None => false, @@ -133,7 +133,7 @@ impl MatchCx<'_> { fn imports_match<'a>( &self, expected: ModuleTypeIndex, - actual_signatures: &SharedSignatures, + actual_signatures: &SignatureCollection, actual_types: &TypeTables, actual_imports: impl Iterator, ) -> Result<()> { @@ -162,7 +162,7 @@ impl MatchCx<'_> { fn exports_match( &self, expected: InstanceTypeIndex, - actual_signatures: &SharedSignatures, + actual_signatures: &SignatureCollection, actual_types: &TypeTables, lookup: impl Fn(&str) -> Option, ) -> Result<()> { @@ -186,7 +186,7 @@ impl MatchCx<'_> { &self, expected: &EntityType, actual_ty: &EntityType, - actual_signatures: &SharedSignatures, + actual_signatures: &SignatureCollection, actual_types: &TypeTables, ) -> Result<()> { let actual_desc = match actual_ty { From 726a936474b9dcf2e4e8406fdb4a877bf6809812 Mon Sep 17 00:00:00 2001 From: Peter Huene Date: Fri, 16 Apr 2021 10:41:55 -0700 Subject: [PATCH 4/8] Remove ArcModuleCode as it is no longer used. --- crates/wasmtime/src/store.rs | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/crates/wasmtime/src/store.rs b/crates/wasmtime/src/store.rs index 6b4c65a1fc..8103278b1f 100644 --- a/crates/wasmtime/src/store.rs +++ b/crates/wasmtime/src/store.rs @@ -15,7 +15,6 @@ use std::ptr; use std::rc::Rc; use std::sync::Arc; use std::task::{Context, Poll}; -use wasmtime_jit::ModuleCode; use wasmtime_runtime::{ InstanceAllocator, InstanceHandle, OnDemandInstanceAllocator, SignalHandler, TrapInfo, VMCallerCheckedAnyfunc, VMContext, VMExternRef, VMExternRefActivationsTable, VMInterrupts, @@ -946,24 +945,6 @@ impl InterruptHandle { } } -// Wrapper struct to implement hash/equality based on the pointer value of the -// `Arc` in question. -struct ArcModuleCode(Arc); - -impl PartialEq for ArcModuleCode { - fn eq(&self, other: &ArcModuleCode) -> bool { - Arc::ptr_eq(&self.0, &other.0) - } -} - -impl Eq for ArcModuleCode {} - -impl Hash for ArcModuleCode { - fn hash(&self, hasher: &mut H) { - Arc::as_ptr(&self.0).hash(hasher) - } -} - struct Reset<'a, T: Copy>(&'a Cell, T); impl Drop for Reset<'_, T> { From 6ac1321162b7a7035082804118d4d1eccbc3259a Mon Sep 17 00:00:00 2001 From: Peter Huene Date: Fri, 16 Apr 2021 11:06:24 -0700 Subject: [PATCH 5/8] Minor corrections with latest changes. --- crates/runtime/src/externref.rs | 2 +- crates/wasmtime/src/func/typed.rs | 2 +- crates/wasmtime/src/instance.rs | 2 +- crates/wasmtime/src/trampoline.rs | 2 +- crates/wasmtime/src/values.rs | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/crates/runtime/src/externref.rs b/crates/runtime/src/externref.rs index 44a60ce797..0eed6562b5 100644 --- a/crates/runtime/src/externref.rs +++ b/crates/runtime/src/externref.rs @@ -750,7 +750,7 @@ impl VMExternRefActivationsTable { /// /// It is the responsibility of the caller to not have the pointer outlive /// the stack map lookup trait object. -pub unsafe trait StackMapLookup { +pub unsafe trait StackMapLookup: 'static { /// Lookup the stack map at a program counter (PC) value. fn lookup(&self, pc: usize) -> Option<*const StackMap>; } diff --git a/crates/wasmtime/src/func/typed.rs b/crates/wasmtime/src/func/typed.rs index b0fe8b574f..627c2f9f3f 100644 --- a/crates/wasmtime/src/func/typed.rs +++ b/crates/wasmtime/src/func/typed.rs @@ -207,7 +207,7 @@ unsafe impl WasmTy for Option { unsafe { store .externref_activations_table() - .insert_with_gc(x.inner, &*store.stack_map_lookup()); + .insert_with_gc(x.inner, store.stack_map_lookup()); } abi } else { diff --git a/crates/wasmtime/src/instance.rs b/crates/wasmtime/src/instance.rs index cf807aee6a..56c2ec3f30 100644 --- a/crates/wasmtime/src/instance.rs +++ b/crates/wasmtime/src/instance.rs @@ -524,7 +524,7 @@ impl<'a> Instantiator<'a> { externref_activations_table: self.store.externref_activations_table() as *const VMExternRefActivationsTable as *mut _, - stack_map_lookup: Some(std::mem::transmute(self.store.stack_map_lookup())), + stack_map_lookup: Some(self.store.stack_map_lookup()), })?; // After we've created the `InstanceHandle` we still need to run diff --git a/crates/wasmtime/src/trampoline.rs b/crates/wasmtime/src/trampoline.rs index 1590bf444a..21eac2a134 100644 --- a/crates/wasmtime/src/trampoline.rs +++ b/crates/wasmtime/src/trampoline.rs @@ -77,7 +77,7 @@ fn create_handle( externref_activations_table: store.externref_activations_table() as *const VMExternRefActivationsTable as *mut _, - stack_map_lookup: Some(std::mem::transmute(store.stack_map_lookup())), + stack_map_lookup: Some(store.stack_map_lookup()), }, )?; diff --git a/crates/wasmtime/src/values.rs b/crates/wasmtime/src/values.rs index 9e10f0aa83..477f0e36b8 100644 --- a/crates/wasmtime/src/values.rs +++ b/crates/wasmtime/src/values.rs @@ -98,7 +98,7 @@ impl Val { let externref_ptr = x.inner.as_raw(); store .externref_activations_table() - .insert_with_gc(x.inner, &*store.stack_map_lookup()); + .insert_with_gc(x.inner, store.stack_map_lookup()); ptr::write(p as *mut *mut u8, externref_ptr) } Val::FuncRef(f) => ptr::write( From b775b68cfb623f8dbf0448986cc43e8e5458aa36 Mon Sep 17 00:00:00 2001 From: Peter Huene Date: Fri, 16 Apr 2021 12:05:38 -0700 Subject: [PATCH 6/8] Make module information lookup from runtime safe. This commit uses a two-phase lookup of stack map information from modules rather than giving back raw pointers to stack maps. First the runtime looks up information about a module from a pc value, which returns an `Arc` it keeps a reference on while completing the stack map lookup. Second it then queries the module information for the stack map from a pc value, getting a reference to the stack map (which is now safe because of the `Arc` held by the runtime). --- crates/environ/src/vmoffsets.rs | 16 ++-- crates/runtime/src/externref.rs | 86 +++++++++---------- crates/runtime/src/instance.rs | 8 +- crates/runtime/src/instance/allocator.rs | 8 +- .../runtime/src/instance/allocator/pooling.rs | 4 +- .../src/instance/allocator/pooling/uffd.rs | 2 +- crates/runtime/src/libcalls.rs | 8 +- crates/wasmtime/src/func/typed.rs | 2 +- crates/wasmtime/src/instance.rs | 2 +- crates/wasmtime/src/module/registry.rs | 67 ++++++++------- crates/wasmtime/src/store.rs | 15 ++-- crates/wasmtime/src/trampoline.rs | 2 +- .../wasmtime/src/trampoline/create_handle.rs | 2 +- crates/wasmtime/src/trampoline/func.rs | 4 +- crates/wasmtime/src/values.rs | 2 +- 15 files changed, 116 insertions(+), 112 deletions(-) diff --git a/crates/environ/src/vmoffsets.rs b/crates/environ/src/vmoffsets.rs index 20f3b8a298..6fc68f5e28 100644 --- a/crates/environ/src/vmoffsets.rs +++ b/crates/environ/src/vmoffsets.rs @@ -6,7 +6,7 @@ // struct VMContext { // interrupts: *const VMInterrupts, // externref_activations_table: *mut VMExternRefActivationsTable, -// stack_map_lookup: *const dyn StackMapLookup, +// module_info_lookup: *const dyn ModuleInfoLookup, // signature_ids: [VMSharedSignatureIndex; module.num_signature_ids], // imported_functions: [VMFunctionImport; module.num_imported_functions], // imported_tables: [VMTableImport; module.num_imported_tables], @@ -77,7 +77,7 @@ pub struct VMOffsets { // precalculated offsets of various member fields interrupts: u32, externref_activations_table: u32, - stack_map_lookup: u32, + module_info_lookup: u32, signature_ids: u32, imported_functions: u32, imported_tables: u32, @@ -149,7 +149,7 @@ impl From for VMOffsets { num_defined_globals: fields.num_defined_globals, interrupts: 0, externref_activations_table: 0, - stack_map_lookup: 0, + module_info_lookup: 0, signature_ids: 0, imported_functions: 0, imported_tables: 0, @@ -168,12 +168,12 @@ impl From for VMOffsets { .interrupts .checked_add(u32::from(fields.pointer_size)) .unwrap(); - ret.stack_map_lookup = ret + ret.module_info_lookup = ret .externref_activations_table .checked_add(u32::from(fields.pointer_size)) .unwrap(); ret.signature_ids = ret - .stack_map_lookup + .module_info_lookup .checked_add(u32::from(fields.pointer_size * 2)) .unwrap(); ret.imported_functions = ret @@ -507,10 +507,10 @@ impl VMOffsets { self.externref_activations_table } - /// The offset of the `*const dyn StackMapLookup` member. + /// The offset of the `*const dyn ModuleInfoLookup` member. #[inline] - pub fn vmctx_stack_map_lookup(&self) -> u32 { - self.stack_map_lookup + pub fn vmctx_module_info_lookup(&self) -> u32 { + self.module_info_lookup } /// The offset of the `signature_ids` array. diff --git a/crates/runtime/src/externref.rs b/crates/runtime/src/externref.rs index 0eed6562b5..4d7b8f9c47 100644 --- a/crates/runtime/src/externref.rs +++ b/crates/runtime/src/externref.rs @@ -99,7 +99,6 @@ //! Examination of Deferred Reference Counting and Cycle Detection* by Quinane: //! -use std::alloc::Layout; use std::any::Any; use std::cell::{Cell, RefCell, UnsafeCell}; use std::cmp::Ordering; @@ -108,6 +107,7 @@ use std::hash::{Hash, Hasher}; use std::mem; use std::ops::Deref; use std::ptr::{self, NonNull}; +use std::{alloc::Layout, sync::Arc}; use wasmtime_environ::ir::StackMap; /// An external reference to some opaque data. @@ -594,10 +594,10 @@ impl VMExternRefActivationsTable { pub unsafe fn insert_with_gc( &self, externref: VMExternRef, - stack_map_lookup: &dyn StackMapLookup, + module_info_lookup: &dyn ModuleInfoLookup, ) { if let Err(externref) = self.try_insert(externref) { - self.gc_and_insert_slow(externref, stack_map_lookup); + self.gc_and_insert_slow(externref, module_info_lookup); } } @@ -605,9 +605,9 @@ impl VMExternRefActivationsTable { unsafe fn gc_and_insert_slow( &self, externref: VMExternRef, - stack_map_lookup: &dyn StackMapLookup, + module_info_lookup: &dyn ModuleInfoLookup, ) { - gc(stack_map_lookup, self); + gc(module_info_lookup, self); // Might as well insert right into the hash set, rather than the bump // chunk, since we are already on a slow path and we get de-duplication @@ -741,29 +741,28 @@ impl VMExternRefActivationsTable { } } -/// Used by the runtime to lookup a stack map from a PC value. -/// -/// # Safety -/// -/// This trait is unsafe as it returns pointers to a stack map without -/// any clear ownership. -/// -/// It is the responsibility of the caller to not have the pointer outlive -/// the stack map lookup trait object. -pub unsafe trait StackMapLookup: 'static { - /// Lookup the stack map at a program counter (PC) value. - fn lookup(&self, pc: usize) -> Option<*const StackMap>; +/// Used by the runtime to lookup information about a module given a +/// program counter value. +pub trait ModuleInfoLookup: 'static { + /// Lookup the module information from a program counter value. + fn lookup(&self, pc: usize) -> Option>; } -pub(crate) struct EmptyStackMapLookup; +/// Used by the runtime to query module information. +pub trait ModuleInfo { + /// Lookup the stack map at a program counter value. + fn lookup_stack_map(&self, pc: usize) -> Option<&StackMap>; +} -unsafe impl StackMapLookup for EmptyStackMapLookup { - fn lookup(&self, _pc: usize) -> Option<*const StackMap> { +pub(crate) struct EmptyModuleInfoLookup; + +impl ModuleInfoLookup for EmptyModuleInfoLookup { + fn lookup(&self, _pc: usize) -> Option> { None } } -pub(crate) const EMPTY_STACK_MAP_LOOKUP: EmptyStackMapLookup = EmptyStackMapLookup; +pub(crate) const EMPTY_MODULE_LOOKUP: EmptyModuleInfoLookup = EmptyModuleInfoLookup; #[derive(Debug, Default)] struct DebugOnly { @@ -810,7 +809,7 @@ impl std::ops::DerefMut for DebugOnly { /// Additionally, you must have registered the stack maps for every Wasm module /// that has frames on the stack with the given `stack_maps_registry`. pub unsafe fn gc( - stack_map_lookup: &dyn StackMapLookup, + module_info_lookup: &dyn ModuleInfoLookup, externref_activations_table: &VMExternRefActivationsTable, ) { // We borrow the precise stack roots `RefCell` for the whole duration of @@ -848,8 +847,7 @@ pub unsafe fn gc( if cfg!(debug_assertions) { // Assert that there aren't any Wasm frames on the stack. backtrace::trace(|frame| { - let stack_map = stack_map_lookup.lookup(frame.ip() as usize); - assert!(stack_map.is_none()); + assert!(module_info_lookup.lookup(frame.ip() as usize).is_none()); true }); } @@ -893,28 +891,30 @@ pub unsafe fn gc( let pc = frame.ip() as usize; let sp = frame.sp() as usize; - if let Some(stack_map) = stack_map_lookup.lookup(pc) { - debug_assert!(sp != 0, "we should always get a valid SP for Wasm frames"); + if let Some(module_info) = module_info_lookup.lookup(pc) { + if let Some(stack_map) = module_info.lookup_stack_map(pc) { + debug_assert!(sp != 0, "we should always get a valid SP for Wasm frames"); - for i in 0..((*stack_map).mapped_words() as usize) { - if (*stack_map).get_bit(i) { - // Stack maps have one bit per word in the frame, and the - // zero^th bit is the *lowest* addressed word in the frame, - // i.e. the closest to the SP. So to get the `i`^th word in - // this frame, we add `i * sizeof(word)` to the SP. - let ptr_to_ref = sp + i * mem::size_of::(); + for i in 0..(stack_map.mapped_words() as usize) { + if stack_map.get_bit(i) { + // Stack maps have one bit per word in the frame, and the + // zero^th bit is the *lowest* addressed word in the frame, + // i.e. the closest to the SP. So to get the `i`^th word in + // this frame, we add `i * sizeof(word)` to the SP. + let ptr_to_ref = sp + i * mem::size_of::(); - let r = std::ptr::read(ptr_to_ref as *const *mut VMExternData); - debug_assert!( - r.is_null() || activations_table_set.contains(&r), - "every on-stack externref inside a Wasm frame should \ - have an entry in the VMExternRefActivationsTable" - ); - if let Some(r) = NonNull::new(r) { - VMExternRefActivationsTable::insert_precise_stack_root( - &mut precise_stack_roots, - r, + let r = std::ptr::read(ptr_to_ref as *const *mut VMExternData); + debug_assert!( + r.is_null() || activations_table_set.contains(&r), + "every on-stack externref inside a Wasm frame should \ + have an entry in the VMExternRefActivationsTable" ); + if let Some(r) = NonNull::new(r) { + VMExternRefActivationsTable::insert_precise_stack_root( + &mut precise_stack_roots, + r, + ); + } } } } diff --git a/crates/runtime/src/instance.rs b/crates/runtime/src/instance.rs index 33a2583cff..7da8d11ee5 100644 --- a/crates/runtime/src/instance.rs +++ b/crates/runtime/src/instance.rs @@ -3,7 +3,7 @@ //! `InstanceHandle` is a reference-counting handle for an `Instance`. use crate::export::Export; -use crate::externref::{StackMapLookup, VMExternRefActivationsTable}; +use crate::externref::{ModuleInfoLookup, VMExternRefActivationsTable}; use crate::memory::{Memory, RuntimeMemoryCreator}; use crate::table::{Table, TableElement}; use crate::traphandlers::Trap; @@ -249,9 +249,9 @@ impl Instance { unsafe { self.vmctx_plus_offset(self.offsets.vmctx_externref_activations_table()) } } - /// Return a pointer to the `StackMapLookup`. - pub fn stack_map_lookup(&self) -> *mut *const dyn StackMapLookup { - unsafe { self.vmctx_plus_offset(self.offsets.vmctx_stack_map_lookup()) } + /// Return a pointer to the `ModuleInfoLookup`. + pub fn module_info_lookup(&self) -> *mut *const dyn ModuleInfoLookup { + unsafe { self.vmctx_plus_offset(self.offsets.vmctx_module_info_lookup()) } } /// Return a reference to the vmctx used by compiled wasm code. diff --git a/crates/runtime/src/instance/allocator.rs b/crates/runtime/src/instance/allocator.rs index 14a9d2eff3..03618a69d1 100644 --- a/crates/runtime/src/instance/allocator.rs +++ b/crates/runtime/src/instance/allocator.rs @@ -1,4 +1,4 @@ -use crate::externref::{StackMapLookup, VMExternRefActivationsTable, EMPTY_STACK_MAP_LOOKUP}; +use crate::externref::{ModuleInfoLookup, VMExternRefActivationsTable, EMPTY_MODULE_LOOKUP}; use crate::imports::Imports; use crate::instance::{Instance, InstanceHandle, RuntimeMemoryCreator}; use crate::memory::{DefaultMemoryCreator, Memory}; @@ -57,8 +57,8 @@ pub struct InstanceAllocationRequest<'a> { /// The pointer to the reference activations table to use for the instance. pub externref_activations_table: *mut VMExternRefActivationsTable, - /// The pointer to the stack map lookup to use for the instance. - pub stack_map_lookup: Option<*const dyn StackMapLookup>, + /// The pointer to the module info lookup to use for the instance. + pub module_info_lookup: Option<*const dyn ModuleInfoLookup>, } /// An link error while instantiating a module. @@ -447,7 +447,7 @@ unsafe fn initialize_vmcontext(instance: &Instance, req: InstanceAllocationReque *instance.interrupts() = req.interrupts; *instance.externref_activations_table() = req.externref_activations_table; - *instance.stack_map_lookup() = req.stack_map_lookup.unwrap_or(&EMPTY_STACK_MAP_LOOKUP); + *instance.module_info_lookup() = req.module_info_lookup.unwrap_or(&EMPTY_MODULE_LOOKUP); // Initialize shared signatures let mut ptr = instance.signature_ids_ptr(); diff --git a/crates/runtime/src/instance/allocator/pooling.rs b/crates/runtime/src/instance/allocator/pooling.rs index 6b89ce603e..5bc342dafc 100644 --- a/crates/runtime/src/instance/allocator/pooling.rs +++ b/crates/runtime/src/instance/allocator/pooling.rs @@ -1370,7 +1370,7 @@ mod test { host_state: Box::new(()), interrupts: std::ptr::null(), externref_activations_table: std::ptr::null_mut(), - stack_map_lookup: None, + module_info_lookup: None, }, ) .expect("allocation should succeed"), @@ -1394,7 +1394,7 @@ mod test { host_state: Box::new(()), interrupts: std::ptr::null(), externref_activations_table: std::ptr::null_mut(), - stack_map_lookup: None, + module_info_lookup: None, }, ) { Err(InstantiationError::Limit(3)) => {} diff --git a/crates/runtime/src/instance/allocator/pooling/uffd.rs b/crates/runtime/src/instance/allocator/pooling/uffd.rs index fb811680bb..4c4b22caba 100644 --- a/crates/runtime/src/instance/allocator/pooling/uffd.rs +++ b/crates/runtime/src/instance/allocator/pooling/uffd.rs @@ -523,7 +523,7 @@ mod test { host_state: Box::new(()), interrupts: ptr::null(), externref_activations_table: ptr::null_mut(), - stack_map_lookup: None, + module_info_lookup: None, }, ) .expect("instance should allocate"), diff --git a/crates/runtime/src/libcalls.rs b/crates/runtime/src/libcalls.rs index 58eb585312..9c5800fd8e 100644 --- a/crates/runtime/src/libcalls.rs +++ b/crates/runtime/src/libcalls.rs @@ -449,8 +449,8 @@ pub unsafe extern "C" fn wasmtime_activations_table_insert_with_gc( let externref = VMExternRef::clone_from_raw(externref); let instance = (&mut *vmctx).instance(); let activations_table = &**instance.externref_activations_table(); - let stack_map_lookup = &**instance.stack_map_lookup(); - activations_table.insert_with_gc(externref, stack_map_lookup); + let module_info_lookup = &**instance.module_info_lookup(); + activations_table.insert_with_gc(externref, module_info_lookup); } /// Perform a Wasm `global.get` for `externref` globals. @@ -466,8 +466,8 @@ pub unsafe extern "C" fn wasmtime_externref_global_get( Some(externref) => { let raw = externref.as_raw(); let activations_table = &**instance.externref_activations_table(); - let stack_map_lookup = &**instance.stack_map_lookup(); - activations_table.insert_with_gc(externref, stack_map_lookup); + let module_info_lookup = &**instance.module_info_lookup(); + activations_table.insert_with_gc(externref, module_info_lookup); raw } } diff --git a/crates/wasmtime/src/func/typed.rs b/crates/wasmtime/src/func/typed.rs index 627c2f9f3f..107810f84b 100644 --- a/crates/wasmtime/src/func/typed.rs +++ b/crates/wasmtime/src/func/typed.rs @@ -207,7 +207,7 @@ unsafe impl WasmTy for Option { unsafe { store .externref_activations_table() - .insert_with_gc(x.inner, store.stack_map_lookup()); + .insert_with_gc(x.inner, store.module_info_lookup()); } abi } else { diff --git a/crates/wasmtime/src/instance.rs b/crates/wasmtime/src/instance.rs index 56c2ec3f30..04a47337ca 100644 --- a/crates/wasmtime/src/instance.rs +++ b/crates/wasmtime/src/instance.rs @@ -524,7 +524,7 @@ impl<'a> Instantiator<'a> { externref_activations_table: self.store.externref_activations_table() as *const VMExternRefActivationsTable as *mut _, - stack_map_lookup: Some(self.store.stack_map_lookup()), + module_info_lookup: Some(self.store.module_info_lookup()), })?; // After we've created the `InstanceHandle` we still need to run diff --git a/crates/wasmtime/src/module/registry.rs b/crates/wasmtime/src/module/registry.rs index 3691c38834..6687119106 100644 --- a/crates/wasmtime/src/module/registry.rs +++ b/crates/wasmtime/src/module/registry.rs @@ -6,10 +6,13 @@ use std::{ sync::{Arc, Mutex}, }; use wasmtime_environ::{ - entity::EntityRef, ir, wasm::DefinedFuncIndex, FunctionAddressMap, TrapInformation, + entity::EntityRef, + ir::{self, StackMap}, + wasm::DefinedFuncIndex, + FunctionAddressMap, TrapInformation, }; use wasmtime_jit::CompiledModule; -use wasmtime_runtime::{VMCallerCheckedAnyfunc, VMTrampoline}; +use wasmtime_runtime::{ModuleInfo, VMCallerCheckedAnyfunc, VMTrampoline}; lazy_static::lazy_static! { static ref GLOBAL_MODULES: Mutex = Default::default(); @@ -27,7 +30,7 @@ fn func_by_pc(module: &CompiledModule, pc: usize) -> Option<(DefinedFuncIndex, u /// /// The `BTreeMap` is used to quickly locate a module based on a program counter value. #[derive(Default)] -pub struct ModuleRegistry(BTreeMap); +pub struct ModuleRegistry(BTreeMap>); impl ModuleRegistry { /// Fetches frame information about a program counter in a backtrace. @@ -48,12 +51,13 @@ impl ModuleRegistry { self.module(pc)?.lookup_trap_info(pc) } - /// Looks up a stack map from a program counter. - pub fn lookup_stack_map<'a>(&'a self, pc: usize) -> Option<&'a ir::StackMap> { - self.module(pc)?.lookup_stack_map(pc) + /// Fetches information about a registered module given a program counter value. + pub fn lookup_module(&self, pc: usize) -> Option> { + self.module(pc) + .map(|m| -> Arc { m.clone() }) } - fn module(&self, pc: usize) -> Option<&RegisteredModule> { + fn module(&self, pc: usize) -> Option<&Arc> { let (end, info) = self.0.range(pc..).next()?; if pc < info.start || *end < pc { return None; @@ -94,11 +98,11 @@ impl ModuleRegistry { let prev = self.0.insert( end, - RegisteredModule { + Arc::new(RegisteredModule { start, module: compiled_module.clone(), signatures: module.signatures().clone(), - }, + }), ); assert!(prev.is_none()); @@ -209,8 +213,29 @@ impl RegisteredModule { Some(&info.traps[idx]) } - /// Looks up a stack map from a program counter - pub fn lookup_stack_map(&self, pc: usize) -> Option<&ir::StackMap> { + fn instr_pos(offset: u32, addr_map: &FunctionAddressMap) -> Option { + // Use our relative position from the start of the function to find the + // machine instruction that corresponds to `pc`, which then allows us to + // map that to a wasm original source location. + match addr_map + .instructions + .binary_search_by_key(&offset, |map| map.code_offset) + { + // Exact hit! + Ok(pos) => Some(pos), + + // This *would* be at the first slot in the array, so no + // instructions cover `pc`. + Err(0) => None, + + // This would be at the `nth` slot, so we're at the `n-1`th slot. + Err(n) => Some(n - 1), + } + } +} + +impl ModuleInfo for RegisteredModule { + fn lookup_stack_map(&self, pc: usize) -> Option<&StackMap> { let (index, offset) = func_by_pc(&self.module, pc)?; let info = self.module.func_info(index); @@ -275,26 +300,6 @@ impl RegisteredModule { Some(&info.stack_maps[index].stack_map) } - - fn instr_pos(offset: u32, addr_map: &FunctionAddressMap) -> Option { - // Use our relative position from the start of the function to find the - // machine instruction that corresponds to `pc`, which then allows us to - // map that to a wasm original source location. - match addr_map - .instructions - .binary_search_by_key(&offset, |map| map.code_offset) - { - // Exact hit! - Ok(pos) => Some(pos), - - // This *would* be at the first slot in the array, so no - // instructions cover `pc`. - Err(0) => None, - - // This would be at the `nth` slot, so we're at the `n-1`th slot. - Err(n) => Some(n - 1), - } - } } // Counterpart to `RegisteredModule`, but stored in the global registry. diff --git a/crates/wasmtime/src/store.rs b/crates/wasmtime/src/store.rs index 8103278b1f..301b1192c1 100644 --- a/crates/wasmtime/src/store.rs +++ b/crates/wasmtime/src/store.rs @@ -16,9 +16,9 @@ use std::rc::Rc; use std::sync::Arc; use std::task::{Context, Poll}; use wasmtime_runtime::{ - InstanceAllocator, InstanceHandle, OnDemandInstanceAllocator, SignalHandler, TrapInfo, - VMCallerCheckedAnyfunc, VMContext, VMExternRef, VMExternRefActivationsTable, VMInterrupts, - VMTrampoline, + InstanceAllocator, InstanceHandle, ModuleInfo, OnDemandInstanceAllocator, SignalHandler, + TrapInfo, VMCallerCheckedAnyfunc, VMContext, VMExternRef, VMExternRefActivationsTable, + VMInterrupts, VMTrampoline, }; /// Used to associate instances with the store. @@ -440,7 +440,7 @@ impl Store { } #[inline] - pub(crate) fn stack_map_lookup(&self) -> &dyn wasmtime_runtime::StackMapLookup { + pub(crate) fn module_info_lookup(&self) -> &dyn wasmtime_runtime::ModuleInfoLookup { self.inner.as_ref() } @@ -910,10 +910,9 @@ impl Drop for StoreInner { } } -unsafe impl wasmtime_runtime::StackMapLookup for StoreInner { - fn lookup(&self, pc: usize) -> Option<*const wasmtime_environ::ir::StackMap> { - // The address of the stack map is stable for the lifetime of the store - self.modules.borrow().lookup_stack_map(pc).map(|m| m as _) +impl wasmtime_runtime::ModuleInfoLookup for StoreInner { + fn lookup(&self, pc: usize) -> Option> { + self.modules.borrow().lookup_module(pc) } } diff --git a/crates/wasmtime/src/trampoline.rs b/crates/wasmtime/src/trampoline.rs index 21eac2a134..ee0bf25c9e 100644 --- a/crates/wasmtime/src/trampoline.rs +++ b/crates/wasmtime/src/trampoline.rs @@ -77,7 +77,7 @@ fn create_handle( externref_activations_table: store.externref_activations_table() as *const VMExternRefActivationsTable as *mut _, - stack_map_lookup: Some(store.stack_map_lookup()), + module_info_lookup: Some(store.module_info_lookup()), }, )?; diff --git a/crates/wasmtime/src/trampoline/create_handle.rs b/crates/wasmtime/src/trampoline/create_handle.rs index 182e421499..d04d2899f9 100644 --- a/crates/wasmtime/src/trampoline/create_handle.rs +++ b/crates/wasmtime/src/trampoline/create_handle.rs @@ -43,7 +43,7 @@ pub(crate) fn create_handle( externref_activations_table: store.externref_activations_table() as *const VMExternRefActivationsTable as *mut _, - stack_map_lookup: &store, + module_info_lookup: &store, })?; Ok(store.add_instance(handle, true)) diff --git a/crates/wasmtime/src/trampoline/func.rs b/crates/wasmtime/src/trampoline/func.rs index 0f95703268..0110e7b2a9 100644 --- a/crates/wasmtime/src/trampoline/func.rs +++ b/crates/wasmtime/src/trampoline/func.rs @@ -287,7 +287,7 @@ pub fn create_function( host_state: Box::new(trampoline_state), interrupts: std::ptr::null(), externref_activations_table: std::ptr::null_mut(), - stack_map_lookup: None, + module_info_lookup: None, })?, trampoline, )) @@ -319,7 +319,7 @@ pub unsafe fn create_raw_function( host_state, interrupts: std::ptr::null(), externref_activations_table: std::ptr::null_mut(), - stack_map_lookup: None, + module_info_lookup: None, })?, ) } diff --git a/crates/wasmtime/src/values.rs b/crates/wasmtime/src/values.rs index 477f0e36b8..7d6adbbedc 100644 --- a/crates/wasmtime/src/values.rs +++ b/crates/wasmtime/src/values.rs @@ -98,7 +98,7 @@ impl Val { let externref_ptr = x.inner.as_raw(); store .externref_activations_table() - .insert_with_gc(x.inner, store.stack_map_lookup()); + .insert_with_gc(x.inner, store.module_info_lookup()); ptr::write(p as *mut *mut u8, externref_ptr) } Val::FuncRef(f) => ptr::write( From dfab471ce5bf8b5cbf29d9b542d143930ea5cf05 Mon Sep 17 00:00:00 2001 From: Peter Huene Date: Fri, 16 Apr 2021 12:26:42 -0700 Subject: [PATCH 7/8] Remove unused file. This file hasn't been used for a while and was mistakenly not deleted. --- .../wasmtime/src/trampoline/create_handle.rs | 51 ------------------- 1 file changed, 51 deletions(-) delete mode 100644 crates/wasmtime/src/trampoline/create_handle.rs diff --git a/crates/wasmtime/src/trampoline/create_handle.rs b/crates/wasmtime/src/trampoline/create_handle.rs deleted file mode 100644 index d04d2899f9..0000000000 --- a/crates/wasmtime/src/trampoline/create_handle.rs +++ /dev/null @@ -1,51 +0,0 @@ -//! Support for a calling of an imported function. - -use crate::trampoline::StoreInstanceHandle; -use crate::Store; -use anyhow::Result; -use std::any::Any; -use std::sync::Arc; -use wasmtime_environ::entity::PrimaryMap; -use wasmtime_environ::wasm::DefinedFuncIndex; -use wasmtime_environ::Module; -use wasmtime_runtime::{ - Imports, InstanceAllocationRequest, InstanceAllocator, StackMapLookup, - VMExternRefActivationsTable, VMFunctionBody, VMFunctionImport, VMSharedSignatureIndex, -}; - -pub(crate) fn create_handle( - module: Module, - store: &Store, - finished_functions: PrimaryMap, - host_state: Box, - func_imports: &[VMFunctionImport], - shared_signature_id: Option, -) -> Result { - let mut imports = Imports::default(); - imports.functions = func_imports; - let module = Arc::new(module); - - unsafe { - // Use the default allocator when creating handles associated with host objects - // The configured instance allocator should only be used when creating module instances - // as we don't want host objects to count towards instance limits. - let handle = store - .engine() - .config() - .default_instance_allocator - .allocate(InstanceAllocationRequest { - module: module.clone(), - finished_functions: &finished_functions, - imports, - shared_signatures: shared_signature_id.into(), - host_state, - interrupts: store.interrupts(), - externref_activations_table: store.externref_activations_table() - as *const VMExternRefActivationsTable - as *mut _, - module_info_lookup: &store, - })?; - - Ok(store.add_instance(handle, true)) - } -} From ef2ad6375d0dd22d1122088c47aea01a86305d1d Mon Sep 17 00:00:00 2001 From: Peter Huene Date: Fri, 16 Apr 2021 12:28:27 -0700 Subject: [PATCH 8/8] Consolidate module construction. This commit adds `Module::from_parts` as an internal constructor that shared the implementation between `Module::from_binary` and module deserialization. --- crates/wasmtime/src/module.rs | 72 +++++++++++++-- crates/wasmtime/src/module/serialization.rs | 97 ++++----------------- 2 files changed, 82 insertions(+), 87 deletions(-) diff --git a/crates/wasmtime/src/module.rs b/crates/wasmtime/src/module.rs index af108d2bac..e9ae9579e3 100644 --- a/crates/wasmtime/src/module.rs +++ b/crates/wasmtime/src/module.rs @@ -320,12 +320,22 @@ impl Module { } }; - let mut modules = CompiledModule::from_artifacts_list( + let modules = CompiledModule::from_artifacts_list( artifacts, engine.compiler().isa(), &*engine.config().profiler, )?; + Self::from_parts(engine, modules, main_module, Arc::new(types), &[]) + } + + fn from_parts( + engine: &Engine, + mut modules: Vec>, + main_module: usize, + types: Arc, + module_upvars: &[serialization::SerializedModuleUpvar], + ) -> Result { // Validate the module can be used with the current allocator engine.allocator().validate(modules[main_module].module())?; @@ -337,16 +347,68 @@ impl Module { let module = modules.remove(main_module); - Ok(Module { + let module_upvars = module_upvars + .iter() + .map(|m| { + mk( + engine, + &modules, + &types, + m.index, + &m.artifact_upvars, + &m.module_upvars, + &signatures, + ) + }) + .collect::>>()?; + + return Ok(Self { inner: Arc::new(ModuleInner { engine: engine.clone(), + types, module, - types: Arc::new(types), artifact_upvars: modules, - module_upvars: Vec::new(), + module_upvars, signatures, }), - }) + }); + + fn mk( + engine: &Engine, + artifacts: &[Arc], + types: &Arc, + module_index: usize, + artifact_upvars: &[usize], + module_upvars: &[serialization::SerializedModuleUpvar], + signatures: &Arc, + ) -> Result { + Ok(Module { + inner: Arc::new(ModuleInner { + engine: engine.clone(), + types: types.clone(), + module: artifacts[module_index].clone(), + artifact_upvars: artifact_upvars + .iter() + .map(|i| artifacts[*i].clone()) + .collect(), + module_upvars: module_upvars + .into_iter() + .map(|m| { + mk( + engine, + artifacts, + types, + m.index, + &m.artifact_upvars, + &m.module_upvars, + signatures, + ) + }) + .collect::>>()?, + signatures: signatures.clone(), + }), + }) + } } /// Validates `binary` input data as a WebAssembly binary given the diff --git a/crates/wasmtime/src/module/serialization.rs b/crates/wasmtime/src/module/serialization.rs index 0d2836889e..8b3620a75d 100644 --- a/crates/wasmtime/src/module/serialization.rs +++ b/crates/wasmtime/src/module/serialization.rs @@ -1,7 +1,6 @@ //! Implements module serialization. -use super::ModuleInner; -use crate::{signatures::SignatureCollection, Engine, Module, OptLevel}; +use crate::{Engine, Module, OptLevel}; use anyhow::{anyhow, bail, Context, Result}; use bincode::Options; use serde::{Deserialize, Serialize}; @@ -124,13 +123,13 @@ impl From for OptLevel { /// A small helper struct for serialized module upvars. #[derive(Serialize, Deserialize)] -struct SerializedModuleUpvar { +pub struct SerializedModuleUpvar { /// The module's index into the compilation artifact. - index: usize, + pub index: usize, /// Indexes into the list of all compilation artifacts for this module. - artifact_upvars: Vec, + pub artifact_upvars: Vec, /// Closed-over module values that are also needed for this module. - module_upvars: Vec, + pub module_upvars: Vec, } impl SerializedModuleUpvar { @@ -285,8 +284,7 @@ impl<'a> SerializedModule<'a> { self.check_tunables(compiler)?; self.check_features(compiler)?; - let types = Arc::new(self.types.unwrap_owned()); - let mut modules = CompiledModule::from_artifacts_list( + let modules = CompiledModule::from_artifacts_list( self.artifacts .into_iter() .map(|i| i.unwrap_owned()) @@ -295,82 +293,17 @@ impl<'a> SerializedModule<'a> { &*engine.config().profiler, )?; - // Validate the module can be used with the current allocator - engine - .allocator() - .validate(modules.last().unwrap().module())?; + assert!(!modules.is_empty()); - let signatures = Arc::new(SignatureCollection::new_for_module( - engine.signatures(), - &types.wasm_signatures, - modules.iter().flat_map(|m| m.trampolines().iter().cloned()), - )); + let main_module = modules.len() - 1; - let module = modules.pop().unwrap(); - - let module_upvars = self - .module_upvars - .iter() - .map(|m| { - mk( - engine, - &modules, - &types, - m.index, - &m.artifact_upvars, - &m.module_upvars, - &signatures, - ) - }) - .collect::>>()?; - - return Ok(Module { - inner: Arc::new(ModuleInner { - engine: engine.clone(), - types, - module, - artifact_upvars: modules, - module_upvars, - signatures, - }), - }); - - fn mk( - engine: &Engine, - artifacts: &[Arc], - types: &Arc, - module_index: usize, - artifact_upvars: &[usize], - module_upvars: &[SerializedModuleUpvar], - signatures: &Arc, - ) -> Result { - Ok(Module { - inner: Arc::new(ModuleInner { - engine: engine.clone(), - types: types.clone(), - module: artifacts[module_index].clone(), - artifact_upvars: artifact_upvars - .iter() - .map(|i| artifacts[*i].clone()) - .collect(), - module_upvars: module_upvars - .into_iter() - .map(|m| { - mk( - engine, - artifacts, - types, - m.index, - &m.artifact_upvars, - &m.module_upvars, - signatures, - ) - }) - .collect::>>()?, - signatures: signatures.clone(), - }), - }) - } + Module::from_parts( + engine, + modules, + main_module, + Arc::new(self.types.unwrap_owned()), + &self.module_upvars, + ) } pub fn to_bytes(&self) -> Result> {