executor: Migrate wasmtime backend to a high-level API #4686

pepyakin · 2020-01-20T14:18:21Z

This PR is basically a refactoring that migrates the code from wasmtime-runtime, wasmtime-jit et al to the now stable wasmtime crate. The crate is now sprinkled with comments. Hopefully, one now doesn't have to have a black belt in unsafejitsu to deal with this crate.

Note that this PR relies on some fixes on the wasmtime side, hence the git reference.

This PR should not introduce any significant functional changes and should have the same behavior.

TODO:

Verify that it syncs on Kusama.
Provide a companion PR for Polkadot
Update to upstream version of wasmtime

This may solve a problem with segfaults.

tomusdrw

My WASM / wasmtime / low level knowledge is pretty limited, but it looks reasonable. I like the unsafe abstractions introduced and the docs are very convincing that everything is correct :)

client/executor/common/src/sandbox.rs

client/executor/wasmtime/src/host.rs

tomusdrw · 2020-01-24T14:30:54Z

client/executor/wasmtime/src/imports.rs

+			// available in substrate.
+			//
+			// This, however, cannot happen since the signature of this function is created from
+			// a `dyn Function` signature of which cannot have a non substrate value by definition.


What if the values that we support change over time? Would the documentation above still be valid?

I guess no. But I am not sure what would be a better way?

tomusdrw · 2020-01-24T14:34:35Z

client/executor/wasmtime/src/instance_wrapper.rs

+	pub fn extract_heap_base(&self) -> Result<u32> {
+		let heap_base_export = self
+			.instance
+			.get_export("__heap_base")


Should stuff like __heap_base rather be declared as some constants? Usually I prefer that over random strings sprinkled all over, at least you can maintain some convention, even if the constant is only used in one place.

That would also give a place to leave some docs!

I think it would be a good idea to address this in a follow-up PR!

Fine with me!

client/executor/wasmtime/src/state_holder.rs

Co-Authored-By: Tomasz Drwięga <[email protected]>

Demi-Marie

There are still many places where code could be made safer. I don’t see any explicit unsoundness, but I also am not convinced that the code is safe either.

client/executor/wasmtime/Cargo.toml

client/executor/wasmtime/src/imports.rs

Demi-Marie · 2020-01-31T21:21:01Z

client/executor/wasmtime/src/instance_wrapper.rs

+	/// # Safety
+	///
+	/// This wrapper requires exclusive ownership over the given instance and all its components.
+	/// Since `Instance` is a `Clone` we cannot use typesystem to prevent this, hence unsafe.


We can (and should) use RefCell or Mutex for this.

I am not sure if I agree: how would they help from violating the invariant? It would still be possible to clone the memory instance from within this module and create aliases, right?

client/executor/wasmtime/src/instance_wrapper.rs

Demi-Marie · 2020-01-31T21:23:48Z

client/executor/wasmtime/src/instance_wrapper.rs

+	///
+	/// Wasmtime doesn't provide comprehensive documentation about the exact behavior of the data
+	/// pointer. If a dynamic style heap is used the base pointer of the heap can change. Since
+	/// growing, we cannot guarantee the lifetime of the returned slice reference.


A better approach would be for this to be a function taking a callback, such that the reference cannot escape the callback.

Demi-Marie · 2020-01-31T21:23:55Z

client/executor/wasmtime/src/instance_wrapper.rs

+	/// See `[memory_as_slice]`. In addition to those requirements, since a mutable reference is
+	/// returned it must be ensured that only one mutable and no shared references to memory exists
+	/// at the same time.
+	unsafe fn memory_as_slice_mut(&self) -> &mut [u8] {


client/executor/wasmtime/src/runtime.rs

# Conflicts: # Cargo.lock # client/executor/common/src/sandbox.rs # client/executor/wasmi/src/lib.rs # client/executor/wasmtime/src/function_executor.rs # client/executor/wasmtime/src/runtime.rs # client/executor/wasmtime/src/util.rs

bkchr

👍

client/executor/wasmtime/Cargo.toml

client/executor/wasmtime/src/imports.rs

bkchr · 2020-02-13T12:33:07Z

client/executor/wasmtime/src/imports.rs

+
+		let resolved = match import_ty.name() {
+			"memory" => {
+				memory_import_index = Some(externs.len());


Should we error when a second memory instance is requested?

I am not sure!

First, it won't be possible atm, since if the module has 2 memory imports then it is a not valid module. Second, as soon as multiple memories are supported then it is OK (although useless) to import the same memory twice.

client/executor/wasmtime/src/state_holder.rs

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

* Migrate wasmtime backend to wasmtime-api * Port to a newer version of wasmtime * Update to the latest changes. * Rejig the sandbox module a bit * Materialze * Fixes. * executor wasm_runtime fix * Refactor everything * More refactoring * Even more refactorings * More cleaning. * Update to the latest wasmtime * Reformat * Renames * Refactoring and comments. * Docs * Rename FunctionExecutor to host. * Imrpove docs. * fmt * Remove panic * Assert the number of arguments are equal between wasmtime and hostfunc. * Comment a possible panic if there is no corresponding value variant. * Check signature of the entrypoint. * Use git version of wasmtime * Refine and doc the sandbox code. * Comment RefCells. * Update wasmtime to the latest-ish master. This may solve a problem with segfaults. * Apply suggestions from code review Co-Authored-By: Tomasz Drwięga <[email protected]> * Use full SHA1 hash of wasmtime commit. * Add a panic message. * Add some documentation * Update wasmtime version to include SIGSEGV fix * Update to crates.io version of wasmtime * Make it work. * Move the creation of memory into `InstanceWrapper::new` * Make `InstanceWrapper` !Send & !Sync * Avoid using `take_mut` * Update client/executor/wasmtime/Cargo.toml Co-Authored-By: Bastian Köcher <[email protected]> * Limit maximum size of memory. * Rename `init_state` to `with_initialized_state` Co-authored-by: Tomasz Drwięga <[email protected]> Co-authored-by: Bastian Köcher <[email protected]>

pepyakin added 26 commits January 13, 2020 16:44

Migrate wasmtime backend to wasmtime-api

07a33b2

Port to a newer version of wasmtime

8fa4494

Update to the latest changes.

03916c0

Rejig the sandbox module a bit

4e5dda4

Materialze

4e43131

Fixes.

e45463f

executor wasm_runtime fix

0eb842c

Refactor everything

112195a

More refactoring

9dfc877

Even more refactorings

990e236

More cleaning.

11391df

Update to the latest wasmtime

97e2b79

Reformat

a8e6189

Renames

0865676

Refactoring and comments.

c1d8961

Docs

ee3627a

Rename FunctionExecutor to host.

6968fde

Imrpove docs.

d7d540e

fmt

b12af52

Remove panic

9644a1a

Assert the number of arguments are equal between wasmtime and hostfunc.

c539762

Comment a possible panic if there is no corresponding value variant.

38626ab

Check signature of the entrypoint.

28ed297

Use git version of wasmtime

ff66590

Refine and doc the sandbox code.

fc49872

Comment RefCells.

6022464

pepyakin added the A3-in_progress Pull request is in progress. No review needed at this stage. label Jan 20, 2020

pepyakin requested review from bkchr and tomusdrw January 20, 2020 14:18

Merge with master

95b1a0e

Update wasmtime to the latest-ish master.

2156655

This may solve a problem with segfaults.

pepyakin mentioned this pull request Jan 21, 2020

Trap handling dereferences a null pointer bytecodealliance/wasmtime#841

Closed

tomusdrw reviewed Jan 24, 2020

View reviewed changes

tomusdrw approved these changes Jan 24, 2020

View reviewed changes

pepyakin mentioned this pull request Jan 24, 2020

Cursed implementation of allowing missing imports on wasmtime #4730

Merged

Apply suggestions from code review

5a310bf

Co-Authored-By: Tomasz Drwięga <[email protected]>

pepyakin mentioned this pull request Jan 28, 2020

wasmtime implementation violates aliasing requirements #4599

Closed

Demi-Marie self-requested a review January 28, 2020 15:04

Demi-Marie reviewed Jan 31, 2020

View reviewed changes

pepyakin added 10 commits February 3, 2020 17:50

Use full SHA1 hash of wasmtime commit.

452f0b3

Add a panic message.

e8bab85

Add some documentation

6ddc6cf

Update wasmtime version to include SIGSEGV fix

db2595b

Update to crates.io version of wasmtime

50c88e0

Make it work.

24c0479

Move the creation of memory into InstanceWrapper::new

b81581d

Make InstanceWrapper !Send & !Sync

0e1bfdf

Avoid using take_mut

8bd5d6e

pepyakin added A0-please_review Pull request needs code review. and removed A1-onice labels Feb 12, 2020

pepyakin requested a review from bkchr February 12, 2020 14:05

bkchr approved these changes Feb 13, 2020

View reviewed changes

pepyakin and others added 3 commits February 13, 2020 14:05

Update client/executor/wasmtime/Cargo.toml

888901d

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

Limit maximum size of memory.

bb42e52

Rename init_state to with_initialized_state

a0292e7

bkchr merged commit 0b2ae97 into master Feb 13, 2020

bkchr deleted the ser-wasmtime-api branch February 13, 2020 13:54

pepyakin mentioned this pull request Sep 26, 2022

Member Request polkadot-fellows/seeding#6

Merged

executor: Migrate wasmtime backend to a high-level API #4686

executor: Migrate wasmtime backend to a high-level API #4686

Uh oh!

Conversation

pepyakin commented Jan 20, 2020 • edited by bkchr Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tomusdrw left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Demi-Marie left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pepyakin Feb 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

bkchr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pepyakin commented Jan 20, 2020 •

edited by bkchr

Loading

pepyakin Feb 3, 2020 •

edited

Loading