-
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
Remove ctx argument on imported functions and allow closures. #219
Conversation
This might indirectly also work around #188 |
@CryZe Could you elaborate? I didn't really change how imported function calling worked on windows. |
Oh yeah, if ctx / some env internally is still used as the first argument, then this still happens. |
Oh, that's a shame. I haven't had time to look into fixing the issues you found; been working on some stuff that we're not ready to show yet. |
Yes. This should fix #169 |
Adding closures would be nice, I think whatever is done should be compatible with our current ctx usage in emscripten/C API/etc. It's not a big deal but this PR would be a little easier to read if the renaming of |
Why do you want to remove the context argument? |
This doesn't remove per-say, but really just repurposes it to hold some environmental data, whether that's the vm ctx or not. Then it adds some convenient sugar around that to make it easy to use closures as imported functions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Closing this PR in favor of the Import State changes from #299: 7b0992e. This new API let the user establish the context data very easily. Lines 33 to 53 in 2aefa73
|
@@ -16,7 +16,7 @@ extern "C" { | |||
#[link_name = "callProtected"] | |||
pub fn __call_protected( | |||
trampoline: Trampoline, | |||
ctx: *mut Ctx, | |||
env: Option<NonNull<Env>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The C code doesn't handle the Option
correctly I guess (it expects a pointer).
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]>
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]>
Remove nearly-all mentions of reborn.
This allows you to use fully-fledged closures for imported functions.
@evgenykuzyakov Does this fix the data initialization issue you were having?