Stream: git-wasmtime

Topic: wasmtime / PR #4105 Expose `TrapCode::Interrupt` on epoch...

Wasmtime GitHub notifications bot (May 05 2022 at 21:30):

saulecabrera opened PR #4105 from expose-trap-code-on-epoch-interruption to main:

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

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

I'm opening the PR as a draft, because I'm unsure if the usage of raise_lib_trap is correct in this context. I'm assuming it is, and at the same time it seems to be the only way to set the trap code. Also, if there aren't any issues with the usage of raise_lib_trap it seems that the error (EpochDeadlineError) is not needed anymore, and it would be safe to remove the implementation (I'll do so upon confirmation).

Wasmtime GitHub notifications bot (May 05 2022 at 21:42):

saulecabrera edited PR #4105 from expose-trap-code-on-epoch-interruption to main:

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

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.

I'm opening the PR as a draft, because I'm unsure if the usage of raise_lib_trap is correct in this context. I'm assuming it is, and at the same time it seems to be the only way to set the trap code. Also, if there aren't any issues with the usage of raise_lib_trap it seems that the error (EpochDeadlineError) is not needed anymore, and it would be safe to remove the implementation (I'll do so upon confirmation).

Wasmtime GitHub notifications bot (May 05 2022 at 21:59):

saulecabrera updated PR #4105 from expose-trap-code-on-epoch-interruption to main.

Wasmtime GitHub notifications bot (May 09 2022 at 22:25):

saulecabrera updated PR #4105 from expose-trap-code-on-epoch-interruption to main.

Wasmtime GitHub notifications bot (May 09 2022 at 22:51):

saulecabrera has marked PR #4105 as ready for review.

Wasmtime GitHub notifications bot (May 09 2022 at 22:56):

saulecabrera edited PR #4105 from expose-trap-code-on-epoch-interruption to main:

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

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.

Wasmtime GitHub notifications bot (May 09 2022 at 23:02):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (May 09 2022 at 23:02):

saulecabrera created PR review comment:

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.

Wasmtime GitHub notifications bot (May 10 2022 at 15:27):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (May 10 2022 at 15:27):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (May 10 2022 at 15:27):

alexcrichton created PR review comment:

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

Wasmtime GitHub notifications bot (May 10 2022 at 15:27):

alexcrichton merged PR #4105.

Last updated: Nov 22 2024 at 17:03 UTC