StackProtector: Use PointerMayBeCaptured

[arsenm]

Summary

This was using its own, outdated list of possible captures. This was
at minimum not catching cmpxchg and addrspacecast captures.

One change is now any volatile access is treated as capturing. The
test coverage for this pass is quite inadequate, but this required
removing volatile in the lifetime capture test.

Also fixes some infrastructure issues to allow running just the IR
pass.

Fixes bug 42238.

[jdoerfert]

Generally fine, three smaller comments though.

lib/CodeGen/StackProtector.cpp

179

VisitedPHIs is not needed anymore.

267–269

Are we sure we want return to capture?

And could you add the /* StoreCaptures */ comments so ppl know what the booleans are.

test/CodeGen/X86/stack-protector.ll

4091

Was the problem we assume volatile stores cause the addrs to escape (=be captured)?
If so, can we add a comment, maybe even a negative test (with the volatile accesses)?

[arsenm]

lib/CodeGen/StackProtector.cpp

267–269

I don't know, I just figured it should be more conservative

test/CodeGen/X86/stack-protector.ll

4091

I think the interpretation of volatile capturing the address is absurd, but I don't care enough to fight that battle. The volatiles were irrelevant for the test here

[jdoerfert]

lib/CodeGen/StackProtector.cpp

267–269

I see. My reasoning was: The original code new about the existence of return and it did not consider it as "address taken" so, ...

test/CodeGen/X86/stack-protector.ll

4091

Fair point.

[arsenm]

Make return not capture

[jdoerfert]

This looks good to me (LGTM). Give it a bit of time though for people familiar with this code to take a look if they want.

test/CodeGen/X86/stack-protector.ll

4091

At least the change is now "documented" in this review ;)

[arsenm]

r363169

include/llvm/CodeGen/StackProtector.h

private:
   /// StackProtector analysis will update this map when determining if an
   /// AllocaInst triggers a stack protector.
   SSPLayoutMap Layout;
 
   /// The minimum size of buffers that will receive stack smashing
   /// protection when -fstack-protection is used.
   unsigned SSPBufferSize = 0;
 
-  /// VisitedPHIs - The set of PHI nodes visited when determining
-  /// if a variable's reference has been taken.  This set
-  /// is maintained to ensure we don't visit the same PHI node multiple
-  /// times.
-  SmallPtrSet<const PHINode *, 16> VisitedPHIs;
-
   // A prologue is generated.
   bool HasPrologue = false;
 
   // IR checking code is generated.
   bool HasIRCheck = false;
 
   /// InsertStackProtectors - Insert code into the prologue and epilogue of
   /// the function.

lib/CodeGen/StackProtector.cpp

 // changed, then there was some sort of violation and the program aborts.
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/CodeGen/StackProtector.h"
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/Statistic.h"
 #include "llvm/Analysis/BranchProbabilityInfo.h"
+#include "llvm/Analysis/CaptureTracking.h"
 #include "llvm/Analysis/EHPersonalities.h"
 #include "llvm/Analysis/OptimizationRemarkEmitter.h"
 #include "llvm/CodeGen/Passes.h"
 #include "llvm/CodeGen/TargetLowering.h"
 #include "llvm/CodeGen/TargetPassConfig.h"
 #include "llvm/CodeGen/TargetSubtargetInfo.h"
 #include "llvm/IR/Attributes.h"
 #include "llvm/IR/BasicBlock.h"
if (ContainsProtectableArray(*I, IsLarge, Strong, true)) {
       if (IsLarge)
         return true;
       NeedsProtector = true;
     }
 
   return NeedsProtector;
 }
 
-bool StackProtector::HasAddressTaken(const Instruction *AI) {
-  for (const User *U : AI->users()) {
-    if (const StoreInst *SI = dyn_cast<StoreInst>(U)) {
-      if (AI == SI->getValueOperand())
-        return true;
-    } else if (const PtrToIntInst *SI = dyn_cast<PtrToIntInst>(U)) {
-      if (AI == SI->getOperand(0))
-        return true;
-    } else if (const CallInst *CI = dyn_cast<CallInst>(U)) {
-      // Ignore intrinsics that are not calls. TODO: Use isLoweredToCall().
-      if (!isa<DbgInfoIntrinsic>(CI) && !CI->isLifetimeStartOrEnd())
-        return true;
-    } else if (isa<InvokeInst>(U)) {
-      return true;
-    } else if (const SelectInst *SI = dyn_cast<SelectInst>(U)) {
-      if (HasAddressTaken(SI))
-        return true;
-    } else if (const PHINode *PN = dyn_cast<PHINode>(U)) {
-      // Keep track of what PHI nodes we have already visited to ensure
-      // they are only visited once.
-      if (VisitedPHIs.insert(PN).second)
-        if (HasAddressTaken(PN))
-          return true;
-    } else if (const GetElementPtrInst *GEP = dyn_cast<GetElementPtrInst>(U)) {
-      if (HasAddressTaken(GEP))
-        return true;
-    } else if (const BitCastInst *BI = dyn_cast<BitCastInst>(U)) {
-      if (HasAddressTaken(BI))
-        return true;
-    }
-  }
-  return false;
-}
-
 /// Search for the first call to the llvm.stackprotector intrinsic and return it
 /// if present.
 static const CallInst *findStackProtectorIntrinsic(Function &F) {
   for (const BasicBlock &BB : F)
     for (const Instruction &I : BB)
       if (const CallInst *CI = dyn_cast<CallInst>(&I))
         if (CI->getCalledFunction() ==
             Intrinsic::getDeclaration(F.getParent(), Intrinsic::stackprotector))
for (const Instruction &I : BB) {
                    << ore::NV("Function", F)
                    << " due to a stack allocated buffer or struct containing a "
                       "buffer";
           });
           NeedsProtector = true;
           continue;
         }
 
-        if (Strong && HasAddressTaken(AI)) {
+        if (Strong && PointerMayBeCaptured(AI,
+                                           /* ReturnCaptures */ false,
+                                           /* StoreCaptures */ true)) {
           ++NumAddrTaken;
           Layout.insert(std::make_pair(AI, MachineFrameInfo::SSPLK_AddrOf));
           ORE.emit([&]() {
             return OptimizationRemark(DEBUG_TYPE, "StackProtectorAddressTaken",
                                       &I)
                    << "Stack protection applied to function "
                    << ore::NV("Function", F)
                    << " due to the address of a local variable being taken";

test/CodeGen/X86/stack-protector.ll

; OPENBSD-AMD64-NEXT:  callq __stack_smash_handler
   ret void, !dbg !9
 }
 
 define i32 @IgnoreIntrinsicTest() #1 {
 ; IGNORE_INTRIN: IgnoreIntrinsicTest:
   %1 = alloca i32, align 4
   %2 = bitcast i32* %1 to i8*
   call void @llvm.lifetime.start.p0i8(i64 4, i8* nonnull %2)
-  store volatile i32 1, i32* %1, align 4
-  %3 = load volatile i32, i32* %1, align 4
+  store i32 1, i32* %1, align 4
+  %3 = load i32, i32* %1, align 4
   %4 = mul nsw i32 %3, 42
   call void @llvm.lifetime.end.p0i8(i64 4, i8* nonnull %2)
   ret i32 %4
 ; IGNORE_INTRIN-NOT: callq __stack_chk_fail
 ; IGNORE_INTRIN:     .cfi_endproc
 }
 
 declare double @testi_aux()

test/Transforms/StackProtector/X86/captures.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -S -mtriple=x86_64-pc-linux-gnu -stack-protector < %s | FileCheck %s
+; Bug 42238: Test some situations missed by old, custom capture tracking.
+
+define void @store_captures() #0 {
+; CHECK-LABEL: @store_captures(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[STACKGUARDSLOT:%.*]] = alloca i8*
+; CHECK-NEXT:    [[STACKGUARD:%.*]] = load volatile i8*, i8* addrspace(257)* inttoptr (i32 40 to i8* addrspace(257)*)
+; CHECK-NEXT:    call void @llvm.stackprotector(i8* [[STACKGUARD]], i8** [[STACKGUARDSLOT]])
+; CHECK-NEXT:    [[RETVAL:%.*]] = alloca i32, align 4
+; CHECK-NEXT:    [[A:%.*]] = alloca i32, align 4
+; CHECK-NEXT:    [[J:%.*]] = alloca i32*, align 8
+; CHECK-NEXT:    store i32 0, i32* [[RETVAL]]
+; CHECK-NEXT:    [[LOAD:%.*]] = load i32, i32* [[A]], align 4
+; CHECK-NEXT:    [[ADD:%.*]] = add nsw i32 [[LOAD]], 1
+; CHECK-NEXT:    store i32 [[ADD]], i32* [[A]], align 4
+; CHECK-NEXT:    store i32* [[A]], i32** [[J]], align 8
+; CHECK-NEXT:    [[STACKGUARD1:%.*]] = load volatile i8*, i8* addrspace(257)* inttoptr (i32 40 to i8* addrspace(257)*)
+; CHECK-NEXT:    [[TMP0:%.*]] = load volatile i8*, i8** [[STACKGUARDSLOT]]
+; CHECK-NEXT:    [[TMP1:%.*]] = icmp eq i8* [[STACKGUARD1]], [[TMP0]]
+; CHECK-NEXT:    br i1 [[TMP1]], label [[SP_RETURN:%.*]], label [[CALLSTACKCHECKFAILBLK:%.*]], !prof !0
+; CHECK:       SP_return:
+; CHECK-NEXT:    ret void
+; CHECK:       CallStackCheckFailBlk:
+; CHECK-NEXT:    call void @__stack_chk_fail()
+; CHECK-NEXT:    unreachable
+;
+entry:
+  %retval = alloca i32, align 4
+  %a = alloca i32, align 4
+  %j = alloca i32*, align 8
+  store i32 0, i32* %retval
+  %load = load i32, i32* %a, align 4
+  %add = add nsw i32 %load, 1
+  store i32 %add, i32* %a, align 4
+  store i32* %a, i32** %j, align 8
+  ret void
+}
+
+define i32* @return_captures() #0 {
+; CHECK-LABEL: @return_captures(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[RETVAL:%.*]] = alloca i32, align 4
+; CHECK-NEXT:    [[A:%.*]] = alloca i32, align 4
+; CHECK-NEXT:    [[J:%.*]] = alloca i32*, align 8
+; CHECK-NEXT:    store i32 0, i32* [[RETVAL]]
+; CHECK-NEXT:    [[LOAD:%.*]] = load i32, i32* [[A]], align 4
+; CHECK-NEXT:    [[ADD:%.*]] = add nsw i32 [[LOAD]], 1
+; CHECK-NEXT:    store i32 [[ADD]], i32* [[A]], align 4
+; CHECK-NEXT:    ret i32* [[A]]
+;
+entry:
+  %retval = alloca i32, align 4
+  %a = alloca i32, align 4
+  %j = alloca i32*, align 8
+  store i32 0, i32* %retval
+  %load = load i32, i32* %a, align 4
+  %add = add nsw i32 %load, 1
+  store i32 %add, i32* %a, align 4
+  ret i32* %a
+}
+
+define void @store_addrspacecast_captures() #0 {
+; CHECK-LABEL: @store_addrspacecast_captures(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[STACKGUARDSLOT:%.*]] = alloca i8*
+; CHECK-NEXT:    [[STACKGUARD:%.*]] = load volatile i8*, i8* addrspace(257)* inttoptr (i32 40 to i8* addrspace(257)*)
+; CHECK-NEXT:    call void @llvm.stackprotector(i8* [[STACKGUARD]], i8** [[STACKGUARDSLOT]])
+; CHECK-NEXT:    [[RETVAL:%.*]] = alloca i32, align 4
+; CHECK-NEXT:    [[A:%.*]] = alloca i32, align 4
+; CHECK-NEXT:    [[J:%.*]] = alloca i32 addrspace(1)*, align 8
+; CHECK-NEXT:    store i32 0, i32* [[RETVAL]]
+; CHECK-NEXT:    [[LOAD:%.*]] = load i32, i32* [[A]], align 4
+; CHECK-NEXT:    [[ADD:%.*]] = add nsw i32 [[LOAD]], 1
+; CHECK-NEXT:    store i32 [[ADD]], i32* [[A]], align 4
+; CHECK-NEXT:    [[A_ADDRSPACECAST:%.*]] = addrspacecast i32* [[A]] to i32 addrspace(1)*
+; CHECK-NEXT:    store i32 addrspace(1)* [[A_ADDRSPACECAST]], i32 addrspace(1)** [[J]], align 8
+; CHECK-NEXT:    [[STACKGUARD1:%.*]] = load volatile i8*, i8* addrspace(257)* inttoptr (i32 40 to i8* addrspace(257)*)
+; CHECK-NEXT:    [[TMP0:%.*]] = load volatile i8*, i8** [[STACKGUARDSLOT]]
+; CHECK-NEXT:    [[TMP1:%.*]] = icmp eq i8* [[STACKGUARD1]], [[TMP0]]
+; CHECK-NEXT:    br i1 [[TMP1]], label [[SP_RETURN:%.*]], label [[CALLSTACKCHECKFAILBLK:%.*]], !prof !0
+; CHECK:       SP_return:
+; CHECK-NEXT:    ret void
+; CHECK:       CallStackCheckFailBlk:
+; CHECK-NEXT:    call void @__stack_chk_fail()
+; CHECK-NEXT:    unreachable
+;
+entry:
+  %retval = alloca i32, align 4
+  %a = alloca i32, align 4
+  %j = alloca i32 addrspace(1)*, align 8
+  store i32 0, i32* %retval
+  %load = load i32, i32* %a, align 4
+  %add = add nsw i32 %load, 1
+  store i32 %add, i32* %a, align 4
+  %a.addrspacecast = addrspacecast i32* %a to i32 addrspace(1)*
+  store i32 addrspace(1)* %a.addrspacecast, i32 addrspace(1)** %j, align 8
+  ret void
+}
+
+define void @cmpxchg_captures() #0 {
+; CHECK-LABEL: @cmpxchg_captures(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[STACKGUARDSLOT:%.*]] = alloca i8*
+; CHECK-NEXT:    [[STACKGUARD:%.*]] = load volatile i8*, i8* addrspace(257)* inttoptr (i32 40 to i8* addrspace(257)*)
+; CHECK-NEXT:    call void @llvm.stackprotector(i8* [[STACKGUARD]], i8** [[STACKGUARDSLOT]])
+; CHECK-NEXT:    [[RETVAL:%.*]] = alloca i32, align 4
+; CHECK-NEXT:    [[A:%.*]] = alloca i32, align 4
+; CHECK-NEXT:    [[J:%.*]] = alloca i32*, align 8
+; CHECK-NEXT:    store i32 0, i32* [[RETVAL]]
+; CHECK-NEXT:    [[LOAD:%.*]] = load i32, i32* [[A]], align 4
+; CHECK-NEXT:    [[ADD:%.*]] = add nsw i32 [[LOAD]], 1
+; CHECK-NEXT:    store i32 [[ADD]], i32* [[A]], align 4
+; CHECK-NEXT:    [[TMP0:%.*]] = cmpxchg i32** [[J]], i32* [[A]], i32* null seq_cst monotonic
+; CHECK-NEXT:    [[STACKGUARD1:%.*]] = load volatile i8*, i8* addrspace(257)* inttoptr (i32 40 to i8* addrspace(257)*)
+; CHECK-NEXT:    [[TMP1:%.*]] = load volatile i8*, i8** [[STACKGUARDSLOT]]
+; CHECK-NEXT:    [[TMP2:%.*]] = icmp eq i8* [[STACKGUARD1]], [[TMP1]]
+; CHECK-NEXT:    br i1 [[TMP2]], label [[SP_RETURN:%.*]], label [[CALLSTACKCHECKFAILBLK:%.*]], !prof !0
+; CHECK:       SP_return:
+; CHECK-NEXT:    ret void
+; CHECK:       CallStackCheckFailBlk:
+; CHECK-NEXT:    call void @__stack_chk_fail()
+; CHECK-NEXT:    unreachable
+;
+entry:
+  %retval = alloca i32, align 4
+  %a = alloca i32, align 4
+  %j = alloca i32*, align 8
+  store i32 0, i32* %retval
+  %load = load i32, i32* %a, align 4
+  %add = add nsw i32 %load, 1
+  store i32 %add, i32* %a, align 4
+
+  cmpxchg i32** %j, i32* %a, i32* null seq_cst monotonic
+  ret void
+}
+
+attributes #0 = { sspstrong }

test/Transforms/StackProtector/X86/lit.local.cfg

+if not 'X86' in config.root.targets:
+    config.unsupported = True
+

tools/opt/opt.cpp

int main(int argc, char **argv) {
   initializeScalarizeMaskedMemIntrinPass(Registry);
   initializeCodeGenPreparePass(Registry);
   initializeAtomicExpandPass(Registry);
   initializeRewriteSymbolsLegacyPassPass(Registry);
   initializeWinEHPreparePass(Registry);
   initializeDwarfEHPreparePass(Registry);
   initializeSafeStackLegacyPassPass(Registry);
   initializeSjLjEHPreparePass(Registry);
+  initializeStackProtectorPass(Registry);
   initializePreISelIntrinsicLoweringLegacyPassPass(Registry);
   initializeGlobalMergePass(Registry);
   initializeIndirectBrExpandPassPass(Registry);
   initializeInterleavedLoadCombinePass(Registry);
   initializeInterleavedAccessPass(Registry);
   initializeEntryExitInstrumenterPass(Registry);
   initializePostInlineEntryExitInstrumenterPass(Registry);
   initializeUnreachableBlockElimLegacyPassPass(Registry);