Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
42 commits
Select commit Hold shift + click to select a range
e10bddd
WIP
cecton Jan 6, 2020
86b6bc5
WIP
cecton Jan 6, 2020
2e61dca
WIP
cecton Jan 6, 2020
ce243f4
WIP
cecton Jan 7, 2020
e19948e
WIP
cecton Jan 7, 2020
cdd65ee
WIP
cecton Jan 7, 2020
935f976
Update client/executor/src/integration_tests/mod.rs
cecton Jan 7, 2020
9ec344c
Update client/executor/src/integration_tests/mod.rs
cecton Jan 7, 2020
5f86a84
WIP
cecton Jan 7, 2020
335038b
WIP
cecton Jan 7, 2020
2333994
WIP
cecton Jan 7, 2020
0ef2746
WIP
cecton Jan 7, 2020
1374979
Use resolver approach
pepyakin Jan 6, 2020
79f4a2a
wasmtime
cecton Jan 7, 2020
997bb60
Revert "wasmtime"
cecton Jan 8, 2020
f7109d1
Revert "Use resolver approach"
cecton Jan 8, 2020
c2f0e0d
WIP
cecton Jan 8, 2020
797bfb6
WIP
cecton Jan 8, 2020
d889cf7
WIP
cecton Jan 8, 2020
cfd9b3b
WIP
cecton Jan 8, 2020
b4b42d9
WIP
cecton Jan 8, 2020
c67be22
Merge remote-tracking branch 'origin/master' into cecton-issue-4456
cecton Jan 8, 2020
0e926a8
Update client/executor/wasmi/src/lib.rs
cecton Jan 8, 2020
4c80f58
WIP
cecton Jan 8, 2020
36d8be2
Update client/executor/wasmi/src/lib.rs
cecton Jan 9, 2020
6786bf8
Update client/executor/wasmi/src/lib.rs
cecton Jan 9, 2020
623787f
Update client/executor/wasmi/src/lib.rs
cecton Jan 9, 2020
000116e
Update client/executor/wasmi/src/lib.rs
cecton Jan 9, 2020
131b7d4
Update client/executor/wasmi/src/lib.rs
cecton Jan 9, 2020
0b67996
Update client/executor/wasmi/src/lib.rs
cecton Jan 9, 2020
2cb0469
Update client/executor/wasmi/src/lib.rs
cecton Jan 9, 2020
a5633f3
Update client/executor/wasmi/src/lib.rs
cecton Jan 9, 2020
cd81107
Update client/executor/wasmi/src/lib.rs
cecton Jan 9, 2020
cc1a4a8
Update client/executor/wasmi/src/lib.rs
cecton Jan 9, 2020
7289892
Update client/executor/wasmi/src/lib.rs
cecton Jan 9, 2020
77fcaa5
Update client/executor/wasmi/src/lib.rs
cecton Jan 9, 2020
3785028
Update client/executor/wasmi/src/lib.rs
cecton Jan 9, 2020
e78e4aa
Update client/executor/wasmi/src/lib.rs
cecton Jan 9, 2020
63dc3ce
Update client/executor/wasmi/src/lib.rs
cecton Jan 9, 2020
04a3db6
WIP
cecton Jan 9, 2020
cc844f5
Apply suggestions from code review
bkchr Jan 9, 2020
199a60e
WIP
cecton Jan 9, 2020
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: 16 additions & 0 deletions client/executor/runtime-test/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,23 @@ use sp_runtime::{print, traits::{BlakeTwo256, Hash}};
#[cfg(not(feature = "std"))]
use sp_core::{ed25519, sr25519};

extern "C" {
#[allow(dead_code)]
fn missing_external();

#[allow(dead_code)]
fn yet_another_missing_external();
}

sp_core::wasm_export_functions! {
fn test_calling_missing_external() {
unsafe { missing_external() }
}

fn test_calling_yet_another_missing_external() {
unsafe { yet_another_missing_external() }
}

fn test_data_in(input: Vec<u8>) -> Vec<u8> {
print("set_storage");
storage::set(b"input", &input);
Expand Down
74 changes: 74 additions & 0 deletions client/executor/src/integration_tests/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,26 @@ fn call_in_wasm<E: Externalities>(
ext,
code,
heap_pages,
false,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Won't the other tests fails because of missing imports? :D

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done (but I think wasmtime is failing at instantiation because of the missing function. I will fix that)

)
}

fn call_in_wasm_with_stub<E: Externalities>(
function: &str,
call_data: &[u8],
execution_method: WasmExecutionMethod,
ext: &mut E,
code: &[u8],
heap_pages: u64,
) -> crate::error::Result<Vec<u8>> {
crate::call_in_wasm::<E, sp_io::SubstrateHostFunctions>(
function,
call_data,
execution_method,
ext,
code,
heap_pages,
true,
)
}

Expand All @@ -68,6 +88,60 @@ fn returning_should_work(wasm_method: WasmExecutionMethod) {
assert_eq!(output, vec![0u8; 0]);
}

#[test_case(WasmExecutionMethod::Interpreted)]
#[cfg_attr(feature = "wasmtime", test_case(WasmExecutionMethod::Compiled))]
#[should_panic(expected = "function test_calling_missing_external does not exist")]
fn call_not_existing_function_with_stub_enabled(wasm_method: WasmExecutionMethod) {
let mut ext = TestExternalities::default();
let mut ext = ext.ext();
let test_code = WASM_BINARY;

let _output = call_in_wasm_with_stub(
Comment thread
cecton marked this conversation as resolved.
Outdated
"test_calling_missing_external",
&[],
wasm_method,
&mut ext,
&test_code[..],
8,
);
}

#[test_case(WasmExecutionMethod::Interpreted)]
#[cfg_attr(feature = "wasmtime", test_case(WasmExecutionMethod::Compiled))]
#[should_panic(expected = "function test_calling_yet_another_missing_external does not exist")]
fn call_yet_another_not_existing_function_with_stub_enabled(wasm_method: WasmExecutionMethod) {
let mut ext = TestExternalities::default();
let mut ext = ext.ext();
let test_code = WASM_BINARY;

let _output = call_in_wasm_with_stub(
Comment thread
cecton marked this conversation as resolved.
Outdated
"test_calling_yet_another_missing_external",
&[],
wasm_method,
&mut ext,
&test_code[..],
8,
);
}

#[test_case(WasmExecutionMethod::Interpreted)]
#[cfg_attr(feature = "wasmtime", test_case(WasmExecutionMethod::Compiled))]
fn call_not_existing_function_without_stub_enabled(wasm_method: WasmExecutionMethod) {
let mut ext = TestExternalities::default();
let mut ext = ext.ext();
let test_code = WASM_BINARY;

let output = call_in_wasm(
"test_calling_missing_external",
&[],
wasm_method,
&mut ext,
&test_code[..],
8,
);
assert!(output.is_err());

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please check for the exact error you expect. Otherwise it could happen that the error type returned changes and the test still finishes successfully.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done but I removed the test entirely as we test only with stub enabled

}

#[test_case(WasmExecutionMethod::Interpreted)]
#[cfg_attr(feature = "wasmtime", test_case(WasmExecutionMethod::Compiled))]
fn panicking_should_work(wasm_method: WasmExecutionMethod) {
Expand Down
3 changes: 3 additions & 0 deletions client/executor/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,12 +67,14 @@ pub fn call_in_wasm<E: Externalities, HF: sp_wasm_interface::HostFunctions>(
ext: &mut E,
code: &[u8],
heap_pages: u64,
enable_stub: bool,
) -> error::Result<Vec<u8>> {
let mut instance = wasm_runtime::create_wasm_runtime_with_code(
execution_method,
heap_pages,
code,
HF::host_functions(),
enable_stub,
)?;
instance.call(ext, function, call_data)
}
Expand Down Expand Up @@ -103,6 +105,7 @@ mod tests {
&mut ext,
&WASM_BINARY,
8,
false,
).unwrap();
assert_eq!(res, vec![0u8; 0]);
}
Expand Down
5 changes: 3 additions & 2 deletions client/executor/src/wasm_runtime.rs
Original file line number Diff line number Diff line change
Expand Up @@ -191,10 +191,11 @@ pub fn create_wasm_runtime_with_code(
heap_pages: u64,
code: &[u8],
host_functions: Vec<&'static dyn Function>,
enable_stub: bool,
) -> Result<Box<dyn WasmRuntime>, WasmError> {
match wasm_method {
WasmExecutionMethod::Interpreted =>
sc_executor_wasmi::create_instance(code, heap_pages, host_functions)
sc_executor_wasmi::create_instance(code, heap_pages, host_functions, enable_stub)
.map(|runtime| -> Box<dyn WasmRuntime> { Box::new(runtime) }),
#[cfg(feature = "wasmtime")]
WasmExecutionMethod::Compiled =>
Expand All @@ -212,7 +213,7 @@ fn create_versioned_wasm_runtime<E: Externalities>(
let code = ext
.original_storage(well_known_keys::CODE)
.ok_or(WasmError::CodeNotFound)?;
let mut runtime = create_wasm_runtime_with_code(wasm_method, heap_pages, &code, host_functions)?;
let mut runtime = create_wasm_runtime_with_code(wasm_method, heap_pages, &code, host_functions, false)?;

// Call to determine runtime version.
let version_result = {
Expand Down
66 changes: 47 additions & 19 deletions client/executor/wasmi/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ struct FunctionExecutor<'a> {
memory: MemoryRef,
table: Option<TableRef>,
host_functions: &'a [&'static dyn Function],
enable_stub: bool,
method: &'a str,
}

impl<'a> FunctionExecutor<'a> {
Expand All @@ -50,13 +52,17 @@ impl<'a> FunctionExecutor<'a> {
heap_base: u32,
t: Option<TableRef>,
host_functions: &'a [&'static dyn Function],
enable_stub: bool,
method: &'a str,
) -> Result<Self, Error> {
Ok(FunctionExecutor {
sandbox_store: sandbox::Store::new(),
heap: allocator::FreeingBumpHeapAllocator::new(heap_base),
memory: m,
table: t,
host_functions,
enable_stub,
method,
})
}
}
Expand Down Expand Up @@ -269,14 +275,17 @@ impl<'a> Sandbox for FunctionExecutor<'a> {
}
}

struct Resolver<'a>(&'a[&'static dyn Function]);
struct Resolver<'a> {
host_functions: &'a[&'static dyn Function],
enable_stub: bool,
}

impl<'a> wasmi::ModuleImportResolver for Resolver<'a> {
fn resolve_func(&self, name: &str, signature: &wasmi::Signature)
-> std::result::Result<wasmi::FuncRef, wasmi::Error>
{
let signature = sp_wasm_interface::Signature::from(signature);
for (function_index, function) in self.0.iter().enumerate() {
for (function_index, function) in self.host_functions.iter().enumerate() {
if name == function.name() {
if signature == function.signature() {
return Ok(
Expand All @@ -295,9 +304,16 @@ impl<'a> wasmi::ModuleImportResolver for Resolver<'a> {
}
}

Err(wasmi::Error::Instantiation(
format!("Export {} not found", name),
))
if self.enable_stub {
trace!("Could not find function {}, a stub will be provided instead.", name);
Comment thread
cecton marked this conversation as resolved.
Outdated
Comment thread
cecton marked this conversation as resolved.
Outdated

// NOTE: provide purposedly an invalid index of the function
Ok(wasmi::FuncInstance::alloc_host(signature.into(), self.host_functions.len()))
} else {
Err(wasmi::Error::Instantiation(
format!("Export {} not found", name),
))
}
}
}

Expand All @@ -306,16 +322,17 @@ impl<'a> wasmi::Externals for FunctionExecutor<'a> {
-> Result<Option<wasmi::RuntimeValue>, wasmi::Trap>
{
let mut args = args.as_ref().iter().copied().map(Into::into);
let function = self.host_functions.get(index).ok_or_else(||
Error::from(
format!("Could not find host function with index: {}", index),
)
)?;

function.execute(self, &mut args)
.map_err(|msg| Error::FunctionExecution(function.name().to_string(), msg))
.map_err(wasmi::Trap::from)
.map(|v| v.map(Into::into))

if let Some(function) = self.host_functions.get(index) {
function.execute(self, &mut args)
.map_err(|msg| Error::FunctionExecution(function.name().to_string(), msg))
.map_err(wasmi::Trap::from)
.map(|v| v.map(Into::into))
} else if self.enable_stub {
panic!("function {} does not exist", self.method);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There can be more than 1 function that does not exist.
And please return an Err

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If I return an Err it doesn't panic anymore and therefore it breaks my test. Does the test should panic?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done (now it unwrap() and catch the panic)

} else {
Err(Error::from(format!("Could not find host function with index: {}", index)).into())
}
}
}

Expand Down Expand Up @@ -351,6 +368,7 @@ fn call_in_wasm_module(
method: &str,
data: &[u8],
host_functions: &[&'static dyn Function],
enable_stub: bool,
) -> Result<Vec<u8>, Error> {
// extract a reference to a linear memory, optional reference to a table
// and then initialize FunctionExecutor.
Expand All @@ -360,7 +378,8 @@ fn call_in_wasm_module(
.and_then(|e| e.as_table().cloned());
let heap_base = get_heap_base(module_instance)?;

let mut fec = FunctionExecutor::new(memory.clone(), heap_base, table, host_functions)?;
let mut fec = FunctionExecutor::new(
memory.clone(), heap_base, table, host_functions, enable_stub, method)?;

// Write the call data
let offset = fec.allocate_memory(data.len() as u32)?;
Expand Down Expand Up @@ -397,8 +416,12 @@ fn instantiate_module(
heap_pages: usize,
module: &Module,
host_functions: &[&'static dyn Function],
enable_stub: bool,
) -> Result<ModuleRef, Error> {
let resolver = Resolver(host_functions);
let resolver = Resolver {
host_functions,
enable_stub,
};
// start module instantiation. Don't run 'start' function yet.
let intermediate_instance = ModuleInstance::new(
module,
Expand Down Expand Up @@ -536,6 +559,8 @@ pub struct WasmiRuntime {
state_snapshot: StateSnapshot,
/// The host functions registered for this instance.
host_functions: Vec<&'static dyn Function>,
/// Enable STUB for function called that are missing
Comment thread
cecton marked this conversation as resolved.
Outdated
enable_stub: bool,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Instead of enable_stub I'd like to suggest to use something like allow_missing_imports. The rationale is that in the wasm spec a missing import is specified an instantiation error, so allow_missing_imports would convey that this is a custom mode. enable_stub is on contrary a little bit vague since there might be different kinds of stubs out there.

Aside, expanding the role of this field in the doc would be great!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fine by me. @bkchr any remark?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I like @pepyakin idea :) Sounds much better!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

}

impl WasmRuntime for WasmiRuntime {
Expand All @@ -561,14 +586,16 @@ impl WasmRuntime for WasmiRuntime {
error!(target: "wasm-executor", "snapshot restoration failed: {}", e);
e
})?;
call_in_wasm_module(ext, &self.instance, method, data, &self.host_functions)
call_in_wasm_module(
ext, &self.instance, method, data, &self.host_functions, self.enable_stub)
}
}

pub fn create_instance(
code: &[u8],
heap_pages: u64,
host_functions: Vec<&'static dyn Function>,
enable_stub: bool,
) -> Result<WasmiRuntime, WasmError> {
let module = Module::from_buffer(&code).map_err(|_| WasmError::InvalidModule)?;

Expand All @@ -579,7 +606,7 @@ pub fn create_instance(
let data_segments = extract_data_segments(&code)?;

// Instantiate this module.
let instance = instantiate_module(heap_pages as usize, &module, &host_functions)
let instance = instantiate_module(heap_pages as usize, &module, &host_functions, enable_stub)
.map_err(|e| WasmError::Instantiation(e.to_string()))?;
Comment thread
bkchr marked this conversation as resolved.
Outdated

// Take state snapshot before executing anything.
Expand All @@ -595,6 +622,7 @@ pub fn create_instance(
instance,
state_snapshot,
host_functions,
enable_stub,
})
}

Expand Down