Remove 'set frame pointer' unwind code from Windows x64 unwind.

[peterhuene]

This PR removes the "set frame pointer" unwind code and frame
pointer information from Windows x64 unwind information.

In Windows x64 unwind information, a "frame pointer" is actually the
base address of the static part of the local frame and would be at some
negative offset to RSP upon establishing the frame pointer.

Currently Cranelift uses a "traditional" notion of a frame pointer, one
that is the highest address in the local frame (i.e. pointing at the
previous frame pointer on the stack).

Windows x64 unwind doesn't describe such frame pointers and only needs
one described if the frame contains a dynamic stack allocation.

Fixes #1967.

[alexcrichton]

Nice find on this, looks like some gnarly debugging to get here! I'm not super familiar with Windows unwinding though so I'd defer to @iximeow's review as well.

[peterhuene]

@iximeow the gist of the problem is that a "frame pointer" in Win64 unwind information should point at the base of the static part of the local frame and not what we discussed earlier when you originally implemented the FPR saves for Windows; that being a "normal" frame pointer with the unwind information describing how to offset it to get that base address for unwinding only.

Windows actually expects the "frame pointer" to be offset from RSP when established and Cranelift doesn't do that. To eliminate the potential for confusion regarding what a "frame pointer" is, I've opted to simply remove encoding the "set frame pointer" unwind code in the unwind information for now. In the future, if we ever need to support dynamically sized frames, we can add it back in the right way (i.e. the frame pointer offset should be discovered via the establishing instruction and not based on the register saves).

[github-actions]

Subscribe to Label Action

cc @bnjbvr

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

Thus the following users have been cc'd because of the following labels:

• bnjbvr: cranelift

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

Learn more.

[iximeow]

🎉

I'm pretty sure I understand how this fits in with Windows x6_64 unwinding generally, but I want to repeat my understanding back to make sure we're on the same page: this change doesn't interact with saved general-purpose registers because they were never frame pointer-relative in the first place. Offsets for FPR-saves were correct all along, but by setting a frame pointer we instructed the Windows unwinder to treat them as offsets an incorrect location, where things go sideways?

So by not setting an explicit frame pointer, the Windows unwinder assumes the frame base is RSP at function entry, and everything is relative to that?

[peterhuene]

That's correct. Windows doesn't need a traditional frame pointer at all to figure out where the frame starts because every function that modifies the SP, in any way, must have unwind information that describes the adjustments. For frames that can't describe that adjustment statically (e.g. a call to alloca), there must be a register that points at what RSP would be post statically-known adjustment (i.e. lea rbp, [rsp-static_frame_size] or sub rsp, static_frame_size; mov rbp, rsp). This is what the Windows x64 unwind information considers a "frame pointer". The FPR saves are always a positive offset from this frame pointer if there is one.

We were calculating the "frame pointer" and describing it correctly in the unwind information, but Cranelift establishes the frame pointer as one would one expect: always using the start (highest address) of the local frame.

I thought that the Windows unwind information also expected this traditional notion of frame pointer and the encoded offset would be subtracted from the frame pointer during unwinding to determine the base address that FPR saves are relative to. Instead it was actually the opposite: the unwinder would add the offset to the frame pointer to find the top of the frame. This obviously caused bad stack walks during unwind, not to mention it restored the saved FPRs from the wrong addresses as well.

[iximeow]

I thought that the Windows unwind information also expected this traditional notion of frame pointer and the encoded offset would be subtracted from the frame pointer during unwinding to determine the base address that FPR saves are relative to.

Needless to say, I'd thought this too :) Thank you for figuring this out, and the test to guard against breaking this in the future.

[peterhuene]

I just noticed a subtle bug in how we're encoding SaveXmm128Far too: the offset should be an unscaled 32-bit value but we're accidentally scaling down by 16. It would require a stack allocation of 1+ MiB (coincidentally, the default stack size for Windows is 1 MiB) for a function to encounter, though.

I'll push up a fix.

[peterhuene]

@iximeow does the commit (b1c7c14) I just pushed up make sense?

[whitequark]

That was an impressively fast fix for such a complex issue! Thanks everyone.

[iximeow]

@iximeow does the commit (b1c7c14) I just pushed up make sense?

Yep! This all looks great.

[nevercast]

Pure speed! Thanks heaps for getting this in main so quick!