Add support to clang-cl driver for /GS switch

[etienneb]

Summary

This patch is adding command-line support for the MSVC buffer security check.

The buffer security check is turned on with the '/GS' compiler switch.
https://msdn.microsoft.com/en-us/library/8dbf701c.aspx

The MSVC buffer security check in implemented here:
http://reviews.llvm.org/D20346

[etienneb]

nits

[etienneb]

revert

[etienneb]

turn /GS on by default

[hans]

The approach looks good, but please add tests (probably in test/Driver/cl-options.c).

include/clang/Driver/CLCompatOptions.td

81 ↗

(On Diff #58460)

The description is not super informative, but it does match the MSVC description (except in the command-line options listing they say "Buffers security check".

Would it make more sense to call it something like stack canary, cookie, protector?

lib/Driver/Tools.cpp

9990 ↗

(On Diff #58460)

Why is this needed?

[etienneb]

fix hans@ comments

[etienneb]

add unittests

[etienneb]

added unittests. Should be fine now.

lib/Driver/Tools.cpp

9990 ↗

(On Diff #58460)

copy paste bug!
Good catch.

[hans]

Please also add a test in test/Driver/cl-options.c which tests that /GS causes the correct flag to be passed to the cc1 invocation, that it happens by default, and that /GS- makes it go away.

[etienneb]

more tests

[hans]

lgtm with comments addressed

test/Driver/cl-options.c

62 ↗

(On Diff #58497)

I'd suggest a test that checks that it's on by default too. You can reuse the same -check-prefix:

// RUN: %clang_cl -### -- %s 2>&1 | FileCheck -check-prefix=GS %s

Also, shouldn't the checks below be for "--stack-protector 2" or something, since we're enabling the strong level?

[etienneb]

more tests

[etienneb]

This patch needs land after http://reviews.llvm.org/D20346.
thx for the review.

[hans]

Is this waiting for anything more now that http://reviews.llvm.org/D20346 has landed?

[thakis]

probably at least the "the XOR with RSP/EBP/ESP" bit still (and maybe EH function upgrades instead of bailing)

lib/Driver/Tools.cpp

9990 ↗

(On Diff #58475)

To pass on /GS- to the fallback compiler when needed, I suppose.

[rnk]

lgtm

Surely this is going to break something, but let's throw the switch and find out.

include/llvm/Target/TargetLowering.h

public:
   /// 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;
 
+  /// Return the function that's previously inserted by insertSSPDeclarations,
+  /// if any, otherwise return nullptr. The returned function needs to be
+  /// called to check the stack protection.
+  /// If nullptr, the stack validation is inlined before each return
+  /// instructions.
+  /// Should be used only when getIRStackGuard returns nullptr.
+  virtual Value *getSDagStackCheck(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:
   ///        BB. Instead, we just initialize the stack protector descriptor
   ///        class (which involves stashing information/creating the success
   ///        mbbb and the failure mbb if we have not created one for this
   ///        function yet) and export the guard variable that we are going to
   ///        compare.
   ///
   ///     c. After we finish selecting the basic block, in FinishBasicBlock if
   ///        the StackProtectorDescriptor attached to the SelectionDAGBuilder is
-  ///        initialized, we first find a splice point in the parent basic block
+  ///        initialized, we produce the instrumentation code with one of this
+  ///        instrumentation:
+  ///          1) with a stack check function
+  ///          2) with inlined instrumentation
+  ///
+  ///        1) We insert a call to the check function before the terminator.
+  ///
+  ///        2) We first find a splice point in the parent basic block
   ///        before the terminator and then splice the terminator of said basic
   ///        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) {}
 
     /// 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;
     }
 
+    bool shouldEmitFunctionCheckStackProtector() const {
+      return ParentMBB && !SuccessMBB && !FailureMBB;
+    }
+
     /// Initialize the stack protector descriptor structure for a new basic
     /// block.
-    void initialize(const BasicBlock *BB, MachineBasicBlock *MBB) {
+    void initialize(const BasicBlock *BB, MachineBasicBlock *MBB,
+                    bool InlineInstrumentation) {
       // Make sure we are not initialized yet.
       assert(!shouldEmitStackProtector() && "Stack Protector Descriptor is "
              "already initialized!");
       ParentMBB = MBB;
+      if (InlineInstrumentation) {
         SuccessMBB = AddSuccessorMBB(BB, MBB, /* IsLikely */ true);
         FailureMBB = AddSuccessorMBB(BB, MBB, /* IsLikely */ false, FailureMBB);
       }
+    }
 
     /// 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).
     ///

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

void SelectionDAGBuilder::visitSPDescriptorParent(StackProtectorDescriptor &SPD,
   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()));
 
+  SDValue StackSlot = DAG.getLoad(
+      PtrTy, dl, DAG.getEntryNode(), StackSlotPtr,
+      MachinePointerInfo::getFixedStack(DAG.getMachineFunction(), FI), true,
+      false, false, Align);
+
+  // Retrieve guard check function, nullptr if instrumentation is inlined.
+  if (const Value *GuardCheck = TLI.getSDagStackCheck(M)) {
+    // Emit a call to the stack protection check function.
+    TargetLowering::ArgListTy Args;
+    TargetLowering::ArgListEntry Entry;
+    Entry.Node = StackSlot;
+    Entry.Ty = Type::getInt8PtrTy(M.getContext());
+    Entry.isInReg = true;
+    Args.push_back(Entry);
+    TargetLowering::CallLoweringInfo CLI(DAG);
+    CLI.setChain(DAG.getEntryNode())
+      .setCallee(CallingConv::X86_FastCall, Type::getVoidTy(M.getContext()),
+                 getValue(GuardCheck), std::move(Args));
+
+    std::pair<SDValue, SDValue> Result = TLI.LowerCallTo(CLI);
+    DAG.setRoot(Result.second);
+    return;
+  }
+
   // If useLoadStackGuardNode returns true, generate LOAD_STACK_GUARD.
   // Otherwise, emit a volatile load to retrieve the stack guard value.
   SDValue Chain = DAG.getEntryNode();
   if (TLI.useLoadStackGuardNode()) {
     Guard = getLoadStackGuard(DAG, dl, Chain);
   } else {
     const Value *IRGuard = TLI.getSDagStackGuard(M);
     SDValue GuardPtr = getValue(IRGuard);
 
     Guard =
         DAG.getLoad(PtrTy, dl, Chain, GuardPtr, MachinePointerInfo(IRGuard, 0),
                     true, false, false, Align);
   }
 
-  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);
 
   SDValue Cmp = DAG.getSetCC(dl, TLI.getSetCCResultType(DAG.getDataLayout(),
                                                         *DAG.getContext(),
                                                         Sub.getValueType()),
                              Sub, DAG.getConstant(0, dl, VT), ISD::SETNE);

lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp

#endif
       FastIS->recomputeInsertPt();
     } else {
       // Lower any arguments needed in this block if this is the entry block.
       if (LLVMBB == &Fn.getEntryBlock()) {
         ++NumEntryBlocks;
         LowerArguments(Fn);
       }
     }
-    if (getAnalysis<StackProtector>().shouldEmitSDCheck(*LLVMBB))
-      SDB->SPDescriptor.initialize(LLVMBB, FuncInfo->MBBMap[LLVMBB]);
+    if (getAnalysis<StackProtector>().shouldEmitSDCheck(*LLVMBB)) {
+      bool InlineInstrumentation = !TLI->getSDagStackCheck(*Fn.getParent());
+      SDB->SPDescriptor.initialize(LLVMBB, FuncInfo->MBBMap[LLVMBB],
+                                   InlineInstrumentation);
+    }
 
     if (Begin != BI)
       ++NumDAGBlocks;
     else
       ++NumFastIselBlocks;
 
     if (Begin != BI) {
       // Run SelectionDAG instruction selection on the remainder of the block

 /// blocks. Luckily, selectiondag always moves physical registers into vregs
 /// when they enter functions and moves them through a sequence of copies back
 /// into the physical registers right before the terminator creating a
 /// ``Terminator Sequence''. This function is searching for the beginning of the
 /// terminator sequence so that we can ensure that we splice off not just the
 /// terminator, but additionally the copies that move the vregs into the
 /// physical registers.
 static MachineBasicBlock::iterator
-FindSplitPointForStackProtector(MachineBasicBlock *BB, DebugLoc DL) {
+FindSplitPointForStackProtector(MachineBasicBlock *BB) {
   MachineBasicBlock::iterator SplitPoint = BB->getFirstTerminator();
   //
   if (SplitPoint == BB->begin())
     return SplitPoint;
 
   MachineBasicBlock::iterator Start = BB->begin();
   MachineBasicBlock::iterator Previous = SplitPoint;
   --Previous;
for (unsigned i = 0, e = FuncInfo->PHINodesToUpdate.size(); i != e; ++i) {
     assert(PHI->isPHI() &&
            "This is not a machine PHI node that we are updating!");
     if (!FuncInfo->MBB->isSuccessor(PHI->getParent()))
       continue;
     PHI.addReg(FuncInfo->PHINodesToUpdate[i].second).addMBB(FuncInfo->MBB);
   }
 
   // Handle stack protector.
-  if (SDB->SPDescriptor.shouldEmitStackProtector()) {
+  if (SDB->SPDescriptor.shouldEmitFunctionCheckStackProtector()) {
+    MachineBasicBlock *ParentMBB = SDB->SPDescriptor.getParentMBB();
+
+    // Add load and check to the basicblock.
+    FuncInfo->MBB = ParentMBB;
+    FuncInfo->InsertPt = //ParentMBB->getFirstInstrTerminator();
+        FindSplitPointForStackProtector(ParentMBB);
+    SDB->visitSPDescriptorParent(SDB->SPDescriptor, ParentMBB);
+    CurDAG->setRoot(SDB->getRoot());
+    SDB->clear();
+    CodeGenAndEmitDAG();
+
+    // Clear the Per-BB State.
+    SDB->SPDescriptor.resetPerBBState();
+  } else if (SDB->SPDescriptor.shouldEmitStackProtector()) {
     MachineBasicBlock *ParentMBB = SDB->SPDescriptor.getParentMBB();
     MachineBasicBlock *SuccessMBB = SDB->SPDescriptor.getSuccessMBB();
 
     // Find the split point to split the parent mbb. At the same time copy all
     // physical registers used in the tail of parent mbb into virtual registers
     // before the split point and back into physical registers after the split
     // point. This prevents us needing to deal with Live-ins and many other
     // register allocation issues caused by us splitting the parent mbb. The
     // register allocator will clean up said virtual copies later on.
     MachineBasicBlock::iterator SplitPoint =
-      FindSplitPointForStackProtector(ParentMBB, SDB->getCurDebugLoc());
+        FindSplitPointForStackProtector(ParentMBB);
 
     // Splice the terminator of ParentMBB into SuccessMBB.
     SuccessMBB->splice(SuccessMBB->end(), ParentMBB,
                        SplitPoint,
                        ParentMBB->end());
 
     // Add compare/jump on neq/jump to the parent BB.
     FuncInfo->MBB = ParentMBB;

lib/CodeGen/TargetLoweringBase.cpp

for (MVT VT : MVT::all_valuetypes()) {
     // These operations default to expand for vector types.
     if (VT.isVector()) {
       setOperationAction(ISD::FCOPYSIGN, VT, Expand);
       setOperationAction(ISD::ANY_EXTEND_VECTOR_INREG, VT, Expand);
       setOperationAction(ISD::SIGN_EXTEND_VECTOR_INREG, VT, Expand);
       setOperationAction(ISD::ZERO_EXTEND_VECTOR_INREG, VT, Expand);
     }
 
-    // For most targets @llvm.get.dynamic.area.offest just returns 0.
+    // For most targets @llvm.get.dynamic.area.offset just returns 0.
     setOperationAction(ISD::GET_DYNAMIC_AREA_OFFSET, VT, Expand);
   }
 
   // Most targets ignore the @llvm.prefetch intrinsic.
   setOperationAction(ISD::PREFETCH, MVT::Other, Expand);
 
   // Most targets also ignore the @llvm.readcyclecounter intrinsic.
   setOperationAction(ISD::READCYCLECOUNTER, MVT::i64, Expand);
void TargetLoweringBase::insertSSPDeclarations(Module &M) const {
   M.getOrInsertGlobal("__stack_chk_guard", Type::getInt8PtrTy(M.getContext()));
 }
 
 // Currently only support "standard" __stack_chk_guard.
 // TODO: add LOAD_STACK_GUARD support.
 Value *TargetLoweringBase::getSDagStackGuard(const Module &M) const {
   return M.getGlobalVariable("__stack_chk_guard");
 }
+
+Value *TargetLoweringBase::getSDagStackCheck(const Module &M) const {
+  return nullptr;
+}

lib/Target/X86/X86ISelLowering.h

public:
     /// or null if the target does not support "fast" ISel.
     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;
+
+    // These methods need to be overriden to implement Stack-Protection
+    // with the SelectionDAGIsel mechanism.
     void insertSSPDeclarations(Module &M) const override;
+    Value *getSDagStackGuard(const Module &M) const override;
+    Value *getSDagStackCheck(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;
 
     bool isNoopAddrSpaceCast(unsigned SrcAS, unsigned DestAS) const override;
 
-    bool useLoadStackGuardNode() 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;
 
     bool supportSwiftError() const override {
       return true;
     }

lib/Target/X86/X86ISelLowering.cpp

Value *X86TargetLowering::getIRStackGuard(IRBuilder<> &IRB) const {
   unsigned Offset = (Subtarget.is64Bit()) ? 0x28 : 0x14;
   unsigned AddressSpace = getAddressSpace();
   return ConstantExpr::getIntToPtr(
       ConstantInt::get(Type::getInt32Ty(IRB.getContext()), Offset),
       Type::getInt8PtrTy(IRB.getContext())->getPointerTo(AddressSpace));
 }
 
 void X86TargetLowering::insertSSPDeclarations(Module &M) const {
-  if (!Subtarget.isTargetGlibc())
+  // MSVC CRT has a global variable holding security cookie.
+  if (Subtarget.getTargetTriple().isOSMSVCRT()) {
+    M.getOrInsertGlobal("__security_cookie",
+                        Type::getInt8PtrTy(M.getContext()));
+
+    auto* SecurityCheckCookie = cast<Function>(
+         M.getOrInsertFunction("__security_check_cookie",
+                               Type::getVoidTy(M.getContext()),
+                               Type::getInt8PtrTy(M.getContext()), nullptr));
+    SecurityCheckCookie->setCallingConv(CallingConv::X86_FastCall);
+    return;
+  }
+  // glibc has a special slot for the stack guard.
+  if (Subtarget.isTargetGlibc())
+    return;
     TargetLowering::insertSSPDeclarations(M);
 }
 
+Value *X86TargetLowering::getSDagStackGuard(const Module &M) const {
+  // MSVC CRT has a global variable holding security cookie.
+  if (Subtarget.getTargetTriple().isOSMSVCRT())
+    return M.getGlobalVariable("__security_cookie");
+  return TargetLowering::getSDagStackGuard(M);
+}
+
+Value *X86TargetLowering::getSDagStackCheck(const Module &M) const {
+  // MSVC CRT provides a function to validate security cookie.
+  if (Subtarget.getTargetTriple().isOSMSVCRT())
+    return M.getFunction("__security_check_cookie");
+  return TargetLowering::getSDagStackCheck(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
   // https://android.googlesource.com/platform/bionic/+/master/libc/private/bionic_tls.h
   unsigned AddressSpace, Offset;