tsan: lock ScopedErrorReportLock around fork

[dvyukov]

Summary

Currently we don't lock ScopedErrorReportLock around fork
and it mostly works becuase tsan has own report_mtx that
is locked around fork and tsan reports.
However, sanitizer_common code prints some own reports
which are not protected by tsan's report_mtx. So it's better
to lock ScopedErrorReportLock explicitly.

[melver]

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_report.cpp

256

Shouldn't the ACQUIRE() annotation go in the header, so that callers see this?

289–290

Any reason why CHECK_LOCKED is not added in this change, i.e. will it be added later?

[dvyukov]

improve thread safety annotations

[dvyukov]

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_report.cpp

256

Well, yes. The reason I put it here is that CommonSanitizerReportMutex is static in this source file, so there is no way to refer to it in the header file. I already encountered this situation in several other places, I guess the annotations were developed with the classical C++ OOP style in mind.

I've moved the mutex to the header and improved the annotations.

PTAL

compiler-rt/lib/sanitizer_common/sanitizer_common.h

 #define VPrintf(level, ...)                                              \
   do {                                                                   \
     if ((uptr)Verbosity() >= (level)) Printf(__VA_ARGS__); \
   } while (0)
 
 // Lock sanitizer error reporting and protects against nested errors.
 class ScopedErrorReportLock {
  public:
-  ScopedErrorReportLock();
-  ~ScopedErrorReportLock();
+  ScopedErrorReportLock() ACQUIRE(mutex_) { Lock(); }
+  ~ScopedErrorReportLock() RELEASE(mutex_) { Unlock(); }
 
-  static void CheckLocked();
+  static void Lock() ACQUIRE(mutex_);
+  static void Unlock() RELEASE(mutex_);
+  static void CheckLocked() CHECK_LOCKED(mutex_);
+
+ private:
+  static atomic_uintptr_t reporting_thread_;
+  static StaticSpinMutex mutex_;
 };
 
 extern uptr stoptheworld_tracer_pid;
 extern uptr stoptheworld_tracer_ppid;
 
 bool IsAccessibleMemoryRange(uptr beg, uptr size);
 
 // Error report formatting.

compiler-rt/lib/sanitizer_common/sanitizer_mutex.h

public:
   }
 
   bool TryLock() TRY_ACQUIRE(true) {
     return atomic_exchange(&state_, 1, memory_order_acquire) == 0;
   }
 
   void Unlock() RELEASE() { atomic_store(&state_, 0, memory_order_release); }
 
-  void CheckLocked() const CHECK_LOCKED {
+  void CheckLocked() const CHECK_LOCKED() {
     CHECK_EQ(atomic_load(&state_, memory_order_relaxed), 1);
   }
 
  private:
   atomic_uint8_t state_;
 
   void NOINLINE LockSlow() {
     for (int i = 0;; i++) {
public:
 
   // This function does not guarantee an explicit check that the calling thread
   // is the thread which owns the mutex. This behavior, while more strictly
   // correct, causes problems in cases like StopTheWorld, where a parent thread
   // owns the mutex but a child checks that it is locked. Rather than
   // maintaining complex state to work around those situations, the check only
   // checks that the mutex is owned, and assumes callers to be generally
   // well-behaved.
-  void CheckLocked() const CHECK_LOCKED;
+  void CheckLocked() const CHECK_LOCKED();
 
  private:
   // Solaris mutex_t has a member that requires 64-bit alignment.
   ALIGNED(8) uptr opaque_storage_[10];
   uptr owner_;  // for debugging
 };
 
 // Reader-writer spin mutex.
public:
 
   void ReadUnlock() RELEASE_SHARED() {
     u32 prev = atomic_fetch_sub(&state_, kReadLock, memory_order_release);
     DCHECK_EQ(prev & kWriteLock, 0);
     DCHECK_GT(prev & ~kWriteLock, 0);
     (void)prev;
   }
 
-  void CheckLocked() const CHECK_LOCKED {
+  void CheckLocked() const CHECK_LOCKED() {
     CHECK_NE(atomic_load(&state_, memory_order_relaxed), kUnlocked);
   }
 
  private:
   atomic_uint32_t state_;
 
   enum {
     kUnlocked = 0,

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_report.cpp

void HandleDeadlySignal(void *siginfo, void *context, u32 tid,
   SignalContext sig(siginfo, context);
   ReportDeadlySignal(sig, tid, unwind, unwind_context);
   Report("ABORTING\n");
   Die();
 }
 
 #endif  // !SANITIZER_FUCHSIA && !SANITIZER_GO
 
-static atomic_uintptr_t reporting_thread = {0};
-static StaticSpinMutex CommonSanitizerReportMutex;
+atomic_uintptr_t ScopedErrorReportLock::reporting_thread_ = {0};
+StaticSpinMutex ScopedErrorReportLock::mutex_;
 
-ScopedErrorReportLock::ScopedErrorReportLock() {
+void ScopedErrorReportLock::Lock() {
   uptr current = GetThreadSelf();
   for (;;) {
     uptr expected = 0;
-    if (atomic_compare_exchange_strong(&reporting_thread, &expected, current,
+    if (atomic_compare_exchange_strong(&reporting_thread_, &expected, current,
                                        memory_order_relaxed)) {
       // We've claimed reporting_thread so proceed.
-      CommonSanitizerReportMutex.Lock();
+      mutex_.Lock();
       return;
     }
 
     if (expected == current) {
       // This is either asynch signal or nested error during error reporting.
       // Fail simple to avoid deadlocks in Report().
 
       // Can't use Report() here because of potential deadlocks in nested
       // signal handlers.
       CatastrophicErrorWrite(SanitizerToolName,
                              internal_strlen(SanitizerToolName));
       static const char msg[] = ": nested bug in the same thread, aborting.\n";
       CatastrophicErrorWrite(msg, sizeof(msg) - 1);
 
       internal__exit(common_flags()->exitcode);
     }
 
     internal_sched_yield();
   }
 }
 
-ScopedErrorReportLock::~ScopedErrorReportLock() {
-  CommonSanitizerReportMutex.Unlock();
-  atomic_store_relaxed(&reporting_thread, 0);
+void ScopedErrorReportLock::Unlock() {
+  mutex_.Unlock();
+  atomic_store_relaxed(&reporting_thread_, 0);
 }
 
-void ScopedErrorReportLock::CheckLocked() {
-  CommonSanitizerReportMutex.CheckLocked();
-}
+void ScopedErrorReportLock::CheckLocked() { mutex_.CheckLocked(); }
 
 }  // namespace __sanitizer

compiler-rt/lib/sanitizer_common/sanitizer_thread_registry.h

public:
   ThreadRegistry(ThreadContextFactory factory);
   ThreadRegistry(ThreadContextFactory factory, u32 max_threads,
                  u32 thread_quarantine_size, u32 max_reuse);
   void GetNumberOfThreads(uptr *total = nullptr, uptr *running = nullptr,
                           uptr *alive = nullptr);
   uptr GetMaxAliveThreads();
 
   void Lock() ACQUIRE() { mtx_.Lock(); }
-  void CheckLocked() const CHECK_LOCKED { mtx_.CheckLocked(); }
+  void CheckLocked() const CHECK_LOCKED() { mtx_.CheckLocked(); }
   void Unlock() RELEASE() { mtx_.Unlock(); }
 
   // Should be guarded by ThreadRegistryLock.
   ThreadContextBase *GetThreadLocked(u32 tid) {
     return threads_.empty() ? nullptr : threads_[tid];
   }
 
   u32 CreateThread(uptr user_id, bool detached, u32 parent_tid, void *arg);

compiler-rt/lib/sanitizer_common/sanitizer_thread_safety.h

 #define ACQUIRE(...) THREAD_ANNOTATION(acquire_capability(__VA_ARGS__))
 #define ACQUIRE_SHARED(...) \
   THREAD_ANNOTATION(acquire_shared_capability(__VA_ARGS__))
 #define TRY_ACQUIRE(...) THREAD_ANNOTATION(try_acquire_capability(__VA_ARGS__))
 #define RELEASE(...) THREAD_ANNOTATION(release_capability(__VA_ARGS__))
 #define RELEASE_SHARED(...) \
   THREAD_ANNOTATION(release_shared_capability(__VA_ARGS__))
 #define EXCLUDES(...) THREAD_ANNOTATION(locks_excluded(__VA_ARGS__))
-#define CHECK_LOCKED THREAD_ANNOTATION(assert_capability(this))
+#define CHECK_LOCKED(...) THREAD_ANNOTATION(assert_capability(__VA_ARGS__))
 #define NO_THREAD_SAFETY_ANALYSIS THREAD_ANNOTATION(no_thread_safety_analysis)
 
 #endif

compiler-rt/lib/tsan/rtl/tsan_rtl.cpp

#endif
 
   return failed ? common_flags()->exitcode : 0;
 }
 
 #if !SANITIZER_GO
 void ForkBefore(ThreadState *thr, uptr pc) NO_THREAD_SAFETY_ANALYSIS {
   ctx->thread_registry->Lock();
   ctx->report_mtx.Lock();
+  ScopedErrorReportLock::Lock();
   // Suppress all reports in the pthread_atfork callbacks.
   // Reports will deadlock on the report_mtx.
   // We could ignore sync operations as well,
   // but so far it's unclear if it will do more good or harm.
   // Unnecessarily ignoring things can lead to false positives later.
   thr->suppress_reports++;
   // On OS X, REAL(fork) can call intercepted functions (OSSpinLockLock), and
   // we'll assert in CheckNoLocks() unless we ignore interceptors.
   thr->ignore_interceptors++;
 }
 
 void ForkParentAfter(ThreadState *thr, uptr pc) NO_THREAD_SAFETY_ANALYSIS {
   thr->suppress_reports--;  // Enabled in ForkBefore.
   thr->ignore_interceptors--;
+  ScopedErrorReportLock::Unlock();
   ctx->report_mtx.Unlock();
   ctx->thread_registry->Unlock();
 }
 
 void ForkChildAfter(ThreadState *thr, uptr pc) NO_THREAD_SAFETY_ANALYSIS {
   thr->suppress_reports--;  // Enabled in ForkBefore.
   thr->ignore_interceptors--;
+  ScopedErrorReportLock::Unlock();
   ctx->report_mtx.Unlock();
   ctx->thread_registry->Unlock();
 
   uptr nthread = 0;
   ctx->thread_registry->GetNumberOfThreads(0, 0, &nthread /* alive threads */);
   VPrintf(1, "ThreadSanitizer: forked new process with pid %d,"
       " parent had %d threads\n", (int)internal_getpid(), (int)nthread);
   if (nthread == 1) {