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) Add wat2wasm to the wasm-c-api module #1635

Merged
merged 6 commits into from
Oct 6, 2020

Conversation

Hywan
Copy link
Contributor

@Hywan Hywan commented Sep 18, 2020

This PR adds the wat2wasm function to the wasm-c-api module.

Caveat of this patch: The wasmer_wasm.h seems to be written by hand. Consequently, if the lib/c-api/ crate is compiled without the wat feature (which is the default), the wat2wasm function will still be defined in wasmer_wasm.h.
Proposal: We must generate the wasmer_wasm.h header file with the same approach than we use for wasm.h. It can be done in a next PR.

Thoughts @MarkMcCaskey

Hywan added a commit to Hywan/go-ext-wasm that referenced this pull request Sep 18, 2020
It depends on wasmerio/wasmer#1635. The
`include/` and `wasmer_wasm.h` files are already from this PR.
@Hywan Hywan requested a review from MarkMcCaskey September 18, 2020 13:30
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.

We discussed organization of the C APIs privately, and I think with some of the changes we talked about we can probably generate that file, but for now it's non-trivial to do so.

I'm unsure if we want to make this transformation explicit or not. In Wasmer we allow taking a &[u8] and we'll handle both wat and wasm. Perhaps we just want to do that for the C API too.

If we want to make it explicit, then this change seems fine. How would users free this data though? And one more point, when turning things like Vec or String into a pointer + length, it's important to shrink_to_fit first so we don't leak memory. Vec has 3 numbers: pointer, length, and capacity. If capacity != length (which is very likely), then we can't reconstruct the Vec correctly and will probably do a partial free

@Hywan Hywan force-pushed the feat-c-api-wat2wasm branch from c1a9c8b to ded3132 Compare October 6, 2020 09:10
@Hywan
Copy link
Contributor Author

Hywan commented Oct 6, 2020

I've rebased this PR on top of the master branch.
I've also added shrint_to_fit.
Finally, I've changed the wat2wasm function type signature:

- unsafe extern "C" fn wat2wasm(wat: &wasm_byte_vec_t, wasm: &mut wasm_byte_vec_t) -> Option<()>
+ unsafe extern "C" fn wat2wasm(wat: &wasm_byte_vec_t) -> Option<Box<wasm_byte_vec_t>>

I believe this API is better. If it returns NULL, it's clear there is an error; Also it helps to use ? in the implementation. It also makes explicit that the callee is responsible of freeing the data.

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 good! Other than some small bugs it looks good to go! One other thing, perhaps we want to add an example/test for this function (it may have caught the use after free bug, though it's highly likely it would not have)

lib/c-api/src/wasm_c_api/wat.rs Outdated Show resolved Hide resolved
lib/c-api/src/wasm_c_api/wat.rs Outdated Show resolved Hide resolved
@Hywan
Copy link
Contributor Author

Hywan commented Oct 6, 2020

bors r+

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!

@bors
Copy link
Contributor

bors bot commented Oct 6, 2020

@bors bors bot merged commit cf47832 into wasmerio:master 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.

3 participants