Skip to content

Commit

Permalink
Fix running enter/exit hooks on start functions (#3001)
Browse files Browse the repository at this point in the history 
This commit fixes running the store's enter/exit hooks into wasm which
accidentally weren't run for an instance's `start` function. The fix
here was mostly to just sink the enter/exit hook much lower in the code
to `invoke_wasm_and_catch_traps`, which is the common entry point for
all wasm calls.

This did involve propagating the `StoreContext<T>` generic rather than
using `StoreOpaque` unfortunately, but it is overally not too too much
code and we generally wanted most of it inlined anyway.
  • Loading branch information
@alexcrichton
alexcrichton authored Jun 21, 2021
1 parent 18cd2f6 commit 8760bcc
Show file tree
Hide file tree
Showing 6 changed files with 193 additions and 178 deletions.
181 changes: 97 additions & 84 deletions crates/wasmtime/src/func.rs
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use crate::store::{StoreData, StoreOpaque, StoreOpaqueSend, Stored};
use crate::store::{StoreData, StoreInnermost, StoreOpaque, Stored};
use crate::{
AsContext, AsContextMut, Engine, Extern, FuncType, Instance, InterruptHandle, StoreContext,
StoreContextMut, Trap, Val, ValType,
Expand Down Expand Up @@ -681,10 +681,7 @@ impl Func {
"must use `call_async` when async support is enabled on the config",
);
let my_ty = self.ty(&store);
store.as_context_mut().0.exiting_native_hook()?;
let r = self.call_impl(&mut store.as_context_mut().opaque(), my_ty, params);
store.as_context_mut().0.entering_native_hook()?;
r
self.call_impl(&mut store.as_context_mut(), my_ty, params)
}

/// Invokes this function with the `params` given, returning the results
Expand Down Expand Up @@ -719,105 +716,110 @@ impl Func {
where
T: Send,
{
let my_ty = self.ty(&store);
store.as_context_mut().0.exiting_native_hook()?;
let r = self
._call_async(store.as_context_mut().opaque_send(), my_ty, params)
.await;
store.as_context_mut().0.entering_native_hook()?;
r
}

#[cfg(feature = "async")]
async fn _call_async(
&self,
mut store: StoreOpaqueSend<'_>,
my_ty: FuncType,
params: &[Val],
) -> Result<Box<[Val]>> {
let mut store = store.as_context_mut();
assert!(
store.async_support(),
store.0.async_support(),
"cannot use `call_async` without enabling async support in the config",
);
let my_ty = self.ty(&store);
let result = store
.on_fiber(|store| self.call_impl(store, my_ty, params))
.await??;
Ok(result)
}

fn call_impl(
fn call_impl<T>(
&self,
store: &mut StoreOpaque<'_>,
store: &mut StoreContextMut<'_, T>,
my_ty: FuncType,
params: &[Val],
) -> Result<Box<[Val]>> {
let data = &store[self.0];
let trampoline = data.trampoline();
let anyfunc = data.export().anyfunc;
// We need to perform a dynamic check that the arguments given to us
// match the signature of this function and are appropriate to pass to
// this function. This involves checking to make sure we have the right
// number and types of arguments as well as making sure everything is
// from the same `Store`.
if my_ty.params().len() != params.len() {
bail!(
"expected {} arguments, got {}",
my_ty.params().len(),
params.len()
);
}

let mut values_vec = vec![0; max(params.len(), my_ty.results().len())];

// Store the argument values into `values_vec`.
let param_tys = my_ty.params();
for ((arg, slot), ty) in params.iter().cloned().zip(&mut values_vec).zip(param_tys) {
if arg.ty() != ty {
bail!(
"argument type mismatch: found {} but expected {}",
arg.ty(),
ty
);
}
if !arg.comes_from_same_store(store) {
bail!("cross-`Store` values are not currently supported");
}
unsafe {
arg.write_value_to(store, slot);
}
}
let mut values_vec = write_params(&mut store.as_context_mut().opaque(), &my_ty, params)?;

// Call the trampoline.
unsafe {
let anyfunc = anyfunc.as_ref();
let data = &store.0.store_data()[self.0];
let trampoline = data.trampoline();
let anyfunc = data.export().anyfunc;
invoke_wasm_and_catch_traps(store, |callee| {
trampoline(
anyfunc.vmctx,
(*anyfunc.as_ptr()).vmctx,
callee,
anyfunc.func_ptr.as_ptr(),
(*anyfunc.as_ptr()).func_ptr.as_ptr(),
values_vec.as_mut_ptr(),
)
})?;
}

// Load the return values out of `values_vec`.
let mut results = Vec::with_capacity(my_ty.results().len());
for (index, ty) in my_ty.results().enumerate() {
unsafe {
let ptr = values_vec.as_ptr().add(index);
results.push(Val::read_value_from(store, ptr, ty));
return Ok(read_results(
&mut store.as_context_mut().opaque(),
&my_ty,
&values_vec,
));

fn write_params(
store: &mut StoreOpaque<'_>,
ty: &FuncType,
params: &[Val],
) -> Result<Vec<u128>> {
// We need to perform a dynamic check that the arguments given to us
// match the signature of this function and are appropriate to pass to
// this function. This involves checking to make sure we have the right
// number and types of arguments as well as making sure everything is
// from the same `Store`.
if ty.params().len() != params.len() {
bail!(
"expected {} arguments, got {}",
ty.params().len(),
params.len()
);
}

let mut values_vec = vec![0; max(params.len(), ty.results().len())];

// Store the argument values into `values_vec`.
let param_tys = ty.params();
for ((arg, slot), ty) in params.iter().cloned().zip(&mut values_vec).zip(param_tys) {
if arg.ty() != ty {
bail!(
"argument type mismatch: found {} but expected {}",
arg.ty(),
ty
);
}
if !arg.comes_from_same_store(store) {
bail!("cross-`Store` values are not currently supported");
}
unsafe {
arg.write_value_to(store, slot);
}
}

Ok(values_vec)
}

Ok(results.into())
fn read_results(
store: &mut StoreOpaque<'_>,
ty: &FuncType,
values_vec: &[u128],
) -> Box<[Val]> {
let mut results = Vec::with_capacity(ty.results().len());
for (index, ty) in ty.results().enumerate() {
unsafe {
let ptr = &values_vec[index];
results.push(Val::read_value_from(store, ptr, ty));
}
}
results.into()
}
}

#[inline]
pub(crate) fn caller_checked_anyfunc(
&self,
store: &StoreOpaque,
store: &StoreInnermost,
) -> NonNull<VMCallerCheckedAnyfunc> {
store[self.0].export().anyfunc
store.store_data()[self.0].export().anyfunc
}

pub(crate) unsafe fn from_wasmtime_function(
Expand Down Expand Up @@ -1033,28 +1035,39 @@ impl Func {
///
/// The `closure` provided receives a default "callee" `VMContext` parameter it
/// can pass to the called wasm function, if desired.
#[inline]
pub(crate) fn invoke_wasm_and_catch_traps(
store: &mut StoreOpaque<'_>,
pub(crate) fn invoke_wasm_and_catch_traps<T>(
store: &mut StoreContextMut<'_, T>,
closure: impl FnMut(*mut VMContext),
) -> Result<(), Trap> {
unsafe {
let exit = if store.externref_activations_table().stack_canary().is_some() {
let exit = if store
.0
.externref_activations_table()
.stack_canary()
.is_some()
{
false
} else {
enter_wasm(store)?;
true
};

if let Err(trap) = store.0.exiting_native_hook() {
if exit {
exit_wasm(store);
}
return Err(trap);
}
let result = wasmtime_runtime::catch_traps(
store.vminterrupts(),
store.signal_handler(),
store.default_callee(),
store.0.vminterrupts(),
store.0.signal_handler(),
store.0.default_callee(),
closure,
);
if exit {
exit_wasm(store);
}
store.0.entering_native_hook()?;
result.map_err(Trap::from_runtime)
}
}
Expand All @@ -1076,8 +1089,7 @@ pub(crate) fn invoke_wasm_and_catch_traps(
///
/// This function may fail if the the stack limit can't be set because an
/// interrupt already happened.
#[inline]
fn enter_wasm(store: &mut StoreOpaque<'_>) -> Result<(), Trap> {
fn enter_wasm<T>(store: &mut StoreContextMut<'_, T>) -> Result<(), Trap> {
let stack_pointer = psm::stack_pointer() as usize;

// Determine the stack pointer where, after which, any wasm code will
Expand Down Expand Up @@ -1107,7 +1119,7 @@ fn enter_wasm(store: &mut StoreOpaque<'_>) -> Result<(), Trap> {
// synchronize with any other memory it's hoped that the choice of `Relaxed`
// here should be correct for our use case.
let wasm_stack_limit = stack_pointer - store.engine().config().max_wasm_stack;
let interrupts = store.interrupts();
let interrupts = store.0.interrupts();
match interrupts.stack_limit.swap(wasm_stack_limit, Relaxed) {
wasmtime_environ::INTERRUPTED => {
// This means that an interrupt happened before we actually
Expand All @@ -1123,18 +1135,19 @@ fn enter_wasm(store: &mut StoreOpaque<'_>) -> Result<(), Trap> {
n => debug_assert_eq!(usize::max_value(), n),
}
store
.0
.externref_activations_table()
.set_stack_canary(Some(stack_pointer));

Ok(())
}

#[inline]
fn exit_wasm(store: &mut StoreOpaque<'_>) {
store.externref_activations_table().set_stack_canary(None);
fn exit_wasm<T>(store: &mut StoreContextMut<'_, T>) {
store.0.externref_activations_table().set_stack_canary(None);

// see docs above for why this uses `Relaxed`
store
.0
.interrupts()
.stack_limit
.store(usize::max_value(), Relaxed);
Expand Down
37 changes: 19 additions & 18 deletions crates/wasmtime/src/func/typed.rs
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use super::{invoke_wasm_and_catch_traps, HostAbi};
use crate::store::StoreOpaque;
use crate::{AsContextMut, ExternRef, Func, Trap, ValType};
use crate::{AsContextMut, ExternRef, Func, StoreContextMut, Trap, ValType};
use anyhow::{bail, Result};
use std::marker;
use std::mem::{self, MaybeUninit};
Expand Down Expand Up @@ -71,15 +71,12 @@ where
/// This function will panic if it is called when the underlying [`Func`] is
/// connected to an asynchronous store.
pub fn call(&self, mut store: impl AsContextMut, params: Params) -> Result<Results, Trap> {
store.as_context_mut().0.exiting_native_hook()?;
let mut store_opaque = store.as_context_mut().opaque();
let mut store = store.as_context_mut();
assert!(
!store_opaque.async_support(),
!store.0.async_support(),
"must use `call_async` with async stores"
);
let r = unsafe { self._call(&mut store_opaque, params) };
store.as_context_mut().0.entering_native_hook()?;
r
unsafe { self._call(&mut store, params) }
}

/// Invokes this WebAssembly function with the specified parameters.
Expand All @@ -103,24 +100,25 @@ where
where
T: Send,
{
store.as_context_mut().0.exiting_native_hook()?;
let mut store_opaque = store.as_context_mut().opaque_send();
let mut store = store.as_context_mut();
assert!(
store_opaque.async_support(),
store.0.async_support(),
"must use `call` with non-async stores"
);
let r = store_opaque
store
.on_fiber(|store| unsafe { self._call(store, params) })
.await?;
store.as_context_mut().0.entering_native_hook()?;
r
.await?
}

unsafe fn _call(&self, store: &mut StoreOpaque<'_>, params: Params) -> Result<Results, Trap> {
unsafe fn _call<T>(
&self,
store: &mut StoreContextMut<'_, T>,
params: Params,
) -> Result<Results, Trap> {
// Validate that all runtime values flowing into this store indeed
// belong within this store, otherwise it would be unsafe for store
// values to cross each other.
let params = match params.into_abi(store) {
let params = match params.into_abi(&mut store.as_context_mut().opaque()) {
Some(abi) => abi,
None => {
return Err(Trap::new(
Expand All @@ -135,7 +133,7 @@ where
// other side of a C++ shim, so it can never be inlined enough to make
// the memory go away, so the size matters here for performance.
let mut captures = (
self.func.caller_checked_anyfunc(store),
self.func.caller_checked_anyfunc(store.0),
MaybeUninit::uninit(),
params,
false,
Expand All @@ -156,7 +154,10 @@ where
let (_, ret, _, returned) = captures;
debug_assert_eq!(result.is_ok(), returned);
result?;
Ok(Results::from_abi(store, ret.assume_init()))
Ok(Results::from_abi(
&mut store.as_context_mut().opaque(),
ret.assume_init(),
))
}
}

Expand Down
Loading

0 comments on commit 8760bcc

Please sign in to comment.