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.

Sign up for GitHub

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

Clean up host env code, add more Send, Sync bounds #1944

Merged
merged 5 commits into from
Dec 18, 2020
Merged

Clean up host env code, add more Send, Sync bounds #1944

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4494bea
Clean up host env code, add more `Send`, `Sync` bounds
Dec 16, 2020
fc9a8f8
Address feedback, clean up code
Dec 18, 2020
e3eb529
Merge branch 'master' into feature/clean-up-host-env-code
MarkMcCaskey Dec 18, 2020
283acc5
Add 1944 to changelog
Dec 18, 2020
4f08f3d
Update deprecated API to new restrictions on dynamic envs
Dec 18, 2020
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
187 changes: 70 additions & 117 deletions lib/api/src/externals/function.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,49 @@ pub struct Function {
pub(crate) exported: ExportFunction,
}

fn build_export_function_metadata<Env>(
env: Env,
import_init_function_ptr: for<'a> fn(
&'a mut Env,
&'a crate::Instance,
) -> Result<(), crate::HostEnvInitError>,
Comment on lines +72 to +75
Copy link
Member

Choose a reason for hiding this comment

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

What is the for syntax? First time I'm seeing this!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Haha yeah, it's not super common

it's used to bind a lifetime in a scope, it's a universal qualifier (like ∀), it's just saying "for all lifetimes 'a, this thing, it's a way of constraining the type (and it's required here because it'll be inferred incorrectly)

Copy link
Contributor

Choose a reason for hiding this comment

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

@syrusakbary That's also because you can't write :

import_init_function_ptr: fn<'a>(&'a mut Env, &'a crate::Instance) -> …,

It's a syntax error.

@MarkMcCaskey I wonder though if there is a difference with:

fn build_export_function_metadata<'a, Env>(
    env: Env,
    import_init_function_ptr: fn(&'a mut Env, &'a crate::Instance) -> …,
) -> …

Anyway, I'm OK with this change!

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, obviously there is a difference. The difference with 'a on the function itself is that it will be inferred from the calling context. That's not what we want! Forget it.

) -> (*mut std::ffi::c_void, ExportFunctionMetadata)
where
Env: Clone + Sized + 'static + Send + Sync,
{
let import_init_function_ptr = Some(unsafe {
std::mem::transmute::<fn(_, _) -> Result<(), _>, fn(_, _) -> Result<(), _>>(
import_init_function_ptr,
)
});
Comment on lines +80 to +84
Copy link
Contributor

Choose a reason for hiding this comment

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

This code can be simplified to:

Suggested change
let import_init_function_ptr = Some(unsafe {
std::mem::transmute::<fn(_, _) -> Result<(), _>, fn(_, _) -> Result<(), _>>(
import_init_function_ptr,
)
});
let import_init_function_ptr = Some(unsafe {
std::mem::transmute::<_, fn(_, _) -> Result<(), _>>(
import_init_function_ptr,
)
});

or even:

Suggested change
let import_init_function_ptr = Some(unsafe {
std::mem::transmute::<fn(_, _) -> Result<(), _>, fn(_, _) -> Result<(), _>>(
import_init_function_ptr,
)
});
let import_init_function_ptr = Some(unsafe {
std::mem::transmute::<_, _>(
import_init_function_ptr,
)
});

so basically to:

Suggested change
let import_init_function_ptr = Some(unsafe {
std::mem::transmute::<fn(_, _) -> Result<(), _>, fn(_, _) -> Result<(), _>>(
import_init_function_ptr,
)
});
let import_init_function_ptr = Some(unsafe { std::mem::transmute(import_init_function_ptr) });

Rust is able to infer the type correctly all the way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's true, I thought listing both types was good documentation though: std::mem::transmute::<fn(_, _) -> Result<(), _>, fn(_, _) -> Result<(), _>>( communicates that the function arguments and error value are changing, but the number of arguments, and the fact that it's Result<(), are not. I'll rereview the code to see if this extra info is useful, but I think it's good to be super explicit with things like transmute as so many things can go wrong

let host_env_clone_fn: fn(*mut std::ffi::c_void) -> *mut std::ffi::c_void = |ptr| {
let env_ref: &Env = unsafe {
ptr.cast::<Env>()
.as_ref()
.expect("`ptr` to the environment is null when cloning it")
};
Box::into_raw(Box::new(env_ref.clone())) as _
};
let host_env_drop_fn: fn(*mut std::ffi::c_void) = |ptr| {
unsafe { Box::from_raw(ptr.cast::<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)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just returning metadata here, and let the rest of the code accessing metadata.env to get the environment? Is it by convenience?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

metadata.env is private! It can only be seen in wasmer_engine not in wasmer_API, this is for the best as the contents of the metadata struct can be dangerous

}

impl Function {
/// Creates a new host `Function` (dynamic) with the provided signature.
///
Expand Down Expand Up @@ -104,7 +147,7 @@ impl Function {
pub fn new<FT, F>(store: &Store, ty: FT, func: F) -> Self
where
FT: Into<FunctionType>,
F: Fn(&[Val]) -> Result<Vec<Val>, RuntimeError> + 'static,
F: Fn(&[Val]) -> Result<Vec<Val>, RuntimeError> + 'static + Send + Sync,
Copy link
Member

Choose a reason for hiding this comment

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

What it means for require a function to be Send + Sync ?
Could it still be possible to be called in parallel through different threads?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe Sync with Fn does mean that the function can be called from multiple threads at the same time. Send just means it can be executed on any thread.

For Send, this means functions can't use thread-local variables.

For Sync, this means functions should not use unsynchronized global variables.

I can't think of any other cases where this is really an issue right now, I'm sure there are a few other restrictions, but most functions that use unsafe correctly or don't use unsafe at all should be both Send and Sync

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it also means that the environment captured by the function must also implement Send + Sync.

Confirmed with:

use std::rc::Rc;

fn main() {
    fn check<F>(f: F) where F: Fn() + 'static + Send + Sync {}
    
    let x = Rc::new(42);
    check(|| { let _ = x; });
}

which raises the following issue:

error[E0277]: `Rc<i32>` cannot be shared between threads safely
 --> src/main.rs:7:5
  |
4 |     fn check<F>(f: F) where F: Fn() + 'static + Send + Sync {}
  |                                                 ---- required by this bound in `check`
...
7 |     check(|| { let _ = x; });
  |     ^^^^^ `Rc<i32>` cannot be shared between threads safely
  |
  = help: the trait `Sync` is not implemented for `Rc<i32>`
  = note: required because of the requirements on the impl of `Send` for `&Rc<i32>`
  = note: required because it appears within the type `[closure@src/main.rs:7:11: 7:28]`

(see the playground)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Hywan , I think having a Send + Sync environment is ultimately what we need for the Wasmer API... because Instance is Send + Sync, so a single function can be called from multiple threads concurrently and in parallel. I don't know a good way around this yet but I think Send + Sync does fix everything for us

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 alternative might be to have separate code and abstractions for non-thread safe things, but that seems pretty complicated 🤔 . I think encouraging code to work on multiple threads by default is a very Rusty thing anyways and I think it positions us and users of wasmer well to scale their applications

{
let ty: FunctionType = ty.into();
let dynamic_ctx: VMDynamicFunctionContext<DynamicFunctionWithoutEnv> =
Expand Down Expand Up @@ -209,7 +252,7 @@ impl Function {
pub fn new_with_env<FT, F, Env>(store: &Store, ty: FT, env: Env, func: F) -> Self
where
FT: Into<FunctionType>,
F: Fn(&Env, &[Val]) -> Result<Vec<Val>, RuntimeError> + 'static,
F: Fn(&Env, &[Val]) -> Result<Vec<Val>, RuntimeError> + 'static + Send + Sync,
Env: Sized + WasmerEnv + 'static,
{
let ty: FunctionType = ty.into();
Expand All @@ -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<DynamicFunctionWithEnv<Env>>,
instance: &crate::Instance| {
Env::init_with_instance(&mut *env.ctx.env, instance)
};
Comment on lines +266 to +270
Copy link
Contributor

Choose a reason for hiding this comment

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

👍


let (host_env, metadata) = build_export_function_metadata::<
VMDynamicFunctionContext<DynamicFunctionWithEnv<Env>>,
>(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<DynamicFunctionWithEnv<Env>>;
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::<fn(_, _) -> 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<DynamicFunctionWithEnv<Env>> = unsafe {
let ptr: *mut VMDynamicFunctionContext<DynamicFunctionWithEnv<Env>> = ptr as _;
let item: &VMDynamicFunctionContext<DynamicFunctionWithEnv<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 VMDynamicFunctionContext<DynamicFunctionWithEnv<Env>>)
};
};

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,
Expand Down Expand Up @@ -374,50 +386,17 @@ impl Function {
let function = inner::Function::<Args, Rets>::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, Env::init_with_instance);

// TODO: look into removing transmute by changing API type signatures
let import_init_function_ptr = Some(unsafe {
std::mem::transmute::<fn(_, _) -> 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,
Expand Down Expand Up @@ -455,43 +434,17 @@ impl Function {
let function = inner::Function::<Args, Rets>::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, 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,
Expand Down Expand Up @@ -855,15 +808,15 @@ 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<Vec<Val>, RuntimeError>;
fn function_type(&self) -> &FunctionType;
}

#[derive(Clone)]
pub(crate) struct DynamicFunctionWithoutEnv {
#[allow(clippy::type_complexity)]
func: Arc<dyn Fn(&[Val]) -> Result<Vec<Val>, RuntimeError> + 'static>,
func: Arc<dyn Fn(&[Val]) -> Result<Vec<Val>, RuntimeError> + 'static + Send + Sync>,
function_type: FunctionType,
}

Expand All @@ -878,15 +831,15 @@ impl VMDynamicFunction for DynamicFunctionWithoutEnv {

pub(crate) struct DynamicFunctionWithEnv<Env>
where
Env: Sized + 'static,
Env: Sized + 'static + Send + Sync,
{
function_type: FunctionType,
#[allow(clippy::type_complexity)]
func: Arc<dyn Fn(&Env, &[Val]) -> Result<Vec<Val>, RuntimeError> + 'static>,
func: Arc<dyn Fn(&Env, &[Val]) -> Result<Vec<Val>, RuntimeError> + 'static + Send + Sync>,
env: Box<Env>,
}

impl<Env: Sized + Clone + 'static> Clone for DynamicFunctionWithEnv<Env> {
impl<Env: Sized + Clone + 'static + Send + Sync> Clone for DynamicFunctionWithEnv<Env> {
fn clone(&self) -> Self {
Self {
env: self.env.clone(),
Expand All @@ -898,7 +851,7 @@ impl<Env: Sized + Clone + 'static> Clone for DynamicFunctionWithEnv<Env> {

impl<Env> VMDynamicFunction for DynamicFunctionWithEnv<Env>
where
Env: Sized + 'static,
Env: Sized + 'static + Send + Sync,
{
fn call(&self, args: &[Val]) -> Result<Vec<Val>, RuntimeError> {
(*self.func)(&*self.env, &args)
Expand Down
Loading