Stream: git-wasmtime

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


view this post on Zulip 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.

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).

view this post on Zulip 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.

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).

view this post on Zulip Wasmtime GitHub notifications bot (May 05 2022 at 21:59):

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

view this post on Zulip Wasmtime GitHub notifications bot (May 09 2022 at 22:25):

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

view this post on Zulip Wasmtime GitHub notifications bot (May 09 2022 at 22:51):

saulecabrera has marked PR #4105 as ready for review.

view this post on Zulip 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.

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.

view this post on Zulip Wasmtime GitHub notifications bot (May 09 2022 at 23:02):

saulecabrera submitted PR review.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (May 10 2022 at 15:27):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (May 10 2022 at 15:27):

alexcrichton submitted PR review.

view this post on Zulip 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

view this post on Zulip Wasmtime GitHub notifications bot (May 10 2022 at 15:27):

alexcrichton merged PR #4105.


Last updated: Dec 23 2024 at 13:07 UTC