From c3d344e5fb60e551bd809f70c830a4a7c8b89e30 Mon Sep 17 00:00:00 2001 From: Mark McCaskey Date: Wed, 19 May 2021 13:23:59 -0700 Subject: [PATCH 1/6] Fix cyclic ref in WasmerEnv --- lib/api/src/exports.rs | 27 +++++++++++++++++ lib/api/src/externals/memory.rs | 4 +++ lib/derive/src/lib.rs | 8 ++--- lib/vm/src/export.rs | 22 ++++++++++++-- lib/vm/src/instance/mod.rs | 4 +-- lib/vm/src/instance/ref.rs | 53 ++++++++++++++++++++++++++++++++- 6 files changed, 108 insertions(+), 10 deletions(-) diff --git a/lib/api/src/exports.rs b/lib/api/src/exports.rs index 4bfea97ede7..26986529a08 100644 --- a/lib/api/src/exports.rs +++ b/lib/api/src/exports.rs @@ -166,6 +166,19 @@ impl Exports { } } + /// Like `get_with_generics` but with a WeakReference to the `InstanceRef` internally. + /// This is useful for passing data into `WasmerEnv`, for example. + pub fn get_with_generics_weak<'a, T, Args, Rets>(&'a self, name: &str) -> Result + where + Args: WasmTypeList, + Rets: WasmTypeList, + T: ExportableWithGenerics<'a, Args, Rets>, + { + let mut out: T = self.get_with_generics(name)?; + out.into_weak_instance_ref(); + Ok(out) + } + /// Get an export as an `Extern`. pub fn get_extern(&self, name: &str) -> Option<&Extern> { self.map.get(name) @@ -294,6 +307,11 @@ pub trait Exportable<'a>: Sized { /// /// [`Instance`]: crate::Instance fn get_self_from_extern(_extern: &'a Extern) -> Result<&'a Self, ExportError>; + + /// TODO: this method doesn't belong here + fn into_weak_instance_ref(&mut self) { + todo!("into_weak_instance_ref") + } } /// A trait for accessing exports (like [`Exportable`]) but it takes generic @@ -302,6 +320,10 @@ pub trait Exportable<'a>: Sized { pub trait ExportableWithGenerics<'a, Args: WasmTypeList, Rets: WasmTypeList>: Sized { /// Get an export with the given generics. fn get_self_from_extern_with_generics(_extern: &'a Extern) -> Result; + /// TODO: this method doesn't belong here + fn into_weak_instance_ref(&mut self) { + todo!("into_weak_instance_ref") + } } /// We implement it for all concrete [`Exportable`] types (that are `Clone`) @@ -310,4 +332,9 @@ impl<'a, T: Exportable<'a> + Clone + 'static> ExportableWithGenerics<'a, (), ()> fn get_self_from_extern_with_generics(_extern: &'a Extern) -> Result { T::get_self_from_extern(_extern).map(|i| i.clone()) } + + /// TODO: this method doesn't belong here + fn into_weak_instance_ref(&mut self) { + ::into_weak_instance_ref(self); + } } diff --git a/lib/api/src/externals/memory.rs b/lib/api/src/externals/memory.rs index f621751d2f8..fd60999f5be 100644 --- a/lib/api/src/externals/memory.rs +++ b/lib/api/src/externals/memory.rs @@ -261,4 +261,8 @@ impl<'a> Exportable<'a> for Memory { _ => Err(ExportError::IncompatibleType), } } + + fn into_weak_instance_ref(&mut self) { + self.vm_memory.instance_ref.as_mut().map(|v| v.into_weak()); + } } diff --git a/lib/derive/src/lib.rs b/lib/derive/src/lib.rs index 22fcca78291..00c34634951 100644 --- a/lib/derive/src/lib.rs +++ b/lib/derive/src/lib.rs @@ -136,12 +136,12 @@ fn derive_struct_fields(data: &DataStruct) -> (TokenStream, TokenStream) { identifier.unwrap_or_else(|| LitStr::new(&name_str, name.span())); let mut access_expr = quote_spanned! { f.span() => - instance.exports.get_with_generics::<#inner_type, _, _>(#item_name) + instance.exports.get_with_generics_weak::<#inner_type, _, _>(#item_name) }; for alias in aliases { access_expr = quote_spanned! { f.span()=> - #access_expr .or_else(|_| instance.exports.get_with_generics::<#inner_type, _, _>(#alias)) + #access_expr .or_else(|_| instance.exports.get_with_generics_weak::<#inner_type, _, _>(#alias)) }; } if optional { @@ -163,12 +163,12 @@ fn derive_struct_fields(data: &DataStruct) -> (TokenStream, TokenStream) { if let Some(identifier) = identifier { let mut access_expr = quote_spanned! { f.span() => - instance.exports.get_with_generics::<#inner_type, _, _>(#identifier) + instance.exports.get_with_generics_weak::<#inner_type, _, _>(#identifier) }; for alias in aliases { access_expr = quote_spanned! { f.span()=> - #access_expr .or_else(|_| instance.exports.get_with_generics::<#inner_type, _, _>(#alias)) + #access_expr .or_else(|_| instance.exports.get_with_generics_weak::<#inner_type, _, _>(#alias)) }; } let local_var = diff --git a/lib/vm/src/export.rs b/lib/vm/src/export.rs index 075f4703d34..b0cfa087b96 100644 --- a/lib/vm/src/export.rs +++ b/lib/vm/src/export.rs @@ -2,7 +2,7 @@ // Attributions: https://github.com/wasmerio/wasmer/blob/master/ATTRIBUTIONS.md use crate::global::Global; -use crate::instance::InstanceRef; +use crate::instance::{InstanceRef, WeakOrStrongInstanceRef}; use crate::memory::{Memory, MemoryStyle}; use crate::table::{Table, TableStyle}; use crate::vmcontext::{VMFunctionBody, VMFunctionEnvironment, VMFunctionKind, VMTrampoline}; @@ -116,14 +116,30 @@ impl From for VMExtern { } /// A memory export value. -#[derive(Debug, Clone, MemoryUsage)] +#[derive(Debug, MemoryUsage)] pub struct VMMemory { /// Pointer to the containing `Memory`. pub from: Arc, /// A “reference” to the instance through the /// `InstanceRef`. `None` if it is a host memory. - pub instance_ref: Option, + pub instance_ref: Option, +} + +// Is this just a bad idea? We want the default behavior to be converting from weak +// to strong... but is this just a footgun? +impl Clone for VMMemory { + fn clone(&self) -> Self { + // REVIEW: panicking in clone, etc etc. There's probably a much more elegant + // way to express this. + Self { + from: self.from.clone(), + instance_ref: self + .instance_ref + .as_ref() + .map(|v| WeakOrStrongInstanceRef::Strong(v.get_strong().unwrap())), + } + } } /// # Safety diff --git a/lib/vm/src/instance/mod.rs b/lib/vm/src/instance/mod.rs index 423ca991653..102ea9ab523 100644 --- a/lib/vm/src/instance/mod.rs +++ b/lib/vm/src/instance/mod.rs @@ -11,7 +11,7 @@ mod allocator; mod r#ref; pub use allocator::InstanceAllocator; -pub use r#ref::InstanceRef; +pub use r#ref::{InstanceRef, WeakInstanceRef, WeakOrStrongInstanceRef}; use crate::export::VMExtern; use crate::func_data_registry::{FuncDataRegistry, VMFuncRef}; @@ -1126,7 +1126,7 @@ impl InstanceHandle { }; VMMemory { from, - instance_ref: Some(instance), + instance_ref: Some(WeakOrStrongInstanceRef::Strong(instance)), } .into() } diff --git a/lib/vm/src/instance/ref.rs b/lib/vm/src/instance/ref.rs index eb2dbfc5158..f98d2d2c153 100644 --- a/lib/vm/src/instance/ref.rs +++ b/lib/vm/src/instance/ref.rs @@ -3,7 +3,7 @@ use loupe::{MemoryUsage, MemoryUsageTracker}; use std::alloc::Layout; use std::mem; use std::ptr::{self, NonNull}; -use std::sync::Arc; +use std::sync::{Arc, Weak}; /// Dynamic instance allocation. /// @@ -151,3 +151,54 @@ impl InstanceRef { (&mut *ptr).as_mut() } } + +#[derive(Debug, Clone)] +/// TODO: document this +pub struct WeakInstanceRef(Weak); + +impl WeakInstanceRef { + // TODO: document this + pub fn upgrade(&self) -> Option { + let inner = self.0.upgrade()?; + Some(InstanceRef(inner)) + } +} + +impl MemoryUsage for WeakInstanceRef { + fn size_of_val(&self, tracker: &mut dyn MemoryUsageTracker) -> usize { + todo!("Probably missing implementation at crate level for `Weak`. Can be done manually here but I'm focused on other things right now"); + } +} + +#[derive(Debug, Clone, MemoryUsage)] +/// TODO: document this +pub enum WeakOrStrongInstanceRef { + /// A weak instance ref. + Weak(WeakInstanceRef), + /// A strong instance ref. + Strong(InstanceRef), +} + +impl WeakOrStrongInstanceRef { + /// Get a strong `InstanceRef`. A return Value of `None` means that the + /// `InstanceRef` has been freed and cannot be accessed. + pub fn get_strong(&self) -> Option { + match self { + Self::Weak(weak) => weak.upgrade(), + Self::Strong(strong) => Some(strong.clone()), + } + } + /// Get a weak `InstanceRef`. + pub fn get_weak(&self) -> WeakInstanceRef { + match self { + Self::Weak(weak) => weak.clone(), + Self::Strong(strong) => WeakInstanceRef(Arc::downgrade(&strong.0)), + } + } + + /// TODO: document this + pub fn into_weak(&mut self) { + let new = self.get_weak(); + *self = Self::Weak(new); + } +} From dea2abb4fc8d7d77fb54787c6c9e5ef89ce09eb7 Mon Sep 17 00:00:00 2001 From: Mark McCaskey Date: Thu, 20 May 2021 10:38:07 -0700 Subject: [PATCH 2/6] Add tests, implement for all externs --- lib/api/src/exports.rs | 4 +- lib/api/src/externals/function.rs | 19 +++ lib/api/src/externals/global.rs | 11 ++ lib/api/src/externals/memory.rs | 7 + lib/api/src/externals/mod.rs | 9 + lib/api/src/externals/table.rs | 11 ++ lib/api/src/native.rs | 10 ++ lib/api/tests/export.rs | 264 ++++++++++++++++++++++++++++++ lib/vm/src/export.rs | 66 +++++++- lib/vm/src/instance/mod.rs | 6 +- lib/vm/src/instance/ref.rs | 15 +- 11 files changed, 407 insertions(+), 15 deletions(-) create mode 100644 lib/api/tests/export.rs diff --git a/lib/api/src/exports.rs b/lib/api/src/exports.rs index 26986529a08..c4e56c2ce6d 100644 --- a/lib/api/src/exports.rs +++ b/lib/api/src/exports.rs @@ -321,9 +321,7 @@ pub trait ExportableWithGenerics<'a, Args: WasmTypeList, Rets: WasmTypeList>: Si /// Get an export with the given generics. fn get_self_from_extern_with_generics(_extern: &'a Extern) -> Result; /// TODO: this method doesn't belong here - fn into_weak_instance_ref(&mut self) { - todo!("into_weak_instance_ref") - } + fn into_weak_instance_ref(&mut self); } /// We implement it for all concrete [`Exportable`] types (that are `Clone`) diff --git a/lib/api/src/externals/function.rs b/lib/api/src/externals/function.rs index aaf91d78bda..57471623657 100644 --- a/lib/api/src/externals/function.rs +++ b/lib/api/src/externals/function.rs @@ -712,6 +712,17 @@ impl Function { fn closures_unsupported_panic() -> ! { unimplemented!("Closures (functions with captured environments) are currently unsupported with native functions. See: https://github.com/wasmerio/wasmer/issues/1840") } + + /// Check if the function holds a strong `InstanceRef`. + /// None means there's no `InstanceRef`, strong or weak. + // TODO: maybe feature gate this, we only need it for tests... + pub fn is_strong_instance_ref(&self) -> Option { + self.exported + .vm_function + .instance_ref + .as_ref() + .map(|v| v.is_strong()) + } } impl<'a> Exportable<'a> for Function { @@ -725,6 +736,14 @@ impl<'a> Exportable<'a> for Function { _ => Err(ExportError::IncompatibleType), } } + + fn into_weak_instance_ref(&mut self) { + self.exported + .vm_function + .instance_ref + .as_mut() + .map(|v| v.into_weak()); + } } impl fmt::Debug for Function { diff --git a/lib/api/src/externals/global.rs b/lib/api/src/externals/global.rs index 1d03cc10a24..e6b7b031f8c 100644 --- a/lib/api/src/externals/global.rs +++ b/lib/api/src/externals/global.rs @@ -208,6 +208,13 @@ impl Global { pub fn same(&self, other: &Self) -> bool { Arc::ptr_eq(&self.vm_global.from, &other.vm_global.from) } + + /// Check if the global holds a strong `InstanceRef`. + /// None means there's no `InstanceRef`, strong or weak. + // TODO: maybe feature gate this, we only need it for tests... + pub fn is_strong_instance_ref(&self) -> Option { + self.vm_global.instance_ref.as_ref().map(|v| v.is_strong()) + } } impl fmt::Debug for Global { @@ -231,4 +238,8 @@ impl<'a> Exportable<'a> for Global { _ => Err(ExportError::IncompatibleType), } } + + fn into_weak_instance_ref(&mut self) { + self.vm_global.instance_ref.as_mut().map(|v| v.into_weak()); + } } diff --git a/lib/api/src/externals/memory.rs b/lib/api/src/externals/memory.rs index fd60999f5be..2a3077f50f5 100644 --- a/lib/api/src/externals/memory.rs +++ b/lib/api/src/externals/memory.rs @@ -248,6 +248,13 @@ impl Memory { pub fn same(&self, other: &Self) -> bool { Arc::ptr_eq(&self.vm_memory.from, &other.vm_memory.from) } + + /// Check if the memory holds a strong `InstanceRef`. + /// None means there's no `InstanceRef`, strong or weak. + // TODO: maybe feature gate this, we only need it for tests... + pub fn is_strong_instance_ref(&self) -> Option { + self.vm_memory.instance_ref.as_ref().map(|v| v.is_strong()) + } } impl<'a> Exportable<'a> for Memory { diff --git a/lib/api/src/externals/mod.rs b/lib/api/src/externals/mod.rs index 174a175e341..71ac65b5729 100644 --- a/lib/api/src/externals/mod.rs +++ b/lib/api/src/externals/mod.rs @@ -72,6 +72,15 @@ impl<'a> Exportable<'a> for Extern { // Since this is already an extern, we can just return it. Ok(_extern) } + + fn into_weak_instance_ref(&mut self) { + match self { + Self::Function(f) => f.into_weak_instance_ref(), + Self::Global(g) => g.into_weak_instance_ref(), + Self::Memory(m) => m.into_weak_instance_ref(), + Self::Table(t) => t.into_weak_instance_ref(), + } + } } impl StoreObject for Extern { diff --git a/lib/api/src/externals/table.rs b/lib/api/src/externals/table.rs index d959212ba31..bdad6998f83 100644 --- a/lib/api/src/externals/table.rs +++ b/lib/api/src/externals/table.rs @@ -146,6 +146,13 @@ impl Table { pub fn same(&self, other: &Self) -> bool { Arc::ptr_eq(&self.vm_table.from, &other.vm_table.from) } + + /// Check if the table holds a strong `InstanceRef`. + /// None means there's no `InstanceRef`, strong or weak. + // TODO: maybe feature gate this, we only need it for tests... + pub fn is_strong_instance_ref(&self) -> Option { + self.vm_table.instance_ref.as_ref().map(|v| v.is_strong()) + } } impl<'a> Exportable<'a> for Table { @@ -159,4 +166,8 @@ impl<'a> Exportable<'a> for Table { _ => Err(ExportError::IncompatibleType), } } + + fn into_weak_instance_ref(&mut self) { + self.vm_table.instance_ref.as_mut().map(|v| v.into_weak()); + } } diff --git a/lib/api/src/native.rs b/lib/api/src/native.rs index 3744f559edc..992b4987597 100644 --- a/lib/api/src/native.rs +++ b/lib/api/src/native.rs @@ -196,6 +196,12 @@ macro_rules! impl_native_traits { } } + /// Check if the function holds a strong `InstanceRef`. + /// None means there's no `InstanceRef`, strong or weak. + // TODO: maybe feature gate this, we only need it for tests... + pub fn is_strong_instance_ref(&self) -> Option { + self.exported.vm_function.instance_ref.as_ref().map(|v| v.is_strong()) + } } #[allow(unused_parens)] @@ -208,6 +214,10 @@ macro_rules! impl_native_traits { use crate::exports::Exportable; crate::Function::get_self_from_extern(_extern)?.native().map_err(|_| crate::exports::ExportError::IncompatibleType) } + + fn into_weak_instance_ref(&mut self) { + self.exported.vm_function.instance_ref.as_mut().map(|v| v.into_weak()); + } } }; } diff --git a/lib/api/tests/export.rs b/lib/api/tests/export.rs new file mode 100644 index 00000000000..646a109a269 --- /dev/null +++ b/lib/api/tests/export.rs @@ -0,0 +1,264 @@ +use anyhow::Result; +use wasmer::*; + +const MEM_WAT: &str = " + (module + (func $host_fn (import \"env\" \"host_fn\") (param) (result)) + (func (export \"call_host_fn\") (param) (result) + (call $host_fn)) + + (memory $mem 0) + (export \"memory\" (memory $mem)) + ) +"; + +const GLOBAL_WAT: &str = " + (module + (func $host_fn (import \"env\" \"host_fn\") (param) (result)) + (func (export \"call_host_fn\") (param) (result) + (call $host_fn)) + + (global $global i32 (i32.const 11)) + (export \"global\" (global $global)) + ) +"; + +const TABLE_WAT: &str = " + (module + (func $host_fn (import \"env\" \"host_fn\") (param) (result)) + (func (export \"call_host_fn\") (param) (result) + (call $host_fn)) + + (table $table 4 4 funcref) + (export \"table\" (table $table)) + ) +"; + +const FUNCTION_WAT: &str = " + (module + (func $host_fn (import \"env\" \"host_fn\") (param) (result)) + (func (export \"call_host_fn\") (param) (result) + (call $host_fn)) + ) +"; + +#[test] +fn strong_weak_behavior_works_memory() -> Result<()> { + #[derive(Clone, Debug, WasmerEnv, Default)] + struct MemEnv { + #[wasmer(export)] + memory: LazyInit, + } + + let host_fn = |env: &MemEnv| { + let mem = env.memory_ref().unwrap(); + assert_eq!(mem.is_strong_instance_ref(), Some(false)); + let mem_clone = mem.clone(); + assert_eq!(mem_clone.is_strong_instance_ref(), Some(true)); + assert_eq!(mem.is_strong_instance_ref(), Some(false)); + }; + + let f: NativeFunc<(), ()> = { + let store = Store::default(); + let module = Module::new(&store, MEM_WAT)?; + let env = MemEnv::default(); + + let instance = Instance::new( + &module, + &imports! { + "env" => { + "host_fn" => Function::new_native_with_env(&store, env, host_fn) + } + }, + )?; + + { + let mem = instance.exports.get_memory("memory")?; + assert_eq!(mem.is_strong_instance_ref(), Some(true)); + } + + let f: NativeFunc<(), ()> = instance.exports.get_native_function("call_host_fn")?; + f.call()?; + f + }; + f.call()?; + + Ok(()) +} + +#[test] +fn strong_weak_behavior_works_global() -> Result<()> { + #[derive(Clone, Debug, WasmerEnv, Default)] + struct GlobalEnv { + #[wasmer(export)] + global: LazyInit, + } + + let host_fn = |env: &GlobalEnv| { + let global = env.global_ref().unwrap(); + assert_eq!(global.is_strong_instance_ref(), Some(false)); + let global_clone = global.clone(); + assert_eq!(global_clone.is_strong_instance_ref(), Some(true)); + assert_eq!(global.is_strong_instance_ref(), Some(false)); + }; + + let f: NativeFunc<(), ()> = { + let store = Store::default(); + let module = Module::new(&store, GLOBAL_WAT)?; + let env = GlobalEnv::default(); + + let instance = Instance::new( + &module, + &imports! { + "env" => { + "host_fn" => Function::new_native_with_env(&store, env, host_fn) + } + }, + )?; + + { + let global = instance.exports.get_global("global")?; + assert_eq!(global.is_strong_instance_ref(), Some(true)); + } + + let f: NativeFunc<(), ()> = instance.exports.get_native_function("call_host_fn")?; + f.call()?; + f + }; + f.call()?; + + Ok(()) +} + +#[test] +fn strong_weak_behavior_works_table() -> Result<()> { + #[derive(Clone, WasmerEnv, Default)] + struct TableEnv { + #[wasmer(export)] + table: LazyInit, + } + + let host_fn = |env: &TableEnv| { + let table = env.table_ref().unwrap(); + assert_eq!(table.is_strong_instance_ref(), Some(false)); + let table_clone = table.clone(); + assert_eq!(table_clone.is_strong_instance_ref(), Some(true)); + assert_eq!(table.is_strong_instance_ref(), Some(false)); + }; + + let f: NativeFunc<(), ()> = { + let store = Store::default(); + let module = Module::new(&store, TABLE_WAT)?; + let env = TableEnv::default(); + + let instance = Instance::new( + &module, + &imports! { + "env" => { + "host_fn" => Function::new_native_with_env(&store, env, host_fn) + } + }, + )?; + + { + let table = instance.exports.get_table("table")?; + assert_eq!(table.is_strong_instance_ref(), Some(true)); + } + + let f: NativeFunc<(), ()> = instance.exports.get_native_function("call_host_fn")?; + f.call()?; + f + }; + f.call()?; + + Ok(()) +} + +#[test] +fn strong_weak_behavior_works_function() -> Result<()> { + #[derive(Clone, WasmerEnv, Default)] + struct FunctionEnv { + #[wasmer(export)] + call_host_fn: LazyInit, + } + + let host_fn = |env: &FunctionEnv| { + let function = env.call_host_fn_ref().unwrap(); + assert_eq!(function.is_strong_instance_ref(), Some(false)); + let function_clone = function.clone(); + assert_eq!(function_clone.is_strong_instance_ref(), Some(true)); + assert_eq!(function.is_strong_instance_ref(), Some(false)); + }; + + let f: NativeFunc<(), ()> = { + let store = Store::default(); + let module = Module::new(&store, FUNCTION_WAT)?; + let env = FunctionEnv::default(); + + let instance = Instance::new( + &module, + &imports! { + "env" => { + "host_fn" => Function::new_native_with_env(&store, env, host_fn) + } + }, + )?; + + { + let function = instance.exports.get_function("call_host_fn")?; + assert_eq!(function.is_strong_instance_ref(), Some(true)); + } + + let f: NativeFunc<(), ()> = instance.exports.get_native_function("call_host_fn")?; + f.call()?; + f + }; + f.call()?; + + Ok(()) +} + +#[test] +fn strong_weak_behavior_works_native_function() -> Result<()> { + #[derive(Clone, WasmerEnv, Default)] + struct FunctionEnv { + #[wasmer(export)] + call_host_fn: LazyInit>, + } + + let host_fn = |env: &FunctionEnv| { + let function = env.call_host_fn_ref().unwrap(); + assert_eq!(function.is_strong_instance_ref(), Some(false)); + let function_clone = function.clone(); + assert_eq!(function_clone.is_strong_instance_ref(), Some(true)); + assert_eq!(function.is_strong_instance_ref(), Some(false)); + }; + + let f: NativeFunc<(), ()> = { + let store = Store::default(); + let module = Module::new(&store, FUNCTION_WAT)?; + let env = FunctionEnv::default(); + + let instance = Instance::new( + &module, + &imports! { + "env" => { + "host_fn" => Function::new_native_with_env(&store, env, host_fn) + } + }, + )?; + + { + let function: NativeFunc<(), ()> = + instance.exports.get_native_function("call_host_fn")?; + assert_eq!(function.is_strong_instance_ref(), Some(true)); + } + + let f: NativeFunc<(), ()> = instance.exports.get_native_function("call_host_fn")?; + f.call()?; + f + }; + f.call()?; + + Ok(()) +} diff --git a/lib/vm/src/export.rs b/lib/vm/src/export.rs index b0cfa087b96..4374622652f 100644 --- a/lib/vm/src/export.rs +++ b/lib/vm/src/export.rs @@ -2,7 +2,7 @@ // Attributions: https://github.com/wasmerio/wasmer/blob/master/ATTRIBUTIONS.md use crate::global::Global; -use crate::instance::{InstanceRef, WeakOrStrongInstanceRef}; +use crate::instance::WeakOrStrongInstanceRef; use crate::memory::{Memory, MemoryStyle}; use crate::table::{Table, TableStyle}; use crate::vmcontext::{VMFunctionBody, VMFunctionEnvironment, VMFunctionKind, VMTrampoline}; @@ -27,7 +27,7 @@ pub enum VMExtern { } /// A function export value. -#[derive(Debug, Clone, PartialEq, MemoryUsage)] +#[derive(Debug, PartialEq, MemoryUsage)] pub struct VMFunction { /// The address of the native-code function. pub address: *const VMFunctionBody, @@ -52,7 +52,27 @@ pub struct VMFunction { /// A “reference” to the instance through the /// `InstanceRef`. `None` if it is a host function. - pub instance_ref: Option, + pub instance_ref: Option, +} + +// Is this just a bad idea? We want the default behavior to be converting from weak +// to strong... but is this just a footgun? +impl Clone for VMFunction { + fn clone(&self) -> Self { + // REVIEW: panicking in clone, etc etc. There's probably a much more elegant + // way to express this. + Self { + address: self.address.clone(), + vmctx: self.vmctx.clone(), + signature: self.signature.clone(), + kind: self.kind.clone(), + call_trampoline: self.call_trampoline.clone(), + instance_ref: self + .instance_ref + .as_ref() + .map(|v| WeakOrStrongInstanceRef::Strong(v.get_strong().unwrap())), + } + } } /// # Safety @@ -70,14 +90,30 @@ impl From for VMExtern { } /// A table export value. -#[derive(Debug, Clone, MemoryUsage)] +#[derive(Debug, MemoryUsage)] pub struct VMTable { /// Pointer to the containing `Table`. pub from: Arc, /// A “reference” to the instance through the /// `InstanceRef`. `None` if it is a host table. - pub instance_ref: Option, + pub instance_ref: Option, +} + +// Is this just a bad idea? We want the default behavior to be converting from weak +// to strong... but is this just a footgun? +impl Clone for VMTable { + fn clone(&self) -> Self { + // REVIEW: panicking in clone, etc etc. There's probably a much more elegant + // way to express this. + Self { + from: self.from.clone(), + instance_ref: self + .instance_ref + .as_ref() + .map(|v| WeakOrStrongInstanceRef::Strong(v.get_strong().unwrap())), + } + } } /// # Safety @@ -178,14 +214,30 @@ impl From for VMExtern { } /// A global export value. -#[derive(Debug, Clone, MemoryUsage)] +#[derive(Debug, MemoryUsage)] pub struct VMGlobal { /// The global declaration, used for compatibility checking. pub from: Arc, /// A “reference” to the instance through the /// `InstanceRef`. `None` if it is a host global. - pub instance_ref: Option, + pub instance_ref: Option, +} + +// Is this just a bad idea? We want the default behavior to be converting from weak +// to strong... but is this just a footgun? +impl Clone for VMGlobal { + fn clone(&self) -> Self { + // REVIEW: panicking in clone, etc etc. There's probably a much more elegant + // way to express this. + Self { + from: self.from.clone(), + instance_ref: self + .instance_ref + .as_ref() + .map(|v| WeakOrStrongInstanceRef::Strong(v.get_strong().unwrap())), + } + } } /// # Safety diff --git a/lib/vm/src/instance/mod.rs b/lib/vm/src/instance/mod.rs index 102ea9ab523..366339f9fdf 100644 --- a/lib/vm/src/instance/mod.rs +++ b/lib/vm/src/instance/mod.rs @@ -1100,7 +1100,7 @@ impl InstanceHandle { signature, vmctx, call_trampoline, - instance_ref: Some(instance), + instance_ref: Some(WeakOrStrongInstanceRef::Strong(instance)), } .into() } @@ -1113,7 +1113,7 @@ impl InstanceHandle { }; VMTable { from, - instance_ref: Some(instance), + instance_ref: Some(WeakOrStrongInstanceRef::Strong(instance)), } .into() } @@ -1141,7 +1141,7 @@ impl InstanceHandle { }; VMGlobal { from, - instance_ref: Some(instance), + instance_ref: Some(WeakOrStrongInstanceRef::Strong(instance)), } .into() } diff --git a/lib/vm/src/instance/ref.rs b/lib/vm/src/instance/ref.rs index f98d2d2c153..50fd3cffa66 100644 --- a/lib/vm/src/instance/ref.rs +++ b/lib/vm/src/instance/ref.rs @@ -156,6 +156,12 @@ impl InstanceRef { /// TODO: document this pub struct WeakInstanceRef(Weak); +impl PartialEq for WeakInstanceRef { + fn eq(&self, other: &Self) -> bool { + self.0.ptr_eq(&other.0) + } +} + impl WeakInstanceRef { // TODO: document this pub fn upgrade(&self) -> Option { @@ -165,12 +171,12 @@ impl WeakInstanceRef { } impl MemoryUsage for WeakInstanceRef { - fn size_of_val(&self, tracker: &mut dyn MemoryUsageTracker) -> usize { + fn size_of_val(&self, _tracker: &mut dyn MemoryUsageTracker) -> usize { todo!("Probably missing implementation at crate level for `Weak`. Can be done manually here but I'm focused on other things right now"); } } -#[derive(Debug, Clone, MemoryUsage)] +#[derive(Debug, Clone, PartialEq, MemoryUsage)] /// TODO: document this pub enum WeakOrStrongInstanceRef { /// A weak instance ref. @@ -201,4 +207,9 @@ impl WeakOrStrongInstanceRef { let new = self.get_weak(); *self = Self::Weak(new); } + + /// Check if the reference contained is strong. + pub fn is_strong(&self) -> bool { + matches!(self, WeakOrStrongInstanceRef::Strong(_)) + } } From fe8d077e4a7d1b1dd800c60b26b3d1ff3ca947bd Mon Sep 17 00:00:00 2001 From: Mark McCaskey Date: Tue, 25 May 2021 12:38:00 -0700 Subject: [PATCH 3/6] Clean up PR for review --- lib/api/src/exports.rs | 13 +++++++------ lib/vm/src/export.rs | 16 ---------------- lib/vm/src/instance/ref.rs | 22 ++++++++++++++++------ 3 files changed, 23 insertions(+), 28 deletions(-) diff --git a/lib/api/src/exports.rs b/lib/api/src/exports.rs index c4e56c2ce6d..899f494a33e 100644 --- a/lib/api/src/exports.rs +++ b/lib/api/src/exports.rs @@ -308,10 +308,10 @@ pub trait Exportable<'a>: Sized { /// [`Instance`]: crate::Instance fn get_self_from_extern(_extern: &'a Extern) -> Result<&'a Self, ExportError>; - /// TODO: this method doesn't belong here - fn into_weak_instance_ref(&mut self) { - todo!("into_weak_instance_ref") - } + /// Convert the extern internally to hold a weak reference to the `InstanceRef`. + /// This is useful for preventing cycles, for example for data stored in a + /// type implementing `WasmerEnv`. + fn into_weak_instance_ref(&mut self); } /// A trait for accessing exports (like [`Exportable`]) but it takes generic @@ -320,7 +320,9 @@ pub trait Exportable<'a>: Sized { pub trait ExportableWithGenerics<'a, Args: WasmTypeList, Rets: WasmTypeList>: Sized { /// Get an export with the given generics. fn get_self_from_extern_with_generics(_extern: &'a Extern) -> Result; - /// TODO: this method doesn't belong here + /// Convert the extern internally to hold a weak reference to the `InstanceRef`. + /// This is useful for preventing cycles, for example for data stored in a + /// type implementing `WasmerEnv`. fn into_weak_instance_ref(&mut self); } @@ -331,7 +333,6 @@ impl<'a, T: Exportable<'a> + Clone + 'static> ExportableWithGenerics<'a, (), ()> T::get_self_from_extern(_extern).map(|i| i.clone()) } - /// TODO: this method doesn't belong here fn into_weak_instance_ref(&mut self) { ::into_weak_instance_ref(self); } diff --git a/lib/vm/src/export.rs b/lib/vm/src/export.rs index 4374622652f..ea5077263a4 100644 --- a/lib/vm/src/export.rs +++ b/lib/vm/src/export.rs @@ -55,12 +55,8 @@ pub struct VMFunction { pub instance_ref: Option, } -// Is this just a bad idea? We want the default behavior to be converting from weak -// to strong... but is this just a footgun? impl Clone for VMFunction { fn clone(&self) -> Self { - // REVIEW: panicking in clone, etc etc. There's probably a much more elegant - // way to express this. Self { address: self.address.clone(), vmctx: self.vmctx.clone(), @@ -100,12 +96,8 @@ pub struct VMTable { pub instance_ref: Option, } -// Is this just a bad idea? We want the default behavior to be converting from weak -// to strong... but is this just a footgun? impl Clone for VMTable { fn clone(&self) -> Self { - // REVIEW: panicking in clone, etc etc. There's probably a much more elegant - // way to express this. Self { from: self.from.clone(), instance_ref: self @@ -162,12 +154,8 @@ pub struct VMMemory { pub instance_ref: Option, } -// Is this just a bad idea? We want the default behavior to be converting from weak -// to strong... but is this just a footgun? impl Clone for VMMemory { fn clone(&self) -> Self { - // REVIEW: panicking in clone, etc etc. There's probably a much more elegant - // way to express this. Self { from: self.from.clone(), instance_ref: self @@ -224,12 +212,8 @@ pub struct VMGlobal { pub instance_ref: Option, } -// Is this just a bad idea? We want the default behavior to be converting from weak -// to strong... but is this just a footgun? impl Clone for VMGlobal { fn clone(&self) -> Self { - // REVIEW: panicking in clone, etc etc. There's probably a much more elegant - // way to express this. Self { from: self.from.clone(), instance_ref: self diff --git a/lib/vm/src/instance/ref.rs b/lib/vm/src/instance/ref.rs index eee1274cda7..53ed9b85764 100644 --- a/lib/vm/src/instance/ref.rs +++ b/lib/vm/src/instance/ref.rs @@ -156,8 +156,10 @@ impl InstanceRef { } } +/// A weak instance ref. This type does not keep the underlying `Instance` alive +/// but can be converted into a full `InstanceRef` if the underlying `Instance` hasn't +/// been deallocated. #[derive(Debug, Clone)] -/// TODO: document this pub struct WeakInstanceRef(Weak); impl PartialEq for WeakInstanceRef { @@ -167,7 +169,7 @@ impl PartialEq for WeakInstanceRef { } impl WeakInstanceRef { - // TODO: document this + /// Try to convert into a strong, `InstanceRef`. pub fn upgrade(&self) -> Option { let inner = self.0.upgrade()?; Some(InstanceRef(inner)) @@ -175,13 +177,21 @@ impl WeakInstanceRef { } impl MemoryUsage for WeakInstanceRef { - fn size_of_val(&self, _tracker: &mut dyn MemoryUsageTracker) -> usize { - todo!("Probably missing implementation at crate level for `Weak`. Can be done manually here but I'm focused on other things right now"); + fn size_of_val(&self, tracker: &mut dyn MemoryUsageTracker) -> usize { + mem::size_of_val(self) + + if let Some(ir) = self.upgrade() { + ir.size_of_val(tracker) + } else { + 0 + } } } +/// An `InstanceRef` that may or may not be keeping the `Instance` alive. +/// +/// This type is useful for types that conditionally must keep / not keep the +/// underlying `Instance` alive. For example, to prevent cycles in `WasmerEnv`s. #[derive(Debug, Clone, PartialEq, MemoryUsage)] -/// TODO: document this pub enum WeakOrStrongInstanceRef { /// A weak instance ref. Weak(WeakInstanceRef), @@ -206,7 +216,7 @@ impl WeakOrStrongInstanceRef { } } - /// TODO: document this + /// Convert the existing type directly into a weak reference. pub fn into_weak(&mut self) { let new = self.get_weak(); *self = Self::Weak(new); From f0ad0bed115a5a79ded48ebc56375381c742e3b1 Mon Sep 17 00:00:00 2001 From: Mark McCaskey Date: Wed, 26 May 2021 13:48:57 -0700 Subject: [PATCH 4/6] Clean up WeakOrStrongInstanceRef methods --- lib/api/src/externals/function.rs | 2 +- lib/api/src/externals/global.rs | 5 ++- lib/api/src/externals/memory.rs | 5 ++- lib/api/src/externals/table.rs | 5 ++- lib/api/src/native.rs | 2 +- lib/vm/src/export.rs | 20 +++-------- lib/vm/src/instance/ref.rs | 59 +++++++++++++++++++++++-------- 7 files changed, 62 insertions(+), 36 deletions(-) diff --git a/lib/api/src/externals/function.rs b/lib/api/src/externals/function.rs index 57471623657..282a2ffdefa 100644 --- a/lib/api/src/externals/function.rs +++ b/lib/api/src/externals/function.rs @@ -742,7 +742,7 @@ impl<'a> Exportable<'a> for Function { .vm_function .instance_ref .as_mut() - .map(|v| v.into_weak()); + .map(|v| *v = v.downgrade()); } } diff --git a/lib/api/src/externals/global.rs b/lib/api/src/externals/global.rs index e6b7b031f8c..a18d46843bd 100644 --- a/lib/api/src/externals/global.rs +++ b/lib/api/src/externals/global.rs @@ -240,6 +240,9 @@ impl<'a> Exportable<'a> for Global { } fn into_weak_instance_ref(&mut self) { - self.vm_global.instance_ref.as_mut().map(|v| v.into_weak()); + self.vm_global + .instance_ref + .as_mut() + .map(|v| *v = v.downgrade()); } } diff --git a/lib/api/src/externals/memory.rs b/lib/api/src/externals/memory.rs index 2a3077f50f5..3c1c593e971 100644 --- a/lib/api/src/externals/memory.rs +++ b/lib/api/src/externals/memory.rs @@ -270,6 +270,9 @@ impl<'a> Exportable<'a> for Memory { } fn into_weak_instance_ref(&mut self) { - self.vm_memory.instance_ref.as_mut().map(|v| v.into_weak()); + self.vm_memory + .instance_ref + .as_mut() + .map(|v| *v = v.downgrade()); } } diff --git a/lib/api/src/externals/table.rs b/lib/api/src/externals/table.rs index bdad6998f83..fdaaeccfaeb 100644 --- a/lib/api/src/externals/table.rs +++ b/lib/api/src/externals/table.rs @@ -168,6 +168,9 @@ impl<'a> Exportable<'a> for Table { } fn into_weak_instance_ref(&mut self) { - self.vm_table.instance_ref.as_mut().map(|v| v.into_weak()); + self.vm_table + .instance_ref + .as_mut() + .map(|v| *v = v.downgrade()); } } diff --git a/lib/api/src/native.rs b/lib/api/src/native.rs index 992b4987597..6c8cb6184b1 100644 --- a/lib/api/src/native.rs +++ b/lib/api/src/native.rs @@ -216,7 +216,7 @@ macro_rules! impl_native_traits { } fn into_weak_instance_ref(&mut self) { - self.exported.vm_function.instance_ref.as_mut().map(|v| v.into_weak()); + self.exported.vm_function.instance_ref.as_mut().map(|v| *v = v.downgrade()); } } }; diff --git a/lib/vm/src/export.rs b/lib/vm/src/export.rs index ea5077263a4..fb9d1fabbd5 100644 --- a/lib/vm/src/export.rs +++ b/lib/vm/src/export.rs @@ -63,10 +63,7 @@ impl Clone for VMFunction { signature: self.signature.clone(), kind: self.kind.clone(), call_trampoline: self.call_trampoline.clone(), - instance_ref: self - .instance_ref - .as_ref() - .map(|v| WeakOrStrongInstanceRef::Strong(v.get_strong().unwrap())), + instance_ref: self.instance_ref.as_ref().map(|v| v.upgrade().unwrap()), } } } @@ -100,10 +97,7 @@ impl Clone for VMTable { fn clone(&self) -> Self { Self { from: self.from.clone(), - instance_ref: self - .instance_ref - .as_ref() - .map(|v| WeakOrStrongInstanceRef::Strong(v.get_strong().unwrap())), + instance_ref: self.instance_ref.as_ref().map(|v| v.upgrade().unwrap()), } } } @@ -158,10 +152,7 @@ impl Clone for VMMemory { fn clone(&self) -> Self { Self { from: self.from.clone(), - instance_ref: self - .instance_ref - .as_ref() - .map(|v| WeakOrStrongInstanceRef::Strong(v.get_strong().unwrap())), + instance_ref: self.instance_ref.as_ref().map(|v| v.upgrade().unwrap()), } } } @@ -216,10 +207,7 @@ impl Clone for VMGlobal { fn clone(&self) -> Self { Self { from: self.from.clone(), - instance_ref: self - .instance_ref - .as_ref() - .map(|v| WeakOrStrongInstanceRef::Strong(v.get_strong().unwrap())), + instance_ref: self.instance_ref.as_ref().map(|v| v.upgrade().unwrap()), } } } diff --git a/lib/vm/src/instance/ref.rs b/lib/vm/src/instance/ref.rs index 53ed9b85764..f11cd1c96c3 100644 --- a/lib/vm/src/instance/ref.rs +++ b/lib/vm/src/instance/ref.rs @@ -1,6 +1,7 @@ use super::Instance; use loupe::{MemoryUsage, MemoryUsageTracker}; use std::alloc::Layout; +use std::convert::TryFrom; use std::mem; use std::ptr::{self, NonNull}; use std::sync::{Arc, Weak}; @@ -200,30 +201,58 @@ pub enum WeakOrStrongInstanceRef { } impl WeakOrStrongInstanceRef { - /// Get a strong `InstanceRef`. A return Value of `None` means that the - /// `InstanceRef` has been freed and cannot be accessed. - pub fn get_strong(&self) -> Option { + /// Tries to upgrade weak references to a strong reference, returning None + /// if it can't be done. + pub fn upgrade(&self) -> Option { match self { - Self::Weak(weak) => weak.upgrade(), - Self::Strong(strong) => Some(strong.clone()), + Self::Weak(weak) => weak.upgrade().map(Self::Strong), + Self::Strong(strong) => Some(Self::Strong(strong.clone())), } } - /// Get a weak `InstanceRef`. - pub fn get_weak(&self) -> WeakInstanceRef { + + /// Clones self into a weak reference. + pub fn downgrade(&self) -> Self { match self { - Self::Weak(weak) => weak.clone(), - Self::Strong(strong) => WeakInstanceRef(Arc::downgrade(&strong.0)), + Self::Weak(weak) => Self::Weak(weak.clone()), + Self::Strong(strong) => Self::Weak(WeakInstanceRef(Arc::downgrade(&strong.0))), } } - /// Convert the existing type directly into a weak reference. - pub fn into_weak(&mut self) { - let new = self.get_weak(); - *self = Self::Weak(new); - } - /// Check if the reference contained is strong. pub fn is_strong(&self) -> bool { matches!(self, WeakOrStrongInstanceRef::Strong(_)) } } + +impl TryFrom for InstanceRef { + type Error = &'static str; + fn try_from(value: WeakOrStrongInstanceRef) -> Result { + match value { + WeakOrStrongInstanceRef::Strong(strong) => Ok(strong), + WeakOrStrongInstanceRef::Weak(weak) => { + weak.upgrade().ok_or("Failed to upgrade weak reference") + } + } + } +} + +impl From for WeakInstanceRef { + fn from(value: WeakOrStrongInstanceRef) -> Self { + match value { + WeakOrStrongInstanceRef::Strong(strong) => Self(Arc::downgrade(&strong.0)), + WeakOrStrongInstanceRef::Weak(weak) => weak, + } + } +} + +impl From for WeakOrStrongInstanceRef { + fn from(value: WeakInstanceRef) -> Self { + Self::Weak(value) + } +} + +impl From for WeakOrStrongInstanceRef { + fn from(value: InstanceRef) -> Self { + Self::Strong(value) + } +} From 9b216cc5e5e6fb519501df68cd0f6d2cbdebb0fa Mon Sep 17 00:00:00 2001 From: Mark McCaskey Date: Fri, 28 May 2021 11:40:33 -0700 Subject: [PATCH 5/6] Address testing related feedback Expose inner VM externals as an unsafe method and move logic into the tests --- lib/api/src/externals/function.rs | 19 +++--- lib/api/src/externals/global.rs | 15 ++-- lib/api/src/externals/memory.rs | 15 ++-- lib/api/src/externals/table.rs | 15 ++-- lib/api/src/native.rs | 20 ++++-- lib/api/tests/export.rs | 110 ++++++++++++++++++++++++------ lib/api/tests/externals.rs | 27 ++++++++ lib/vm/src/instance/ref.rs | 5 -- lib/vm/src/lib.rs | 1 + 9 files changed, 171 insertions(+), 56 deletions(-) diff --git a/lib/api/src/externals/function.rs b/lib/api/src/externals/function.rs index 282a2ffdefa..0f3790d81de 100644 --- a/lib/api/src/externals/function.rs +++ b/lib/api/src/externals/function.rs @@ -713,15 +713,16 @@ impl Function { unimplemented!("Closures (functions with captured environments) are currently unsupported with native functions. See: https://github.com/wasmerio/wasmer/issues/1840") } - /// Check if the function holds a strong `InstanceRef`. - /// None means there's no `InstanceRef`, strong or weak. - // TODO: maybe feature gate this, we only need it for tests... - pub fn is_strong_instance_ref(&self) -> Option { - self.exported - .vm_function - .instance_ref - .as_ref() - .map(|v| v.is_strong()) + /// Get access to the backing VM value for this extern. This function is for + /// tests it should not be called by users of the Wasmer API. + /// + /// # Safety + /// This function is unsafe to call outside of tests for the wasmer crate + /// because there is no stability guarantee for the returned type and we may + /// make breaking changes to it at any time or remove this method. + #[doc(hidden)] + pub unsafe fn get_vm_function(&self) -> &VMFunction { + &self.exported.vm_function } } diff --git a/lib/api/src/externals/global.rs b/lib/api/src/externals/global.rs index a18d46843bd..f8ab68d72b1 100644 --- a/lib/api/src/externals/global.rs +++ b/lib/api/src/externals/global.rs @@ -209,11 +209,16 @@ impl Global { Arc::ptr_eq(&self.vm_global.from, &other.vm_global.from) } - /// Check if the global holds a strong `InstanceRef`. - /// None means there's no `InstanceRef`, strong or weak. - // TODO: maybe feature gate this, we only need it for tests... - pub fn is_strong_instance_ref(&self) -> Option { - self.vm_global.instance_ref.as_ref().map(|v| v.is_strong()) + /// Get access to the backing VM value for this extern. This function is for + /// tests it should not be called by users of the Wasmer API. + /// + /// # Safety + /// This function is unsafe to call outside of tests for the wasmer crate + /// because there is no stability guarantee for the returned type and we may + /// make breaking changes to it at any time or remove this method. + #[doc(hidden)] + pub unsafe fn get_vm_global(&self) -> &VMGlobal { + &self.vm_global } } diff --git a/lib/api/src/externals/memory.rs b/lib/api/src/externals/memory.rs index 3c1c593e971..8ffda1c934d 100644 --- a/lib/api/src/externals/memory.rs +++ b/lib/api/src/externals/memory.rs @@ -249,11 +249,16 @@ impl Memory { Arc::ptr_eq(&self.vm_memory.from, &other.vm_memory.from) } - /// Check if the memory holds a strong `InstanceRef`. - /// None means there's no `InstanceRef`, strong or weak. - // TODO: maybe feature gate this, we only need it for tests... - pub fn is_strong_instance_ref(&self) -> Option { - self.vm_memory.instance_ref.as_ref().map(|v| v.is_strong()) + /// Get access to the backing VM value for this extern. This function is for + /// tests it should not be called by users of the Wasmer API. + /// + /// # Safety + /// This function is unsafe to call outside of tests for the wasmer crate + /// because there is no stability guarantee for the returned type and we may + /// make breaking changes to it at any time or remove this method. + #[doc(hidden)] + pub unsafe fn get_vm_memory(&self) -> &VMMemory { + &self.vm_memory } } diff --git a/lib/api/src/externals/table.rs b/lib/api/src/externals/table.rs index fdaaeccfaeb..071b70ae42d 100644 --- a/lib/api/src/externals/table.rs +++ b/lib/api/src/externals/table.rs @@ -147,11 +147,16 @@ impl Table { Arc::ptr_eq(&self.vm_table.from, &other.vm_table.from) } - /// Check if the table holds a strong `InstanceRef`. - /// None means there's no `InstanceRef`, strong or weak. - // TODO: maybe feature gate this, we only need it for tests... - pub fn is_strong_instance_ref(&self) -> Option { - self.vm_table.instance_ref.as_ref().map(|v| v.is_strong()) + /// Get access to the backing VM value for this extern. This function is for + /// tests it should not be called by users of the Wasmer API. + /// + /// # Safety + /// This function is unsafe to call outside of tests for the wasmer crate + /// because there is no stability guarantee for the returned type and we may + /// make breaking changes to it at any time or remove this method. + #[doc(hidden)] + pub unsafe fn get_vm_table(&self) -> &VMTable { + &self.vm_table } } diff --git a/lib/api/src/native.rs b/lib/api/src/native.rs index 6c8cb6184b1..11421aa042d 100644 --- a/lib/api/src/native.rs +++ b/lib/api/src/native.rs @@ -55,6 +55,18 @@ where pub(crate) fn arg_kind(&self) -> VMFunctionKind { self.exported.vm_function.kind } + + /// Get access to the backing VM value for this extern. This function is for + /// tests it should not be called by users of the Wasmer API. + /// + /// # Safety + /// This function is unsafe to call outside of tests for the wasmer crate + /// because there is no stability guarantee for the returned type and we may + /// make breaking changes to it at any time or remove this method. + #[doc(hidden)] + pub unsafe fn get_vm_function(&self) -> &wasmer_vm::VMFunction { + &self.exported.vm_function + } } /* @@ -194,14 +206,8 @@ macro_rules! impl_native_traits { } } } - - } - /// Check if the function holds a strong `InstanceRef`. - /// None means there's no `InstanceRef`, strong or weak. - // TODO: maybe feature gate this, we only need it for tests... - pub fn is_strong_instance_ref(&self) -> Option { - self.exported.vm_function.instance_ref.as_ref().map(|v| v.is_strong()) } + } #[allow(unused_parens)] diff --git a/lib/api/tests/export.rs b/lib/api/tests/export.rs index 646a109a269..b65d2487bb2 100644 --- a/lib/api/tests/export.rs +++ b/lib/api/tests/export.rs @@ -1,5 +1,6 @@ use anyhow::Result; use wasmer::*; +use wasmer_vm::WeakOrStrongInstanceRef; const MEM_WAT: &str = " (module @@ -42,6 +43,63 @@ const FUNCTION_WAT: &str = " ) "; +fn is_memory_instance_ref_strong(memory: &Memory) -> Option { + // This is safe because we're calling it from a test to test the internals + unsafe { + memory + .get_vm_memory() + .instance_ref + .as_ref() + .map(|v| matches!(v, WeakOrStrongInstanceRef::Strong(_))) + } +} + +fn is_table_instance_ref_strong(table: &Table) -> Option { + // This is safe because we're calling it from a test to test the internals + unsafe { + table + .get_vm_table() + .instance_ref + .as_ref() + .map(|v| matches!(v, WeakOrStrongInstanceRef::Strong(_))) + } +} + +fn is_global_instance_ref_strong(global: &Global) -> Option { + // This is safe because we're calling it from a test to test the internals + unsafe { + global + .get_vm_global() + .instance_ref + .as_ref() + .map(|v| matches!(v, WeakOrStrongInstanceRef::Strong(_))) + } +} + +fn is_function_instance_ref_strong(f: &Function) -> Option { + // This is safe because we're calling it from a test to test the internals + unsafe { + f.get_vm_function() + .instance_ref + .as_ref() + .map(|v| matches!(v, WeakOrStrongInstanceRef::Strong(_))) + } +} + +fn is_native_function_instance_ref_strong(f: &NativeFunc) -> Option +where + Args: WasmTypeList, + Rets: WasmTypeList, +{ + // This is safe because we're calling it from a test to test the internals + unsafe { + f.get_vm_function() + .instance_ref + .as_ref() + .map(|v| matches!(v, WeakOrStrongInstanceRef::Strong(_))) + } +} + #[test] fn strong_weak_behavior_works_memory() -> Result<()> { #[derive(Clone, Debug, WasmerEnv, Default)] @@ -52,10 +110,10 @@ fn strong_weak_behavior_works_memory() -> Result<()> { let host_fn = |env: &MemEnv| { let mem = env.memory_ref().unwrap(); - assert_eq!(mem.is_strong_instance_ref(), Some(false)); + assert_eq!(is_memory_instance_ref_strong(&mem), Some(false)); let mem_clone = mem.clone(); - assert_eq!(mem_clone.is_strong_instance_ref(), Some(true)); - assert_eq!(mem.is_strong_instance_ref(), Some(false)); + assert_eq!(is_memory_instance_ref_strong(&mem_clone), Some(true)); + assert_eq!(is_memory_instance_ref_strong(&mem), Some(false)); }; let f: NativeFunc<(), ()> = { @@ -74,7 +132,7 @@ fn strong_weak_behavior_works_memory() -> Result<()> { { let mem = instance.exports.get_memory("memory")?; - assert_eq!(mem.is_strong_instance_ref(), Some(true)); + assert_eq!(is_memory_instance_ref_strong(&mem), Some(true)); } let f: NativeFunc<(), ()> = instance.exports.get_native_function("call_host_fn")?; @@ -96,10 +154,10 @@ fn strong_weak_behavior_works_global() -> Result<()> { let host_fn = |env: &GlobalEnv| { let global = env.global_ref().unwrap(); - assert_eq!(global.is_strong_instance_ref(), Some(false)); + assert_eq!(is_global_instance_ref_strong(&global), Some(false)); let global_clone = global.clone(); - assert_eq!(global_clone.is_strong_instance_ref(), Some(true)); - assert_eq!(global.is_strong_instance_ref(), Some(false)); + assert_eq!(is_global_instance_ref_strong(&global_clone), Some(true)); + assert_eq!(is_global_instance_ref_strong(&global), Some(false)); }; let f: NativeFunc<(), ()> = { @@ -118,7 +176,7 @@ fn strong_weak_behavior_works_global() -> Result<()> { { let global = instance.exports.get_global("global")?; - assert_eq!(global.is_strong_instance_ref(), Some(true)); + assert_eq!(is_global_instance_ref_strong(&global), Some(true)); } let f: NativeFunc<(), ()> = instance.exports.get_native_function("call_host_fn")?; @@ -140,10 +198,10 @@ fn strong_weak_behavior_works_table() -> Result<()> { let host_fn = |env: &TableEnv| { let table = env.table_ref().unwrap(); - assert_eq!(table.is_strong_instance_ref(), Some(false)); + assert_eq!(is_table_instance_ref_strong(&table), Some(false)); let table_clone = table.clone(); - assert_eq!(table_clone.is_strong_instance_ref(), Some(true)); - assert_eq!(table.is_strong_instance_ref(), Some(false)); + assert_eq!(is_table_instance_ref_strong(&table_clone), Some(true)); + assert_eq!(is_table_instance_ref_strong(&table), Some(false)); }; let f: NativeFunc<(), ()> = { @@ -162,7 +220,7 @@ fn strong_weak_behavior_works_table() -> Result<()> { { let table = instance.exports.get_table("table")?; - assert_eq!(table.is_strong_instance_ref(), Some(true)); + assert_eq!(is_table_instance_ref_strong(&table), Some(true)); } let f: NativeFunc<(), ()> = instance.exports.get_native_function("call_host_fn")?; @@ -184,10 +242,10 @@ fn strong_weak_behavior_works_function() -> Result<()> { let host_fn = |env: &FunctionEnv| { let function = env.call_host_fn_ref().unwrap(); - assert_eq!(function.is_strong_instance_ref(), Some(false)); + assert_eq!(is_function_instance_ref_strong(&function), Some(false)); let function_clone = function.clone(); - assert_eq!(function_clone.is_strong_instance_ref(), Some(true)); - assert_eq!(function.is_strong_instance_ref(), Some(false)); + assert_eq!(is_function_instance_ref_strong(&function_clone), Some(true)); + assert_eq!(is_function_instance_ref_strong(&function), Some(false)); }; let f: NativeFunc<(), ()> = { @@ -206,7 +264,7 @@ fn strong_weak_behavior_works_function() -> Result<()> { { let function = instance.exports.get_function("call_host_fn")?; - assert_eq!(function.is_strong_instance_ref(), Some(true)); + assert_eq!(is_function_instance_ref_strong(&function), Some(true)); } let f: NativeFunc<(), ()> = instance.exports.get_native_function("call_host_fn")?; @@ -228,10 +286,19 @@ fn strong_weak_behavior_works_native_function() -> Result<()> { let host_fn = |env: &FunctionEnv| { let function = env.call_host_fn_ref().unwrap(); - assert_eq!(function.is_strong_instance_ref(), Some(false)); + assert_eq!( + is_native_function_instance_ref_strong(&function), + Some(false) + ); let function_clone = function.clone(); - assert_eq!(function_clone.is_strong_instance_ref(), Some(true)); - assert_eq!(function.is_strong_instance_ref(), Some(false)); + assert_eq!( + is_native_function_instance_ref_strong(&function_clone), + Some(true) + ); + assert_eq!( + is_native_function_instance_ref_strong(&function), + Some(false) + ); }; let f: NativeFunc<(), ()> = { @@ -251,7 +318,10 @@ fn strong_weak_behavior_works_native_function() -> Result<()> { { let function: NativeFunc<(), ()> = instance.exports.get_native_function("call_host_fn")?; - assert_eq!(function.is_strong_instance_ref(), Some(true)); + assert_eq!( + is_native_function_instance_ref_strong(&function), + Some(true) + ); } let f: NativeFunc<(), ()> = instance.exports.get_native_function("call_host_fn")?; diff --git a/lib/api/tests/externals.rs b/lib/api/tests/externals.rs index 1241eb3640a..dff570d2ef2 100644 --- a/lib/api/tests/externals.rs +++ b/lib/api/tests/externals.rs @@ -400,6 +400,33 @@ fn function_outlives_instance() -> Result<()> { Ok(()) } +#[test] +fn weak_instance_ref_externs_after_instance() -> Result<()> { + let store = Store::default(); + let wat = r#"(module + (memory (export "mem") 1) + (type $sum_t (func (param i32 i32) (result i32))) + (func $sum_f (type $sum_t) (param $x i32) (param $y i32) (result i32) + local.get $x + local.get $y + i32.add) + (export "sum" (func $sum_f))) +"#; + + let f = { + let module = Module::new(&store, wat)?; + let instance = Instance::new(&module, &imports! {})?; + let f: NativeFunc<(i32, i32), i32> = instance.exports.get_with_generics_weak("sum")?; + + assert_eq!(f.call(4, 5)?, 9); + f + }; + + assert_eq!(f.call(4, 5)?, 9); + + Ok(()) +} + #[test] fn manually_generate_wasmer_env() -> Result<()> { let store = Store::default(); diff --git a/lib/vm/src/instance/ref.rs b/lib/vm/src/instance/ref.rs index f11cd1c96c3..47f319593af 100644 --- a/lib/vm/src/instance/ref.rs +++ b/lib/vm/src/instance/ref.rs @@ -217,11 +217,6 @@ impl WeakOrStrongInstanceRef { Self::Strong(strong) => Self::Weak(WeakInstanceRef(Arc::downgrade(&strong.0))), } } - - /// Check if the reference contained is strong. - pub fn is_strong(&self) -> bool { - matches!(self, WeakOrStrongInstanceRef::Strong(_)) - } } impl TryFrom for InstanceRef { diff --git a/lib/vm/src/lib.rs b/lib/vm/src/lib.rs index dc4a1d8f2fb..87d51529a45 100644 --- a/lib/vm/src/lib.rs +++ b/lib/vm/src/lib.rs @@ -44,6 +44,7 @@ pub use crate::global::*; pub use crate::imports::Imports; pub use crate::instance::{ ImportFunctionEnv, ImportInitializerFuncPtr, InstanceAllocator, InstanceHandle, + WeakOrStrongInstanceRef, }; pub use crate::memory::{LinearMemory, Memory, MemoryError, MemoryStyle}; pub use crate::mmap::Mmap; From 229d0dcd32c868fd02e61db05e5bce371790a840 Mon Sep 17 00:00:00 2001 From: Mark McCaskey Date: Fri, 28 May 2021 12:05:52 -0700 Subject: [PATCH 6/6] Address feedback about location of InstanceRef upgrade --- lib/api/src/externals/function.rs | 14 +++++- lib/api/src/externals/global.rs | 14 +++++- lib/api/src/externals/memory.rs | 14 +++++- lib/api/src/externals/table.rs | 14 +++++- lib/api/src/native.rs | 14 +++++- lib/vm/src/export.rs | 78 +++++++++++++++---------------- 6 files changed, 103 insertions(+), 45 deletions(-) diff --git a/lib/api/src/externals/function.rs b/lib/api/src/externals/function.rs index 0f3790d81de..8bc8565e707 100644 --- a/lib/api/src/externals/function.rs +++ b/lib/api/src/externals/function.rs @@ -39,7 +39,7 @@ use wasmer_vm::{ /// with native functions. Attempting to create a native `Function` with one will /// result in a panic. /// [Closures as host functions tracking issue](https://github.com/wasmerio/wasmer/issues/1840) -#[derive(Clone, PartialEq, MemoryUsage)] +#[derive(PartialEq, MemoryUsage)] pub struct Function { pub(crate) store: Store, pub(crate) exported: ExportFunction, @@ -747,6 +747,18 @@ impl<'a> Exportable<'a> for Function { } } +impl Clone for Function { + fn clone(&self) -> Self { + let mut exported = self.exported.clone(); + exported.vm_function.upgrade_instance_ref().unwrap(); + + Self { + store: self.store.clone(), + exported, + } + } +} + impl fmt::Debug for Function { fn fmt(&self, formatter: &mut fmt::Formatter) -> fmt::Result { formatter diff --git a/lib/api/src/externals/global.rs b/lib/api/src/externals/global.rs index f8ab68d72b1..788c649bf48 100644 --- a/lib/api/src/externals/global.rs +++ b/lib/api/src/externals/global.rs @@ -17,7 +17,7 @@ use wasmer_vm::{Global as RuntimeGlobal, VMGlobal}; /// It consists of an individual value and a flag indicating whether it is mutable. /// /// Spec: -#[derive(Clone, MemoryUsage)] +#[derive(MemoryUsage)] pub struct Global { store: Store, vm_global: VMGlobal, @@ -222,6 +222,18 @@ impl Global { } } +impl Clone for Global { + fn clone(&self) -> Self { + let mut vm_global = self.vm_global.clone(); + vm_global.upgrade_instance_ref().unwrap(); + + Self { + store: self.store.clone(), + vm_global, + } + } +} + impl fmt::Debug for Global { fn fmt(&self, formatter: &mut fmt::Formatter) -> fmt::Result { formatter diff --git a/lib/api/src/externals/memory.rs b/lib/api/src/externals/memory.rs index 8ffda1c934d..0f02f8ccec3 100644 --- a/lib/api/src/externals/memory.rs +++ b/lib/api/src/externals/memory.rs @@ -24,7 +24,7 @@ use wasmer_vm::{MemoryError, VMMemory}; /// mutable from both host and WebAssembly. /// /// Spec: -#[derive(Debug, Clone, MemoryUsage)] +#[derive(Debug, MemoryUsage)] pub struct Memory { store: Store, vm_memory: VMMemory, @@ -262,6 +262,18 @@ impl Memory { } } +impl Clone for Memory { + fn clone(&self) -> Self { + let mut vm_memory = self.vm_memory.clone(); + vm_memory.upgrade_instance_ref().unwrap(); + + Self { + store: self.store.clone(), + vm_memory, + } + } +} + impl<'a> Exportable<'a> for Memory { fn to_export(&self) -> Export { self.vm_memory.clone().into() diff --git a/lib/api/src/externals/table.rs b/lib/api/src/externals/table.rs index 071b70ae42d..b006927b1f6 100644 --- a/lib/api/src/externals/table.rs +++ b/lib/api/src/externals/table.rs @@ -18,7 +18,7 @@ use wasmer_vm::{Table as RuntimeTable, TableElement, VMTable}; /// mutable from both host and WebAssembly. /// /// Spec: -#[derive(Clone, MemoryUsage)] +#[derive(MemoryUsage)] pub struct Table { store: Store, vm_table: VMTable, @@ -160,6 +160,18 @@ impl Table { } } +impl Clone for Table { + fn clone(&self) -> Self { + let mut vm_table = self.vm_table.clone(); + vm_table.upgrade_instance_ref().unwrap(); + + Self { + store: self.store.clone(), + vm_table, + } + } +} + impl<'a> Exportable<'a> for Table { fn to_export(&self) -> Export { self.vm_table.clone().into() diff --git a/lib/api/src/native.rs b/lib/api/src/native.rs index 11421aa042d..a350c6f090b 100644 --- a/lib/api/src/native.rs +++ b/lib/api/src/native.rs @@ -18,7 +18,6 @@ use wasmer_vm::{VMDynamicFunctionContext, VMFunctionBody, VMFunctionEnvironment, /// A WebAssembly function that can be called natively /// (using the Native ABI). -#[derive(Clone)] pub struct NativeFunc { store: Store, exported: ExportFunction, @@ -88,6 +87,19 @@ where } }*/ +impl Clone for NativeFunc { + fn clone(&self) -> Self { + let mut exported = self.exported.clone(); + exported.vm_function.upgrade_instance_ref().unwrap(); + + Self { + store: self.store.clone(), + exported, + _phantom: PhantomData, + } + } +} + impl From<&NativeFunc> for ExportFunction where Args: WasmTypeList, diff --git a/lib/vm/src/export.rs b/lib/vm/src/export.rs index fb9d1fabbd5..f9567962109 100644 --- a/lib/vm/src/export.rs +++ b/lib/vm/src/export.rs @@ -27,7 +27,7 @@ pub enum VMExtern { } /// A function export value. -#[derive(Debug, PartialEq, MemoryUsage)] +#[derive(Clone, Debug, PartialEq, MemoryUsage)] pub struct VMFunction { /// The address of the native-code function. pub address: *const VMFunctionBody, @@ -55,16 +55,14 @@ pub struct VMFunction { pub instance_ref: Option, } -impl Clone for VMFunction { - fn clone(&self) -> Self { - Self { - address: self.address.clone(), - vmctx: self.vmctx.clone(), - signature: self.signature.clone(), - kind: self.kind.clone(), - call_trampoline: self.call_trampoline.clone(), - instance_ref: self.instance_ref.as_ref().map(|v| v.upgrade().unwrap()), +impl VMFunction { + /// Converts the stored instance ref into a strong `InstanceRef` if it is weak. + /// Returns None if it cannot be upgraded. + pub fn upgrade_instance_ref(&mut self) -> Option<()> { + if let Some(ref mut ir) = self.instance_ref { + *ir = ir.upgrade()?; } + Some(()) } } @@ -83,7 +81,7 @@ impl From for VMExtern { } /// A table export value. -#[derive(Debug, MemoryUsage)] +#[derive(Clone, Debug, MemoryUsage)] pub struct VMTable { /// Pointer to the containing `Table`. pub from: Arc, @@ -93,15 +91,6 @@ pub struct VMTable { pub instance_ref: Option, } -impl Clone for VMTable { - fn clone(&self) -> Self { - Self { - from: self.from.clone(), - instance_ref: self.instance_ref.as_ref().map(|v| v.upgrade().unwrap()), - } - } -} - /// # Safety /// This is correct because there is no non-threadsafe logic directly in this type; /// correct use of the raw table from multiple threads via `definition` requires `unsafe` @@ -129,6 +118,15 @@ impl VMTable { pub fn same(&self, other: &Self) -> bool { Arc::ptr_eq(&self.from, &other.from) } + + /// Converts the stored instance ref into a strong `InstanceRef` if it is weak. + /// Returns None if it cannot be upgraded. + pub fn upgrade_instance_ref(&mut self) -> Option<()> { + if let Some(ref mut ir) = self.instance_ref { + *ir = ir.upgrade()?; + } + Some(()) + } } impl From for VMExtern { @@ -138,7 +136,7 @@ impl From for VMExtern { } /// A memory export value. -#[derive(Debug, MemoryUsage)] +#[derive(Debug, Clone, MemoryUsage)] pub struct VMMemory { /// Pointer to the containing `Memory`. pub from: Arc, @@ -148,15 +146,6 @@ pub struct VMMemory { pub instance_ref: Option, } -impl Clone for VMMemory { - fn clone(&self) -> Self { - Self { - from: self.from.clone(), - instance_ref: self.instance_ref.as_ref().map(|v| v.upgrade().unwrap()), - } - } -} - /// # Safety /// This is correct because there is no non-threadsafe logic directly in this type; /// correct use of the raw memory from multiple threads via `definition` requires `unsafe` @@ -184,6 +173,15 @@ impl VMMemory { pub fn same(&self, other: &Self) -> bool { Arc::ptr_eq(&self.from, &other.from) } + + /// Converts the stored instance ref into a strong `InstanceRef` if it is weak. + /// Returns None if it cannot be upgraded. + pub fn upgrade_instance_ref(&mut self) -> Option<()> { + if let Some(ref mut ir) = self.instance_ref { + *ir = ir.upgrade()?; + } + Some(()) + } } impl From for VMExtern { @@ -193,7 +191,7 @@ impl From for VMExtern { } /// A global export value. -#[derive(Debug, MemoryUsage)] +#[derive(Debug, Clone, MemoryUsage)] pub struct VMGlobal { /// The global declaration, used for compatibility checking. pub from: Arc, @@ -203,15 +201,6 @@ pub struct VMGlobal { pub instance_ref: Option, } -impl Clone for VMGlobal { - fn clone(&self) -> Self { - Self { - from: self.from.clone(), - instance_ref: self.instance_ref.as_ref().map(|v| v.upgrade().unwrap()), - } - } -} - /// # Safety /// This is correct because there is no non-threadsafe logic directly in this type; /// correct use of the raw global from multiple threads via `definition` requires `unsafe` @@ -229,6 +218,15 @@ impl VMGlobal { pub fn same(&self, other: &Self) -> bool { Arc::ptr_eq(&self.from, &other.from) } + + /// Converts the stored instance ref into a strong `InstanceRef` if it is weak. + /// Returns None if it cannot be upgraded. + pub fn upgrade_instance_ref(&mut self) -> Option<()> { + if let Some(ref mut ir) = self.instance_ref { + *ir = ir.upgrade()?; + } + Some(()) + } } impl From for VMExtern {