XOR the frame pointer with the stack cookie when protecting the stack

[rnk]

Summary

This strengthens the guard and matches MSVC.

[hans]

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

2136

Is there some way we could assert the register we use here stays the same from entry to exit? This is the only part that sounds potentially scary, the rest looks pretty straight-forward.

llvm/lib/Target/X86/X86ISelLowering.cpp

1690

It doesn't really matter what the CRT does though, right? I mean, we could do this for non-MSVC targets too, it's just that it's not common practice?

[rnk]

I discovered that this doesn't work quite right with fastisel, so I'll reupload with a fix for that.

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

2136

Perhaps instead I should test and commit this patch separately:

diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
index cb37137d547..c6d43b04003 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
@@ -1358,6 +1358,10 @@ static void preassignSwiftErrorRegs(const TargetLowering *TLI,
 }

 void SelectionDAGISel::SelectAllBasicBlocks(const Function &Fn) {
+#ifndef NDEBUG
+  const TargetRegisterInfo &TRI = *MF->getSubtarget().getRegisterInfo();
+  unsigned OriginalFrameReg = TRI.getFrameRegister(MF);
+#endif
   FastISelFailed = false;
   // Initialize the Fast-ISel state, if needed.
   FastISel *FastIS = nullptr;
@@ -1621,6 +1625,9 @@ void SelectionDAGISel::SelectAllBasicBlocks(const Function &Fn) {
   delete FastIS;
   SDB->clearDanglingDebugInfo();
   SDB->SPDescriptor.resetPerFunctionState();
+
+  assert(OriginalFrameReg == TRI.getFrameRegister(MF) &&
+         "frame register changed during instruction selection");
 }

 /// Given that the input MI is before a partial terminator sequence TSeq, return

llvm/lib/Target/X86/X86ISelLowering.cpp

1690

Yeah, I structured it this way so that we could expose it as a flag or enable it everywhere if we want to make that policy change to -fstack-protector. I don't think it costs much and it should improve security, but that's a longer discussion we need to have about beefing up LLVM's -fstack-protector.

[rnk]

• fix fastisel

[hans]

lgtm

I'm curious what the binary size impact will be.

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

2136

Doesn't need to be separate since this might be the first time we start depending on this invariant. Sounds like a good idea to me.

llvm/lib/CodeGen/StackProtector.cpp

390

s/it's/its/

[rnk]

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

2136

I discovered there's a reason we don't depend on this invariant: it doesn't hold at all. It would really be better if we had a way to assert if anyone calls getFrameRegister before ISel is finished...

[rnk]

I had a possible idea for making this work. I thought about creating a virtual register that will ultimately be assigned to the frame register, which is calculated at the time of freezeReservedRegs. I prototyped something, but this virtual register breaks a lot of rules because it has no instructions that define it.

@MatzeB, does this seem like a good idea? It has the advantage that we'll go through instruction selection with a normal register, and then we'll rewrite it in the normal course of register allocation.

Alternatively, I can do things the usual way and add Yet Another custom pseudo-instruction that is X86-specific and we'll have to do this all over again when someone wants this on ARM.

[MatzeB]

In D40622#940008, @rnk wrote:

I had a possible idea for making this work. I thought about creating a virtual register that will ultimately be assigned to the frame register, which is calculated at the time of freezeReservedRegs. I prototyped something, but this virtual register breaks a lot of rules because it has no instructions that define it.

@MatzeB, does this seem like a good idea? It has the advantage that we'll go through instruction selection with a normal register, and then we'll rewrite it in the normal course of register allocation.

I'm not sure I completely understand your plan. But some assumption that come to my mind:

• We should only assign/decide on reserved registers once. (I know 1 or 2 targets currently violate this rule, but that should be fixed sooner or later).
• The register allocators will generally not assign a reserved register to a virtual register.
• The copy coalescer has some cases where copies from reserved regs to vregs get eliminated.
• I'm not sure why you would want to use a vreg when you expect it to end up containing the frame pointer anyway. Instead just assign the reserved registers right away.

[MatzeB]

Looking around the code, I think the simplest solution would be to make useStackGuardXorFP() return the register that should be used or 0.

[rnk]

In D40622#940035, @MatzeB wrote:

Looking around the code, I think the simplest solution would be to make useStackGuardXorFP() return the register that should be used or 0.

The problem is that we don't know what register to use until freezeReservedRegs happens and hasFP stops changing value. I want to use a virtual register as a place-holder for the final frame register so that we can do normal instruction selection and then replace that virtual register with the frame register during virtual register rewriting. It really has no interaction with register allocation, it's just that virtual register rewriting seems like a convenient place to replace this place-holder. Is this a reasonable idea?

[rnk]

In D40622#940850, @rnk wrote:

In D40622#940035, @MatzeB wrote:

Looking around the code, I think the simplest solution would be to make useStackGuardXorFP() return the register that should be used or 0.

The problem is that we don't know what register to use until freezeReservedRegs happens and hasFP stops changing value. I want to use a virtual register as a place-holder for the final frame register so that we can do normal instruction selection and then replace that virtual register with the frame register during virtual register rewriting. It really has no interaction with register allocation, it's just that virtual register rewriting seems like a convenient place to replace this place-holder. Is this a reasonable idea?

Maybe a better way to think about it is it's a physical register that gets replaced with another physical register. Does this seem like something that would be useful? I'm surprised we don't have something like this already.

[MatzeB]

In D40622#940853, @rnk wrote:

In D40622#940850, @rnk wrote:

In D40622#940035, @MatzeB wrote:

Looking around the code, I think the simplest solution would be to make useStackGuardXorFP() return the register that should be used or 0.

The problem is that we don't know what register to use until freezeReservedRegs happens and hasFP stops changing value. I want to use a virtual register as a place-holder for the final frame register so that we can do normal instruction selection and then replace that virtual register with the frame register during virtual register rewriting. It really has no interaction with register allocation, it's just that virtual register rewriting seems like a convenient place to replace this place-holder. Is this a reasonable idea?

Maybe a better way to think about it is it's a physical register that gets replaced with another physical register. Does this seem like something that would be useful? I'm surprised we don't have something like this already.

On a design/high level this stack protector stuff feels to me like it would be a better fit for the prolog epilogue insertion as it's basically inserting a few extra instructions there and people just happened to do it earlier because they wanted to use generic instructions so they don't have to reimplement it for every backend...

But short of redesigning the whole thing, using a vreg and replacing it in a custom pass is fine too.

[rnk]

• Redo the whole thing with a plain old x86 pseudo instruction =P

[hans]

Nice. This seems like the most straight-forward solution to me.

llvm/lib/CodeGen/StackProtector.cpp

390

This one still applies (for the second it's)

llvm/lib/Target/X86/X86InstrCompiler.td

145

s/use/used/

[rnk]

Thanks! I think this is the way to go for now. Having some kind of virtual frame register might be nice if we had a second use case for it, but I can't think of one now.

llvm/lib/CodeGen/StackProtector.cpp

390

Hm, actually that comment is wrong. The target's fast isel implementation doesn't have to do anything. I checked, and the protection check is inserted at a higher level after fast isel.

[rnk]

• fix comments

llvm/include/llvm/CodeGen/TargetLowering.h

   /// Should be used only when getIRStackGuard returns nullptr.
   virtual void insertSSPDeclarations(Module &M) const;
 
   /// Return the variable that's previously inserted by insertSSPDeclarations,
   /// if any, otherwise return nullptr. Should be used only when
   /// getIRStackGuard returns nullptr.
   virtual Value *getSDagStackGuard(const Module &M) const;
 
+  /// If this function returns true, stack protection checks should XOR the
+  /// frame pointer (or whichever pointer is used to address locals) into the
+  /// stack guard value before checking it. getIRStackGuard must return nullptr
+  /// if this returns true.
+  virtual bool useStackGuardXorFP() const { return false; }
+
   /// If the target has a standard stack protection check function that
   /// performs validation and error handling, returns the function. Otherwise,
   /// returns nullptr. Must be previously inserted by insertSSPDeclarations.
   /// Should be used only when getIRStackGuard returns nullptr.
   virtual Value *getSSPStackGuardCheck(const Module &M) const;
 
 protected:
   Value *getDefaultSafeStackPointerLocation(IRBuilder<> &IRB,
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;
   }
 
+  virtual SDValue emitStackGuardXorFP(SelectionDAG &DAG, SDValue Val,
+                                      const SDLoc &DL) const {
+    llvm_unreachable("not implemented for this target");
+  }
+
   /// Lower TLS global address SDNode for target independent emulated TLS model.
   virtual SDValue LowerToTLSEmulatedModel(const GlobalAddressSDNode *GA,
                                           SelectionDAG &DAG) const;
 
   // seteq(x, 0) -> truncate(srl(ctlz(zext(x)), log2(#bits)))
   // If we're comparing for equality to zero and isCtlzFast is true, expose the
   // fact that this can be implemented as a ctlz/srl pair, so that the dag
   // combiner can fold the new nodes.

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

 ///
 /// For a high level explanation of how this fits into the stack protector
 /// generation see the comment on the declaration of class
 /// StackProtectorDescriptor.
 void SelectionDAGBuilder::visitSPDescriptorParent(StackProtectorDescriptor &SPD,
                                                   MachineBasicBlock *ParentBB) {
 
   // 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();
 
   SDValue Guard;
   SDLoc dl = getCurSDLoc();
   SDValue StackSlotPtr = DAG.getFrameIndex(FI, PtrTy);
   const Module &M = *ParentBB->getParent()->getFunction()->getParent();
   unsigned Align = DL->getPrefTypeAlignment(Type::getInt8PtrTy(M.getContext()));
 
   // Generate code to load the content of the guard slot.
-  SDValue StackSlot = DAG.getLoad(
+  SDValue GuardVal = DAG.getLoad(
       PtrTy, dl, DAG.getEntryNode(), StackSlotPtr,
       MachinePointerInfo::getFixedStack(DAG.getMachineFunction(), FI), Align,
       MachineMemOperand::MOVolatile);
 
+  if (TLI.useStackGuardXorFP())
+    GuardVal = TLI.emitStackGuardXorFP(DAG, GuardVal, dl);
+
   // Retrieve guard check function, nullptr if instrumentation is inlined.
   if (const Value *GuardCheck = TLI.getSSPStackGuardCheck(M)) {
     // The target provides a guard check function to validate the guard value.
     // Generate a call to that function with the content of the guard slot as
     // argument.
     auto *Fn = cast<Function>(GuardCheck);
     FunctionType *FnTy = Fn->getFunctionType();
     assert(FnTy->getNumParams() == 1 && "Invalid function signature");
 
     TargetLowering::ArgListTy Args;
     TargetLowering::ArgListEntry Entry;
-    Entry.Node = StackSlot;
+    Entry.Node = GuardVal;
     Entry.Ty = FnTy->getParamType(0);
     if (Fn->hasAttribute(1, Attribute::AttrKind::InReg))
       Entry.IsInReg = true;
     Args.push_back(Entry);
 
     TargetLowering::CallLoweringInfo CLI(DAG);
     CLI.setDebugLoc(getCurSDLoc())
       .setChain(DAG.getEntryNode())
if (TLI.useLoadStackGuardNode()) {
 
     Guard =
         DAG.getLoad(PtrTy, dl, Chain, GuardPtr, MachinePointerInfo(IRGuard, 0),
                     Align, MachineMemOperand::MOVolatile);
   }
 
   // Perform the comparison via a subtract/getsetcc.
   EVT VT = Guard.getValueType();
-  SDValue Sub = DAG.getNode(ISD::SUB, dl, VT, Guard, StackSlot);
+  SDValue Sub = DAG.getNode(ISD::SUB, dl, VT, Guard, GuardVal);
 
   SDValue Cmp = DAG.getSetCC(dl, TLI.getSetCCResultType(DAG.getDataLayout(),
                                                         *DAG.getContext(),
                                                         Sub.getValueType()),
                              Sub, DAG.getConstant(0, dl, VT), ISD::SETNE);
 
   // If the sub is not 0, then we know the guard/stackslot do not equal, so
   // branch to failure MBB.
   SDValue BrCond = DAG.getNode(ISD::BRCOND, dl,
-                               MVT::Other, StackSlot.getOperand(0),
+                               MVT::Other, GuardVal.getOperand(0),
                                Cmp, DAG.getBasicBlock(SPD.getFailureMBB()));
   // Otherwise branch to success MBB.
   SDValue Br = DAG.getNode(ISD::BR, dl,
                            MVT::Other, BrCond,
                            DAG.getBasicBlock(SPD.getSuccessMBB()));
 
   DAG.setRoot(Br);
 }
if (TLI.useLoadStackGuardNode()) {
       Res = getLoadStackGuard(DAG, sdl, Chain);
     } else {
       const Value *Global = TLI.getSDagStackGuard(M);
       unsigned Align = DL->getPrefTypeAlignment(Global->getType());
       Res = DAG.getLoad(PtrTy, sdl, Chain, getValue(Global),
                         MachinePointerInfo(Global, 0), Align,
                         MachineMemOperand::MOVolatile);
     }
+    if (TLI.useStackGuardXorFP())
+      Res = TLI.emitStackGuardXorFP(DAG, Res, sdl);
     DAG.setRoot(Chain);
     setValue(&I, Res);
     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();

llvm/lib/CodeGen/StackProtector.cpp

 
 /// 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() {
+  // If the target wants to XOR the frame pointer into the guard value, it's
+  // impossible to emit the check in IR, so the target *must* support stack
+  // protection in SDAG.
   bool SupportsSelectionDAGSP =
-      EnableSelectionDAGSP && !TM->Options.EnableFastISel;
+      TLI->useStackGuardXorFP() ||
+      (EnableSelectionDAGSP && !TM->Options.EnableFastISel);
   AllocaInst *AI = nullptr;       // Place on stack that stores the stack guard.
 
   for (Function::iterator I = F->begin(), E = F->end(); I != E;) {
     BasicBlock *BB = &*I++;
     ReturnInst *RI = dyn_cast<ReturnInst>(BB->getTerminator());
     if (!RI)
       continue;
 

llvm/lib/Target/X86/X86ISelLowering.h

public:
     FastISel *createFastISel(FunctionLoweringInfo &funcInfo,
                              const TargetLibraryInfo *libInfo) const override;
 
     /// If the target has a standard location for the stack protector cookie,
     /// returns the address of that location. Otherwise, returns nullptr.
     Value *getIRStackGuard(IRBuilder<> &IRB) const override;
 
     bool useLoadStackGuardNode() const override;
+    bool useStackGuardXorFP() const override;
     void insertSSPDeclarations(Module &M) const override;
     Value *getSDagStackGuard(const Module &M) const override;
     Value *getSSPStackGuardCheck(const Module &M) const override;
+    SDValue emitStackGuardXorFP(SelectionDAG &DAG, SDValue Val,
+                                const SDLoc &DL) 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;

llvm/lib/Target/X86/X86ISelLowering.cpp

   verifyIntrinsicTables();
 }
 
 // This has so far only been implemented for 64-bit MachO.
 bool X86TargetLowering::useLoadStackGuardNode() const {
   return Subtarget.isTargetMachO() && Subtarget.is64Bit();
 }
 
+bool X86TargetLowering::useStackGuardXorFP() const {
+  // Currently only MSVC CRTs XOR the frame pointer into the stack guard value.
+  return Subtarget.getTargetTriple().isOSMSVCRT();
+}
+
+SDValue X86TargetLowering::emitStackGuardXorFP(SelectionDAG &DAG, SDValue Val,
+                                               const SDLoc &DL) const {
+  EVT PtrTy = getPointerTy(DAG.getDataLayout());
+  unsigned XorOp = Subtarget.is64Bit() ? X86::XOR64_FP : X86::XOR32_FP;
+  MachineSDNode *Node = DAG.getMachineNode(XorOp, DL, PtrTy, Val);
+  return SDValue(Node, 0);
+}
+
 TargetLoweringBase::LegalizeTypeAction
 X86TargetLowering::getPreferredVectorAction(EVT VT) const {
   if (ExperimentalVectorWideningLegalization &&
       VT.getVectorNumElements() != 1 &&
       VT.getVectorElementType().getSimpleVT() != MVT::i1)
     return TypeWidenVector;
 
   return TargetLoweringBase::getPreferredVectorAction(VT);

llvm/lib/Target/X86/X86InstrCompiler.td

def WIN_ALLOCA_32 : I<0, Pseudo, (outs), (ins GR32:$size),
                      Requires<[NotLP64]>;
 
 let Defs = [RAX, RSP, EFLAGS], Uses = [RSP] in
 def WIN_ALLOCA_64 : I<0, Pseudo, (outs), (ins GR64:$size),
                      "# dynamic stack allocation",
                      [(X86WinAlloca GR64:$size)]>,
                      Requires<[In64BitMode]>;
 
+// These instructions XOR the frame pointer into a GPR. They are used in some
+// stack protection schemes. These are post-RA pseudos because we only know the
+// frame register after register allocation.
+let Constraints = "$src = $dst", isPseudo = 1 in {
+  def XOR32_FP : I<0, Pseudo, (outs GR32:$dst), (ins GR32:$src),
+                  "xorl\t$$FP, $src", []>, Requires<[NotLP64]>;
+  def XOR64_FP : I<0, Pseudo, (outs GR64:$dst), (ins GR64:$src),
+                  "xorq\t$$FP $src", []>, Requires<[In64BitMode]>;
+}
 
 //===----------------------------------------------------------------------===//
 // EH Pseudo Instructions
 //
 let SchedRW = [WriteSystem] in {
 let isTerminator = 1, isReturn = 1, isBarrier = 1,
     hasCtrlDep = 1, isCodeGenOnly = 1 in {
 def EH_RETURN   : I<0xC3, RawFrm, (outs), (ins GR32:$addr),

llvm/lib/Target/X86/X86InstrInfo.cpp

static void expandLoadStackGuard(MachineInstrBuilder &MIB,
   BuildMI(MBB, I, DL, TII.get(X86::MOV64rm), Reg).addReg(X86::RIP).addImm(1)
       .addReg(0).addGlobalAddress(GV, 0, X86II::MO_GOTPCREL).addReg(0)
       .addMemOperand(MMO);
   MIB->setDebugLoc(DL);
   MIB->setDesc(TII.get(X86::MOV64rm));
   MIB.addReg(Reg, RegState::Kill).addImm(1).addReg(0).addImm(0).addReg(0);
 }
 
+static bool expandXorFP(MachineInstrBuilder &MIB, const TargetInstrInfo &TII) {
+  MachineBasicBlock &MBB = *MIB->getParent();
+  MachineFunction &MF = *MBB.getParent();
+  const X86Subtarget &Subtarget = MF.getSubtarget<X86Subtarget>();
+  const X86RegisterInfo *TRI = Subtarget.getRegisterInfo();
+  unsigned XorOp =
+      MIB->getOpcode() == X86::XOR64_FP ? X86::XOR64rr : X86::XOR32rr;
+  MIB->setDesc(TII.get(XorOp));
+  MIB.addReg(TRI->getFrameRegister(MF), RegState::Undef);
+  return true;
+}
+
 // This is used to handle spills for 128/256-bit registers when we have AVX512,
 // but not VLX. If it uses an extended register we need to use an instruction
 // that loads the lower 128/256-bit, but is available with only AVX512F.
 static bool expandNOVLXLoad(MachineInstrBuilder &MIB,
                             const TargetRegisterInfo *TRI,
                             const MCInstrDesc &LoadDesc,
                             const MCInstrDesc &BroadcastDesc,
                             unsigned SubIdx) {
bool X86InstrInfo::expandPostRAPseudo(MachineInstr &MI) const {
   case X86::KSET0D: return Expand2AddrKreg(MIB, get(X86::KXORDrr), X86::K0);
   case X86::KSET0Q: return Expand2AddrKreg(MIB, get(X86::KXORQrr), X86::K0);
   case X86::KSET1W: return Expand2AddrKreg(MIB, get(X86::KXNORWrr), X86::K0);
   case X86::KSET1D: return Expand2AddrKreg(MIB, get(X86::KXNORDrr), X86::K0);
   case X86::KSET1Q: return Expand2AddrKreg(MIB, get(X86::KXNORQrr), X86::K0);
   case TargetOpcode::LOAD_STACK_GUARD:
     expandLoadStackGuard(MIB, *this);
     return true;
+  case X86::XOR64_FP:
+  case X86::XOR32_FP:
+    return expandXorFP(MIB, *this);
   }
   return false;
 }
 
 /// Return true for all instructions that only update
 /// the first 32 or 64-bits of the destination register and leave the rest
 /// unmodified. This can be used to avoid folding loads if the instructions
 /// only update part of the destination register, and the non-updated part is

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

+; RUN: llc -mtriple=i386-pc-windows-msvc < %s -o - | FileCheck -check-prefix=MSVC-X86 %s
+; RUN: llc -mtriple=x86_64-pc-windows-msvc < %s -o - | FileCheck -check-prefix=MSVC-X64 %s
 
-; RUN: llc -mtriple=i386-pc-windows-msvc < %s -o - | FileCheck -check-prefix=MSVC-I386 %s
-; RUN: llc -mtriple=x86_64-pc-windows-msvc < %s -o - | FileCheck -check-prefix=MSVC-64 %s
-
-; MSVC-I386: movl ___security_cookie, %[[REG1:[a-z]*]]
-; MSVC-I386: movl %[[REG1]], [[SLOT:[0-9]*]](%esp)
-; MSVC-I386: calll _strcpy
-; MSVC-I386: movl [[SLOT]](%esp), %ecx
-; MSVC-I386: calll @__security_check_cookie@4
-; MSVC-I386: retl
-
-; MSVC-64: movq __security_cookie(%rip), %[[REG1:[a-z]*]]
-; MSVC-64: movq %[[REG1]], [[SLOT:[0-9]*]](%rsp)
-; MSVC-64: callq strcpy
-; MSVC-64: movq [[SLOT]](%rsp), %rcx
-; MSVC-64: callq __security_check_cookie
+; Make sure fastisel falls back and does something secure.
+; RUN: llc -mtriple=i686-pc-windows-msvc -O0 < %s -o - | FileCheck -check-prefix=MSVC-X86-O0 %s
+; RUN: llc -mtriple=x86_64-pc-windows-msvc -O0 < %s -o - | FileCheck -check-prefix=MSVC-X64-O0 %s
 
 @"\01LC" = internal constant [11 x i8] c"buf == %s\0A\00"    ; <[11 x i8]*> [#uses=1]
 
 define void @test(i8* %a) nounwind ssp {
 entry:
  %a_addr = alloca i8*    ; <i8**> [#uses=2]
  %buf = alloca [8 x i8]    ; <[8 x i8]*> [#uses=2]
-  %"alloca point" = bitcast i32 0 to i32   ; <i32> [#uses=0]
  store i8* %a, i8** %a_addr
  %buf1 = bitcast [8 x i8]* %buf to i8*   ; <i8*> [#uses=1]
  %0 = load i8*, i8** %a_addr, align 4    ; <i8*> [#uses=1]
  %1 = call i8* @strcpy(i8* %buf1, i8* %0) nounwind   ; <i8*> [#uses=0]
   %buf2 = bitcast [8 x i8]* %buf to i8*    ; <i8*> [#uses=1]
  %2 = call i32 (i8*, ...) @printf(i8* getelementptr ([11 x i8], [11 x i8]* @"\01LC", i32 0, i32 0), i8* %buf2) nounwind    ; <i32> [#uses=0]
  br label %return
 
 return:    ; preds = %entry
  ret void
 }
 
+; MSVC-X86-LABEL: _test:
+; MSVC-X86: movl ___security_cookie, %[[REG1:[^ ]*]]
+; MSVC-X86: xorl %esp, %[[REG1]]
+; MSVC-X86: movl %[[REG1]], [[SLOT:[0-9]*]](%esp)
+; MSVC-X86: calll _strcpy
+; MSVC-X86: movl [[SLOT]](%esp), %ecx
+; MSVC-X86: xorl %esp, %ecx
+; MSVC-X86: calll @__security_check_cookie@4
+; MSVC-X86: retl
+
+; MSVC-X64-LABEL: test:
+; MSVC-X64: movq __security_cookie(%rip), %[[REG1:[^ ]*]]
+; MSVC-X64: xorq %rsp, %[[REG1]]
+; MSVC-X64: movq %[[REG1]], [[SLOT:[0-9]*]](%rsp)
+; MSVC-X64: callq strcpy
+; MSVC-X64: movq [[SLOT]](%rsp), %rcx
+; MSVC-X64: xorq %rsp, %rcx
+; MSVC-X64: callq __security_check_cookie
+; MSVC-X64: retq
+
+; MSVC-X86-O0-LABEL: _test:
+; MSVC-X86-O0: movl ___security_cookie, %[[REG1:[^ ]*]]
+; MSVC-X86-O0: xorl %esp, %[[REG1]]
+; MSVC-X86-O0: movl %[[REG1]], [[SLOT:[0-9]*]](%esp)
+; MSVC-X86-O0: calll _strcpy
+; MSVC-X86-O0: movl [[SLOT]](%esp), %[[REG1:[^ ]*]]
+; MSVC-X86-O0: xorl %esp, %[[REG1]]
+; MSVC-X86-O0: movl %[[REG1]], %ecx
+; MSVC-X86-O0: calll @__security_check_cookie@4
+; MSVC-X86-O0: retl
+
+; MSVC-X64-O0-LABEL: test:
+; MSVC-X64-O0: movq __security_cookie(%rip), %[[REG1:[^ ]*]]
+; MSVC-X64-O0: xorq %rsp, %[[REG1]]
+; MSVC-X64-O0: movq %[[REG1]], [[SLOT:[0-9]*]](%rsp)
+; MSVC-X64-O0: callq strcpy
+; MSVC-X64-O0: movq [[SLOT]](%rsp), %[[REG1:[^ ]*]]
+; MSVC-X64-O0: xorq %rsp, %[[REG1]]
+; MSVC-X64-O0: movq %[[REG1]], %rcx
+; MSVC-X64-O0: callq __security_check_cookie
+; MSVC-X64-O0: retq
+
+
+declare void @escape(i32*)
+
+define void @test_vla(i32 %n) nounwind ssp {
+  %vla = alloca i32, i32 %n
+  call void @escape(i32* %vla)
+  ret void
+}
+
+; MSVC-X86-LABEL: _test_vla:
+; MSVC-X86: pushl %ebp
+; MSVC-X86: movl %esp, %ebp
+; MSVC-X86: movl ___security_cookie, %[[REG1:[^ ]*]]
+; MSVC-X86: xorl %ebp, %[[REG1]]
+; MSVC-X86: movl %[[REG1]], [[SLOT:-[0-9]*]](%ebp)
+; MSVC-X86: calll __chkstk
+; MSVC-X86: pushl
+; MSVC-X86: calll _escape
+; MSVC-X86: movl [[SLOT]](%ebp), %ecx
+; MSVC-X86: xorl %ebp, %ecx
+; MSVC-X86: calll @__security_check_cookie@4
+; MSVC-X86: movl %ebp, %esp
+; MSVC-X86: popl %ebp
+; MSVC-X86: retl
+
+; MSVC-X64-LABEL: test_vla:
+; MSVC-X64: pushq %rbp
+; MSVC-X64: subq $16, %rsp
+; MSVC-X64: leaq 16(%rsp), %rbp
+; MSVC-X64: movq __security_cookie(%rip), %[[REG1:[^ ]*]]
+; MSVC-X64: xorq %rbp, %[[REG1]]
+; MSVC-X64: movq %[[REG1]], [[SLOT:-[0-9]*]](%rbp)
+; MSVC-X64: callq __chkstk
+; MSVC-X64: callq escape
+; MSVC-X64: movq [[SLOT]](%rbp), %rcx
+; MSVC-X64: xorq %rbp, %rcx
+; MSVC-X64: callq __security_check_cookie
+; MSVC-X64: retq
+
+
+; This case is interesting because we address local variables with RBX but XOR
+; the guard value with RBP. That's fine, either value will do, as long as they
+; are the same across the life of the frame.
+
+define void @test_vla_realign(i32 %n) nounwind ssp {
+  %realign = alloca i32, align 32
+  %vla = alloca i32, i32 %n
+  call void @escape(i32* %realign)
+  call void @escape(i32* %vla)
+  ret void
+}
+
+; MSVC-X86-LABEL: _test_vla_realign:
+; MSVC-X86: pushl %ebp
+; MSVC-X86: movl %esp, %ebp
+; MSVC-X86: pushl %esi
+; MSVC-X86: andl $-32, %esp
+; MSVC-X86: subl $32, %esp
+; MSVC-X86: movl %esp, %esi
+; MSVC-X86: movl ___security_cookie, %[[REG1:[^ ]*]]
+; MSVC-X86: xorl %ebp, %[[REG1]]
+; MSVC-X86: movl %[[REG1]], [[SLOT:[0-9]*]](%esi)
+; MSVC-X86: calll __chkstk
+; MSVC-X86: pushl
+; MSVC-X86: calll _escape
+; MSVC-X86: movl [[SLOT]](%esi), %ecx
+; MSVC-X86: xorl %ebp, %ecx
+; MSVC-X86: calll @__security_check_cookie@4
+; MSVC-X86: leal -8(%ebp), %esp
+; MSVC-X86: popl %esi
+; MSVC-X86: popl %ebp
+; MSVC-X86: retl
+
+; MSVC-X64-LABEL: test_vla_realign:
+; MSVC-X64: pushq %rbp
+; MSVC-X64: pushq %rbx
+; MSVC-X64: subq $32, %rsp
+; MSVC-X64: leaq 32(%rsp), %rbp
+; MSVC-X64: andq $-32, %rsp
+; MSVC-X64: movq %rsp, %rbx
+; MSVC-X64: movq __security_cookie(%rip), %[[REG1:[^ ]*]]
+; MSVC-X64: xorq %rbp, %[[REG1]]
+; MSVC-X64: movq %[[REG1]], [[SLOT:[0-9]*]](%rbx)
+; MSVC-X64: callq __chkstk
+; MSVC-X64: callq escape
+; MSVC-X64: movq [[SLOT]](%rbx), %rcx
+; MSVC-X64: xorq %rbp, %rcx
+; MSVC-X64: callq __security_check_cookie
+; MSVC-X64: retq
+
+
 declare i8* @strcpy(i8*, i8*) nounwind
 
 declare i32 @printf(i8*, ...) nounwind
 

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

 ; DARWIN-IR: CALL64pcrel32 <ga:@__stack_chk_fail>
 
 ; MSVC-SELDAG: # Machine code for function test_branch_weights:
 ; MSVC-SELDAG: mem:Volatile LD4[@__security_cookie]
 ; MSVC-SELDAG: ST4[FixedStack0]
 ; MSVC-SELDAG: LD4[FixedStack0]
 ; MSVC-SELDAG: CALLpcrel32 <ga:@__security_check_cookie>
 
+; MSVC always uses selection DAG now.
 ; MSVC-IR: # Machine code for function test_branch_weights:
 ; MSVC-IR: mem:Volatile LD4[@__security_cookie]
 ; MSVC-IR: ST4[FixedStack0]
-; MSVC-IR: LD4[%StackGuardSlot]
+; MSVC-IR: LD4[FixedStack0]
 ; MSVC-IR: CALLpcrel32 <ga:@__security_check_cookie>
 
 define i32 @test_branch_weights(i32 %n) #0 {
 entry:
   %a = alloca [128 x i32], align 16
   %0 = bitcast [128 x i32]* %a to i8*
   call void @llvm.lifetime.start.p0i8(i64 512, i8* %0)
   %arraydecay = getelementptr inbounds [128 x i32], [128 x i32]* %a, i64 0, i64 0