[PATCH] [compiler-rt] Implement AddressSanitizer suppressions

[Alexey Samsonov]

Looks good, I'm mostly happy with the code itself, moving to comments about the test cases.

================
Comment at: lib/asan/asan_suppressions.cc:42
@@ +41,3 @@
+  SuppressionContext *ctx = SuppressionContext::Get();
+  return ctx->HasSuppressionType(SuppressionInterceptorViaFunction)
+    || ctx->HasSuppressionType(SuppressionInterceptorViaLibrary);
----------------
Please run the patch through clang-format

================
Comment at: lib/asan/asan_suppressions.cc:55
@@ +54,3 @@
+    uptr addr = stack->trace[i];
+    Symbolizer *symbolizer = Symbolizer::GetOrInit();
+
----------------
I think you can define this variable outside of for-loop.

================
Comment at: lib/asan/asan_suppressions.cc:61
@@ +60,3 @@
+      if (symbolizer->GetModuleNameAndOffsetForPC(addr, &module_name,
+                                                  &module_offset)) {
+        // Match "interceptor_via_library" suppressions.
----------------
Collapse these two if's into a single one with &&

================
Comment at: lib/asan/asan_suppressions.cc:71
@@ +70,3 @@
+      for (SymbolizedStack *cur = frames; cur; cur = cur->next) {
+        const char *function_name = cur->info.function;
+        // Match "interceptor_via_function" suppressions.
----------------
Note that function_name can be nullptr here. It's okay, and SuppressionContext::Match() will work correctly and return false, though. Leaving this up to you.

================
Comment at: lib/asan/asan_suppressions.h:25
@@ +24,3 @@
+bool HaveStackTraceBasedSuppressions();
+bool IsStackTraceSuppressed(StackTrace *stack);
+
----------------
const StackTrace *stack

================
Comment at: test/asan/TestCases/Darwin/suppressions-darwin.cc:6
@@ +5,3 @@
+// Check that suppressing the interceptor by name works.
+// RUN: echo "interceptor_name:memmove" > %tmp
+// RUN: ASAN_OPTIONS=suppressions=%tmp %run %t 2>&1 | FileCheck --check-prefix=CHECK-IGNORE %s
----------------
Isn't %tmp expanded to `/some/path/to/filemp` ? Consider using %t.supp instead, so that file (if we go look for it in the test directory) has a sane name (here and below)

================
Comment at: test/asan/TestCases/Darwin/suppressions-darwin.cc:14
@@ +13,3 @@
+// Check that suppressing library works even without the symbolizer.
+// RUN: ASAN_OPTIONS=suppressions=%tmp ASAN_SYMBOLIZER_PATH=/dev/null %run %t 2>&1 | FileCheck --check-prefix=CHECK-IGNORE %s
+
----------------
Use
  ASAN_OPTIONS=suppressions=%t.supp:symbolize=false
instead. Also, you may want to check that suppress-by-interceptor-name also works w/o symbolizer.

================
Comment at: test/asan/TestCases/Darwin/suppressions-darwin.cc:21
@@ +20,3 @@
+  strcpy(a, "hello");
+  CFStringRef str = CFStringCreateWithBytes(kCFAllocatorDefault, (unsigned char *)a, 10, kCFStringEncodingUTF8, FALSE); // BOOM
+  fprintf(stderr, "Ignored.\n");
----------------
use 80 cols for this line

================
Comment at: test/asan/TestCases/Darwin/suppressions-darwin.cc:22
@@ +21,3 @@
+  CFStringRef str = CFStringCreateWithBytes(kCFAllocatorDefault, (unsigned char *)a, 10, kCFStringEncodingUTF8, FALSE); // BOOM
+  fprintf(stderr, "Ignored.\n");
+}
----------------
Do you need to free() the buffer here?

================
Comment at: test/asan/TestCases/Darwin/suppressions-darwin.cc:28
@@ +27,2 @@
+// CHECK-IGNORE: Ignored.
+// CHECK-IGNORE-NOT: AddressSanitizer: heap-buffer-overflow
----------------
This CHECK-IGNORE-NOT piece should go before CHECK-IGNORE above (here and below)

================
Comment at: test/asan/TestCases/suppressions-function.cc:2
@@ +1,3 @@
+// Check that without suppressions, we catch the issue.
+// RUN: %clangxx_asan -O0 %s -o %t
+// RUN: not %run %t 2>&1 | FileCheck --check-prefix=CHECK-CRASH %s
----------------
Can you test it on higher optimization levels? Suppression functionality should work there.

================
Comment at: test/asan/TestCases/suppressions-instrumented-code.cc:1
@@ +1,2 @@
+// Check that reports from instrumented code cannot be suppressed.
+// RUN: %clangxx_asan -O0 %s -o %t
----------------
Wait, how do you enforce that? By assuming that memset() in user code will be turned into __asan_memset() call? In fact, I see little harm in suppressing reports from memset() from instrumented code if the user explicitly added `interceptor_name:memset` to suppression file. After all, this would work for another interceptors (like strcpy), won't it?

================
Comment at: test/asan/TestCases/suppressions-instrumented-code.cc:3
@@ +2,3 @@
+// RUN: %clangxx_asan -O0 %s -o %t
+// RUN: echo "interceptor_name:memset" > %tmp
+// RUN: echo "interceptor_via_function:main" >> %tmp
----------------
Also add memcpy/memmove here.

http://reviews.llvm.org/D6280