[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

remove else {
Per llvm coding style.

200

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

432

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

• 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

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

1665

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.

lib/Target/X86/X86FrameLowering.cpp

if (isInt<8>(Imm))
     return X86::AND32ri8;
   return X86::AND32ri;
 }
 
 static unsigned getLEArOpcode(unsigned IsLP64) {
   return IsLP64 ? X86::LEA64r : X86::LEA32r;
 }
 
-/// findDeadCallerSavedReg - Return a caller-saved register that isn't live
-/// when it reaches the "return" instruction. We can then pop a stack object
-/// to this register without worry about clobbering it.
-static unsigned findDeadCallerSavedReg(MachineBasicBlock &MBB,
-                                       MachineBasicBlock::iterator &MBBI,
-                                       const X86RegisterInfo *TRI,
-                                       bool Is64Bit) {
-  const MachineFunction *MF = MBB.getParent();
-  const Function *F = MF->getFunction();
-  if (!F || MF->getMMI().callsEHReturn())
-    return 0;
-
-  const TargetRegisterClass &AvailableRegs = *TRI->getGPRsForTailCall(*MF);
-
-  unsigned Opc = MBBI->getOpcode();
+static bool isReturnInstruction(unsigned Opc) {
   switch (Opc) {
-  default: return 0;
+  default: return false;
   case X86::RET:
   case X86::RETL:
   case X86::RETQ:
   case X86::RETIL:
   case X86::RETIQ:
   case X86::TCRETURNdi:
   case X86::TCRETURNri:
   case X86::TCRETURNmi:
   case X86::TCRETURNdi64:
   case X86::TCRETURNri64:
   case X86::TCRETURNmi64:
   case X86::EH_RETURN:
-  case X86::EH_RETURN64: {
-    SmallSet<uint16_t, 8> Uses;
-    for (unsigned i = 0, e = MBBI->getNumOperands(); i != e; ++i) {
-      MachineOperand &MO = MBBI->getOperand(i);
-      if (!MO.isReg() || MO.isDef())
-        continue;
-      unsigned Reg = MO.getReg();
-      if (!Reg)
-        continue;
-      for (MCRegAliasIterator AI(Reg, TRI, true); AI.isValid(); ++AI)
-        Uses.insert(*AI);
-    }
+  case X86::EH_RETURN64:
+    return true;
+  }
+}
+
+static SmallSet<uint16_t, 8> getReturnInstructionUses(
+    MachineBasicBlock::iterator &MBBI,
+    const X86RegisterInfo *TRI) {
+  assert(isReturnInstruction(MBBI->getOpcode()));
+  return Uses;
+}
+
+/// findDeadCallerSavedReg - Return a caller-saved register that isn't live
+/// when it reaches the "return" instruction. We can then pop a stack object
+/// to this register without worry about clobbering it.
+static unsigned findDeadCallerSavedReg(MachineBasicBlock &MBB,
+                                       MachineBasicBlock::iterator &MBBI,
+                                       const X86RegisterInfo *TRI,
+                                       bool Is64Bit) {
+  const MachineFunction *MF = MBB.getParent();
+  const Function *F = MF->getFunction();
+  if (!F || MF->getMMI().callsEHReturn())
+    return 0;
+
+  const TargetRegisterClass &AvailableRegs = *TRI->getGPRsForTailCall(*MF);
+
+  unsigned Opc = MBBI->getOpcode();
+  if (!isReturnInstruction(Opc)) {
+    return 0;
+  } else {
+    SmallSet<uint16_t, 8> Uses = getReturnInstructionUses(MBBI, TRI);
 
     for (auto CS : AvailableRegs)
       if (!Uses.count(CS) && CS != X86::RIP)
         return CS;
   }
-  }
 
   return 0;
 }
 
 static bool isEAXLiveIn(MachineBasicBlock &MBB) {
   for (MachineBasicBlock::RegisterMaskPair RegMask : MBB.liveins()) {
     unsigned Reg = RegMask.PhysReg;
 
if ((Opc == X86::ADD64ri32 || Opc == X86::ADD64ri8 ||
     if (!doMergeWithPrevious) MBBI = NI;
   } else if ((Opc == X86::SUB64ri32 || Opc == X86::SUB64ri8 ||
               Opc == X86::SUB32ri || Opc == X86::SUB32ri8) &&
              PI->getOperand(0).getReg() == StackPtr) {
     assert(PI->getOperand(1).getReg() == StackPtr);
     Offset -= PI->getOperand(2).getImm();
     MBB.erase(PI);
     if (!doMergeWithPrevious) MBBI = NI;
+  } else if ((Opc == X86::POP64r || Opc == X86::POP32r) &&
+             PI->getOperand(0).getReg() != X86::RIP &&
+             isReturnInstruction(MBBI->getOpcode())) {
+    assert(doMergeWithPrevious);
+    SmallSet<uint16_t, 8> Uses = getReturnInstructionUses(MBBI, TRI);
+    while ((Opc == X86::POP64r || Opc == X86::POP32r) &&
+           PI->getOperand(0).getReg() != X86::RIP &&
+           !Uses.count(PI->getOperand(0).getReg())) {
+      Offset += (Opc == X86::POP64r ? 8 : 4);
+      MBB.erase(PI);
+      if (MBBI == MBB.begin())
+        break;
+      PI = std::prev(MBBI);
+      Opc = PI->getOpcode();
+    }
   }
 
   return Offset;
 }
 
 void X86FrameLowering::BuildCFI(MachineBasicBlock &MBB,
                                 MachineBasicBlock::iterator MBBI,
                                 const DebugLoc &DL,
void X86FrameLowering::emitEpilogue(MachineFunction &MF,
   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();
 

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
 }
 
+define cc 11 { i32, i32, i32 } @tailcaller(i32, i32) nounwind {
+  ; CHECK:      movl  $15, %eax
+  ; CHECK-NEXT: movl  $31, %edx
+  ; CHECK-NEXT: movl  $47, %ecx
+  ; CHECK-NEXT: popl  %edi
+  ; CHECK-NEXT: jmp tailcallee
+  %3 = tail call cc11 { i32, i32, i32 } @tailcallee(i32 %0, i32 %1, i32 15,
+     i32 31, i32 47, i32 63) nounwind
+  ret { i32, i32, i32 } %3
+}
+
 @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)

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
 }
 
+define cc 11 { i64, i64, i64 } @tailcaller(i64, i64) #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              # TAILCALL
+  %3 = tail call cc11 { i64, i64, i64 } @tailcallee(i64 %0, i64 %1, i64 15,
+     i64 31, i64 47, i64 63, i64 79) #1
+  ret { i64, i64, i64 } %3
+}
+
 @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)