[X86] Emit correct unwind info for split-stack prologue

[cherry]

Summary

When -fsplit-stack is used, the function prologue contains two additional blocks: the first checks the stack bound, and the second calls __morestack. The second block may be laid out later in the function body, as it is an unlikely block. However, it is still the prologue, where we haven't pushed callee-save registers and the frame pointer (if used) yet. Currently we didn't take this into account when generating the unwind info. The CFIInstrInserter pass does take care of the return address correctly, but it does not handle the frame pointer and callee-save registers.

This change fixes this by generating .cfi_restore for saved registers to reset them as of the function entry. We also generate .cfi_remember_state and .cfi_restore_state before and after this block, so that the blocks before and after it are not affected no matter where this block is laid out. As it is specific to split-stack, we do it in X86FrameLowering, instead of teaching CFIInstrInserter about all the saved register stuff (which is generally not needed).

Added basic support of .cfi_remember_state and .cfi_restore_state support to CFIInstrInserter and AsmPrinter.

[thanm]

LGTM. I did some hand testing with the patch and the results seem fine.

[wmi]

I suspect there is some problem caused by interaction of .cfi_remember_state/.cfi_restore_state and CFIInstrInserter pass.

Considering the allocMBB is in the middle of the function. The CFA register is rbp instead of rsp in the prev MBB of allocMBB. At the beginning of allocMBB, CFIInstrInserter will insert a def_cfa_register to change the CFA register to rsp, which matches the state of function entry. The def_cfa_register will be inserted at the beginning of allocMBB and will be remembered by cfi_remember_state. Then .cfi_restore_state will restore the beginning of next MBB of allocMBB to have rsp instead of rbp as the CFA register, which is wrong.

If the cfi adjustment inserted by CFIInstrInserter pass is placed after .cfi_remember_state, it matches what we want here.

lib/CodeGen/CFIInstrInserter.cpp

192–196

I feel it is not the correct meaning of .cfi_remember_state. .cfi_remember_state means remembering the current set of CFI rules so it is related with layout instead of function control flow. Here calculateOutgoingCFAInfo is based on function control flow.

For allocMBB, the .cfi_remember_state remembers the state its prev MBB, not the state of its predecessor MBB, which is the function entry.

I think we should ignore .cfi_remember_state and .cfi_restore_state inside of calculateOutgoingCFAInfo because they are not directives associated with CFI changing machine instructions.

[wmi]

lib/CodeGen/CFIInstrInserter.cpp

192–196

And handle .cfi_remember_state and .cfi_restore_state inside of insertCFIInstrs, when the outgoing cfi information of prev BB (not predecessor BB) is available.

[cherry]

In D66013#1627633, @wmi wrote:

I suspect there is some problem caused by interaction of .cfi_remember_state/.cfi_restore_state and CFIInstrInserter pass.

Considering the allocMBB is in the middle of the function. The CFA register is rbp instead of rsp in the prev MBB of allocMBB. At the beginning of allocMBB, CFIInstrInserter will insert a def_cfa_register to change the CFA register to rsp, which matches the state of function entry. The def_cfa_register will be inserted at the beginning of allocMBB and will be remembered by cfi_remember_state. Then .cfi_restore_state will restore the beginning of next MBB of allocMBB to have rsp instead of rbp as the CFA register, which is wrong.

Thanks for the review. I think the idea is that CFIInstrInserter will make the CFIs in the layout order match the logical control flow. In your example, we start with prevMBB, allocMBB, nextMBB in layout order. You're right that it will insert def_cfa_register at the beginning of allocMBB to change the CFA to rsp, and cfi_remember_state/cfi_restore_state will remember/restore the CFA as rsp. This is indeed what calculateOutgoingCFAInfo sees, so it will record that the outgoing CFA of allocMBB is rsp (just as before without this change). So this pass will insert def_cfa_register to rbp at the beginning of nextMBB (as before).

lib/CodeGen/CFIInstrInserter.cpp

192–196

Yes, cfi_remember_state remembers the state its prev MBB, which is exactly what we want. It is mostly for remembering the state of saved registers, not the CFA. This pass will take care of CFA.

Suppose we have prevMBB, allocMBB, nextMBB. Both prevMBB and nextMBB are normal function body, which have CFA register rbp, and callee-save registers already pushed on stack. After this pass we will have

prevMBB:

...

allocMBB:

.cfi_def_cfa rsp
.cfi_remember_state
.cfi_restore callee-save
...
.cfi_restore_state

nextMBB:

.cfi_def_cfa rbp
...

cfi_def_cfa inserted by this pass will make the CFA correct (cfi_remember_state/cfi_restore_state does not affect it), and the callee-save registers are set correctly by cfi_restore and cfi_remember_state/cfi_restore_state.

I understand that this is a little hard to explain. Let me know if I could make this more clear. Thanks.

I think we should ignore .cfi_remember_state and .cfi_restore_state inside of calculateOutgoingCFAInfo because they are not directives associated with CFI changing machine instructions.

This could be an option, as long as cfi_remember_state/cfi_restore_state are paired and no CFA-changing instructions in between. And it works for the particular case of allocMBB. I could make this change if you prefer.

[wmi]

I see. You already take the adjustment in insertCFIInstrs into consideration and assume the CFIs in the layout order will match CFIs in control flow in the end. In this way, .cfi_remember_state at the beginning of current BB is always assumed to remember the outgoing CFI information of its predecessor BB.

So the CFI sequence is correct but may not be optimal -- if .cfi_remember_state is inserted before the adjustment from insertCFIInstrs, nextMBB doesn't need the .cfi_def_cfa right?

[thanm]

FYI, I filed an LLVM bug to track this issue (has a testcase and dumps showing incorrect unwind, etc). Bug is

https://bugs.llvm.org/show_bug.cgi?id=43005

lib/CodeGen/AsmPrinter/AsmPrinterDwarf.cpp

case MCCFIInstruction::OpGnuArgsSize:
     OutStreamer->EmitCFIGnuArgsSize(Inst.getOffset());
     break;
   case MCCFIInstruction::OpEscape:
     OutStreamer->EmitCFIEscape(Inst.getValues());
     break;
   case MCCFIInstruction::OpRestore:
     OutStreamer->EmitCFIRestore(Inst.getRegister());
     break;
+  case MCCFIInstruction::OpRememberState:
+    OutStreamer->EmitCFIRememberState();
+    break;
+  case MCCFIInstruction::OpRestoreState:
+    OutStreamer->EmitCFIRestoreState();
+    break;
   }
 }
 
 void AsmPrinter::emitDwarfDIE(const DIE &Die) const {
   // Emit the code (index) for the abbreviation.
   if (isVerbose())
     OutStreamer->AddComment("Abbrev [" + Twine(Die.getAbbrevNumber()) + "] 0x" +
                             Twine::utohexstr(Die.getOffset()) + ":0x" +

lib/CodeGen/CFIInstrInserter.cpp

 void CFIInstrInserter::calculateOutgoingCFAInfo(MBBCFAInfo &MBBInfo) {
   // Outgoing cfa offset set by the block.
   int SetOffset = MBBInfo.IncomingCFAOffset;
   // Outgoing cfa register set by the block.
   unsigned SetRegister = MBBInfo.IncomingCFARegister;
   const std::vector<MCCFIInstruction> &Instrs =
       MBBInfo.MBB->getParent()->getFrameInstructions();
 
+  // Remembered cfa register and offset, for cfi_remember_state
+  // and cfi_restore_state. Currently we only support at most
+  // one pair of cfi_remember_state and cfi_restore_state in a
+  // block.
+  unsigned RememberedReg = ~0u;
+  int RememberedOffset = 0;
+
   // Determine cfa offset and register set by the block.
   for (MachineInstr &MI : *MBBInfo.MBB) {
     if (MI.isCFIInstruction()) {
       unsigned CFIIndex = MI.getOperand(0).getCFIIndex();
       const MCCFIInstruction &CFI = Instrs[CFIIndex];
       switch (CFI.getOperation()) {
       case MCCFIInstruction::OpDefCfaRegister:
         SetRegister = CFI.getRegister();
         break;
       case MCCFIInstruction::OpDefCfaOffset:
         SetOffset = CFI.getOffset();
         break;
       case MCCFIInstruction::OpAdjustCfaOffset:
         SetOffset += CFI.getOffset();
         break;
       case MCCFIInstruction::OpDefCfa:
         SetRegister = CFI.getRegister();
         SetOffset = CFI.getOffset();
         break;
       case MCCFIInstruction::OpRememberState:
-        // TODO: Add support for handling cfi_remember_state.
+        if (RememberedReg == ~0u) {
+          RememberedReg = SetRegister;
+          RememberedOffset = SetOffset;
+          break;
+        }
 #ifndef NDEBUG
         report_fatal_error(
-            "Support for cfi_remember_state not implemented! Value of CFA "
-            "may be incorrect!\n");
+            "Support for multiple cfi_remember_state not implemented! "
+            "Value of CFA may be incorrect!\n");
 #endif
         break;
       case MCCFIInstruction::OpRestoreState:
-        // TODO: Add support for handling cfi_restore_state.
+        if (RememberedReg != ~0u) {
+          SetRegister = RememberedReg;
+          SetOffset = RememberedOffset;
+          break;
+        }
 #ifndef NDEBUG
         report_fatal_error(
-            "Support for cfi_restore_state not implemented! Value of CFA may "
-            "be incorrect!\n");
+            "Support for cfi_restore_state without cfi_remember_state not "
+            "implemented! Value of CFA may be incorrect!\n");
 #endif
         break;
       // Other CFI directives do not affect CFA value.
       case MCCFIInstruction::OpSameValue:
       case MCCFIInstruction::OpOffset:
       case MCCFIInstruction::OpRelOffset:
       case MCCFIInstruction::OpEscape:
       case MCCFIInstruction::OpRestore:

lib/Target/X86/X86FrameLowering.cpp

else
         .addExternalSymbol("__morestack");
   }
 
   if (IsNested)
     BuildMI(allocMBB, DL, TII.get(X86::MORESTACK_RET_RESTORE_R10));
   else
     BuildMI(allocMBB, DL, TII.get(X86::MORESTACK_RET));
 
+  // allocMBB is still in the function prologue, and we haven't pushed
+  // callee-save registers and the frame pointer (if used) yet. But it
+  // may be laid out in the middle or the end of the function. We need
+  // to restore the unwind info for pushed registers.
+  // We use cfi_remember_state and cfi_restore_state to ensure blocks
+  // laid out before and after it are not affected.
+  const Function &Fn = MF.getFunction();
+  MachineModuleInfo &MMI = MF.getMMI();
+  bool IsWin64Prologue = MF.getTarget().getMCAsmInfo()->usesWindowsCFI();
+  bool NeedsDwarfCFI =
+      !IsWin64Prologue && (MMI.hasDebugInfo() || Fn.needsUnwindTableEntry());
+  if (NeedsDwarfCFI) {
+    auto MBBI = allocMBB->begin();
+    BuildCFI(*allocMBB, MBBI, DL,
+             MCCFIInstruction::createRememberState(nullptr));
+
+    // Restore the frame pointer
+    bool HasFP = hasFP(MF);
+    if (HasFP) {
+      unsigned FramePtr = TRI->getFrameRegister(MF);
+      const unsigned MachineFramePtr = STI.isTarget64BitILP32() ?
+          getX86SubSuperRegister(FramePtr, 64) : FramePtr;
+      unsigned DwarfFramePtr = TRI->getDwarfRegNum(MachineFramePtr, true);
+      BuildCFI(*allocMBB, MBBI, DL,
+               MCCFIInstruction::createRestore(nullptr, DwarfFramePtr));
+    }
+
+    // Restore callee-save registers.
+    const std::vector<CalleeSavedInfo> &CSI = MFI.getCalleeSavedInfo();
+    for (std::vector<CalleeSavedInfo>::const_iterator
+           I = CSI.begin(), E = CSI.end(); I != E; ++I) {
+      unsigned Reg = I->getReg();
+      unsigned DwarfReg = TRI->getDwarfRegNum(Reg, true);
+      BuildCFI(*allocMBB, MBBI, DL,
+               MCCFIInstruction::createRestore(nullptr, DwarfReg));
+    }
+
+    BuildCFI(*allocMBB, allocMBB->end(), DL,
+             MCCFIInstruction::createRestoreState(nullptr));
+  }
+
   allocMBB->addSuccessor(&PrologueMBB);
 
   checkMBB->addSuccessor(allocMBB, BranchProbability::getZero());
   checkMBB->addSuccessor(&PrologueMBB, BranchProbability::getOne());
 
 #ifdef EXPENSIVE_CHECKS
   MF.verify();
 #endif

test/CodeGen/X86/segmented-stacks.ll

 
 ; X32ABI-LABEL: test_nested_unused:
 ; X32ABI-NOT:   movl %r10d, %eax
 ; X32ABI:       callq __morestack
 ; X32ABI-NOT:   movq %rax, %r10
 
 }
 
+; Test that we have correct unwind info for the block
+; that calls morestack.
+
+define void @test_cfi(i32 %x) #1 {
+  %mem = alloca i32, i32 10
+  call void @dummy_use (i32* %mem, i32 %x)
+  ret void
+
+; X32-Linux-LABEL: test_cfi:
+; X32-Linux:       .cfi_remember_state
+; X32-Linux:       .cfi_restore %ebp
+; X32-Linux:       calll __morestack
+; X32-Linux:       retl
+; X32-Linux:       .cfi_restore_state
+
+; X64-Linux-LABEL: test_cfi:
+; X64-Linux:       .cfi_remember_state
+; X64-Linux:       .cfi_restore %rbp
+; X64-Linux:       callq __morestack
+; X64-Linux:       retq
+; X64-Linux:       .cfi_restore_state
+
+; X32ABI-Linux-LABEL: test_cfi:
+; X32ABI-Linux:       .cfi_remember_state
+; X32ABI-Linux:       .cfi_restore %rbp
+; X32ABI-Linux:       callq __morestack
+; X32ABI-Linux:       retq
+; X32ABI-Linux:       .cfi_restore_state
+
+; X32-Darwin-LABEL: test_cfi:
+; X32-Darwin:       .cfi_remember_state
+; X32-Darwin:       .cfi_restore %ebp
+; X32-Darwin:       calll ___morestack
+; X32-Darwin:       retl
+; X32-Darwin:       .cfi_restore_state
+
+; X64-Darwin-LABEL: test_cfi:
+; X64-Darwin:       .cfi_remember_state
+; X64-Darwin:       .cfi_restore %rbp
+; X64-Darwin:       callq ___morestack
+; X64-Darwin:       retq
+; X64-Darwin:       .cfi_restore_state
+
+; X64-FreeBSD-LABEL: test_cfi:
+; X64-FreeBSD:       .cfi_remember_state
+; X64-FreeBSD:       .cfi_restore %rbp
+; X64-FreeBSD:       callq __morestack
+; X64-FreeBSD:       retq
+; X64-FreeBSD:       .cfi_restore_state
+
+; X32-DFlyBSD-LABEL: test_cfi:
+; X32-DFlyBSD:       .cfi_remember_state
+; X32-DFlyBSD:       .cfi_restore %ebp
+; X32-DFlyBSD:       calll __morestack
+; X32-DFlyBSD:       retl
+; X32-DFlyBSD:       .cfi_restore_state
+
+; X64-DFlyBSD-LABEL: test_cfi:
+; X64-DFlyBSD:       .cfi_remember_state
+; X64-DFlyBSD:       .cfi_restore %rbp
+; X64-DFlyBSD:       callq __morestack
+; X64-DFlyBSD:       retq
+; X64-DFlyBSD:       .cfi_restore_state
+
+}
+
 attributes #0 = { "split-stack" }
+attributes #1 = { "split-stack" "no-frame-pointer-elim"="true" }
 
 ; X64-Linux-Large: .rodata
 ; X64-Linux-Large-NEXT: __morestack_addr:
 ; X64-Linux-Large-NEXT: .quad __morestack
 
 ; X32-Linux: .section ".note.GNU-split-stack","",@progbits
 ; X32-Linux: .section ".note.GNU-no-split-stack","",@progbits