Windows fprs preservation #1216

iximeow · 2020-03-03T18:42:54Z

Moved from bytecodealliance/cranelift#1378.

This has been discussed in issue Windows (Fastcall) calling convention: Callee-saved XMM (FP) registers are not actually saved #1177
A short description: Windows appears to have extended ABI constraints to require callees to save XMM6-XMM15 (see Windows (Fastcall) calling convention: Callee-saved XMM (FP) registers are not actually saved #1177 for links to reference material). This PR adds preserve/restore behavior for those registers.
This PR contains test cases.
A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so and/or ping
bnjbvr. The list of suggested reviewers on the right can help you.

There were a few unresolved conversations from the original PR that I'll copy below.

cranelift/filetests/filetests/isa/x86/windows_fastcall_x64_unwind.clif

iximeow · 2020-03-03T19:01:07Z

cranelift/filetests/filetests/isa/x86/windows_fastcall_x64.clif

+; nextln:                         store notrap aligned v8, v7
+; nextln:                         store notrap aligned v9, v7+16
+; check:                          v10 = stack_addr.i64 ss0
+; nextln:                         v11 = load.f64 notrap aligned v10


I had a comment in the original PR referencing a line I couldn't get to test properly here, but would make me happier these tests being robust in the face of the future.

The exact text doesn't match anymore but the same idea applies. I would like to test that preserves and restores have the right registers. In theory a nextln like

; nextln: [Mp2fld#710,%xmm6] v11 = load.f64 notrap aligned v10

would do the trick, but this yields a test error like

> [RexOp1spaddr8_id#808d,%rbp] v10 = stack_addr.i64 ss0 ^~~~~~~~~~~~~~~~~~~~~~~~ Matched #10: \bv10 = stack_addr\.i64 ss0\b > [Mp2fld#710,%xmm6] v11 = load.f64 notrap aligned v10 Missed #11: \[Mp2fld\#710,%xmm6\] v11 = load\.f64 notrap aligned v10\b

which is suspicious because the only difference on the matching lines is whitespace. When whitespace is equalized, this still yields an error. We use specific checks like this in other filetests also run as test compile, so I suspect this is a bug in the filetest runner?

I would greatly appreciate thoughts if anyone is particularly familiar with filetest.

cranelift/codegen/src/isa/x86/abi.rs

…ytecodealliance#1224)

…solved (bytecodealliance#1224)" This reverts commit 589fa95.

cranelift/filetests/filetests/isa/x86/windows_fastcall_x64_unwind.clif

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

iximeow · 2020-03-11T19:43:33Z

The CI failure is due to tripping this check that we wouldn't misreport unwind information in cases where we have callee-save floating-point registers. I tried reproducing this locally by building the module in multi_func_value/func.wast by clif-util wasm funct_module.wast --target x86_64-windows -D, but for some reason that seems to build fine, using a significantly smaller amount of stack space? I ran clif-util from Linux, but the only thing that should be a factor here is the target specification. I'm not sure why it doesn't reproduce this way. Is there a flag I should set that influences lowering to clif as well?

Anyway, I printed the faulting prologue in CI to double-check that the panic is correctly hit, and indeed a function in the module uses well over the 240-byte limit I've noted above:

x86_push.i64 v105
offset 2: copy_special %rsp -> %rbp 
offset 5: x86_push.i64 v106 
offset 7: x86_push.i64 v107 
offset 9: x86_push.i64 v108 
offset 11: x86_push.i64 v109 
offset 13: x86_push.i64 v110 
offset 15: x86_push.i64 v111 
offset 17: adjust_sp_down_imm 336
offset 24: v112 = stack_addr.i64 ss18 
offset 32: store.f64 notrap aligned v113, v112 
offset 37: store.f64 notrap aligned v114, v112+16
offset 43: store.f64 notrap aligned v115, v112+32

Since this function uses callee-save floats, we miscompile it on Windows today. Should I disable this test on Windows? (not sure if there's a mechanism for this in spectests, honestly) It would be fixed in the same PR that adjusts stack layout to not be constrained to 240 byte frames when using callee-save xmm registers.

Edit: this panic is only reached if you want a fastcall function with all of:

a >240 byte stack frame
callee-save floats
unwind information

This PR fixes callee-save float preservation in fastcall functions generally. Asking for unwind information causes the panic. I'm not sure how easily Windows targets can get the former without the latter.

iximeow · 2020-03-14T00:08:08Z

Nudging this again as I think this is as good as this fix can get without getting into the weeds on stack layout details.

I have three(ish) issues to open assuming a 👍 :

Use of an F64 type instead of the more accurate F64x2 for callee-save FPRs, which works but just isn't fully expressive of intent. See here
Fix how Cranelift lays out Fastcall stacks to avoid the 240-byte stack frame limit when using unwind information and callee-save XMM registers. See here for conversation and here for a test I've had to disable as a consequence.
Might not be worth a tracking issue, but there are two TODO's contingent on an encoding existing that would result in changes here after this lands.

github-actions · 2020-03-17T10:52:05Z

Subscribe to Label Action

This issue or pull request has been labeled: "cranelift"

Users Subscribed to "cranelift"

@bnjbvr

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

iximeow · 2020-03-26T21:03:11Z

Pardon the rebase, just want to get this on a more recent master so as to pick up a fix for nightly builds failing in CI.

bjorn3 · 2020-03-27T10:17:09Z

@iximeow I think you forgot to push :)

iximeow · 2020-03-27T17:58:54Z

@bjorn3 even worse, I'd pushed to the wrong remote. Thanks for the heads up!

peterhuene · 2020-03-31T02:14:15Z

It looks like there's at least a few more tests to disable due to the frame size limitation this adds to functions using Windows x64 calling convention.

iximeow · 2020-04-03T17:50:28Z

it passes 🎉

GitHub appears to believe that 3be2d1c doesn't exist, so I pushed an empty commit (ea9f77a) to fool CI into running after yesterday's availability issues. After ignoring more tests, it's green again 🥴

peterhuene · 2020-04-03T21:56:00Z

@iximeow I'll review this soon and if everyone signs off, I think we should merge this before my unwind refactoring changes. I can easily update that PR to integrate this PR's changes to save the FPRs in the windows unwind info.

peterhuene

This looks good to me 👍, even with the introduced stack size limit on Windows.

Just a few comments that aren't particularly blocking.

We probably want @bjorn3 or another core cranelift maintainer to sign off as well.

peterhuene · 2020-04-06T17:51:26Z

build.rs

+            | ("multi_value", "func") => {
+                // FIXME These involves functions with very large stack frames that Cranelift currently
+                // cannot compile using the fastcall (Windows) calling convention.
+                // See https://github.com/bytecodealliance/wasmtime/pull/1216.


Can we create a new issue to track the stack layout for CSRs problem and link here instead of the PR? My concern is only that it's hard to make out what's relevant of a complex PR like this one.

Absolutely. That's the third item in my to-do list which I've been waiting for even a preliminary ✔️ to actually file.

Just a reminder to update this to https://github.com/bytecodealliance/wasmtime/issues/1475.

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

bjorn3 · 2020-04-06T18:47:59Z

We probably want @bjorn3 or another core cranelift maintainer to sign off as well.

I am actually not a maintainer. :)

peterhuene · 2020-04-06T19:07:44Z

😱 my apologies, although your review is obviously always appreciated :) I'm not actually a core cranelift maintainer either, so I'm more comfortable when others sign off on bits I haven't been directly involved in.

bjorn3 · 2020-04-06T19:20:05Z

😱 my apologies, although your review is obviously always appreciated :)

You are not the first to think I am a maintainer of a project. :) I often review PR's on projects I am (slightly) interested in. And sometimes the PR author assumes I am a maintainer.

peterhuene · 2020-04-06T19:56:09Z

I'll ping @bnjbvr for the core maintainer sign-off, then.

another temporary commit. Cranelift::spec::multi_value::func panics but this is not reproducible using clif-util targeting windows?

This reverts commit 29c6c40.

ExplicitSlot is noted as intended for use with stack_load and stack_store, which are morally equivalent to FPR save/restore

…nUnwindInfo::new

iximeow · 2020-04-10T16:50:23Z

I had to force push to rebase and fix a merge conflict that crept in. Unwind-producing functions are Result-y, which of course resulted in some Wasmtime changes as well. Given the wider scope, I'd like one last +1 before clicking the tempting green button :)

cranelift/codegen/src/isa/mod.rs

Co-Authored-By: bjorn3 <bjorn3@users.noreply.github.com>

peterhuene

👍 on the result return change.

peterhuene · 2020-04-10T23:14:52Z

🎉 thanks for fixing this @iximeow! 🎉 I realize it was a slog of a PR.

Now I get to merge these changes in to my refactoring PR and we'll see about prioritizing the fixing of the stack layout of the register saves 😄

hrydgard · 2020-04-15T07:26:41Z

Awesome, thanks for getting this done!

This reverts commit 4cca510.

peterhuene reviewed Mar 3, 2020

View reviewed changes

cranelift/filetests/filetests/isa/x86/windows_fastcall_x64_unwind.clif Show resolved Hide resolved

iximeow commented Mar 3, 2020

View reviewed changes

cranelift/codegen/src/isa/x86/abi.rs Outdated Show resolved Hide resolved

iximeow commented Mar 3, 2020

View reviewed changes

cranelift/codegen/src/isa/x86/abi.rs Show resolved Hide resolved

hrydgard mentioned this pull request Mar 4, 2020

Heads up about serious bug in Cranelift (Windows) wasmerio/wasmer#1179

Closed

arkpar pushed a commit to paritytech/wasmtime that referenced this pull request Mar 4, 2020

Temporarily disable fuzzing until bytecodealliance#1216 is resolved (b…

37c7099

…ytecodealliance#1224)

arkpar pushed a commit to paritytech/wasmtime that referenced this pull request Mar 4, 2020

Revert "Temporarily disable fuzzing until bytecodealliance#1216 is re…

c5d6805

…solved (bytecodealliance#1224)" This reverts commit 589fa95.

iximeow force-pushed the windows-fprs-preservation branch from fbd067a to b55a43d Compare March 10, 2020 18:53

peterhuene reviewed Mar 10, 2020

View reviewed changes

cranelift/filetests/filetests/isa/x86/windows_fastcall_x64_unwind.clif Show resolved Hide resolved

bjorn3 reviewed Mar 10, 2020

View reviewed changes

cranelift/codegen/src/isa/x86/unwind.rs Show resolved Hide resolved

Hoverbear mentioned this pull request Mar 11, 2020

feat(new transform): Introduce WASM Plugins vectordotdev/vector#2006

Merged

6 tasks

github-actions bot added the cranelift Issues related to the Cranelift code generator label Mar 11, 2020

bytecodealliance deleted a comment from github-actions bot Mar 12, 2020

iximeow force-pushed the windows-fprs-preservation branch from 5c0e249 to e71b7fe Compare March 27, 2020 17:56

peterhuene approved these changes Apr 6, 2020

View reviewed changes

peterhuene mentioned this pull request Apr 6, 2020

Refactor unwind generation in Cranelift. #1466

Merged

iximeow added 18 commits April 9, 2020 15:28

only check for aligned frame allocation when we need it to be aligned

eeea0dd

print the whole prologue that caused issues in unwind

74d4ab8

another temporary commit. Cranelift::spec::multi_value::func panics but this is not reproducible using clif-util targeting windows?

add a comment explaining the seemingly-trunctaed test

f0f2394

Revert "ci speak to me of windows woes"

7bcff83

This reverts commit 29c6c40.

preserve FPRs in an ExplicitSlot rather than SpillSlot

56e45cf

ExplicitSlot is noted as intended for use with stack_load and stack_store, which are morally equivalent to FPR save/restore

remove helper function that only existed due to a bad rebase

84da124

temporarily disable the panic-reaching multi-return spectest

ceb48bf

rust ... format ...

291d09b

ignore more windows-unfriendly tests?

0a418a2

try different names for the testsuites?

a7e23a9

rustfmt

52425ef

update panic message with reference to implementation limit

3060c8c

review feedback

42d129b

fix date

c7547ed

try making unwind info fallible

c3530e6

pass errors in generating unwind info back up through CompiledFunctio…

f403c0a

…nUnwindInfo::new

forgot to rustfmt

750507a

make emit_fde Result-y as well

4e1b10e

iximeow force-pushed the windows-fprs-preservation branch from 87b7017 to 4e1b10e Compare April 9, 2020 23:12

bjorn3 reviewed Apr 10, 2020

View reviewed changes

cranelift/codegen/src/isa/mod.rs Outdated Show resolved Hide resolved

Update cranelift/codegen/src/isa/mod.rs

89af7e1

Co-Authored-By: bjorn3 <bjorn3@users.noreply.github.com>

peterhuene approved these changes Apr 10, 2020

View reviewed changes

iximeow merged commit 4cca510 into bytecodealliance:master Apr 10, 2020

iximeow deleted the windows-fprs-preservation branch April 10, 2020 20:28

iximeow mentioned this pull request Apr 13, 2020

Use F64X2 as type when saving and restoring XMM registers #1499

Merged

alexcrichton mentioned this pull request Apr 23, 2020

Rust doesn't work by default on Windows #1587

Closed

CryZe added a commit to CryZe/wasmtime that referenced this pull request Apr 25, 2020

Revert "Windows FPRs preservation (bytecodealliance#1216)"

a1d9030

This reverts commit 4cca510.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Windows fprs preservation #1216

Windows fprs preservation #1216

iximeow commented Mar 3, 2020 •

edited

Loading

iximeow Mar 3, 2020

iximeow commented Mar 11, 2020 •

edited

Loading

iximeow commented Mar 14, 2020 •

edited

Loading

github-actions bot commented Mar 17, 2020

iximeow commented Mar 26, 2020

bjorn3 commented Mar 27, 2020

iximeow commented Mar 27, 2020

peterhuene commented Mar 31, 2020

iximeow commented Apr 3, 2020

peterhuene commented Apr 3, 2020

peterhuene left a comment

peterhuene Apr 6, 2020

iximeow Apr 6, 2020

peterhuene Apr 8, 2020

bjorn3 commented Apr 6, 2020

peterhuene commented Apr 6, 2020

bjorn3 commented Apr 6, 2020

peterhuene commented Apr 6, 2020

iximeow commented Apr 10, 2020

peterhuene left a comment

peterhuene commented Apr 10, 2020

hrydgard commented Apr 15, 2020

Windows fprs preservation #1216

Windows fprs preservation #1216

Conversation

iximeow commented Mar 3, 2020 • edited Loading

iximeow Mar 3, 2020

Choose a reason for hiding this comment

iximeow commented Mar 11, 2020 • edited Loading

iximeow commented Mar 14, 2020 • edited Loading

github-actions bot commented Mar 17, 2020

Subscribe to Label Action

iximeow commented Mar 26, 2020

bjorn3 commented Mar 27, 2020

iximeow commented Mar 27, 2020

peterhuene commented Mar 31, 2020

iximeow commented Apr 3, 2020

peterhuene commented Apr 3, 2020

peterhuene left a comment

Choose a reason for hiding this comment

peterhuene Apr 6, 2020

Choose a reason for hiding this comment

iximeow Apr 6, 2020

Choose a reason for hiding this comment

peterhuene Apr 8, 2020

Choose a reason for hiding this comment

bjorn3 commented Apr 6, 2020

peterhuene commented Apr 6, 2020

bjorn3 commented Apr 6, 2020

peterhuene commented Apr 6, 2020

iximeow commented Apr 10, 2020

peterhuene left a comment

Choose a reason for hiding this comment

peterhuene commented Apr 10, 2020

hrydgard commented Apr 15, 2020

iximeow commented Mar 3, 2020 •

edited

Loading

iximeow commented Mar 11, 2020 •

edited

Loading

iximeow commented Mar 14, 2020 •

edited

Loading