Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.
Closed
Show file tree
Hide file tree
Changes from all 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
16 changes: 15 additions & 1 deletion client/executor/wasmi/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ struct FunctionExecutor<'a> {
host_functions: &'a [&'static dyn Function],
allow_missing_func_imports: bool,
missing_functions: &'a [String],
instance_panicked: Option<Error>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Could be just panicked maybe?

}

impl<'a> FunctionExecutor<'a> {
Expand All @@ -65,8 +66,13 @@ impl<'a> FunctionExecutor<'a> {
host_functions,
allow_missing_func_imports,
missing_functions,
instance_panicked: None,
})
}

fn panicked(&mut self) -> Option<Error> {
self.instance_panicked.take()
}
}

impl<'a> sandbox::SandboxCapabilities for FunctionExecutor<'a> {
Expand Down Expand Up @@ -124,6 +130,10 @@ impl<'a> FunctionContext for FunctionExecutor<'a> {
fn sandbox(&mut self) -> &mut dyn Sandbox {
self
}

fn instance_panicked(&mut self, message: &str) {
self.instance_panicked = Some(Error::RuntimePanicked(message.into()));
}
}

impl<'a> Sandbox for FunctionExecutor<'a> {
Expand Down Expand Up @@ -503,7 +513,11 @@ fn call_in_wasm_module(
"Failed to execute code with {} pages",
memory.current_size().0,
);
Err(e.into())

match function_executor.panicked() {
Some(e) => Err(e),
None => Err(e.into()),
}
},
_ => Err(Error::InvalidReturn),
}
Expand Down
11 changes: 11 additions & 0 deletions client/executor/wasmtime/src/host.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ pub struct HostState {
sandbox_store: RefCell<sandbox::Store<SupervisorFuncRef>>,
allocator: RefCell<FreeingBumpHeapAllocator>,
instance: Rc<InstanceWrapper>,
instance_panicked: RefCell<Option<Error>>,
}

impl HostState {
Expand All @@ -63,13 +64,19 @@ impl HostState {
sandbox_store: RefCell::new(sandbox::Store::new()),
allocator: RefCell::new(allocator),
instance,
instance_panicked: RefCell::new(None),
}
}

/// Materialize `HostContext` that can be used to invoke a substrate host `dyn Function`.
pub fn materialize<'a>(&'a self) -> HostContext<'a> {
HostContext(self)
}

/// Returns an error if the wasm instance informed about us about itself panicking.
fn panicked(&mut self) -> Option<Error> {
self.instance_panicked.borrow_mut().take()
}
}

/// A `HostContext` implements `FunctionContext` for making host calls from a Wasmtime
Expand Down Expand Up @@ -156,6 +163,10 @@ impl<'a> sp_wasm_interface::FunctionContext for HostContext<'a> {
fn sandbox(&mut self) -> &mut dyn Sandbox {
self
}

fn instance_panicked(&mut self, message: &str) {
*self.instance_panicked.borrow_mut() = Some(Error::RuntimePanicked(message.into()));
}
}

impl<'a> Sandbox for HostContext<'a> {
Expand Down
19 changes: 9 additions & 10 deletions client/executor/wasmtime/src/instance_wrapper.rs
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ impl EntryPoint {
let data_ptr = u32::from(data_ptr) as i32;
let data_len = u32::from(data_len) as i32;

(match self.call_type {
match self.call_type {
EntryPointType::Direct => {
self.func.call(&[
wasmtime::Val::I32(data_ptr),
Expand All @@ -112,15 +112,14 @@ impl EntryPoint {
wasmtime::Val::I32(data_len),
])
},
})
.map(|results|
// the signature is checked to have i64 return type
results[0].unwrap_i64() as u64
)
.map_err(|err| Error::from(format!(
"Wasm execution trapped: {}",
err
)))
}.map(|results|
// the signature is checked to have i64 return type
results[0].unwrap_i64() as u64
)
.map_err(|err| Error::from(format!(
"Wasm execution trapped: {}",
err,
)))
}

pub fn direct(func: wasmtime::Func) -> std::result::Result<Self, &'static str> {
Expand Down
13 changes: 9 additions & 4 deletions client/executor/wasmtime/src/runtime.rs
Original file line number Diff line number Diff line change
Expand Up @@ -153,10 +153,15 @@ fn perform_call(
) -> Result<Vec<u8>> {
let (data_ptr, data_len) = inject_input_data(&instance_wrapper, &mut allocator, data)?;

let host_state = HostState::new(allocator, instance_wrapper.clone());
let ret = state_holder::with_initialized_state(&host_state, || -> Result<_> {
Ok(unpack_ptr_and_len(entrypoint.call(data_ptr, data_len)?))
});
let mut host_state = HostState::new(allocator, instance_wrapper.clone());
let ret = state_holder::with_initialized_state(&host_state, ||
entrypoint.call(data_ptr, data_len).map(unpack_ptr_and_len)
);

if let Some(err) = host_state.panicked() {
return Err(err);
}

let (output_ptr, output_len) = ret?;
let output = extract_output_data(&instance_wrapper, output_ptr, output_len)?;

Expand Down
11 changes: 11 additions & 0 deletions primitives/io/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1007,6 +1007,17 @@ trait Allocator {
}
}

/// Wasm only panic handler.
#[runtime_interface(wasm_only)]
trait PanicHandler {
/// Should be called when the wasm instance is panicking.
///
/// The given `message` should correspond to the reason the instance panicked.
Copy link
Contributor

Choose a reason for hiding this comment

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

Some points that should be IMO addressed in the docs:

  1. It's not mandatory to call this function when "panic" happens, but if the user does it, then they will get a proper message and not a message some general trap message.
  2. panicking is a Rust term. I would suggest we use the pharsing: "when the wasm instance entered an unrecoverable state and needs to terminate" or something along these lines.
  3. there is nothing said what are the semantics if called more than once. The last call wins?

UPD: I also see in the code that if this function was called at least once, then it will return an error, even if there were no traps within the call. That should be either documented as well.

Alternatively, we could just give trapping semantics to this call. I.e. when it's called it stashes the message and then traps.

UPD2: I see that wasmi doesn't have the same behavior. If panicked was called but no trap happened we do return with success.

fn panicking(&mut self, message: &str) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe just panicked for consistency?

self.instance_panicked(message);
}
}

/// Interface that provides functions for logging from within the runtime.
#[runtime_interface]
pub trait Logging {
Expand Down
6 changes: 5 additions & 1 deletion primitives/wasm-interface/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,9 @@ impl PartialEq for dyn Function {
}
}

/// Context used by `Function` to interact with the allocator and the memory of the wasm instance.
/// Context used by [`Function`] to interact with the executing context.
///
/// This includes access to the memory and the sandbox.
pub trait FunctionContext {
/// Read memory from `address` into a vector.
fn read_memory(&self, address: Pointer<u8>, size: WordSize) -> Result<Vec<u8>> {
Expand All @@ -290,6 +292,8 @@ pub trait FunctionContext {
fn deallocate_memory(&mut self, ptr: Pointer<u8>) -> Result<()>;
/// Provides access to the sandbox.
fn sandbox(&mut self) -> &mut dyn Sandbox;
/// The wasm instance panicked with the given `message`.
fn instance_panicked(&mut self, message: &str);
}

/// Sandbox memory identifier.
Expand Down