[PATCH] D38019: [asan] Fix nested error detection

[Vitaly Buka via Phabricator via llvm-commits]

vitalybuka added inline comments.


================
Comment at: compiler-rt/lib/asan/asan_report.cc:126
   explicit ScopedInErrorReport(bool fatal = false) {
     halt_on_error_ = fatal || flags()->halt_on_error;
     u32 current_tid = GetCurrentTidOrInvalid();
----------------
dvyukov wrote:
> This looks like a data race.
> While one thread executes ScopedInErrorReport, another thread can do something like:
> 
> halt_on_error_ = fatal;
> if (flags()->halt_on_error) halt_on_error_ = true;
> 
> And then the first thread can fail to halt and the program will hang for 100 seconds.
> 
> Or, there can be 2 thread: one with fatal=true and another with fatal=false, and asan will halt after a non-fatal error which can cause user confusion.
> This looks like a data race.
> While one thread executes ScopedInErrorReport, another thread can do something like:
> halt_on_error_ = fatal;
> if (flags()->halt_on_error) halt_on_error_ = true;

Not sure I understand you.
halt_on_error_ is not static, and only one thread suppose to access particular ScopedInErrorReport instance.

> Or, there can be 2 thread: one with fatal=true and another with fatal=false, and asan will halt after a non-fatal error which can cause user confusion.

Correct. I noticed that this is weird too, but I believe it's preexisted behavior and not related to the bug I am fixing. So I was not trying to improve that.
If I understand correctly, to avoid such confusion, I can throw away "if (halt_on_error_) break;" And code after the loop.


================
Comment at: compiler-rt/lib/asan/asan_report.cc:129
 
-    if (halt_on_error_) {
-      // Do not print more than one report, otherwise they will mix up.
-      // Error reporting functions shouldn't return at this situation, as
-      // they are effectively no-returns.
+    for (int i = 0; i < 100; ++i) {
+      u32 expected_tid = kInvalidTid;
----------------
dvyukov wrote:
> This looks somewhat fishy. Is there any known problem with making this an infinite loop as it is now?
> Pretty sure SleepForMillis can undersleep if SIGPOF profiling is enabled, and then we can terminate the process while another thread still reports an error.
Probably nothing known.
I'll switch back to the infinit one.


https://reviews.llvm.org/D38019