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

Mach ports continued + support aarch64-apple unwinding #2723

Merged
merged 6 commits into from
Mar 17, 2021

Conversation

bnjbvr
Copy link
Member

@bnjbvr bnjbvr commented Mar 12, 2021

This is #2632 rebased and continued. Many thanks to @alexcrichton for providing a lot of help with the CFI directives in the custom assembly stub; this really helped simplify the design of frame unwinding.

This is sufficient to make mach ports work on Mac M1, with the slight exception that the system's libunwind on this machine doesn't correctly interpret the CFI directives generated by the assembly stub. As a consequence, the stack trace information for wasm call frames is missing. Retrieving the source of another libunwind implementation, in Apple's LLVM fork or from the opensource.apple.com website, building it and linking it against wasmtime is sufficient to get it working as expected. In addition to this, some experiments showed that the system libunwind's source code doesn't match what's on Apple's repository: this was inferred from looking at an abort error message's line number, and seeing it didn't match the line number for the same message in local sources. As a matter of fact, this is impossible to debug as is. Yet it seemed better to have something that worked, even incompletely. In the future, either we can wait for a new functioning version of OSX libunwind, or we could consider shipping our own, as a small C/C++ dependency.

This is also blocked on fitzgen/mach#64 to be landed and released first.

There's a bit more work to do to fully support Mac M1: I'm seeing WASI failures when trying to run the full test suite, to be handled in a second time.

@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:area:aarch64 Issues related to AArch64 backend. wasmtime:api Related to the API of the `wasmtime` crate itself labels Mar 12, 2021
@github-actions
Copy link

Subscribe to Label Action

cc @peterhuene

This issue or pull request has been labeled: "cranelift", "cranelift:area:aarch64", "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.

@bnjbvr bnjbvr force-pushed the mach-ports-continued branch from 7bbfb62 to 9813c41 Compare March 12, 2021 18:38
@bnjbvr bnjbvr closed this Mar 15, 2021
@bnjbvr bnjbvr reopened this Mar 15, 2021
@bnjbvr bnjbvr force-pushed the mach-ports-continued branch 2 times, most recently from 2013ea2 to 741dc81 Compare March 15, 2021 15:35
@bnjbvr
Copy link
Member Author

bnjbvr commented Mar 15, 2021

I'm hitting this in CI and I've got no clue how to get around it:

E: The repository 'https://download.opensuse.org/repositories/devel:/kubic:/libcontainers:/stable/xUbuntu_18.04  Release' no longer has a Release file.

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

👍

I wonder if the clobbering of fp is causing the issues with the system unwinder perhaps? (just a random shot in the dark)

As for the CI issues, it seems like it's probably related to the image that we're running in (whatever is the default on GitHub Actions). If apt-get update -y doesn't work we're unlikely to be the only ones experiencing it, so I'd imagine it'll be fixed in a day or so.

crates/wasmtime/src/store.rs Outdated Show resolved Hide resolved
crates/runtime/src/traphandlers/macos.rs Show resolved Hide resolved
crates/runtime/src/traphandlers/macos.rs Outdated Show resolved Hide resolved
crates/runtime/src/traphandlers/macos.rs Outdated Show resolved Hide resolved
crates/runtime/src/traphandlers/macos_aarch64.S Outdated Show resolved Hide resolved
Copy link
Member

@cfallin cfallin left a comment

Choose a reason for hiding this comment

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

Thanks @bnjbvr! 👍 for the unwinding-info / return-pointer-auth stuff in particular. I'm a little lost on the stack frame manipulation / stub insertion (a figure or summary might help?) but the general gist seems fine; I'll defer to others for the +1 on that though :-)

crates/runtime/src/traphandlers/macos.rs Show resolved Hide resolved
crates/runtime/src/traphandlers/macos_aarch64.S Outdated Show resolved Hide resolved
crates/runtime/src/traphandlers/macos_aarch64.S Outdated Show resolved Hide resolved
crates/runtime/src/traphandlers/macos.rs Outdated Show resolved Hide resolved
@bnjbvr bnjbvr force-pushed the mach-ports-continued branch from 741dc81 to 1351f35 Compare March 16, 2021 13:48
@bnjbvr
Copy link
Member Author

bnjbvr commented Mar 16, 2021

Thanks for the reviews! This will require a review of fitzgen/mach#64, which I can incorporate in this PR, if it's simpler -- would be nice to have the code be in the right places, though :)

alexcrichton and others added 4 commits March 17, 2021 10:14
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
Aarch64 post ARMv8.3 has a feature called pointer authentication,
designed to fight ROP/JOP attacks: some pointers may be signed using new
instructions, adding payloads to the high (previously unused) bits of
the pointers. More on this here: https://lwn.net/Articles/718888/

Unwinders on aarch64 need to know if some pointers contained on the call
frame contain an authentication code or not, to be able to properly
authenticate them or use them directly. Since native code may have
enabled it by default (as is the case on the Mac M1), and the default is
that this configuration value is inherited, we need to explicitly
disable it, for the only kind of supported pointers (return addresses).

To do so, we set the value of a non-existing dwarf pseudo register (34)
to 0, as documented in
https://github.com/ARM-software/abi-aa/blob/master/aadwarf64/aadwarf64.rst#note-8.

This is done at the function granularity, in the spirit of Cranelift
compilation model. Alternatively, a single directive could be generated
in the CIE, generating less information per module.
@bnjbvr bnjbvr force-pushed the mach-ports-continued branch from ef3c1df to 8a4e93c Compare March 17, 2021 09:32
@bnjbvr
Copy link
Member Author

bnjbvr commented Mar 17, 2021

Now not depending on the Mach PR anymore; should make the verify CI task happy!

@bnjbvr bnjbvr force-pushed the mach-ports-continued branch from 8a4e93c to 8fa9859 Compare March 17, 2021 10:13
@alexcrichton
Copy link
Member

To confirm, am I correct in understanding that this PR now serves two purposes and is reading for landing:

  1. First it fixes the combination of wasmtime and breakpad on macOS by using mach ports for exceptions instead of signal handlers.
  2. Second it adds a small bit of support for AArch64 mac machines to the point where we can "catch" wasm traps but the backtraces are incorrect. (maybe only one frame of wasm instead of all the frames)

Does that sound right? If so seems reasonable to me to land!

@bnjbvr
Copy link
Member Author

bnjbvr commented Mar 17, 2021

Yes, this is entirely correct! MacOS CI passing suggests that the backtraces are still properly filled (i.e. contain all the frames) on darwin x86_64.

One note for readers: it's possible to get all the frames in backtrace by using a custom build of libunwind:

  • clone and build libunwind from LLVM's repository. Say /tmp/libunwind_build/ contains the build artifacts.
  • in crates/jit/src/unwind/system.rs, add #[link(name = "unwind", kind = "static")] on top of the extern "C" block that defines __register_frame.
  • build with RUSTFLAGS="-L /tmp/libunwind_build/" cargo build

This is sufficient to get all the frames appearing on backtraces on mac aarch64.

@alexcrichton
Copy link
Member

Ok! Let's go ahead and land this to get to a more working state along those two points, and we can figure out later how to best handle the unwinding intricacies on macOS AArch64

@alexcrichton alexcrichton merged commit 5fecdfa into bytecodealliance:main Mar 17, 2021
@bnjbvr bnjbvr deleted the mach-ports-continued branch March 17, 2021 15:19
@bnjbvr bnjbvr restored the mach-ports-continued branch March 17, 2021 15:19
@bnjbvr bnjbvr deleted the mach-ports-continued branch April 6, 2021 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift:area:aarch64 Issues related to AArch64 backend. cranelift Issues related to the Cranelift code generator wasmtime:api Related to the API of the `wasmtime` crate itself
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants