[PATCH] D12952: Android support for SafeStack

[Eric Christopher via llvm-commits]

echristo added a comment.

Some inline comments :)

-eric


================
Comment at: include/llvm/LinkAllPasses.h:136
@@ -135,3 +135,3 @@
       (void) llvm::createSCCPPass();
-      (void) llvm::createSafeStackPass();
+      (void) llvm::createSafeStackPass(nullptr);
       (void) llvm::createScalarReplAggregatesPass();
----------------
Default = nullptr to avoid this?

================
Comment at: include/llvm/Transforms/Instrumentation.h:19
@@ -18,2 +18,3 @@
 #include "llvm/IR/BasicBlock.h"
+#include "llvm/Target/TargetMachine.h"
 #include <vector>
----------------
Can probably just forward declare right?

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:2063
@@ -2062,1 +2062,3 @@
 
+bool X86TargetLowering::getSafeStackPointerLocation(unsigned &AddressSpace,
+                                                    unsigned &Offset) const {
----------------
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.)

================
Comment at: lib/Transforms/Instrumentation/SafeStack.cpp:22
@@ -21,3 +21,3 @@
 #include "llvm/Analysis/AliasAnalysis.h"
-#include "llvm/Analysis/TargetTransformInfo.h"
+#include "llvm/CodeGen/Passes.h"
 #include "llvm/IR/Constants.h"
----------------
?

================
Comment at: lib/Transforms/Instrumentation/SafeStack.cpp:22
@@ -21,3 +21,3 @@
 #include "llvm/Analysis/AliasAnalysis.h"
-#include "llvm/Analysis/TargetTransformInfo.h"
+#include "llvm/CodeGen/Passes.h"
 #include "llvm/IR/Constants.h"
----------------
echristo wrote:
> ?
Can you remove the dependence on TTI separately? I don't see any use otherwise?

================
Comment at: lib/Transforms/Instrumentation/SafeStack.cpp:226
@@ -218,2 +225,3 @@
   static char ID; // Pass identification, replacement for typeid.
-  SafeStack() : FunctionPass(ID), DL(nullptr) {
+  SafeStack(TargetMachine *TM)
+      : FunctionPass(ID), TM(TM), TLI(nullptr), DL(nullptr) {
----------------
const?

================
Comment at: lib/Transforms/Instrumentation/SafeStack.cpp:263-268
@@ -258,1 +262,8 @@
+
+  if (TargetTriple.getEnvironment() == llvm::Triple::Android) {
+    Value *Fn = M.getOrInsertFunction(kUnsafeStackPtrAddrFn,
+                                      StackPtrTy->getPointerTo(0), nullptr);
+    IRBuilder<> IRB(F.begin()->getFirstInsertionPt());
+    Value *V = IRB.CreateCall(Fn);
+    return V;
   } else {
----------------
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?

================
Comment at: lib/Transforms/Instrumentation/SafeStack.cpp:387-390
@@ -358,3 +386,6 @@
 
-  IRBuilder<> IRB(F.getEntryBlock().getFirstInsertionPt());
+  IRBuilder<> IRB(
+      isa<Instruction>(UnsafeStackPtr)
+          ? cast<Instruction>(UnsafeStackPtr)->getNextNode()
+          : cast<Instruction>(F.getEntryBlock().getFirstInsertionPt()));
   DIBuilder DIB(*F.getParent());
----------------
?



Repository:
  rL LLVM

http://reviews.llvm.org/D12952