-
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
Add host env via trait with proc macro #1739
Conversation
149d0b9
to
46204d6
Compare
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.
Could you break the vmctx -> union part to its own PR?
lib/api/src/lib.rs
Outdated
} | ||
} | ||
|
||
unsafe impl<T: Send> Send for InitAfterInstance<T> {} |
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.
Why InitAfterInstance
must implement Send
? Is it to preserve the behavior of T
in this case?
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.
This was also for WasiEnv
but more largely because the way Wasmer works, we can pass functions to other threads and call it meaning that it's Send
... perhaps it can also be used in a Sync
way but I didn't think so...
Someone filed an issue for us to review this and I agree, that we should probably audit what we actually allow users to do
lib/vm/src/export.rs
Outdated
@@ -31,6 +31,8 @@ pub struct ExportFunction { | |||
pub address: *const VMFunctionBody, | |||
/// Pointer to the containing `VMContext`. | |||
pub vmctx: *mut VMContext, | |||
/// temp code to set vmctx for host functions | |||
pub function_ptr: Option<fn(*mut std::ffi::c_void, *const std::ffi::c_void)>, |
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.
I don't understand what it does exactly, and why the name function_ptr
? Can you explain please?
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.
This is WasmerEnv::finish
for the env used by this function, due to our heirarchy of types, we can't be explicit about the type signature because this is in vm
and it takes &api::Instance
as an argument.
It's really not ideal but I haven't come up with a better way to get access to the initializer function yet. This is the thing I was hoping to get the most feedback on early
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.
I'm unclear on why we need this here. The env finisher should live in the InstanceHandle, right?
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.
Good question!
This is how the function pointers get into the InstanceHandle
: we lose type information, but wasmer::externals::function::Function
methods have access to the initializer, we have to store it somewhere for later and this is the best place I've found.
The little code window above that Ivan commented on is showing out of date code, but I left a comment there asking for feedback if anyone knows a better way to get this information to where we need it.
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.
Why don't store it in InstanceHandle
?
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.
We do, but we have to get that information to the InstanceHandle
.
The data flow starts when a user makes a new Function
:
-
Function::new_with_env : by way of the trait constraint, we can get the initializer and turn it into a function pointer. We then put it in
ExportFunction
because it seemed like the most obvious place to put it. -
We need this data when instantiating Instance::new, Module::instantiate , we only get a module and a resolver (the resolver is where all the functions are living)
-
We end up in engine::Artifact::instantiate here we extract these values from the
Imports
during instantiation and make it part of theInstanceHandle
-
Later we call a function on
InstanceHandle
to do something with this stored data
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 PR I think it moves to the good direction.
Feedback:
- Test the wasm-c-api
wasm_func_new_with_env
with a custom finalizer - Test the behavior of a creating a custom environment. That the finalizer and free methods get called.
@@ -140,6 +143,12 @@ impl Function { | |||
let vmctx = VMFunctionEnvironment { | |||
host_env: Box::into_raw(Box::new(dynamic_ctx)) as *mut _, | |||
}; | |||
// TODO: look into removing transmute by changing API type signatures |
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.
How we can change the API?
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.
There's no good way to do this that I can think of yet. The only way to avoid the transmutes would be to to make the WasmerEnv::init_with_instance
function take void*
directly... but that seems like a bad trade off.
lib/api/src/externals/function.rs
Outdated
// TODO: look into removing transmute by changing API type signatures | ||
let function_ptr = Some(std::mem::transmute::< | ||
fn(_, _) -> Result<(), _>, | ||
fn(_, _) -> Result<(), _>, | ||
>(Env::init_with_instance)); |
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.
I'm not super sure why this is needed. Can you provide some context @MarkMcCaskey ?
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.
Which part? The function_ptr at all or the transmute?
We need the function pointer to initialize the data, so far this is the most direct place to put it so we can access where we need it.
We have to transmute it because WasmerEnv
isn't known to the vm
so we can't use the real name of the type, I have some work in progress trying to make it so vm can see WasmerEnv
but it's a bit stuck
lib/api/src/externals/function.rs
Outdated
@@ -291,6 +317,7 @@ impl Function { | |||
address, | |||
kind: VMFunctionKind::Static, | |||
vmctx, | |||
function_ptr, |
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.
We should move this out of this struct.
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.
This is the same as #1739 (comment) ; it may be possible but I'm not exactly sure how yet
lib/api/src/native.rs
Outdated
// TODO: | ||
function_ptr: None, |
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.
I really think this doesn't belong here.
/// instantiating the module. | ||
#[derive(Debug)] | ||
pub(crate) struct FunctionWrapper { | ||
pub(crate) func: NonNull<Function>, | ||
pub(crate) legacy_env: NonNull<UnsafeMutableEnv>, | ||
pub(crate) legacy_env: NonNull<LegacyEnv>, |
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.
I actually like more UnsafeMutableLegacyEnv
. Verbose but it conveys the meaning. MutableLegacyEnv
is also good
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.
So this change is actually fixing an issue from the find and replace done on in another PR. It should really be split out and merged to master separately, but I accidentally renamed a struct with the same name in another place by doing bulk find and replace, this just corrects that mistake
I can't figure out how we can ever use the non-wrapped type though
bors r+ |
This PR includes a trait-based host-env init and deinit flow (including the plumbing to call these functions) as well as a procedural macro to generate this code.
For example,
will generate code roughly looking like:
The exact details of the interface are subject to change and should be reviewed thoroughly, for example we will want to be able to handle errors properly instead of
unwrap
ing and assuming they don't happen.note to self: The generated helper functions can be used in an unsound way if called before we instantiate, but it'd be nice to not have to use unsafe or deal with
Option
when being called from a host function...Review
WasmerEnv
traitWasmerEnv
)