StackProtector: use isInTailCallPosition to verify tail call position

[taoliq]

The issue is caused by D133860.
The guard would be inserted in wrong place in some cases, like the test case showed below.
This patch fixed the issue by using isInTailCallPosition() to verify whether the tail call is in right position.

[llvmbot]

@llvm/pr-subscribers-backend-x86

Author: Liqiang TAO (taoliq)

Changes

The issue is caused by D133860.
The guard would be inserted in wrong place in some cases, like the test case showed below.
This patch fixed the issue by using isInTailCallPosition() to verify whether the tail call is in right position.

---

Full diff: https://github.com/llvm/llvm-project/pull/68997.diff

2 Files Affected:

• (modified) llvm/lib/CodeGen/StackProtector.cpp (+5-9)
• (added) llvm/test/CodeGen/X86/tailcc-ssp2.ll (+14)

diff --git a/llvm/lib/CodeGen/StackProtector.cpp b/llvm/lib/CodeGen/StackProtector.cpp
index 387b653f8815367..8204425a350914d 100644
--- a/llvm/lib/CodeGen/StackProtector.cpp
+++ b/llvm/lib/CodeGen/StackProtector.cpp
@@ -20,6 +20,7 @@
 #include "llvm/Analysis/BranchProbabilityInfo.h"
 #include "llvm/Analysis/MemoryLocation.h"
 #include "llvm/Analysis/OptimizationRemarkEmitter.h"
+#include "llvm/CodeGen/Analysis.h"
 #include "llvm/CodeGen/Passes.h"
 #include "llvm/CodeGen/TargetLowering.h"
 #include "llvm/CodeGen/TargetPassConfig.h"
@@ -520,17 +521,12 @@ bool StackProtector::InsertStackProtectors() {
     HasIRCheck = true;
 
     // 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 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.
+    // inserted before the call rather than between it and the return.
     Instruction *Prev = CheckLoc->getPrevNonDebugInstruction();
-    if (Prev && isa<CallInst>(Prev) && cast<CallInst>(Prev)->isTailCall())
+    if (Prev && isa<CallInst>(Prev) &&
+        cast<CallInst>(Prev)->isTailCall() &&
+        isInTailCallPosition(*cast<CallInst>(Prev), *TM)) {
       CheckLoc = Prev;
-    else if (Prev) {
-      Prev = Prev->getPrevNonDebugInstruction();
-      if (Prev && isa<CallInst>(Prev) && cast<CallInst>(Prev)->isTailCall())
-        CheckLoc = Prev;
     }
 
     // Generate epilogue instrumentation. The epilogue intrumentation can be
diff --git a/llvm/test/CodeGen/X86/tailcc-ssp2.ll b/llvm/test/CodeGen/X86/tailcc-ssp2.ll
new file mode 100644
index 000000000000000..af6ddaecae2032b
--- /dev/null
+++ b/llvm/test/CodeGen/X86/tailcc-ssp2.ll
@@ -0,0 +1,14 @@
+; RUN: llc -mtriple=x86_64-linux-gnu %s -o - 2>&1 | FileCheck %s
+
+declare void @callee()
+define void @caller() sspreq {
+; CHECK: callq   callee@PLT
+; CHECK: callq   callee@PLT
+; CHECK: cmpq
+; CHECK: jne
+; CHECK: callq   __stack_chk_fail@PLT
+
+  tail call void @callee()
+  call void @callee()
+  ret void
+}

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[taoliq]

May I merge this patch at December 15 if there is no other problems?

[efriedma-quic]

Please don't try to set deadlines on reviews; the amount of time people have to spend on reviews can be a bit unpredictable. A lack of review doesn't imply approval.

If someone who has been reviewing a patch doesn't respond, put a "ping" comment in the review, and if there's still no response try to contact directly via email. And if there's still no response, note that on the PR, and ask other reviewers to approve.

(I can pick up reviewing here if there's no response in a week or so.)