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

[Alexey Samsonov via llvm-commits]

samsonov added inline comments.

================
Comment at: lib/asan/asan_internal.h:122
@@ +121,3 @@
+// unnecessary.
+static inline bool QuickCheckSilence(bool is_error) {
+  if (LIKELY(flags()->halt_on_error))
----------------
ygribov wrote:
> 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.
The name is really confusing. It should be smth. like `IgnoreError`. Also, instead of passing `is_error` as a parameter, and doing conditional atomic_fetch_add, can you just split this function into two: one would check if need to ignore the error (halt_on_error is true, or number of reported errors is too large already), and another one would increment the number of detected errors. (could you then call the latter one from `ScopedInErrorReport` constructor)?

Can you make it ALWAYS_INLINE?

================
Comment at: lib/asan/asan_report.cc:698
@@ +697,3 @@
+  static StaticSpinMutex lock;
+  static u32 reporting_thread_tid;
+  bool halt_on_error;
----------------
ygribov wrote:
> 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.
There are no trailing _ in simple structs, but there should be in classes.


Repository:
  rL LLVM

http://reviews.llvm.org/D12318