-
Notifications
You must be signed in to change notification settings - Fork 821
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(c-api) Implement the traps
argument of wasm_instance_new
#1761
Conversation
When running `Instance::new`, it can error with an `InstantiationError`. There is 2 scenarii: 1. Either it's a `InstantiationError::Link`. In this case, the `wasm_instance_new` function must return `NULL` and register the error in the Wasmer error registry. 2. Either it's a `InstantiationError::Start`. In this case, the `wasm_instance_new` function must return `NULL` and the error must be converted into a `wasm_trap_t`, which is stored in the `wasm_trap_t**` array. This array is initialized by `wasm_instance_new` itself.
I'm not sure why |
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.
Looks like a good change overall, but I think we need to iterate a bit more on the ownership of this
@@ -17,8 +17,7 @@ pub unsafe extern "C" fn wasm_instance_new( | |||
_store: &wasm_store_t, | |||
module: &wasm_module_t, | |||
imports: &wasm_extern_vec_t, | |||
// own |
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.
Might be useful to put this comment back if it's correct, otherwise it's not clear what the ownership is of *mut *mut
, own
means we're responsible for freeing it I believe
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 can't own it. We can't free it. The start.c
example explicitely drops the trap
value:
wasmer/lib/c-api/tests/wasm_c_api/wasm-c-api/example/start.c
Lines 58 to 60 in 86f3438
own wasm_trap_t* trap = NULL; | |
own wasm_instance_t* instance = | |
wasm_instance_new(store, module, &imports, &trap); |
and later:
wasm_trap_delete(trap); |
I believe that the value is owned by the caller, not by the callee.
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.
Huh, own wasm_trap_t**
is weird in the context of using arguments as outputs.
It seems like there's no need for the Vec
at all then, this just seems to be a double pointer for the sake of setting a trap*
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 wasm_trap_t**
must not be interpreted as an array of wasm_trap_t*
, hmm. Let's fix that then.
Ensure that the size of the vector of the instance's traps equals its capacity by shrinking 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.
Based on the example in start.c, it seems that we just need one pointer here, not two. The Vec
is never freed nor used here!
…an array. The `wasm_trap_t**` argument of `wasm_instance_new` represents an output pointer to a `wasm_trap_t*`, not an array of `wasm_trap_t*`. This patch updates the code accordingly.
bors r+ |
Build succeeded: |
Description
When running
Instance::new
, it can error with anInstantiationError
. There is 2 scenarii:Either it's a
InstantiationError::Link
. In this case, thewasm_instance_new
function must returnNULL
and register theerror in the Wasmer error registry.
Either it's a
InstantiationError::Start
. In this case, thewasm_instance_new
function must returnNULL
and the error must beconverted into a
wasm_trap_t*
, which is stored in thewasm_trap_t**
.Review
Fixes #1744.