Stream: git-wasmtime

Topic: wasmtime / PR #3063 Restore POSIX signal handling on MacO...

Wasmtime GitHub notifications bot (Jul 05 2021 at 16:31):

ulan opened PR #3063 from posix-signals-on-macos to main:

As described in Issue #3052, the switch to Mach Exception handling
removed unix::StoreExt from the public API of crate on MacOS.
That is a breaking change and makes it difficult for some
application to upgrade to the current stable Wasmtime.

As a workaround this PR introduces a feature flag called
posix-signals-on-macos that when enabled restores the old behaviour
on MacOS. The flag is disabled by default.

The only non-trivial changes in this PR are in traphandlers/unix.rs that
restore MacOS-related code removed from traphandlers.rs in PR #2723.

<!--

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

Wasmtime GitHub notifications bot (Jul 05 2021 at 16:37):

ulan has marked PR #3063 as ready for review.

Wasmtime GitHub notifications bot (Jul 05 2021 at 16:37):

ulan submitted PR review.

Wasmtime GitHub notifications bot (Jul 05 2021 at 16:37):

ulan submitted PR review.

Wasmtime GitHub notifications bot (Jul 05 2021 at 16:37):

ulan created PR review comment:

Not sure about the name. Bikeshedding is welcome. Possible alternatives: macos-signals, macos-posix-signals.

Wasmtime GitHub notifications bot (Jul 05 2021 at 16:37):

ulan created PR review comment:

This code is taken from https://github.com/bytecodealliance/wasmtime/pull/2723/files#diff-560b66f5eac16eef20c78bcaf2c66582dbc6cf76fa1cc72faf6bca7b736cd660L205 without any changes.

Wasmtime GitHub notifications bot (Jul 05 2021 at 16:37):

ulan created PR review comment:

This code is taken from https://github.com/bytecodealliance/wasmtime/pull/2723/files#diff-560b66f5eac16eef20c78bcaf2c66582dbc6cf76fa1cc72faf6bca7b736cd660L223 without any changes.

Wasmtime GitHub notifications bot (Jul 05 2021 at 16:37):

ulan created PR review comment:

This code is taken from https://github.com/bytecodealliance/wasmtime/pull/2723/files#diff-560b66f5eac16eef20c78bcaf2c66582dbc6cf76fa1cc72faf6bca7b736cd660L124 after renaming Unwind to wasmtime_longjmp. The comment is also adjusted accordingly.

Wasmtime GitHub notifications bot (Jul 05 2021 at 16:59):

ulan updated PR #3063 from posix-signals-on-macos to main.

Wasmtime GitHub notifications bot (Jul 05 2021 at 17:07):

ulan edited PR #3063 from posix-signals-on-macos to main:

As described in Issue #3052, the switch to Mach Exception handling
removed unix::StoreExt from the public API of the crate on MacOS.
That is a breaking change and makes it difficult for some
application to upgrade to the current stable Wasmtime.

As a workaround this PR introduces a feature flag called
posix-signals-on-macos that when enabled restores the old behaviour
on MacOS. The flag is disabled by default.

The only non-trivial changes in this PR are in traphandlers/unix.rs that
restore MacOS-related code removed from traphandlers.rs in PR #2723.

<!--

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

Wasmtime GitHub notifications bot (Jul 12 2021 at 14:05):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jul 12 2021 at 14:05):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jul 12 2021 at 14:05):

alexcrichton created PR review comment:

I think it's probably fine to skip this feature on the wasmtime-cli crate since it's mostly the wasmtime embedding crate that wants this.

Wasmtime GitHub notifications bot (Jul 12 2021 at 20:10):

ulan submitted PR review.

Wasmtime GitHub notifications bot (Jul 12 2021 at 20:10):

ulan created PR review comment:

It is useful for conditionally enabling tests/all/custom_signal_handler.rs on MacOS and running the tests with cargo test --features=posix-signals-on-macos custom_signal_handler
But indeed it's not needed for production. Should I remove it?

Wasmtime GitHub notifications bot (Jul 12 2021 at 20:12):

ulan updated PR #3063 from posix-signals-on-macos to main.

Wasmtime GitHub notifications bot (Jul 12 2021 at 21:25):

alexcrichton merged PR #3063.

Last updated: Oct 23 2024 at 20:03 UTC