This is an archive of the discontinued LLVM Phabricator instance.

[StackProtector] Ignore certain intrinsics when calculating sspstrong heuristic.
ClosedPublic

Authored by mattd on Apr 5 2018, 11:33 AM.

Details

Summary

The 'strong' StackProtector heuristic takes into consideration call instructions.
Certain intrinsics, such as lifetime.start, can cause the
StackProtector to protect functions that do not need to be protected.

Specifically, a volatile variable, (not optimized away), but belonging to a stack
allocation will encourage a llvm.lifetime.start to be inserted during
compilation. Because that intrinsic is a 'call' the strong StackProtector
will see that the alloca'd variable is being passed to a call instruction, and
insert a stack protector. In this case the intrinsic isn't really lowered to a
call. This can cause unnecessary stack checking, at the cost of additional
(wasted) CPU cycles.

In the future we should rely on TargetTransformInfo::isLoweredToCall, but as of
now that routine considers all intrinsics as not being lowerable. That needs
to be corrected, and such a change is on my list of things to get moving on.

As a side note, the updated stack-protector-dbginfo.ll test always seems to
pass. I never see the dbg.declare/dbg.value reaching the
StackProtector::HasAddressTaken, but I don't see any code excluding dbg
intrinsic calls either, so I think it's the safest thing to do.

Diff Detail

Event Timeline

mattd created this revision.Apr 5 2018, 11:33 AM

In the future we should rely on TargetTransformInfo::isLoweredToCall, but as of

now that routine considers all intrinsics as not being lowerable.

Can we just add lifetime intrinsics support into TTI::isLoweredToCall() ? Will it break other things? If it's doable, then the stackprotector just needs to call isLoweredToCall().

mattd added a comment.Apr 6 2018, 8:23 AM

In the future we should rely on TargetTransformInfo::isLoweredToCall, but as of
now that routine considers all intrinsics as not being lowerable.

Can we just add lifetime intrinsics support into TTI::isLoweredToCall() ? Will it break other things? If it's doable, then the stackprotector just needs to call isLoweredToCall().

I'm hesitant to modify isLoweredToCall right now, as I think it will need more work to decide whether a particular intrinsic is lowerable. For now, I figure we just put a
check to avoid counting the lifetime/debug intrinsics in this heuristic. It seems checks similar to the ones in this patch are pretty common throughout the codebase, at least for ignoring debug intrinsics.

timshen accepted this revision.Apr 6 2018, 11:23 AM

Sounds good, as it is not hard to track down and inspect all lifetime intrinsic heuristics in the future for using the improved isLoweredToCall().

This revision is now accepted and ready to land.Apr 6 2018, 11:23 AM
This revision was automatically updated to reflect the committed changes.