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

[Kostya Serebryany via llvm-commits]

kcc added inline comments.

================
Comment at: lib/asan/asan_flags.inc:147
@@ +146,3 @@
+          "WARNING: USE AT YOUR OWN RISK!)")
+ASAN_FLAG(uptr, max_errors, 10,
+          "Maximum number of errors reported if halting-on-error is disabled")
----------------
Shouldn't this go to sanitizer_common? 

================
Comment at: lib/asan/asan_interceptors.cc:52
@@ -51,2 +51,3 @@
 #define ACCESS_MEMORY_RANGE(ctx, offset, size, isWrite) do {            \
+    if (QuickCheckSilence(false)) break;                                \
     uptr __offset = (uptr)(offset);                                     \
----------------
why do we need it here?  

================
Comment at: lib/asan/asan_internal.h:124
@@ +123,3 @@
+// unnecessary.
+static inline bool QuickCheckSilence(bool is_error) {
+  if (LIKELY(flags()->halt_on_error))
----------------
Does it have to be static inline? 
Also, I don't like the name. But it depends on another question later. 

================
Comment at: lib/asan/asan_report.cc:29
@@ +28,3 @@
+#define QUICK_CHECK_SILENCE \
+  if (QuickCheckSilence(true)) return
+
----------------
Don't define this macro, just use the C++ line as is

================
Comment at: lib/asan/asan_report.cc:697
@@ +696,3 @@
+    lock.Unlock();
+    if (LIKELY(halt_on_error)) {
+      Report("ABORTING\n");
----------------
Agree. 

================
Comment at: lib/asan/asan_report.cc:1014
@@ -966,3 +1013,3 @@
 void __asan_report_error(uptr pc, uptr bp, uptr sp, uptr addr, int is_write,
-                         uptr access_size, u32 exp) {
+                         uptr access_size, u32 exp, int fatal) {
   ENABLE_FRAME_POINTER;
----------------
I think we should revert the name and the meaning of the flag: 
make it "int recover" instead of "int fatal" to match the flag. 
No? 

Also, we need to make the parameter more clear. 
One way is to declare a enum {ABORT_ON_ERROR, RECOVER_ON_ERROR} and pass it as the parameter. 
Another is to 
define two more internal functions 
__asan_report_error_and_abort() &
__asan_report_error_and_maybe_recover()
and instead of calling __asan_report_error with a constant in the last parameter call one of these.

At the very least, write /*recover=*/false

================
Comment at: test/asan/TestCases/Linux/halt_on_error-torture.cc:29
@@ +28,3 @@
+    // Expect error collisions here
+    // CHECK: AddressSanitizer: use-after-poison
+    volatile int idx = 0;
----------------
don't we want to check that multiple errors were reported? 


check how the tests set ASAN_OPTIONS nowadays. 
There were some massive changes... 

================
Comment at: test/asan/TestCases/Linux/halt_on_error-torture.cc:44
@@ +43,3 @@
+
+  pthread_t *tids = (pthread_t *)malloc(nthreads * sizeof(pthread_t));
+
----------------
use C++ new/delete 


Repository:
  rL LLVM

http://reviews.llvm.org/D12318