Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for shared memory in Wasm. #702

Merged
merged 17 commits into from
Aug 21, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
464 changes: 250 additions & 214 deletions Cargo.lock

Large diffs are not rendered by default.

2,062 changes: 2,061 additions & 1 deletion lib/llvm-backend/src/code.rs

Large diffs are not rendered by default.

2 changes: 2 additions & 0 deletions lib/llvm-backend/src/intrinsics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,7 @@ pub struct Intrinsics {
pub trap_call_indirect_oob: BasicValueEnum,
pub trap_memory_oob: BasicValueEnum,
pub trap_illegal_arithmetic: BasicValueEnum,
pub trap_misaligned_atomic: BasicValueEnum,

// VM intrinsics.
pub memory_grow_dynamic_local: FunctionValue,
Expand Down Expand Up @@ -458,6 +459,7 @@ impl Intrinsics {
trap_call_indirect_oob: i32_ty.const_int(3, false).as_basic_value_enum(),
trap_memory_oob: i32_ty.const_int(2, false).as_basic_value_enum(),
trap_illegal_arithmetic: i32_ty.const_int(4, false).as_basic_value_enum(),
trap_misaligned_atomic: i32_ty.const_int(5, false).as_basic_value_enum(),

// VM intrinsics.
memory_grow_dynamic_local: module.add_function(
Expand Down
1 change: 1 addition & 0 deletions lib/runtime-core/src/backend.rs
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@ impl Default for MemoryBoundCheckMode {
#[derive(Debug, Default)]
pub struct Features {
pub simd: bool,
pub threads: bool,
}

/// Configuration data for the compiler
Expand Down
2 changes: 1 addition & 1 deletion lib/runtime-core/src/codegen.rs
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ impl<
pub fn validating_parser_config(features: &Features) -> wasmparser::ValidatingParserConfig {
wasmparser::ValidatingParserConfig {
operator_config: wasmparser::OperatorValidatorConfig {
enable_threads: false,
enable_threads: features.threads,
enable_reference_types: false,
enable_simd: features.simd,
enable_bulk_memory: false,
Expand Down
2 changes: 1 addition & 1 deletion lib/runtime-core/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ pub fn validate_and_report_errors_with_features(
enable_bulk_memory: false,
enable_multi_value: false,
enable_reference_types: false,
enable_threads: false,
enable_threads: features.threads,
},
mutable_global_imports: true,
};
Expand Down
62 changes: 42 additions & 20 deletions lib/runtime-core/src/memory/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,15 @@ use std::{
cell::{Cell, RefCell},
fmt, mem,
rc::Rc,
sync::Arc,
};

pub use self::dynamic::DynamicMemory;
pub use self::static_::{SharedStaticMemory, StaticMemory};
pub use self::static_::StaticMemory;
pub use self::view::{Atomically, MemoryView};

use parking_lot::Mutex;

mod dynamic;
pub mod ptr;
mod static_;
Expand Down Expand Up @@ -151,20 +154,10 @@ impl Memory {
unsafe { MemoryView::new(base as _, length as u32) }
}

/// Convert this memory to a shared memory if the shared flag
/// is present in the description used to create it.
pub fn shared(self) -> Option<SharedMemory> {
if self.desc.shared {
Some(SharedMemory { desc: self.desc })
} else {
None
}
}

pub(crate) fn vm_local_memory(&self) -> *mut vm::LocalMemory {
match &self.variant {
MemoryVariant::Unshared(unshared_mem) => unshared_mem.vm_local_memory(),
MemoryVariant::Shared(_) => unimplemented!(),
MemoryVariant::Shared(shared_mem) => shared_mem.vm_local_memory(),
}
}
}
Expand Down Expand Up @@ -241,7 +234,7 @@ impl UnsharedMemory {
MemoryType::SharedStatic => panic!("attempting to create shared unshared memory"),
};

Ok(UnsharedMemory {
Ok(Self {
internal: Rc::new(UnsharedMemoryInternal {
storage: RefCell::new(storage),
local: Cell::new(local),
Expand Down Expand Up @@ -289,27 +282,56 @@ impl Clone for UnsharedMemory {
}

pub struct SharedMemory {
#[allow(dead_code)]
desc: MemoryDescriptor,
internal: Arc<SharedMemoryInternal>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need an internal structure? can we move all the fields from SharedMemoryInternal into the SharedMemory itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue is the implementation of Clone for SharedMemory. Mind you, I'm suspicious of the fact that we clone memories to begin with, but I took a look and decided that was a problem for another day. The important part is that the underlying size/grow methods share one lock, which is held by SharedMemoryInternal while the SharedMemory can be cloned to your heart's content.

}

pub struct SharedMemoryInternal {
memory: RefCell<Box<StaticMemory>>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe dumb question: shared memory is always static?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we've implemented it, yes, but I can't see any reason we couldn't add SharedDynamic if we wanted to. The challenge would be ensuring that the base pointer of the dynamic memory doesn't move while we're forming and using a pointer into that memory. A naive implementation could hold a mutex lock for every memory access in the wasm program.

An efficient implementation might implement base changes by temporarily mapping the same pages at two addresses in memory so that old pointers continue to work during the move. Then, remove the old mapping such that the pointers fault. You can't use that memory again until you can prove all old pointers are gone (otherwise you break Wasm security model). For the wasm program's loads and stores, either it would need to reload the base pointer if the access faults (if we unmap the memory early), or you keep the temporary mapping around until no such pointers exist but that means the grow operation is blocked until then too.

local: Cell<vm::LocalMemory>,
lock: Mutex<()>,
}

impl SharedMemory {
fn new(desc: MemoryDescriptor) -> Result<Self, CreationError> {
Ok(Self { desc })
let mut local = vm::LocalMemory {
base: std::ptr::null_mut(),
bound: 0,
memory: std::ptr::null_mut(),
};

let memory = StaticMemory::new(desc, &mut local)?;

Ok(Self {
internal: Arc::new(SharedMemoryInternal {
memory: RefCell::new(memory),
local: Cell::new(local),
lock: Mutex::new(()),
}),
})
}

pub fn grow(&self, _delta: Pages) -> Result<Pages, GrowError> {
unimplemented!()
pub fn grow(&self, delta: Pages) -> Result<Pages, GrowError> {
let _guard = self.internal.lock.lock();
let mut local = self.internal.local.get();
let pages = self.internal.memory.borrow_mut().grow(delta, &mut local);
pages
}

pub fn size(&self) -> Pages {
unimplemented!()
let _guard = self.internal.lock.lock();
self.internal.memory.borrow_mut().size()
}

pub(crate) fn vm_local_memory(&self) -> *mut vm::LocalMemory {
self.internal.local.as_ptr()
}
}

impl Clone for SharedMemory {
fn clone(&self) -> Self {
unimplemented!()
SharedMemory {
internal: Arc::clone(&self.internal),
}
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,12 +1,10 @@
use crate::error::GrowError;
use crate::{
error::CreationError,
memory::static_::{SAFE_STATIC_GUARD_SIZE, SAFE_STATIC_HEAP_SIZE},
sys,
types::MemoryDescriptor,
units::Pages,
vm,
};
use crate::{error::CreationError, sys, types::MemoryDescriptor, units::Pages, vm};

#[doc(hidden)]
pub const SAFE_STATIC_HEAP_SIZE: usize = 1 << 32; // 4 GiB
#[doc(hidden)]
pub const SAFE_STATIC_GUARD_SIZE: usize = 1 << 31; // 2 GiB

/// This is an internal-only api.
///
Expand Down
10 changes: 0 additions & 10 deletions lib/runtime-core/src/memory/static_/mod.rs

This file was deleted.

11 changes: 0 additions & 11 deletions lib/runtime-core/src/memory/static_/shared.rs

This file was deleted.

2 changes: 2 additions & 0 deletions lib/runtime-core/src/typed_func.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ pub enum WasmTrapInfo {
MemoryOutOfBounds = 2,
CallIndirectOOB = 3,
IllegalArithmetic = 4,
MisalignedAtomicAccess = 5,
Unknown,
}

Expand All @@ -39,6 +40,7 @@ impl fmt::Display for WasmTrapInfo {
WasmTrapInfo::MemoryOutOfBounds => "memory out-of-bounds access",
WasmTrapInfo::CallIndirectOOB => "`call_indirect` out-of-bounds",
WasmTrapInfo::IllegalArithmetic => "illegal arithmetic operation",
WasmTrapInfo::MisalignedAtomicAccess => "misaligned atomic access",
WasmTrapInfo::Unknown => "unknown",
}
)
Expand Down
4 changes: 2 additions & 2 deletions lib/runtime-core/src/vm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -208,15 +208,15 @@ fn get_intrinsics_for_module(m: &ModuleInfo) -> *const Intrinsics {
match mem_desc.memory_type() {
MemoryType::Dynamic => &INTRINSICS_LOCAL_DYNAMIC_MEMORY,
MemoryType::Static => &INTRINSICS_LOCAL_STATIC_MEMORY,
MemoryType::SharedStatic => unimplemented!(),
MemoryType::SharedStatic => &INTRINSICS_LOCAL_STATIC_MEMORY,
}
}
LocalOrImport::Import(import_mem_index) => {
let mem_desc = &m.imported_memories[import_mem_index].1;
match mem_desc.memory_type() {
MemoryType::Dynamic => &INTRINSICS_IMPORTED_DYNAMIC_MEMORY,
MemoryType::Static => &INTRINSICS_IMPORTED_STATIC_MEMORY,
MemoryType::SharedStatic => unimplemented!(),
MemoryType::SharedStatic => &INTRINSICS_IMPORTED_STATIC_MEMORY,
}
}
}
Expand Down
Loading