This is an archive of the discontinued LLVM Phabricator instance.

[safestack] Fast access to the unsafe stack pointer on AArch64/Android.
ClosedPublic

Authored by eugenis on Oct 5 2015, 5:22 PM.

Details

Summary

Android libc provides a fixed TLS slot for the unsafe stack pointer,
and this change implements direct access to that slot on AArch64 via
__builtin_thread_pointer() + offset.

This change also moves more code into TargetLowering and its
target-specific subclasses to get rid of target-specific codegen
in SafeStackPass.

This change does not touch the ARM backend because it lowers
builting_thread_pointer as aeabi_read_tp, which is not available
on Android.

Diff Detail

Repository
rL LLVM

Event Timeline

eugenis updated this revision to Diff 36573.Oct 5 2015, 5:22 PM
eugenis retitled this revision from to [safestack] Fast access to the unsafe stack pointer on AArch64/Android..
eugenis updated this object.
eugenis added reviewers: echristo, pcc.
eugenis set the repository for this revision to rL LLVM.
eugenis added a subscriber: llvm-commits.

Hi Evgeniy,

This looks like a very Android-specific patch on the AArch64 lowering mechanism. Since no one else is using that, why don't you make that function specific to the pass?

cheers,
--renato

Moving all of this into SafeStackPass would work, and would be much simpler, but it looks like llvm generally tries to avoid target-specific code in IR passes. I'm not really opposed to that, just want to see what people think.

Another option would be to implement x86.thread.pointer intrinsic (tied to __builtin_thread_pointer) and use that in safe stack pass. It would have to include an optimization that wraps x86.thread.pointer + offset into a single %gs:offset load (straightforward codegen would do *(%gs:0)+offset).

Yeah, I'm not convinced either. But I don't like that android function name right in the middle of the TargetLowering.h. TBH, I don't like it either in the pass, but I assumed it was an android-specific pass.

If that's not true, than I'd think that an intrinsic would make more sense.

echristo edited edge metadata.Oct 7 2015, 2:53 PM

Hi Evgeniy,

A few inline comments here. I think if you fix those it'll also fix some of the complaints that Renato had - I'll leave it up to him to confirm. We've got a bit of sausage like this in the middle end, but it seems like we should avoid it if possible and take a slight code duplication hit to do so.

-eric

include/llvm/Target/TargetLowering.h
1951

Should probably be folded in.

lib/CodeGen/TargetLoweringBase.cpp
1665–1666

This comment should go with the Android bits below and a generic comment above the function. Might be better to just duplicate the code in every target rather than have a generic one only implemented for for android (i.e. if there were a generic function in compiler-rt it might make more sense).

lib/Target/AArch64/AArch64ISelLowering.cpp
9754–9756

Go ahead and put this over the android bits.

9761

Non-standard naming.

It is Android-specific at the moment, but really it is intended for any platform that provides system-wide safestack support in libc. For example, there are some patches for FreeBSD (I don't know what their status is and if they are intended to be merged upstream or not): https://github.com/cpi-llvm/freebsd/commit/a542e1d0c55e71dce888736032deedb30c1a6c92#diff-f8caf191619c46a5ffd4f7f010fe97eaR53

rengolin added inline comments.Oct 7 2015, 3:17 PM
include/llvm/Target/TargetLowering.h
1951

I was thinking about this one, too. It's only ever used once.

lib/CodeGen/TargetLoweringBase.cpp
1665–1666

Especially when no one it using it...

eugenis added inline comments.Oct 7 2015, 4:23 PM
lib/CodeGen/TargetLoweringBase.cpp
1665–1666

Well there is a generic function in compiler-rt. Or, rather, a thread-local variable. See the remaining part of SafeStack::getOrCreateUnsafeStackPtr which could be moved here as well, and then this function would apply to all imaginable targets.

echristo added inline comments.Oct 7 2015, 4:26 PM
lib/CodeGen/TargetLoweringBase.cpp
1665–1666

SGTM. Let's see what that looks like.

It does give the assumption that anyone using safe stack is using compiler-rt, but...

eugenis updated this revision to Diff 36811.Oct 7 2015, 5:09 PM
eugenis edited edge metadata.
eugenis marked 3 inline comments as done.
eugenis added inline comments.
lib/CodeGen/TargetLoweringBase.cpp
1665–1666

If it's not a known platform (android) and not using compiler-rt, there is nothing we can do anyway.

lib/Target/AArch64/AArch64ISelLowering.cpp
9761

Should it be just "TlsOffset", without the "k" prefix?
CodingStandards does not say anything special about constants.

echristo added inline comments.Oct 7 2015, 5:13 PM
lib/Target/AArch64/AArch64ISelLowering.cpp
9761

Right. Constants aren't special in llvm naming here.

eugenis updated this revision to Diff 36813.Oct 7 2015, 5:14 PM
eugenis marked an inline comment as done.
lib/CodeGen/TargetLoweringBase.cpp
1665–1666

Done, PTAL

echristo added inline comments.Oct 7 2015, 5:21 PM
lib/CodeGen/TargetLoweringBase.cpp
1677

Naming, and you can just fold it in as well.

1686–1690

Pretty much all the arguments are fairly self explanatory, probably don't need the comments.

1693

No braces.

1697

Ditto.

lib/Target/AArch64/AArch64Subtarget.h
126–128

Given how pervasive this is now, it'll probably be worth splitting this out into a separate commit and doing a look for Triple.isAndroid() and updating all of the callers in llvm and clang.

eugenis updated this revision to Diff 36899.Oct 8 2015, 3:00 PM
eugenis marked 3 inline comments as done.
eugenis added inline comments.
lib/CodeGen/TargetLoweringBase.cpp
1677

It's used in 4 places. Fixed the naming.

lib/Target/AArch64/AArch64Subtarget.h
126–128

Done and rebased.

Right, I'm ok with this. Eric?

echristo accepted this revision.Oct 15 2015, 9:21 AM
echristo edited edge metadata.

Sure. I'd still probably fold in the function name because I don't think local is getting us anything, but styles are different :)

-eric

This revision is now accepted and ready to land.Oct 15 2015, 9:21 AM

I just think that repeating a rather long string constant four times in the source code is typo-prone and less readable.

rengolin accepted this revision.Oct 15 2015, 1:43 PM
rengolin added a reviewer: rengolin.

I just think that repeating a rather long string constant four times in the source code is typo-prone and less readable.

I agree.

LGTM, too. Thanks!

At least the name is checked by the compiler.
Thanks for the review! Landed as r250456.

eugenis closed this revision.Oct 15 2015, 1:53 PM

Good point! Thanks for the work.