[tsan] Handle pthread_mutex_timedlock() with PTHREAD_MUTEX_ROBUST

[jkt]

Summary

The interceptor for pthread_mutex_timedlock() was treating the EOWNERDEAD return value properly, and it instead assumed that that value is an actual error. TSAN therefore thought that the mutex was "not taken", and in the trace reports this appeared as if that robust mutex was not held at all for the recovery-after-a-death branch.

The test case is adopted from mutex_robust.cpp. In the thr() function, though, it is necessary to lock, write, unlock, and then lock again. If there was no unlock, then TSAN would report a race (which is was mutex_robust2.cpp tests for, so that's no change).

Discovered when trying to debug an issue in sysrepo; thank for Michal Vaško for a walkthrough through the original user code.

Bug: https://github.com/sysrepo/sysrepo/issues/2560

A git commit is available at https://github.com/jktjkt/llvm-project/commit/70e198ac563a2dfc20dabf082bcfcc28f4c5a52e .

[jkt]

Hi @dvyukov , just a gentle reminder. Can you please take a look at this patch?

[dvyukov]

Just back from vacation and getting through backlock.
The fix itself looks good to me, but there are some things in the test to improve.
Thanks

compiler-rt/test/tsan/Linux/mutex_robust3.cpp

9

Please make it long. Unit tests may have issues with variables less than 8 bytes due to tsan internal details.

26

Sleep-based coordination proved to be flaky on CI bots. 1 second was frequently not enough, and even 3 sec delays fired sometimes.
Please use a barrier instead (see other tests). I think it will be enough to ensure that thr has acquire m second time and then we can proceed with pthread_mutex_timedlock.

27

This looks like an uninit variable used in pthread_mutex_timedlock.

31

I think it may be useful to access i here to ensure that EOWNERDEAD was treated as successful acquisition by tsan and that it understands synchronization.

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

if (res == 0 || res == errno_EOWNERDEAD)
     MutexPostLock(thr, pc, (uptr)m, MutexFlagTryLock);
   return res;
 }
 
 #if !SANITIZER_MAC
 TSAN_INTERCEPTOR(int, pthread_mutex_timedlock, void *m, void *abstime) {
   SCOPED_TSAN_INTERCEPTOR(pthread_mutex_timedlock, m, abstime);
   int res = REAL(pthread_mutex_timedlock)(m, abstime);
-  if (res == 0) {
+  if (res == errno_EOWNERDEAD)
+    MutexRepair(thr, pc, (uptr)m);
+  if (res == 0 || res == errno_EOWNERDEAD) {
     MutexPostLock(thr, pc, (uptr)m, MutexFlagTryLock);
   }
   return res;
 }
 #endif
 
 #if !SANITIZER_MAC
 TSAN_INTERCEPTOR(int, pthread_spin_init, void *m, int pshared) {

compiler-rt/test/tsan/Linux/mutex_robust3.cpp

+// RUN: %clangxx_tsan -O1 %s -o %t && %run %t 2>&1 | FileCheck %s
+#include <pthread.h>
+#include <stdlib.h>
+#include <stdio.h>
+#include <unistd.h>
+#include <errno.h>
+
+pthread_mutex_t m;
+int i;
+
+void *thr(void *p) {
+  pthread_mutex_lock(&m);
+  i = 10;
+  pthread_mutex_unlock(&m); // so that TSAN "sees" the critical section properly
+  pthread_mutex_lock(&m);
+  return 0;
+}
+
+int main() {
+  pthread_mutexattr_t a;
+  pthread_mutexattr_init(&a);
+  pthread_mutexattr_setrobust(&a, PTHREAD_MUTEX_ROBUST);
+  pthread_mutex_init(&m, &a);
+  pthread_t th;
+  pthread_create(&th, 0, thr, 0);
+  sleep(1);
+  struct timespec ts;
+  if (pthread_mutex_timedlock(&m, &ts) != EOWNERDEAD) {
+    fprintf(stderr, "not EOWNERDEAD\n");
+    exit(1);
+  }
+  pthread_join(th, 0);
+  fprintf(stderr, "DONE\n");
+}
+
+// This is a correct code, and tsan must not bark.
+// CHECK-NOT: WARNING: ThreadSanitizer
+// CHECK-NOT: EOWNERDEAD
+// CHECK: DONE
+// CHECK-NOT: WARNING: ThreadSanitizer
+