Page MenuHomePhabricator

[safestack] Move allocations to support separate stack segment
AcceptedPublic

Authored by mlemay-intel on Feb 10 2016, 1:25 PM.

Details

Summary

When some option is enabled that necessitates using only local stack
pointers, move allocations to the unsafe stack to help satisfy that
requirement. The only option of this type that is currently supported
is the separate stack segment option for x86-32.

Diff Detail

Event Timeline

mlemay-intel retitled this revision from to [safestack] Add -safe-stack-subr-acc-as-unsafe option.
mlemay-intel updated this object.
eugenis added inline comments.Feb 10 2016, 3:57 PM
lib/CodeGen/SafeStack.cpp
64

This is hard to read, please change to something like safe-stack-subroutine-access-unsafe.

mlemay-intel added inline comments.Feb 10 2016, 4:40 PM
lib/CodeGen/SafeStack.cpp
64

That sounds good. I'll wait for any additional comments you may have on the patches so I can batch this in with other revisions, if you don't mind.

qcolombet edited edge metadata.Feb 18 2016, 9:12 AM

Please add ll-commit as a subscriber as well.

Thanks,
-Quentin

mlemay-intel edited edge metadata.Feb 18 2016, 9:29 AM
mlemay-intel added a subscriber: llvm-commits.

FWIW, LGTM but please address Eugenis’ comment.

I believe Eugenis is better suited to give the final LGTM.

Cheers,
-Quentin

pcc added a subscriber: pcc.Mar 15 2016, 12:08 PM

Regarding the first motivation, the code is already being conservative. As far as I know, the nocapture readnone attributes are stronger than what safestack requires of a pointer. For example, they forbid loading from a pointer at a constant offset (which safestack could conceivably allow). Rather than adding this flag, we should in the long term be adding an attribute that specifically means what safestack requires.

Regarding the second motivation, if we do intend to start using segment selectors on x86 in some cases, we should probably make this behavior gated on the target (e.g. the separate-stack-seg feature you appear to be adding) rather than on a flag.

In D17094#375794, @pcc wrote:

I apologize for not responding to this until now. Email notifications of new activity on my patches are apparently not making it to my inbox for some reason.

Regarding the first motivation, the code is already being conservative. As far as I know, the nocapture readnone attributes are stronger than what safestack requires of a pointer. For example, they forbid loading from a pointer at a constant offset (which safestack could conceivably allow). Rather than adding this flag, we should in the long term be adding an attribute that specifically means what safestack requires.

Thank you for pointing out my misinterpretation of the FIXME. I will update the commit message accordingly.

Regarding the second motivation, if we do intend to start using segment selectors on x86 in some cases, we should probably make this behavior gated on the target (e.g. the separate-stack-seg feature you appear to be adding) rather than on a flag.

This would result in simpler command line option sequences. I'll try to revise this patch to do what you suggested.

Check for the separate-stack-seg feature instead of defining a new command line option.

pcc added a comment.Apr 15 2016, 11:37 AM

Please update the commit message and add a test case.

mlemay-intel retitled this revision from [safestack] Add -safe-stack-subr-acc-as-unsafe option to [safestack] Move allocations to support separate stack segment.Apr 15 2016, 11:44 AM
mlemay-intel updated this object.

Add test. Do not move allocations to unsafe stack simply due to them being passed to lifetime intrinsics.

pcc accepted this revision.Apr 15 2016, 1:09 PM
pcc added a reviewer: pcc.

LGTM

This revision is now accepted and ready to land.Apr 15 2016, 1:09 PM