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

[Dmitry Vyukov via Phabricator via llvm-commits]

dvyukov 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();
----------------
vitalybuka wrote:
> 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.
Right. It's not static. Withdrawn.


https://reviews.llvm.org/D38019