[PATCH] Protection against stack-based memory corruption errors using SafeStack

[Kostya Serebryany]

================
Comment at: include/llvm/Analysis/TargetTransformInfo.h:507
@@ +506,3 @@
+  /// space, and populates the address space and offset as appropriate.
+  bool getUnsafeStackPtrLocation(unsigned &AddressSpace,
+                                 unsigned &Offset) const;
----------------
Do we need this at all? 

================
Comment at: lib/CodeGen/LLVMBuild.txt:25
@@ -24,2 +24,2 @@
 parent = Libraries
-required_libraries = Analysis Core MC Scalar Support Target TransformUtils
+required_libraries = Analysis Core Instrumentation MC Scalar Support Target TransformUtils
----------------
why do you need this? 

================
Comment at: lib/Transforms/Instrumentation/SafeStack.cpp:105
@@ +104,3 @@
+
+        /* fallthough */
+
----------------
Maybe this? 
http://clang.llvm.org/docs/AttributeReference.html#fallthrough-clang-fallthrough

================
Comment at: lib/Transforms/Instrumentation/SafeStack.cpp:163
@@ +162,3 @@
+/// local variables that are accessed in unsafe ways.
+class SafeStack : public ModulePass {
+  const DataLayout *DL;
----------------
does it have to be a module pass? 
why not a function pass? 

================
Comment at: lib/Transforms/Instrumentation/SafeStack.cpp:244
@@ +243,3 @@
+
+  // Check where the unsafe stack pointer is stored on this architecture
+  unsigned AddressSpace, Offset;
----------------
here and elsewhere: period after a comment. 

================
Comment at: lib/Transforms/Instrumentation/SafeStack.cpp:249
@@ +248,3 @@
+    // (usually in the thread control block)
+    Constant *OffsetVal = ConstantInt::get(Int32Ty, Offset);
+    return ConstantExpr::getIntToPtr(
----------------
why int32 here? 

================
Comment at: lib/Transforms/Instrumentation/SafeStack.cpp:252
@@ +251,3 @@
+        OffsetVal, Int8Ty->getPointerTo()->getPointerTo(AddressSpace));
+  } else {
+    // The unsafe stack pointer is stored in a global variable with a magic name
----------------
which of the ways is used where? do we need both? 

================
Comment at: lib/Transforms/Instrumentation/SafeStack.cpp:256
@@ +255,3 @@
+    // FIXME: make the name start with "llvm."
+    static const char* unsafe_stack_ptr_var = "__safestack_unsafe_stack_ptr";
+
----------------
const variables are named like kUnsafeStackPtrVar

================
Comment at: lib/Transforms/Instrumentation/SafeStack.cpp:271
@@ +270,3 @@
+      // The variable exists, check its type and attributes
+      if (UnsafeStackPtr->getValueType() != Int8Ty->getPointerTo()) {
+        report_fatal_error(Twine(unsafe_stack_ptr_var) +
----------------
why not just assert? 

================
Comment at: lib/Transforms/Instrumentation/SafeStack.cpp:286
@@ +285,3 @@
+
+bool SafeStack::runOnFunction(Function &F) {
+  ++NumFunctions;
----------------
This function is too big. Please split it into as many smaller ones as possible. With comments. 

================
Comment at: lib/Transforms/Instrumentation/SafeStack.cpp:289
@@ +288,3 @@
+
+  unsigned StackAlignment = 16;
+  Constant *UnsafeStackPtr = getOrCreateUnsafeStackPtr(F);
----------------
const?

================
Comment at: lib/Transforms/Instrumentation/SafeStack.cpp:328
@@ +327,3 @@
+      if (CI->getCalledFunction() && CI->canReturnTwice())
+          //CI->getCalledFunction()->getName() == "_setjmp")
+        StackRestorePoints.push_back(CI);
----------------
do you need this comment? 

================
Comment at: lib/Transforms/Instrumentation/SafeStack.cpp:541
@@ +540,3 @@
+    Value *CurrentTop = DynamicTop ? IRB.CreateLoad(DynamicTop) : StaticTop;
+    IRB.CreateStore(CurrentTop, UnsafeStackPtr);
+  }
----------------
Is this tested in any of the unit tests? 

================
Comment at: test/Transforms/SafeStack/safestack.ll:1
@@ +1,2 @@
+; RUN: opt -safe-stack -S -mtriple=i386-pc-linux-gnu < %s -o - | FileCheck --check-prefix=CHECK --check-prefix=LINUX %s
+; RUN: opt -safe-stack -S -mtriple=x86_64-pc-linux-gnu < %s -o - | FileCheck --check-prefix=CHECK --check-prefix=LINUX %s
----------------
A comment would be nice. 
Explain why you have CHECK and LINUX
Consider splitting this test into several

http://reviews.llvm.org/D6094

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/