This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Make sure the first probe is full size or is the last probe when stack is realigned
ClosedPublic

Authored by lkail on Apr 12 2021, 2:08 AM.

Details

Summary

When -fstack-clash-protection is enabled and stack has to be realigned, some parts of redzone is written prior the probe, so probe might overwrite content already written in redzone. To avoid it, we have to make sure the first probe is at full probe size or is the last probe so that we can skip redzone.

It also fixes violation of ABI under PPC where r1 isn't updated atomically.

This fixes https://bugs.llvm.org/show_bug.cgi?id=49903.

Diff Detail

Event Timeline

lkail created this revision.Apr 12 2021, 2:08 AM
lkail requested review of this revision.Apr 12 2021, 2:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 12 2021, 2:08 AM
lkail updated this revision to Diff 336784.Apr 12 2021, 2:30 AM
lkail updated this revision to Diff 336785.Apr 12 2021, 2:43 AM
lkail planned changes to this revision.EditedApr 14 2021, 7:45 PM

Current solution hasn't guaranteed back-chain pointer is updated atomically, I have another solution for this to guarantee safety and atomic.

tstellar added a subscriber: tstellar.
lkail updated this revision to Diff 337648.Apr 15 2021, 12:57 AM
lkail retitled this revision from [PowerPC] Don't probe in redzone to [PowerPC] Make sure the first probe is full size or the last probe.
lkail edited the summary of this revision. (Show Details)
lkail added inline comments.Apr 15 2021, 12:59 AM
llvm/test/CodeGen/PowerPC/stack-clash-prologue.ll
681

Old implementation doesn't make backchain pointer updated atomically, violating ABI.

lkail retitled this revision from [PowerPC] Make sure the first probe is full size or the last probe to [PowerPC] Make sure the first probe is full size or is the last probe.Apr 15 2021, 1:10 AM

What is the status of this patch? It fixes a blocking bug for the 12.0.1 release.

What is the status of this patch? It fixes a blocking bug for the 12.0.1 release.

@jsji Do you have a bit of time to review this? If not, we might have to mark the bug as a non-blocker for the release and accept that stack clash protection has this issue in 12.0.0 and 12.0.1. I am hoping we can get this done and backported.

jsji added a comment.Jun 4 2021, 8:30 PM

This somehow was missed in my radar.. I will have a look .

lkail edited the summary of this revision. (Show Details)Jun 5 2021, 4:26 AM
lkail updated this revision to Diff 350086.EditedJun 5 2021, 9:06 PM
  1. Adjust comments
  2. Swap usage of r0 and r12
  3. Shortened liverange of FinalStackPtr
lkail added inline comments.Jun 5 2021, 9:14 PM
llvm/test/CodeGen/PowerPC/stack-clash-prologue.ll
86

Looks like a bug... I'll check it.

lkail updated this revision to Diff 350091.Jun 6 2021, 1:50 AM
jsji added a comment.Jun 7 2021, 10:38 AM

Looks mostly good to me, but I would prefer we split this into two fixes, one for the realign bp algorithm change, another for the other.

llvm/lib/Target/PowerPC/PPCFrameLowering.cpp
1317–1318

*that* -> *when*?

1317–1318

maybe rename dynamicProbeto something like dynamicProbe_RealignWithBP as this is only called with HasBP && MaxAlign >1?

1318

Add the comments about atomic update required by ABI.

1322

This is inconsistent of what we generate below: using stdu <backchain>, <negprobesize>(r1)?

1325

If we use stdux above in pseudo code, shouldn't we use stdux here?

1326

Reorg this comment please. HasBP is always true, so this is really the difference for isPPC64 vs isPPC32?

1341–1356

This looks like a new limitation? Can we add a FIXME here?

1347

Do you mean we can use xform if we have an additional idle register?

1351

*materialized* -> *materializable*?

1352

Do we still need this lambda? Seems like only a ternary operator here?

1383

FPReg? Typo? TempReg?

1411

true /*UseDForm*/

1519

Split change related to this into another patch?

lkail updated this revision to Diff 350482.Jun 7 2021, 9:04 PM
lkail retitled this revision from [PowerPC] Make sure the first probe is full size or is the last probe to [PowerPC] Make sure the first probe is full size or is the last probe when stack is realigned.

Address comments.

lkail marked 8 inline comments as done.Jun 7 2021, 9:06 PM
lkail added inline comments.
llvm/lib/Target/PowerPC/PPCFrameLowering.cpp
1326

Should be HasBP && HasRedzone, I have updated the condition.

1347

Exactly.

1519

Yes, we can do it later.

jsji accepted this revision as: jsji.Jun 8 2021, 9:04 AM

LGTM , with some comments on comments.

llvm/lib/Target/PowerPC/PPCFrameLowering.cpp
1330

How about?

When HasBP & HasRedzone, back-chain pointer is already saved in BPReg before probe code, we don't need to save it, so we get one additional reg that can be used to materialize the probeside if needed to use xform.
Otherwise, we can NOT materialize probeside, so we can only use Dform for now. 

The allocations are:
if(HasBP & HasRedzone){
r0: materialize the probesize if needed so that we can use xform.
r12: `neg_gap`
}
else{
r0: back-chain pointer
r12: `neg_gap`.
}
1337

probeStackWhenRealigned -> probeRealignedStack

1352

probeStackWhenRealigned is called only under if (HasBP && MaxAlign > 1) { ,
so the HasBP should be expected to be always true in this function,
I think we can safely remove it from the condition? (you can add an assert if you want).

1352

we should have a idle register to materialize -> we can use xform if we have an additional idle register

1382

PROBED_STACKALLOC_64 assumes Operand(1) stores the old sp, copy BPReg to TempReg to satisfy it.

llvm/test/CodeGen/PowerPC/stack-clash-prologue-nounwind.ll
337

Looks like this can be ignored if we found the imm is 0. Can be fixed in a follow up patch.

This revision is now accepted and ready to land.Jun 8 2021, 9:04 AM
lkail updated this revision to Diff 350764.Jun 8 2021, 7:52 PM
lkail marked 5 inline comments as done.
lkail added inline comments.
llvm/test/CodeGen/PowerPC/stack-clash-prologue-nounwind.ll
337

Good catch.

lkail updated this revision to Diff 350778.Jun 8 2021, 10:08 PM

Try to fix pre-merge build failure on windows.

This revision was landed with ongoing or failed builds.Jun 8 2021, 11:35 PM
This revision was automatically updated to reflect the committed changes.