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

[Kostya Serebryany via llvm-commits]

kcc added a comment.

The patch is already too long and has too many comments. 
Some parts are less controversial, some are more. 
I suggest you to split it at this point and start reviewing/submitting it in parts. 
Probably start from the driver/flags changes.


================
Comment at: lib/Transforms/Instrumentation/AddressSanitizer.cpp:1099
@@ -1083,2 +1098,3 @@
     Value *Cmp2 = createSlowPathCmp(IRB, AddrLong, ShadowValue, TypeSize);
-    BasicBlock *CrashBlock =
+    if (Recover)
+      CrashTerm = SplitBlockAndInsertIfThen(Cmp2, CheckTerm, false);
----------------
I think we use {} in such cases (if followed by an else with {})

================
Comment at: lib/asan/asan_flags.inc:141
@@ +140,3 @@
+          "WARNING: USE AT YOUR OWN RISK!)")
+ASAN_FLAG(uptr, max_errors, 10,
+          "Maximum number of errors reported if halting-on-error is disabled")
----------------
No, just add it to sanitizer_common and write something like ("currently supported by asan") in the description. 

If we add things that are potentially useful to other sanitizers to one of them (not to sanitizer_common) 
we then end up having duplicates in all sanitizers. 


================
Comment at: lib/asan/asan_internal.h:122
@@ +121,3 @@
+// unnecessary.
+static inline bool QuickCheckSilence(bool is_error) {
+  if (LIKELY(flags()->halt_on_error))
----------------
I don't like the name, 
I don't like the fact that it has a parameter, 
I don't like that it's static inline, 
I don't like its behavior. 

We have two situations: 
1. We are reporting an error and don't want to do it (is_error=true). 
There is no harm in making this function non-inline in this case. 
bool WantToReportMoreError(); 

2. We don't know whether there will be an error, but we are shutting down the checking functionality
just in case (or to win a bit of performance). 
I don't think we need to do this at all. 
The win is not clear, but the added complexity is. 



================
Comment at: lib/asan/asan_report.cc:626
@@ +625,3 @@
+                               bool fatal = false) {
+    halt_on_error = fatal || flags()->halt_on_error;
+
----------------
This is all exceptionally horrible.
I've spent quite a bit of time and failed to understand how it works, 
also I don't understand how to test it. 
Why not just grab a lock for the entire duration of error reporting? 

The current logic was there because we never reported more than one bug, 
now that we do -- simply grab a lock! 

================
Comment at: lib/asan/asan_report.cc:698
@@ +697,3 @@
+  static StaticSpinMutex lock;
+  static u32 reporting_thread_tid;
+  bool halt_on_error;
----------------
class members have the  _ suffix. 

================
Comment at: lib/asan/asan_rtl.cc:126
@@ +125,3 @@
+  GET_CALLER_PC_BP_SP;                                              \
+  ReportGenericError(pc, bp, sp, addr, is_write, size, 0, false);   \
+}                                                                   \
----------------
please leave  __asan_report_error here. It is used as a target for break points in debugger. 

You may then check halt_on_error inside __asan_report_error


Repository:
  rL LLVM

http://reviews.llvm.org/D12318