[PATCH] D40622: XOR the frame pointer with the stack cookie when protecting the stack

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

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



================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:2136
+  const TargetRegisterInfo *TRI = DAG.getSubtarget().getRegisterInfo();
+  unsigned FrameReg = TRI->getFrameRegister(MF);
+  MVT PtrVT = TLI.getPointerTy(DAG.getDataLayout(), 0);
----------------
hans wrote:
> 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.
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
```


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:1692
+  // Currently only MSVC CRTs XOR the frame pointer into the stack guard value.
+  return Subtarget.getTargetTriple().isOSMSVCRT();
+}
----------------
hans wrote:
> 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?
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.


https://reviews.llvm.org/D40622