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

Jump to bottom

c-api: Create RootScope where necessary #8374

Merged
merged 2 commits into from
Apr 15, 2024
Merged

c-api: Create RootScope where necessary #8374

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
58ebadf
c-api: Create `RootScope` where necessary
alexcrichton Apr 15, 2024
347c594
Review comments
alexcrichton Apr 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
23 changes: 12 additions & 11 deletions crates/c-api/src/async.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,9 @@ use std::pin::Pin;
use std::sync::Arc;
use std::task::{Context, Poll};
use std::{ptr, str};

use wasmtime::{AsContextMut, Func, Instance, Result, StackCreator, StackMemory, Trap, Val};
use wasmtime::{
AsContextMut, Func, Instance, Result, RootScope, StackCreator, StackMemory, Trap, Val,
};

use crate::{
bad_utf8, handle_result, to_str, translate_args, wasm_config_t, wasm_functype_t, wasm_trap_t,
Expand Down Expand Up @@ -116,7 +117,7 @@ async fn invoke_c_async_callback<'a>(
params
.iter()
.cloned()
.map(|p| wasmtime_val_t::from_val(&mut caller, p)),
.map(|p| wasmtime_val_t::from_val_unrooted(&mut caller, p)),
);
hostcall_val_storage.extend((0..results.len()).map(|_| wasmtime_val_t {
kind: WASMTIME_I32,
Expand Down Expand Up @@ -155,7 +156,7 @@ async fn invoke_c_async_callback<'a>(
// Translate the `wasmtime_val_t` results into the `results` space
for (i, result) in out_results.iter().enumerate() {
unsafe {
results[i] = result.to_val(&mut caller.caller);
results[i] = result.to_val_unrooted(&mut caller.caller);
}
}
// Move our `vals` storage back into the store now that we no longer
Expand Down Expand Up @@ -218,15 +219,14 @@ fn handle_call_error(
}

async fn do_func_call_async(
mut store: WasmtimeStoreContextMut<'_>,
mut store: RootScope<WasmtimeStoreContextMut<'_>>,
func: &Func,
args: impl ExactSizeIterator<Item = Val>,
results: &mut [MaybeUninit<wasmtime_val_t>],
trap_ret: &mut *mut wasm_trap_t,
err_ret: &mut *mut wasmtime_error_t,
) {
let mut store = store.as_context_mut();
let mut params = mem::take(&mut store.data_mut().wasm_val_storage);
let mut params = mem::take(&mut store.as_context_mut().data_mut().wasm_val_storage);
let (wt_params, wt_results) = translate_args(&mut params, args, results.len());
let result = func.call_async(&mut store, wt_params, wt_results).await;

Expand All @@ -236,15 +236,15 @@ async fn do_func_call_async(
crate::initialize(slot, wasmtime_val_t::from_val(&mut store, val.clone()));
}
params.truncate(0);
store.data_mut().wasm_val_storage = params;
store.as_context_mut().data_mut().wasm_val_storage = params;
}
Err(err) => handle_call_error(err, trap_ret, err_ret),
}
}

#[no_mangle]
pub unsafe extern "C" fn wasmtime_func_call_async<'a>(
mut store: WasmtimeStoreContextMut<'a>,
store: WasmtimeStoreContextMut<'a>,
func: &'a Func,
args: *const wasmtime_val_t,
nargs: usize,
Expand All @@ -253,13 +253,14 @@ pub unsafe extern "C" fn wasmtime_func_call_async<'a>(
trap_ret: &'a mut *mut wasm_trap_t,
err_ret: &'a mut *mut wasmtime_error_t,
) -> Box<wasmtime_call_future_t<'a>> {
let mut scope = RootScope::new(store);
let args = crate::slice_from_raw_parts(args, nargs)
.iter()
.map(|i| i.to_val(&mut store))
.map(|i| i.to_val(&mut scope))
.collect::<Vec<_>>();
let results = crate::slice_from_raw_parts_mut(results, nresults);
let fut = Box::pin(do_func_call_async(
store,
scope,
func,
args.into_iter(),
results,
Expand Down
38 changes: 28 additions & 10 deletions crates/c-api/src/func.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,19 @@ use crate::{
wasmtime_extern_t, wasmtime_val_t, wasmtime_val_union, WasmtimeStoreContext,
WasmtimeStoreContextMut,
};
use crate::{wasm_trap_t, WasmtimeCaller};
use crate::{wasm_trap_t, WasmtimeCaller, WasmtimeStoreData};
use anyhow::{Error, Result};
use std::any::Any;
use std::ffi::c_void;
use std::mem::{self, MaybeUninit};
use std::panic::{self, AssertUnwindSafe};
use std::ptr;
use std::str;
use wasmtime::{AsContextMut, Extern, Func, Trap, Val, ValRaw};
use wasmtime::{
AsContext, AsContextMut, Extern, Func, RootScope, StoreContext, StoreContextMut, Trap, Val,
ValRaw,
};

#[derive(Clone)]
#[repr(transparent)]
pub struct wasm_func_t {
Expand Down Expand Up @@ -203,6 +207,20 @@ pub struct wasmtime_caller_t<'a> {
pub(crate) caller: WasmtimeCaller<'a>,
}

impl AsContext for wasmtime_caller_t<'_> {
type Data = WasmtimeStoreData;

fn as_context(&self) -> StoreContext<'_, WasmtimeStoreData> {
self.caller.as_context()
}
}

impl AsContextMut for wasmtime_caller_t<'_> {
fn as_context_mut(&mut self) -> StoreContextMut<'_, WasmtimeStoreData> {
self.caller.as_context_mut()
}
}

pub type wasmtime_func_callback_t = extern "C" fn(
*mut c_void,
*mut wasmtime_caller_t,
Expand Down Expand Up @@ -253,7 +271,7 @@ pub(crate) unsafe fn c_callback_to_rust_fn(
params
.iter()
.cloned()
.map(|p| wasmtime_val_t::from_val(&mut caller, p)),
.map(|p| wasmtime_val_t::from_val_unrooted(&mut caller, p)),
);
vals.extend((0..results.len()).map(|_| wasmtime_val_t {
kind: crate::WASMTIME_I32,
Expand All @@ -277,7 +295,7 @@ pub(crate) unsafe fn c_callback_to_rust_fn(

// Translate the `wasmtime_val_t` results into the `results` space
for (i, result) in out_results.iter().enumerate() {
results[i] = result.to_val(&mut caller.caller);
results[i] = result.to_val_unrooted(&mut caller);
}

// Move our `vals` storage back into the store now that we no longer
Expand Down Expand Up @@ -329,13 +347,13 @@ pub unsafe extern "C" fn wasmtime_func_call(
nresults: usize,
trap_ret: &mut *mut wasm_trap_t,
) -> Option<Box<wasmtime_error_t>> {
let mut store = store.as_context_mut();
let mut params = mem::take(&mut store.data_mut().wasm_val_storage);
let mut scope = RootScope::new(&mut store);
let mut params = mem::take(&mut scope.as_context_mut().data_mut().wasm_val_storage);
let (wt_params, wt_results) = translate_args(
&mut params,
crate::slice_from_raw_parts(args, nargs)
.iter()
.map(|i| i.to_val(&mut store)),
.map(|i| i.to_val(&mut scope)),
nresults,
);

Expand All @@ -344,16 +362,16 @@ pub unsafe extern "C" fn wasmtime_func_call(
// can. As a result we catch panics here and transform them to traps to
// allow the caller to have any insulation possible against Rust panics.
let result = panic::catch_unwind(AssertUnwindSafe(|| {
func.call(&mut store, wt_params, wt_results)
func.call(&mut scope, wt_params, wt_results)
}));
match result {
Ok(Ok(())) => {
let results = crate::slice_from_raw_parts_mut(results, nresults);
for (slot, val) in results.iter_mut().zip(wt_results.iter()) {
crate::initialize(slot, wasmtime_val_t::from_val(&mut store, val.clone()));
crate::initialize(slot, wasmtime_val_t::from_val(&mut scope, val.clone()));
}
params.truncate(0);
store.data_mut().wasm_val_storage = params;
scope.as_context_mut().data_mut().wasm_val_storage = params;
None
}
Ok(Err(trap)) => store_err(trap, trap_ret),
Expand Down
19 changes: 11 additions & 8 deletions crates/c-api/src/global.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use crate::{
wasmtime_val_t, WasmtimeStoreContext, WasmtimeStoreContextMut,
};
use std::mem::MaybeUninit;
use wasmtime::{Extern, Global};
use wasmtime::{Extern, Global, RootScope};

#[derive(Clone)]
#[repr(transparent)]
Expand Down Expand Up @@ -84,8 +84,9 @@ pub unsafe extern "C" fn wasmtime_global_new(
val: &wasmtime_val_t,
ret: &mut Global,
) -> Option<Box<wasmtime_error_t>> {
let val = val.to_val(&mut store);
let global = Global::new(store, gt.ty().ty.clone(), val);
let mut scope = RootScope::new(&mut store);
let val = val.to_val(&mut scope);
let global = Global::new(scope, gt.ty().ty.clone(), val);
handle_result(global, |global| {
*ret = global;
})
Expand All @@ -101,12 +102,13 @@ pub extern "C" fn wasmtime_global_type(

#[no_mangle]
pub extern "C" fn wasmtime_global_get(
mut store: WasmtimeStoreContextMut<'_>,
store: WasmtimeStoreContextMut<'_>,
global: &Global,
val: &mut MaybeUninit<wasmtime_val_t>,
) {
let gval = global.get(&mut store);
crate::initialize(val, wasmtime_val_t::from_val(store, gval))
let mut scope = RootScope::new(store);
let gval = global.get(&mut scope);
crate::initialize(val, wasmtime_val_t::from_val(&mut scope, gval))
}

#[no_mangle]
Expand All @@ -115,6 +117,7 @@ pub unsafe extern "C" fn wasmtime_global_set(
global: &Global,
val: &wasmtime_val_t,
) -> Option<Box<wasmtime_error_t>> {
let val = val.to_val(&mut store);
handle_result(global.set(store, val), |()| {})
let mut scope = RootScope::new(&mut store);
let val = val.to_val(&mut scope);
handle_result(global.set(scope, val), |()| {})
}
24 changes: 14 additions & 10 deletions crates/c-api/src/table.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use crate::{
};
use anyhow::anyhow;
use std::mem::MaybeUninit;
use wasmtime::{Extern, HeapType, Ref, Table, TableType};
use wasmtime::{Extern, HeapType, Ref, RootScope, Table, TableType};

#[derive(Clone)]
#[repr(transparent)]
Expand Down Expand Up @@ -121,11 +121,12 @@ pub unsafe extern "C" fn wasmtime_table_new(
init: &wasmtime_val_t,
out: &mut Table,
) -> Option<Box<wasmtime_error_t>> {
let mut scope = RootScope::new(&mut store);
handle_result(
init.to_val(&mut store)
init.to_val(&mut scope)
.ref_()
.ok_or_else(|| anyhow!("wasmtime_table_new init value is not a reference"))
.and_then(|init| Table::new(store, tt.ty().ty.clone(), init)),
.and_then(|init| Table::new(scope, tt.ty().ty.clone(), init)),
|table| *out = table,
)
}
Expand All @@ -140,14 +141,15 @@ pub unsafe extern "C" fn wasmtime_table_type(

#[no_mangle]
pub extern "C" fn wasmtime_table_get(
mut store: WasmtimeStoreContextMut<'_>,
store: WasmtimeStoreContextMut<'_>,
table: &Table,
index: u32,
ret: &mut MaybeUninit<wasmtime_val_t>,
) -> bool {
match table.get(&mut store, index) {
let mut scope = RootScope::new(store);
match table.get(&mut scope, index) {
Some(r) => {
crate::initialize(ret, wasmtime_val_t::from_val(store, r.into()));
crate::initialize(ret, wasmtime_val_t::from_val(&mut scope, r.into()));
true
}
None => false,
Expand All @@ -161,11 +163,12 @@ pub unsafe extern "C" fn wasmtime_table_set(
index: u32,
val: &wasmtime_val_t,
) -> Option<Box<wasmtime_error_t>> {
let mut scope = RootScope::new(&mut store);
handle_result(
val.to_val(&mut store)
val.to_val(&mut scope)
.ref_()
.ok_or_else(|| anyhow!("wasmtime_table_set value is not a reference"))
.and_then(|val| table.set(store, index, val)),
.and_then(|val| table.set(scope, index, val)),
|()| {},
)
}
Expand All @@ -183,11 +186,12 @@ pub unsafe extern "C" fn wasmtime_table_grow(
val: &wasmtime_val_t,
prev_size: &mut u32,
) -> Option<Box<wasmtime_error_t>> {
let mut scope = RootScope::new(&mut store);
handle_result(
val.to_val(&mut store)
val.to_val(&mut scope)
.ref_()
.ok_or_else(|| anyhow!("wasmtime_table_grow value is not a reference"))
.and_then(|val| table.grow(store, delta, val)),
.and_then(|val| table.grow(scope, delta, val)),
|prev| *prev_size = prev,
)
}
45 changes: 39 additions & 6 deletions crates/c-api/src/val.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use crate::{
};
use std::mem::{self, ManuallyDrop, MaybeUninit};
use std::ptr;
use wasmtime::{AsContextMut, Func, HeapType, Ref, Val, ValType};
use wasmtime::{AsContextMut, Func, HeapType, Ref, RootScope, Val, ValType};

#[repr(C)]
pub struct wasm_val_t {
Expand Down Expand Up @@ -177,7 +177,24 @@ pub struct wasmtime_func_t {
}

impl wasmtime_val_t {
pub fn from_val(cx: impl AsContextMut, val: Val) -> wasmtime_val_t {
/// Creates a new `wasmtime_val_t` from a `wasmtime::Val`.
///
/// Note that this requires a `RootScope` to be present to serve as proof
/// that `val` is not require to be rooted in the store itself which would
/// prevent GC. Callers should prefer this API where possible, creating a
/// temporary `RootScope` when needed.
pub fn from_val(cx: &mut RootScope<impl AsContextMut>, val: Val) -> wasmtime_val_t {
Self::from_val_unrooted(cx, val)
}

/// Equivalent of [`wasmtime_val_t::from_val`] except that a `RootScope`
/// is not required.
///
/// This method should only be used when a `RootScope` is known to be
/// elsewhere on the stack. For example this is used when we call back out
/// to the embedder. In such a situation we know we previously entered with
/// some other call so the root scope is on the stack there.
pub fn from_val_unrooted(cx: impl AsContextMut, val: Val) -> wasmtime_val_t {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can rename this from_val_unscoped or something? "unrooted" had me convinced that this was passing around ExternRef::to_raw results and stuff like that potentially across GCs which is not what's going on.

Copy link
Member

Choose a reason for hiding this comment

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

Also, the wasmtime_val_t::from_val[_unrooted] constructors never create Rooteds so I don't think we actually need two variants of these ones? Its just creating a Val (which contains a Rooted and means we might be creating a Rooted ourselves) that needs the scoping.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point about the naming, I'll switch to your suggestion.

I thought the same for from_val but I ended up realizing that APIs like wasmtime_global_get also will want a RootScope on them. That's only calling from_val but if we forgot a RootScope there the any value coming out of a global would forever-after be attached to the store (since the Rooted coming out has no other scope). That convinced me to leave the from_val{,_unscoped} distinction to continue to encourage the use of RootScope anywhere in the C API that might use it.

match val {
Val::I32(i) => wasmtime_val_t {
kind: crate::WASMTIME_I32,
Expand Down Expand Up @@ -232,7 +249,22 @@ impl wasmtime_val_t {
}
}

pub unsafe fn to_val(&self, cx: impl AsContextMut) -> Val {
/// Convert this `wasmtime_val_t` into a `wasmtime::Val`.
///
/// See [`wasmtime_val_t::from_val`] for notes on the `RootScope`
/// requirement here. Note that this is particularly meaningful for this
/// API as the `Val` returned may contain a `Rooted<T>` which requires a
/// `RootScope` if we don't want the value to live for the entire lifetime
/// of the `Store`.
pub unsafe fn to_val(&self, cx: &mut RootScope<impl AsContextMut>) -> Val {
self.to_val_unrooted(cx)
}

/// Equivalent of `to_val` except doesn't require a `RootScope`.
///
/// See notes on [`wasmtime_val_t::from_val_unrooted`] for notes on when to
/// use this.
pub unsafe fn to_val_unrooted(&self, cx: impl AsContextMut) -> Val {
match self.kind {
crate::WASMTIME_I32 => Val::I32(self.of.i32),
crate::WASMTIME_I64 => Val::I64(self.of.i64),
Expand Down Expand Up @@ -293,10 +325,11 @@ pub unsafe extern "C" fn wasmtime_val_delete(

#[no_mangle]
pub unsafe extern "C" fn wasmtime_val_copy(
mut cx: WasmtimeStoreContextMut<'_>,
cx: WasmtimeStoreContextMut<'_>,
dst: &mut MaybeUninit<wasmtime_val_t>,
src: &wasmtime_val_t,
) {
let val = src.to_val(&mut cx);
crate::initialize(dst, wasmtime_val_t::from_val(cx, val))
let mut scope = RootScope::new(cx);
let val = src.to_val(&mut scope);
crate::initialize(dst, wasmtime_val_t::from_val(&mut scope, val))
}