From 4494bea70ae29ca845e423e40e6756b11e6be713 Mon Sep 17 00:00:00 2001 From: Mark McCaskey Date: Wed, 16 Dec 2020 08:37:47 -0800 Subject: [PATCH 1/4] Clean up host env code, add more `Send`, `Sync` bounds --- lib/api/src/externals/function.rs | 187 +++++++++++------------------- lib/vm/src/vmcontext.rs | 11 +- 2 files changed, 79 insertions(+), 119 deletions(-) diff --git a/lib/api/src/externals/function.rs b/lib/api/src/externals/function.rs index f961ceeda32..475ffde6e37 100644 --- a/lib/api/src/externals/function.rs +++ b/lib/api/src/externals/function.rs @@ -67,6 +67,49 @@ pub struct Function { pub(crate) exported: ExportFunction, } +fn build_export_function_metadata( + env: Env, + import_init_function_ptr: for<'a> fn( + &'a mut Env, + &'a crate::Instance, + ) -> Result<(), crate::HostEnvInitError>, +) -> (*mut std::ffi::c_void, ExportFunctionMetadata) +where + Env: Clone + Sized + 'static + Send + Sync, +{ + let import_init_function_ptr = Some(unsafe { + std::mem::transmute:: Result<(), _>, fn(_, _) -> Result<(), _>>( + import_init_function_ptr, + ) + }); + let host_env_clone_fn: fn(*mut std::ffi::c_void) -> *mut std::ffi::c_void = |ptr| { + let duped_env: Env = unsafe { + let ptr: *mut Env = ptr as _; + let item: &Env = &*ptr; + item.clone() + }; + Box::into_raw(Box::new(duped_env)) as _ + }; + let host_env_drop_fn: fn(*mut std::ffi::c_void) = |ptr: *mut std::ffi::c_void| { + unsafe { Box::from_raw(ptr as *mut Env) }; + }; + let env = Box::into_raw(Box::new(env)) as _; + + // # Safety + // - All these functions work on all threads + // - The host env is `Send`. + let metadata = unsafe { + ExportFunctionMetadata::new( + env, + import_init_function_ptr, + host_env_clone_fn, + host_env_drop_fn, + ) + }; + + (env, metadata) +} + impl Function { /// Creates a new host `Function` (dynamic) with the provided signature. /// @@ -104,7 +147,7 @@ impl Function { pub fn new(store: &Store, ty: FT, func: F) -> Self where FT: Into, - F: Fn(&[Val]) -> Result, RuntimeError> + 'static, + F: Fn(&[Val]) -> Result, RuntimeError> + 'static + Send + Sync, { let ty: FunctionType = ty.into(); let dynamic_ctx: VMDynamicFunctionContext = @@ -209,7 +252,7 @@ impl Function { pub fn new_with_env(store: &Store, ty: FT, env: Env, func: F) -> Self where FT: Into, - F: Fn(&Env, &[Val]) -> Result, RuntimeError> + 'static, + F: Fn(&Env, &[Val]) -> Result, RuntimeError> + 'static + Send + Sync, Env: Sized + WasmerEnv + 'static, { let ty: FunctionType = ty.into(); @@ -220,58 +263,27 @@ impl Function { function_type: ty.clone(), }); + let import_init_function_ptr: for<'a> fn(&'a mut _, &'a _) -> Result<(), _> = + |env: &mut VMDynamicFunctionContext>, + instance: &crate::Instance| { + Env::init_with_instance(&mut *env.ctx.env, instance) + }; + + let (host_env, metadata) = build_export_function_metadata::< + VMDynamicFunctionContext>, + >(dynamic_ctx, import_init_function_ptr); + // We don't yet have the address with the Wasm ABI signature. // The engine linker will replace the address with one pointing to a // generated dynamic trampoline. let address = std::ptr::null() as *const VMFunctionBody; - let host_env = Box::into_raw(Box::new(dynamic_ctx)) as *mut _; let vmctx = VMFunctionEnvironment { host_env }; - let import_init_function_ptr: fn(_, _) -> Result<(), _> = - |ptr: *mut std::ffi::c_void, instance: *const std::ffi::c_void| { - let ptr = ptr as *mut VMDynamicFunctionContext>; - unsafe { - let env = &mut *ptr; - let env: &mut Env = &mut *env.ctx.env; - let instance = &*(instance as *const crate::Instance); - Env::init_with_instance(env, instance) - } - }; - let import_init_function_ptr = Some(unsafe { - std::mem::transmute:: Result<(), _>, fn(_, _) -> Result<(), _>>( - import_init_function_ptr, - ) - }); - let host_env_clone_fn: fn(*mut std::ffi::c_void) -> *mut std::ffi::c_void = |ptr| { - let duped_env: VMDynamicFunctionContext> = unsafe { - let ptr: *mut VMDynamicFunctionContext> = ptr as _; - let item: &VMDynamicFunctionContext> = &*ptr; - item.clone() - }; - Box::into_raw(Box::new(duped_env)) as _ - }; - let host_env_drop_fn: fn(*mut std::ffi::c_void) = |ptr: *mut std::ffi::c_void| { - unsafe { - Box::from_raw(ptr as *mut VMDynamicFunctionContext>) - }; - }; Self { store: store.clone(), definition: FunctionDefinition::Host(HostFunctionDefinition { has_env: true }), exported: ExportFunction { - metadata: Some(Arc::new( - // # Safety - // - All these functions work on all threads - // - The host env is `Send`. - unsafe { - ExportFunctionMetadata::new( - host_env, - import_init_function_ptr, - host_env_clone_fn, - host_env_drop_fn, - ) - }, - )), + metadata: Some(Arc::new(metadata)), vm_function: VMExportFunction { address, kind: VMFunctionKind::Dynamic, @@ -374,50 +386,17 @@ impl Function { let function = inner::Function::::new(func); let address = function.address(); - // TODO: We need to refactor the Function context. - // Right now is structured as it's always a `VMContext`. However, only - // Wasm-defined functions have a `VMContext`. - // In the case of Host-defined functions `VMContext` is whatever environment - // the user want to attach to the function. - let host_env = Box::into_raw(Box::new(env)) as *mut _; - let vmctx = VMFunctionEnvironment { host_env }; - let host_env_clone_fn: fn(*mut std::ffi::c_void) -> *mut std::ffi::c_void = |ptr| { - let duped_env = unsafe { - let ptr: *mut Env = ptr as _; - let item: &Env = &*ptr; - item.clone() - }; - Box::into_raw(Box::new(duped_env)) as _ - }; - let host_env_drop_fn: fn(*mut std::ffi::c_void) = |ptr: *mut std::ffi::c_void| { - unsafe { Box::from_raw(ptr as *mut Env) }; - }; + let (host_env, metadata) = + build_export_function_metadata::(env, Env::init_with_instance); - // TODO: look into removing transmute by changing API type signatures - let import_init_function_ptr = Some(unsafe { - std::mem::transmute:: Result<(), _>, fn(_, _) -> Result<(), _>>( - Env::init_with_instance, - ) - }); + let vmctx = VMFunctionEnvironment { host_env }; let signature = function.ty(); Self { store: store.clone(), definition: FunctionDefinition::Host(HostFunctionDefinition { has_env: true }), exported: ExportFunction { - metadata: Some(Arc::new( - // # Safety - // - All these functions work on all threads - // - The host env is `Send`. - unsafe { - ExportFunctionMetadata::new( - host_env, - import_init_function_ptr, - host_env_clone_fn, - host_env_drop_fn, - ) - }, - )), + metadata: Some(Arc::new(metadata)), vm_function: VMExportFunction { address, kind: VMFunctionKind::Static, @@ -455,43 +434,17 @@ impl Function { let function = inner::Function::::new(func); let address = function.address(); - let box_env = Box::new(env); - let host_env = Box::into_raw(box_env) as *mut _; - let vmctx = VMFunctionEnvironment { host_env }; - let host_env_clone_fn: fn(*mut std::ffi::c_void) -> *mut std::ffi::c_void = |ptr| { - let duped_env: Env = { - let ptr: *mut Env = ptr as _; - let item: &Env = &*ptr; - item.clone() - }; - Box::into_raw(Box::new(duped_env)) as _ - }; - let host_env_drop_fn: fn(*mut std::ffi::c_void) = |ptr: *mut std::ffi::c_void| { - Box::from_raw(ptr as *mut Env); - }; + let (host_env, metadata) = + build_export_function_metadata::(env, Env::init_with_instance); + let vmctx = VMFunctionEnvironment { host_env }; let signature = function.ty(); - // TODO: look into removing transmute by changing API type signatures - let import_init_function_ptr = Some(std::mem::transmute::< - fn(_, _) -> Result<(), _>, - fn(_, _) -> Result<(), _>, - >(Env::init_with_instance)); Self { store: store.clone(), definition: FunctionDefinition::Host(HostFunctionDefinition { has_env: true }), exported: ExportFunction { - metadata: Some(Arc::new( - // # Safety - // - All these functions work on all threads - // - The host env is `Send`. - ExportFunctionMetadata::new( - host_env, - import_init_function_ptr, - host_env_clone_fn, - host_env_drop_fn, - ), - )), + metadata: Some(Arc::new(metadata)), vm_function: VMExportFunction { address, kind: VMFunctionKind::Static, @@ -855,7 +808,7 @@ impl fmt::Debug for Function { } /// This trait is one that all dynamic functions must fulfill. -pub(crate) trait VMDynamicFunction { +pub(crate) trait VMDynamicFunction: Send + Sync { fn call(&self, args: &[Val]) -> Result, RuntimeError>; fn function_type(&self) -> &FunctionType; } @@ -863,7 +816,7 @@ pub(crate) trait VMDynamicFunction { #[derive(Clone)] pub(crate) struct DynamicFunctionWithoutEnv { #[allow(clippy::type_complexity)] - func: Arc Result, RuntimeError> + 'static>, + func: Arc Result, RuntimeError> + 'static + Send + Sync>, function_type: FunctionType, } @@ -878,15 +831,15 @@ impl VMDynamicFunction for DynamicFunctionWithoutEnv { pub(crate) struct DynamicFunctionWithEnv where - Env: Sized + 'static, + Env: Sized + 'static + Send + Sync, { function_type: FunctionType, #[allow(clippy::type_complexity)] - func: Arc Result, RuntimeError> + 'static>, + func: Arc Result, RuntimeError> + 'static + Send + Sync>, env: Box, } -impl Clone for DynamicFunctionWithEnv { +impl Clone for DynamicFunctionWithEnv { fn clone(&self) -> Self { Self { env: self.env.clone(), @@ -898,7 +851,7 @@ impl Clone for DynamicFunctionWithEnv { impl VMDynamicFunction for DynamicFunctionWithEnv where - Env: Sized + 'static, + Env: Sized + 'static + Send + Sync, { fn call(&self, args: &[Val]) -> Result, RuntimeError> { (*self.func)(&*self.env, &args) diff --git a/lib/vm/src/vmcontext.rs b/lib/vm/src/vmcontext.rs index 4f478485423..4f4b3abb017 100644 --- a/lib/vm/src/vmcontext.rs +++ b/lib/vm/src/vmcontext.rs @@ -95,7 +95,7 @@ mod test_vmfunction_import { /// containing the relevant context for running the function indicated /// in `address`. #[repr(C)] -pub struct VMDynamicFunctionContext { +pub struct VMDynamicFunctionContext { /// The address of the inner dynamic function. /// /// Note: The function must be on the form of @@ -106,7 +106,14 @@ pub struct VMDynamicFunctionContext { pub ctx: T, } -impl Clone for VMDynamicFunctionContext { +// The `ctx` itself must be `Send`, `address` can be passed between +// threads because all usage is `unsafe` and synchronized. +unsafe impl Send for VMDynamicFunctionContext {} +// The `ctx` itself must be `Sync`, `address` can be shared between +// threads because all usage is `unsafe` and synchronized. +unsafe impl Sync for VMDynamicFunctionContext {} + +impl Clone for VMDynamicFunctionContext { fn clone(&self) -> Self { Self { address: self.address, From fc9a8f87df09ecdac0550b5237c4c27281a10a51 Mon Sep 17 00:00:00 2001 From: Mark McCaskey Date: Fri, 18 Dec 2020 07:54:30 -0800 Subject: [PATCH 2/4] Address feedback, clean up code --- lib/api/src/externals/function.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/lib/api/src/externals/function.rs b/lib/api/src/externals/function.rs index 475ffde6e37..1faa69273f3 100644 --- a/lib/api/src/externals/function.rs +++ b/lib/api/src/externals/function.rs @@ -83,15 +83,15 @@ where ) }); let host_env_clone_fn: fn(*mut std::ffi::c_void) -> *mut std::ffi::c_void = |ptr| { - let duped_env: Env = unsafe { - let ptr: *mut Env = ptr as _; - let item: &Env = &*ptr; - item.clone() + let env_ref: &Env = unsafe { + ptr.cast::() + .as_ref() + .expect("`ptr` to the environment is null when cloning it") }; - Box::into_raw(Box::new(duped_env)) as _ + Box::into_raw(Box::new(env_ref.clone())) as _ }; - let host_env_drop_fn: fn(*mut std::ffi::c_void) = |ptr: *mut std::ffi::c_void| { - unsafe { Box::from_raw(ptr as *mut Env) }; + let host_env_drop_fn: fn(*mut std::ffi::c_void) = |ptr| { + unsafe { Box::from_raw(ptr.cast::()) }; }; let env = Box::into_raw(Box::new(env)) as _; From 283acc55b584e30bfacd048483198f6760dc6609 Mon Sep 17 00:00:00 2001 From: Mark McCaskey Date: Fri, 18 Dec 2020 07:58:11 -0800 Subject: [PATCH 3/4] Add 1944 to changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index fda945847cf..8af2ccda490 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ * [#1894](https://github.com/wasmerio/wasmer/pull/1894) Added exports `wasmer::{CraneliftOptLevel, LLVMOptLevel}` to allow using `Cranelift::opt_level` and `LLVM::opt_level` directly via the `wasmer` crate ### Changed +* [#1944](https://github.com/wasmerio/wasmer/pull/1944) Require `WasmerEnv` to be `Send + Sync` even in dynamic functions. ### Fixed From 4f08f3d66361834fdc9c74e289cb050c56e63b00 Mon Sep 17 00:00:00 2001 From: Mark McCaskey Date: Fri, 18 Dec 2020 09:48:07 -0800 Subject: [PATCH 4/4] Update deprecated API to new restrictions on dynamic envs --- lib/deprecated/runtime-core/Cargo.lock | 26 +++++++++++------------ lib/deprecated/runtime-core/src/module.rs | 17 +++++++++------ lib/deprecated/runtime/Cargo.lock | 26 +++++++++++------------ 3 files changed, 37 insertions(+), 32 deletions(-) diff --git a/lib/deprecated/runtime-core/Cargo.lock b/lib/deprecated/runtime-core/Cargo.lock index c22464150c3..18ff6d81526 100644 --- a/lib/deprecated/runtime-core/Cargo.lock +++ b/lib/deprecated/runtime-core/Cargo.lock @@ -1158,7 +1158,7 @@ checksum = "cccddf32554fecc6acb585f82a32a72e28b48f8c4c1883ddfeeeaa96f7d8e519" [[package]] name = "wasmer" -version = "1.0.0-beta1" +version = "1.0.0-beta2" dependencies = [ "cfg-if 0.1.10", "indexmap", @@ -1181,7 +1181,7 @@ dependencies = [ [[package]] name = "wasmer-cache" -version = "1.0.0-beta1" +version = "1.0.0-beta2" dependencies = [ "blake3", "hex", @@ -1191,7 +1191,7 @@ dependencies = [ [[package]] name = "wasmer-compiler" -version = "1.0.0-beta1" +version = "1.0.0-beta2" dependencies = [ "enumset", "raw-cpuid", @@ -1207,7 +1207,7 @@ dependencies = [ [[package]] name = "wasmer-compiler-cranelift" -version = "1.0.0-beta1" +version = "1.0.0-beta2" dependencies = [ "cranelift-codegen", "cranelift-frontend", @@ -1224,7 +1224,7 @@ dependencies = [ [[package]] name = "wasmer-compiler-llvm" -version = "1.0.0-beta1" +version = "1.0.0-beta2" dependencies = [ "byteorder", "cc", @@ -1246,7 +1246,7 @@ dependencies = [ [[package]] name = "wasmer-compiler-singlepass" -version = "1.0.0-beta1" +version = "1.0.0-beta2" dependencies = [ "byteorder", "dynasm", @@ -1263,7 +1263,7 @@ dependencies = [ [[package]] name = "wasmer-derive" -version = "1.0.0-beta1" +version = "1.0.0-beta2" dependencies = [ "proc-macro-error", "proc-macro2", @@ -1273,7 +1273,7 @@ dependencies = [ [[package]] name = "wasmer-engine" -version = "1.0.0-beta1" +version = "1.0.0-beta2" dependencies = [ "backtrace", "bincode", @@ -1292,7 +1292,7 @@ dependencies = [ [[package]] name = "wasmer-engine-jit" -version = "1.0.0-beta1" +version = "1.0.0-beta2" dependencies = [ "bincode", "cfg-if 0.1.10", @@ -1308,7 +1308,7 @@ dependencies = [ [[package]] name = "wasmer-engine-native" -version = "1.0.0-beta1" +version = "1.0.0-beta2" dependencies = [ "bincode", "cfg-if 0.1.10", @@ -1327,7 +1327,7 @@ dependencies = [ [[package]] name = "wasmer-object" -version = "1.0.0-beta1" +version = "1.0.0-beta2" dependencies = [ "object 0.22.0", "thiserror", @@ -1355,7 +1355,7 @@ dependencies = [ [[package]] name = "wasmer-types" -version = "1.0.0-beta1" +version = "1.0.0-beta2" dependencies = [ "cranelift-entity", "serde", @@ -1364,7 +1364,7 @@ dependencies = [ [[package]] name = "wasmer-vm" -version = "1.0.0-beta1" +version = "1.0.0-beta2" dependencies = [ "backtrace", "cc", diff --git a/lib/deprecated/runtime-core/src/module.rs b/lib/deprecated/runtime-core/src/module.rs index a122d0f8e44..7bcd9b6d5d5 100644 --- a/lib/deprecated/runtime-core/src/module.rs +++ b/lib/deprecated/runtime-core/src/module.rs @@ -10,10 +10,10 @@ use crate::{ }; use new::wasmer::Export; use std::{ - cell::RefCell, collections::HashMap, convert::{AsRef, Infallible}, ptr, + sync::{Arc, Mutex}, }; pub use new::wasmer_types::{DataInitializer, ExportIndex, TableInitializer}; @@ -126,20 +126,22 @@ impl Module { // is the same! struct VMDynamicFunctionWithEnv where - Env: Sized + 'static, + Env: Sized + 'static + Send + Sync, { #[allow(unused)] function_type: FuncSig, #[allow(unused)] - func: Box< + func: Arc< dyn Fn( &mut Env, &[Value], ) -> Result, RuntimeError> - + 'static, + + 'static + + Send + + Sync, >, - env: RefCell, + env: Box>, } // Get back the `vmctx` as it is @@ -154,7 +156,10 @@ impl Module { }; // Replace the environment by ours. - vmctx.ctx.env.borrow_mut().vmctx = pre_instance.vmctx(); + { + let mut guard = vmctx.ctx.env.lock().unwrap(); + guard.vmctx = pre_instance.vmctx(); + } // … without anyone noticing… function.vm_function.vmctx.host_env = Box::into_raw(vmctx) as _; diff --git a/lib/deprecated/runtime/Cargo.lock b/lib/deprecated/runtime/Cargo.lock index c41e119903d..068083724ec 100644 --- a/lib/deprecated/runtime/Cargo.lock +++ b/lib/deprecated/runtime/Cargo.lock @@ -1158,7 +1158,7 @@ checksum = "cccddf32554fecc6acb585f82a32a72e28b48f8c4c1883ddfeeeaa96f7d8e519" [[package]] name = "wasmer" -version = "1.0.0-beta1" +version = "1.0.0-beta2" dependencies = [ "cfg-if 0.1.10", "indexmap", @@ -1181,7 +1181,7 @@ dependencies = [ [[package]] name = "wasmer-cache" -version = "1.0.0-beta1" +version = "1.0.0-beta2" dependencies = [ "blake3", "hex", @@ -1191,7 +1191,7 @@ dependencies = [ [[package]] name = "wasmer-compiler" -version = "1.0.0-beta1" +version = "1.0.0-beta2" dependencies = [ "enumset", "raw-cpuid", @@ -1207,7 +1207,7 @@ dependencies = [ [[package]] name = "wasmer-compiler-cranelift" -version = "1.0.0-beta1" +version = "1.0.0-beta2" dependencies = [ "cranelift-codegen", "cranelift-frontend", @@ -1224,7 +1224,7 @@ dependencies = [ [[package]] name = "wasmer-compiler-llvm" -version = "1.0.0-beta1" +version = "1.0.0-beta2" dependencies = [ "byteorder", "cc", @@ -1246,7 +1246,7 @@ dependencies = [ [[package]] name = "wasmer-compiler-singlepass" -version = "1.0.0-beta1" +version = "1.0.0-beta2" dependencies = [ "byteorder", "dynasm", @@ -1263,7 +1263,7 @@ dependencies = [ [[package]] name = "wasmer-derive" -version = "1.0.0-beta1" +version = "1.0.0-beta2" dependencies = [ "proc-macro-error", "proc-macro2", @@ -1273,7 +1273,7 @@ dependencies = [ [[package]] name = "wasmer-engine" -version = "1.0.0-beta1" +version = "1.0.0-beta2" dependencies = [ "backtrace", "bincode", @@ -1292,7 +1292,7 @@ dependencies = [ [[package]] name = "wasmer-engine-jit" -version = "1.0.0-beta1" +version = "1.0.0-beta2" dependencies = [ "bincode", "cfg-if 0.1.10", @@ -1308,7 +1308,7 @@ dependencies = [ [[package]] name = "wasmer-engine-native" -version = "1.0.0-beta1" +version = "1.0.0-beta2" dependencies = [ "bincode", "cfg-if 0.1.10", @@ -1327,7 +1327,7 @@ dependencies = [ [[package]] name = "wasmer-object" -version = "1.0.0-beta1" +version = "1.0.0-beta2" dependencies = [ "object 0.22.0", "thiserror", @@ -1362,7 +1362,7 @@ dependencies = [ [[package]] name = "wasmer-types" -version = "1.0.0-beta1" +version = "1.0.0-beta2" dependencies = [ "cranelift-entity", "serde", @@ -1371,7 +1371,7 @@ dependencies = [ [[package]] name = "wasmer-vm" -version = "1.0.0-beta1" +version = "1.0.0-beta2" dependencies = [ "backtrace", "cc",