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

[Filipe Cabecinhas via llvm-commits]

filcab added a comment.

Still missing the Darwin interface symbols file.

Thanks for working on this.

Filipe


================
Comment at: lib/asan/asan_interceptors.cc:73
@@ -71,3 +72,3 @@
         GET_CURRENT_PC_BP_SP;                                           \
-        __asan_report_error(pc, bp, sp, __bad, isWrite, __size, 0);     \
+        __asan_report_error(pc, bp, sp, __bad, isWrite, __size, 0, false);\
       }                                                                 \
----------------
Why `false`? Shouldn't it be the flag for `halt_on_error`?

================
Comment at: lib/asan/asan_internal.h:125
@@ +124,3 @@
+static inline bool QuickCheckSilence(bool is_error) {
+  if (LIKELY(flags()->halt_on_error))
+    return false;
----------------
So, I see two things that can happen:

`halt_on_error` is true:
 We go through the `if` once (maybe more times, if we find several errors in several threads).
 `QuickCheckSilence` returns false
 `LIKELY` Isn't really doing much. Tweaking a jump that is executed once doesn't really matter.

`halt_on_error` is false:
 We go through the `if` roughly `max_errors` times (let's say around 100, which seems to me is already a too big number for `max_errors` (if you actually manage to skip 100 errors and still execute code, that's a huge amount of luck!)

>From these two, I would actually say that, at most, you should have an `UNLIKELY`, and not a `LIKELY`. *If* we execute that `if` more than once, then I should always expect it to be false.

Still, even in the case where we execute it "a bunch of times" we're still "almost never" executing it. `QuickCheckSilence` is called only when reporting errors, which means that:
 If `halt_on_error`, then the program is terminating right after this report. We're not winning anything.
 If `halt_on_error`, then the `LIKELY` is reversed. And we're still not winning much, we're not expecting this code to be executed more than `max_errors`, which, I'm assuming, will always be a small number.

My opinion is that it's not making the code faster, and it's making it less clear.

You mentioned performance increases. What tests were you running, and how?

================
Comment at: lib/asan/asan_internal.h:131
@@ +130,3 @@
+  if (is_error)
+    atomic_fetch_add(&num_reported_errors, 1, memory_order_relaxed);
+  return false;
----------------
You can change the previous `atomic_load` to be an `atomic_fetch_add` of 0 or 1, no?

================
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-RECOVER: "-fsanitize-recover={{((address|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
----------------
Why didn't you update the number too? Does the test pass?

================
Comment at: test/asan/TestCases/Linux/halt_on_error-torture.cc:1
@@ +1,2 @@
+// Stress test recovery mode with many threads.
+//
----------------
Why is this test linux only?


Repository:
  rL LLVM

http://reviews.llvm.org/D12318