ARCEntryPointBuilder: Don't mark the call instructions as tail-calls

rdar://problem/48833545

From the LLVM Manual regarding tail/musttail : "Both markers imply that the callee does not access allocas from the caller”

Swift’s LLVMARCContract just marks all the calls it creates as tail call without any analysis and/or checking if we are allowed to do that. This created an interesting runtime crash that was a pain to debug - story time:

I traced a runtime crash back to Swift’s LLVMARCContract, but could not grok why the transformation there is wrong: we replaced two consecutive _swift_bridgeObjectRelease(x) calls with _swift_bridgeObjectRelease_n(x, 2), which is a perfectly valid thing to do.

I noticed that the new call is marked as a tail call, disabling that portion of the pass “solved” the runtime crash, but I wanted to understand *why*:

This code worked:
	pushq	$2
	popq	%rsi
	movq	-168(%rbp), %rdi
	callq	_swift_bridgeObjectRelease_n
	leaq	-40(%rbp), %rsp
	popq	%rbx
	popq	%r12
	popq	%r13
	popq	%r14
	popq	%r15
	popq	%rbp
	retq

While this version crashed further on during the run:
	movq	-168(%rbp), %rdi
	leaq	-40(%rbp), %rsp
	popq	%rbx
	popq	%r12
	popq	%r13
	popq	%r14
	popq	%r15
	popq	%rbp
	jmp	_swift_bridgeObjectRelease_n

As you can see, the call is the last thing we do before returning, so nothing appeared out of the ordinary at first…

Dumping the heap object at the release basic block looked perfectly fine: the ref count was 2 and all the fields looked valid.

However, when we reached the callee the value was modified / dumping it showed it changed somewhere. Which did not make any sense.

Setting up a memory watchpoint on the heap object and/or its reference count did not get us anywhere: the watchpoint triggered on unrelated code in the entry to the callee..

I then realized what’s going on, here’s a an amusing reproducer that you can checkout in LLDB:
Experiment 1:
Setup a breakpoint at leaq	-40(%rbp), %rsp
Dump the heap object - it looks good
Experiment 2:
Rerun the same test with a small modification:
Setup a breakpoint at popq	%rbx (the instruction after leaq, do not set a breakpoint at leaq)
Dump the heap object - it looks bad!

So what is going on there? The SIL Optimizer changed an alloc_ref instruction into an alloc_ref [stack], which is a perfectly valid thing to do.

However, this means we allocated the heap object on the stack, and then tail-called into the swift runtime with said object. After having modified the stack pointer in the caller’s epilogue.

So why does experiment 2 show garbage? We’ve updated the stack pointer, and it just so happens that we are after the red zone on the stack. When the breakpoint is hit (the OS passes control back to LLDB), it is perfectly allowed to use the memory where the heap object used to reside.

Note: I then realized something even more concerning, that we were lucky not have hit so far: not only did we not check if we are allowed to mark a call as ’tail’ in this situation, which could have been considered a corner case,  we could have if we have not promoted it from heap to stack, but we marked *ALL* the call instructions created in this pass as tail call even if they are not the last thing that occurred in the calling function! Looking at the LVMPasses/contract.ll test case, which is modified in this PR, we see some scary checks that are just wrong: we are checking if a call is marked as ‘tail’ in the middle of the function, then check the rest of the function in CHECK-NEXT lines. Knowing full well that the new ‘tail call’ is not the last thing that should execute in the caller.

Author: Joe Shajrawi

lib/LLVMPasses/ARCEntryPointBuilder.h

@@ -119,7 +119,6 @@ class ARCEntryPointBuilder {
 
     // Create the call.
     CallInst *CI = CreateCall(getRetain(OrigI), V);
-    CI->setTailCall(true);
     return CI;
   }
 
@@ -129,7 +128,6 @@ class ARCEntryPointBuilder {
 
     // Create the call.
     CallInst *CI = CreateCall(getRelease(OrigI), V);
-    CI->setTailCall(true);
     return CI;
   }
 
@@ -139,23 +137,20 @@ class ARCEntryPointBuilder {
     V = B.CreatePointerCast(V, getObjectPtrTy());
 
     CallInst *CI = CreateCall(getCheckUnowned(OrigI), V);
-    CI->setTailCall(true);
     return CI;
   }
 
   CallInst *createRetainN(Value *V, uint32_t n, CallInst *OrigI) {
     // Cast just to make sure that we have the right object type.
     V = B.CreatePointerCast(V, getObjectPtrTy());
     CallInst *CI = CreateCall(getRetainN(OrigI), {V, getIntConstant(n)});
-    CI->setTailCall(true);
     return CI;
   }
 
   CallInst *createReleaseN(Value *V, uint32_t n, CallInst *OrigI) {
     // Cast just to make sure we have the right object type.
     V = B.CreatePointerCast(V, getObjectPtrTy());
     CallInst *CI = CreateCall(getReleaseN(OrigI), {V, getIntConstant(n)});
-    CI->setTailCall(true);
     return CI;
   }
 
@@ -164,7 +159,6 @@ class ARCEntryPointBuilder {
     V = B.CreatePointerCast(V, getObjectPtrTy());
     CallInst *CI =
         CreateCall(getUnknownObjectRetainN(OrigI), {V, getIntConstant(n)});
-    CI->setTailCall(true);
     return CI;
   }
 
@@ -173,23 +167,20 @@ class ARCEntryPointBuilder {
     V = B.CreatePointerCast(V, getObjectPtrTy());
     CallInst *CI =
         CreateCall(getUnknownObjectReleaseN(OrigI), {V, getIntConstant(n)});
-    CI->setTailCall(true);
     return CI;
   }
 
   CallInst *createBridgeRetainN(Value *V, uint32_t n, CallInst *OrigI) {
     // Cast just to make sure we have the right object type.
     V = B.CreatePointerCast(V, getBridgeObjectPtrTy());
     CallInst *CI = CreateCall(getBridgeRetainN(OrigI), {V, getIntConstant(n)});
-    CI->setTailCall(true);
     return CI;
   }
 
   CallInst *createBridgeReleaseN(Value *V, uint32_t n, CallInst *OrigI) {
     // Cast just to make sure we have the right object type.
     V = B.CreatePointerCast(V, getBridgeObjectPtrTy());
     CallInst *CI = CreateCall(getBridgeReleaseN(OrigI), {V, getIntConstant(n)});
-    CI->setTailCall(true);
     return CI;
   }