Unwind: separate the "set frame reg" and "define new frame" operations to fix metadata bug related to stack checks.

In #3859 we discovered that the unwind metadata info on Windows is
placed slightly incorrectly in the presence of explicit stack checks in
function prologues. In particular, if the stack check fails, then we
start the process of backtracing from a state where we have actually
updated rbp (the frame pointer) but we have not emitted the metadata
saying we have done so.

The fix is to emit the `SetFPReg` Win32 unwind op exactly at the offset
of the `mov rbp, rsp` instruction, not after the stack check. However,
actually achieving this output given our current unwind design (which
abstracts over SysV and Win32) is slightly tricky, because:

- We previously had a single `DefineNewFrame` op that both indicated
  where the FP was set, and gave the size of the clobbers;
- We don't know the size of the clobbers until we generate the
  clobber-save sequence; and
- The clobber-save sequence is generated after any explicit stack check.

To resolve the problem, this PR splits the `DefineNewFrame` op into two
pieces: `SetFrameReg` and `DefineNewFrame`. On x86-64 and aarch64 this
is emitted in the appropriate place. (`s390x` does not use a frame
register, so has no need for this op.) This should resolve the issue we
were seeing with Windows-2022 and unwinding.

(Thanks to @alexcrichton, @iximeow and @peterhuene for help debugging
this!)

Author: cfallin

.github/workflows/main.yml

@@ -216,7 +216,8 @@ jobs:
           - os: ubuntu-latest
           - os: macos-latest
           - os: windows-2019
-          - os: windows-2019
+          - os: windows-2022
+          - os: windows-2022
             target: x86_64-pc-windows-gnu
           - os: ubuntu-latest
             target: aarch64-unknown-linux-gnu

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

@@ -673,6 +673,13 @@ impl ABIMachineSpec for AArch64MachineDeps {
                 shift12: false,
             },
         });
+
+        if flags.unwind_info() {
+            insts.push(Inst::Unwind {
+                inst: UnwindInst::SetFrameReg,
+            });
+        }
+
         insts
     }
 
@@ -734,8 +741,8 @@ impl ABIMachineSpec for AArch64MachineDeps {
             // clobbers, just below the saved FP/LR pair.
             insts.push(Inst::Unwind {
                 inst: UnwindInst::DefineNewFrame {
-                    offset_downward_to_clobbers: clobber_size as u32,
                     offset_upward_to_caller_sp: 16, // FP, LR
+                    offset_downward_to_clobbers: clobber_size as u32,
                 },
             });
         }

cranelift/codegen/src/isa/unwind.rs

@@ -118,8 +118,8 @@ pub enum UnwindInfo {
 /// push rbp
 /// unwind UnwindInst::PushFrameRegs { offset_upward_to_caller_sp: 16 }
 /// mov rbp, rsp
-/// unwind UnwindInst::DefineNewFrame { offset_upward_to_caller_sp: 16,
-///                                     offset_downward_to_clobbers: 16 }
+/// unwind UnwindInst::SetFrameReg { offset_upward_to_caller_sp: 16 }
+/// unwind UnwindInst::DefineNewFrame { offset_downward_to_clobbers: 16 }
 /// sub rsp, 32
 /// mov [rsp+16], r12
 /// unwind UnwindInst::SaveReg { reg: R12, clobber_offset: 0 }
@@ -143,15 +143,20 @@ pub enum UnwindInst {
     },
     /// The frame-pointer register for this architecture has just been
     /// set to the current stack location. We wish to define a new
-    /// frame that is anchored on this new FP value. Offsets are provided
-    /// upward to the caller's stack frame and downward toward the clobber
-    /// area. We expect this pseudo-op to come after `PushFrameRegs`.
+    /// frame that is anchored on this new FP value. We expect this
+    /// pseudo-op to come after `PushFrameRegs`.
+    SetFrameReg,
+    /// We now know the size of clobbers, if any, and can define a
+    /// frame in which we will give their offsets. Offsets are
+    /// provided upward to the caller's stack frame and downward
+    /// toward the clobber area. We expect this pseudo op to come
+    /// after `SetFrameReg`.
     DefineNewFrame {
-        /// The offset from the current SP and FP value upward to the value of
-        /// SP at the callsite that invoked us.
+        /// The offset from the current SP and FP value upward to the
+        /// value of SP at the callsite that invoked us.
         offset_upward_to_caller_sp: u32,
-        /// The offset from the current SP and FP value downward to the start of
-        /// the clobber area.
+        /// The offset from the current SP and FP value downward to
+        /// the start of the clobber area.
         offset_downward_to_clobbers: u32,
     },
     /// The stack pointer was adjusted to allocate the stack.

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

@@ -202,18 +202,21 @@ pub(crate) fn create_unwind_info_from_insts<MR: RegisterMapper<regalloc::Reg>>(
                     ));
                 }
             }
-            &UnwindInst::DefineNewFrame {
-                offset_upward_to_caller_sp,
-                offset_downward_to_clobbers,
-            } => {
+            &UnwindInst::SetFrameReg => {
                 // Define CFA in terms of FP. Note that we assume it was already
                 // defined correctly in terms of the current SP, and FP has just
                 // been set to the current SP, so we do not need to change the
                 // offset, only the register.  (This is done only if the target
                 // defines a frame pointer register.)
-                if let Some(fp) = mr.fp() {
-                    instructions.push((instruction_offset, CallFrameInstruction::CfaRegister(fp)));
-                }
+                instructions.push((
+                    instruction_offset,
+                    CallFrameInstruction::CfaRegister(mr.fp().unwrap()),
+                ));
+            }
+            &UnwindInst::DefineNewFrame {
+                offset_upward_to_caller_sp,
+                offset_downward_to_clobbers,
+            } => {
                 // Record initial CFA offset.  This will be used with later
                 // StackAlloc calls if we do not have a frame pointer.
                 cfa_offset = offset_upward_to_caller_sp;

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

@@ -277,12 +277,14 @@ pub(crate) fn create_unwind_info_from_insts<MR: RegisterMapper<regalloc::Reg>>(
                     reg: UNWIND_RBP_REG,
                 });
             }
+            &UnwindInst::SetFrameReg => {
+                unwind_codes.push(UnwindCode::SetFPReg { instruction_offset });
+            }
             &UnwindInst::DefineNewFrame {
                 offset_downward_to_clobbers,
                 ..
             } => {
                 frame_register_offset = ensure_unwind_offset(offset_downward_to_clobbers)?;
-                unwind_codes.push(UnwindCode::SetFPReg { instruction_offset });
             }
             &UnwindInst::StackAlloc { size } => {
                 unwind_codes.push(UnwindCode::StackAlloc {

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

@@ -463,6 +463,13 @@ impl ABIMachineSpec for X64ABIMachineSpec {
         // `mov %rsp, %rbp`
         // RSP is now 0 % 16
         insts.push(Inst::mov_r_r(OperandSize::Size64, r_rsp, w_rbp));
+
+        if flags.unwind_info() {
+            insts.push(Inst::Unwind {
+                inst: UnwindInst::SetFrameReg,
+            });
+        }
+
         insts
     }
 
@@ -513,8 +520,8 @@ impl ABIMachineSpec for X64ABIMachineSpec {
             // part of our actual frame but do not concern the unwinder.
             insts.push(Inst::Unwind {
                 inst: UnwindInst::DefineNewFrame {
-                    offset_downward_to_clobbers: clobbered_size,
                     offset_upward_to_caller_sp: 16, // RBP, return address
+                    offset_downward_to_clobbers: clobbered_size,
                 },
             });
         }