[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

5380

I'm not sure I understand this comment.

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h

550–551

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

lib/CodeGen/StackProtector.cpp

431

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

lib/Target/AArch64/AArch64ISelLowering.cpp

10104–10106

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

5380

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

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h

550–551

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

446

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

lib/Target/X86/X86ISelLowering.cpp

2154

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

[timshen]

Updated HasIRCheck comment.

lib/Target/X86/X86ISelLowering.cpp

2154

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 ↗

(On Diff #51844)

Comment.

lib/CodeGen/StackProtector.cpp

229

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

lib/CodeGen/TargetLoweringBase.cpp

1749

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 ↗

(On Diff #51844)

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

1012

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

1012

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

 '``llvm.stackprotectorcheck``' Intrinsic
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 Syntax:
 """""""
 
 ::
 
-      declare void @llvm.stackprotectorcheck(i8** <guard>)
+      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.
 

 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.experimental_stackguardvalue``' Intrinsic
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+Syntax:
+"""""""
+
+::
+
+      declare i8* @llvm.experimental_stackguardvalue()
+
+Overview:
+"""""""""
+
+The ``llvm.experimental_stackguardvalue`` intrinsic. It is only supported by
+platforms that do not have an exported symbol as a stack guard.
+
+Arguments:
+""""""""""
+
+None.
+
+Semantics:
+""""""""""
+
+The ``llvm.experimental_stackguardvalue`` intrinsic returns current stack guard.
+
 '``llvm.objectsize``' Intrinsic
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 Syntax:
 """""""
 
 ::
 

include/llvm/IR/Intrinsics.td

 def int_readcyclecounter : Intrinsic<[llvm_i64_ty]>;
 
 // 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.
+//
+// stackprotectorcheck is only used when -enable-selectiondag-sp is on and
+// target supports SSP in SelectionDAG. Otherwise SSP pass will generate IR
+// level code, including experimental_stackguardvalue calls. Backends may lower
+// experimental_stackguardvalue by overriding getStackGuardAddr and implementing
+// LOAD_STACK_GUARD pseudo instruction.
 def int_stackprotector : Intrinsic<[], [llvm_ptr_ty, llvm_ptrptr_ty], []>;
-def int_stackprotectorcheck : Intrinsic<[], [llvm_ptrptr_ty],
+def int_stackprotectorcheck : Intrinsic<[], [llvm_ptr_ty],
                                         [IntrReadWriteArgMem]>;
+def int_experimental_stackguardvalue : Intrinsic<[llvm_ptr_ty], [], []>;
 
 // 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

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 the target supports selection dag based stack protector.
+  /// TODO: Adjust visitSPDescriptorFailure in SelectionDAGBuilder so that
+  /// SelectionDAG Stack Protector is always supported. Then remove this
+  /// function and possibly the flag -enable-selectiondag-sp.
+  virtual bool supportsSelectionDAGSP() const {
+    return !getTargetMachine().getTargetTriple().isOSOpenBSD();
   }
 
+  /// Return the address of stack guard, or return nullptr if the target wants
+  /// to lower LOAD_STACK_GUARD manually.
+  virtual Value *getStackGuardAddr(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;
   }
public:
   /// This method should be implemented by targets that mark instructions with
   /// the 'hasPostISelHook' flag. These instructions must be adjusted after
   /// instruction selection by target hooks.  e.g. To fill in optional defs for
   /// ARM 's' setting instructions.
   virtual void
   AdjustInstrPostInstrSelection(MachineInstr *MI, SDNode *Node) const;
 
   /// If this function returns true, SelectionDAGBuilder emits a
-  /// LOAD_STACK_GUARD node when it is lowering Intrinsic::stackprotector.
-  virtual bool useLoadStackGuardNode() const {
-    return false;
-  }
+  /// LOAD_STACK_GUARD node when it is lowering Intrinsic::stackprotector,
+  /// even if getStackGuardAddr returns a global variable.
+  virtual bool forceLoadStackGuardNode() const { return false; }
 
   /// Lower TLS global address SDNode for target independent emulated TLS model.
   virtual SDValue LowerToTLSEmulatedModel(const GlobalAddressSDNode *GA,
                                           SelectionDAG &DAG) const;
 };
 
 /// Given an LLVM IR type and return type attributes, compute the return value
 /// EVTs and flags, and optionally also the offsets, if the return value is
 /// being lowered to memory.
 void GetReturnInfo(Type *ReturnType, AttributeSet attr,
                    SmallVectorImpl<ISD::OutputArg> &Outs,
                    const TargetLowering &TLI, const DataLayout &DL);
 
 } // end llvm namespace
 
 #endif

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h

public:
     /// 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;
     }
 
     /// 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,
+                    const Value *G) {
       // 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);
+        Guard = G;
     }
 
     /// 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).
private:
     /// 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.
+    /// It either stores a load to an global variable address to the stack
+    /// guard, or stores a llvm.experimental_stackguardvalue CallInst.
     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.

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();
-  SDValue GuardPtr = getValue(IRGuard);
-  SDValue StackSlotPtr = DAG.getFrameIndex(FI, PtrTy);
-
   unsigned Align = DL->getPrefTypeAlignment(IRGuard->getType());
-
-  SDValue Guard;
   SDLoc dl = getCurSDLoc();
 
-  // If GuardReg is set and useLoadStackGuardNode returns true, retrieve the
+  SDValue Guard;
+  // If GuardReg is set and forceLoadStackGuardNode returns true, retrieve the
   // guard value from the virtual register holding the value. Otherwise, emit a
   // volatile load to retrieve the stack guard value.
   unsigned GuardReg = SPD.getGuardReg();
-
-  if (GuardReg && TLI.useLoadStackGuardNode())
+  if (GuardReg && TLI.forceLoadStackGuardNode()) {
     Guard = DAG.getCopyFromReg(DAG.getEntryNode(), dl, GuardReg,
                                PtrTy);
-  else
-    Guard = DAG.getLoad(PtrTy, dl, DAG.getEntryNode(),
-                        GuardPtr, MachinePointerInfo(IRGuard, 0),
-                        true, false, false, Align);
+  } else if (dyn_cast<CallInst>(IRGuard)) {
+    Guard = getValue(IRGuard);
+  } else if (auto Load = dyn_cast<LoadInst>(IRGuard)) {
+    SDValue GuardPtr = getValue(Load->getPointerOperand());
+    Guard = DAG.getLoad(PtrTy, dl, DAG.getEntryNode(), GuardPtr,
+                        MachinePointerInfo(Load->getPointerOperand(), 0), true,
+                        false, false, Align);
+  } else {
+    llvm_unreachable("llvm.stackprotectorcheck expects either a load of a "
+                     "global variable, or a llvm.experimental_stackguardvalue "
+                     "call.");
+  }
 
+  SDValue StackSlotPtr = DAG.getFrameIndex(FI, PtrTy);
   SDValue StackSlot = DAG.getLoad(
       PtrTy, dl, DAG.getEntryNode(), StackSlotPtr,
       MachinePointerInfo::getFixedStack(DAG.getMachineFunction(), FI), true,
       false, false, Align);
 
   // Perform the comparison via a subtract/getsetcc.
   EVT VT = Guard.getValueType();
   SDValue Sub = DAG.getNode(ISD::SUB, dl, VT, Guard, StackSlot);
if (PtrTy != ResTy)
       report_fatal_error("Wrong result type for @llvm.get.dynamic.area.offset"
                          " intrinsic!");
     Res = DAG.getNode(ISD::GET_DYNAMIC_AREA_OFFSET, sdl, DAG.getVTList(ResTy),
                       Op);
     DAG.setRoot(Op);
     setValue(&I, Res);
     return nullptr;
   }
+  case Intrinsic::experimental_stackguardvalue: {
+    EVT PtrTy = TLI.getPointerTy(DAG.getDataLayout());
+    SDValue Chain = getRoot();
+    MachineSDNode *Node =
+        DAG.getMachineNode(TargetOpcode::LOAD_STACK_GUARD, sdl, PtrTy, Chain);
+    DAG.setRoot(Chain);
+    setValue(&I, SDValue(Node, 0));
+    return nullptr;
+  }
   case Intrinsic::stackprotector: {
     // Emit code into the DAG to store the stack guard onto the stack.
     MachineFunction &MF = DAG.getMachineFunction();
     MachineFrameInfo *MFI = MF.getFrameInfo();
     EVT PtrTy = TLI.getPointerTy(DAG.getDataLayout());
     SDValue Src, Chain = getRoot();
-    const Value *Ptr = cast<LoadInst>(I.getArgOperand(0))->getPointerOperand();
-    const GlobalVariable *GV = dyn_cast<GlobalVariable>(Ptr);
+    const GlobalVariable *GV = nullptr;
+    if (auto Ptr = dyn_cast<LoadInst>(I.getArgOperand(0))) {
+      GV = dyn_cast<GlobalVariable>(Ptr->getPointerOperand());
 
-    // See if Ptr is a bitcast. If it is, look through it and see if we can get
-    // global variable __stack_chk_guard.
+      // See if Ptr is a bitcast. If it is, look through it and see if we can
+      // get global variable __stack_chk_guard.
       if (!GV)
-      if (const Operator *BC = dyn_cast<Operator>(Ptr))
+        if (const Operator *BC = dyn_cast<Operator>(Ptr->getPointerOperand()))
           if (BC->getOpcode() == Instruction::BitCast)
             GV = dyn_cast<GlobalVariable>(BC->getOperand(0));
+    }
 
-    if (GV && TLI.useLoadStackGuardNode()) {
+    if (GV && TLI.forceLoadStackGuardNode()) {
       // Emit a LOAD_STACK_GUARD node.
       MachineSDNode *Node = DAG.getMachineNode(TargetOpcode::LOAD_STACK_GUARD,
                                                sdl, PtrTy, Chain);
       MachinePointerInfo MPInfo(GV);
       MachineInstr::mmo_iterator MemRefs = MF.allocateMemRefsArray(1);
       unsigned Flags = MachineMemOperand::MOLoad |
                        MachineMemOperand::MOInvariant;
       *MemRefs = MF.getMachineMemOperand(MPInfo, Flags,
                                          PtrTy.getSizeInBits() / 8,
                                          DAG.getEVTAlignment(PtrTy));
       Node->setMemRefs(MemRefs, MemRefs + 1);
 
-      // Copy the guard value to a virtual register so that it can be
-      // retrieved in the epilogue.
       Src = SDValue(Node, 0);
+    } else {
+      Src = getValue(I.getArgOperand(0)); // The guard's value.
+    }
+    // We get a LOAD_STCK_GUARD either from forceLoadStackGuardNode or from
+    // lowering llvm.experimental_stackguardvalue. Copy the guard value to a
+    // virtual register so that it can be retrieved in the epilogue.
+    if ((GV && TLI.forceLoadStackGuardNode()) ||
+        dyn_cast<CallInst>(I.getArgOperand(0))) {
       const TargetRegisterClass *RC =
           TLI.getRegClassFor(Src.getSimpleValueType());
       unsigned Reg = MF.getRegInfo().createVirtualRegister(RC);
 
       SPDescriptor.setGuardReg(Reg);
       Chain = DAG.getCopyToReg(Chain, sdl, Reg, Src);
-    } else {
-      Src = getValue(I.getArgOperand(0));   // The guard's value.
     }
 
     AllocaInst *Slot = cast<AllocaInst>(I.getArgOperand(1));
 
     int FI = FuncInfo.StaticAllocaMap[Slot];
     MFI->setStackProtectorIndex(FI);
 
     SDValue FIN = DAG.getFrameIndex(FI, PtrTy);
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
+    // the stack protector descriptor and export the guard value so we can
     // access it in FinishBasicBlock.
     const BasicBlock *BB = I.getParent();
-    SPDescriptor.initialize(BB, FuncInfo.MBBMap[BB], I);
+    SPDescriptor.initialize(BB, FuncInfo.MBBMap[BB], I.getArgOperand(0));
     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;

lib/CodeGen/StackProtector.cpp

 //===-- StackProtector.cpp - Stack Protector Insertion --------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // This pass inserts stack protectors into functions which need them. A variable
 // with a random value in it is stored onto the stack before the local variables
 // are allocated. Upon exiting the block, the stored value is checked. If it's
 // changed, then there was some sort of violation and the program aborts.
 //
+// Currently we have 3 code paths:
+// 1) IR without SupportsSelectionDAGSP -> FastISel -> Optional backend lowering
+// 2) IR with SupportsSelectionDAGSP -> SelectionDAG -> Optional backend
+//    lowering
+// 3) IR without SupportsSelectionDAGSP -> SelectionDAG -> Optional backend
+//    lowering
+//
+// For details on llvm.stackprotectorcheck, see comment on
+// StackProtectorDescriptor.
+//
+// We do want to eliminate 3), since all cases it handles can be handled by 2).
+// The current problem is visitSPDescriptorFailure hardcodes the failure
+// handler, which isn't corret on OpenBSD. Once this is solved, 3) will be
+// removed.
+//
+// By combining TLI::getStackGuardAddr and TLI::forceLoadStackGuardNode, SSP
+// guard loading can be customized by backends:
+// * Some backends want to lower the global variable manually, in such case
+//   forceLoadStackGuardNode should return true and getStackGuardAddr returns
+//   the global variable.
+// * Other platforms (e.g. PowerPC 64-bit) want to lower the stack guard loading
+//   completely manually, getStackGuardAddr should return nullptr.
+// * For the rest, forceLoadStackGuardNode returns false and
+//   getStackGuardAddr returns the global variable, thus guard loading not
+//   customized.
+//
 //===----------------------------------------------------------------------===//
 
 #include "llvm/CodeGen/StackProtector.h"
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/Statistic.h"
 #include "llvm/Analysis/BranchProbabilityInfo.h"
 #include "llvm/Analysis/ValueTracking.h"
 #include "llvm/CodeGen/Analysis.h"

 /// 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;
   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 (!F->hasFnAttribute(Attribute::StackProtect))
     return false;
 
   for (const BasicBlock &BB : *F) {
for (BasicBlock::reverse_iterator I = std::next(BB->rbegin()), E = BB->rend();
 
     // Increment max search.
     SearchCounter++;
   }
 
   return nullptr;
 }
 
-/// Insert code into the entry block that stores the __stack_chk_guard
+/// Return the instruction to load a stack guard.
+static Value *CreateLoadStackGuard(IRBuilder<> &B, Module &M,
+                                   const TargetLoweringBase *TLI) {
+  if (auto StackGuardAddr = TLI->getStackGuardAddr(M))
+    return B.CreateLoad(StackGuardAddr, "StackGuard");
+  return B.CreateCall(
+      Intrinsic::getDeclaration(&M, Intrinsic::experimental_stackguardvalue));
+}
+
+/// Insert code into the entry block that stores the stack 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,
+///     StackGuard = <load or intrinsic to read stack guard>
+///     call void @llvm.stackprotector(StackGuard, StackGuardSlot)
+static void CreatePrologue(Function *F, Module *M, ReturnInst *RI,
                            const TargetLoweringBase *TLI, const Triple &TT,
-                           AllocaInst *&AI, Value *&StackGuardVar) {
-  bool SupportsSelectionDAGSP = false;
+                           AllocaInst *&AI, Value *&StackGuardVal) {
   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);
-  }
 
   IRBuilder<> B(&F->getEntryBlock().front());
   AI = B.CreateAlloca(PtrTy, nullptr, "StackGuardSlot");
-  LoadInst *LI = B.CreateLoad(StackGuardVar, "StackGuard");
+  StackGuardVal = CreateLoadStackGuard(B, *M, TLI);
   B.CreateCall(Intrinsic::getDeclaration(M, Intrinsic::stackprotector),
-               {LI, AI});
-
-  return SupportsSelectionDAGSP;
+               {StackGuardVal, AI});
 }
 
 /// 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;
+  bool SupportsSelectionDAGSP = EnableSelectionDAGSP &&
+                                !TM->Options.EnableFastISel &&
+                                TLI->supportsSelectionDAGSP();
   AllocaInst *AI = nullptr;       // Place on stack that stores the stack guard.
-  Value *StackGuardVar = nullptr; // The stack guard variable.
+  Value *StackGuardVal = nullptr; // The stack guard value.
 
   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);
+      CreatePrologue(F, M, RI, TLI, Trip, AI, StackGuardVal);
     }
 
     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.");
       }
 
       Function *Intrinsic =
           Intrinsic::getDeclaration(M, Intrinsic::stackprotectorcheck);
-      CallInst::Create(Intrinsic, StackGuardVar, "", InsertionPt);
+      CallInst::Create(Intrinsic, StackGuardVal, "", InsertionPt);
     } else {
       // 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
+      //     %1 = <load or intrinsic to read stack 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();
 
       // 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);
       }
 
       // Remove default branch instruction to the new BB.
       BB->getTerminator()->eraseFromParent();
 
       // Move the newly created basic block to the point right after the old
       // basic block so that it's in the "fall through" position.
       NewBB->moveAfter(BB);
 
       // Generate the stack protector instructions in the old basic block.
       IRBuilder<> B(BB);
-      LoadInst *LI1 = B.CreateLoad(StackGuardVar);
+      Value *LI1 = CreateLoadStackGuard(B, *M, TLI);
       LoadInst *LI2 = B.CreateLoad(AI);
       Value *Cmp = B.CreateICmpEQ(LI1, LI2);
       auto SuccessProb =
           BranchProbabilityInfo::getBranchProbStackProtector(true);
       auto FailureProb =
           BranchProbabilityInfo::getBranchProbStackProtector(false);
       MDNode *Weights = MDBuilder(F->getContext())
                             .createBranchWeights(SuccessProb.getNumerator(),

lib/CodeGen/TargetLoweringBase.cpp

case 2:
     // Allow 2*r as r+r.
     break;
   default: // Don't allow n * r
     return false;
   }
 
   return true;
 }
+
+Value *TargetLoweringBase::getStackGuardAddr(Module &M) const {
+  PointerType *PtrTy = Type::getInt8PtrTy(M.getContext());
+  if (getTargetMachine().getTargetTriple().isOSOpenBSD()) {
+    auto StackGuardAddr = M.getOrInsertGlobal("__guard_local", PtrTy);
+    cast<GlobalValue>(StackGuardAddr)
+        ->setVisibility(GlobalValue::HiddenVisibility);
+    return StackGuardAddr;
+  }
+  return M.getOrInsertGlobal("__stack_chk_guard", PtrTy);
+}

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':
+    // The old version takes a guard address i8**, while the new one takes a
+    // value i8*.
+    if (Name == "stackprotectorcheck") {
+      if (F->arg_size() == 1 &&
+          F->arg_begin()->getType() ==
+              PointerType::getUnqual(Type::getInt8PtrTy(F->getContext()))) {
+        F->setName(Name + ".old");
+        NewFn = Intrinsic::getDeclaration(F->getParent(),
+                                          Intrinsic::stackprotectorcheck);
+        return true;
+      }
+    }
+    break;
+
   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.") ||
case Intrinsic::objectsize:
     return;
 
   case Intrinsic::ctpop: {
     CI->replaceAllUsesWith(Builder.CreateCall(NewFn, {CI->getArgOperand(0)}));
     CI->eraseFromParent();
     return;
   }
 
+  // Create a load of the address.
+  case Intrinsic::stackprotectorcheck: {
+    CI->replaceAllUsesWith(
+        Builder.CreateCall(NewFn, {Builder.CreateLoad(CI->getArgOperand(0))}));
+    CI->eraseFromParent();
+    return;
+  }
+
   case Intrinsic::x86_xop_vfrcz_ss:
   case Intrinsic::x86_xop_vfrcz_sd:
     CI->replaceAllUsesWith(
         Builder.CreateCall(NewFn, {CI->getArgOperand(1)}, Name));
     CI->eraseFromParent();
     return;
 
   case Intrinsic::x86_sse41_ptestc:

lib/Target/AArch64/AArch64ISelLowering.h

public:
   TargetLoweringBase::AtomicExpansionKind
   shouldExpandAtomicLoadInIR(LoadInst *LI) const override;
   bool shouldExpandAtomicStoreInIR(StoreInst *SI) const override;
   TargetLoweringBase::AtomicExpansionKind
   shouldExpandAtomicRMWInIR(AtomicRMWInst *AI) const override;
 
   bool shouldExpandAtomicCmpXchgInIR(AtomicCmpXchgInst *AI) const override;
 
-  bool useLoadStackGuardNode() const override;
+  bool forceLoadStackGuardNode() const override;
   TargetLoweringBase::LegalizeTypeAction
   getPreferredVectorAction(EVT VT) const override;
 
   /// If the target has a standard location for the unsafe stack pointer,
   /// returns the address of that location. Otherwise, returns nullptr.
   Value *getSafeStackPointerLocation(IRBuilder<> &IRB) const override;
 
   /// If a physical register, this returns the register that receives the

lib/Target/AArch64/AArch64ISelLowering.cpp

void AArch64TargetLowering::ReplaceNodeResults(
   case ISD::FP_TO_UINT:
   case ISD::FP_TO_SINT:
     assert(N->getValueType(0) == MVT::i128 && "unexpected illegal conversion");
     // Let normal code take care of it by not adding anything to Results.
     return;
   }
 }
 
-bool AArch64TargetLowering::useLoadStackGuardNode() const {
-  return true;
-}
+// Manually lower LOAD_STACK_GUARD to prevent the guard address from being
+// spilled to the stack, which may be override by user input.
+bool AArch64TargetLowering::forceLoadStackGuardNode() const { return true; }
 
 unsigned AArch64TargetLowering::combineRepeatedFPDivisors() const {
   // Combine multiple FDIVs with the same divisor into multiple FMULs by the
   // reciprocal if there are three or more FDIVs.
   return 3;
 }
 
 TargetLoweringBase::LegalizeTypeAction

lib/Target/ARM/ARMISelLowering.h

public:
     bool shouldInsertFencesForAtomic(const Instruction *I) const override;
     TargetLoweringBase::AtomicExpansionKind
     shouldExpandAtomicLoadInIR(LoadInst *LI) const override;
     bool shouldExpandAtomicStoreInIR(StoreInst *SI) const override;
     TargetLoweringBase::AtomicExpansionKind
     shouldExpandAtomicRMWInIR(AtomicRMWInst *AI) const override;
     bool shouldExpandAtomicCmpXchgInIR(AtomicCmpXchgInst *AI) const override;
 
-    bool useLoadStackGuardNode() const override;
+    bool forceLoadStackGuardNode() const override;
 
     bool canCombineStoreAndExtract(Type *VectorTy, Value *Idx,
                                    unsigned &Cost) const override;
 
     bool isCheapToSpeculateCttz() const override;
     bool isCheapToSpeculateCtlz() const override;
 
   protected:

lib/Target/ARM/ARMISelLowering.cpp

 }
 
 bool ARMTargetLowering::shouldInsertFencesForAtomic(
     const Instruction *I) const {
   return InsertFencesForAtomic;
 }
 
 // This has so far only been implemented for MachO.
-bool ARMTargetLowering::useLoadStackGuardNode() const {
+bool ARMTargetLowering::forceLoadStackGuardNode() const {
   return Subtarget->isTargetMachO();
 }
 
 bool ARMTargetLowering::canCombineStoreAndExtract(Type *VectorTy, Value *Idx,
                                                   unsigned &Cost) const {
   // If we do not have NEON, vector types are not natively supported.
   if (!Subtarget->hasNEON())
     return false;

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;
+
+    Value *getStackGuardAddr(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;
 
     bool isNoopAddrSpaceCast(unsigned SrcAS, unsigned DestAS) const override;
 
-    bool useLoadStackGuardNode() const override;
+    bool forceLoadStackGuardNode() const override;
     /// \brief Customize the preferred legalization strategy for certain types.
     LegalizeTypeAction getPreferredVectorAction(EVT VT) const override;
 
     bool isIntDivCheap(EVT VT, AttributeSet Attr) const override;
 
   protected:
     std::pair<const TargetRegisterClass *, uint8_t>
     findRepresentativeClass(const TargetRegisterInfo *TRI,

lib/Target/X86/X86ISelLowering.cpp

   PredictableSelectIsExpensive = Subtarget.getSchedModel().isOutOfOrder();
   EnableExtLdPromotion = true;
   setPrefFunctionAlignment(4); // 2^4 bytes.
 
   verifyIntrinsicTables();
 }
 
 // This has so far only been implemented for 64-bit MachO.
-bool X86TargetLowering::useLoadStackGuardNode() const {
+bool X86TargetLowering::forceLoadStackGuardNode() const {
   return Subtarget.isTargetMachO() && Subtarget.is64Bit();
 }
 
 TargetLoweringBase::LegalizeTypeAction
 X86TargetLowering::getPreferredVectorAction(EVT VT) const {
   if (ExperimentalVectorWideningLegalization &&
       VT.getVectorNumElements() != 1 &&
       VT.getVectorElementType().getSimpleVT() != MVT::i1)

 }
 
 unsigned X86TargetLowering::getAddressSpace() const {
   if (Subtarget.is64Bit())
     return (getTargetMachine().getCodeModel() == CodeModel::Kernel) ? 256 : 257;
   return 256;
 }
 
-bool X86TargetLowering::getStackCookieLocation(unsigned &AddressSpace,
-                                               unsigned &Offset) const {
-  if (!Subtarget.isTargetLinux())
+bool X86TargetLowering::supportsSelectionDAGSP() const {
+  if (Subtarget.isTargetLinux())
     return false;
+  return TargetLowering::supportsSelectionDAGSP();
+}
 
+Value *X86TargetLowering::getStackGuardAddr(Module &M) const {
+  /// 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.
+  if (Subtarget.isTargetLinux()) {
+    unsigned AddressSpace = getAddressSpace();
     // %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;
+    Constant *OffsetVal =
+        ConstantInt::get(Type::getInt32Ty(M.getContext()), Offset);
+
+    PointerType *PtrTy = Type::getInt8PtrTy(M.getContext());
+    return ConstantExpr::getIntToPtr(OffsetVal,
+                                     PointerType::get(PtrTy, AddressSpace));
+  }
+  return TargetLowering::getStackGuardAddr(M);
 }
 
 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
 }
+
+@user_defined_guard = external global i8*
+
+declare void @llvm.stackprotectorcheck(i8**)
+
+define void @test.stackprotectorcheck() {
+entry:
+; CHECK-LABEL: @test.stackprotectorcheck
+; CHECK: %0 = load i8*, i8** @user_defined_guard
+; CHECK: call void @llvm.stackprotectorcheck(i8* %0)
+  call void @llvm.stackprotectorcheck(i8** @user_defined_guard)
+  ret void
+}
+
+; CHECK-DAG: declare i32 @llvm.objectsize.i32.p0i8