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(runtime-c-api) Implement wasmer_trap #1133

Merged
merged 20 commits into from
Jan 15, 2020

Conversation

Hywan
Copy link
Contributor

@Hywan Hywan commented Jan 10, 2020

Address wasmerio/wasmer-go#58.

Idea is to provide an API to get fallible host function by calling wasmer_trap to run the Wasmer trapping API.

This is probably the easiest solution to not break the existing API, and not add a lot of complexity in the code.

@Hywan Hywan added 🎉 enhancement New feature! 📦 lib-c-api About wasmer-c-api 🧪 tests I love tests 📚 documentation Do you like to read? labels Jan 10, 2020
@Hywan Hywan requested a review from bjfish as a code owner January 10, 2020 14:17
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.

Good idea for an update!

lib/runtime-c-api/src/import/mod.rs Show resolved Hide resolved
lib/runtime-c-api/src/import/mod.rs Show resolved Hide resolved
@Hywan
Copy link
Contributor Author

Hywan commented Jan 13, 2020

Ready for a review!

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! Just a few small nit-picks:

The naming of wasmer_import_trap is a little confusing to me, it sounds like it could give you the handle of something that traps that you could use in your import_object. Not sure what else to call it, though and not a huge issue. wasmer_trap may make more sense, it sounds more like a command to me.

.do_early_trap(Box::new(error_message)); // never returns

#[allow(unreachable_code)]
wasmer_result_t::WASMER_OK
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we should panic here? Seems like a small thing, not sure what best practice is here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cbindgen doesn't generate a C bindgen for a Rust function with ! as returned type.

Since I need to error before calling do_early_trap, I've decided to return a wasmer_result_t. And in the best scenario, I return WASMER_OK, even if it's unreachable. I'll add more documentation to explain.

@@ -219,10 +219,11 @@ pub trait RunnableModule: Send + Sync {
}

/// A wasm trampoline contains the necessary data to dynamically call an exported wasm function.
/// Given a particular signature index, we are returned a trampoline that is matched with that
/// Given a particular signature index, we are returning a trampoline that is matched with that
Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer returned to returning here;

, we return a trampoline... is probably best though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought it was an English typo.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe it's grammatically correct and called the "passive voice", it's often discouraged in English though because it takes more words and ends up being less specific. For example, I generally prefer direct writing like, "This function takes a function signature and returns a corresponding trampoline".

@Hywan
Copy link
Contributor Author

Hywan commented Jan 15, 2020

Go for wasmer_trap :-).

@Hywan Hywan changed the title feat(runtime-c-api) Implement wasmer_import_trap feat(runtime-c-api) Implement wasmer_trap Jan 15, 2020
@Hywan
Copy link
Contributor Author

Hywan commented Jan 15, 2020

bors r+

bors bot added a commit that referenced this pull request Jan 15, 2020
1133: feat(runtime-c-api) Implement `wasmer_trap` r=Hywan a=Hywan

Idea is to provide an API to get fallible host function by calling `wasmer_trap` to run the Wasmer trapping API.

This is probably the easiest solution to not break the existing API, and not add a lot of complexity in the code.

Co-authored-by: Ivan Enderlin <[email protected]>
@bors
Copy link
Contributor

bors bot commented Jan 15, 2020

Build succeeded

@bors bors bot merged commit fb06ee3 into wasmerio:master Jan 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📚 documentation Do you like to read? 🎉 enhancement New feature! 📦 lib-c-api About wasmer-c-api 🧪 tests I love tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants