This is an archive of the discontinued LLVM Phabricator instance.

Android support for SafeStack
ClosedPublic

Authored by eugenis on Sep 17 2015, 5:31 PM.

Details

Reviewers
pcc
echristo
Summary

Add two new ways of accessing the unsafe stack pointer:

  • at a fixed offset from the thread TLS base. This is very similar to StackProtector cookies, but we plan to extend it to other backends (ARM in particular) soon. Bionic-side implementation here: https://android-review.googlesource.com/170988
  • via a function call, as a fallback for platforms that provide neither a fixed TLS slot, nor a reasonable TLS implementation (i.e. not emutls).

Diff Detail

Repository
rL LLVM

Event Timeline

eugenis updated this revision to Diff 35050.Sep 17 2015, 5:31 PM
eugenis retitled this revision from to Android support for SafeStack.
eugenis updated this object.
eugenis added reviewers: pcc, echristo.
eugenis set the repository for this revision to rL LLVM.
eugenis added a subscriber: llvm-commits.
echristo edited edge metadata.Sep 21 2015, 4:40 PM

Some inline comments :)

-eric

include/llvm/LinkAllPasses.h
136 ↗(On Diff #35050)

Default = nullptr to avoid this?

include/llvm/Transforms/Instrumentation.h
19

Can probably just forward declare right?

lib/Target/X86/X86ISelLowering.cpp
2084

Block point explaining the idea behind it? (Also might want the same sort of thing on the function just above if you wouldn't mind.)

lib/Transforms/Instrumentation/SafeStack.cpp
21–23

?

21–23

Can you remove the dependence on TTI separately? I don't see any use otherwise?

226–227

const?

263–268

Needs a comment.

Also, can we do this in such a way that we don't need to check for android? Similar to the stack protector pattern?

385–386

?

eugenis updated this revision to Diff 35335.Sep 21 2015, 6:10 PM
eugenis edited edge metadata.
eugenis marked 6 inline comments as done.
eugenis added inline comments.
lib/Transforms/Instrumentation/SafeStack.cpp
21–23

Passes.h needed for INITIALIZE_TM_PASS_BEGIN

263–268

We could move this logic to TargetLowering.h, making the X86 implementation delegate to the common implementation if not android, but it does not improve anything.

385–386

Simplified.
This code moved insertion point after the calculation of UnsafeStackPtr, unless that's a constant.

echristo accepted this revision.Sep 22 2015, 5:05 PM
echristo edited edge metadata.

LGTM.

Thanks!

-eric

This revision is now accepted and ready to land.Sep 22 2015, 5:05 PM
eugenis closed this revision.Sep 22 2015, 6:05 PM

Thanks for the review! Submitted as r248357.