Workaround TSan false positive in lookup_thread called by suspend_handler

* include/private/pthread_support.h (THREAD_TABLE_INDEX): Move the
macro from pthread_support.c file.
* pthread_stop_world.c [!GC_OPENBSD_UTHREADS && !NACL
&& THREAD_SANITIZER] (GC_lookup_thread_async): New static function
(same implementation as of GC_lookup_thread but with
GC_ATTR_NO_SANITIZE_THREAD attribute); add comment.
* pthread_stop_world.c [!GC_OPENBSD_UTHREADS && !NACL
&& !THREAD_SANITIZER] (GC_lookup_thread_async): Define as macro.
* pthread_stop_world.c [!GC_OPENBSD_UTHREADS && !NACL]
(GC_suspend_handler_inner): Call GC_lookup_thread_async() instead of
GC_lookup_thread(); move the corresponding comment to
GC_lookup_thread_async.

diff --git a/include/private/pthread_support.h b/include/private/pthread_support.h
index 20b08ea..4ab9c21 100644 (file)
--- a/include/private/pthread_support.h
+++ b/include/private/pthread_support.h
@@ -125,6 +125,12 @@ typedef struct GC_Thread_Rep {
 #ifndef THREAD_TABLE_SZ
 # define THREAD_TABLE_SZ 256    /* Power of 2 (for speed). */
 #endif
+
+#define THREAD_TABLE_INDEX(id) \
+                (int)(((NUMERIC_THREAD_ID(id) >> 16) \
+                       ^ (NUMERIC_THREAD_ID(id) >> 8) \
+                       ^ NUMERIC_THREAD_ID(id)) % THREAD_TABLE_SZ)
+
 GC_EXTERN volatile GC_thread GC_threads[THREAD_TABLE_SZ];
 
 GC_EXTERN GC_bool GC_thr_initialized;

diff --git a/pthread_stop_world.c b/pthread_stop_world.c
index b5a79c3..4eba05c 100644 (file)
--- a/pthread_stop_world.c
+++ b/pthread_stop_world.c
@@ -240,6 +240,27 @@ STATIC void GC_suspend_handler_inner(ptr_t dummy, void *context);
   errno = old_errno;
 }
 
+/* The lookup here is safe, since this is done on behalf        */
+/* of a thread which holds the allocation lock in order         */
+/* to stop the world.  Thus concurrent modification of the      */
+/* data structure is impossible.  Unfortunately, we have to     */
+/* instruct TSan that the lookup is safe.                       */
+#ifdef THREAD_SANITIZER
+  /* The implementation of the function is the same as that of  */
+  /* GC_lookup_thread except for the attribute added here.      */
+  GC_ATTR_NO_SANITIZE_THREAD
+  static GC_thread GC_lookup_thread_async(pthread_t id)
+  {
+    GC_thread p = GC_threads[THREAD_TABLE_INDEX(id)];
+
+    while (p != NULL && !THREAD_EQUAL(p->id, id))
+      p = p->next;
+    return p;
+  }
+#else
+# define GC_lookup_thread_async GC_lookup_thread
+#endif
+
 GC_ATTR_NO_SANITIZE_THREAD
 static void update_last_stop_count(GC_thread me, AO_t my_stop_count)
 {
@@ -267,11 +288,7 @@ STATIC void GC_suspend_handler_inner(ptr_t dummy GC_ATTR_UNUSED,
     GC_log_printf("Suspending %p\n", (void *)self);
 # endif
 
-  me = GC_lookup_thread(self);
-  /* The lookup here is safe, since I'm doing this on behalf    */
-  /* of a thread which holds the allocation lock in order       */
-  /* to stop the world.  Thus concurrent modification of the    */
-  /* data structure is impossible.                              */
+  me = GC_lookup_thread_async(self);
 
 # ifdef GC_ENABLE_SUSPEND_THREAD
     if ((me -> flags & SUSPENDED_EXT) != 0) {

diff --git a/pthread_support.c b/pthread_support.c
index cbb66c5..90602de 100644 (file)
--- a/pthread_support.c
+++ b/pthread_support.c
@@ -528,11 +528,6 @@ void GC_push_thread_structures(void)
 /* It may not be safe to allocate when we register the first thread.    */
 static struct GC_Thread_Rep first_thread;
 
-#define THREAD_TABLE_INDEX(id) \
-                (int)(((NUMERIC_THREAD_ID(id) >> 16) \
-                       ^ (NUMERIC_THREAD_ID(id) >> 8) \
-                       ^ NUMERIC_THREAD_ID(id)) % THREAD_TABLE_SZ)
-
 /* Add a thread to GC_threads.  We assume it wasn't already there.      */
 /* Caller holds allocation lock.                                        */
 STATIC GC_thread GC_new_thread(pthread_t id)