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

Switch macOS to using mach ports for trap handling #2632

Closed
wants to merge 2 commits into from

Conversation

alexcrichton
Copy link
Member

This commit moves macOS to using mach ports instead of signals for
handling traps. The motivation for this is listed in #2456, namely that
once mach ports are used in a process that means traditional UNIX signal
handlers won't get used. This means that if Wasmtime is integrated with
Breakpad, for example, then Wasmtime's trap handler never fires and
traps don't work.

The traphandlers module is refactored as part of this commit to split
the platform-specific bits into their own files (it was growing quite a
lot for one inline cfg_if!). The unix.rs and windows.rs files
remain the same as they were before with a few minor tweaks for some
refactored interfaces. The macos.rs file is brand new and lifts almost
its entire implementation from SpiderMonkey, adapted for Wasmtime
though.

The main gotcha with mach ports is that a separate thread is what
services the exception. Some unsafe magic allows this separate thread to
read non-Send and temporary state from other threads, but is hoped to
be safe in this context. The unfortunate downside is that calling wasm
on macOS now involves taking a global lock and modifying a global hash
map twice-per-call. I'm not entirely sure how to get out of this cost
for now, but hopefully for any embeddings on macOS it's not the end of
the world.

Closes #2456

This commit moves macOS to using mach ports instead of signals for
handling traps. The motivation for this is listed in bytecodealliance#2456, namely that
once mach ports are used in a process that means traditional UNIX signal
handlers won't get used. This means that if Wasmtime is integrated with
Breakpad, for example, then Wasmtime's trap handler never fires and
traps don't work.

The `traphandlers` module is refactored as part of this commit to split
the platform-specific bits into their own files (it was growing quite a
lot for one inline `cfg_if!`). The `unix.rs` and `windows.rs` files
remain the same as they were before with a few minor tweaks for some
refactored interfaces. The `macos.rs` file is brand new and lifts almost
its entire implementation from SpiderMonkey, adapted for Wasmtime
though.

The main gotcha with mach ports is that a separate thread is what
services the exception. Some unsafe magic allows this separate thread to
read non-`Send` and temporary state from other threads, but is hoped to
be safe in this context. The unfortunate downside is that calling wasm
on macOS now involves taking a global lock and modifying a global hash
map twice-per-call. I'm not entirely sure how to get out of this cost
for now, but hopefully for any embeddings on macOS it's not the end of
the world.

Closes bytecodealliance#2456
@alexcrichton
Copy link
Member Author

One thing worth pointing out is that I believe Wasmtime currently complies for arm64 macOS (although it doesn't work AFAIK), but this PR would break that. Structures for the thread state would need to be copied over from various headers to get that working.

@Jake-Shadle
Copy link

We actually run aarch64 mac CI on our project that uses wasmtime, so I think we can just add a PR that builds off of this one for the aarch64 specific things that would need to change. 🙂

@Jake-Shadle
Copy link

Oh wait, I was just reminded that we might not be able to verify this PR on the aarch64 mac though, due to #2552, we actually had to disable the tests we had that purposefully paniced inside the wasm module for aarch64 mac due to it actually crashing our host program.

@github-actions github-actions bot added the wasmtime:api Related to the API of the `wasmtime` crate itself label Feb 3, 2021
@github-actions
Copy link

github-actions bot commented Feb 3, 2021

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.

@alexcrichton
Copy link
Member Author

Ah ok, I didn't realize it already worked well enough to at least have some testing! I've pushed up d07d29b although it is entirely untested on my end (I don't have arm64 hardware). It should compile but I suspect at least some tests fail.

@Jake-Shadle
Copy link

Great, thanks!

alexcrichton added a commit to alexcrichton/wasmtime that referenced this pull request Feb 22, 2021
This commit fixes an issue discovered in the wasmtime-go bindings when
the Go runtime was crashing on macOS only when running wasm code that
trapped. It turns out that our switch to `siglongjmp` from `longjmp`
actually broke macOS! This breakage happens because all subsequent
signals after the first signal are all delivered on the main stack, not
the sigaltstack, even if the sigaltstack is configured. This causes the
Go runtime to crash since it expects to run on the sigaltstack.

The fix in this commit is to actually return from the signal handler to
trigger the kernel's updating of the sigaltstack no longer being in use.
Before we return, however, we configure the register context to return
to to call some custom code which immediately does the unwind we would
otherwise have done. This works around the issue on macOS hopefully
without adding too many portability problems. Ideally this will all go
away as well with bytecodealliance#2632 as well.
alexcrichton added a commit that referenced this pull request Feb 23, 2021
* Fix preservation of the sigaltstack on macOS

This commit fixes an issue discovered in the wasmtime-go bindings when
the Go runtime was crashing on macOS only when running wasm code that
trapped. It turns out that our switch to `siglongjmp` from `longjmp`
actually broke macOS! This breakage happens because all subsequent
signals after the first signal are all delivered on the main stack, not
the sigaltstack, even if the sigaltstack is configured. This causes the
Go runtime to crash since it expects to run on the sigaltstack.

The fix in this commit is to actually return from the signal handler to
trigger the kernel's updating of the sigaltstack no longer being in use.
Before we return, however, we configure the register context to return
to to call some custom code which immediately does the unwind we would
otherwise have done. This works around the issue on macOS hopefully
without adding too many portability problems. Ideally this will all go
away as well with #2632 as well.

* Fix compile warning
@bnjbvr
Copy link
Member

bnjbvr commented Mar 17, 2021

I've based my work in #2723 from this PR, which has been rebased and tweaked. Thanks a lot @alexcrichton for all the initial work on this!

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.

Consider adding support for MacOS exception ports
3 participants