[PATCH] D13455: [safestack] Fast access to the unsafe stack pointer on AArch64/Android.

[Eric Christopher via llvm-commits]

echristo added a comment.

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


================
Comment at: include/llvm/Target/TargetLowering.h:1951
@@ -1954,1 +1950,3 @@
 
+  const char *kAndroidUnsafeStackPtrAddrFn = "__safestack_pointer_address";
+
----------------
Should probably be folded in.

================
Comment at: lib/CodeGen/TargetLoweringBase.cpp:1665-1666
@@ -1664,1 +1664,4 @@
 
+/// Android provides a libc function to retrieve the address of the current
+/// thread's unsafe stack pointer.
+Value *TargetLoweringBase::getSafeStackPointerLocation(IRBuilder<> &IRB) const {
----------------
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).

================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:9754-9756
@@ +9753,5 @@
+
+/// Android provides a fixed TLS slot for the SafeStack pointer.
+/// See the definition of TLS_SLOT_SAFESTACK in
+/// https://android.googlesource.com/platform/bionic/+/master/libc/private/bionic_tls.h
+Value *AArch64TargetLowering::getSafeStackPointerLocation(IRBuilder<> &IRB) const {
----------------
Go ahead and put this over the android bits.

================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:9761
@@ +9760,3 @@
+
+  const unsigned kTlsOffset = 0x48;
+  Module *M = IRB.GetInsertBlock()->getParent()->getParent();
----------------
Non-standard naming.


Repository:
  rL LLVM

http://reviews.llvm.org/D13455