[PATCH] Implement variable-sized alloca instrumentation.

[Kostya Serebryany]

================
Comment at: lib/Transforms/Instrumentation/AddressSanitizer.cpp:472
@@ -466,1 +471,3 @@
 
+  // Stores left, middle and right redzone shadow addresses for dynamic alloca.
+  struct DynamicAllocaCall {
----------------
what is the middle rz? 

================
Comment at: lib/Transforms/Instrumentation/AddressSanitizer.cpp:476
@@ +475,3 @@
+    Value *LeftRzAddr;
+    Value *PartialRzAddr;
+    Value *RightRzAddr;
----------------
add a comment describing the 3 redzones. 
What is PartialRz, is it always non-empty? 

I think you should store the pointers to the shadow here, not the pointers to the app memory. 


================
Comment at: lib/Transforms/Instrumentation/AddressSanitizer.cpp:480
@@ +479,3 @@
+                      Value *LeftRzAddr = nullptr,
+                      Value *PartialRzAddr= nullptr,
+                      Value *RightRzAddr = nullptr)
----------------
space before = 

================
Comment at: lib/Transforms/Instrumentation/AddressSanitizer.cpp:559
@@ +558,3 @@
+    Value *Shift = IRB.CreateAnd(PartialSize, IRB.getInt32(~7));
+    unsigned Val1Int = 0xcbcbcb00;
+    unsigned Val2Int = 0x000000cb;
----------------
no constants here, please. 
Define kAsanAllocaLeftMagic/kAsanAllocaRightMagic at the top of the file

================
Comment at: lib/Transforms/Instrumentation/AddressSanitizer.cpp:585
@@ +584,3 @@
+    const unsigned Align = std::max(kAllocaRzSize, AI->getAlignment());
+    const unsigned long AllocaRedzoneMask = kAllocaRzSize - 1;
+
----------------
why long? 
If it has to be 64-bit use uint64_t

================
Comment at: lib/Transforms/Instrumentation/AddressSanitizer.cpp:650
@@ +649,3 @@
+    // Poisoning right redzone.
+    poisonAddrForAlloca(RightRzAddress, IRB, 0xcbcbcbcbU);
+    AllocaCall.RightRzAddr = RightRzAddress;
----------------
no constants here. 

================
Comment at: lib/Transforms/Instrumentation/AddressSanitizer.cpp:656
@@ +655,3 @@
+                                                       Int32PtrTy);
+    // if (PartialSize) {
+    //   PartialRzMagic = calculatePartialRzMagic(PartialSize);
----------------
instead of creating a new BB for partial RZ, I would do this: 
make sure that PartialSize is never zero, i.e. instead of being in 0..31 it is in 1..32
This is better as we will not need to keep both pointers (PartialRz and RightRz) alive throughout the procedure. 

================
Comment at: test/Instrumentation/AddressSanitizer/instrument-dynamic-allocas.ll:6
@@ +5,3 @@
+; RUN: opt < %s -asan -asan-module -asan-instrument-allocas=0 -S | FileCheck %s --check-prefix=CHECK-NOALLOCA
+; RUN: opt < %s -asan -asan-module -S | FileCheck %s --check-prefix=CHECK-DEFAULT
+target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64"
----------------
use CHECK-NOALLOCA here

================
Comment at: test/asan/TestCases/alloca_instruments_all_paddings.cc:11
@@ +10,3 @@
+  assert(!(reinterpret_cast<long>(str) & 31L));
+  char *q = (char *)__asan_region_is_poisoned((char *)str + index, 64 - index);
+  assert(q && ((q - str) == index));
----------------
Can you replace "str+index" with &str[0]? 
This wya we will ensure that the valid memory is unpoisoned. 


================
Comment at: test/asan/TestCases/alloca_instruments_all_paddings.cc:16
@@ +15,3 @@
+int main(int argc, char **argv) {
+  for (int i = 1; i < 33; ++i)
+    foo(i, i);
----------------
run this loop twice to ensure that we properly unpoison the stack

http://reviews.llvm.org/D6055