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

Make Function Env immutable #1663

Merged
merged 11 commits into from
Nov 20, 2020
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
4 changes: 2 additions & 2 deletions examples/imports_function_env.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,10 +74,10 @@ fn main() -> Result<(), Box<dyn std::error::Error>> {
}

// Create the functions
fn get_counter(env: &mut Env) -> i32 {
fn get_counter(env: &Env) -> i32 {
*env.counter.borrow()
}
fn add_to_counter(env: &mut Env, add: i32) -> i32 {
fn add_to_counter(env: &Env, add: i32) -> i32 {
let mut counter_ref = env.counter.borrow_mut();

*counter_ref += add;
Expand Down
2 changes: 2 additions & 0 deletions lib/api/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@ llvm = [
"wasmer-compiler-llvm",
"compiler",
]
# enables internal features used by the deprecated API.
deprecated = []
Comment on lines +70 to +71
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional: really wish we didn't have to do this. Deprecated ought to be built on top of the API. Or put another way, anything deprecated does should be supported by our real API as something that other library consumer may also want to do.

Not that I'm offering an alternative. IIUC we considered wrapping it in a Mutex and rejected that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not that I'm offering an alternative. IIUC we considered wrapping it in a Mutex and rejected that?

That was a different PR, if it were as simple as just conditionally putting something behind a mutex, then I agree. I tried 2-3 times to make it work without editing the API and this is the only way I've been able to make it work.

I agree that it kind of sucks, but I figure the deprecated API won't be around forever so 🤷

default-compiler = []
default-engine = []

Expand Down
113 changes: 112 additions & 1 deletion lib/api/src/externals/function.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@ use crate::FunctionType;
use crate::NativeFunc;
use crate::RuntimeError;
pub use inner::{FromToNativeWasmType, HostFunction, WasmTypeList, WithEnv, WithoutEnv};
#[cfg(feature = "deprecated")]
pub use inner::{LegacyEnv, WithLegacyEnv};

use std::cmp::max;
use std::fmt;
use wasmer_vm::{
Expand Down Expand Up @@ -235,7 +238,50 @@ impl Function {
// In the case of Host-defined functions `VMContext` is whatever environment
// the user want to attach to the function.
let box_env = Box::new(env);
let vmctx = Box::into_raw(box_env) as *mut _ as *const VMContext;
let vmctx = VMFunctionEnvironment {
host_env: Box::into_raw(box_env) as *mut _,
};
let signature = function.ty();

Self {
store: store.clone(),
definition: FunctionDefinition::Host(HostFunctionDefinition { has_env: true }),
exported: ExportFunction {
address,
kind: VMFunctionKind::Static,
vmctx,
signature,
call_trampoline: None,
},
}
}

/// Function used by the deprecated API to call a function with a `&mut` Env.
///
/// This is not a stable API and may be broken at any time.
MarkMcCaskey marked this conversation as resolved.
Show resolved Hide resolved
///
/// # Safety
/// - This function is only safe to use from the deprecated API.
#[doc(hidden)]
#[cfg(feature = "deprecated")]
pub unsafe fn new_native_with_env_legacy<F, Args, Rets, Env>(
store: &Store,
env: Env,
func: F,
) -> Self
where
F: HostFunction<Args, Rets, WithLegacyEnv, Env>,
Args: WasmTypeList,
Rets: WasmTypeList,
Env: LegacyEnv + 'static,
{
let function = inner::Function::<Args, Rets>::new(func);
let address = function.address();

let box_env = Box::new(env);
let vmctx = VMFunctionEnvironment {
host_env: Box::into_raw(box_env) as *mut _,
};
let signature = function.ty();

Self {
Expand Down Expand Up @@ -974,6 +1020,14 @@ mod inner {
fn function_body_ptr(self) -> *const VMFunctionBody;
}

/// Marker trait to limit what the hidden APIs needed for the deprecated API
/// can be used on.
///
/// Marks an environment as being passed by `&mut`.
#[cfg(feature = "deprecated")]
#[doc(hidden)]
pub unsafe trait LegacyEnv: Sized {}

/// Empty trait to specify the kind of `HostFunction`: With or
/// without an environment.
///
Expand All @@ -989,6 +1043,18 @@ mod inner {

impl HostFunctionKind for WithEnv {}

/// An empty struct to help Rust typing to determine
/// when a `HostFunction` does have an environment.
MarkMcCaskey marked this conversation as resolved.
Show resolved Hide resolved
///
/// This environment is passed by `&mut` and exists soley for the deprecated
MarkMcCaskey marked this conversation as resolved.
Show resolved Hide resolved
/// API.
#[cfg(feature = "deprecated")]
#[doc(hidden)]
pub struct WithLegacyEnv;

#[cfg(feature = "deprecated")]
impl HostFunctionKind for WithLegacyEnv {}

/// An empty struct to help Rust typing to determine
/// when a `HostFunction` does not have an environment.
pub struct WithoutEnv;
Expand Down Expand Up @@ -1214,6 +1280,51 @@ mod inner {
}
}

// Implement `HostFunction` for a function that has the same arity than the tuple.
// This specific function has an environment.
#[doc(hidden)]
#[cfg(feature = "deprecated")]
#[allow(unused_parens)]
impl< $( $x, )* Rets, RetsAsResult, Env, Func >
HostFunction<( $( $x ),* ), Rets, WithLegacyEnv, Env>
for
Func
where
$( $x: FromToNativeWasmType, )*
Rets: WasmTypeList,
RetsAsResult: IntoResult<Rets>,
Env: LegacyEnv,
Func: Fn(&mut Env, $( $x , )*) -> RetsAsResult + Send + 'static,
{
#[allow(non_snake_case)]
fn function_body_ptr(self) -> *const VMFunctionBody {
/// This is a function that wraps the real host
/// function. Its address will be used inside the
/// runtime.
extern fn func_wrapper<$( $x, )* Rets, RetsAsResult, Env, Func>( env: &mut Env, $( $x: $x::Native, )* ) -> Rets::CStruct
where
$( $x: FromToNativeWasmType, )*
Rets: WasmTypeList,
RetsAsResult: IntoResult<Rets>,
Env: Sized,
Func: Fn(&mut Env, $( $x ),* ) -> RetsAsResult + 'static
{
let func: &Func = unsafe { &*(&() as *const () as *const Func) };

let result = panic::catch_unwind(AssertUnwindSafe(|| {
func(env, $( FromToNativeWasmType::from_native($x) ),* ).into_result()
}));

match result {
Ok(Ok(result)) => return result.into_c_struct(),
Ok(Err(trap)) => unsafe { raise_user_trap(Box::new(trap)) },
Err(panic) => unsafe { resume_panic(panic) },
}
}

func_wrapper::< $( $x, )* Rets, RetsAsResult, Env, Self > as *const VMFunctionBody
}
}
};
}

Expand Down
3 changes: 3 additions & 0 deletions lib/api/src/externals/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@ mod table;
pub use self::function::{
FromToNativeWasmType, Function, HostFunction, WasmTypeList, WithEnv, WithoutEnv,
};

#[cfg(feature = "deprecated")]
pub use self::function::{LegacyEnv, WithLegacyEnv};
pub use self::global::Global;
pub use self::memory::Memory;
pub use self::table::Table;
Expand Down
2 changes: 2 additions & 0 deletions lib/api/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ pub mod internals {
//! `wasmer-vm`. Please don't use any of this types directly, as
//! they might change frequently or be removed in the future.
#[cfg(feature = "deprecated")]
pub use crate::externals::{LegacyEnv, WithLegacyEnv};
pub use crate::externals::{WithEnv, WithoutEnv};
}

Expand Down
6 changes: 3 additions & 3 deletions lib/api/src/native.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ pub struct NativeFunc<Args = (), Rets = ()> {
definition: FunctionDefinition,
store: Store,
address: *const VMFunctionBody,
vmctx: *const VMContext,
vmctx: VMFunctionEnvironment,
arg_kind: VMFunctionKind,
// exported: ExportFunction,
_phantom: PhantomData<(Args, Rets)>,
Expand All @@ -42,7 +42,7 @@ where
pub(crate) fn new(
store: Store,
address: *const VMFunctionBody,
vmctx: *const VMContext,
vmctx: VMFunctionEnvironment,
arg_kind: VMFunctionKind,
definition: FunctionDefinition,
) -> Self {
Expand Down Expand Up @@ -165,7 +165,7 @@ macro_rules! impl_native_traits {
match self.arg_kind {
VMFunctionKind::Static => {
let results = catch_unwind(AssertUnwindSafe(|| unsafe {
let f = std::mem::transmute::<_, unsafe extern "C" fn( *const VMContext, $( $x, )*) -> Rets::CStruct>(self.address);
let f = std::mem::transmute::<_, unsafe extern "C" fn( VMFunctionEnvironment, $( $x, )*) -> Rets::CStruct>(self.address);
// We always pass the vmctx
f( self.vmctx, $( $x, )* )
})).map_err(|e| RuntimeError::new(format!("{:?}", e)))?;
Expand Down
59 changes: 29 additions & 30 deletions lib/c-api/src/wasm_c_api/externals/function.rs
Original file line number Diff line number Diff line change
Expand Up @@ -100,42 +100,41 @@ pub unsafe extern "C" fn wasm_func_new_with_env(

let func_sig = &function_type.inner().function_type;
let num_rets = func_sig.results().len();
let inner_callback =
move |env: &mut *mut c_void, args: &[Val]| -> Result<Vec<Val>, RuntimeError> {
let processed_args: wasm_val_vec_t = args
.into_iter()
.map(TryInto::try_into)
.collect::<Result<Vec<wasm_val_t>, _>>()
.expect("Argument conversion failed")
.into();

let mut results: wasm_val_vec_t = vec![
wasm_val_t {
kind: wasm_valkind_enum::WASM_I64 as _,
of: wasm_val_inner { int64_t: 0 },
};
num_rets
]
let inner_callback = move |env: &*mut c_void, args: &[Val]| -> Result<Vec<Val>, RuntimeError> {
let processed_args: wasm_val_vec_t = args
.into_iter()
.map(TryInto::try_into)
.collect::<Result<Vec<wasm_val_t>, _>>()
.expect("Argument conversion failed")
.into();

let trap = callback(*env, &processed_args, &mut results);
let mut results: wasm_val_vec_t = vec![
wasm_val_t {
kind: wasm_valkind_enum::WASM_I64 as _,
of: wasm_val_inner { int64_t: 0 },
};
num_rets
]
.into();

if !trap.is_null() {
let trap: Box<wasm_trap_t> = Box::from_raw(trap);
let trap = callback(*env, &processed_args, &mut results);

return Err(trap.inner);
}
if !trap.is_null() {
let trap: Box<wasm_trap_t> = Box::from_raw(trap);

let processed_results = results
.into_slice()
.expect("Failed to convert `results` into a slice")
.into_iter()
.map(TryInto::try_into)
.collect::<Result<Vec<Val>, _>>()
.expect("Result conversion failed");
return Err(trap.inner);
}

Ok(processed_results)
};
let processed_results = results
.into_slice()
.expect("Failed to convert `results` into a slice")
.into_iter()
.map(TryInto::try_into)
.collect::<Result<Vec<Val>, _>>()
.expect("Result conversion failed");

Ok(processed_results)
};

let function = Function::new_with_env(&store.inner, &func_sig, env, inner_callback);

Expand Down
Loading