[PATCH] D54521: tsan: Add pthread_tryjoin_np and pthread_timedjoin_np interceptors

[Yuri Per via Phabricator via llvm-commits]

yuri added inline comments.


================
Comment at: lib/tsan/rtl/tsan_interceptors.cc:1057
+  else
+    ThreadNotJoined(thr, pc, tid, (uptr)th);
+  return res;
----------------
dvyukov wrote:
> Why is this necessary?
> We've already set it in CreateThread.
> Also this looks racy and can CHECK-fail. Consider that this pthread_tryjoin fails but another thread concurrently also executes pthread_tryjoin and it succeeds and thread is destroyed.
This is needed because ThreadTid()/FindThreadByUid() clears user_id. This behaviour can be traced back to commit https://github.com/llvm-mirror/compiler-rt/commit/411b2c9d6787b64939fc15fdeec65e9d65ba1a51.

Concurrent call of pthread_tryjoin() or any other join functions is racy because you can end up with already joined thread and access freed memory. Problem of current thread sanitizer code that in case of such racy application it fails CHECK instead of reporting clear error.

I personally do no like such implementation. Instead of clearing user_id, it would be better to set flag "thread being joined" and store stack context of calling thread. If later someone attempts to join the same thread – report an error. In such case ThreadNotJoined() would clear this flag.


================
Comment at: test/tsan/Linux/thread_timedjoin.c:15
+  pthread_create(&t, 0, Thread, 0);
+  barrier_wait(&barrier);
+  struct timespec ts;
----------------
dvyukov wrote:
> Why do we need a barrier in this test? It looks like it's deterministic without the barrier.
I just copied it from pthread_deatch test. Actually brarier can be useful in this test if it is moved farther . I will update tests.


================
Comment at: test/tsan/Linux/thread_timedjoin.c:22
+  }
+  fprintf(stderr, "PASS\n");
+  return 0;
----------------
dvyukov wrote:
> Let's also check that pthread_timedjoin_np provides necessary synchronization. It's easy to add to this test and that's an important guarantee of joining a thread.
> I.e. create a global var, write to it in the thread function and write in main after join. It must not race.
> That will also check that pthread_timedjoin_np did not return some other error.
> 
OK


https://reviews.llvm.org/D54521