tsan: make invisible test barrier portable

[dvyukov]

Summary

The current implementation does not work on darwin and can have issues with other OSes in future. See http://reviews.llvm.org/D14427
Make it portable once and for all (minus usleep call).

[kubamracek]

Yes, this works for me, thanks!

Just a quick question: Wouldn't usleep() invoke the interceptor and establish an unwanted synchronization?

[kubamracek]

Also, this fails a single test on Linux for me, thread_name2.cc. I didn't investigate further.

[kubamracek]

Huh, changing *barrier = count; in the initialization into:

__atomic_store_n(barrier, count, __ATOMIC_RELAXED);

fixes the failing test. But I'm not going to pretend that I understand why exactly was it failing...

[dvyukov]

Also, this fails a single test on Linux for me, thread_name2.cc. I didn't investigate further.

How exactly does it fail? Please also run it as a stand-alone binary and post output.
FWIW I run linux tests 30 times before uploading this change. All passed.

Huh, changing *barrier = count; in the initialization into:

I don't understand how it can affect anything. Initialization happens before any threads are started.

Just a quick question: Wouldn't usleep() invoke the interceptor and establish an unwanted synchronization?

usleep is not synchronization logically. So if the interceptor introduces any synchronization, then it's a bug that we will need to fix. Hopefully the tests will catch it, there are positive tests that must detect races and they should be pretty sensitive to any parasitic synchronization.

[kubamracek]

In D14434#283439, @dvyukov wrote:

Also, this fails a single test on Linux for me, thread_name2.cc. I didn't investigate further.

How exactly does it fail? Please also run it as a stand-alone binary and post output.
FWIW I run linux tests 30 times before uploading this change. All passed.

It doesn't detect any race, the output is empty. FYI, it's an Ubuntu Server 15.10 machine.

[kubamracek]

Also, the test (thread_name2.cc) compiles under -O1. If I change it to -O0, -O2 or -O3, it passes (!).

[dvyukov]

Does changing 'int Global;' to 'long long Global;' fix it?

[dvyukov]

Or better change:
typedef unsigned invisible_barrier_t;
to:
typedef unsigned long long invisible_barrier_t;
in this patch.

[kubamracek]

Does changing 'int Global;' to 'long long Global;' fix it?

Yes, this fixes it.

Or better change typedef unsigned invisible_barrier_t; to: typedef unsigned long long invisible_barrier_t;

This works as well.

What's going on here, can you explain it to me?

[dvyukov]

In D14434#283454, @dvyukov wrote:

Or better change:
typedef unsigned invisible_barrier_t;
to:
typedef unsigned long long invisible_barrier_t;
in this patch.

Reuploaded this patch with this change. Should fix the issue.

[dvyukov]

The problem was that tsan shadow has 8-byte granularity. When I changed pthread_barrier_t (which is at least 8 bytes) to unsigned, it started being collocated with the Global variable in the same 8-bytes, and that caused shadow slot evictions for Global.

[kubamracek]

Ah, that makes sense. Thanks!

[dvyukov]

Committed in 252292

[kubamracek]

Follow up question: Wouldn't it be good to simply force all globals to be at least 8-aligned, when using -fsanitize=thread?

[dvyukov]

Filed https://github.com/google/sanitizers/issues/621 for this. Thanks for suggestion.

test/tsan/test.h

 #include <pthread.h>
 #include <stdlib.h>
 #include <stdio.h>
 #include <unistd.h>
 #include <dlfcn.h>
 #include <stddef.h>
 
 // TSan-invisible barrier.
 // Tests use it to establish necessary execution order in a way that does not
 // interfere with tsan (does not establish synchronization between threads).
-__typeof(pthread_barrier_wait) *barrier_wait;
+// 8 lsb is thread count, the remaining are count of entered threads.
+typedef unsigned long long invisible_barrier_t;
 
-void barrier_init(pthread_barrier_t *barrier, unsigned count) {
-#if defined(__FreeBSD__)
-  static const char libpthread_name[] = "libpthread.so";
-#else
-  static const char libpthread_name[] = "libpthread.so.0";
-#endif
-
-  if (barrier_wait == 0) {
-    void *h = dlopen(libpthread_name, RTLD_LAZY);
-    if (h == 0) {
-      fprintf(stderr, "failed to dlopen %s, exiting\n", libpthread_name);
-      exit(1);
-    }
-    barrier_wait = (__typeof(barrier_wait))dlsym(h, "pthread_barrier_wait");
-    if (barrier_wait == 0) {
-      fprintf(stderr, "failed to resolve pthread_barrier_wait, exiting\n");
-      exit(1);
-    }
+void barrier_init(invisible_barrier_t *barrier, unsigned count) {
+  if (count >= (1 << 8))
+      exit(fprintf(stderr, "barrier_init: count is too large (%d)\n", count));
+  *barrier = count;
+}
+
+void barrier_wait(invisible_barrier_t *barrier) {
+  unsigned old = __atomic_fetch_add(barrier, 1 << 8, __ATOMIC_RELAXED);
+  unsigned old_epoch = (old >> 8) / (old & 0xff);
+  for (;;) {
+    unsigned cur = __atomic_load_n(barrier, __ATOMIC_RELAXED);
+    unsigned cur_epoch = (cur >> 8) / (cur & 0xff);
+    if (cur_epoch != old_epoch)
+      return;
+    usleep(1000);
   }
-  pthread_barrier_init(barrier, 0, count);
 }
 
 // Default instance of the barrier, but a test can declare more manually.
-pthread_barrier_t barrier;
+invisible_barrier_t barrier;
 
 void print_address(void *address) {
 // On FreeBSD, the %p conversion specifier works as 0x%x and thus does not match
 // to the format used in the diagnotic message.
 #ifdef __x86_64__
   fprintf(stderr, "0x%012lx", (unsigned long) address);
 #elif defined(__mips64)
   fprintf(stderr, "0x%010lx", (unsigned long) address);

test/tsan/thread_name2.cc

 // RUN: %clangxx_tsan -O1 %s -o %t && %deflake %run %t | FileCheck %s
 #include "test.h"
 
 #if defined(__FreeBSD__)
 #include <pthread_np.h>
 #define pthread_setname_np pthread_set_name_np
 #endif
 
-int Global;
+long long Global;
 
 void *Thread1(void *x) {
   barrier_wait(&barrier);
   Global++;
   return 0;
 }
 
 void *Thread2(void *x) {