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

Change proc_exit to unwind the stack rather than exiting the host process. #1646

Merged
8 changes: 1 addition & 7 deletions cranelift/codegen/src/ir/trapcode.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,6 @@ pub enum TrapCode {
/// A `table_addr` instruction detected an out-of-bounds error.
TableOutOfBounds,

/// Other bounds checking error.
OutOfBounds,

/// Indirect call to a null table entry.
IndirectCallToNull,

Expand Down Expand Up @@ -63,7 +60,6 @@ impl Display for TrapCode {
StackOverflow => "stk_ovf",
HeapOutOfBounds => "heap_oob",
TableOutOfBounds => "table_oob",
OutOfBounds => "oob",
IndirectCallToNull => "icall_null",
BadSignature => "bad_sig",
IntegerOverflow => "int_ovf",
Expand All @@ -86,7 +82,6 @@ impl FromStr for TrapCode {
"stk_ovf" => Ok(StackOverflow),
"heap_oob" => Ok(HeapOutOfBounds),
"table_oob" => Ok(TableOutOfBounds),
"oob" => Ok(OutOfBounds),
"icall_null" => Ok(IndirectCallToNull),
"bad_sig" => Ok(BadSignature),
"int_ovf" => Ok(IntegerOverflow),
Expand All @@ -106,11 +101,10 @@ mod tests {
use alloc::string::ToString;

// Everything but user-defined codes.
const CODES: [TrapCode; 11] = [
const CODES: [TrapCode; 10] = [
TrapCode::StackOverflow,
TrapCode::HeapOutOfBounds,
TrapCode::TableOutOfBounds,
TrapCode::OutOfBounds,
TrapCode::IndirectCallToNull,
TrapCode::BadSignature,
TrapCode::IntegerOverflow,
Expand Down
4 changes: 2 additions & 2 deletions cranelift/filetests/filetests/parser/flags.clif
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ block201:
return

block202:
trap oob
trap heap_oob
}
; check: v1 = ifcmp_imm v0, 17
; check: brif eq v1, block201
Expand Down Expand Up @@ -56,7 +56,7 @@ block201:
return

block202:
trap oob
trap heap_oob
}
; check: v2 = ffcmp v0, v1
; check: brff eq v2, block201
Expand Down
4 changes: 2 additions & 2 deletions cranelift/filetests/filetests/parser/tiny.clif
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ function %cond_traps(i32) {
block0(v0: i32):
trapz v0, stk_ovf
v1 = ifcmp_imm v0, 5
trapif ugt v1, oob
trapif ugt v1, heap_oob
v2 = bitcast.f32 v1
v3 = ffcmp v2, v2
trapff uno v3, int_ovf
Expand All @@ -233,7 +233,7 @@ block0(v0: i32):
; nextln: block0(v0: i32):
; nextln: trapz v0, stk_ovf
; nextln: v1 = ifcmp_imm v0, 5
; nextln: trapif ugt v1, oob
; nextln: trapif ugt v1, heap_oob
; nextln: v2 = bitcast.f32 v1
; nextln: v3 = ffcmp v2, v2
; nextln: trapff uno v3, int_ovf
Expand Down
2 changes: 1 addition & 1 deletion cranelift/filetests/filetests/regalloc/iterate.clif
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ block0(v0: i64):
jump block4

block4:
trap oob
trap heap_oob

block2:
v8 = load.i64 v5+8
Expand Down
2 changes: 1 addition & 1 deletion cranelift/filetests/filetests/wasm/control.clif
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ block0(v0: i32):
br_table v0, block4, jt0

block4:
trap oob
trap heap_oob

block1:
return
Expand Down
7 changes: 0 additions & 7 deletions crates/wasi-common/src/old/snapshot_0/hostcalls_impl/misc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -342,13 +342,6 @@ pub(crate) struct FdEventData<'a> {
pub(crate) userdata: wasi::__wasi_userdata_t,
}

pub(crate) fn proc_exit(_wasi_ctx: &WasiCtx, _memory: &mut [u8], rval: wasi::__wasi_exitcode_t) {
trace!("proc_exit(rval={:?})", rval);
// TODO: Rather than call std::process::exit here, we should trigger a
// stack unwind similar to a trap.
std::process::exit(rval as i32);
}

pub(crate) fn proc_raise(
_wasi_ctx: &WasiCtx,
_memory: &mut [u8],
Expand Down
11 changes: 4 additions & 7 deletions crates/wasi-common/src/snapshots/wasi_snapshot_preview1.rs
Original file line number Diff line number Diff line change
Expand Up @@ -813,13 +813,10 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx {
Ok(nevents)
}

// This is just a temporary to ignore the warning which becomes a hard error
// in the CI. Once we figure out non-returns in `wiggle`, this should be gone.
#[allow(unreachable_code)]
fn proc_exit(&self, rval: types::Exitcode) -> std::result::Result<(), ()> {
// TODO: Rather than call std::process::exit here, we should trigger a
// stack unwind similar to a trap.
std::process::exit(rval as i32);
fn proc_exit(&self, _rval: types::Exitcode) -> std::result::Result<(), ()> {
// proc_exit is special in that it's expected to unwind the stack, which
// typically requires runtime-specific logic.
unimplemented!("runtimes are expected to override this implementation")
}

fn proc_raise(&self, _sig: types::Signal) -> Result<()> {
Expand Down
7 changes: 7 additions & 0 deletions crates/wasi-common/wig/src/hostcalls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,13 @@ pub fn define(args: TokenStream) -> TokenStream {

for module in doc.modules() {
for func in module.funcs() {
// `proc_exit` is special; it's essentially an unwinding primitive,
// so we implement it in the runtime rather than use the implementation
// in wasi-common.
if func.name.as_str() == "proc_exit" {
continue;
}

ret.extend(generate_wrappers(&func, old));
}
}
Expand Down
18 changes: 18 additions & 0 deletions crates/wasi-common/wig/src/wasi.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,15 @@ pub fn define_struct(args: TokenStream) -> TokenStream {
linker_add.push(quote! {
linker.define(#module_name, #name, self.#name_ident.clone())?;
});
// `proc_exit` is special; it's essentially an unwinding primitive,
// so we implement it in the runtime rather than use the implementation
// in wasi-common.
if name == "proc_exit" {
ctor_externs.push(quote! {
let #name_ident = wasmtime::Func::wrap(store, crate::wasi_proc_exit);
});
continue;
}

let mut shim_arg_decls = Vec::new();
let mut params = Vec::new();
Expand Down Expand Up @@ -291,6 +300,15 @@ pub fn define_struct_for_wiggle(args: TokenStream) -> TokenStream {
linker_add.push(quote! {
linker.define(#module_name, #name, self.#name_ident.clone())?;
});
// `proc_exit` is special; it's essentially an unwinding primitive,
// so we implement it in the runtime rather than use the implementation
// in wasi-common.
if name == "proc_exit" {
ctor_externs.push(quote! {
let #name_ident = wasmtime::Func::wrap(store, crate::wasi_proc_exit);
});
continue;
}

let mut shim_arg_decls = Vec::new();
let mut params = Vec::new();
Expand Down
16 changes: 16 additions & 0 deletions crates/wasi/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use wasmtime::Trap;

pub mod old;

pub use wasi_common::{WasiCtx, WasiCtxBuilder};
Expand All @@ -12,3 +14,17 @@ pub fn is_wasi_module(name: &str) -> bool {
// trick.
name.starts_with("wasi")
}

/// Implement the WASI `proc_exit` function. This function is implemented here
/// instead of in wasi-common so that we can use the runtime to perform an
/// unwind rather than exiting the host process.
fn wasi_proc_exit(status: i32) -> Result<(), Trap> {
// Check that the status is within WASI's range.
if status >= 0 && status < 126 {
Err(Trap::i32_exit(status))
} else {
Err(Trap::new(
"exit with invalid exit status outside of [0..126)",
))
}
}
2 changes: 1 addition & 1 deletion crates/wasmtime/src/externals.rs
Original file line number Diff line number Diff line change
Expand Up @@ -420,7 +420,7 @@ impl Table {
let src_table = src_table.instance.get_defined_table(src_table_index);

runtime::Table::copy(dst_table, src_table, dst_index, src_index, len)
.map_err(Trap::from_jit)?;
.map_err(Trap::from_runtime)?;
Ok(())
}

Expand Down
24 changes: 12 additions & 12 deletions crates/wasmtime/src/func.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@ use std::mem;
use std::panic::{self, AssertUnwindSafe};
use std::ptr;
use std::rc::Weak;
use wasmtime_runtime::{raise_user_trap, ExportFunction, VMTrampoline};
use wasmtime_runtime::{Export, InstanceHandle, VMContext, VMFunctionBody};
use wasmtime_runtime::{ExportFunction, VMTrampoline};

/// A WebAssembly function which can be called.
///
Expand Down Expand Up @@ -747,7 +747,7 @@ pub(crate) fn catch_traps(
signalhandler.as_deref(),
closure,
)
.map_err(Trap::from_jit)
.map_err(Trap::from_runtime)
}
}

Expand Down Expand Up @@ -941,7 +941,7 @@ unsafe impl<T: WasmTy> WasmRet for Result<T, Trap> {
}

fn handle_trap(trap: Trap) -> ! {
unsafe { wasmtime_runtime::raise_user_trap(Box::new(trap)) }
unsafe { raise_user_trap(trap.into()) }
}
}

Expand Down Expand Up @@ -1133,9 +1133,9 @@ macro_rules! impl_into_func {
R::push(&mut ret);
let ty = FuncType::new(_args.into(), ret.into());
let store_weak = store.weak();
unsafe {
let trampoline = trampoline::<$($args,)* R>;
let (instance, export) = crate::trampoline::generate_raw_func_export(
let trampoline = trampoline::<$($args,)* R>;
let (instance, export) = unsafe {
crate::trampoline::generate_raw_func_export(
&ty,
std::slice::from_raw_parts_mut(
shim::<F, $($args,)* R> as *mut _,
Expand All @@ -1145,12 +1145,12 @@ macro_rules! impl_into_func {
store,
Box::new((self, store_weak)),
)
.expect("failed to generate export");
Func {
instance,
export,
trampoline,
}
.expect("failed to generate export")
};
Func {
instance,
export,
trampoline,
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion crates/wasmtime/src/instance.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ fn instantiate(
.map_err(|e| -> Error {
match e {
InstantiationError::StartTrap(trap) | InstantiationError::Trap(trap) => {
Trap::from_jit(trap).into()
Trap::from_runtime(trap).into()
}
other => other.into(),
}
Expand Down
2 changes: 1 addition & 1 deletion crates/wasmtime/src/trampoline/func.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ unsafe extern "C" fn stub_fn(

// If a trap was raised (an error returned from the imported function)
// then we smuggle the trap through `Box<dyn Error>` through to the
// call-site, which gets unwrapped in `Trap::from_jit` later on as we
// call-site, which gets unwrapped in `Trap::from_runtime` later on as we
// convert from the internal `Trap` type to our own `Trap` type in this
// crate.
Ok(Err(trap)) => wasmtime_runtime::raise_user_trap(Box::new(trap)),
Expand Down
61 changes: 53 additions & 8 deletions crates/wasmtime/src/trap.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,27 @@ pub struct Trap {
inner: Arc<TrapInner>,
}

/// State describing the occasion which evoked a trap.
#[derive(Debug)]
enum TrapReason {
/// An error message describing a trap.
Message(String),

/// An `i32` exit status describing an explicit program exit.
I32Exit(i32),
}

impl fmt::Display for TrapReason {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match self {
TrapReason::Message(s) => write!(f, "{}", s),
TrapReason::I32Exit(status) => write!(f, "Exited with i32 exit status {}", status),
}
}
}

struct TrapInner {
message: String,
reason: TrapReason,
wasm_trace: Vec<FrameInfo>,
native_trace: Backtrace,
}
Expand All @@ -34,9 +53,21 @@ impl Trap {
Trap::new_with_trace(&info, None, message.into(), Backtrace::new_unresolved())
}

pub(crate) fn from_jit(jit: wasmtime_runtime::Trap) -> Self {
/// Creates a new `Trap` representing an explicit program exit with a classic `i32`
/// exit status value.
pub fn i32_exit(status: i32) -> Self {
Trap {
inner: Arc::new(TrapInner {
reason: TrapReason::I32Exit(status),
wasm_trace: Vec::new(),
native_trace: Backtrace::from(Vec::new()),
}),
}
}

pub(crate) fn from_runtime(runtime_trap: wasmtime_runtime::Trap) -> Self {
let info = FRAME_INFO.read().unwrap();
match jit {
match runtime_trap {
wasmtime_runtime::Trap::User(error) => {
// Since we're the only one using the wasmtime internals (in
// theory) we should only see user errors which were originally
Expand Down Expand Up @@ -85,7 +116,6 @@ impl Trap {
StackOverflow => "call stack exhausted",
HeapOutOfBounds => "out of bounds memory access",
TableOutOfBounds => "undefined element: out of bounds table access",
OutOfBounds => "out of bounds",
IndirectCallToNull => "uninitialized element",
BadSignature => "indirect call type mismatch",
IntegerOverflow => "integer overflow",
Expand Down Expand Up @@ -128,16 +158,31 @@ impl Trap {
}
Trap {
inner: Arc::new(TrapInner {
message,
reason: TrapReason::Message(message),
wasm_trace,
native_trace,
}),
}
}

/// Returns a reference the `message` stored in `Trap`.
///
/// In the case of an explicit exit, the exit status can be obtained by
/// calling `i32_exit_status`.
pub fn message(&self) -> &str {
&self.inner.message
match &self.inner.reason {
TrapReason::Message(message) => message,
TrapReason::I32Exit(_) => "explicitly exited",
}
}

/// If the trap was the result of an explicit program exit with a classic
/// `i32` exit status value, return the value, otherwise return `None`.
pub fn i32_exit_status(&self) -> Option<i32> {
match self.inner.reason {
TrapReason::I32Exit(status) => Some(status),
_ => None,
}
}

/// Returns a list of function frames in WebAssembly code that led to this
Expand All @@ -150,7 +195,7 @@ impl Trap {
impl fmt::Debug for Trap {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
f.debug_struct("Trap")
.field("message", &self.inner.message)
.field("reason", &self.inner.reason)
.field("wasm_trace", &self.inner.wasm_trace)
.field("native_trace", &self.inner.native_trace)
.finish()
Expand All @@ -159,7 +204,7 @@ impl fmt::Debug for Trap {

impl fmt::Display for Trap {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
write!(f, "{}", self.inner.message)?;
write!(f, "{}", self.inner.reason)?;
let trace = self.trace();
if trace.is_empty() {
return Ok(());
Expand Down
Loading