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

[Alexey Samsonov]

================
Comment at: include/llvm/Transforms/Utils/ASanStackFrameLayout.h:11
@@ +10,3 @@
+// This header defines ComputeASanStackFrameLayout and auxilary data structs.
+// This and the corresponding .cpp file do not include anything from the rest
+// of LLVM so that a) the functionaliry can be unit-tested separately and
----------------
The comment is now obsolete

================
Comment at: include/llvm/Transforms/Utils/ASanStackFrameLayout.h:16
@@ +15,3 @@
+//===----------------------------------------------------------------------===//
+#ifndef LLVM_TRANSFORMS_UTILS_ASAN_STACK_FRAME_LAYOUT_H
+#define LLVM_TRANSFORMS_UTILS_ASAN_STACK_FRAME_LAYOUT_H
----------------
Can you use LLVM_TRANSFORMS_UTILS_ASANSTACKFRAMELAYOUT_H for consistency with another files?

================
Comment at: include/llvm/Transforms/Utils/ASanStackFrameLayout.h:22
@@ +21,3 @@
+#include <stdint.h>
+#include <string>
+#include <vector>
----------------
I guess you don't need <string> and <vector> now.

================
Comment at: include/llvm/Transforms/Utils/ASanStackFrameLayout.h:27
@@ +26,3 @@
+
+// These magic constants should be the same as in asan/asan_internal.h
+static const int kAsanStackLeftRedzoneMagic = 0xf1;
----------------
"as in asan_internal.h from ASan runtime in compiler-rt"

================
Comment at: include/llvm/Transforms/Utils/ASanStackFrameLayout.h:33
@@ +32,3 @@
+// Input/output data struct for ComputeASanStackFrameLayout.
+struct StackVariableDescription {
+  const char *Name;  // Name of the variable that will be displayed by asan
----------------
Consider adding one more namespace. llvm::StackVariableDescription may cause conflicts. In fact this struct isn't generally useful, so you may hide it inside ASanStackFrameLayout. And I still think ComputeASanStackFrameLayout() may be turned into Compute() method.

================
Comment at: include/llvm/Transforms/Utils/ASanStackFrameLayout.h:45
@@ +44,3 @@
+struct ASanStackFrameLayout {
+  // Frame description, see DescribeAddressIfStack in asan/asan_report.cc.
+  SmallString<64> DescriptionString;
----------------
"see DescribeAddressIfStack in ASan runtime"

================
Comment at: include/llvm/Transforms/Utils/ASanStackFrameLayout.h:38
@@ +37,3 @@
+  size_t Alignment;  // Alignment of the variable (power of 2).
+  void *Variable;    // Opaque pointer to the internal compiler object.
+  size_t Offset;     // Offset from the beginning of the frame;
----------------
this can be AllocaInst* now.

================
Comment at: include/llvm/Transforms/Utils/ASanStackFrameLayout.h:56
@@ +55,3 @@
+    // The array of stack variables. The elements may get reordered and changed.
+    std::vector<StackVariableDescription> &Vars,
+    // AddressSanitizer's shadow granularity. Usually 8, may also be 16, 32, 64.
----------------
What about using SmallVectorImpl here?

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

================
Comment at: lib/Transforms/Instrumentation/AddressSanitizer.cpp:512
@@ -518,3 +511,3 @@
             AI.isStaticAlloca() &&
             AI.getAlignment() <= RedzoneSize() &&
             AI.getAllocatedType()->isSized());
----------------
The RedzoneSize() here may lie.

================
Comment at: lib/Transforms/Utils/ASanStackFrameLayout.cpp:17
@@ +16,3 @@
+#include <algorithm>
+#include <string>
+
----------------
is <string> needed?

================
Comment at: lib/Transforms/Utils/ASanStackFrameLayout.cpp:58
@@ +57,3 @@
+                                 ASanStackFrameLayout *Layout) {
+  assert(Granularity >= 8);
+  assert(Granularity <= 64);
----------------
collapse 3 asserts about Granularity

================
Comment at: unittests/Transforms/Utils/ASanStackFrameLayoutTest.cpp:16
@@ +15,3 @@
+using namespace llvm;
+
+static std::string
----------------
add anonymous namespace

================
Comment at: unittests/Transforms/Utils/ASanStackFrameLayoutTest.cpp:33
@@ +32,3 @@
+                       size_t Granularity, size_t MinHeaderSize,
+                       const char *ExpectedDescr, const char *ExpectedShadow) {
+  ASanStackFrameLayout L;
----------------
use const std::string& here for arguments.

================
Comment at: unittests/Transforms/Utils/ASanStackFrameLayoutTest.cpp:36
@@ +35,3 @@
+  ComputeASanStackFrameLayout(Vars, Granularity, MinHeaderSize, &L);
+  EXPECT_EQ(L.DescriptionString, ExpectedDescr);
+  EXPECT_EQ(ShadowBytesToString(L.ShadowBytes), ExpectedShadow);
----------------
order of arguments should be reversed.


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