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

[Francis Visoiu Mistrih via Phabricator via llvm-commits]

thegameg marked 2 inline comments as done.
thegameg 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() &&
----------------
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).



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

https://reviews.llvm.org/D64757