A round of Mac M1 fixes #3274

bnjbvr · 2021-09-01T16:05:16Z

Three commits that make the Mac M1 pass all the tests run in CI:

in the custom asm code for the fibers, on aarch64, the custom CFI directive that was indicating where FP (x29) lives was completely incorrect. The pushes in wasmtime_fiber_switch start with stp lr, fp, [sp, -16]!, so FP was at CFA - 0x8, not CFA - 0x60. Unclear why it did work in the first place, and why it stopped working at some point; likely that something in Apple's libunwind has changed in the meanwhile.
the OS page size is 16K, not 4K on Mac M1: a constant is used for the code section page's size, and defined per target, and reused in a few places in the code base. Thanks to @alexcrichton for indicating me the place to look at!
unfortunately we need to disable pointer authentication (until we implement it!) for function frames that don't have a prologue too. Preferred to split gen_prologue_frame_setup, so there's a new function gen_debug_frame_info that's defaulting to doing nothing, and implemented on aarch64 and disabling pointer auth. This function is called independently from the fact that the function requires a prologue/epilogue, so we can put the unwind instruction that will generate the CFI directive in there. cc @cfallin @akirilov-arm for this change

bjorn3 · 2021-09-01T16:46:59Z

crates/cranelift/src/compiler.rs

+pub(crate) const CODE_SECTION_ALIGNMENT: u64 = 0x4000;
+
+#[cfg(not(all(target_os = "macos", target_arch = "aarch64")))]
+pub(crate) const CODE_SECTION_ALIGNMENT: u64 = 0x1000;


At least on macOS you have to query the page size at runtime. This is for example necessary for Rosetta 2 to work as it uses AArch64 sized pages. There may be other cases where it isn't possible to know this at compile time. You can use region::page::size() to query it.

TIL! @alexcrichton in this case, would it be just simpler to call region::page::size() (maybe cache it, if we expect this to be expensive) instead of having the raw constants scattered in the code?

I think a better fix for this would be to inspect self.isa.target() to accomodate cross-compiling use cases. If the section is over-aligned for some embeddings that's not an issue.

Good point, fixed!

…aarch64 Fixes bytecodealliance#3256.

Fixes bytecodealliance#3278.

bnjbvr · 2021-09-02T08:14:06Z

Seems that CI is running cargo check for 1.43 with a comment (from last year) stating it's necessary for Firefox, however Firefox has been updated to use 1.54 according to https://bugzilla.mozilla.org/show_bug.cgi?id=1723016. Can we remove this requirement now? (Of course can tweak the code to not use or patterns.)

alexcrichton

Wasmtime bits look good to me, but someon else may wish to confirm the cranelift changes. Seems like updating rustc for that check is fine to me as well, yeah.

Firefox has updated its toolchain to something much more recent.

bjorn3 · 2021-09-02T14:57:55Z

.github/workflows/main.yml

    - run: cargo check --target wasm32-unknown-emscripten -p wasi-common
    - run: cargo check --target armv7-unknown-linux-gnueabihf -p wasi-common

-    # Check that codegen and wasm crates typecheck on 1.43.0; this is required


Maybe just bump it to a newer version instead of removing it completely?

I was under the impression that Firefox is now following latest stable, so that cargo check step ought to be unnecessary. Plus, unclear whether Cranelift is still being updated from this particular repository or Mozilla's fork. I'll check in with the Mozilla folks, it'd be easy to add back later, if necessary.

Yury from Mozilla confirmed on Matrix mozilla-central is using stable, so we should be good here!

Aren't they updating like one week after a new rust version releases?

It might be, but Cranelift in Firefox seems to receive updates at a lesser frequency (which makes sense for their stability guarantees, and considering the Cranelift bug has been resolved as inactive). Last update was 3 months ago.

cfallin

LGTM on Cranelift/ABI bits as well -- thanks!

akirilov-arm · 2021-09-06T12:15:40Z

@alexcrichton @bnjbvr I have just seen this:

The pushes in wasmtime_fiber_switch start with stp lr, fp, [sp, -16]!, so FP was at CFA - 0x8, not CFA - 0x60. Unclear why it did work in the first place, and why it stopped working at some point; likely that something in Apple's libunwind has changed in the meanwhile.

Is there any specific reason to push the registers in that order? The Procedure Call Standard actually specifies the reverse:

The lowest addressed double-word shall point to the previous frame record and the highest addressed double-word shall contain the value passed in LR on entry to the current function.

bnjbvr · 2021-09-06T12:23:48Z

Is there any specific reason to push the registers in that order? The Procedure Call Standard actually specifies the reverse:

No, no particular reasons. One could easily get confused with the push order in stp...

akirilov-arm · 2021-09-07T11:41:13Z

Perhaps that is partially the cause of the issue with libunwind? I.e. maybe the unwinder has been changed to have stricter frame record layout expectations (in the presence of incorrect CFI information)?

bnjbvr · 2021-09-07T11:45:55Z

Perhaps that is partially the cause of the issue with libunwind? I.e. maybe the unwinder has been changed to have stricter frame record layout expectations (in the presence of incorrect CFI information)?

No: the change in this PR was sufficient to make the test pass, and it wouldn't have been the case if the unwinder had very precise frame record layout expectations. That being said, I'm all in favor of using the most conforming push order :-)

alexcrichton · 2021-09-08T14:23:50Z

Nah there is no reason to push the registers in that order, only that I'd never written arm64 assembly before when I wrote this. I think it would be best to follow arm64 conventions and standards though!

bnjbvr mentioned this pull request Sep 1, 2021

Embark's pull requests EmbarkStudios/rust-ecosystem#20

Open

26 tasks

github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:area:machinst Issues related to instruction selection and the new MachInst backend. cranelift:area:aarch64 Issues related to AArch64 backend. labels Sep 1, 2021

bjorn3 reviewed Sep 1, 2021

View reviewed changes

This was referenced Sep 1, 2021

Use relative call instructions between wasm functions #3275

Merged

Wasmtime: text section alignment is always 4k, but should use the compile target's page size #3278

Closed

Correctly set the address of FP when unwinding from within fibers on …

f871e8c

…aarch64 Fixes bytecodealliance#3256.

bnjbvr force-pushed the fix-m1 branch from f56f933 to 9fc0909 Compare September 2, 2021 07:10

bnjbvr added 2 commits September 2, 2021 09:16

Use 16K code pages on Mac M1

fb94b81

Fixes bytecodealliance#3278.

Aarch64: always generate the CFA directive indicating no pointer signing

85ec11a

bnjbvr force-pushed the fix-m1 branch from 9fc0909 to 85ec11a Compare September 2, 2021 07:16

alexcrichton approved these changes Sep 2, 2021

View reviewed changes

Remove cargo check for old rustc versions

0cf9b2d

Firefox has updated its toolchain to something much more recent.

bjorn3 reviewed Sep 2, 2021

View reviewed changes

cfallin approved these changes Sep 2, 2021

View reviewed changes

cfallin merged commit 2389a4e into bytecodealliance:main Sep 2, 2021

bnjbvr deleted the fix-m1 branch September 2, 2021 16:49

A round of Mac M1 fixes #3274

A round of Mac M1 fixes #3274

Uh oh!

Conversation

bnjbvr commented Sep 1, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnjbvr commented Sep 2, 2021

Uh oh!

alexcrichton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cfallin left a comment

Choose a reason for hiding this comment

Uh oh!

akirilov-arm commented Sep 6, 2021

Uh oh!

bnjbvr commented Sep 6, 2021

Uh oh!

akirilov-arm commented Sep 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bnjbvr commented Sep 7, 2021

Uh oh!

alexcrichton commented Sep 8, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

akirilov-arm commented Sep 7, 2021 •

edited

Loading