[SSP] Remove llvm.stackprotectorcheck.

[timshen]

Summary

This is a cleanup patch for SSP support in LLVM. There is no functional change.
llvm.stackprotectorcheck is not needed, because SelectionDAG isn't
actually lowering it in SelectBasicBlock; rather, it adds check code in
FinishBasicBlock, ignoring the position where the intrinsic is inserted
(See FindSplitPointForStackProtector()).

[echristo]

I don't really understand why we need new intrinsics here. Can you explain the design a bit more? Can you also comment the intrinsics with what they do and how they're different from the existing ones?

Thanks!

-eric

[timshen]

tl;dr: because we have a million code paths. :)

The customization point in backends is the opcode LOAD_STACK_GUARD. It already exists for other reasons, but suitable for our purpose.

Previously we have two code paths to handle SSP, SelectionDAG path and IR path.

To customize stack guard loading in IR SSP, we need llvm.stackguardvalue. It lowers to LOAD_STACK_GUARD directly.

To customize stack guard loading in SelectionDAG, llvm.stackprotectorcheck has to take a value, so it doesn't have to be an address (e.g. could be a llvm.stackguardvalue call). To not break existing user code (it's documented) and tests, I created llvm.stackprotectorcheckvalue instead. llvm.stackprotectorcheck is not generated by LLVM anymore. See http://lists.llvm.org/pipermail/llvm-dev/2016-February/095760.html. We may want to delete llvm.stackprotectorcheck?

I haven't push all generic stack guard loading (typically loading the global variable __stack_chk_guard) to the backends. Once we push them all down, we can remove target-independent code path that deals with global variables. But we still need these two intrinsics for supporting both SelectionDAG and IR approach.

I added some comments for these two intrinsics.

[echristo]

Overall I think we're going to want a path that takes us through AutoUpgrade so that going forward we have a single path for all targets.

<insert about 30-45 minutes of discussion>

We're looking at a major design overhaul here, I think, so getting some notes down:

TLI->

lowerStackGuardStore
lowerStackGuardLoad
lowerStackCanaryLoad

We already have a guard node, do we need a separate SDag node for each of these?

Do we need this, effectively, duplicated 3x with SDag, FastIsel, Post-RA handling?

How do we keep the analysis information in the analysis pass and the rest of it in the IR? This would be the other side information like how to sort, strong, etc.

Need to continue to handle:

Coloring reuse
Sibcall optimizations
Avoid stack spill of canary values
Architecture neutral base case

and of course all of this with auto upgrade because I like my ponies to have unicorn horns.

[timshen]

I looked at LLVM manual and fortunately both llvm.stackprotector and
llvm.stackprotectorcheck is required to be passed in @__stack_chk_guard.
This limited flexibility allows us to directly change stackprotectorcheck's
signature and make an auto-upgrade for it.

In long term, I'd like to keep a diamond code path:

1. An IR pass that generates IR that calls llvm.stackprotector, llvm.stackprotectorcheck and llvm.experimental_stackguardvalue.
2. Either FastISel, or SelectionDAG handles the generated IR. Their shouldn't be an "IR approach" when FastISel isn't turned on (see the comment on TLI::supportsSelectionDAGSP).
3. Optional backend lowering is supported by overriding TLI::forceLoadStackGuardNode and TLI::getStackGuardAddr.

Notice that forceLoadStackGuardNode and getStackGuardAddr serves following
different purposes:

• Some backends want to lower the global variable manually, in such case forceLoadStackGuardNode should return true and getStackGuardAddr returns the global variable (search LOAD_STACK_GUARD in lib/Target).
• Other platforms, e.g. PowerPC 64 bit wants to lower the stack guard loading completely manually, getStackGuardAddr should returns nullptr.
• For the rest forceLoadStackGuardNode returns false and getStackGuardAddr returns the global variable.

[echristo]

Starting to look better, needs autoupgrade support and tests. I'm uncertain about putting one aspect of the set of intrinsics into experimental since the other two aren't already, but it's a simple change once we get the rest of it.

Can you add to the patch description a lot of the comments in the review? Also talk about the change of useLoadStackGuardNode to forceLoadStackGuardNode, etc.

Thanks!

-eric

[timshen]

As discussed, added documentation for StackProtector module. Also added a test case for auto-upgrade.

[echristo]

test/CodeGen/Generic/upgrade-stackprotectorcheck.ll

1 ↗

(On Diff #51614)

Should probably be in test/Assembler/auto_upgrade_intrinsics.ll or some such?

And you should check that the correct IR is generated at autoupgrade time.

[timshen]

Done. test/Bitcode seems to be a suitable place as well.

[echristo]

But there's already a large test that covers all sorts of intrinsics where I mentioned?

[timshen]

The last function created/touched in test/Assembler/auto_upgrade_intrinsics.ll is at 2013-10-07. This file has only three tests - seems not popular :).

What's the benefit of putting all test cases in one file?

[echristo]

For all of these? Because we're just parsing and disassembling, nothing complicated that'd be helped by multiple files and multiple process overhead.

[timshen]

Ah never mind, I guess test/Assembler/auto_upgrade_intrinsics.ll is a good place to hold target-independent intrinsic upgrades - there are actually not that many.

[echristo]

Cool. This is OK with me then.

Thanks for all of the hard work here.

[iteratee]

Some comments, mostly about comments and documentation.

include/llvm/IR/Intrinsics.td

329

I agree with eric that this shouldn't be experimental.

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

5357

I'm not sure I understand this comment.

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h

544–545

Is this still accurate? Are we keeping support for loading an address?

lib/CodeGen/StackProtector.cpp

331

I think this comment should read %1 = <load or intrinsic to read stack guard>

lib/Target/AArch64/AArch64ISelLowering.cpp

10104 ↗

(On Diff #51619)

Can you add a comment about why this needs to be overridden for this platform?

[timshen]

This comment was removed by timshen.

[timshen]

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

5357

Updated - simply forward the Value* to visitSPDescriptorParent, and inspect if it's a CallInst or a Load there.

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h

544–545

According to the documentation, we never support loading an arbitrary address. We only support loading @__stack_chk_guard. I would say in visitSPDescriptorParent, it doesn't have to check if the LoadInst is actually loading @__stack_chk_guard, because we are already doing more than that (loads @__guard_local on OpenBSD).

[timshen]

include/llvm/IR/Intrinsics.td

329

Why do you think removing experimental is safer? Isn't keeping it in any situation a safer move?

[timshen]

include/llvm/IR/Intrinsics.td

329

First safer -> better

[timshen]

Hi,

I realized that we don't really need llvm.stackguardcheck, so I try to come up with this patch as the first step of cleanup.

[timshen]

Also deleted documentation.

[iteratee]

Mostly looks good.

lib/CodeGen/StackProtector.cpp

346

Comment seems to belong to the instruction below. A comment about HasIRCheck would be nice too.

lib/Target/X86/X86ISelLowering.cpp

2179

The address space will go away in a later patch, correct?

[timshen]

Updated HasIRCheck comment.

lib/Target/X86/X86ISelLowering.cpp

2179

Yes, finally X86 should use LOAD_STACK_GUARD as well, but before that I'd like to introduce llvm.stackguard() intrinsic. After that point getStackGuardAddr isn't even needed.

[iteratee]

I have no more problems, and generally like your approach. It would be nice to get someone who has worked on this code before to sign off.

[timshen]

Hi Michael, since you wrote the SSP code (including stackprotectorcheck, according to git history), can you take a look at this cleanup?

Thanks!

[timshen]

Friendly ping. :)

[echristo]

Few inline comments and requests for more comments. Otherwise it's looking pretty nice.

Thanks!

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

1939

Typical llvm style is: assert((cond) && "Some explanatory text")

(This happens in other places in your patch, I'll just include the first one)

lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp

1361

Comment.

lib/CodeGen/StackProtector.cpp

202

Standard style for this would be no braces for any of the conditionals or block.

lib/CodeGen/TargetLoweringBase.cpp

1752

Block comments on all of these if there aren't any in the .h file please? (I prefer more detailed comments on the implementation, but still).

test/Assembler/auto_upgrade_intrinsics.ll

57

Hmm?

[echristo]

Oh, and long as you add the inline comments, fix the style stuff, etc, I think we're good to go.

-eric

test/Assembler/auto_upgrade_intrinsics.ll

57

Nevermind, I see it now. That's frustrating, can you add a comment that the declare is for the objectsize upgrade and why?

[timshen]

Updated patch.

[timshen]

Thanks for the style check! It's a big patch and I clearly missed those.

Also rebased to include D18632's change.

lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp

1361

As suggested by gottesmm, moved this condition into StackProtector::shouldEmitSDCheck(), and documented.

[timshen]

Updated patch for fixing a typo.

[timshen]

Hi eugenis, I changed the function name getStackCookieLocation to getIRStackGuard, since there are two SSP code paths and such a name seems to cause less confusion. Do you think it's a fine name?

[eugenis]

In D17736#395796, @timshen wrote:

Hi eugenis, I changed the function name getStackCookieLocation to getIRStackGuard, since there are two SSP code paths and such a name seems to cause less confusion. Do you think it's a fine name?

Absolutely.

include/llvm/Target/TargetLowering.h

1006–1015

This comment makes it sound like this function makes the decision to use SDAG vs IR stack protector.

[timshen]

Updated comment for getIRStackGuard.

include/llvm/Target/TargetLowering.h

1006–1015

Moved the comment to StackProtector::CreatePrologue. At that point I was not aware that other code will use this function.

[eugenis]

looks good

docs/LangRef.rst

 This intrinsic causes the prologue/epilogue inserter to force the position of
 the ``AllocaInst`` stack slot to be before local variables on the stack. This is
 to ensure that if a local variable on the stack is overwritten, it will destroy
 the value of the guard. When the function exits, the guard on the stack is
 checked against the original guard by ``llvm.stackprotectorcheck``. If they are
 different, then ``llvm.stackprotectorcheck`` causes the program to abort by
 calling the ``__stack_chk_fail()`` function.
 
-'``llvm.stackprotectorcheck``' Intrinsic
-^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-
-Syntax:
-"""""""
-
-::
-
-      declare void @llvm.stackprotectorcheck(i8** <guard>)
-
-Overview:
-"""""""""
-
-The ``llvm.stackprotectorcheck`` intrinsic compares ``guard`` against an already
-created stack protector and if they are not equal calls the
-``__stack_chk_fail()`` function.
-
-Arguments:
-""""""""""
-
-The ``llvm.stackprotectorcheck`` intrinsic requires one pointer argument, the
-the variable ``@__stack_chk_guard``.
-
-Semantics:
-""""""""""
-
-This intrinsic is provided to perform the stack protector check by comparing
-``guard`` with the stack slot created by ``llvm.stackprotector`` and if the
-values do not match call the ``__stack_chk_fail()`` function.
-
-The reason to provide this as an IR level intrinsic instead of implementing it
-via other IR operations is that in order to perform this operation at the IR
-level without an intrinsic, one would need to create additional basic blocks to
-handle the success/failure cases. This makes it difficult to stop the stack
-protector check from disrupting sibling tail calls in Codegen. With this
-intrinsic, we are able to generate the stack protector basic blocks late in
-codegen after the tail call decision has occurred.
-
 '``llvm.objectsize``' Intrinsic
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 Syntax:
 """""""
 
 ::
 

include/llvm/CodeGen/StackProtector.h

private:
   unsigned SSPBufferSize;
 
   /// 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.
   ///
   ///  - The prologue code loads and stores the stack guard onto the stack.
   ///  - The epilogue checks the value stored in the prologue against the
   ///    original value. It calls __stack_chk_fail if they differ.
   bool InsertStackProtectors();
 
public:
   void getAnalysisUsage(AnalysisUsage &AU) const override {
     AU.addPreserved<DominatorTreeWrapperPass>();
   }
 
   SSPLayoutKind getSSPLayout(const AllocaInst *AI) const;
   void adjustForColoring(const AllocaInst *From, const AllocaInst *To);
 
   bool runOnFunction(Function &Fn) override;
+
+  bool hasPrologue() const { return HasPrologue; }
+
+  bool hasIRCheck() const { return HasIRCheck; }
 };
 } // end namespace llvm
 
 #endif // LLVM_CODEGEN_STACKPROTECTOR_H

include/llvm/IR/Intrinsics.td

 
 // The assume intrinsic is marked as arbitrarily writing so that proper
 // control dependencies will be maintained.
 def int_assume        : Intrinsic<[], [llvm_i1_ty], []>;
 
 // Stack Protector Intrinsic - The stackprotector intrinsic writes the stack
 // guard to the correct place on the stack frame.
 def int_stackprotector : Intrinsic<[], [llvm_ptr_ty, llvm_ptrptr_ty], []>;
-def int_stackprotectorcheck : Intrinsic<[], [llvm_ptrptr_ty],
-                                        [IntrReadWriteArgMem]>;
 
 // A counter increment for instrumentation based profiling.
 def int_instrprof_increment : Intrinsic<[],
                                         [llvm_ptr_ty, llvm_i64_ty,
                                          llvm_i32_ty, llvm_i32_ty],
                                         []>;
 
 // A call to profile runtime for value profiling of target expressions
 // through instrumentation based profiling.
 def int_instrprof_value_profile : Intrinsic<[],
                                             [llvm_ptr_ty, llvm_i64_ty,

include/llvm/Target/TargetLowering.h

unsigned getPrefFunctionAlignment() const {
     return PrefFunctionAlignment;
   }
 
   /// Return the preferred loop alignment.
   virtual unsigned getPrefLoopAlignment(MachineLoop *ML = nullptr) const {
     return PrefLoopAlignment;
   }
 
-  /// Return true if the target stores stack protector cookies at a fixed offset
-  /// in some non-standard address space, and populates the address space and
-  /// offset as appropriate.
-  virtual bool getStackCookieLocation(unsigned &/*AddressSpace*/,
-                                      unsigned &/*Offset*/) const {
-    return false;
-  }
+  // Return whether target supports SelectionDAG stack protection.
+  // TODO: Remove this. SSP should always be supported in SelectionDAG.
+  virtual bool supportsSelectionDAGSP() const;
+
+  /// Inserts necessary declarations for SSP purpose.
+  virtual void insertSSPDeclarations(Module &M) const;
+
+  /// Return the variable that's previously inserted by insertSSPDeclarations,
+  /// if any, otherwise return nullptr.
+  virtual Value *getStackGuardAddr(const Module &M) const;
 
   /// If the target has a standard location for the unsafe stack pointer,
   /// returns the address of that location. Otherwise, returns nullptr.
   virtual Value *getSafeStackPointerLocation(IRBuilder<> &IRB) const;
 
   /// Returns true if a cast between SrcAS and DestAS is a noop.
   virtual bool isNoopAddrSpaceCast(unsigned SrcAS, unsigned DestAS) const {
     return false;

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h

private:
   ///        block into the success basic block. Then we code-gen a new tail for
   ///        the parent basic block consisting of the two loads, the comparison,
   ///        and finally two branches to the success/failure basic blocks. We
   ///        conclude by code-gening the failure basic block if we have not
   ///        code-gened it already (all stack protector checks we generate in
   ///        the same function, use the same failure basic block).
   class StackProtectorDescriptor {
   public:
-    StackProtectorDescriptor() : ParentMBB(nullptr), SuccessMBB(nullptr),
-                                 FailureMBB(nullptr), Guard(nullptr),
-                                 GuardReg(0) { }
+    StackProtectorDescriptor()
+        : ParentMBB(nullptr), SuccessMBB(nullptr), FailureMBB(nullptr),
+          GuardReg(0) {}
 
     /// Returns true if all fields of the stack protector descriptor are
     /// initialized implying that we should/are ready to emit a stack protector.
     bool shouldEmitStackProtector() const {
-      return ParentMBB && SuccessMBB && FailureMBB && Guard;
+      return ParentMBB && SuccessMBB && FailureMBB;
     }
 
     /// Initialize the stack protector descriptor structure for a new basic
     /// block.
-    void initialize(const BasicBlock *BB,
-                    MachineBasicBlock *MBB,
-                    const CallInst &StackProtCheckCall) {
+    void initialize(const BasicBlock *BB, MachineBasicBlock *MBB) {
       // Make sure we are not initialized yet.
       assert(!shouldEmitStackProtector() && "Stack Protector Descriptor is "
              "already initialized!");
       ParentMBB = MBB;
       SuccessMBB = AddSuccessorMBB(BB, MBB, /* IsLikely */ true);
       FailureMBB = AddSuccessorMBB(BB, MBB, /* IsLikely */ false, FailureMBB);
-      if (!Guard)
-        Guard = StackProtCheckCall.getArgOperand(0);
     }
 
     /// Reset state that changes when we handle different basic blocks.
     ///
     /// This currently includes:
     ///
     /// 1. The specific basic block we are generating a
     /// stack protector for (ParentMBB).
public:
     ///
     /// 1. FailureMBB since we reuse the failure code path for all stack
     /// protector checks created in an individual function.
     ///
     /// 2.The guard variable since the guard variable we are checking against is
     /// always the same.
     void resetPerFunctionState() {
       FailureMBB = nullptr;
-      Guard = nullptr;
       GuardReg = 0;
     }
 
     MachineBasicBlock *getParentMBB() { return ParentMBB; }
     MachineBasicBlock *getSuccessMBB() { return SuccessMBB; }
     MachineBasicBlock *getFailureMBB() { return FailureMBB; }
-    const Value *getGuard() { return Guard; }
 
     unsigned getGuardReg() const { return GuardReg; }
     void setGuardReg(unsigned R) { GuardReg = R; }
 
   private:
     /// The basic block for which we are generating the stack protector.
     ///
     /// As a result of stack protector generation, we will splice the
     /// terminators of this basic block into the successor mbb SuccessMBB and
     /// replace it with a compare/branch to the successor mbbs
     /// SuccessMBB/FailureMBB depending on whether or not the stack protector
     /// was violated.
     MachineBasicBlock *ParentMBB;
 
     /// A basic block visited on stack protector check success that contains the
     /// terminators of ParentMBB.
     MachineBasicBlock *SuccessMBB;
 
     /// This basic block visited on stack protector check failure that will
     /// contain a call to __stack_chk_fail().
     MachineBasicBlock *FailureMBB;
 
-    /// The guard variable which we will compare against the stored value in the
-    /// stack protector stack slot.
-    const Value *Guard;
-
     /// The virtual register holding the stack guard value.
     unsigned GuardReg;
 
     /// Add a successor machine basic block to ParentMBB. If the successor mbb
     /// has not been created yet (i.e. if SuccMBB = 0), then the machine basic
     /// block will be created. Assign a large weight if IsLikely is true.
     MachineBasicBlock *AddSuccessorMBB(const BasicBlock *BB,
                                        MachineBasicBlock *ParentMBB,
                                        bool IsLikely,

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

void SelectionDAGBuilder::visitSPDescriptorParent(StackProtectorDescriptor &SPD,
 
   // First create the loads to the guard/stack slot for the comparison.
   const TargetLowering &TLI = DAG.getTargetLoweringInfo();
   EVT PtrTy = TLI.getPointerTy(DAG.getDataLayout());
 
   MachineFrameInfo *MFI = ParentBB->getParent()->getFrameInfo();
   int FI = MFI->getStackProtectorIndex();
 
-  const Value *IRGuard = SPD.getGuard();
+  const Module &M = *ParentBB->getParent()->getFunction()->getParent();
+  const Value *IRGuard = TLI.getStackGuardAddr(M);
+  assert(IRGuard);
   SDValue GuardPtr = getValue(IRGuard);
   SDValue StackSlotPtr = DAG.getFrameIndex(FI, PtrTy);
 
   unsigned Align = DL->getPrefTypeAlignment(IRGuard->getType());
 
   SDValue Guard;
   SDLoc dl = getCurSDLoc();
 
SelectionDAGBuilder::visitIntrinsicCall(const CallInst &I, unsigned Intrinsic) {
   }
   case Intrinsic::invariant_start:
     // Discard region information.
     setValue(&I, DAG.getUNDEF(TLI.getPointerTy(DAG.getDataLayout())));
     return nullptr;
   case Intrinsic::invariant_end:
     // Discard region information.
     return nullptr;
-  case Intrinsic::stackprotectorcheck: {
-    // Do not actually emit anything for this basic block. Instead we initialize
-    // the stack protector descriptor and export the guard variable so we can
-    // access it in FinishBasicBlock.
-    const BasicBlock *BB = I.getParent();
-    SPDescriptor.initialize(BB, FuncInfo.MBBMap[BB], I);
-    ExportFromCurrentBlock(SPDescriptor.getGuard());
-
-    // Flush our exports since we are going to process a terminator.
-    (void)getControlRoot();
-    return nullptr;
-  }
   case Intrinsic::clear_cache:
     return TLI.getClearCacheBuiltinName();
   case Intrinsic::donothing:
     // ignore
     return nullptr;
   case Intrinsic::experimental_stackmap: {
     visitStackmap(I);
     return nullptr;
   }
   case Intrinsic::experimental_patchpoint_void:
   case Intrinsic::experimental_patchpoint_i64: {

lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp

 //===-- SelectionDAGISel.cpp - Implement the SelectionDAGISel class -------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // This implements the SelectionDAGISel class.
 //
 //===----------------------------------------------------------------------===//
 
-#include "llvm/CodeGen/GCStrategy.h"
+#include "llvm/CodeGen/SelectionDAG.h"
 #include "ScheduleDAGSDNodes.h"
 #include "SelectionDAGBuilder.h"
 #include "llvm/ADT/PostOrderIterator.h"
 #include "llvm/ADT/Statistic.h"
 #include "llvm/Analysis/AliasAnalysis.h"
 #include "llvm/Analysis/BranchProbabilityInfo.h"
 #include "llvm/Analysis/CFG.h"
 #include "llvm/Analysis/EHPersonalities.h"
 #include "llvm/Analysis/TargetLibraryInfo.h"
 #include "llvm/CodeGen/Analysis.h"
 #include "llvm/CodeGen/FastISel.h"
 #include "llvm/CodeGen/FunctionLoweringInfo.h"
 #include "llvm/CodeGen/GCMetadata.h"
+#include "llvm/CodeGen/GCStrategy.h"
 #include "llvm/CodeGen/MachineFrameInfo.h"
 #include "llvm/CodeGen/MachineFunction.h"
 #include "llvm/CodeGen/MachineInstrBuilder.h"
 #include "llvm/CodeGen/MachineModuleInfo.h"
 #include "llvm/CodeGen/MachineRegisterInfo.h"
 #include "llvm/CodeGen/ScheduleHazardRecognizer.h"
 #include "llvm/CodeGen/SchedulerRegistry.h"
-#include "llvm/CodeGen/SelectionDAG.h"
 #include "llvm/CodeGen/SelectionDAGISel.h"
+#include "llvm/CodeGen/StackProtector.h"
 #include "llvm/CodeGen/WinEHFuncInfo.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/DebugInfo.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/InlineAsm.h"
 #include "llvm/IR/Instructions.h"
 #include "llvm/IR/IntrinsicInst.h"
 #include "llvm/IR/Intrinsics.h"
SelectionDAGISel::~SelectionDAGISel() {
   delete SDB;
   delete CurDAG;
   delete FuncInfo;
 }
 
 void SelectionDAGISel::getAnalysisUsage(AnalysisUsage &AU) const {
   AU.addRequired<AAResultsWrapperPass>();
   AU.addRequired<GCModuleInfo>();
+  AU.addRequired<StackProtector>();
+  AU.addPreserved<StackProtector>();
   AU.addPreserved<GCModuleInfo>();
   AU.addRequired<TargetLibraryInfoWrapperPass>();
   if (UseMBPI && OptLevel != CodeGenOpt::None)
     AU.addRequired<BranchProbabilityInfoWrapperPass>();
   MachineFunctionPass::getAnalysisUsage(AU);
 }
 
 /// SplitCriticalSideEffectEdges - Look for critical edges with a PHI value that
#endif
       FastIS->recomputeInsertPt();
     } else {
       // Lower any arguments needed in this block if this is the entry block.
       if (LLVMBB == &Fn.getEntryBlock()) {
         ++NumEntryBlocks;
         LowerArguments(Fn);
       }
     }
+    auto &SP = getAnalysis<StackProtector>();
+    if (dyn_cast<ReturnInst>(LLVMBB->getTerminator()) && SP.hasPrologue() &&
+        !SP.hasIRCheck()) {
+      SDB->SPDescriptor.initialize(LLVMBB, FuncInfo->MBBMap[LLVMBB]);
+    }
 
     if (Begin != BI)
       ++NumDAGBlocks;
     else
       ++NumFastIselBlocks;
 
     if (Begin != BI) {
       // Run SelectionDAG instruction selection on the remainder of the block

lib/CodeGen/StackProtector.cpp

 using namespace llvm;
 
 #define DEBUG_TYPE "stack-protector"
 
 STATISTIC(NumFunProtected, "Number of functions protected");
 STATISTIC(NumAddrTaken, "Number of local variables that have their address"
                         " taken.");
 
-static cl::opt<bool> EnableSelectionDAGSP("enable-selectiondag-sp",
-                                          cl::init(true), cl::Hidden);
-
 char StackProtector::ID = 0;
 INITIALIZE_PASS(StackProtector, "stack-protector", "Insert stack protectors",
                 false, true)
 
 FunctionPass *llvm::createStackProtectorPass(const TargetMachine *TM) {
   return new StackProtector(TM);
 }
 

 
 bool StackProtector::runOnFunction(Function &Fn) {
   F = &Fn;
   M = F->getParent();
   DominatorTreeWrapperPass *DTWP =
       getAnalysisIfAvailable<DominatorTreeWrapperPass>();
   DT = DTWP ? &DTWP->getDomTree() : nullptr;
   TLI = TM->getSubtargetImpl(Fn)->getTargetLowering();
+  HasPrologue = false;
+  HasIRCheck = false;
 
   Attribute Attr = Fn.getFnAttribute("stack-protector-buffer-size");
   if (Attr.isStringAttribute() &&
       Attr.getValueAsString().getAsInteger(10, SSPBufferSize))
       return false; // Invalid integer string
 
   if (!RequiresStackProtector())
     return false;

 /// strong heuristic will add a guard variables to functions that call alloca
 /// regardless of size, functions with any buffer regardless of type and size,
 /// functions with aggregates that contain any buffer regardless of type and
 /// size, and functions that contain stack-based variables that have had their
 /// address taken.
 bool StackProtector::RequiresStackProtector() {
   bool Strong = false;
   bool NeedsProtector = false;
+  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)) {
+          HasPrologue = true;
+        }
+      }
+    }
+  }
   if (F->hasFnAttribute(Attribute::StackProtectReq)) {
     NeedsProtector = true;
     Strong = true; // Use the same heuristic as strong to determine SSPLayout
   } else if (F->hasFnAttribute(Attribute::StackProtectStrong))
     Strong = true;
+  else if (HasPrologue)
+    NeedsProtector = true;
   else if (!F->hasFnAttribute(Attribute::StackProtect))
     return false;
 
   for (const BasicBlock &BB : *F) {
     for (const Instruction &I : BB) {
       if (const AllocaInst *AI = dyn_cast<AllocaInst>(&I)) {
         if (AI->isArrayAllocation()) {
           // SSP-Strong: Enable protectors for any call to alloca, regardless
for (const Instruction &I : BB) {
         }
       }
     }
   }
 
   return NeedsProtector;
 }
 
-static bool InstructionWillNotHaveChain(const Instruction *I) {
-  return !I->mayHaveSideEffects() && !I->mayReadFromMemory() &&
-         isSafeToSpeculativelyExecute(I);
-}
-
-/// Identify if RI has a previous instruction in the "Tail Position" and return
-/// it. Otherwise return 0.
-///
-/// This is based off of the code in llvm::isInTailCallPosition. The difference
-/// is that it inverts the first part of llvm::isInTailCallPosition since
-/// isInTailCallPosition is checking if a call is in a tail call position, and
-/// we are searching for an unknown tail call that might be in the tail call
-/// position. Once we find the call though, the code uses the same refactored
-/// code, returnTypeIsEligibleForTailCall.
-static CallInst *FindPotentialTailCall(BasicBlock *BB, ReturnInst *RI,
-                                       const TargetLoweringBase *TLI) {
-  // Establish a reasonable upper bound on the maximum amount of instructions we
-  // will look through to find a tail call.
-  unsigned SearchCounter = 0;
-  const unsigned MaxSearch = 4;
-  bool NoInterposingChain = true;
-
-  for (BasicBlock::reverse_iterator I = std::next(BB->rbegin()), E = BB->rend();
-       I != E && SearchCounter < MaxSearch; ++I) {
-    Instruction *Inst = &*I;
-
-    // Skip over debug intrinsics and do not allow them to affect our MaxSearch
-    // counter.
-    if (isa<DbgInfoIntrinsic>(Inst))
-      continue;
-
-    // If we find a call and the following conditions are satisifed, then we
-    // have found a tail call that satisfies at least the target independent
-    // requirements of a tail call:
-    //
-    // 1. The call site has the tail marker.
-    //
-    // 2. The call site either will not cause the creation of a chain or if a
-    // chain is necessary there are no instructions in between the callsite and
-    // the call which would create an interposing chain.
-    //
-    // 3. The return type of the function does not impede tail call
-    // optimization.
-    if (CallInst *CI = dyn_cast<CallInst>(Inst)) {
-      if (CI->isTailCall() &&
-          (InstructionWillNotHaveChain(CI) || NoInterposingChain) &&
-          returnTypeIsEligibleForTailCall(BB->getParent(), CI, RI, *TLI))
-        return CI;
-    }
-
-    // If we did not find a call see if we have an instruction that may create
-    // an interposing chain.
-    NoInterposingChain =
-        NoInterposingChain && InstructionWillNotHaveChain(Inst);
-
-    // Increment max search.
-    SearchCounter++;
-  }
-
-  return nullptr;
-}
-
 /// Insert code into the entry block that stores the __stack_chk_guard
 /// variable onto the stack:
 ///
 ///   entry:
 ///     StackGuardSlot = alloca i8*
 ///     StackGuard = load __stack_chk_guard
 ///     call void @llvm.stackprotect.create(StackGuard, StackGuardSlot)
 ///
 /// Returns true if the platform/triple supports the stackprotectorcreate pseudo
 /// node.
-static bool CreatePrologue(Function *F, Module *M, ReturnInst *RI,
-                           const TargetLoweringBase *TLI, const Triple &TT,
-                           AllocaInst *&AI, Value *&StackGuardVar) {
-  bool SupportsSelectionDAGSP = false;
-  PointerType *PtrTy = Type::getInt8PtrTy(RI->getContext());
-  unsigned AddressSpace, Offset;
-  if (TLI->getStackCookieLocation(AddressSpace, Offset)) {
-    Constant *OffsetVal =
-        ConstantInt::get(Type::getInt32Ty(RI->getContext()), Offset);
-
-    StackGuardVar =
-        ConstantExpr::getIntToPtr(OffsetVal, PointerType::get(PtrTy,
-                                                              AddressSpace));
-  } else if (TT.isOSOpenBSD()) {
-    StackGuardVar = M->getOrInsertGlobal("__guard_local", PtrTy);
-    cast<GlobalValue>(StackGuardVar)
-        ->setVisibility(GlobalValue::HiddenVisibility);
-  } else {
-    SupportsSelectionDAGSP = true;
-    StackGuardVar = M->getOrInsertGlobal("__stack_chk_guard", PtrTy);
-  }
+static void CreatePrologue(Function *F, Module *M, ReturnInst *RI,
+                           const TargetLoweringBase *TLI, AllocaInst *&AI,
+                           Value *&StackGuardVar) {
+  TLI->insertSSPDeclarations(*M);
+  StackGuardVar = TLI->getStackGuardAddr(*M);
+  assert(StackGuardVar);
 
+  PointerType *PtrTy = Type::getInt8PtrTy(RI->getContext());
   IRBuilder<> B(&F->getEntryBlock().front());
   AI = B.CreateAlloca(PtrTy, nullptr, "StackGuardSlot");
   LoadInst *LI = B.CreateLoad(StackGuardVar, "StackGuard");
   B.CreateCall(Intrinsic::getDeclaration(M, Intrinsic::stackprotector),
                {LI, AI});
-
-  return SupportsSelectionDAGSP;
 }
 
 /// InsertStackProtectors - Insert code into the prologue and epilogue of the
 /// function.
 ///
 ///  - The prologue code loads and stores the stack guard onto the stack.
 ///  - The epilogue checks the value stored in the prologue against the original
 ///    value. It calls __stack_chk_fail if they differ.
 bool StackProtector::InsertStackProtectors() {
-  bool HasPrologue = false;
-  bool SupportsSelectionDAGSP =
-      EnableSelectionDAGSP && !TM->Options.EnableFastISel;
   AllocaInst *AI = nullptr;       // Place on stack that stores the stack guard.
   Value *StackGuardVar = nullptr; // The stack guard variable.
 
   for (Function::iterator I = F->begin(), E = F->end(); I != E;) {
     BasicBlock *BB = &*I++;
     ReturnInst *RI = dyn_cast<ReturnInst>(BB->getTerminator());
     if (!RI)
       continue;
 
     if (!HasPrologue) {
       HasPrologue = true;
-      SupportsSelectionDAGSP &=
-          CreatePrologue(F, M, RI, TLI, Trip, AI, StackGuardVar);
-    }
-
-    if (SupportsSelectionDAGSP) {
-      // Since we have a potential tail call, insert the special stack check
-      // intrinsic.
-      Instruction *InsertionPt = nullptr;
-      if (CallInst *CI = FindPotentialTailCall(BB, RI, TLI)) {
-        InsertionPt = CI;
-      } else {
-        InsertionPt = RI;
-        // At this point we know that BB has a return statement so it *DOES*
-        // have a terminator.
-        assert(InsertionPt != nullptr &&
-               "BB must have a terminator instruction at this point.");
-      }
+      CreatePrologue(F, M, RI, TLI, AI, StackGuardVar);
 
-      Function *Intrinsic =
-          Intrinsic::getDeclaration(M, Intrinsic::stackprotectorcheck);
-      CallInst::Create(Intrinsic, StackGuardVar, "", InsertionPt);
-    } else {
+    if (!TLI->supportsSelectionDAGSP()) {
       // If we do not support SelectionDAG based tail calls, generate IR level
       // tail calls.
       //
       // For each block with a return instruction, convert this:
       //
       //   return:
       //     ...
       //     ret ...
       //
       // into this:
       //
       //   return:
       //     ...
       //     %1 = load __stack_chk_guard
       //     %2 = load StackGuardSlot
       //     %3 = cmp i1 %1, %2
       //     br i1 %3, label %SP_return, label %CallStackCheckFailBlk
       //
       //   SP_return:
       //     ret ...
       //
       //   CallStackCheckFailBlk:
       //     call void @__stack_chk_fail()
       //     unreachable
 
       // Create the FailBB. We duplicate the BB every time since the MI tail
       // merge pass will merge together all of the various BB into one including
       // fail BB generated by the stack protector pseudo instruction.
       BasicBlock *FailBB = CreateFailBB();
 
+      // Set HasIRCheck to true, so that SelectionDAG will not generate its own
+      // version.
+      HasIRCheck = true;
+
       // Split the basic block before the return instruction.
       BasicBlock *NewBB = BB->splitBasicBlock(RI->getIterator(), "SP_return");
 
       // Update the dominator tree if we need to.
       if (DT && DT->isReachableFromEntry(BB)) {
         DT->addNewBlock(NewBB, BB);
         DT->addNewBlock(FailBB, BB);
       }

lib/CodeGen/TargetLoweringBase.cpp

 #include <cctype>
 using namespace llvm;
 
 static cl::opt<bool> JumpIsExpensiveOverride(
     "jump-is-expensive", cl::init(false),
     cl::desc("Do not create extra branches to split comparison logic."),
     cl::Hidden);
 
+static cl::opt<bool> EnableSelectionDAGSP("enable-selectiondag-sp",
+                                          cl::init(true), cl::Hidden);
+
 /// InitLibcallNames - Set default libcall names.
 ///
 static void InitLibcallNames(const char **Names, const Triple &TT) {
   Names[RTLIB::SHL_I16] = "__ashlhi3";
   Names[RTLIB::SHL_I32] = "__ashlsi3";
   Names[RTLIB::SHL_I64] = "__ashldi3";
   Names[RTLIB::SHL_I128] = "__ashlti3";
   Names[RTLIB::SRL_I16] = "__lshrhi3";
case 2:
     // Allow 2*r as r+r.
     break;
   default: // Don't allow n * r
     return false;
   }
 
   return true;
 }
+
+bool TargetLoweringBase::supportsSelectionDAGSP() const {
+  return EnableSelectionDAGSP && !getTargetMachine().Options.EnableFastISel &&
+         !getTargetMachine().getTargetTriple().isOSOpenBSD();
+}
+
+void TargetLoweringBase::insertSSPDeclarations(Module &M) const {
+  PointerType *PtrTy = Type::getInt8PtrTy(M.getContext());
+  if (getTargetMachine().getTargetTriple().isOSOpenBSD()) {
+    cast<GlobalValue>(M.getOrInsertGlobal("__guard_local", PtrTy))
+        ->setVisibility(GlobalValue::HiddenVisibility);
+  } else {
+    M.getOrInsertGlobal("__stack_chk_guard", PtrTy);
+  }
+}
+
+Value *TargetLoweringBase::getStackGuardAddr(const Module &M) const {
+  if (getTargetMachine().getTargetTriple().isOSOpenBSD()) {
+    return M.getGlobalVariable("__guard_local");
+  }
+  return M.getGlobalVariable("__stack_chk_guard");
+}

lib/IR/AutoUpgrade.cpp

if (F->arg_size() == 2 && Name.startswith("objectsize.")) {
         F->setName(Name + ".old");
         NewFn = Intrinsic::getDeclaration(F->getParent(),
                                           Intrinsic::objectsize, Tys);
         return true;
       }
     }
     break;
 
+  case 's':
+    if (Name == "stackprotectorcheck") {
+      NewFn = nullptr;
+      return true;
+    }
+
   case 'x': {
     if (Name.startswith("x86.sse2.pcmpeq.") ||
         Name.startswith("x86.sse2.pcmpgt.") ||
         Name.startswith("x86.avx2.pcmpeq.") ||
         Name.startswith("x86.avx2.pcmpgt.") ||
         Name.startswith("x86.avx2.vbroadcast") ||
         Name.startswith("x86.avx2.pbroadcast") ||
         Name.startswith("x86.avx.vpermil.") ||
if (Name.startswith("llvm.x86.sse2.pcmpeq.") ||
       SmallVector<Constant*, 4> Idxs(NumElts);
       for (unsigned i = 0; i != NumElts; ++i) {
         unsigned Idx = Imm ? (i + NumElts) : i;
         Idxs[i] = Builder.getInt32(Idx);
       }
 
       Value *UndefV = UndefValue::get(Op0->getType());
       Rep = Builder.CreateShuffleVector(Op0, UndefV, ConstantVector::get(Idxs));
+    } else if (Name == "llvm.stackprotectorcheck") {
+      Rep = nullptr;
     } else {
       bool PD128 = false, PD256 = false, PS128 = false, PS256 = false;
       if (Name == "llvm.x86.avx.vpermil.pd.256")
         PD256 = true;
       else if (Name == "llvm.x86.avx.vpermil.pd")
         PD128 = true;
       else if (Name == "llvm.x86.avx.vpermil.ps.256")
         PS256 = true;
if (Name.startswith("llvm.x86.sse2.pcmpeq.") ||
           llvm_unreachable("Unexpected function");
 
         Rep = Builder.CreateShuffleVector(Op0, Op0, ConstantVector::get(Idxs));
       } else {
         llvm_unreachable("Unknown function for CallInst upgrade.");
       }
     }
 
+    if (Rep)
       CI->replaceAllUsesWith(Rep);
     CI->eraseFromParent();
     return;
   }
 
   std::string Name = CI->getName();
   if (!Name.empty())
     CI->setName(Name + ".old");
 

lib/Target/X86/X86ISelLowering.h

public:
 
     virtual bool needsFixedCatchObjects() const override;
 
     /// This method returns a target specific FastISel object,
     /// or null if the target does not support "fast" ISel.
     FastISel *createFastISel(FunctionLoweringInfo &funcInfo,
                              const TargetLibraryInfo *libInfo) const override;
 
-    /// Return true if the target stores stack protector cookies at a fixed
-    /// offset in some non-standard address space, and populates the address
-    /// space and offset as appropriate.
-    bool getStackCookieLocation(unsigned &AddressSpace,
-                                unsigned &Offset) const override;
+    bool supportsSelectionDAGSP() const override;
+
+    void insertSSPDeclarations(Module &M) const override;
+
+    Value *getStackGuardAddr(const Module &M) const override;
 
     /// Return true if the target stores SafeStack pointer at a fixed offset in
     /// some non-standard address space, and populates the address space and
     /// offset as appropriate.
     Value *getSafeStackPointerLocation(IRBuilder<> &IRB) const override;
 
     SDValue BuildFILD(SDValue Op, EVT SrcVT, SDValue Chain, SDValue StackSlot,
                       SelectionDAG &DAG) const;

lib/Target/X86/X86ISelLowering.cpp

 }
 
 unsigned X86TargetLowering::getAddressSpace() const {
   if (Subtarget.is64Bit())
     return (getTargetMachine().getCodeModel() == CodeModel::Kernel) ? 256 : 257;
   return 256;
 }
 
-bool X86TargetLowering::getStackCookieLocation(unsigned &AddressSpace,
-                                               unsigned &Offset) const {
+bool X86TargetLowering::supportsSelectionDAGSP() const {
+  return !Subtarget.isTargetLinux() && TargetLowering::supportsSelectionDAGSP();
+}
+
+void X86TargetLowering::insertSSPDeclarations(Module &M) const {
   if (!Subtarget.isTargetLinux())
-    return false;
+    TargetLowering::insertSSPDeclarations(M);
+}
+
+Value *X86TargetLowering::getStackGuardAddr(const Module &M) const {
+  if (!Subtarget.isTargetLinux())
+    return TargetLowering::getStackGuardAddr(M);
 
   // %fs:0x28, unless we're using a Kernel code model, in which case it's %gs:
   // %gs:0x14 on i386
-  Offset = (Subtarget.is64Bit()) ? 0x28 : 0x14;
-  AddressSpace = getAddressSpace();
-  return true;
+  unsigned Offset = (Subtarget.is64Bit()) ? 0x28 : 0x14;
+  unsigned AddressSpace = getAddressSpace();
+  PointerType *PtrTy = Type::getInt8PtrTy(M.getContext());
+  Constant *OffsetVal =
+      ConstantInt::get(Type::getInt32Ty(M.getContext()), Offset);
+  return ConstantExpr::getIntToPtr(OffsetVal,
+                                   PointerType::get(PtrTy, AddressSpace));
 }
 
 Value *X86TargetLowering::getSafeStackPointerLocation(IRBuilder<> &IRB) const {
   if (!Subtarget.isTargetAndroid())
     return TargetLowering::getSafeStackPointerLocation(IRB);
 
   // Android provides a fixed TLS slot for the SafeStack pointer. See the
   // definition of TLS_SLOT_SAFESTACK in

test/Assembler/auto_upgrade_intrinsics.ll

 }
 
 
 @a = private global [60 x i8] zeroinitializer, align 1
 
 define i32 @test.objectsize() {
 ; CHECK-LABEL: @test.objectsize(
 ; CHECK: @llvm.objectsize.i32.p0i8
-; CHECK-DAG: declare i32 @llvm.objectsize.i32.p0i8
   %s = call i32 @llvm.objectsize.i32(i8* getelementptr inbounds ([60 x i8], [60 x i8]* @a, i32 0, i32 0), i1 false)
   ret i32 %s
 }
+
+@__stack_chk_guard = external global i8*
+declare void @llvm.stackprotectorcheck(i8**)
+
+define void @test.stackprotectorcheck() {
+; CHECK-LABEL: @test.stackprotectorcheck(
+; CHECK-NEXT: ret void
+  call void @llvm.stackprotectorcheck(i8** @__stack_chk_guard)
+  ret void
+}
+
+; CHECK: declare i32 @llvm.objectsize.i32.p0i8