Implement unwind support for ARM64 Windows #9266

dpaoliello · 2024-09-17T16:26:10Z

Implements unwind support for ARM64 Windows.

This also fixes an issue with the current AArch64 ABI code where it wasn't emitting unwind instructions when adjusting the stack pointer (i.e., doing a stack alloc).

With this change, cargo test passes all tests on my Windows ARM64 device.

Fixes #4992

alexcrichton · 2024-09-17T20:47:53Z

At a high level this looks great, thanks for working on this! We'll probably be trusting you on the particulars of the table format here, and we unfortunately also probably won't be able to run this on CI (unless you know of way to run this and/or emulation on CI). That being said once you're able to run tests locally that's probably a good enough sign for now and should be good to land afterwards.

It may also be worth noting that the test failures may not be related to unwinding necessarily, I believe Wasmtime has seen very little testing on Windows ARM64 meaning that there could be little other things here and there to overcome as well.

cfallin

I made a pass over this code as well and it's generally very high-quality, thanks!

To confirm specifically: does the whole Wasmtime suite (cargo test in the root) pass now on Windows/aarch64?

Echoing Alex's thoughts -- if we can test this somehow (WINE on qemu-aarch64? or does Windows/x64 support running aarch64 binaries with emulation somehow? or ...?) then we can add release binaries.

(There's precedent for adding release binaries before that, from the macOS/aarch64 case -- we just got GitHub runners for that a few months ago -- though in that case there was external CI run by a third party and those of us with new Macs also manually checked when able.)

cfallin · 2024-09-18T18:20:07Z

cranelift/codegen/src/isa/unwind/winarm64.rs

+                const LARGE_STACK_ALLOC_MAX: u32 = (1 << 24) * 16;
+                match size {
+                    0..SMALL_STACK_ALLOC_MAX => {
+                        unwind_codes.push(UnwindCode::SmallStackAlloc { size: size as u16 })


At least in new code, we're trying to avoid as-casts; even though here it's infallibly correct, could we write u16::try_from(size).unwrap() (or .expect("constant max size for this case exceeds u16") or somesuch so that auditing for overflows is a little easier?

Done: removed the as casts.

fitzgen

LGTM modulo one nitpick below

fitzgen · 2024-09-18T18:20:57Z

cranelift/codegen/src/isa/unwind/winarm64.rs

+/// The supported unwind codes for the Arm64 Windows ABI.
+///
+/// See: <https://learn.microsoft.com/en-us/cpp/build/arm64-exception-handling>
+/// Only what is needed to describe the prologues generated by the Cranelift AArch64 ISA are represented here.
+#[allow(dead_code)]
+#[derive(Clone, Debug, PartialEq, Eq)]
+#[cfg_attr(feature = "enable-serde", derive(Serialize, Deserialize))]
+pub(crate) enum UnwindCode {
+    /// Save int register, or register pair.
+    SaveReg {
+        reg: u8,
+        stack_offset: u16,
+        is_pair: bool,
+    },
+    /// Save floating point register, or register pair.
+    SaveFReg {
+        reg: u8,
+        stack_offset: u16,
+        is_pair: bool,
+    },
+    /// Save frame-pointer register (X29) and LR register pair.
+    SaveFpLrPair {
+        stack_offset: u16,
+    },
+    SmallStackAlloc {
+        size: u16,
+    },
+    MediumStackAlloc {
+        size: u16,
+    },
+    LargeStackAlloc {
+        size: u32,
+    },
+    /// PAC sign the LR register.
+    PacSignLr,
+    /// Set the frame-pointer register to the stack-pointer register.
+    SetFp,
+    /// Set the frame-pointer register to the stack-pointer register with an
+    /// offset.
+    AddFp {
+        offset: u16,
+    },
+}


Can the names of each of these individual codes match the name in the link above? Specifically SmallStackAlloc is called alloc_s there so I'd expect it to be AllocS here.

Also, it would be nice to have doc comments for the variants that are missing them. For example, the difference between small vs medium stack alloc isn't immediately clear to the reader because they have the same u16 data representation.

Done: Renamed the alloc items and added a comment to explain the max size of each one.

dpaoliello · 2024-09-18T18:43:25Z

To confirm specifically: does the whole Wasmtime suite (cargo test in the root) pass now on Windows/aarch64?

Correct: cargo test in the root passes on an ARM64 Windows device.

Echoing Alex's thoughts -- if we can test this somehow (WINE on qemu-aarch64? or does Windows/x64 support running aarch64 binaries with emulation somehow? or ...?) then we can add release binaries.

That's a tough one.

I've never tried WINE on AArch64 (emulated or physical), so I can't comment on that.

Windows x64 can't run AArch64 binaries.

GitHub does offer Windows ARM64 runners however the only image available is a clean Windows 11 install (No Visual Studio, no Git, etc.). The lack of an image with pre-installed tools is also blocking making aarch64-pc-windows-msvc a tier 1 Rust target, so it's high on my priority list to get the correct team at GitHub/Microsoft to make and support those images.

alexcrichton · 2024-09-18T19:07:07Z

If you're interested @dpaoliello the "fully exhaustive test suite" can be run with ./ci/run-tests.sh locally (and/or copying out the cargo invocation since you probably don't have bash on windows) which basically is cargo test --workspace with a few --exclude for things that aren't supposed to be tested. That will run all wasmtime-wasi tests for example which exercise some "larger modules" than the cargo test does at the root of the repo. (but for development most of us I think do just cargo test and let CI figure everything else out). I mention this if you want to perform an extra double-check that all the various bits are working, but I suspect that if cargo test works this won't turn up much else.

I'm also happy to leave this untested on CI until it's easier to set up CI with tools and such. The likelihood of this regressing is relatively low and we can always ping you @dpaoliello (if you're ok with that) if something looks like it's changing. Otherwise I'm also happy to investigate adding ARM64 binaries for Windows to our CI to get release artifacts as well.

dpaoliello · 2024-09-18T19:38:45Z

If you're interested @dpaoliello the "fully exhaustive test suite" can be run with ./ci/run-tests.sh locally (and/or copying out the cargo invocation since you probably don't have bash on windows) which basically is cargo test --workspace with a few --exclude for things that aren't supposed to be tested. That will run all wasmtime-wasi tests for example which exercise some "larger modules" than the cargo test does at the root of the repo. (but for development most of us I think do just cargo test and let CI figure everything else out). I mention this if you want to perform an extra double-check that all the various bits are working, but I suspect that if cargo test works this won't turn up much else.

Sure, I'll give it a go. Otherwise, I was starting to look into getting rustc_codegen_cranelift working.

I'm also happy to leave this untested on CI until it's easier to set up CI with tools and such.

I'll add it to my list of repos to setup CIs once the Windows ARM64 images are available...

we can always ping you @dpaoliello (if you're ok with that) if something looks like it's changing.

Yes, please!
Feel free to summon me for any Rust -msvc target or Windows on ARM issue.

fitzgen

Thanks so much!

fitzgen · 2024-09-19T16:08:08Z

Looks like this is failing on our MSRV of Rust 1.78:

https://github.com/bytecodealliance/wasmtime/actions/runs/10944294333/job/30385843729#step:18:102

error[E0658]: exclusive range pattern syntax is experimental
   --> cranelift/codegen/src/isa/unwind/winarm64.rs:231:21
    |
231 |                     0..SMALL_STACK_ALLOC_MAX => unwind_codes.push(UnwindCode::AllocS {
    |                     ^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: see issue #37854 <https://github.com/rust-lang/rust/issues/37854> for more information
    = help: use an inclusive range pattern, like N..=M

error[E0658]: exclusive range pattern syntax is experimental
   --> cranelift/codegen/src/isa/unwind/winarm64.rs:234:21
    |
234 |                     SMALL_STACK_ALLOC_MAX..MEDIUM_STACK_ALLOC_MAX => {
    |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: see issue #37854 <https://github.com/rust-lang/rust/issues/37854> for more information
    = help: use an inclusive range pattern, like N..=M

error[E0658]: exclusive range pattern syntax is experimental
   --> cranelift/codegen/src/isa/unwind/winarm64.rs:239:21
    |
239 |                     MEDIUM_STACK_ALLOC_MAX..LARGE_STACK_ALLOC_MAX => {
    |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: see issue #37854 <https://github.com/rust-lang/rust/issues/37854> for more information
    = help: use an inclusive range pattern, like N..=M

dpaoliello · 2024-09-19T16:26:47Z

Looks like this is failing on our MSRV of Rust 1.78

Fixed

With bytecodealliance#9266 Wasmtime should have full support for this platform. While not tested in CI it can still be useful to build artifacts nonetheless in case users are interested.

With bytecodealliance#9266 Wasmtime should have full support for this platform. While not tested in CI it can still be useful to build artifacts nonetheless in case users are interested. prtest:full

With #9266 Wasmtime should have full support for this platform. While not tested in CI it can still be useful to build artifacts nonetheless in case users are interested. prtest:full

Wasmtime recently completed Windows ARM64 support with bytecodealliance/wasmtime#9266 and added release artifacts with bytecodealliance/wasmtime#9283. Fixes bytecodealliance#298

dpaoliello mentioned this pull request Sep 17, 2024

Trap handling for Windows ARM64 #4992

Closed

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 Sep 17, 2024

dpaoliello force-pushed the arm64unwind branch from 11ae3c4 to 76c7e35 Compare September 17, 2024 18:03

dpaoliello force-pushed the arm64unwind branch 3 times, most recently from 24d6330 to 9804955 Compare September 18, 2024 18:01

dpaoliello marked this pull request as ready for review September 18, 2024 18:10

dpaoliello requested review from a team as code owners September 18, 2024 18:10

dpaoliello requested review from abrown and fitzgen and removed request for a team September 18, 2024 18:10

cfallin approved these changes Sep 18, 2024

View reviewed changes

fitzgen approved these changes Sep 18, 2024

View reviewed changes

dpaoliello force-pushed the arm64unwind branch from 9804955 to 1577f30 Compare September 18, 2024 18:43

fitzgen approved these changes Sep 19, 2024

View reviewed changes

fitzgen added this pull request to the merge queue Sep 19, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 19, 2024

Implement unwind support for ARM64 Windows

43926a3

dpaoliello force-pushed the arm64unwind branch from 1577f30 to 43926a3 Compare September 19, 2024 16:26

cfallin enabled auto-merge September 19, 2024 16:31

cfallin added this pull request to the merge queue Sep 19, 2024

Merged via the queue into bytecodealliance:main with commit 4e50ecc Sep 19, 2024
39 checks passed

alexcrichton mentioned this pull request Sep 19, 2024

Add release artifacts for ARM64 Windows #9283

Merged

kpreisser mentioned this pull request Sep 23, 2024

Add Wasmtime Windows ARM64 (AArch64) binary bytecodealliance/wasmtime-dotnet#327

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement unwind support for ARM64 Windows #9266

Implement unwind support for ARM64 Windows #9266

dpaoliello commented Sep 17, 2024 •

edited

Loading

alexcrichton commented Sep 17, 2024

cfallin left a comment

cfallin Sep 18, 2024

dpaoliello Sep 18, 2024 •

edited

Loading

fitzgen left a comment

fitzgen Sep 18, 2024

dpaoliello Sep 18, 2024

dpaoliello commented Sep 18, 2024

alexcrichton commented Sep 18, 2024

dpaoliello commented Sep 18, 2024

fitzgen left a comment

fitzgen commented Sep 19, 2024 •

edited

Loading

dpaoliello commented Sep 19, 2024

Implement unwind support for ARM64 Windows #9266

Implement unwind support for ARM64 Windows #9266

Conversation

dpaoliello commented Sep 17, 2024 • edited Loading

alexcrichton commented Sep 17, 2024

cfallin left a comment

Choose a reason for hiding this comment

cfallin Sep 18, 2024

Choose a reason for hiding this comment

dpaoliello Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

fitzgen left a comment

Choose a reason for hiding this comment

fitzgen Sep 18, 2024

Choose a reason for hiding this comment

dpaoliello Sep 18, 2024

Choose a reason for hiding this comment

dpaoliello commented Sep 18, 2024

alexcrichton commented Sep 18, 2024

dpaoliello commented Sep 18, 2024

fitzgen left a comment

Choose a reason for hiding this comment

fitzgen commented Sep 19, 2024 • edited Loading

dpaoliello commented Sep 19, 2024

dpaoliello commented Sep 17, 2024 •

edited

Loading

dpaoliello Sep 18, 2024 •

edited

Loading

fitzgen commented Sep 19, 2024 •

edited

Loading