[PATCH] D12318: [ASan] Enable optional ASan recovery

[Filipe Cabecinhas via llvm-commits]

filcab added a subscriber: filcab.
filcab added a comment.

What about the file `test/asan/TestCases/Darwin/interface_symbols_darwin.c`? Shouldn't it be changed too?


================
Comment at: lib/asan/asan_internal.h:125
@@ +124,3 @@
+static inline bool QuickCheckSilence(bool is_error) {
+  if (UNLIKELY(flags()->recover)) {
+    if (num_reported_errors >= flags()->max_errors)
----------------
Technically it might be unlikely, but this is noise in the code.
If we're not recovering, we will blow up anyway. Removing `UNLIKELY` would make the code easier to read with 0 practical difference in performance for any case.
In fact, it we're not silencing, *then* we want to be as fast as possible. (I'd go with the simple if, though)

================
Comment at: lib/asan/asan_internal.h:129
@@ +128,3 @@
+    // This is racy but we don't need 100% correctness.
+    if (is_error) ++num_reported_errors;
+  }
----------------
ygribov wrote:
> samsonov wrote:
> > We have atomic_fetch_add
> Yeah but it may be slower.
Slower is better than undefined behavior, no? ;-)

================
Comment at: lib/asan/asan_report.cc:632
@@ -627,6 +631,3 @@
     if (atomic_fetch_add(&num_calls, 1, memory_order_relaxed) != 0) {
-      // Do not print more than one report, otherwise they will mix up.
-      // Error reporting functions shouldn't return at this situation, as
-      // they are defined as no-return.
-      Report("AddressSanitizer: while reporting a bug found another one. "
-                 "Ignoring.\n");
+      atomic_fetch_add(&num_calls, -1, memory_order_relaxed);
+      return false;
----------------
cmpxchg should work, no?

================
Comment at: lib/asan/asan_report.cc:646
@@ -633,6 +645,3 @@
       u32 current_tid = GetCurrentTidOrInvalid();
-      if (current_tid != reporting_thread_tid) {
-        // ASan found two bugs in different threads simultaneously. Sleep
-        // long enough to make sure that the thread which started to print
-        // an error report will finish doing it.
-        SleepForSeconds(Max(100, flags()->sleep_before_dying + 1));
+      if (UNLIKELY(recover)) {
+        if (current_tid != reporting_thread_tid) {
----------------
Again, no benefit in tagging these as `UNLIKELY`.

================
Comment at: lib/asan/asan_report.cc:652
@@ +651,3 @@
+            if (TryLockReporting())
+              goto locked;
+          }
----------------
At the very least, you can create a helper function with the code executed when you get the lock, no?

================
Comment at: lib/asan/asan_report.cc:659
@@ +658,3 @@
+        // Error reporting functions shouldn't return at this situation, as
+        // they are defined as no-return.
+        if (current_tid != reporting_thread_tid) {
----------------
This comment should be changed, too. It's not true after this diff gets applied.

================
Comment at: lib/asan/asan_report.cc:661
@@ +660,3 @@
+        if (current_tid != reporting_thread_tid) {
+          // Sleep long enough to make sure that the thread which started
+          // to print an error report will finish doing it.
----------------
Why did you remove the first part of the comment?
It's still the reason why we have the `sleep` call here.

================
Comment at: lib/asan/asan_report.cc:695
@@ -668,3 +694,3 @@
     }
-    Report("ABORTING\n");
-    Die();
+    atomic_fetch_add(&num_calls, -1, memory_order_relaxed);
+    CommonSanitizerReportMutex.Unlock();
----------------
Comment on line 632 would apply here too.

================
Comment at: lib/asan/asan_report.cc:697
@@ +696,3 @@
+    CommonSanitizerReportMutex.Unlock();
+    if (LIKELY(!recover)) {
+      Report("ABORTING\n");
----------------
The usual "I see no benefit in using `LIKELY` here".

================
Comment at: lib/asan/scripts/asan_symbolize.py:429
@@ -428,3 +428,3 @@
     stack_trace_line_format = (
-        '^( *#([0-9]+) *)(0x[0-9a-f]+) *\((.*)\+(0x[0-9a-f]+)\)')
+        '^(?:[A-Z]\/[a-z ]*\([0-9 ]+\):)?( *#([0-9]+) *)(0x[0-9a-f]+) *\((.*)\+(0x[0-9a-f]+)\)')
     match = re.match(stack_trace_line_format, line)
----------------
Why? I might have missed the reason for this change.

================
Comment at: test/Driver/fsanitize.c:168
@@ -167,3 +167,3 @@
 
 // CHECK-RECOVER: "-fsanitize-recover={{((signed-integer-overflow|integer-divide-by-zero|float-divide-by-zero|function|shift-base|shift-exponent|vla-bound|alignment|null|vptr|object-size|float-cast-overflow|array-bounds|enum|bool|returns-nonnull-attribute|nonnull-attribute),?){17}"}}
 // CHECK-NO-RECOVER-NOT: sanitize-recover
----------------
Shouldn't `address` be added here too?


Repository:
  rL LLVM

http://reviews.llvm.org/D12318