-
Notifications
You must be signed in to change notification settings - Fork 824
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.
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
feat(runtime-core) Support closures with a captured environment as host functions #925
Merged
bors
merged 34 commits into
wasmerio:master
from
Hywan:feat-runtime-core-clos-host-function
Nov 12, 2019
Merged
feat(runtime-core) Support closures with a captured environment as host functions #925
bors
merged 34 commits into
wasmerio:master
from
Hywan:feat-runtime-core-clos-host-function
Nov 12, 2019
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Hywan
added
📦 lib-compiler-cranelift
About wasmer-compiler-cranelift
📦 lib-deprecated
About the deprecated crates
📦 lib-spectests
📦 lib-compiler-llvm
About wasmer-compiler-llvm
labels
Nov 4, 2019
syrusakbary
reviewed
Nov 4, 2019
syrusakbary
reviewed
Nov 4, 2019
Loving this PR ❤️ |
Hywan
added
🎉 enhancement
New feature!
🧪 tests
I love tests
and removed
📦 lib-spectests
labels
Nov 4, 2019
Well done, Ivan 👍 |
`vm::FuncCtx` replaces `vm::Ctx` as first argument passed to host functions (aka imported functions).
…ffset. `ImportedFunc::offset_vmctx` becomes `ImportedFunc::offset_func_ctx`.
…an host function.
In the `wrap` functions, we use `std::mem::transmute(&())` to get the pointer to the value “around” `wrap` (`Fn` has a method `to_raw` which declares a `wrap` function, which uses `transmute` to retrieve `Fn`). This is an undefined behavior. It was working until the `FuncCtx` is introduced. Since then, the undefined behavior was causing an error with the Singlepass backend. This patch stores the pointer to `Fn` in `func_env`, so that the pointer to the user-defined host function is always predictable.
Hywan
force-pushed
the
feat-runtime-core-clos-host-function
branch
from
November 6, 2019 13:50
8b185ae
to
cf74b68
Compare
bors try |
Description updated! Ready to review :-). cc @MarkMcCaskey @nlewycky |
bors try |
tryAlready running a review |
bors try- |
bors try |
bjfish
approved these changes
Nov 12, 2019
tryBuild failed
|
…ll` types. It fails only in release mode. That's a bug from the `field-offset` crate. This patch is a temporary fix.
bors r+ |
bors bot
added a commit
that referenced
this pull request
Nov 12, 2019
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 <[email protected]>
Build failed
|
It will be fixed in a following PR.
I hope all bugs with bors r+ |
bors bot
added a commit
that referenced
this pull request
Nov 12, 2019
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 <[email protected]>
bors r+ |
Already running a review |
Build succeeded
|
bors bot
added a commit
that referenced
this pull request
Nov 13, 2019
955: feat(runtime-core) Replace the `field-offset` crate by a custom `offset_of!` macro r=Hywan a=Hywan The `field-offset` crate is unmaintained. When using its `offset_of!` macro on a struct with a field of type `std::ptr::NonNull`, in release mode, it generates a sigill. This patch removes the `field-offset` crate, and implements a custom `offset_of!` macro. See #925 last commits to see an illustration of this bug. Co-authored-by: Ivan Enderlin <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
🎉 enhancement
New feature!
📦 lib-compiler-cranelift
About wasmer-compiler-cranelift
📦 lib-compiler-llvm
About wasmer-compiler-llvm
📦 lib-deprecated
About the deprecated crates
🧪 tests
I love tests
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 avm::Ctx
as its first argument, which is passed to the host function when necessary. The patch keeps this behavior but it comes fromvm::FuncCtx
, which is a new structure. Avm::FuncCtx
is held byvm::ImportedFunc
such as:where
vm::FuncCtx
is:where
vm::FuncEnv
is: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 associatedFuncCtx
(by using the import backing), which holdsvm::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:
To get a pointer to its captured environment, we use this statement:
(in
typed_func.rs
, in thewrap
functions).To reconstruct the closure based on the pointer, we use this statement:
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 ofFN
). The transmute is replaced byFuncEnv
, 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 thevm
modules, this is the other core idea of this patch: Updating thebacking
module so thatvm::ImportedFunc
replacesvm::Ctx
byvm::FuncCtx
.When creating
vm::ImportedFunc
, a newvm::FuncCtx
is created and its pointer is used. We are purposely leakingvm::FuncCtx
so that the pointer is always valid. Hence the specificDrop
implementation onImportBacking
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 derefFuncCtx
to reach itsvmctx
field.Impact on
Instance
Because
vm::ImportedFunc
has changed, it has a minor impact on theinstance
module, nothing crazy though.