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

[Kostya Serebryany]

================
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
----------------
Alexey Samsonov wrote:
> The comment is now obsolete
done

================
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
----------------
Alexey Samsonov wrote:
> Can you use LLVM_TRANSFORMS_UTILS_ASANSTACKFRAMELAYOUT_H for consistency with another files?
done

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

================
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;
----------------
Alexey Samsonov wrote:
> "as in asan_internal.h from ASan runtime in compiler-rt"
done

================
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
----------------
Alexey Samsonov wrote:
> 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.
that's an overkill. renamed to ASanStackVariableDescription

================
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;
----------------
Alexey Samsonov wrote:
> this can be AllocaInst* now.
done

================
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;
----------------
Alexey Samsonov wrote:
> "see DescribeAddressIfStack in ASan runtime"
done

================
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.
----------------
Alexey Samsonov wrote:
> What about using SmallVectorImpl here?
done

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

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

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

================
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);
----------------
Alexey Samsonov wrote:
> order of arguments should be reversed.
done

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


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