[X86] Fix tailcall return address clobber bug

[margnus1]

Summary

This bug (PR28124) was introduced by r237977, which refactored the tail call
sequence to be generated in two passes instead of one.

Unfortunately, the stack adjustment produced by the first pass was not
recognized by X86FrameLowering::mergeSPUpdates() in all cases, causing
code such as the following, which clobbers the return address, to be
generated:

popl    %edi
popl    %edi
pushl   %eax
jmp     tailcallee              # TAILCALL

By moving the stack adjustment by X86MachineFunctionInfo::getTCReturnAddrDelta() from X86FrameLowering::emitEpilogue() to X86ExpandPseudo::ExpandMI(), the bug is fixed.

[margnus1]

Hi! I wrote this patch under the assumption that you intended X86FrameLowering::mergeSPUpdates() to work this way in your refactoring. Feel free to say so if this isn't the best way of fixing the problem.

[qcolombet]

Hi Magnus,

The refactoring of findDeadCallerSavedReg looks good, though at this point don't know if it is necessary.
The merge SP update part seems like an optimization to me, and it feels wrong to have to resort on an optimization to generate correct code.

How was this working previously?
Or put different why is the merge SP update necessary to fix the issue?

Cheers,
-Quentin

lib/Target/X86/X86FrameLowering.cpp

200 ↗

(On Diff #60682)

remove else {
Per llvm coding style.

200 ↗

(On Diff #60682)

No need for brackets for one line block, per llvm coding style.

432 ↗

(On Diff #60682)

Add comments on what you are doing here.
For instance, something along the line of: this is fine to drop pop instructions when their results in not used by the return instruction, i.e., the values are not live out.

Explain why we need to make a special case for RIP. That is not clear, at least, to me. I would have expected the proper implicit uses to be set for that register.

test/CodeGen/X86/hipe-cc.ll

76 ↗

(On Diff #60682)

• Add a comment on what is this function testing.
• Use "opt -instnamer" on this IR to get rid of the [0-9]+ variables.

test/CodeGen/X86/hipe-cc64.ll

86 ↗

(On Diff #60682)

Ditto

[margnus1]

Hi Quentin!

This worked before by emitting both the adjustment by X86MachineFunctionInfo::getTCReturnAddrDelta() along with the stack adjustment in the tail call instruction in a single call to emitSPUpdate().

While writing this patch, I had initially assumed that it was not possible to do the stack adjustment in a single call to emitSPUpdate() without undoing the refactoring. I have now challenged that assumption and found it to be false, and since I agree that fixing the problem in mergeSPUpdates() is a poor solution, I have changed my patch so that the entire stack adjustment is performed in X86ExpandPseudo::ExpandMI().

[qcolombet]

lib/Target/X86/X86ExpandPseudo.cpp

257 ↗

(On Diff #62093)

I’d add X86FI as a member so that we do not have to pass it around.

lib/Target/X86/X86FrameLowering.cpp

1628 ↗

(On Diff #62093)

add a helper function isTailCallOpcode or something.

[margnus1]

Updated as per your instructions.

[qcolombet]

Thanks Magnus.

LGTM.

[margnus1]

Thanks for the review. I would appriciate help to commit this as I lack commit access.

[qcolombet]

Hi Magnus,

Sure!

Could you rebase the patch?

It does not apply cleaning for me.

Thanks,
-Quentin

[margnus1]

Of course. I should have done so earlier, I apologize.

llvm/trunk/lib/Target/X86/X86ExpandPseudo.cpp

void getAnalysisUsage(AnalysisUsage &AU) const override {
     AU.addPreservedID(MachineLoopInfoID);
     AU.addPreservedID(MachineDominatorsID);
     MachineFunctionPass::getAnalysisUsage(AU);
   }
 
   const X86Subtarget *STI;
   const X86InstrInfo *TII;
   const X86RegisterInfo *TRI;
+  const X86MachineFunctionInfo *X86FI;
   const X86FrameLowering *X86FL;
 
   bool runOnMachineFunction(MachineFunction &Fn) override;
 
   MachineFunctionProperties getRequiredProperties() const override {
     return MachineFunctionProperties().set(
         MachineFunctionProperties::Property::AllVRegsAllocated);
   }
bool X86ExpandPseudo::ExpandMI(MachineBasicBlock &MBB,
   case X86::TCRETURNmi64: {
     bool isMem = Opcode == X86::TCRETURNmi || Opcode == X86::TCRETURNmi64;
     MachineOperand &JumpTarget = MBBI->getOperand(0);
     MachineOperand &StackAdjust = MBBI->getOperand(isMem ? 5 : 1);
     assert(StackAdjust.isImm() && "Expecting immediate value.");
 
     // Adjust stack pointer.
     int StackAdj = StackAdjust.getImm();
+    int MaxTCDelta = X86FI->getTCReturnAddrDelta();
+    int Offset = 0;
+    assert(MaxTCDelta <= 0 && "MaxTCDelta should never be positive");
+
+    // Incoporate the retaddr area.
+    Offset = StackAdj-MaxTCDelta;
+    assert(Offset >= 0 && "Offset should never be negative");
 
-    if (StackAdj) {
+    if (Offset) {
       // Check for possible merge with preceding ADD instruction.
-      StackAdj += X86FL->mergeSPUpdates(MBB, MBBI, true);
-      X86FL->emitSPUpdate(MBB, MBBI, StackAdj, /*InEpilogue=*/true);
+      Offset += X86FL->mergeSPUpdates(MBB, MBBI, true);
+      X86FL->emitSPUpdate(MBB, MBBI, Offset, /*InEpilogue=*/true);
     }
 
     // Jump to label or value in register.
     bool IsWin64 = STI->isTargetWin64();
     if (Opcode == X86::TCRETURNdi || Opcode == X86::TCRETURNdi64) {
       unsigned Op = (Opcode == X86::TCRETURNdi)
                         ? X86::TAILJMPd
                         : (IsWin64 ? X86::TAILJMPd64_REX : X86::TAILJMPd64);
bool X86ExpandPseudo::ExpandMBB(MachineBasicBlock &MBB) {
 
   return Modified;
 }
 
 bool X86ExpandPseudo::runOnMachineFunction(MachineFunction &MF) {
   STI = &static_cast<const X86Subtarget &>(MF.getSubtarget());
   TII = STI->getInstrInfo();
   TRI = STI->getRegisterInfo();
+  X86FI = MF.getInfo<X86MachineFunctionInfo>();
   X86FL = STI->getFrameLowering();
 
   bool Modified = false;
   for (MachineBasicBlock &MBB : MF)
     Modified |= ExpandMBB(MBB);
   return Modified;
 }
 
 /// Returns an instance of the pseudo instruction expansion pass.
 FunctionPass *llvm::createX86ExpandPseudoPass() {
   return new X86ExpandPseudo();
 }

llvm/trunk/lib/Target/X86/X86FrameLowering.cpp

X86FrameLowering::getWinEHFuncletFrameSize(const MachineFunction &MF) const {
   // everything is 16 byte aligned. Everything we allocate before an outgoing
   // call must also be 16 byte aligned.
   unsigned FrameSizeMinusRBP = alignTo(CSSize + UsedSize, getStackAlignment());
   // Subtract out the size of the callee saved registers. This is how much stack
   // each funclet will allocate.
   return FrameSizeMinusRBP - CSSize;
 }
 
+static bool isTailCallOpcode(unsigned Opc) {
+    return Opc == X86::TCRETURNri || Opc == X86::TCRETURNdi ||
+        Opc == X86::TCRETURNmi ||
+        Opc == X86::TCRETURNri64 || Opc == X86::TCRETURNdi64 ||
+        Opc == X86::TCRETURNmi64;
+}
+
 void X86FrameLowering::emitEpilogue(MachineFunction &MF,
                                     MachineBasicBlock &MBB) const {
   const MachineFrameInfo *MFI = MF.getFrameInfo();
   X86MachineFunctionInfo *X86FI = MF.getInfo<X86MachineFunctionInfo>();
   MachineBasicBlock::iterator MBBI = MBB.getFirstTerminator();
+  unsigned RetOpcode = MBBI->getOpcode();
   DebugLoc DL;
   if (MBBI != MBB.end())
     DL = MBBI->getDebugLoc();
   // standard x86_64 and NaCl use 64-bit frame/stack pointers, x32 - 32-bit.
   const bool Is64BitILP32 = STI.isTarget64BitILP32();
   unsigned FramePtr = TRI->getFrameRegister(MF);
   unsigned MachineFramePtr =
       Is64BitILP32 ? getX86SubSuperRegister(FramePtr, 64) : FramePtr;
void X86FrameLowering::emitEpilogue(MachineFunction &MF,
   // either in prologue or in epilogue.  This behavior causes a problem when a
   // call immediately precedes an epilogue, because the return address points
   // into the epilogue.  To cope with that, we insert an epilogue marker here,
   // then replace it with a 'nop' if it ends up immediately after a CALL in the
   // final emitted code.
   if (NeedsWinCFI)
     BuildMI(MBB, MBBI, DL, TII.get(X86::SEH_Epilogue));
 
+  if (!isTailCallOpcode(RetOpcode)) {
     // Add the return addr area delta back since we are not tail calling.
     int Offset = -1 * X86FI->getTCReturnAddrDelta();
     assert(Offset >= 0 && "TCDelta should never be positive");
     if (Offset) {
       MBBI = MBB.getFirstTerminator();
 
       // Check for possible merge with preceding ADD instruction.
       Offset += mergeSPUpdates(MBB, MBBI, true);
       emitSPUpdate(MBB, MBBI, Offset, /*InEpilogue=*/true);
     }
   }
+}
 
 // NOTE: this only has a subset of the full frame index logic. In
 // particular, the FI < 0 and AfterFPPop logic is handled in
 // X86RegisterInfo::eliminateFrameIndex, but not here. Possibly
 // (probably?) it should be moved into here.
 int X86FrameLowering::getFrameIndexReference(const MachineFunction &MF, int FI,
                                              unsigned &FrameReg) const {
   const MachineFrameInfo *MFI = MF.getFrameInfo();

llvm/trunk/test/CodeGen/X86/hipe-cc.ll

define cc 11 void @baz() nounwind {
   %tmp_clos2 = inttoptr i32 %tmp_clos to i32*
   %indirect_call = bitcast i32* %tmp_clos2 to void (i32, i32, i32)*
   ; CHECK:      movl $42, %eax
   ; CHECK-NEXT: jmpl *clos
   tail call cc 11 void %indirect_call(i32 undef, i32 undef, i32 42) nounwind
   ret void
 }
 
+; Sanity-check the tail call sequence. Number of arguments was chosen as to
+; expose a bug where the tail call sequence clobbered the stack.
+define cc 11 { i32, i32, i32 } @tailcaller(i32 %hp, i32 %p) nounwind {
+  ; CHECK:      movl  $15, %eax
+  ; CHECK-NEXT: movl  $31, %edx
+  ; CHECK-NEXT: movl  $47, %ecx
+  ; CHECK-NEXT: popl  %edi
+  ; CHECK-NEXT: jmp tailcallee
+  %ret = tail call cc11 { i32, i32, i32 } @tailcallee(i32 %hp, i32 %p, i32 15,
+     i32 31, i32 47, i32 63) nounwind
+  ret { i32, i32, i32 } %ret
+}
+
 !hipe.literals = !{ !0, !1, !2 }
 !0 = !{ !"P_NSP_LIMIT", i32 84 }
 !1 = !{ !"X86_LEAF_WORDS", i32 24 }
 !2 = !{ !"AMD64_LEAF_WORDS", i32 24 }
 @clos = external constant i32
 declare cc 11 void @bar(i32, i32, i32, i32, i32)
+declare cc 11 { i32, i32, i32 } @tailcallee(i32, i32, i32, i32, i32, i32)

llvm/trunk/test/CodeGen/X86/hipe-cc64.ll

define cc 11 void @baz() nounwind {
   %tmp_clos2 = inttoptr i64 %tmp_clos to i64*
   %indirect_call = bitcast i64* %tmp_clos2 to void (i64, i64, i64)*
   ; CHECK:      movl $42, %esi
   ; CHECK-NEXT: jmpq *(%rax)
   tail call cc 11 void %indirect_call(i64 undef, i64 undef, i64 42) nounwind
   ret void
 }
 
+; Sanity-check the tail call sequence. Number of arguments was chosen as to
+; expose a bug where the tail call sequence clobbered the stack.
+define cc 11 { i64, i64, i64 } @tailcaller(i64 %hp, i64 %p) #0 {
+  ; CHECK:      movl  $15, %esi
+  ; CHECK-NEXT: movl  $31, %edx
+  ; CHECK-NEXT: movl  $47, %ecx
+  ; CHECK-NEXT: movl  $63, %r8d
+  ; CHECK-NEXT: popq  %rax
+  ; CHECK-NEXT: jmp tailcallee
+  %ret = tail call cc11 { i64, i64, i64 } @tailcallee(i64 %hp, i64 %p, i64 15,
+     i64 31, i64 47, i64 63, i64 79) #1
+  ret { i64, i64, i64 } %ret
+}
+
 !hipe.literals = !{ !0, !1, !2 }
 !0 = !{ !"P_NSP_LIMIT", i32 160 }
 !1 = !{ !"X86_LEAF_WORDS", i32 24 }
 !2 = !{ !"AMD64_LEAF_WORDS", i32 24 }
 @clos = external constant i64
 declare cc 11 void @bar(i64, i64, i64, i64, i64, i64)
+declare cc 11 { i64, i64, i64 } @tailcallee(i64, i64, i64, i64, i64, i64, i64)