Wasm executor should provide stubs for unknown externs (wasmi) #4550

cecton · 2020-01-07T10:29:47Z

This is part of the implementation for #4456, only for wasmi

(Let me squash merge so I can put a proper commit message)

parity-cla-bot · 2020-01-07T10:29:53Z

It looks like @cecton signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

bkchr · 2020-01-07T10:37:21Z

client/executor/wasmi/src/lib.rs

+				.map_err(wasmi::Trap::from)
+				.map(|v| v.map(Into::into))
+		} else if self.enable_stub {
+			panic!("function {} does not exist", self.method);


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

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

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

client/executor/src/integration_tests/mod.rs

client/executor/wasmi/src/lib.rs

bkchr · 2020-01-07T11:52:41Z

client/executor/wasmi/src/lib.rs

+				.map(|v| v.map(Into::into))
+		} else if self.enable_stub {
+			Err(Error::from(
+				format!("functions do not exist: {}", self.missing_functions.join(", "))


Please only print the actual function that was tried to be called (by using the unique index generated in resolve).

If I don't print all the not existing functions, why am I gathering them in a Vec? This implementation here was printing the function that was tried: e19948e

Because you only want to know which function exactly was called. It does not helps us to know that 50 functions are not provided ^^

Ok so after discussion it finally 💡 in my mind. It's the name of the missing export we need, not the name of the function in the module. Will fix this

bkchr · 2020-01-07T11:53:06Z

client/executor/wasmi/src/lib.rs

 			})?;
-		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,


Please put each parameter it its own line.

bkchr · 2020-01-07T11:53:20Z

client/executor/wasmi/src/lib.rs

-		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,
+			&self.missing_functions)


) needs to go to a new line.

Co-Authored-By: Bastian Köcher <[email protected]>

pepyakin · 2020-01-07T15:53:56Z

client/executor/wasmi/src/lib.rs

 	/// The host functions registered for this instance.
 	host_functions: Vec<&'static dyn Function>,
+	/// Enable STUB for function called that are missing
+	enable_stub: bool,


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!

Fine by me. @bkchr any remark?

I like @pepyakin idea :) Sounds much better!

client/executor/wasmi/src/lib.rs

bkchr · 2020-01-08T09:08:25Z

client/executor/src/integration_tests/mod.rs

 		ext,
 		code,
 		heap_pages,
+		false,


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

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

bkchr · 2020-01-08T09:09:11Z

client/executor/src/integration_tests/mod.rs

+		&test_code[..],
+		8,
+	);
+	assert!(output.is_err());


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

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

bkchr · 2020-01-08T09:11:55Z

client/executor/wasmi/src/lib.rs

+		if self.enable_stub {
+			trace!("Could not find function {}, a stub will be provided instead.", name);
+			let id = self.missing_function_id.borrow().clone();
+			self.missing_functions.borrow_mut().insert(id, name.to_string());


We actually just require missing_functions to be a Vec.

The index for the function will be computed as:

self.host_functions.len() + self.missing_functions.len()

(before adding the new missing function name!)

And in invoke we do the following

if index >= host_functions.len() { Err("{} not found whatever", self.missing_functions[index - host_functions.len()) }

Yes that's what I implemented before as you can see here: 2333994

But I told you this looks way too clever and I should probably use a hashmap and you said you would have use a hashmap.

I don't mind reverting it but I'm not in favor of this because I find that it doesn't look like it brings performance improvement, maybe memory, but it certainly make the code harder to understand.

Now that I shared my humble opinion. What do you prefer? 😁

Yeah, I wanted to use a hashmap because I did not thought about this solution 😅
It clearly will improve the performance, as we just need to access the memory by index and not need to look into a hashmap. Please revert, but also make sure that invoke does not tries to use an invalid index.

cecton · 2020-01-08T15:37:43Z

This is ready to be merged. I don't know why that test failed but it doesn't seem related to my changes.

bkchr · 2020-01-08T16:15:15Z

client/executor/src/integration_tests/mod.rs

+		}
+	}
+
+	struct MissingExternalFunction;


You only need one of these structs. Just put the name into the struct as parameter.

client/executor/src/integration_tests/mod.rs

client/executor/wasmi/src/lib.rs

Co-Authored-By: Bastian Köcher <[email protected]>

bkchr

Last round and than we are good to go.

client/executor/wasmi/src/lib.rs

bkchr · 2020-01-09T08:45:13Z

client/executor/wasmi/src/lib.rs


-	let mut fec = FunctionExecutor::new(memory.clone(), heap_base, table, host_functions)?;
+	let mut fec = FunctionExecutor::new(
+		memory.clone(), heap_base, table, host_functions, allow_missing_imports, missing_functions)?;


Please put every parameter on its own line.

bkchr · 2020-01-09T08:45:50Z

client/executor/wasmi/src/lib.rs

 	// Instantiate this module.
-	let instance = instantiate_module(heap_pages as usize, &module, &host_functions)
+	let (instance, missing_functions) = instantiate_module(
+			heap_pages as usize, &module, &host_functions, allow_missing_imports)


Each parameter on its own line.

Co-Authored-By: Bastian Köcher <[email protected]>

client/executor/wasmi/src/lib.rs

cecton added 5 commits January 6, 2020 13:41

WIP

e10bddd

WIP

86b6bc5

WIP

2e61dca

WIP

ce243f4

WIP

e19948e

cecton added J0-enhancement An additional feature request. Z6-mentor An easy task where a mentor is available. Please indicate in the issue who the mentor could be. labels Jan 7, 2020

cecton requested review from bkchr, jimpo and pepyakin January 7, 2020 10:29

cecton self-assigned this Jan 7, 2020

bkchr suggested changes Jan 7, 2020

View reviewed changes

WIP

cdd65ee

bkchr suggested changes Jan 7, 2020

View reviewed changes

cecton and others added 6 commits January 7, 2020 12:54

Update client/executor/src/integration_tests/mod.rs

935f976

Co-Authored-By: Bastian Köcher <[email protected]>

Update client/executor/src/integration_tests/mod.rs

9ec344c

Co-Authored-By: Bastian Köcher <[email protected]>

WIP

5f86a84

WIP

335038b

WIP

2333994

WIP

0ef2746

pepyakin reviewed Jan 7, 2020

View reviewed changes

client/executor/wasmi/src/lib.rs Outdated Show resolved Hide resolved

pepyakin and others added 2 commits January 8, 2020 09:52

Use resolver approach

1374979

wasmtime

79f4a2a

cecton force-pushed the cecton-issue-4456 branch from a9caa1f to 79f4a2a Compare January 8, 2020 09:00

bkchr reviewed Jan 8, 2020

View reviewed changes

bkchr added A5-grumble and removed J0-enhancement An additional feature request. Z6-mentor An easy task where a mentor is available. Please indicate in the issue who the mentor could be. labels Jan 8, 2020

Merge remote-tracking branch 'origin/master' into cecton-issue-4456

c67be22

bkchr suggested changes Jan 8, 2020

View reviewed changes

cecton and others added 2 commits January 8, 2020 17:34

Update client/executor/wasmi/src/lib.rs

0e926a8

Co-Authored-By: Bastian Köcher <[email protected]>

WIP

4c80f58

bkchr reviewed Jan 9, 2020

View reviewed changes

cecton and others added 16 commits January 9, 2020 09:52

Update client/executor/wasmi/src/lib.rs

36d8be2

Co-Authored-By: Bastian Köcher <[email protected]>

Update client/executor/wasmi/src/lib.rs

6786bf8

Co-Authored-By: Bastian Köcher <[email protected]>

Update client/executor/wasmi/src/lib.rs

623787f

Co-Authored-By: Bastian Köcher <[email protected]>

Update client/executor/wasmi/src/lib.rs

000116e

Co-Authored-By: Bastian Köcher <[email protected]>

Update client/executor/wasmi/src/lib.rs

131b7d4

Co-Authored-By: Bastian Köcher <[email protected]>

Update client/executor/wasmi/src/lib.rs

0b67996

Co-Authored-By: Bastian Köcher <[email protected]>

Update client/executor/wasmi/src/lib.rs

2cb0469

Co-Authored-By: Bastian Köcher <[email protected]>

Update client/executor/wasmi/src/lib.rs

a5633f3

Co-Authored-By: Bastian Köcher <[email protected]>

Update client/executor/wasmi/src/lib.rs

cd81107

Co-Authored-By: Bastian Köcher <[email protected]>

Update client/executor/wasmi/src/lib.rs

cc1a4a8

Co-Authored-By: Bastian Köcher <[email protected]>

Update client/executor/wasmi/src/lib.rs

7289892

Co-Authored-By: Bastian Köcher <[email protected]>

Update client/executor/wasmi/src/lib.rs

77fcaa5

Co-Authored-By: Bastian Köcher <[email protected]>

Update client/executor/wasmi/src/lib.rs

3785028

Co-Authored-By: Bastian Köcher <[email protected]>

Update client/executor/wasmi/src/lib.rs

e78e4aa

Co-Authored-By: Bastian Köcher <[email protected]>

Update client/executor/wasmi/src/lib.rs

63dc3ce

Co-Authored-By: Bastian Köcher <[email protected]>

WIP

04a3db6

bkchr approved these changes Jan 9, 2020

View reviewed changes

client/executor/wasmi/src/lib.rs Outdated Show resolved Hide resolved

client/executor/wasmi/src/lib.rs Outdated Show resolved Hide resolved

client/executor/wasmi/src/lib.rs Outdated Show resolved Hide resolved

bkchr and others added 2 commits January 9, 2020 10:08

Apply suggestions from code review

cc844f5

WIP

199a60e

cecton merged commit fc6b7e8 into master Jan 9, 2020

cecton deleted the cecton-issue-4456 branch January 9, 2020 10:18

pepyakin removed the request for review from jimpo January 9, 2020 18:02

pepyakin mentioned this pull request Jan 9, 2020

Don't allow missing imports by default paritytech/polkadot#746

Closed

pepyakin mentioned this pull request Jan 24, 2020

Cursed implementation of allowing missing imports on wasmtime #4730

Merged

Wasm executor should provide stubs for unknown externs (wasmi) #4550

Wasm executor should provide stubs for unknown externs (wasmi) #4550

Uh oh!

Conversation

cecton commented Jan 7, 2020

Uh oh!

parity-cla-bot commented Jan 7, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cecton commented Jan 8, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bkchr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment