Additional checks and clarifications in make_trampoline.

core/executor/src/error.rs

@@ -126,7 +126,7 @@ pub enum WasmError {
 	/// The number of heap pages requested is disallowed by the module.
 	InvalidHeapPages,
 	/// Instantiation error.
-	Instantiation(Error),
+	Instantiation(String),
 	/// The compiler does not support the host machine as a target.
 	#[cfg(feature = "wasmtime")]
 	MissingCompilerSupport(&'static str),

core/executor/src/wasmi_execution.rs

@@ -614,7 +614,7 @@ pub fn create_instance<E: Externalities>(ext: &mut E, code: &[u8], heap_pages: u
 
 	// Instantiate this module.
 	let instance = instantiate_module(heap_pages as usize, &module)
-		.map_err(WasmError::Instantiation)?;
+		.map_err(|e| WasmError::Instantiation(e.to_string()))?;
 
 	// Take state snapshot before executing anything.
 	let state_snapshot = StateSnapshot::take(&instance, data_segments, heap_pages)

core/executor/src/wasmtime/runtime.rs

@@ -242,7 +242,7 @@ fn instantiate_env_module(
 			&mut fn_builder_ctx,
 			func_id.index() as u32,
 			&sig,
-		);
+		)?;
 		finished_functions.push(trampoline);
 	}
 

core/executor/src/wasmtime/trampoline.rs

@@ -27,14 +27,21 @@ use wasmtime_runtime::{VMContext, VMFunctionBody};
 use wasm_interface::{HostFunctions, Function, Value, ValueType};
 use std::{cmp, panic, ptr};
 
-use crate::error::{Error, Result};
+use crate::error::{Error, WasmError};
 use crate::wasmtime::function_executor::{FunctionExecutorState, FunctionExecutor};
 
 const CALL_SUCCESS: u32 = 0;
 const CALL_FAILED_WITH_ERROR: u32 = 1;
 const CALL_WITH_BAD_HOST_STATE: u32 = 2;
 const CALL_PANICKED: u32 = 3;
 
+/// A code to trap with that indicates a host call error.
+const TRAP_USER_CODE: u16 = 0;
+
+/// The only Wasm types allowed in host function signatures (I32, I64, F32, F64) are all
+/// represented in at most 8 bytes.
+const MAX_WASM_TYPE_SIZE: usize = 8;
+
 /// The top-level host state of the "env" module. This state is used by the trampoline function to
 /// construct a `FunctionExecutor` which can execute the host call.
 pub struct EnvState {
@@ -79,7 +86,7 @@ unsafe extern "C" fn stub_fn(vmctx: *mut VMContext, func_index: u32, values_vec:
 				&mut state.compiler,
 				state.executor_state.as_mut(),
 				func_index,
-				values_vec
+				values_vec,
 			) {
 				Ok(()) => CALL_SUCCESS,
 				Err(err) => {
@@ -104,7 +111,7 @@ unsafe fn stub_fn_inner(
 	executor_state: Option<&mut FunctionExecutorState>,
 	func_index: u32,
 	values_vec: *mut i64,
-) -> Result<()>
+) -> Result<(), Error>
 {
 	let func = externals.get(func_index as usize)
 		.ok_or_else(|| format!("call to undefined external function with index {}", func_index))?;
@@ -135,33 +142,36 @@ unsafe fn stub_fn_inner(
 ///
 /// The trampoline is a dynamically generated entry point to a runtime host call. The function is
 /// generated by manually constructing Cranelift IR and using the Cranelift compiler. The
-/// trampoline embeds the function index (the `call_id` parameter) as a constant and delegates to
-/// a stub function in Rust, which takes the function index and a memory reference to the stack
-/// arguments and return value slots.
+/// trampoline embeds the function index as a constant and delegates to a stub function in Rust,
+/// which takes the function index and a memory reference to the stack arguments and return value
+/// slots.
 ///
-/// This code is taken entirely from wasmtime's wasmtime-api/src/trampoline/func.rs. It is
-/// deliberately not modified to improve readability in this code base in order to simplify diffs
-/// with the upstream code and updates. We do, however, insert additional comments beginning with
-/// "NOTE:" for clarity when helpful.
+/// This code is of modified copy of wasmtime's wasmtime-api/src/trampoline/func.rs.
 pub fn make_trampoline(
 	isa: &dyn isa::TargetIsa,
 	code_memory: &mut CodeMemory,
 	fn_builder_ctx: &mut FunctionBuilderContext,
-	call_id: u32,
+	func_index: u32,
 	signature: &ir::Signature,
-) -> *const VMFunctionBody {
+) -> Result<*const VMFunctionBody, WasmError> {
 	// Mostly reverse copy of the similar method from wasmtime's
 	// wasmtime-jit/src/compiler.rs.
 	let pointer_type = isa.pointer_type();
 	let mut stub_sig = ir::Signature::new(isa.frontend_config().default_call_conv);
 
+	// Ensure that the first parameter of the generated function is the `VMContext` pointer.
+	assert_eq!(
+		signature.params[0],
+		ir::AbiParam::special(pointer_type, ir::ArgumentPurpose::VMContext)
+	);
+
 	// Add the `vmctx` parameter.
 	stub_sig.params.push(ir::AbiParam::special(
 		pointer_type,
 		ir::ArgumentPurpose::VMContext,
 	));
 
-	// Add the `call_id` parameter.
+	// Add the `func_index` parameter.
 	stub_sig.params.push(ir::AbiParam::new(ir::types::I32));
 
 	// Add the `values_vec` parameter.
@@ -170,21 +180,22 @@ pub fn make_trampoline(
 	// Add error/trap return.
 	stub_sig.returns.push(ir::AbiParam::new(ir::types::I32));
 
-	// NOTE: Each parameter and return value gets a 64-bit (8-byte) wide slot on the stack, as that
-	// is large enough to fit all Wasm primitive types. The `VMContext` pointer, which is a
-	// parameter the function signature, is excluded as it is passed directly to the stub function,
-	// rather than being looked up on the caller stack from the `values_vec` pointer.
-	let values_vec_len = 8 * cmp::max(signature.params.len() - 1, signature.returns.len()) as u32;
+	// Each parameter and return value gets a 64-bit (8-byte) wide slot on the stack, as that is
+	// large enough to fit all Wasm primitive types that can be used in host function signatures.
+	// The `VMContext` pointer, which is a parameter of the function signature, is excluded as it
+	// is passed directly to the stub function rather than being looked up on the caller stack from
+	// the `values_vec` pointer.
+	let values_vec_len = cmp::max(signature.params.len() - 1, signature.returns.len());
+	let values_vec_size = (MAX_WASM_TYPE_SIZE * values_vec_len) as u32;
 
 	let mut context = Context::new();
 	context.func =
 		ir::Function::with_name_signature(ir::ExternalName::user(0, 0), signature.clone());
 
 	let ss = context.func.create_stack_slot(StackSlotData::new(
 		StackSlotKind::ExplicitSlot,
-		values_vec_len,
+		values_vec_size,
 	));
-	let value_size = 8;
 
 	{
 		let mut builder = FunctionBuilder::new(&mut context.func, fn_builder_ctx);
@@ -197,23 +208,19 @@ pub fn make_trampoline(
 		let values_vec_ptr_val = builder.ins().stack_addr(pointer_type, ss, 0);
 		let mflags = ir::MemFlags::trusted();
 		for i in 1..signature.params.len() {
-			if i == 0 {
-				continue;
-			}
-
 			let val = builder.func.dfg.ebb_params(block0)[i];
 			builder.ins().store(
 				mflags,
 				val,
 				values_vec_ptr_val,
-				((i - 1) * value_size) as i32,
+				((i - 1) * MAX_WASM_TYPE_SIZE) as i32,
 			);
 		}
 
 		let vmctx_ptr_val = builder.func.dfg.ebb_params(block0)[0];
-		let call_id_val = builder.ins().iconst(ir::types::I32, call_id as i64);
+		let func_index_val = builder.ins().iconst(ir::types::I32, func_index as i64);
 
-		let callee_args = vec![vmctx_ptr_val, call_id_val, values_vec_ptr_val];
+		let callee_args = vec![vmctx_ptr_val, func_index_val, values_vec_ptr_val];
 
 		let new_sig = builder.import_signature(stub_sig.clone());
 
@@ -225,7 +232,7 @@ pub fn make_trampoline(
 			.call_indirect(new_sig, callee_value, &callee_args);
 
 		let call_result = builder.func.dfg.inst_results(call)[0];
-		builder.ins().trapnz(call_result, TrapCode::User(0));
+		builder.ins().trapnz(call_result, TrapCode::User(TRAP_USER_CODE));
 
 		let mflags = ir::MemFlags::trusted();
 		let mut results = Vec::new();
@@ -234,7 +241,7 @@ pub fn make_trampoline(
 				r.value_type,
 				mflags,
 				values_vec_ptr_val,
-				(i * value_size) as i32,
+				(i * MAX_WASM_TYPE_SIZE) as i32,
 			);
 			results.push(load);
 		}
@@ -243,7 +250,7 @@ pub fn make_trampoline(
 	}
 
 	let mut code_buf: Vec<u8> = Vec::new();
-	let mut reloc_sink = RelocSink {};
+	let mut reloc_sink = RelocSink;
 	let mut trap_sink = binemit::NullTrapSink {};
 	let mut stackmap_sink = binemit::NullStackmapSink {};
 	context
@@ -254,17 +261,18 @@ pub fn make_trampoline(
 			&mut trap_sink,
 			&mut stackmap_sink,
 		)
-		.expect("compile_and_emit");
+		.map_err(|e| WasmError::Instantiation(format!("failed to compile trampoline: {}", e)))?;
 
-	code_memory
+	let func_ref = code_memory
 		.allocate_copy_of_byte_slice(&code_buf)
-		.expect("allocate_copy_of_byte_slice")
-		.as_ptr()
+		.map_err(|e| WasmError::Instantiation(format!("failed to allocate code memory: {}", e)))?;
+
+	Ok(func_ref.as_ptr())
 }
 
 /// We don't expect trampoline compilation to produce any relocations, so
 /// this `RelocSink` just asserts that it doesn't recieve any.
-struct RelocSink {}
+struct RelocSink;
 
 impl binemit::RelocSink for RelocSink {
 	fn reloc_ebb(