StackProtector: enable tail call optimization even without musttail

[taolq]

Summary

When -O2 and -fstack-protector-all are added together in clang, the tail call optimization will be disabled.
See here https://godbolt.org/z/bffPPv3zh
Because opt only emit ssqreq and tail call attribute for them,
and theses optimizations are handled by llc exactly.

So in this patch, I would like to release the condition in StackProtector.
Even for tail call (not musttail call), both stack protection and TCO(tail call optimization) will be enabled.
And finally, clang will also benefit from this small change.

[compnerd]

What do you think of keeping the musttail variant of the test while augmenting it with the tailcall cases?

[taolq]

• append tail cases to musttail test

[taolq]

In D133860#3790200, @compnerd wrote:

What do you think of keeping the musttail variant of the test while augmenting it with the tailcall cases?

Thanks for the comment.
The tests have been updated.

[compnerd]

llvm/lib/CodeGen/StackProtector.cpp

478

Please update the comment to replace the references to musttail.

llvm/test/CodeGen/AArch64/stack-protector-musttail.ll

106

Add missing newline please.

llvm/test/CodeGen/ARM/Windows/stack-protector-musttail.ll

88

Add missing newline please

[taolq]

• update the comment
• add missing newline

[taolq]

• fix format

[efriedma]

llvm/lib/CodeGen/StackProtector.cpp

485

Are you sure this actually does the right thing? Consider something like:

tail call void f()
call void g()
ret void

If I'm following correctly, we'll insert the guard before the call to f(), which seems wrong.

(The old code worked because a musttail call is always in tail position; a "tail" call is not.)

[efriedma]

Do we care that the backend might not choose to tail-call a function even if it's in tail position? The "tail" marker is a hint; depending on attributes, calling convention, etc., the backend might not actually generate a tail call.

[taolq]

In D133860#3796726, @efriedma wrote:

Do we care that the backend might not choose to tail-call a function even if it's in tail position? The "tail" marker is a hint; depending on attributes, calling convention, etc., the backend might not actually generate a tail call.

I am not sure about this. Is that possible that a musttail-call is handled but a tail-call is not generated with all the same conditions?

llvm/lib/CodeGen/StackProtector.cpp

485

Thanks for your comment. I didn't notice that.
So we should remove the latter if condition.
Or let the verifier guarantee a tail call is always in the tail position.
Which one could be better?

[efriedma]

In D133860#3798549, @taolq wrote:

In D133860#3796726, @efriedma wrote:

Do we care that the backend might not choose to tail-call a function even if it's in tail position? The "tail" marker is a hint; depending on attributes, calling convention, etc., the backend might not actually generate a tail call.

I am not sure about this. Is that possible that a musttail-call is handled but a tail-call is not generated with all the same conditions?

Yes. The most obvious case is if the caller has the "disable-tail-calls" attribute, but there are others.

llvm/lib/CodeGen/StackProtector.cpp

485

Other parts of CodeGen uses llvm::isInTailCallPosition to verify tail calls; I think it accepts more constructs than the verifier.

We intentionally allow calls marked "tail" in non-tail position: it simplifies the code that does the marking, and the "tail" marking can also be used for alias analysis.

[efriedma]

Did the review comments here ever get addressed?

[taolq]

In D133860#4303529, @efriedma wrote:

Did the review comments here ever get addressed?

Sorry for forgetting to solve that. I notice that I still have something to figure out. I will try my best to address it.

[taolq]

In D133860#4303529, @efriedma wrote:

Did the review comments here ever get addressed?

This issue is fixed by https://github.com/llvm/llvm-project/pull/68997

llvm/lib/CodeGen/StackProtector.cpp

if (!AI) {
       AI = cast<AllocaInst>(SPCall->getArgOperand(1));
     }
 
     // Set HasIRCheck to true, so that SelectionDAG will not generate its own
     // version. SelectionDAG called 'shouldEmitSDCheck' to check whether
     // instrumentation has already been generated.
     HasIRCheck = true;
 
-    // If we're instrumenting a block with a musttail call, the check has to be
+    // If we're instrumenting a block with a tail call, the check has to be
     // inserted before the call rather than between it and the return. The
-    // verifier guarantees that a musttail call is either directly before the
+    // verifier guarantees that a tail call is either directly before the
     // return or with a single correct bitcast of the return value in between so
     // we don't need to worry about many situations here.
     Instruction *CheckLoc = RI;
     Instruction *Prev = RI->getPrevNonDebugInstruction();
-    if (Prev && isa<CallInst>(Prev) && cast<CallInst>(Prev)->isMustTailCall())
+    if (Prev && isa<CallInst>(Prev) && cast<CallInst>(Prev)->isTailCall())
       CheckLoc = Prev;
     else if (Prev) {
       Prev = Prev->getPrevNonDebugInstruction();
-      if (Prev && isa<CallInst>(Prev) && cast<CallInst>(Prev)->isMustTailCall())
+      if (Prev && isa<CallInst>(Prev) && cast<CallInst>(Prev)->isTailCall())
         CheckLoc = Prev;
     }
 
     // Generate epilogue instrumentation. The epilogue intrumentation can be
     // function-based or inlined depending on which mechanism the target is
     // providing.
     if (Function *GuardCheck = TLI->getSSPStackGuardCheck(*M)) {
       // Generate the function-based epilogue instrumentation.

llvm/test/CodeGen/AArch64/stack-protector-musttail.ll

 ; CHECK-NEXT: ret i8* [[RES]]
 
   %var = alloca [2 x i64]
   store [2 x i64]* %var, [2 x i64]** @var
   %tmp = musttail call i64* @callee2()
   %res = bitcast i64* %tmp to i8*
   ret i8* %res
 }
+
+define void @caller3() ssp {
+; CHECK-LABEL: define void @caller3()
+; Prologue:
+; CHECK: @llvm.stackguard
+
+; CHECK: [[GUARD:%.*]] = call i8* @llvm.stackguard()
+; CHECK: [[TOKEN:%.*]] = load volatile i8*, i8** {{%.*}}
+; CHECK: [[TST:%.*]] = icmp eq i8* [[GUARD]], [[TOKEN]]
+; CHECK: br i1 [[TST]]
+
+; CHECK: tail call void @callee()
+; CHECK-NEXT: ret void
+  %var = alloca [2 x i64]
+  store [2 x i64]* %var, [2 x i64]** @var
+  tail call void @callee()
+  ret void
+}
+
+define i8* @caller4() ssp {
+; CHECK-LABEL: define i8* @caller4()
+; Prologue:
+; CHECK: @llvm.stackguard
+
+; CHECK: [[GUARD:%.*]] = call i8* @llvm.stackguard()
+; CHECK: [[TOKEN:%.*]] = load volatile i8*, i8** {{%.*}}
+; CHECK: [[TST:%.*]] = icmp eq i8* [[GUARD]], [[TOKEN]]
+; CHECK: br i1 [[TST]]
+
+; CHECK: [[TMP:%.*]] = tail call i64* @callee2()
+; CHECK-NEXT: [[RES:%.*]] = bitcast i64* [[TMP]] to i8*
+; CHECK-NEXT: ret i8* [[RES]]
+
+  %var = alloca [2 x i64]
+  store [2 x i64]* %var, [2 x i64]** @var
+  %tmp = tail call i64* @callee2()
+  %res = bitcast i64* %tmp to i8*
+  ret i8* %res
+}

llvm/test/CodeGen/ARM/Windows/stack-protector-musttail.ll

; CHECK: ret void
   %var = alloca [2 x i64]
   store [2 x i64]* %var, [2 x i64]** @var
   br label %retblock
 
 retblock:
   ret void
 }
 
-
 declare i64* @callee2()
 
 define i8* @caller2() sspreq {
 ; CHECK-LABEL: define i8* @caller2()
 ; Prologue:
 ; CHECK: @llvm.stackguard
 
 ; CHECK: call void @__security_check_cookie
 
 ; CHECK: [[TMP:%.*]] = musttail call i64* @callee2()
 ; CHECK-NEXT: [[RES:%.*]] = bitcast i64* [[TMP]] to i8*
 ; CHECK-NEXT: ret i8* [[RES]]
 
   %var = alloca [2 x i64]
   store [2 x i64]* %var, [2 x i64]** @var
   %tmp = musttail call i64* @callee2()
   %res = bitcast i64* %tmp to i8*
   ret i8* %res
 }
+
+define void @caller3() sspreq {
+; CHECK-LABEL: define void @caller3()
+; Prologue:
+
+; CHECK: call void @__security_check_cookie
+
+; CHECK: tail call void @callee()
+; CHECK-NEXT: ret void
+  %var = alloca [2 x i64]
+  store [2 x i64]* %var, [2 x i64]** @var
+  tail call void @callee()
+  ret void
+}
+
+define i8* @caller4() sspreq {
+; CHECK-LABEL: define i8* @caller4()
+; Prologue:
+; CHECK: @llvm.stackguard
+
+; CHECK: call void @__security_check_cookie
+
+; CHECK: [[TMP:%.*]] = tail call i64* @callee2()
+; CHECK-NEXT: [[RES:%.*]] = bitcast i64* [[TMP]] to i8*
+; CHECK-NEXT: ret i8* [[RES]]
+
+  %var = alloca [2 x i64]
+  store [2 x i64]* %var, [2 x i64]** @var
+  %tmp = tail call i64* @callee2()
+  %res = bitcast i64* %tmp to i8*
+  ret i8* %res
+}

llvm/test/CodeGen/X86/stack-protector-musttail.ll

+; RUN: llc -mtriple=x86_64-linux-gnu -fast-isel %s -o - -start-before=stack-protector -stop-after=stack-protector  | FileCheck %s
+
+@var = global [2 x i64]* null
+
+declare void @callee()
+
+define void @caller1() sspreq {
+; CHECK-LABEL: define void @caller1()
+; Prologue:
+; CHECK: @llvm.stackprotector
+
+; CHECK: [[GUARD:%.*]] = load volatile i8*, i8*
+; CHECK: [[TOKEN:%.*]] = load volatile i8*, i8** {{%.*}}
+; CHECK: [[TST:%.*]] = icmp eq i8* [[GUARD]], [[TOKEN]]
+; CHECK: br i1 [[TST]]
+
+; CHECK: musttail call void @callee()
+; CHECK-NEXT: ret void
+  %var = alloca [2 x i64]
+  store [2 x i64]* %var, [2 x i64]** @var
+  musttail call void @callee()
+  ret void
+}
+
+define void @justret() sspreq {
+; CHECK-LABEL: define void @justret()
+; Prologue:
+; CHECK: @llvm.stackprotector
+
+; CHECK: [[GUARD:%.*]] = load volatile i8*, i8*
+; CHECK: [[TOKEN:%.*]] = load volatile i8*, i8** {{%.*}}
+; CHECK: [[TST:%.*]] = icmp eq i8* [[GUARD]], [[TOKEN]]
+; CHECK: br i1 [[TST]]
+
+; CHECK: ret void
+  %var = alloca [2 x i64]
+  store [2 x i64]* %var, [2 x i64]** @var
+  br label %retblock
+
+retblock:
+  ret void
+}
+
+
+declare i64* @callee2()
+
+define i8* @caller2() sspreq {
+; CHECK-LABEL: define i8* @caller2()
+; Prologue:
+; CHECK: @llvm.stackprotector
+
+; CHECK: [[GUARD:%.*]] = load volatile i8*, i8*
+; CHECK: [[TOKEN:%.*]] = load volatile i8*, i8** {{%.*}}
+; CHECK: [[TST:%.*]] = icmp eq i8* [[GUARD]], [[TOKEN]]
+; CHECK: br i1 [[TST]]
+
+; CHECK: [[TMP:%.*]] = musttail call i64* @callee2()
+; CHECK-NEXT: [[RES:%.*]] = bitcast i64* [[TMP]] to i8*
+; CHECK-NEXT: ret i8* [[RES]]
+
+  %var = alloca [2 x i64]
+  store [2 x i64]* %var, [2 x i64]** @var
+  %tmp = musttail call i64* @callee2()
+  %res = bitcast i64* %tmp to i8*
+  ret i8* %res
+}
+
+define void @caller3() sspreq {
+; CHECK-LABEL: define void @caller3()
+; Prologue:
+; CHECK: @llvm.stackprotector
+
+; CHECK: [[GUARD:%.*]] = load volatile i8*, i8*
+; CHECK: [[TOKEN:%.*]] = load volatile i8*, i8** {{%.*}}
+; CHECK: [[TST:%.*]] = icmp eq i8* [[GUARD]], [[TOKEN]]
+; CHECK: br i1 [[TST]]
+
+; CHECK: tail call void @callee()
+; CHECK-NEXT: ret void
+  %var = alloca [2 x i64]
+  store [2 x i64]* %var, [2 x i64]** @var
+  tail call void @callee()
+  ret void
+}
+
+define i8* @caller4() sspreq {
+; CHECK-LABEL: define i8* @caller4()
+; Prologue:
+; CHECK: @llvm.stackprotector
+
+; CHECK: [[GUARD:%.*]] = load volatile i8*, i8*
+; CHECK: [[TOKEN:%.*]] = load volatile i8*, i8** {{%.*}}
+; CHECK: [[TST:%.*]] = icmp eq i8* [[GUARD]], [[TOKEN]]
+; CHECK: br i1 [[TST]]
+
+; CHECK: [[TMP:%.*]] = tail call i64* @callee2()
+; CHECK-NEXT: [[RES:%.*]] = bitcast i64* [[TMP]] to i8*
+; CHECK-NEXT: ret i8* [[RES]]
+
+  %var = alloca [2 x i64]
+  store [2 x i64]* %var, [2 x i64]** @var
+  %tmp = tail call i64* @callee2()
+  %res = bitcast i64* %tmp to i8*
+  ret i8* %res
+}

llvm/test/CodeGen/X86/tailcc-ssp.ll

-; RUN: llc -mtriple=x86_64-windows-msvc %s -o - -verify-machineinstrs | FileCheck %s
+; RUN: llc -mtriple=x86_64-windows-msvc %s -o - -verify-machineinstrs | FileCheck %s -check-prefix=WINDOWS
+; RUN: llc -mtriple=x86_64-linux-gnu    %s -o - -verify-machineinstrs | FileCheck %s -check-prefix=LINUX
 
 declare void @h(ptr, i64, ptr)
 
 define tailcc void @tailcall_frame(ptr %0, i64 %1) sspreq {
-; CHECK-LABEL: tailcall_frame:
-; CHECK: callq __security_check_cookie
-; CHECK: xorl %ecx, %ecx
-; CHECK: jmp h
+; WINDOWS-LABEL: tailcall_frame:
+; WINDOWS: callq __security_check_cookie
+; WINDOWS: xorl %ecx, %ecx
+; WINDOWS: jmp h
+
+; LINUX-LABEL: tailcall_frame:
+; LINUX: jne
+; LINUX: jmp h
+; LINUX: callq __stack_chk_fail
 
    tail call tailcc void @h(ptr null, i64 0, ptr null)
    ret void
 }
 
 declare void @bar()
 define void @tailcall_unrelated_frame() sspreq {
-; CHECK-LABEL: tailcall_unrelated_frame:
-; CHECK: subq [[STACK:\$.*]], %rsp
-; CHECK: callq bar
-; CHECK: callq __security_check_cookie
-; CHECK: addq [[STACK]], %rsp
-; CHECK: jmp bar
+; WINDOWS-LABEL: tailcall_unrelated_frame:
+; WINDOWS: subq [[STACK:\$.*]], %rsp
+; WINDOWS: callq bar
+; WINDOWS: callq __security_check_cookie
+; WINDOWS: addq [[STACK]], %rsp
+; WINDOWS: jmp bar
+
+; LINUX-LABEL: tailcall_unrelated_frame:
+; LINUX: callq bar
+; LINUX: jne
+; LINUX: jmp bar
+; LINUX: callq __stack_chk_fail
+
   call void @bar()
   tail call void @bar()
   ret void
 }