Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(runtime-core) Share the definition of Trampoline across all the backends #915

Merged
merged 1 commit into from
Oct 31, 2019
Merged

fix(runtime-core) Share the definition of Trampoline across all the backends #915

merged 1 commit into from
Oct 31, 2019

Conversation

Hywan
Copy link
Contributor

@Hywan Hywan commented Oct 30, 2019

Extracted from #882.

This patch updates all the backends to use the definition of
Trampoline as defined in the wasmer_runtime_core::typed_func
module. That way, there is no copy of that type, and as such, it is
easier to avoid regression (a simple cargo check does the job).

This patch also formats the use statements in the updated files.

@Hywan Hywan added 🎉 enhancement New feature! 📦 lib-compiler-cranelift About wasmer-compiler-cranelift 📦 lib-deprecated About the deprecated crates 📦 lib-win-exception-handler 📦 lib-compiler-llvm About wasmer-compiler-llvm 📦 lib-compiler-singlepass About wasmer-compiler-singlepass labels Oct 30, 2019
@Hywan Hywan changed the title fx(runtime-core) Share the definition of Trampoline across all the backends fix(runtime-core) Share the definition of Trampoline across all the backends Oct 30, 2019
Hywan
Hywan commented Oct 30, 2019
View reviewed changes
lib/clif-backend/src/signal/windows.rs
@@ -1,24 +1,30 @@
use crate::relocation::{TrapCode, TrapData};
use crate::signal::{CallProtError, HandlerData};
use crate::trampoline::Trampoline;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This line is replaced by wasmer_runtime_core::typed_func::Trampoline.

Hywan
Hywan commented Oct 30, 2019
View reviewed changes
lib/clif-backend/src/trampoline.rs
@@ -28,8 +26,6 @@ impl RelocSink for NullRelocSink {
fn reloc_jt(&mut self, _: u32, _: Reloc, _: ir::JumpTable) {}
}

pub type Trampoline = unsafe extern "C" fn(*mut vm::Ctx, NonNull<vm::Func>, *const u64, *mut u64);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replaced by wasmer_runtime_core::typed_func::Trampoline.

Hywan
Hywan commented Oct 30, 2019
View reviewed changes
lib/runtime-core/src/typed_func.rs
@@ -52,16 +52,21 @@ impl fmt::Display for WasmTrapInfo {
/// of the `Func` struct.
pub trait Kind {}

pub type Trampoline = unsafe extern "C" fn(*mut Ctx, NonNull<vm::Func>, *const u64, *mut u64);
pub type Trampoline = unsafe extern "C" fn(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nothing has changed here, except arguments are now labelled.

Hywan
Hywan commented Oct 30, 2019
View reviewed changes
lib/runtime-core/src/typed_func.rs
*mut WasmTrapInfo,
*mut Option<Box<dyn Any>>,
Option<NonNull<c_void>>,
trampoline: Trampoline,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above: Nothing has changed except arguments are now labelled.

Hywan
Hywan commented Oct 30, 2019
View reviewed changes
lib/singlepass-backend/src/codegen_x64.rs
@@ -225,7 +229,7 @@ impl RunnableModule for X64ExecutionContext {
14: 41 ff e3 jmpq *%r11
*/
#[repr(packed)]
struct Trampoline {
struct LocalTrampoline {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This structure has been renamed to avoid naming clashing with wasmer_runtime_core::typed_func::Trampoline.

@Hywan
fix(runtime-core) Share the definition of Trampoline across all the…
edb6cbe 
… backends.

This patch updates all the backends to use the definition of
`Trampoline` as defined in the `wasmer_runtime_core::typed_func`
module. That way, there is no copy of that type, and as such, it is
easier to avoid regression (a simple `cargo check` does the job).

This patch also formats the `use` statements in the updated files.
@Hywan Hywan force-pushed the feat-runtime-core-trampoline branch from 4dc9888 to edb6cbe Compare October 30, 2019 12:10
Hywan
Hywan commented Oct 30, 2019
View reviewed changes
lib/win-exception-handler/src/exception_handling.rs

type Trampoline = unsafe extern "C" fn(*mut Ctx, NonNull<Func>, *const u64, *mut u64);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replaced by wasmer_runtime_core::typed_func::Trampoline.

@Hywan
Copy link
Contributor Author

Hywan commented Oct 30, 2019

bors try

bors bot added a commit that referenced this pull request Oct 30, 2019
@bors
Try #915:
7b235bd
@bors
Copy link
Contributor

bors bot commented Oct 30, 2019

try

Build succeeded

  • wasmerio.wasmer

MarkMcCaskey
MarkMcCaskey approved these changes Oct 30, 2019
View reviewed changes
Copy link
Contributor

@MarkMcCaskey MarkMcCaskey left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This change looks good to me, though you may want to get review from people who work on the backends more

edit: nothing should be changing here, so it's probably safe to just ship it

@Hywan
Copy link
Contributor Author

Hywan commented Oct 31, 2019

bors r+

bors bot added a commit that referenced this pull request Oct 31, 2019
@bors @Hywan
Merge #915
fb0615d 
915: fix(runtime-core) Share the definition of `Trampoline` across all the backends r=Hywan a=Hywan

Extracted from #882.

This patch updates all the backends to use the definition of
`Trampoline` as defined in the `wasmer_runtime_core::typed_func`
module. That way, there is no copy of that type, and as such, it is
easier to avoid regression (a simple `cargo check` does the job).

This patch also formats the `use` statements in the updated files.


Co-authored-by: Ivan Enderlin <ivan.enderlin@hoa-project.net>
@bors
Copy link
Contributor

bors bot commented Oct 31, 2019

Build succeeded

  • wasmerio.wasmer

@bors bors bot merged commit edb6cbe into wasmerio:master Oct 31, 2019
@Hywan Hywan mentioned this pull request Nov 4, 2019
Merged
Hywan added a commit to Hywan/wasmer that referenced this pull request Nov 6, 2019
@Hywan
doc(changelog) Add wasmerio#915, wasmerio#917 and wasmerio#925.
ac8aece
bors bot added a commit that referenced this pull request Nov 12, 2019
@bors @Hywan
Merge #925
365bcd6 
925: feat(runtime-core) Support closures with a captured environment as host functions r=Hywan a=Hywan

Reboot of #882 and #219.

For the moment, host functions (aka imported functions) can be regular function pointers, or (as a side-effect) closures without a captured environment. This PR extends the support of host functions to closures with a captured environment. This is required for many other features (incl. the Python integration, the Ruby integration, WebAssembly Interface Types [see #787], and so on).

This PR is the culmination of previous ones, notably #915, #916 and #917. 

### General idea

The user-defined host function is wrapped inside a `wrap` function. This wrapper function initially receives a `vm::Ctx` as its first argument, which is passed to the host function when necessary. The patch keeps this behavior but it comes from `vm::FuncCtx`, which is a new structure. A `vm::FuncCtx` is held by `vm::ImportedFunc` such as:

```rust
#[repr(C)]
pub struct ImportedFunc {
    pub(crate) func: *const Func,
    pub(crate) func_ctx: NonNull<FuncCtx>,
}
```

where `vm::FuncCtx` is:

```rust
#[repr(C)]
pub struct FuncCtx {
    pub(crate) vmctx: NonNull<Ctx>,
    pub(crate) func_env: Option<NonNull<FuncEnv>>,
}
```

where `vm::FuncEnv` is:

```rust
#[repr(transparent)]
pub struct FuncEnv(pub(self) *mut c_void);
```

i.e. a raw opaque pointer.

So the wrapper function of a host function receives a `vm::Ctx`, which is used to find out the associated `FuncCtx` (by using the import backing), which holds `vm::FuncEnv`. It holds a pointer to the closure captured environment.

### Implementation details

#### How to get a pointer to a closure captured environment

A closure with a captured environment has a memory size greater than zero. This is how we detect it:

```rust
if mem::size_of::<Self>() != 0 { … }
```

To get a pointer to its captured environment, we use this statement:

```rust
NonNull::new(Box::into_raw(Box::new(self))).map(NonNull::cast)
```

(in `typed_func.rs`, in the `wrap` functions).

To reconstruct the closure based on the pointer, we use this statement:

```rust
let func: &FN = {
    let func: NonNull<FN> = func_env.cast();
    &*func.as_ptr()
};
```

That's basically how it works. And that's the core idea of this patch.

As a side effect, we have removed an undefined behavior (UB) in 2 places: The `mem::transmute(&())` has been removed (it was used to get the function pointer of `FN`). The transmute is replaced by `FuncEnv`, which provides a unified API, erasing the difference between host functions as closures with a captured environment, and host functions as function pointer. For a reason I ignore, the UB wasn't showing himself until this PR and a modification in the Singlepass backend. But now it's fixed.

#### Impact on `Backing`

After the modification on the `typed_func` and the `vm` modules, this is the other core idea of this patch: Updating the `backing` module so that `vm::ImportedFunc` replaces `vm::Ctx` by `vm::FuncCtx`.

When creating `vm::ImportedFunc`, a new `vm::FuncCtx` is created and its pointer is used. We are purposely leaking `vm::FuncCtx` so that the pointer is always valid. Hence the specific `Drop` implementation on `ImportBacking` to dereference the pointer, and to drop it properly.

#### Impact on the backends

Since the structure of `vm::ImportedFunc` has changed, backends must be updated. We must deref `FuncCtx` to reach its `vmctx` field.

#### Impact on `Instance`

Because `vm::ImportedFunc` has changed, it has a minor impact on the `instance` module, nothing crazy though.

Co-authored-by: Ivan Enderlin <ivan.enderlin@hoa-project.net>
bors bot added a commit that referenced this pull request Nov 12, 2019
@bors @Hywan
Merge #925
8e32c9b 
925: feat(runtime-core) Support closures with a captured environment as host functions r=Hywan a=Hywan

Reboot of #882 and #219.

For the moment, host functions (aka imported functions) can be regular function pointers, or (as a side-effect) closures without a captured environment. This PR extends the support of host functions to closures with a captured environment. This is required for many other features (incl. the Python integration, the Ruby integration, WebAssembly Interface Types [see #787], and so on).

This PR is the culmination of previous ones, notably #915, #916 and #917. 

### General idea

The user-defined host function is wrapped inside a `wrap` function. This wrapper function initially receives a `vm::Ctx` as its first argument, which is passed to the host function when necessary. The patch keeps this behavior but it comes from `vm::FuncCtx`, which is a new structure. A `vm::FuncCtx` is held by `vm::ImportedFunc` such as:

```rust
#[repr(C)]
pub struct ImportedFunc {
    pub(crate) func: *const Func,
    pub(crate) func_ctx: NonNull<FuncCtx>,
}
```

where `vm::FuncCtx` is:

```rust
#[repr(C)]
pub struct FuncCtx {
    pub(crate) vmctx: NonNull<Ctx>,
    pub(crate) func_env: Option<NonNull<FuncEnv>>,
}
```

where `vm::FuncEnv` is:

```rust
#[repr(transparent)]
pub struct FuncEnv(pub(self) *mut c_void);
```

i.e. a raw opaque pointer.

So the wrapper function of a host function receives a `vm::Ctx`, which is used to find out the associated `FuncCtx` (by using the import backing), which holds `vm::FuncEnv`. It holds a pointer to the closure captured environment.

### Implementation details

#### How to get a pointer to a closure captured environment

A closure with a captured environment has a memory size greater than zero. This is how we detect it:

```rust
if mem::size_of::<Self>() != 0 { … }
```

To get a pointer to its captured environment, we use this statement:

```rust
NonNull::new(Box::into_raw(Box::new(self))).map(NonNull::cast)
```

(in `typed_func.rs`, in the `wrap` functions).

To reconstruct the closure based on the pointer, we use this statement:

```rust
let func: &FN = {
    let func: NonNull<FN> = func_env.cast();
    &*func.as_ptr()
};
```

That's basically how it works. And that's the core idea of this patch.

As a side effect, we have removed an undefined behavior (UB) in 2 places: The `mem::transmute(&())` has been removed (it was used to get the function pointer of `FN`). The transmute is replaced by `FuncEnv`, which provides a unified API, erasing the difference between host functions as closures with a captured environment, and host functions as function pointer. For a reason I ignore, the UB wasn't showing himself until this PR and a modification in the Singlepass backend. But now it's fixed.

#### Impact on `Backing`

After the modification on the `typed_func` and the `vm` modules, this is the other core idea of this patch: Updating the `backing` module so that `vm::ImportedFunc` replaces `vm::Ctx` by `vm::FuncCtx`.

When creating `vm::ImportedFunc`, a new `vm::FuncCtx` is created and its pointer is used. We are purposely leaking `vm::FuncCtx` so that the pointer is always valid. Hence the specific `Drop` implementation on `ImportBacking` to dereference the pointer, and to drop it properly.

#### Impact on the backends

Since the structure of `vm::ImportedFunc` has changed, backends must be updated. We must deref `FuncCtx` to reach its `vmctx` field.

#### Impact on `Instance`

Because `vm::ImportedFunc` has changed, it has a minor impact on the `instance` module, nothing crazy though.

Co-authored-by: Ivan Enderlin <ivan.enderlin@hoa-project.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MarkMcCaskey MarkMcCaskey MarkMcCaskey approved these changes

@nlewycky nlewycky nlewycky approved these changes

@bjfish bjfish Awaiting requested review from bjfish

@losfair losfair Awaiting requested review from losfair

Assignees
No one assigned
Labels
🎉 enhancement New feature! 📦 lib-compiler-cranelift About wasmer-compiler-cranelift 📦 lib-compiler-llvm About wasmer-compiler-llvm 📦 lib-compiler-singlepass About wasmer-compiler-singlepass 📦 lib-deprecated About the deprecated crates
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Hywan @nlewycky @MarkMcCaskey