[PATCH] [compiler-rt] Implement AddressSanitizer suppressions

[Alexey Samsonov]

================
Comment at: lib/asan/asan_interceptors.cc:142
@@ -123,4 +141,3 @@
       return REAL(func)(__VA_ARGS__);                                          \
-    ctx = 0;                                                                   \
-    (void) ctx;                                                                \
+    ASAN_INTERCEPTOR_ENTER(ctx, func, __VA_ARGS__)                             \
     if (SANITIZER_MAC && UNLIKELY(!asan_inited))                               \
----------------
Why do you need to pass __VA_ARGS__ here?

================
Comment at: lib/asan/asan_interceptors.cc:173
@@ +172,3 @@
+// for them.
+#define COMMON_SYSCALL_PRE_READ_RANGE(p, s) ASAN_READ_RANGE(0, p, s)
+#define COMMON_SYSCALL_PRE_WRITE_RANGE(p, s) ASAN_WRITE_RANGE(0, p, s)
----------------
s/0/nullptr here and in another similar places.

================
Comment at: lib/asan/asan_interceptors.cc:401
@@ +400,3 @@
+// memset is called inside Printf.
+#define ASAN_MEMSET_CTX(ctx, block, c, size) do {                              \
+    if (UNLIKELY(!asan_inited)) return internal_memset(block, c, size);        \
----------------
Probably, ASAN_MEMSET_IMPL ?

================
Comment at: lib/asan/asan_interceptors.cc:411
@@ +410,3 @@
+    return REAL(memset)(block, c, size);                                       \
+  } while (0);
+
----------------
You don't need trailing semicolon here (and in other places), as you use this macro as
  ASAN_MEMSET_CTX(...);

================
Comment at: lib/asan/asan_interceptors.cc:426
@@ +425,3 @@
+    return internal_memmove(to, from, size);                                   \
+  } while (0);
+
----------------
ditto

================
Comment at: lib/asan/asan_suppressions.cc:33
@@ +32,3 @@
+
+bool IsSymbolizedStackFrameSuppressed(SuppressionContext *ctx,
+  AddressInfo *info, Suppression **s) {
----------------
static

================
Comment at: lib/asan/asan_suppressions.cc:62
@@ +61,3 @@
+    const int kMaxAddrFrames = 16;
+    InternalScopedBuffer<AddressInfo> addr_frames(kMaxAddrFrames);
+    for (uptr i = 0; i < kMaxAddrFrames; i++)
----------------
Please wait till I pass the review and land http://reviews.llvm.org/D6394 - it should make the code easier.

================
Comment at: lib/asan/asan_suppressions.cc:65
@@ +64,3 @@
+      new(&addr_frames[i]) AddressInfo();
+    GET_STACK_TRACE(kStackTraceMax, true);
+
----------------
GET_STACK_TRACE_FATAL_HERE (we should respect what user told us in common_flags()->fast_unwind_on_fatal.

Also, it seems like the wrong place to fetch the stack trace. The fact that
  `IsSuppressed("my_interceptor");`
*unwinds the stack trace* is very surprising and something that is totally unexpected from function declaration. We also have "rule of thumb" in
asan_stack.h: unwind the stack as early as possible. I'd rather split "IsSuppressed" into two methods - suppress by interceptor name, and suppress by stack trace fetched from interceptor. That would compilcate the ACCESS_MEMORY_RANGE macro, but would hopefully make the logic more clear.


There's also an alternative. Instead of calling __asan_report_error directly from that macro, you may introduce
  MaybeReportInterceptorError()
or smth. like this. There you can unwind the stack trace (once), then either match suppression and bail out (if there are active suppression), or
proceed to producing an error report and reporting this very same stack trace at hand.



================
Comment at: lib/asan/asan_suppressions.cc:68
@@ +67,3 @@
+    for (uptr i = 0; !matched && i < stack.size && stack.trace[i]; i++) {
+      uptr pc = StackTrace::GetPreviousInstructionPc(stack.trace[i]);
+      uptr addr_frames_num = Symbolizer::GetOrInit()->SymbolizePC(
----------------
You can do the following: first fetch the module name/offset for PC and see if it's suppressed (by SuppressionInterceptorViaLibrary), then symbolize the PC and see if it's suppressed by SuppressionInterceptorViaFunction (you don't need symbolization to fetch the module name). See lsan_common.cc.

================
Comment at: lib/asan/asan_suppressions.cc:83
@@ +82,3 @@
+  if (matched) {
+    Report("AddressSanitizer: matched suppression '%s'\n", s->templ);
+    s->hit_count++;
----------------
You don't really want to report this every time. I can imaging library function doing invalid strlen() call (e.g. on the same data) hundreds of times.

================
Comment at: lib/asan/asan_suppressions.h:23
@@ +22,3 @@
+void InitializeSuppressions();
+bool IsSuppressed(const char *interceptor_name);
+
----------------
See below - you might want to split this to "suppressed by interceptor name" and "suppressed by stack trace".

http://reviews.llvm.org/D6280