[PATCH] D15080: [asan] Reports suppressions for ASan recovery mode (compiler-rt part).

[Yury Gribov via llvm-commits]

ygribov added inline comments.

================
Comment at: lib/asan/asan_rtl.cc:123
@@ +122,3 @@
+  if (atomic_load_relaxed(&pc_num) >= kAsanBuggyPcPoolSize)
+    Die();
+
----------------
You already have this check after loop. Why do you need this preliminary check? It'll slow fast path.

================
Comment at: lib/asan/asan_rtl.cc:129
@@ +128,3 @@
+    // Find available slot for new PC. Don't override already acquired records.
+    if (atomic_compare_exchange_strong(&AsanBuggyPcPool[i], &zero, pc,
+                                       memory_order_acq_rel)) {
----------------
I'm not familiar with atomics but I think that CAS is much slower than ordinary load. Can you do loads for fast path and only switch to CAS when reaching NULL?

================
Comment at: lib/asan/asan_rtl.cc:136
@@ +135,3 @@
+    zero = 0;
+    if (pc == atomic_load_relaxed(&AsanBuggyPcPool[i])) return true;
+  }
----------------
It's better to avoid this extra atomic by using value returned by atomic_compare_exchange.

================
Comment at: lib/asan/asan_rtl.cc:146
@@ +145,3 @@
+  // precise here.
+  atomic_fetch_add(&pc_num, 1, memory_order_relaxed);
+  return false;
----------------
I believe we can get rid of pc_num completely.

================
Comment at: lib/asan/asan_rtl.cc:147
@@ +146,3 @@
+  atomic_fetch_add(&pc_num, 1, memory_order_relaxed);
+  return false;
+}
----------------
This function seems to always return false.

================
Comment at: lib/asan/asan_rtl.cc:166
@@ -125,2 +165,3 @@
   GET_CALLER_PC_BP_SP;                                              \
+  if (SuppressErrorReport(pc)) return;                              \
   ReportGenericError(pc, bp, sp, addr, is_write, size, 0, false);   \
----------------
I wonder if it makes sense to move suppression check to ReportGenericError. It'll remove duplicate code and also automatically handle other cases (ReportDoubleFree, etc.).

================
Comment at: test/asan/TestCases/halt_on_error-2.c:4
@@ +3,3 @@
+// RUN: %clang_asan -fsanitize-recover=address -DCHECK_SUPPRESSION=1 %s -o %t
+// RUN: %env_asan_opts=halt_on_error=false %run %t 2>&1 | FileCheck --check-prefix=CHECK-SUPPRESSION %s
+
----------------
Why no grep here?

================
Comment at: test/asan/TestCases/halt_on_error-2.c:7
@@ +6,3 @@
+// RUN: %clang_asan -fsanitize-recover=address -DCHECK_THRESHOLD=1 %s -o %t
+// RUN: %env_asan_opts=halt_on_error=false %run %t >3.txt 2>&1 || true
+// RUN: [ $(grep -c 'ERROR: AddressSanitizer: stack-buffer-overflow' 3.txt) -eq 25 ]
----------------
Can you dump to %t.log or something like that? Also you don't need true.

================
Comment at: test/asan/TestCases/halt_on_error-2.c:25
@@ +24,3 @@
+  for (int i = 0; i < kNumIterations; ++i) {
+#ifdef CHECK_SUPPRESSION
+    // CHECK-SUPPRESSION: READ of size 1
----------------
You can control this with cmdline args and avoid double compilation.


http://reviews.llvm.org/D15080