Stream: git-wasmtime

Topic: wasmtime / PR #9266 Implement unwind support for ARM64 Wi...

view this post on Zulip Wasmtime GitHub notifications bot (Sep 17 2024 at 16:26):

dpaoliello opened PR #9266 from dpaoliello:arm64unwind to bytecodealliance:main:

THIS IS A WORK IN PROGRESS.

Current status: tests partially succeeding, but there is a crash in JITted code.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 17 2024 at 16:26):

dpaoliello edited PR #9266:

THIS IS A WORK IN PROGRESS.

Current status: tests partially succeeding, but there is a crash in JITted code.

Fixes #4992

view this post on Zulip Wasmtime GitHub notifications bot (Sep 17 2024 at 16:41):

dpaoliello edited PR #9266:

THIS IS A WORK IN PROGRESS.

Current status: tests partially succeeding, but there is a crash due to a malformed unwind table.

Fixes #4992

view this post on Zulip Wasmtime GitHub notifications bot (Sep 17 2024 at 18:03):

dpaoliello updated PR #9266.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 17 2024 at 20:47):

alexcrichton commented on PR #9266:

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.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 17 2024 at 22:21):

dpaoliello updated PR #9266.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 18 2024 at 17:24):

dpaoliello updated PR #9266.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 18 2024 at 18:01):

dpaoliello updated PR #9266.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 18 2024 at 18:08):

dpaoliello edited PR #9266:

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

Fixes #4992

view this post on Zulip Wasmtime GitHub notifications bot (Sep 18 2024 at 18:09):

dpaoliello edited PR #9266:

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

view this post on Zulip Wasmtime GitHub notifications bot (Sep 18 2024 at 18:10):

dpaoliello has marked PR #9266 as ready for review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 18 2024 at 18:10):

dpaoliello requested wasmtime-compiler-reviewers for a review on PR #9266.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 18 2024 at 18:10):

dpaoliello requested abrown for a review on PR #9266.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 18 2024 at 18:10):

dpaoliello requested fitzgen for a review on PR #9266.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 18 2024 at 18:10):

dpaoliello requested wasmtime-core-reviewers for a review on PR #9266.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 18 2024 at 18:25):

cfallin submitted PR review:

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

view this post on Zulip Wasmtime GitHub notifications bot (Sep 18 2024 at 18:25):

cfallin created PR review comment:

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?

view this post on Zulip Wasmtime GitHub notifications bot (Sep 18 2024 at 18:25):

fitzgen submitted PR review:

LGTM modulo one nitpick below

view this post on Zulip Wasmtime GitHub notifications bot (Sep 18 2024 at 18:25):

fitzgen created PR review comment:

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.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 18 2024 at 18:43):

dpaoliello commented on PR #9266:

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.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 18 2024 at 18:43):

dpaoliello updated PR #9266.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 18 2024 at 18:44):

dpaoliello submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 18 2024 at 18:44):

dpaoliello created PR review comment:

Done: removes the as casts.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 18 2024 at 18:44):

dpaoliello edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 18 2024 at 18:44):

dpaoliello submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 18 2024 at 18:44):

dpaoliello created PR review comment:

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

view this post on Zulip Wasmtime GitHub notifications bot (Sep 18 2024 at 19:07):

alexcrichton commented on PR #9266:

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.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 18 2024 at 19:38):

dpaoliello commented on PR #9266:

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.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 19 2024 at 15:35):

fitzgen submitted PR review:

Thanks so much!

view this post on Zulip Wasmtime GitHub notifications bot (Sep 19 2024 at 16:08):

fitzgen commented on PR #9266:

Looks like this is failing on our MSRV:

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

view this post on Zulip Wasmtime GitHub notifications bot (Sep 19 2024 at 16:08):

fitzgen edited a comment on PR #9266:

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

view this post on Zulip Wasmtime GitHub notifications bot (Sep 19 2024 at 16:26):

dpaoliello updated PR #9266.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 19 2024 at 16:26):

dpaoliello commented on PR #9266:

Looks like this is failing on our MSRV of Rust 1.78

Fixed

view this post on Zulip Wasmtime GitHub notifications bot (Sep 19 2024 at 16:31):

cfallin has enabled auto merge for PR #9266.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 19 2024 at 16:55):

cfallin merged PR #9266.

Last updated: Nov 22 2024 at 16:03 UTC