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

[David Chisnall]

================
Comment at: lib/CodeGen/SafeStack.cpp:231
@@ +230,3 @@
+
+      if (F.getName().startswith("llvm.") ||
+          F.getName().startswith("__llvm__")) {
----------------
The first check should be F.isIntrinsic().  I'm a bit surprised it's needed though, because intrinsics should never have be definitions in the IR...

================
Comment at: lib/CodeGen/SafeStack.cpp:237
@@ +236,3 @@
+
+      if (!F.hasFnAttribute(Attribute::SafeStack)) {
+        DEBUG(dbgs() << "[SafeStack]     safestack is not requested"
----------------
It's more likely that functions won't have the safe stack attribute than that they'll start with __llvm__, so I'd switch the order of this test and the previous one.

================
Comment at: lib/CodeGen/SafeStack.cpp:287
@@ +286,3 @@
+    UnsafeStackPtr = dyn_cast_or_null<GlobalVariable>(
+          M.getNamedValue("__llvm__unsafe_stack_ptr"));
+
----------------
The string "__llvm__unsafe_stack_ptr" appears in (at least) two places, please make it a constant somewhere (ideally somewhere that can be shared with compiler-rt, although that's a bit harder).

================
Comment at: lib/CodeGen/SafeStack.cpp:329
@@ +328,3 @@
+
+  // Collect all points where stack gets unwinded and needs to be restored
+  // This is only necessary because the runtime (setjmp and unwind code) is
----------------
unwound

================
Comment at: lib/CodeGen/SafeStack.cpp:338
@@ +337,3 @@
+  Type *IntPtrTy = DL->getIntPtrType(F.getContext());
+  Type *Int32Ty = Type::getInt32Ty(F.getContext());
+
----------------
These three shouldn't change between functions.  Make them fields and initialise them in the module init code.

================
Comment at: lib/CodeGen/SafeStack.cpp:392
@@ +391,3 @@
+    // We explicitly compute and set the unsafe stack layout for all unsafe
+    // static alloca instructions. We safe the unsafe "base pointer" in the
+    // prologue into a local variable and restore it in the epilogue.
----------------
We save

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:1947
@@ +1946,3 @@
+                                                  unsigned &Offset) const {
+  if (Subtarget->isTargetLinux()) {
+    if (Subtarget->is64Bit()) {
----------------
These look like quite magic numbers - please document where they come from (why does Linux use fs- or gs-relative things depending on whether it's 64-bit, whereas Darwin uses gs-relative addressing all of the time).  The isTargetLinux() test looks wrong, as this looks to be something that relates to userland ABI and not to the kernel.  What is responsible for setting up the unsafe stack pointer?

http://reviews.llvm.org/D6094