tsan: fix handling of condition variable destruction

[dvyukov]

Summary

POSIX states that "It shall be safe to destroy an initialized condition variable upon which no threads are currently blocked", and later clarifies "A condition variable can be destroyed immediately after all the threads that are blocked on it are awakened) (in examples section). Tsan reported such destruction as a data race.
Fixes https://llvm.org/bugs/show_bug.cgi?id=23616

[EricWF]

The test case and fix look right to me. Couldn't you just use the exit code of the test directly instead of using FileCheck?

[dvyukov]

The exit code is also checked. lit test driver ensure that all commands exit with 0 status (we run tests that contain races with 'not' bash command). So that's kind of two levels of protection :)

[dvyukov]

Submitted in 241082.

lib/tsan/rtl/tsan_interceptors.cc

INTERCEPTOR(int, pthread_cond_init, void *c, void *a) {
   SCOPED_TSAN_INTERCEPTOR(pthread_cond_init, cond, a);
   MemoryAccessRange(thr, pc, (uptr)c, sizeof(uptr), true);
   return REAL(pthread_cond_init)(cond, a);
 }
 
 INTERCEPTOR(int, pthread_cond_wait, void *c, void *m) {
   void *cond = init_cond(c);
   SCOPED_TSAN_INTERCEPTOR(pthread_cond_wait, cond, m);
-  MutexUnlock(thr, pc, (uptr)m);
   MemoryAccessRange(thr, pc, (uptr)c, sizeof(uptr), false);
+  MutexUnlock(thr, pc, (uptr)m);
   CondMutexUnlockCtx arg = {&si, thr, pc, m};
   int res = 0;
   // This ensures that we handle mutex lock even in case of pthread_cancel.
   // See test/tsan/cond_cancel.cc.
   {
     // Enable signal delivery while the thread is blocked.
     BlockingCall bc(thr);
     res = call_pthread_cancel_with_cleanup(
         (int(*)(void *c, void *m, void *abstime))REAL(pthread_cond_wait),
         cond, m, 0, (void(*)(void *arg))cond_mutex_unlock, &arg);
   }
   if (res == errno_EOWNERDEAD)
     MutexRepair(thr, pc, (uptr)m);
   MutexLock(thr, pc, (uptr)m);
   return res;
 }
 
 INTERCEPTOR(int, pthread_cond_timedwait, void *c, void *m, void *abstime) {
   void *cond = init_cond(c);
   SCOPED_TSAN_INTERCEPTOR(pthread_cond_timedwait, cond, m, abstime);
-  MutexUnlock(thr, pc, (uptr)m);
   MemoryAccessRange(thr, pc, (uptr)c, sizeof(uptr), false);
+  MutexUnlock(thr, pc, (uptr)m);
   CondMutexUnlockCtx arg = {&si, thr, pc, m};
   int res = 0;
   // This ensures that we handle mutex lock even in case of pthread_cancel.
   // See test/tsan/cond_cancel.cc.
   {
     BlockingCall bc(thr);
     res = call_pthread_cancel_with_cleanup(
         REAL(pthread_cond_timedwait), cond, m, abstime,

test/tsan/cond_destruction.cc

+// RUN: %clangxx_tsan -O1 %s -o %t
+// RUN: %run %t 2>&1 | FileCheck %s
+// RUN: %run %t arg 2>&1 | FileCheck %s
+// RUN: %run %t arg arg 2>&1 | FileCheck %s
+#include "test.h"
+
+// Test for destruction of pthread_cond_t.
+// POSIX states that it is safe  to destroy a condition variable upon which no
+// threads are currently blocked. That is, it is not necessary to wait untill
+// other threads return from pthread_cond_wait, they just need to be unblocked.
+
+pthread_mutex_t m;
+pthread_cond_t c;
+bool done1, done2;
+
+void *thr(void *p) {
+  pthread_mutex_lock(&m);
+  done1 = true;
+  pthread_cond_signal(&c);
+  while (!done2)
+    pthread_cond_wait(&c, &m);
+  pthread_mutex_unlock(&m);
+  return 0;
+}
+
+int main(int argc, char **argv) {
+  pthread_t th;
+  pthread_mutex_init(&m, 0);
+  pthread_cond_init(&c, 0);
+  pthread_create(&th, 0, thr, 0);
+  pthread_mutex_lock(&m);
+  while (!done1)
+    pthread_cond_wait(&c, &m);
+  done2 = true;
+  // Any of these sequences is legal.
+  if (argc == 1) {
+    pthread_cond_signal(&c);
+    pthread_mutex_unlock(&m);
+    pthread_cond_destroy(&c);
+  } else if (argc == 2) {
+    pthread_mutex_unlock(&m);
+    pthread_cond_signal(&c);
+    pthread_cond_destroy(&c);
+  } else {
+    pthread_cond_signal(&c);
+    pthread_cond_destroy(&c);
+    pthread_mutex_unlock(&m);
+  }
+  pthread_join(th, 0);
+  fprintf(stderr, "DONE\n");
+}
+
+// CHECK-NOT: ThreadSanitizer: data race