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

[Reid Kleckner via Phabricator via llvm-commits]

rnk added inline comments.


================
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:
> rnk wrote:
> > 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
> > ```
> 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.
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...


https://reviews.llvm.org/D40622