[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:
> > 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.


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

https://reviews.llvm.org/D64757