From 17a0e78cef5e3ef043f431784968fd78935047e8 Mon Sep 17 00:00:00 2001 From: Mark McCaskey Date: Tue, 17 Sep 2019 10:37:37 -0700 Subject: [PATCH 1/6] Implement Send for Instance --- lib/runtime-core/src/backing.rs | 20 ++++----- lib/runtime-core/src/export.rs | 4 ++ lib/runtime-core/src/global.rs | 8 ++-- lib/runtime-core/src/import.rs | 71 +++++++++++++++++++++++------- lib/runtime-core/src/typed_func.rs | 2 + 5 files changed, 72 insertions(+), 33 deletions(-) diff --git a/lib/runtime-core/src/backing.rs b/lib/runtime-core/src/backing.rs index 814dc56e79c..1bb3bec4dd8 100644 --- a/lib/runtime-core/src/backing.rs +++ b/lib/runtime-core/src/backing.rs @@ -536,9 +536,8 @@ fn import_functions( let namespace = module.info.namespace_table.get(*namespace_index); let name = module.info.name_table.get(*name_index); - let import = imports - .get_namespace(namespace) - .and_then(|namespace| namespace.get_export(name)); + let import = + imports.maybe_with_namespace(namespace, |namespace| namespace.get_export(name)); match import { Some(Export::Function { func, @@ -624,9 +623,8 @@ fn import_memories( let namespace = module.info.namespace_table.get(*namespace_index); let name = module.info.name_table.get(*name_index); - let memory_import = imports - .get_namespace(&namespace) - .and_then(|namespace| namespace.get_export(&name)); + let memory_import = + imports.maybe_with_namespace(namespace, |namespace| namespace.get_export(name)); match memory_import { Some(Export::Memory(memory)) => { if expected_memory_desc.fits_in_imported(memory.descriptor()) { @@ -696,9 +694,8 @@ fn import_tables( let namespace = module.info.namespace_table.get(*namespace_index); let name = module.info.name_table.get(*name_index); - let table_import = imports - .get_namespace(&namespace) - .and_then(|namespace| namespace.get_export(&name)); + let table_import = + imports.maybe_with_namespace(namespace, |namespace| namespace.get_export(name)); match table_import { Some(Export::Table(mut table)) => { if expected_table_desc.fits_in_imported(table.descriptor()) { @@ -767,9 +764,8 @@ fn import_globals( { let namespace = module.info.namespace_table.get(*namespace_index); let name = module.info.name_table.get(*name_index); - let import = imports - .get_namespace(namespace) - .and_then(|namespace| namespace.get_export(name)); + let import = + imports.maybe_with_namespace(namespace, |namespace| namespace.get_export(name)); match import { Some(Export::Global(mut global)) => { if global.descriptor() == *imported_global_desc { diff --git a/lib/runtime-core/src/export.rs b/lib/runtime-core/src/export.rs index 52ea9cf618e..96560cb6a0e 100644 --- a/lib/runtime-core/src/export.rs +++ b/lib/runtime-core/src/export.rs @@ -11,6 +11,8 @@ pub enum Context { Internal, } +unsafe impl Send for Context {} + #[derive(Debug, Clone)] pub enum Export { Function { @@ -26,6 +28,8 @@ pub enum Export { #[derive(Debug, Clone)] pub struct FuncPointer(*const vm::Func); +unsafe impl Send for FuncPointer {} + impl FuncPointer { /// This needs to be unsafe because there is /// no way to check whether the passed function diff --git a/lib/runtime-core/src/global.rs b/lib/runtime-core/src/global.rs index 8d421253112..beadd10ef4d 100644 --- a/lib/runtime-core/src/global.rs +++ b/lib/runtime-core/src/global.rs @@ -4,11 +4,11 @@ use crate::{ types::{GlobalDescriptor, Type, Value}, vm, }; -use std::{cell::RefCell, fmt, rc::Rc}; +use std::{cell::RefCell, fmt, sync::Arc}; pub struct Global { desc: GlobalDescriptor, - storage: Rc>, + storage: Arc>, } impl Global { @@ -56,7 +56,7 @@ impl Global { Self { desc, - storage: Rc::new(RefCell::new(local_global)), + storage: Arc::new(RefCell::new(local_global)), } } @@ -120,7 +120,7 @@ impl Clone for Global { fn clone(&self) -> Self { Self { desc: self.desc, - storage: Rc::clone(&self.storage), + storage: Arc::clone(&self.storage), } } } diff --git a/lib/runtime-core/src/import.rs b/lib/runtime-core/src/import.rs index fc1da43b730..c7e15c90d70 100644 --- a/lib/runtime-core/src/import.rs +++ b/lib/runtime-core/src/import.rs @@ -2,9 +2,9 @@ use crate::export::Export; use std::collections::VecDeque; use std::collections::{hash_map::Entry, HashMap}; use std::{ - cell::{Ref, RefCell}, + borrow::{Borrow, BorrowMut}, ffi::c_void, - rc::Rc, + sync::{Arc, Mutex}, }; pub trait LikeNamespace { @@ -45,8 +45,8 @@ impl IsExport for Export { /// } /// ``` pub struct ImportObject { - map: Rc>>>, - pub(crate) state_creator: Option (*mut c_void, fn(*mut c_void))>>, + map: Arc>>>, + pub(crate) state_creator: Option (*mut c_void, fn(*mut c_void)) + Send>>, pub allow_missing_functions: bool, } @@ -54,7 +54,7 @@ impl ImportObject { /// Create a new `ImportObject`. pub fn new() -> Self { Self { - map: Rc::new(RefCell::new(HashMap::new())), + map: Arc::new(Mutex::new(HashMap::new())), state_creator: None, allow_missing_functions: false, } @@ -62,11 +62,11 @@ impl ImportObject { pub fn new_with_data(state_creator: F) -> Self where - F: Fn() -> (*mut c_void, fn(*mut c_void)) + 'static, + F: Fn() -> (*mut c_void, fn(*mut c_void)) + 'static + Send, { Self { - map: Rc::new(RefCell::new(HashMap::new())), - state_creator: Some(Rc::new(state_creator)), + map: Arc::new(Mutex::new(HashMap::new())), + state_creator: Some(Arc::new(state_creator)), allow_missing_functions: false, } } @@ -92,9 +92,10 @@ impl ImportObject { pub fn register(&mut self, name: S, namespace: N) -> Option> where S: Into, - N: LikeNamespace + 'static, + N: LikeNamespace + Send + 'static, { - let mut map = self.map.borrow_mut(); + let mut guard = self.map.lock().unwrap(); + let map = guard.borrow_mut(); match map.entry(name.into()) { Entry::Vacant(empty) => { @@ -105,19 +106,50 @@ impl ImportObject { } } - pub fn get_namespace(&self, namespace: &str) -> Option> { - let map_ref = self.map.borrow(); - + /*pub fn get_namespace( + &self, + namespace: &str, + ) -> Option< + MutexGuardRef>, &(dyn LikeNamespace + Send)>, + > { + MutexGuardRef::new(self.map.lock().unwrap()) + .try_map(|mg| mg.get(namespace).map(|ns| &ns.as_ref()).ok_or(())) + .ok() + }*/ + + /// Apply a function on the namespace if it exists + /// If your function can fail, consider using `maybe_with_namespace` + pub fn with_namespace(&self, namespace: &str, f: Func) -> Option + where + Func: FnOnce(&(dyn LikeNamespace + Send)) -> InnerRet, + InnerRet: Sized, + { + let guard = self.map.lock().unwrap(); + let map_ref = guard.borrow(); if map_ref.contains_key(namespace) { - Some(Ref::map(map_ref, |map| &*map[namespace])) + Some(f(map_ref[namespace].as_ref())) } else { None } } + /// The same as `with_namespace` but takes a function that may fail + pub fn maybe_with_namespace(&self, namespace: &str, f: Func) -> Option + where + Func: FnOnce(&(dyn LikeNamespace + Send)) -> Option, + InnerRet: Sized, + { + let guard = self.map.lock().unwrap(); + let map_ref = guard.borrow(); + map_ref + .get(namespace) + .map(|ns| ns.as_ref()) + .and_then(|ns| f(ns)) + } + pub fn clone_ref(&self) -> Self { Self { - map: Rc::clone(&self.map), + map: Arc::clone(&self.map), state_creator: self.state_creator.clone(), allow_missing_functions: false, } @@ -125,7 +157,9 @@ impl ImportObject { fn get_objects(&self) -> VecDeque<(String, String, Export)> { let mut out = VecDeque::new(); - for (name, ns) in self.map.borrow().iter() { + let guard = self.map.lock().unwrap(); + let map = guard.borrow(); + for (name, ns) in map.iter() { for (id, exp) in ns.get_exports() { out.push_back((name.clone(), id, exp)); } @@ -158,7 +192,8 @@ impl IntoIterator for ImportObject { impl Extend<(String, String, Export)> for ImportObject { fn extend>(&mut self, iter: T) { - let mut map = self.map.borrow_mut(); + let mut guard = self.map.lock().unwrap(); + let map = guard.borrow_mut(); for (ns, id, exp) in iter.into_iter() { if let Some(like_ns) = map.get_mut(&ns) { like_ns.maybe_insert(&id, exp); @@ -175,6 +210,8 @@ pub struct Namespace { map: HashMap>, } +unsafe impl Send for Namespace {} + impl Namespace { pub fn new() -> Self { Self { diff --git a/lib/runtime-core/src/typed_func.rs b/lib/runtime-core/src/typed_func.rs index 91da26cbe37..789c858cb96 100644 --- a/lib/runtime-core/src/typed_func.rs +++ b/lib/runtime-core/src/typed_func.rs @@ -165,6 +165,8 @@ pub struct Func<'a, Args = (), Rets = (), Inner: Kind = Wasm> { _phantom: PhantomData<(&'a (), Args, Rets)>, } +unsafe impl<'a, Args, Rets> Send for Func<'a, Args, Rets> {} + impl<'a, Args, Rets> Func<'a, Args, Rets, Wasm> where Args: WasmTypeList, From 9e9343878d8ad9246218231623c2a7348daf61e2 Mon Sep 17 00:00:00 2001 From: Mark McCaskey Date: Tue, 17 Sep 2019 11:45:13 -0700 Subject: [PATCH 2/6] Implement Send for everything except Memory --- lib/runtime-core/src/export.rs | 2 ++ lib/runtime-core/src/global.rs | 19 +++++++++++++------ lib/runtime-core/src/import.rs | 8 +++----- lib/runtime-core/src/table/mod.rs | 26 +++++++++++++++++--------- lib/runtime-core/src/typed_func.rs | 3 ++- lib/runtime-core/src/vm.rs | 4 ++++ 6 files changed, 41 insertions(+), 21 deletions(-) diff --git a/lib/runtime-core/src/export.rs b/lib/runtime-core/src/export.rs index 96560cb6a0e..67cd1e5fff1 100644 --- a/lib/runtime-core/src/export.rs +++ b/lib/runtime-core/src/export.rs @@ -25,6 +25,8 @@ pub enum Export { Global(Global), } +unsafe impl Send for Export {} + #[derive(Debug, Clone)] pub struct FuncPointer(*const vm::Func); diff --git a/lib/runtime-core/src/global.rs b/lib/runtime-core/src/global.rs index beadd10ef4d..b59d1599b8e 100644 --- a/lib/runtime-core/src/global.rs +++ b/lib/runtime-core/src/global.rs @@ -4,11 +4,14 @@ use crate::{ types::{GlobalDescriptor, Type, Value}, vm, }; -use std::{cell::RefCell, fmt, sync::Arc}; +use std::{ + fmt, + sync::{Arc, Mutex}, +}; pub struct Global { desc: GlobalDescriptor, - storage: Arc>, + storage: Arc>, } impl Global { @@ -56,7 +59,7 @@ impl Global { Self { desc, - storage: Arc::new(RefCell::new(local_global)), + storage: Arc::new(Mutex::new(local_global)), } } @@ -83,7 +86,8 @@ impl Global { Value::V128(x) => x, }, }; - *self.storage.borrow_mut() = local_global; + let mut storage = self.storage.lock().unwrap(); + *storage = local_global; } else { panic!("Wrong type for setting this global") } @@ -94,7 +98,8 @@ impl Global { /// Get the value held by this global. pub fn get(&self) -> Value { - let data = self.storage.borrow().data; + let storage = self.storage.lock().unwrap(); + let data = storage.data; match self.desc.ty { Type::I32 => Value::I32(data as i32), @@ -105,8 +110,10 @@ impl Global { } } + // TODO: think about this and if this should now be unsafe pub(crate) fn vm_local_global(&mut self) -> *mut vm::LocalGlobal { - &mut *self.storage.borrow_mut() + let mut storage = self.storage.lock().unwrap(); + &mut *storage } } diff --git a/lib/runtime-core/src/import.rs b/lib/runtime-core/src/import.rs index c7e15c90d70..1b79e7422ae 100644 --- a/lib/runtime-core/src/import.rs +++ b/lib/runtime-core/src/import.rs @@ -207,11 +207,9 @@ impl Extend<(String, String, Export)> for ImportObject { } pub struct Namespace { - map: HashMap>, + map: HashMap>, } -unsafe impl Send for Namespace {} - impl Namespace { pub fn new() -> Self { Self { @@ -219,10 +217,10 @@ impl Namespace { } } - pub fn insert(&mut self, name: S, export: E) -> Option> + pub fn insert(&mut self, name: S, export: E) -> Option> where S: Into, - E: IsExport + 'static, + E: IsExport + Send + 'static, { self.map.insert(name.into(), Box::new(export)) } diff --git a/lib/runtime-core/src/table/mod.rs b/lib/runtime-core/src/table/mod.rs index 2e11507b1de..c83e72ae7cc 100644 --- a/lib/runtime-core/src/table/mod.rs +++ b/lib/runtime-core/src/table/mod.rs @@ -5,7 +5,10 @@ use crate::{ types::{ElementType, TableDescriptor}, vm, }; -use std::{cell::RefCell, fmt, ptr, rc::Rc}; +use std::{ + fmt, ptr, + sync::{Arc, Mutex}, +}; mod anyfunc; @@ -25,7 +28,7 @@ pub enum TableStorage { pub struct Table { desc: TableDescriptor, - storage: Rc>, + storage: Arc>, } impl Table { @@ -71,7 +74,7 @@ impl Table { Ok(Self { desc, - storage: Rc::new(RefCell::new((storage, local))), + storage: Arc::new(Mutex::new((storage, local))), }) } @@ -82,7 +85,8 @@ impl Table { /// Set the element at index. pub fn set(&self, index: u32, element: Element) -> Result<(), ()> { - match &mut *self.storage.borrow_mut() { + let mut storage = self.storage.lock().unwrap(); + match &mut *storage { (TableStorage::Anyfunc(ref mut anyfunc_table), _) => { match element { Element::Anyfunc(anyfunc) => anyfunc_table.set(index, anyfunc), @@ -96,14 +100,16 @@ impl Table { where F: FnOnce(&mut [vm::Anyfunc]) -> R, { - match &mut *self.storage.borrow_mut() { + let mut storage = self.storage.lock().unwrap(); + match &mut *storage { (TableStorage::Anyfunc(ref mut anyfunc_table), _) => f(anyfunc_table.internal_buffer()), } } /// The current size of this table. pub fn size(&self) -> u32 { - match &*self.storage.borrow() { + let storage = self.storage.lock().unwrap(); + match &*storage { (TableStorage::Anyfunc(ref anyfunc_table), _) => anyfunc_table.current_size(), } } @@ -114,7 +120,8 @@ impl Table { return Ok(self.size()); } - match &mut *self.storage.borrow_mut() { + let mut storage = self.storage.lock().unwrap(); + match &mut *storage { (TableStorage::Anyfunc(ref mut anyfunc_table), ref mut local) => anyfunc_table .grow(delta, local) .ok_or(GrowError::TableGrowError), @@ -122,7 +129,8 @@ impl Table { } pub fn vm_local_table(&mut self) -> *mut vm::LocalTable { - &mut self.storage.borrow_mut().1 + let mut storage = self.storage.lock().unwrap(); + &mut storage.1 } } @@ -136,7 +144,7 @@ impl Clone for Table { fn clone(&self) -> Self { Self { desc: self.desc, - storage: Rc::clone(&self.storage), + storage: Arc::clone(&self.storage), } } } diff --git a/lib/runtime-core/src/typed_func.rs b/lib/runtime-core/src/typed_func.rs index 789c858cb96..0014e13cd7d 100644 --- a/lib/runtime-core/src/typed_func.rs +++ b/lib/runtime-core/src/typed_func.rs @@ -165,7 +165,8 @@ pub struct Func<'a, Args = (), Rets = (), Inner: Kind = Wasm> { _phantom: PhantomData<(&'a (), Args, Rets)>, } -unsafe impl<'a, Args, Rets> Send for Func<'a, Args, Rets> {} +unsafe impl<'a, Args, Rets> Send for Func<'a, Args, Rets, Wasm> {} +unsafe impl<'a, Args, Rets> Send for Func<'a, Args, Rets, Host> {} impl<'a, Args, Rets> Func<'a, Args, Rets, Wasm> where diff --git a/lib/runtime-core/src/vm.rs b/lib/runtime-core/src/vm.rs index 51f883318d1..1b85ecedcfa 100644 --- a/lib/runtime-core/src/vm.rs +++ b/lib/runtime-core/src/vm.rs @@ -501,6 +501,8 @@ pub struct LocalTable { pub table: *mut (), } +unsafe impl Send for LocalTable {} + impl LocalTable { #[allow(clippy::erasing_op)] // TODO pub fn offset_base() -> u8 { @@ -580,6 +582,8 @@ pub struct Anyfunc { pub sig_id: SigId, } +unsafe impl Send for Anyfunc {} + impl Anyfunc { pub fn null() -> Self { Self { From 83c3909b00df2d7d0dfb55d6d641fe63308f05d7 Mon Sep 17 00:00:00 2001 From: Mark McCaskey Date: Tue, 17 Sep 2019 14:58:26 -0700 Subject: [PATCH 3/6] Implement it for memory and make Instance Send --- lib/runtime-core/src/backing.rs | 4 +++ lib/runtime-core/src/export.rs | 2 -- lib/runtime-core/src/import.rs | 5 ++-- lib/runtime-core/src/instance.rs | 2 ++ lib/runtime-core/src/memory/mod.rs | 40 ++++++++++++++----------- lib/runtime-core/src/sys/unix/memory.rs | 6 ++-- lib/runtime-core/src/vm.rs | 2 ++ 7 files changed, 37 insertions(+), 24 deletions(-) diff --git a/lib/runtime-core/src/backing.rs b/lib/runtime-core/src/backing.rs index 1bb3bec4dd8..df1ab0e9626 100644 --- a/lib/runtime-core/src/backing.rs +++ b/lib/runtime-core/src/backing.rs @@ -54,6 +54,8 @@ pub struct LocalBacking { pub(crate) internals: Internals, } +unsafe impl Send for LocalBacking {} + impl LocalBacking { pub(crate) fn new( module: &ModuleInner, @@ -461,6 +463,8 @@ pub struct ImportBacking { pub(crate) vm_globals: BoxedMap, } +unsafe impl Send for ImportBacking {} + impl ImportBacking { pub fn new( module: &ModuleInner, diff --git a/lib/runtime-core/src/export.rs b/lib/runtime-core/src/export.rs index 67cd1e5fff1..96560cb6a0e 100644 --- a/lib/runtime-core/src/export.rs +++ b/lib/runtime-core/src/export.rs @@ -25,8 +25,6 @@ pub enum Export { Global(Global), } -unsafe impl Send for Export {} - #[derive(Debug, Clone)] pub struct FuncPointer(*const vm::Func); diff --git a/lib/runtime-core/src/import.rs b/lib/runtime-core/src/import.rs index 1b79e7422ae..c66cc28f996 100644 --- a/lib/runtime-core/src/import.rs +++ b/lib/runtime-core/src/import.rs @@ -46,7 +46,8 @@ impl IsExport for Export { /// ``` pub struct ImportObject { map: Arc>>>, - pub(crate) state_creator: Option (*mut c_void, fn(*mut c_void)) + Send>>, + pub(crate) state_creator: + Option (*mut c_void, fn(*mut c_void)) + Send + Sync + 'static>>, pub allow_missing_functions: bool, } @@ -62,7 +63,7 @@ impl ImportObject { pub fn new_with_data(state_creator: F) -> Self where - F: Fn() -> (*mut c_void, fn(*mut c_void)) + 'static + Send, + F: Fn() -> (*mut c_void, fn(*mut c_void)) + 'static + Send + Sync, { Self { map: Arc::new(Mutex::new(HashMap::new())), diff --git a/lib/runtime-core/src/instance.rs b/lib/runtime-core/src/instance.rs index e6ba365f6ef..50ab0ce0088 100644 --- a/lib/runtime-core/src/instance.rs +++ b/lib/runtime-core/src/instance.rs @@ -25,6 +25,8 @@ pub(crate) struct InstanceInner { pub(crate) vmctx: *mut vm::Ctx, } +unsafe impl Send for InstanceInner {} + impl Drop for InstanceInner { fn drop(&mut self) { // Drop the vmctx. diff --git a/lib/runtime-core/src/memory/mod.rs b/lib/runtime-core/src/memory/mod.rs index 75e9ea007dc..ee71160adab 100644 --- a/lib/runtime-core/src/memory/mod.rs +++ b/lib/runtime-core/src/memory/mod.rs @@ -8,12 +8,9 @@ use crate::{ units::Pages, vm, }; -use std::{ - cell::{Cell, RefCell}, - fmt, mem, - rc::Rc, - sync::Arc, -}; +use std::{cell::Cell, fmt, mem, sync::Arc}; + +use std::sync::Mutex as StdMutex; pub use self::dynamic::DynamicMemory; pub use self::static_::StaticMemory; @@ -208,14 +205,17 @@ enum UnsharedMemoryStorage { } pub struct UnsharedMemory { - internal: Rc, + internal: Arc, } struct UnsharedMemoryInternal { - storage: RefCell, + storage: StdMutex, local: Cell, } +unsafe impl Send for UnsharedMemoryInternal {} +unsafe impl Sync for UnsharedMemoryInternal {} + impl UnsharedMemory { pub fn new(desc: MemoryDescriptor) -> Result { let mut local = vm::LocalMemory { @@ -235,15 +235,15 @@ impl UnsharedMemory { }; Ok(Self { - internal: Rc::new(UnsharedMemoryInternal { - storage: RefCell::new(storage), + internal: Arc::new(UnsharedMemoryInternal { + storage: StdMutex::new(storage), local: Cell::new(local), }), }) } pub fn grow(&self, delta: Pages) -> Result { - let mut storage = self.internal.storage.borrow_mut(); + let mut storage = self.internal.storage.lock().unwrap(); let mut local = self.internal.local.get(); @@ -260,7 +260,7 @@ impl UnsharedMemory { } pub fn size(&self) -> Pages { - let storage = self.internal.storage.borrow(); + let storage = self.internal.storage.lock().unwrap(); match &*storage { UnsharedMemoryStorage::Dynamic(ref dynamic_memory) => dynamic_memory.size(), @@ -276,7 +276,7 @@ impl UnsharedMemory { impl Clone for UnsharedMemory { fn clone(&self) -> Self { UnsharedMemory { - internal: Rc::clone(&self.internal), + internal: Arc::clone(&self.internal), } } } @@ -286,11 +286,14 @@ pub struct SharedMemory { } pub struct SharedMemoryInternal { - memory: RefCell>, + memory: StdMutex>, local: Cell, lock: Mutex<()>, } +unsafe impl Send for SharedMemoryInternal {} +unsafe impl Sync for SharedMemoryInternal {} + impl SharedMemory { fn new(desc: MemoryDescriptor) -> Result { let mut local = vm::LocalMemory { @@ -303,7 +306,7 @@ impl SharedMemory { Ok(Self { internal: Arc::new(SharedMemoryInternal { - memory: RefCell::new(memory), + memory: StdMutex::new(memory), local: Cell::new(local), lock: Mutex::new(()), }), @@ -313,15 +316,18 @@ impl SharedMemory { pub fn grow(&self, delta: Pages) -> Result { let _guard = self.internal.lock.lock(); let mut local = self.internal.local.get(); - let pages = self.internal.memory.borrow_mut().grow(delta, &mut local); + let mut memory = self.internal.memory.lock().unwrap(); + let pages = memory.grow(delta, &mut local); pages } pub fn size(&self) -> Pages { let _guard = self.internal.lock.lock(); - self.internal.memory.borrow_mut().size() + let memory = self.internal.memory.lock().unwrap(); + memory.size() } + // This function is scary, because the mutex is not locked here pub(crate) fn vm_local_memory(&self) -> *mut vm::LocalMemory { self.internal.local.as_ptr() } diff --git a/lib/runtime-core/src/sys/unix/memory.rs b/lib/runtime-core/src/sys/unix/memory.rs index f4af2493b61..fc60d732b6f 100644 --- a/lib/runtime-core/src/sys/unix/memory.rs +++ b/lib/runtime-core/src/sys/unix/memory.rs @@ -4,7 +4,7 @@ use errno; use nix::libc; use page_size; use std::ops::{Bound, RangeBounds}; -use std::{fs::File, os::unix::io::IntoRawFd, path::Path, ptr, rc::Rc, slice}; +use std::{fs::File, os::unix::io::IntoRawFd, path::Path, ptr, slice, sync::Arc}; unsafe impl Send for Memory {} unsafe impl Sync for Memory {} @@ -14,7 +14,7 @@ pub struct Memory { ptr: *mut u8, size: usize, protection: Protect, - fd: Option>, + fd: Option>, } impl Memory { @@ -49,7 +49,7 @@ impl Memory { ptr: ptr as *mut u8, size: file_len as usize, protection, - fd: Some(Rc::new(raw_fd)), + fd: Some(Arc::new(raw_fd)), }) } } diff --git a/lib/runtime-core/src/vm.rs b/lib/runtime-core/src/vm.rs index 1b85ecedcfa..5f1f1fb0cef 100644 --- a/lib/runtime-core/src/vm.rs +++ b/lib/runtime-core/src/vm.rs @@ -474,6 +474,8 @@ pub struct ImportedFunc { pub vmctx: *mut Ctx, } +unsafe impl Send for ImportedFunc {} + impl ImportedFunc { #[allow(clippy::erasing_op)] // TODO pub fn offset_func() -> u8 { From 9c205e05a2cd7818d07b2279613972aec7e08ac6 Mon Sep 17 00:00:00 2001 From: Mark McCaskey Date: Tue, 17 Sep 2019 18:35:12 -0700 Subject: [PATCH 4/6] Add comments explaining the unsafe impls and simplify the code, too --- lib/runtime-core/src/backing.rs | 2 ++ lib/runtime-core/src/export.rs | 2 ++ lib/runtime-core/src/import.rs | 11 ----------- lib/runtime-core/src/instance.rs | 1 + lib/runtime-core/src/memory/mod.rs | 6 ++++-- lib/runtime-core/src/vm.rs | 6 ++++++ 6 files changed, 15 insertions(+), 13 deletions(-) diff --git a/lib/runtime-core/src/backing.rs b/lib/runtime-core/src/backing.rs index df1ab0e9626..ef43bd58c64 100644 --- a/lib/runtime-core/src/backing.rs +++ b/lib/runtime-core/src/backing.rs @@ -54,6 +54,7 @@ pub struct LocalBacking { pub(crate) internals: Internals, } +// Manually implemented because LocalBacking contains raw pointers directly unsafe impl Send for LocalBacking {} impl LocalBacking { @@ -463,6 +464,7 @@ pub struct ImportBacking { pub(crate) vm_globals: BoxedMap, } +// manually implemented because ImportBacking contains raw pointers directly unsafe impl Send for ImportBacking {} impl ImportBacking { diff --git a/lib/runtime-core/src/export.rs b/lib/runtime-core/src/export.rs index 96560cb6a0e..7960d76e699 100644 --- a/lib/runtime-core/src/export.rs +++ b/lib/runtime-core/src/export.rs @@ -11,6 +11,7 @@ pub enum Context { Internal, } +// Manually implemented because context contains a raw pointer to Ctx unsafe impl Send for Context {} #[derive(Debug, Clone)] @@ -28,6 +29,7 @@ pub enum Export { #[derive(Debug, Clone)] pub struct FuncPointer(*const vm::Func); +// Manually implemented because FuncPointer contains a raw pointer to Ctx unsafe impl Send for FuncPointer {} impl FuncPointer { diff --git a/lib/runtime-core/src/import.rs b/lib/runtime-core/src/import.rs index c66cc28f996..ff09450feb1 100644 --- a/lib/runtime-core/src/import.rs +++ b/lib/runtime-core/src/import.rs @@ -107,17 +107,6 @@ impl ImportObject { } } - /*pub fn get_namespace( - &self, - namespace: &str, - ) -> Option< - MutexGuardRef>, &(dyn LikeNamespace + Send)>, - > { - MutexGuardRef::new(self.map.lock().unwrap()) - .try_map(|mg| mg.get(namespace).map(|ns| &ns.as_ref()).ok_or(())) - .ok() - }*/ - /// Apply a function on the namespace if it exists /// If your function can fail, consider using `maybe_with_namespace` pub fn with_namespace(&self, namespace: &str, f: Func) -> Option diff --git a/lib/runtime-core/src/instance.rs b/lib/runtime-core/src/instance.rs index 50ab0ce0088..6f30b0a7489 100644 --- a/lib/runtime-core/src/instance.rs +++ b/lib/runtime-core/src/instance.rs @@ -25,6 +25,7 @@ pub(crate) struct InstanceInner { pub(crate) vmctx: *mut vm::Ctx, } +// manually implemented because InstanceInner contains a raw pointer to Ctx unsafe impl Send for InstanceInner {} impl Drop for InstanceInner { diff --git a/lib/runtime-core/src/memory/mod.rs b/lib/runtime-core/src/memory/mod.rs index ee71160adab..7f272be37bb 100644 --- a/lib/runtime-core/src/memory/mod.rs +++ b/lib/runtime-core/src/memory/mod.rs @@ -213,7 +213,8 @@ struct UnsharedMemoryInternal { local: Cell, } -unsafe impl Send for UnsharedMemoryInternal {} +// Manually implemented because UnsharedMemoryInternal uses `Cell` and is used in an Arc; +// this is safe because the lock for storage can be used to protect (seems like a weak reason: PLEASE REVIEW!) unsafe impl Sync for UnsharedMemoryInternal {} impl UnsharedMemory { @@ -291,7 +292,8 @@ pub struct SharedMemoryInternal { lock: Mutex<()>, } -unsafe impl Send for SharedMemoryInternal {} +// Manually implemented because SharedMemoryInternal uses `Cell` and is used in Arc; +// this is safe because of `lock`; accesing `local` without locking `lock` is not safe (Maybe we could put the lock on Local then?) unsafe impl Sync for SharedMemoryInternal {} impl SharedMemory { diff --git a/lib/runtime-core/src/vm.rs b/lib/runtime-core/src/vm.rs index 5f1f1fb0cef..40ca5d16ef1 100644 --- a/lib/runtime-core/src/vm.rs +++ b/lib/runtime-core/src/vm.rs @@ -474,6 +474,7 @@ pub struct ImportedFunc { pub vmctx: *mut Ctx, } +// manually implemented because ImportedFunc contains raw pointers directly; `Func` is marked Send (But `Ctx` actually isn't! (TODO: review this, shouldn't `Ctx` be Send?)) unsafe impl Send for ImportedFunc {} impl ImportedFunc { @@ -503,6 +504,7 @@ pub struct LocalTable { pub table: *mut (), } +// manually implemented because LocalTable contains raw pointers directly unsafe impl Send for LocalTable {} impl LocalTable { @@ -534,6 +536,9 @@ pub struct LocalMemory { pub memory: *mut (), } +// manually implemented because LocalMemory contains raw pointers +unsafe impl Send for LocalMemory {} + impl LocalMemory { #[allow(clippy::erasing_op)] // TODO pub fn offset_base() -> u8 { @@ -584,6 +589,7 @@ pub struct Anyfunc { pub sig_id: SigId, } +// manually implemented because Anyfunc contains raw pointers directly unsafe impl Send for Anyfunc {} impl Anyfunc { From 05ad1aaea4c8d259ef10e334e70ddb53dca4e17d Mon Sep 17 00:00:00 2001 From: Mark McCaskey Date: Mon, 23 Sep 2019 11:04:31 -0700 Subject: [PATCH 5/6] Add test for Instance, fix tests for ImportObject --- CHANGELOG.md | 1 + lib/runtime-core/src/import.rs | 30 ++++++++++++++++-------------- lib/runtime-core/src/instance.rs | 12 ++++++++++++ 3 files changed, 29 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1651f7ffbeb..caa92879209 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ Blocks of changes will separated by version increments. ## **[Unreleased]** +- [#807](https://github.com/wasmerio/wasmer/pull/807) Implement Send for `Instance`, breaking change on `ImportObject`, remove method `get_namespace` replaced with `with_namespace` and `maybe_with_namespace` - [#790](https://github.com/wasmerio/wasmer/pull/790) Fix flaky test failure with LLVM, switch to large code model. - [#788](https://github.com/wasmerio/wasmer/pull/788) Use union merge on the changelog file. - [#785](https://github.com/wasmerio/wasmer/pull/785) Include Apache license file for spectests. diff --git a/lib/runtime-core/src/import.rs b/lib/runtime-core/src/import.rs index ff09450feb1..2cccf2325af 100644 --- a/lib/runtime-core/src/import.rs +++ b/lib/runtime-core/src/import.rs @@ -266,12 +266,14 @@ mod test { imports1.extend(imports2); - let cat_ns = imports1.get_namespace("cat").unwrap(); - assert!(cat_ns.get_export("small").is_some()); - - let dog_ns = imports1.get_namespace("dog").unwrap(); - assert!(dog_ns.get_export("happy").is_some()); - assert!(dog_ns.get_export("small").is_some()); + let small_cat_export = + imports1.maybe_with_namespace("cat", |cat_ns| cat_ns.get_export("small")); + assert!(small_cat_export.is_some()); + + let entries = imports1.maybe_with_namespace("dog", |dog_ns| { + Some((dog_ns.get_export("happy")?, dog_ns.get_export("small")?)) + }); + assert!(entries.is_some()); } #[test] @@ -289,14 +291,14 @@ mod test { }; imports1.extend(imports2); - let dog_ns = imports1.get_namespace("dog").unwrap(); + let happy_dog_entry = imports1 + .maybe_with_namespace("dog", |dog_ns| dog_ns.get_export("happy")) + .unwrap(); - assert!( - if let Export::Global(happy_dog_global) = dog_ns.get_export("happy").unwrap() { - happy_dog_global.get() == Value::I32(4) - } else { - false - } - ); + assert!(if let Export::Global(happy_dog_global) = happy_dog_entry { + happy_dog_global.get() == Value::I32(4) + } else { + false + }); } } diff --git a/lib/runtime-core/src/instance.rs b/lib/runtime-core/src/instance.rs index 6f30b0a7489..c6867d1005e 100644 --- a/lib/runtime-core/src/instance.rs +++ b/lib/runtime-core/src/instance.rs @@ -752,3 +752,15 @@ impl<'a> DynFunc<'a> { } } } + +#[cfg(test)] +mod test { + use super::*; + + fn is_send() {} + + #[test] + fn test_instance_is_send() { + is_send::(); + } +} From c4818f12dc7a6c031eaf5f5f868b4abb26156c99 Mon Sep 17 00:00:00 2001 From: Mark McCaskey Date: Mon, 23 Sep 2019 13:43:01 -0700 Subject: [PATCH 6/6] Update spectests to work with new Instance; use Arc> --- lib/runtime-core/src/instance.rs | 28 +++- lib/spectests/tests/spectest.rs | 223 +++++++++++++------------------ 2 files changed, 121 insertions(+), 130 deletions(-) diff --git a/lib/runtime-core/src/instance.rs b/lib/runtime-core/src/instance.rs index 9ffd0577ebd..85e9f8f768b 100644 --- a/lib/runtime-core/src/instance.rs +++ b/lib/runtime-core/src/instance.rs @@ -16,7 +16,12 @@ use crate::{ vm::{self, InternalField}, }; use smallvec::{smallvec, SmallVec}; -use std::{mem, pin::Pin, ptr::NonNull, sync::Arc}; +use std::{ + mem, + pin::Pin, + ptr::NonNull, + sync::{Arc, Mutex}, +}; pub(crate) struct InstanceInner { #[allow(dead_code)] @@ -520,6 +525,27 @@ impl LikeNamespace for Rc { } } +impl LikeNamespace for Arc> { + fn get_export(&self, name: &str) -> Option { + let instance = self.lock().unwrap(); + let export_index = instance.module.info.exports.get(name)?; + + Some( + instance + .inner + .get_export_from_index(&instance.module, export_index), + ) + } + + fn get_exports(&self) -> Vec<(String, Export)> { + unimplemented!("Use the exports method instead"); + } + + fn maybe_insert(&mut self, _name: &str, _export: Export) -> Option<()> { + None + } +} + #[must_use] fn call_func_with_index( info: &ModuleInfo, diff --git a/lib/spectests/tests/spectest.rs b/lib/spectests/tests/spectest.rs index 9cd18d636a8..66a730d7650 100644 --- a/lib/spectests/tests/spectest.rs +++ b/lib/spectests/tests/spectest.rs @@ -18,7 +18,7 @@ mod tests { // TODO Files could be run with multiple threads // TODO Allow running WAST &str directly (E.g. for use outside of spectests) - use std::rc::Rc; + use std::sync::{Arc, Mutex}; struct SpecFailure { file: String, @@ -119,6 +119,24 @@ mod tests { "unknown" } + fn with_instance( + maybe_instance: Option>>, + named_modules: &HashMap>>, + module: &Option, + f: F, + ) -> Option + where + R: Sized, + F: FnOnce(&Instance) -> R, + { + let ref ins = module + .as_ref() + .and_then(|name| named_modules.get(name).cloned()) + .or(maybe_instance)?; + let guard = ins.lock().unwrap(); + Some(f(guard.borrow())) + } + use glob::glob; use std::collections::HashMap; use std::fs; @@ -173,11 +191,11 @@ mod tests { .expect(&format!("Failed to parse script {}", &filename)); use std::panic; - let mut instance: Option> = None; + let mut instance: Option>> = None; - let mut named_modules: HashMap> = HashMap::new(); + let mut named_modules: HashMap>> = HashMap::new(); - let mut registered_modules: HashMap> = HashMap::new(); + let mut registered_modules: HashMap>> = HashMap::new(); // while let Some(Command { kind, line }) = @@ -236,9 +254,9 @@ mod tests { instance = None; } Ok(i) => { - let i = Rc::new(i); + let i = Arc::new(Mutex::new(i)); if name.is_some() { - named_modules.insert(name.unwrap(), Rc::clone(&i)); + named_modules.insert(name.unwrap(), Arc::clone(&i)); } instance = Some(i); } @@ -251,20 +269,17 @@ mod tests { field, args, } => { - let instance: Option<&Instance> = match module { - Some(ref name) => { - let i = named_modules.get(name); - match i { - Some(ins) => Some(ins.borrow()), - None => None, - } - } - None => match instance { - Some(ref i) => Some(i.borrow()), - None => None, + let maybe_call_result = with_instance( + instance.clone(), + &named_modules, + &module, + |instance| { + let params: Vec = + args.iter().cloned().map(convert_value).collect(); + instance.call(&field, ¶ms[..]) }, - }; - if instance.is_none() { + ); + if maybe_call_result.is_none() { test_report.add_failure( SpecFailure { file: filename.to_string(), @@ -276,9 +291,7 @@ mod tests { excludes, ); } else { - let params: Vec = - args.iter().cloned().map(|x| convert_value(x)).collect(); - let call_result = instance.unwrap().call(&field, ¶ms[..]); + let call_result = maybe_call_result.unwrap(); match call_result { Err(e) => { test_report.add_failure( @@ -316,20 +329,17 @@ mod tests { } } Action::Get { module, field } => { - let instance: Option<&Instance> = match module { - Some(ref name) => { - let i = named_modules.get(name); - match i { - Some(ins) => Some(ins.borrow()), - None => None, - } - } - None => match instance { - Some(ref i) => Some(i.borrow()), - None => None, + let maybe_call_result = with_instance( + instance.clone(), + &named_modules, + &module, + |instance| { + instance + .get_export(&field) + .expect(&format!("missing global {:?}", &field)) }, - }; - if instance.is_none() { + ); + if maybe_call_result.is_none() { test_report.add_failure( SpecFailure { file: filename.to_string(), @@ -341,10 +351,7 @@ mod tests { excludes, ); } else { - let export: Export = instance - .unwrap() - .get_export(&field) - .expect(&format!("missing global {:?}", &field)); + let export: Export = maybe_call_result.unwrap(); match export { Export::Global(g) => { let value = g.get(); @@ -392,20 +399,13 @@ mod tests { field, args, } => { - let instance: Option<&Instance> = match module { - Some(ref name) => { - let i = named_modules.get(name); - match i { - Some(ins) => Some(ins.borrow()), - None => None, - } - } - None => match instance { - Some(ref i) => Some(i.borrow()), - None => None, - }, - }; - if instance.is_none() { + let maybe_call_result = + with_instance(instance.clone(), &named_modules, &module, |instance| { + let params: Vec = + args.iter().cloned().map(convert_value).collect(); + instance.call(&field, ¶ms[..]) + }); + if maybe_call_result.is_none() { test_report.add_failure( SpecFailure { file: filename.to_string(), @@ -417,9 +417,7 @@ mod tests { excludes, ); } else { - let params: Vec = - args.iter().cloned().map(|x| convert_value(x)).collect(); - let call_result = instance.unwrap().call(&field, ¶ms[..]); + let call_result = maybe_call_result.unwrap(); match call_result { Err(e) => { test_report.add_failure( @@ -468,20 +466,13 @@ mod tests { field, args, } => { - let instance: Option<&Instance> = match module { - Some(ref name) => { - let i = named_modules.get(name); - match i { - Some(ins) => Some(ins.borrow()), - None => None, - } - } - None => match instance { - Some(ref i) => Some(i.borrow()), - None => None, - }, - }; - if instance.is_none() { + let maybe_call_result = + with_instance(instance.clone(), &named_modules, &module, |instance| { + let params: Vec = + args.iter().cloned().map(convert_value).collect(); + instance.call(&field, ¶ms[..]) + }); + if maybe_call_result.is_none() { test_report.add_failure( SpecFailure { file: filename.to_string(), @@ -493,9 +484,7 @@ mod tests { excludes, ); } else { - let params: Vec = - args.iter().cloned().map(|x| convert_value(x)).collect(); - let call_result = instance.unwrap().call(&field, ¶ms[..]); + let call_result = maybe_call_result.unwrap(); match call_result { Err(e) => { test_report.add_failure( @@ -544,20 +533,13 @@ mod tests { field, args, } => { - let instance: Option<&Instance> = match module { - Some(ref name) => { - let i = named_modules.get(name); - match i { - Some(ins) => Some(ins.borrow()), - None => None, - } - } - None => match instance { - Some(ref i) => Some(i.borrow()), - None => None, - }, - }; - if instance.is_none() { + let maybe_call_result = + with_instance(instance.clone(), &named_modules, &module, |instance| { + let params: Vec = + args.iter().cloned().map(convert_value).collect(); + instance.call(&field, ¶ms[..]) + }); + if maybe_call_result.is_none() { test_report.add_failure( SpecFailure { file: filename.to_string(), @@ -569,9 +551,7 @@ mod tests { excludes, ); } else { - let params: Vec = - args.iter().cloned().map(|x| convert_value(x)).collect(); - let call_result = instance.unwrap().call(&field, ¶ms[..]); + let call_result = maybe_call_result.unwrap(); use wasmer_runtime_core::error::{CallError, RuntimeError}; match call_result { Err(e) => { @@ -749,20 +729,17 @@ mod tests { field, args, } => { - let instance: Option<&Instance> = match module { - Some(ref name) => { - let i = named_modules.get(name); - match i { - Some(ins) => Some(ins.borrow()), - None => None, - } - } - None => match instance { - Some(ref i) => Some(i.borrow()), - None => None, + let maybe_call_result = with_instance( + instance.clone(), + &named_modules, + &module, + |instance| { + let params: Vec = + args.iter().cloned().map(convert_value).collect(); + instance.call(&field, ¶ms[..]) }, - }; - if instance.is_none() { + ); + if maybe_call_result.is_none() { test_report.add_failure( SpecFailure { file: filename.to_string(), @@ -774,9 +751,7 @@ mod tests { excludes, ); } else { - let params: Vec = - args.iter().cloned().map(|x| convert_value(x)).collect(); - let call_result = instance.unwrap().call(&field, ¶ms[..]); + let call_result = maybe_call_result.unwrap(); match call_result { Err(_e) => { // TODO is specific error required? @@ -871,16 +846,16 @@ mod tests { } } CommandKind::Register { name, as_name } => { - let instance: Option> = match name { + let instance: Option>> = match name { Some(ref name) => { let i = named_modules.get(name); match i { - Some(ins) => Some(Rc::clone(ins)), + Some(ins) => Some(Arc::clone(ins)), None => None, } } None => match instance { - Some(ref i) => Some(Rc::clone(i)), + Some(ref i) => Some(Arc::clone(i)), None => None, }, }; @@ -906,21 +881,13 @@ mod tests { field, args, } => { - let instance: Option<&Instance> = match module { - Some(ref name) => { - let i = named_modules.get(name); - match i { - Some(ins) => Some(ins.borrow()), - None => None, - } - } - None => match instance { - Some(ref i) => Some(i.borrow()), - None => None, - }, - }; - - if instance.is_none() { + let maybe_call_result = + with_instance(instance.clone(), &named_modules, &module, |instance| { + let params: Vec = + args.iter().cloned().map(convert_value).collect(); + instance.call(&field, ¶ms[..]) + }); + if maybe_call_result.is_none() { test_report.add_failure( SpecFailure { file: filename.to_string(), @@ -932,9 +899,7 @@ mod tests { excludes, ); } else { - let params: Vec = - args.iter().cloned().map(|x| convert_value(x)).collect(); - let call_result = instance.unwrap().call(&field, ¶ms[..]); + let call_result = maybe_call_result.unwrap(); match call_result { Err(e) => { test_report.add_failure( @@ -1054,7 +1019,7 @@ mod tests { } fn get_spectest_import_object( - registered_modules: &HashMap>, + registered_modules: &HashMap>>, ) -> ImportObject { let memory = Memory::new(MemoryDescriptor { minimum: Pages(1), @@ -1091,7 +1056,7 @@ mod tests { }; for (name, instance) in registered_modules.iter() { - import_object.register(name.clone(), Rc::clone(instance)); + import_object.register(name.clone(), Arc::clone(instance)); } import_object }