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

Expose TrapCode::Interrupt on epoch based interruption #4105

Conversation

saulecabrera
Copy link
Member

@saulecabrera saulecabrera commented May 5, 2022

I had a discussion via Zulip with @alexcrichton about this change. I decided to expose the TrapCode instead of the private error (EpochDeadlineError) mainly to keep the same behaviour as the deprecated InterruptHandle approach.

@saulecabrera saulecabrera force-pushed the expose-trap-code-on-epoch-interruption branch from 95ade38 to 9b06286 Compare May 5, 2022 21:59
@alexcrichton
Copy link
Member

Using the Interrupt trap code here seems reasonable to me, but instead of discarding the trap that's returned could the insertion and customization of the TrapCode be done when the trap is first created? (e.g. in the new_epoch handler within wasmtime)

@saulecabrera saulecabrera force-pushed the expose-trap-code-on-epoch-interruption branch from 9b06286 to 8d94f5b Compare May 9, 2022 22:25
@saulecabrera saulecabrera marked this pull request as ready for review May 9, 2022 22:51
@saulecabrera
Copy link
Member Author

Thanks for the feedback @alexcrichton! I agree, that's a much cleaner solution, I had overlooked this approach due a misunderstanding in the relationship between anyhow::Error and wasmtime::Trap.

@@ -1856,7 +1856,14 @@ unsafe impl<T> wasmtime_runtime::Store for StoreInner<T> {

fn new_epoch(&mut self) -> Result<u64, anyhow::Error> {
return match &self.epoch_deadline_behavior {
&EpochDeadline::Trap => Err(anyhow::Error::new(EpochDeadlineError)),
&EpochDeadline::Trap => {
let trap = Trap::new_wasm(
Copy link
Member Author

Choose a reason for hiding this comment

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

It seems that this call could be simplified in this case by exposing a Trap::from_trap_code function in the Trap implementation, which would take in the TrapCode and apply the corresponding defaults for the pc and backtrace.

Didn't introduce this change right away since this is the only use case but I can totally do it if you think it's worth it.

Copy link
Member

Choose a reason for hiding this comment

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

This seems reasonable enough to me for now, if this pops up more often we can make a helper for it

@github-actions github-actions bot added the wasmtime:api Related to the API of the `wasmtime` crate itself label May 9, 2022
@github-actions
Copy link

github-actions bot commented May 9, 2022

Subscribe to Label Action

cc @peterhuene

This issue or pull request has been labeled: "wasmtime:api"

Thus the following users have been cc'd because of the following labels:

  • peterhuene: wasmtime:api

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

@@ -1856,7 +1856,14 @@ unsafe impl<T> wasmtime_runtime::Store for StoreInner<T> {

fn new_epoch(&mut self) -> Result<u64, anyhow::Error> {
return match &self.epoch_deadline_behavior {
&EpochDeadline::Trap => Err(anyhow::Error::new(EpochDeadlineError)),
&EpochDeadline::Trap => {
let trap = Trap::new_wasm(
Copy link
Member

Choose a reason for hiding this comment

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

This seems reasonable enough to me for now, if this pops up more often we can make a helper for it

@alexcrichton alexcrichton merged commit 52524d2 into bytecodealliance:main May 10, 2022
@saulecabrera saulecabrera deleted the expose-trap-code-on-epoch-interruption branch May 24, 2022 19:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasmtime:api Related to the API of the `wasmtime` crate itself
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants