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

[Yury Gribov via llvm-commits]

ygribov added a comment.

> Some parts are less controversial, some are more.


What's your general impression on the feature though?

> I suggest you to split it at this point and start reviewing/submitting it in parts. 

>  Probably start from the driver/flags changes.


Hm but I don't see any functionality which can be moved to independent commit (except the flags part which is really minor). We could split to separate patches for rt, clang and llvm but I don't see how this is going to simplify review.


================
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")
----------------
kcc wrote:
> 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. 
> 
> 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.

That makes sense, we already have too many semi-duplicate flags.

================
Comment at: lib/asan/asan_internal.h:122
@@ +121,3 @@
+// unnecessary.
+static inline bool QuickCheckSilence(bool is_error) {
+  if (LIKELY(flags()->halt_on_error))
----------------
kcc wrote:
> 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. 
> 
> 
Problem is that lack of this check would cause program to crawl in some cases. But ok, this may live in a private branch.

================
Comment at: lib/asan/asan_report.cc:626
@@ +625,3 @@
+                               bool fatal = false) {
+    halt_on_error = fatal || flags()->halt_on_error;
+
----------------
kcc wrote:
> 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! 
> also I don't understand how to test it. 

Did you ever understand how to test original code :) ? The behavior for fatal errors didn't change by the way.

> Why not just grab a lock for the entire duration of error reporting?

Because we may run into second error while reporting the first one in the same thread. Grabbing a lock would cause a deadlock in this case.

================
Comment at: lib/asan/asan_report.cc:698
@@ +697,3 @@
+  static StaticSpinMutex lock;
+  static u32 reporting_thread_tid;
+  bool halt_on_error;
----------------
kcc wrote:
> class members have the  _ suffix. 
I'm not sure that's true for statics, I see numerous places where they aren't undered.


Repository:
  rL LLVM

http://reviews.llvm.org/D12318