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

[Maxim Ostapenko via llvm-commits]

m.ostepenko added inline comments.

================
Comment at: lib/asan/asan_flags.inc:141
@@ -140,1 +140,3 @@
           "(WARNING: USE AT YOUR OWN RISK!)")
+ASAN_FLAG(bool, deduplicate_reports, true,
+          "Deduplicate multiple reports for single source location in "
----------------
ygribov wrote:
> kcc wrote:
> > maybe use names similar to ones in tsan? 
> > e.g. suppress_equal_pcs? 
> And also make the flag common.
Ok.

================
Comment at: lib/asan/asan_rtl.cc:117
@@ +116,3 @@
+  if (!flags()->deduplicate_reports) return false;
+  static u32 pc_num;
+  // We have exceeded reasonable number of different reports, subsequent
----------------
kcc wrote:
> can't you make it __sanitizer::atomic_uint32_t ? 
Right, thank you.

================
Comment at: lib/asan/asan_rtl.cc:121
@@ +120,3 @@
+  if (pc_num >= kAsanBuggyPcPoolSize) {
+    if (!flags()->sleep_before_dying)
+      SleepForMillis(100);
----------------
kcc wrote:
> Bad. 
> We should not duplicate this kind of functionality. 
> Why not call Die() here? 
Ok.

================
Comment at: lib/asan/asan_rtl.cc:129
@@ +128,3 @@
+
+  AsanBuggyPcPool[pc_num] = pc;
+  atomic_fetch_add((__sanitizer::atomic_uint32_t *)&pc_num, 1,
----------------
kcc wrote:
> this is actually racy, isn't it? :) 
Yes, but do we actually need to be atomic here? I mean, if we hit the race, we would just jump over one entry in array, so it would contain 0. I don't think exact precision is critical here. Atomic operation may introduce unnecessary slowdown... not sure about this.

================
Comment at: test/asan/TestCases/halt_on_error-2.c:6
@@ +5,3 @@
+// RUN: FileCheck %s < 2.txt
+// RUN: [ $(grep -c 'ERROR: AddressSanitizer: stack-buffer-overflow' 2.txt) -eq 3 ]
+
----------------
kcc wrote:
> I would just add
> CHECK-NOT: ERROR 
> as a 4-th CHECK line
> 
> Also, add a run-line that actually produces 30 error messages and checks for that. 
Ok.


http://reviews.llvm.org/D15080