Page MenuHomePhabricator

[PEI] Don't re-allocate a pre-allocated stack protector slot
ClosedPublic

Authored by thegameg on Jul 15 2019, 10:10 AM.

Details

Summary

The LocalStackSlotPass pre-allocates a stack protector and makes sure that it comes before the local variables on the stack.

We need to make sure that later during PEI we don't re-allocate a new stack protector slot. If that happens, the new stack protector slot will end up being after the local variables that it should be protecting.

Therefore, we would have two slots assigned for two different stack protectors, one at the top of the stack, and one at the bottom. Since PEI will overwrite the assigned slot for the stack protector, the load that is used to compare the value of the stack protector will use the slot assigned by PEI, which is wrong.

For this, we need to check if the object is pre-allocated, and re-use that pre-allocated slot.

Diff Detail

Repository
rL LLVM

Event Timeline

thegameg created this revision.Jul 15 2019, 10:10 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 15 2019, 10:10 AM
efriedma added inline comments.
llvm/lib/CodeGen/LocalStackSlotAllocation.cpp
202 ↗(On Diff #209896)

Is this change a no-op?

llvm/lib/CodeGen/PrologEpilogInserter.cpp
932 ↗(On Diff #209896)

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.

thegameg marked 2 inline comments as done.Jul 15 2019, 1:45 PM
thegameg added inline comments.
llvm/lib/CodeGen/LocalStackSlotAllocation.cpp
202 ↗(On Diff #209896)

Yes, it just adds the assert.

llvm/lib/CodeGen/PrologEpilogInserter.cpp
932 ↗(On Diff #209896)

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)
aadg added inline comments.Jul 15 2019, 1:47 PM
llvm/lib/CodeGen/LocalStackSlotAllocation.cpp
202 ↗(On Diff #209896)

Yes, this change is a no-op.

efriedma added inline comments.Jul 15 2019, 2:20 PM
llvm/lib/CodeGen/PrologEpilogInserter.cpp
932 ↗(On Diff #209896)

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.

thegameg marked 2 inline comments as done.Jul 15 2019, 3:02 PM
thegameg added inline comments.
llvm/lib/CodeGen/PrologEpilogInserter.cpp
932 ↗(On Diff #209896)

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).

efriedma added inline comments.Jul 15 2019, 3:23 PM
llvm/lib/CodeGen/PrologEpilogInserter.cpp
932 ↗(On Diff #209896)

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.

thegameg updated this revision to Diff 209993.Jul 15 2019, 4:33 PM
  • Move NFC changes into a separate commit
  • In PEI, verify that LocalStackSlotPass assigned the stack protector and the protected stack objects
thegameg marked 5 inline comments as done.Jul 15 2019, 4:34 PM

This will fix https://bugs.llvm.org/show_bug.cgi?id=25610, so it would be good if we could have a test for that as well (and to close that bug once this is committed).

This bug also affects the arm and powerpc targets (as they also return true for TargetRegisterInfo::requiresVirtualBaseRegisters), so it would be good to have tests for those as well. An example which affects all three targets (with -fomit-frame-pointer) is

#include <string.h>
#include <stdio.h>

int fn(const char *str) {
  char buffer[65536];
  strcpy(buffer, str);
  puts(buffer);
  return buffer[65535];
}
This revision is now accepted and ready to land.Jul 16 2019, 11:42 AM
thegameg updated this revision to Diff 210153.Jul 16 2019, 1:05 PM
  • Add common test for ARM, AArch64 and PowerPC in previous commit
  • Update tests with the modifications coming from this patch
thegameg updated this revision to Diff 210164.Jul 16 2019, 1:37 PM

Add test from PR25610.

john.brawn accepted this revision.Jul 17 2019, 3:19 AM

Tests look good to me.

aadg accepted this revision.Jul 17 2019, 8:37 AM

LGTM as well.

This revision was automatically updated to reflect the committed changes.