[PATCH] [asan] rewrite asan's stack frame layout

[Alexey Samsonov]

  Is it also possible to add .ll tests verifying that we're doing the right thing for function stack?


================
Comment at: lib/Transforms/Instrumentation/AddressSanitizer.cpp:142
@@ -146,1 +141,3 @@
+       cl::desc("Realign stack to the value of this flag (power of two)"),
+       cl::Hidden, cl::init(true));
 static cl::opt<std::string> ClBlacklistFile("asan-blacklist",
----------------
Fix the flag default value.

================
Comment at: lib/Transforms/Instrumentation/ASanStackFrameLayout.h:44
@@ +43,3 @@
+
+void ComputeASanStackFrameLayout(
+    // The array of stack variables. The elements may get reordered.
----------------
This should be a method of ASanStackFrameLayout

================
Comment at: lib/Transforms/Instrumentation/ASanStackFrameLayout.h:35
@@ +34,3 @@
+struct ASanStackFrameLayout {
+  // Frame description as explained in DescribeAddressIfStack (asan_report.cc).
+  std::string DescriptionString;
----------------
Either provide better reference to a piece of compiler-rt, or remove it.

================
Comment at: lib/Transforms/Instrumentation/ASanStackFrameLayout.h:18
@@ +17,3 @@
+#define LLVM_TRANSFORMS_INSTRUMENTATION_ASAN_STACK_FRAME_LAYOUT_H
+#include <vector>
+#include <string>
----------------
Nit: header order

================
Comment at: lib/Transforms/Instrumentation/ASanStackFrameLayout.h:13
@@ +12,3 @@
+// of LLVM so that a) the functionaliry can be unit-tested separately and
+// b) the code can be easily reused by any other compiler written in C++.
+//
----------------
That's a bit unfortunate that we can't use LLVM ADT here.

================
Comment at: lib/Transforms/Instrumentation/ASanStackFrameLayout.cpp:21
@@ +20,3 @@
+
+// These magic constants should be the same as in asan_internal.h
+static const int kAsanStackLeftRedzoneMagic = 0xf1;
----------------
ditto for compiler-rt reference

================
Comment at: lib/Transforms/Instrumentation/ASanStackFrameLayout.cpp:33
@@ +32,3 @@
+// for the stack variables we avoid reordering them too much.
+static inline bool CompareVars(const StackVariableDescription &a,
+                               const StackVariableDescription &b) {
----------------
Side note: wow, we will probably break some code with this reordering.

================
Comment at: lib/Transforms/Instrumentation/ASanStackFrameLayout.cpp:60
@@ +59,3 @@
+
+static std::string ShadowBytesToString(std::vector<uint8_t> &ShadowBytes) {
+  std::ostringstream os;
----------------
you can make this a method of ASanStackFrameLayout (dumpShadowBytes)

================
Comment at: lib/Transforms/Instrumentation/ASanStackFrameLayout.cpp:91
@@ +90,3 @@
+// clang++ ASanStackFrameLayout.cpp -DTestASanStackFrameLayout=main && ./a.out
+int TestASanStackFrameLayout() {
+#define VEC1(a) std::vector<StackVariableDescription>(1, a)
----------------
Please factor this into a separate file and add it under unittests/ Otherwise this code will quickly rot.

================
Comment at: lib/Transforms/Instrumentation/ASanStackFrameLayout.cpp:155
@@ +154,3 @@
+                                 ASanStackFrameLayout *Layout) {
+  // We want this file to be as self-contained as possible so we run the tests
+  // from here (only once and only in debug builds).
----------------
why?

================
Comment at: lib/Transforms/Instrumentation/ASanStackFrameLayout.cpp:162
@@ +161,3 @@
+
+  assert(Granularity >= 8);
+  assert((Granularity & (Granularity - 1)) == 0);
----------------
you may also check upper bound for Granularity (64) and lower bound on MinHeaderSize (16).

================
Comment at: lib/Transforms/Instrumentation/ASanStackFrameLayout.h:45
@@ +44,3 @@
+void ComputeASanStackFrameLayout(
+    // The array of stack variables. The elements may get reordered.
+    std::vector<StackVariableDescription> &Vars,
----------------
they can also be changed (Alignment and Offset fields).

================
Comment at: lib/Transforms/Instrumentation/AddressSanitizer.cpp:535
@@ -540,3 +534,3 @@
   AllocaInst *findAllocaForValue(Value *V);
-  void poisonRedZones(const ArrayRef<AllocaInst*> &AllocaVec, IRBuilder<> &IRB,
+  void poisonRedZones(const std::vector<uint8_t> ShadowBytes, IRBuilder<> &IRB,
                       Value *ShadowBase, bool DoPoison);
----------------
pass ShadowBytes by reference

================
Comment at: lib/Transforms/Instrumentation/AddressSanitizer.cpp:1364
@@ -1430,10 +1363,3 @@
     }
-
-    // Poison the full redzone at right.
-    Ptr = IRB.CreateAdd(ShadowBase,
-                        ConstantInt::get(IntptrTy, Pos >> Mapping.Scale));
-    bool LastAlloca = (i == AllocaVec.size() - 1);
-    Value *Poison = LastAlloca ? PoisonRight : PoisonMid;
-    IRB.CreateStore(Poison, IRB.CreateIntToPtr(Ptr, RZPtrTy));
-
-    Pos += RedzoneSize();
+    if (!Val) continue;
+    Value *Ptr = IRB.CreateAdd(ShadowBase, ConstantInt::get(IntptrTy, i));
----------------
So this loop running time is O(frame_size), but for large stack frames (local array with 10^6 elements) we'll spend much time calculating zeroes. We can be more efficient by using smth. like RLE encoding for ShadowBytes: e.g. encode LLLL00000000RRRR as {(4,'L'), (8, '\0'), (4, 'R')}

================
Comment at: lib/Transforms/Instrumentation/AddressSanitizer.cpp:535
@@ -534,3 +528,3 @@
   }
   uint64_t getAlignedAllocaSize(AllocaInst *AI) const {
     uint64_t SizeInBytes = getAllocaSizeInBytes(AI);
----------------
Alexey Samsonov wrote:
> pass ShadowBytes by reference
I think you don't need this function now (same for getAlignedSize(SizeInBytes)

================
Comment at: lib/Transforms/Instrumentation/AddressSanitizer.cpp:523
@@ -522,3 +516,3 @@
 
   size_t RedzoneSize() const {
     return RedzoneSizeForScale(Mapping.Scale);
----------------
Please review usage of this function.


http://llvm-reviews.chandlerc.com/D2324