This is an archive of the discontinued LLVM Phabricator instance.

[SSP, 2/2] Create llvm.stackguard() intrinsic and lower it to LOAD_STACK_GUARD
ClosedPublic

Authored by timshen on Apr 14 2016, 4:41 PM.

Details

Summary

With this change, ideally IR pass can always generate llvm.stackguard
call to get the stack guard; but for now there are still IR form stack
guard customizations around (see getIRStackGuard()). Future SSP
customization should go through LOAD_STACK_GUARD.

There is a behavior change: stack guard values are not CSEed anymore,
since we should never reuse the value in case that it has been spilled (and
corrupted). See ssp-guard-spill.ll. This also cause the change of stack size and codegen in X86 and AArch64 test cases.

The solution above isn't ideal. Ideally we'd like to know if the guard created in llvm.stackprotector() gets
spilled or not. If the value is spilled, discard the value and reload
stack guard; otherwise reuse the value. This can be done by teaching
register allocator to know how to rematerialize LOAD_STACK_GUARD and
force a rematerialization (which seems hard), or check for spilling in
expandPostRAPseudo. It only makes sense when the stack guard is a global
variable, which requires more instructions to load. Anyway, this seems to go out
of the scope of the current patch.

Diff Detail

Event Timeline

timshen updated this revision to Diff 53810.Apr 14 2016, 4:41 PM
timshen retitled this revision from to [SSP, 2/2] Create llvm.stackguard() intrinsic and lower it to LOAD_STACK_GUARD.
timshen updated this object.
timshen added reviewers: echristo, iteratee, gottesmm.
timshen added a subscriber: llvm-commits.
echristo edited edge metadata.Apr 18 2016, 2:32 PM

Can you add in the patch description explanations of the changes in stack size and code generation?

A few inline comments as well.

Thanks!

-eric

lib/CodeGen/StackProtector.cpp
282–284

This looks weird, can you explain it?

test/CodeGen/AArch64/stack-guard-remat-bitcast.ll
9

This looks like what I'd expect, but can you explain why the test changed?

test/CodeGen/X86/ssp-guard-spill.ll
4

Add comments of what you're testing here.

timshen updated this revision to Diff 54125.Apr 18 2016, 3:16 PM
timshen marked 3 inline comments as done.
timshen edited edge metadata.

Updated comments.

timshen updated this object.Apr 18 2016, 3:18 PM
echristo accepted this revision.Apr 18 2016, 11:51 PM
echristo edited edge metadata.

I'm going to go ahead and approve this. It's definitely going down the path we want to go, fixes some fairly problematic issues in a nice way, provides an extension point, fixes a bit of a bug, and points out the way forward.

Thanks for all the work Tim!

-eric

This revision is now accepted and ready to land.Apr 18 2016, 11:51 PM
timshen closed this revision.Apr 19 2016, 12:47 PM

Thanks Eric for review!

This is committed as r266806.