[PATCH] D64757: [PEI] Don't re-allocate a pre-allocated stack protector slot

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/CodeGen/PrologEpilogInserter.cpp:932
+  // LocalStackSlotPass didn't already allocate a slot for it. If it did, we
+  // need to use that one.
+  if (MFI.hasStackProtectorIndex() &&
----------------
thegameg wrote:
> efriedma wrote:
> > thegameg wrote:
> > > efriedma wrote:
> > > > We obviously don't want to re-allocate a slot for the stack protector, but what about the other stack objects?  Do we guarantee that LocalStackSlotPass will allocate all obejcts where getObjectSSPLayout() is not SSPLK_None?  If we do, we can just simplify this check to `if (MFI.getUseLocalStackAllocationBlock())`.  Otherwise, we should still run this code for all the all the non-stack-protector frame indexes.
> > > `LocalStackSlotPass` works in two steps:
> > > 
> > > 1) Assign offsets to all stack objects except objects used for saving callee saved registers in `MFI.LocalFrameObjects` . These offsets will be **potentially** used by PEI by sliding them to an actual offset after allocating slots for other things like CSRs.
> > > 2) Replace `FrameIndex` operands with a vreg operand (itself defined by a FI + offset) + offset operand
> > > 
> > > `1)` always runs, but if `2)` does not rewrite any FI operands, it does not set `MFI.UseLocalStackAllocationBlock`, and PEI will not use anything from `MFI.LocalFrameObjects`, and it will instead re-allocate everything in `MFI.Objects` as if the pass did not even run. In this specific case, `1)` runs and sets `MFI.StackProtectorIndex` (which is "global"), but only assigns the offsets of local variables in `MFI.LocalFrameObjects`.
> > > 
> > > `AdjustStackOffset` from `LocalStackSlotPass` uses
> > > ```
> > > MFI.mapLocalFrameObject(FrameIdx, LocalOffset);
> > > ```
> > > 
> > > `AdjustStackOffset` from `PEI` uses
> > > ```
> > > MFI.setObjectOffset(FrameIdx, Offset)
> > > ```
> > > 
> > I guess I commented too fast with my previous review comment; the check here doesn't simplify quite the way I suggested.  It only simplifies to `if (MFI.hasStackProtectorIndex() && !MFI.getUseLocalStackAllocationBlock())`.  And then we can get rid of the `if (MFI.isObjectPreAllocated(i) && MFI.getUseLocalStackAllocationBlock())` check, because `getUseLocalStackAllocationBlock()` is never true inside the if statement.
> > 
> > The question I was trying to ask was, is the additional guard for the calls to AssignProtectedObjSet a no-op in practice?  I don't think you addressed that at all.
> Yes, please ignore my previous answer, that was confusing on my part.
> 
> `Do we guarantee that LocalStackSlotPass will allocate all obejcts where getObjectSSPLayout() is not SSPLK_None?`
> I'm not sure we guarantee it, but I believe this is what is happening right now. We could maybe assert if it's not true? FWIW, I think we should guarantee it, since the order actually matters based on the type, and if PEI allocates some of them, the objects won't be sorted as expected.
> 
> `It only simplifies to if (MFI.hasStackProtectorIndex() && !MFI.getUseLocalStackAllocationBlock()).`
> Right, `PreAllocated` should not matter if `MFI.UseLocalStackAllocationBlock` is not set.
> 
> `And then we can get rid of the if (MFI.isObjectPreAllocated(i) && MFI.getUseLocalStackAllocationBlock()) check`
> In that case, I agree, we could remove the check inside the loop.
> 
> `is the additional guard for the calls to AssignProtectedObjSet a no-op in practice?`
> I would expect the sets to be empty if `MFI.UseLocalStackAllocationBlock` is set (if the assumptions from the first question are true).
> 
> FWIW, I think we should guarantee it, since the order actually matters based on the type, and if PEI allocates some of them, the objects won't be sorted as expected.

This makes sense.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64757/new/

https://reviews.llvm.org/D64757