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

feat(c-api) Implement the traps argument of wasm_instance_new #1761

Merged
merged 7 commits into from
Oct 30, 2020
Merged

feat(c-api) Implement the traps argument of wasm_instance_new #1761

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a2d96d9
feat(c-api) Implement the `traps` argument of `wasm_instance_new`.
Hywan Oct 26, 2020
38c57e1
test(c-api) Enable the `wasm-c-api-start` test.
Hywan Oct 26, 2020
246b7d2
doc(changelog) Add #1761.
Hywan Oct 26, 2020
4521813
fix(c-api) Remove a `debug_assert` macro.
Hywan Oct 27, 2020
4102dcb
feat(c-api) Shrink vector of traps.
Hywan Oct 27, 2020
9583a91
fix(c-api) `wasm_trap_t**` in `wasm_instance_new` does not represent …
Hywan Oct 30, 2020
bdb0dc7
Merge branch 'master' into feat-c-api-instance-new-traps
Hywan Oct 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,10 @@
## **[Unreleased]**

- [#1710](https://github.com/wasmerio/wasmer/pull/1710) Memory for function call trampolines is now owned by the Artifact.

### Added

- [#1761](https://github.com/wasmerio/wasmer/pull/1761) Implement the `wasm_trap_t**` argument of `wasm_instance_new` in the Wasm C API.
- [#1687](https://github.com/wasmerio/wasmer/pull/1687) Add basic table example; fix ownership of local memory and local table metadata in the VM.
- [#1751](https://github.com/wasmerio/wasmer/pull/1751) Implement `wasm_trap_t` inside a function declared with `wasm_func_new_with_env` in the Wasm C API.
- [#1741](https://github.com/wasmerio/wasmer/pull/1741) Implement `wasm_memory_type` in the Wasm C API.
Expand Down
23 changes: 19 additions & 4 deletions lib/c-api/src/wasm_c_api/instance.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use super::trap::wasm_trap_t;
use crate::ordered_resolver::OrderedResolver;
use std::mem;
use std::sync::Arc;
use wasmer::{Extern, Instance};
use wasmer::{Extern, Instance, InstantiationError};

#[allow(non_camel_case_types)]
pub struct wasm_instance_t {
Expand All @@ -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
Copy link
Contributor

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

Copy link
Contributor Author

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:

wasmer/lib/c-api/tests/wasm_c_api/wasm-c-api/example/start.c

Line 95 in 86f3438

wasm_trap_delete(trap);

I believe that the value is owned by the caller, not by the callee.

Copy link
Contributor

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*

Copy link
Contributor Author

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.

_traps: *mut *mut wasm_trap_t,
traps: *mut *mut wasm_trap_t,
) -> Option<Box<wasm_instance_t>> {
let wasm_module = &module.inner;
let module_imports = wasm_module.imports();
Expand All @@ -32,7 +31,23 @@ pub unsafe extern "C" fn wasm_instance_new(
.cloned()
.collect();

let instance = Arc::new(c_try!(Instance::new(wasm_module, &resolver)));
let instance = match Instance::new(wasm_module, &resolver) {
Ok(instance) => Arc::new(instance),

Err(InstantiationError::Link(link_error)) => {
crate::error::update_last_error(link_error);

return None;
}

Err(InstantiationError::Start(runtime_error)) => {
let trap: Box<wasm_trap_t> = Box::new(runtime_error.into());
*traps = Box::into_raw(trap);

return None;
}
};

Some(Box::new(wasm_instance_t { inner: instance }))
}

Expand Down
14 changes: 7 additions & 7 deletions lib/c-api/tests/wasm_c_api/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ add_executable(wasm-c-api-hello wasm-c-api/example/hello.c)
#add_executable(wasm-c-api-multi wasm-c-api/example/multi.c)
add_executable(wasm-c-api-reflect wasm-c-api/example/reflect.c)
add_executable(wasm-c-api-serialize wasm-c-api/example/serialize.c)
#add_executable(wasm-c-api-start wasm-c-api/example/start.c)
add_executable(wasm-c-api-start wasm-c-api/example/start.c)
#add_executable(wasm-c-api-table wasm-c-api/example/table.c)
#add_executable(wasm-c-api-threads wasm-c-api/example/threads.c)
add_executable(wasm-c-api-trap wasm-c-api/example/trap.c)
Expand Down Expand Up @@ -106,12 +106,12 @@ add_test(NAME wasm-c-api-serialize
WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/wasm-c-api/example/
)

#target_link_libraries(wasm-c-api-start general ${WASMER_LIB})
#target_compile_options(wasm-c-api-start PRIVATE ${COMPILER_OPTIONS})
#add_test(NAME wasm-c-api-start
# COMMAND wasm-c-api-start
# WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/wasm-c-api/example/
#)
target_link_libraries(wasm-c-api-start general ${WASMER_LIB})
target_compile_options(wasm-c-api-start PRIVATE ${COMPILER_OPTIONS})
add_test(NAME wasm-c-api-start
COMMAND wasm-c-api-start
WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/wasm-c-api/example/
)

#target_link_libraries(wasm-c-api-table general ${WASMER_LIB})
#target_compile_options(wasm-c-api-table PRIVATE ${COMPILER_OPTIONS})
Expand Down