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.

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 wasm_module_validate #1636

Merged
merged 2 commits into from
Oct 5, 2020

Conversation

Hywan
Copy link
Contributor

@Hywan Hywan commented Sep 18, 2020

This PR implements wasm_module_validate.

It returns a bool, but it populates the error handler too. I took the liberty to update the c_try! macro to return any kind of value, so that it is generic over the returned type of the function using c_try!.

@Hywan Hywan requested a review from MarkMcCaskey September 18, 2020 13:26
Hywan added a commit to Hywan/go-ext-wasm that referenced this pull request Sep 18, 2020
It depends on It requires
wasmerio/wasmer#1636. The `include/` file is
already from this PR.
Copy link
Contributor

@MarkMcCaskey MarkMcCaskey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

I was initially a bit uncertain about having anything but the bare minimum syntax in a macro, but I think it's probably fine!

lib/c-api/src/wasm_c_api/mod.rs Outdated Show resolved Hide resolved
lib/c-api/src/wasm_c_api/mod.rs Outdated Show resolved Hide resolved
lib/c-api/src/wasm_c_api/mod.rs Outdated Show resolved Hide resolved
@Hywan Hywan force-pushed the feat-c-api-module-validate branch from 97521a7 to ff4cb6d Compare October 5, 2020 12:50
@Hywan
Copy link
Contributor Author

Hywan commented Oct 5, 2020

@MarkMcCaskey I've rebased the PR on top of the master branch. I took this as an opportunity to not change the c_try! macro. The code is simpler.

I don't use c_try! at all because the function returns a bool, not a Result.

Finally, I prefer to use Option<NonNull<wasm_store_t>> rather than *const wasm_store_t, it provides a better Rust idiomatic API from my point of view.

Thoughts?

@MarkMcCaskey
Copy link
Contributor

Finally, I prefer to use Option<NonNull<wasm_store_t>> rather than *const wasm_store_t, it provides a better Rust idiomatic API from my point of view.

Thoughts?

My biggest issue with NonNull is that it implies ownership... It's too bad Rust doesn't have an immutable NonNull pointer... cbindgen will generate code that implies ownership with NonNull I believe...

I think &wasm_store_t is probably fine. As long as the C API doesn't say we need to handle null pointers for the store, then I think it's fine to go into UB world if the C caller messes up.

Copy link
Contributor

@MarkMcCaskey MarkMcCaskey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than that, looks good to me!

@@ -32,6 +33,30 @@ pub unsafe extern "C" fn wasm_module_new(
#[no_mangle]
pub unsafe extern "C" fn wasm_module_delete(_module: Option<Box<wasm_module_t>>) {}

#[no_mangle]
pub unsafe extern "C" fn wasm_module_validate(
store_ptr: Option<NonNull<wasm_store_t>>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I commented elsewhere on this PR about the implied semantics of NonNull, store: &wasm_store_t is probably better, especially if we're calling unwrap on it

`wasm_store_t` is now a proper struct (rather than an opaque type) of
kind:

```rs
struct wasm_store_t {
    inner: Store
}
```

The rest of the patch updates the code accordingly.
@Hywan
Copy link
Contributor Author

Hywan commented Oct 5, 2020

I've updated the definition of wasm_store_t from being an opaque Rust type to a real Rust struct such as:

struct wasm_store {
    inner: Store
}

The rest of the patch above updates the code accordingly.

Thoughts?

Copy link
Contributor

@MarkMcCaskey MarkMcCaskey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great to me!

@Hywan
Copy link
Contributor Author

Hywan commented Oct 5, 2020

bors r+

bors bot added a commit that referenced this pull request Oct 5, 2020
1636: feat(c-api) Implement `wasm_module_validate` r=Hywan a=Hywan

This PR implements `wasm_module_validate`.

It returns a bool, but it populates the error handler too. I took the liberty to update the `c_try!` macro to return any kind of value, so that it is generic over the returned type of the function using `c_try!`.

Co-authored-by: Ivan Enderlin <ivan@mnt.io>
@bors
Copy link
Contributor

bors bot commented Oct 5, 2020

This PR was included in a batch that successfully built, but then failed to merge into master (it was a non-fast-forward update). It will be automatically retried.

@bors
Copy link
Contributor

bors bot commented Oct 5, 2020

@bors bors bot merged commit 1b90a8f into wasmerio:master Oct 5, 2020
Hywan added a commit to Hywan/wasmer that referenced this pull request Oct 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants