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

[Yury Gribov via llvm-commits]

ygribov added a comment.

Thanks a lot for detailed review!

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


Yup.


================
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)
----------------
filcab wrote:
> 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)
Sorry, could you give more details? I indeed favor the default (non-recovery) case here. As for the UNLIKELY, we've seen cases where it significantly improved performance.

================
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;
+  }
----------------
filcab wrote:
> ygribov wrote:
> > samsonov wrote:
> > > We have atomic_fetch_add
> > Yeah but it may be slower.
> Slower is better than undefined behavior, no? ;-)
Again, I was probably too scary to slow down the default path. I'll switch to atomics in next version then.

================
Comment at: lib/asan/asan_poisoning.cc:221
@@ -220,3 +220,3 @@
       uptr __bad = __asan_region_is_poisoned(__p, __size);    \
-      __asan_report_error(pc, bp, sp, __bad, isWrite, __size, 0);\
+      __asan_report_error(pc, bp, sp, __bad, isWrite, __size, 0, false); \
     }                                                         \
----------------
BTW what are these sanitizer_unaligned functions for? I don't think compiler emits then but they are also missing in public headers.

================
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;
----------------
filcab wrote:
> cmpxchg should work, no?
Yup.

================
Comment at: lib/asan/asan_report.cc:652
@@ +651,3 @@
+            if (TryLockReporting())
+              goto locked;
+          }
----------------
filcab wrote:
> At the very least, you can create a helper function with the code executed when you get the lock, no?
Ah, so it's all about this tiny goto (not about hacky trial-and-error mutex acquisition). Ok, I'll put the code to a separate function.

================
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) {
----------------
filcab wrote:
> This comment should be changed, too. It's not true after this diff gets applied.
Right.

================
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.
----------------
filcab wrote:
> Why did you remove the first part of the comment?
> It's still the reason why we have the `sleep` call here.
I moved the first part of the comment above because it's applicable to both branches.

================
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)
----------------
filcab wrote:
> Why? I might have missed the reason for this change.
Hm, I don't remember doing this hunk at all. I'll double check when at work.

================
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
----------------
filcab wrote:
> Shouldn't `address` be added here too?
Ok.


Repository:
  rL LLVM

http://reviews.llvm.org/D12318