[PATCH 01/25] Don't use pthread_mutex_t in gdb.base/step-over-clone.c

[Pedro Alves <pedro@palves.net>]

I noticed this in gdb.log after running gdb.base/step-over-clone.exp:

 ...
 gdbserver: PID mismatch!  Expected 1790818, got 1790817
 gdbserver: Cannot find thread after clone.
 gdbserver: PID mismatch!  Expected 1790819, got 1790817
 gdbserver: Cannot find thread after clone.
 gdbserver: PID mismatch!  Expected 1790820, got 1790817
 gdbserver: Cannot find thread after clone.
 ...

Those "PID mismatch" come from gdbserver/thread_db.c.  The problem is
that the testcase program is testing raw clone, which bypasses
libpthread entirely and leaves libthread_db confused.  The testcase is
linking with pthreads because it wants to use pthread_mutex_t for
synchronization between the clones.  Mixing pthreads and raw clone is
just something we shouldn't do, however.

My first thought was to fix this by using an atomic decrement
(__atomic_fetch_sub) instead of a mutex, for synchronization.
However, on some archs, that may require linking with -latomic, which
can itself pull in libpthread.

My next idea, is to make each thread write to its own "I'm ready"
variable, such that we can't actually have read-modify-write races.
This is what this patch does.

Change-Id: Id418978ac86bfa6d51d0af1e1625a86cdd039a20
---
 gdb/testsuite/gdb.base/step-over-clone.c     | 69 +++++++++++++-------
 gdb/testsuite/gdb.base/step-over-syscall.exp |  7 +-
 2 files changed, 45 insertions(+), 31 deletions(-)

diff --git a/gdb/testsuite/gdb.base/step-over-clone.c b/gdb/testsuite/gdb.base/step-over-clone.c
index c0f67af188b..8a56b492e5e 100644
--- a/gdb/testsuite/gdb.base/step-over-clone.c
+++ b/gdb/testsuite/gdb.base/step-over-clone.c
@@ -19,7 +19,7 @@
 #include <stdlib.h>
 #include <unistd.h>
 #include <sched.h>
-#include <pthread.h>
+#include <signal.h>
 
 static void
 marker ()
@@ -27,30 +27,55 @@ marker ()
 
 #define STACK_SIZE 0x1000
 
-/* These are used to signal that the threads have started correctly.  The
-   GLOBAL_THREAD_COUNT is set to the number of threads in main, then
-   decremented (under a lock) in each new thread.  */
-pthread_mutex_t global_lock = PTHREAD_MUTEX_INITIALIZER;
-int global_thread_count = 0;
+#define NUM_THREADS 6
+
+/* This is used to signal that the threads have started correctly.  We
+   can't use a single global updated by all thread guarded by a
+   pthread mutex, or anything pthread related for the matter, since we
+   are using raw clone.  A single global updated with atomics
+   (__atomic_fetch* etc.) instead of a pthread mutex would sound
+   appealing, but we avoid that too because for some architectures,
+   we'd have to link with -latomic, which itself links with
+   -lpthread...  So instead have one array with one element per
+   thread, and each thread only ever writes to its own array element.
+   We make the array have sig_atomic_t elements so that the elements
+   are portably naturally aligned and free from data races on all
+   archs, when different threads write to different elements.  In
+   practice, "int" would work too, as accesses to int are pretty much
+   garanteed to be atomic on all Linux systems, but sig_atomic_t is
+   explicit.  */
+volatile sig_atomic_t thread_started[NUM_THREADS];
 
 static int
-clone_fn (void *unused)
+clone_fn (void *started)
 {
   /* Signal that this thread has started correctly.  */
-  if (pthread_mutex_lock (&global_lock) != 0)
-    abort ();
-  global_thread_count--;
-  if (pthread_mutex_unlock (&global_lock) != 0)
-    abort ();
+  *(volatile sig_atomic_t *) started = 1;
 
   return 0;
 }
 
+/* Return true if all threads have started.  */
+
+static int
+all_threads_started (void)
+{
+  int i;
+
+  /* Force full memory barrier so that caches are flushed and
+     THREAD_STARTED is refetched.  */
+  __sync_synchronize ();
+  for (i = 0; i < NUM_THREADS; i++)
+    if (thread_started[i] == 0)
+      return 0;
+  return 1;
+}
+
 int
 main (void)
 {
   int i, pid;
-  unsigned char *stack[6];
+  unsigned char *stack[NUM_THREADS];
 
   /* Due to bug gdb/19675 the cloned thread _might_ try to reenter main
      (this depends on where the displaced instruction is placed for
@@ -62,18 +87,16 @@ main (void)
   else
     abort ();
 
-  for (i = 0; i < (sizeof (stack) / sizeof (stack[0])); i++)
+  for (i = 0; i < NUM_THREADS; i++)
     stack[i] = malloc (STACK_SIZE);
 
-  global_thread_count = (sizeof (stack) / sizeof (stack[0]));
-
-  for (i = 0; i < (sizeof (stack) / sizeof (stack[0])); i++)
+  for (i = 0; i < NUM_THREADS; i++)
     {
       pid = clone (clone_fn, stack[i] + STACK_SIZE, CLONE_FILES | CLONE_VM,
-		   NULL);
+		   (void *) &thread_started[i]);
     }
 
-  for (i = 0; i < (sizeof (stack) / sizeof (stack[0])); i++)
+  for (i = 0; i < NUM_THREADS; i++)
     free (stack[i]);
 
   /* Set an alarm so we don't end up stuck waiting for threads that might
@@ -81,12 +104,8 @@ main (void)
   alarm (120);
 
   /* Now wait for all the threads to start up.  */
-  while (global_thread_count != 0)
-    {
-      /* Force memory barrier so GLOBAL_THREAD_COUNT will be refetched.  */
-      asm volatile ("" ::: "memory");
-      sleep (1);
-    }
+  while (!all_threads_started ())
+    sleep (1);
 
   /* Call marker, this is what GDB is waiting for.  */
   marker ();
diff --git a/gdb/testsuite/gdb.base/step-over-syscall.exp b/gdb/testsuite/gdb.base/step-over-syscall.exp
index 788f6e3f5d0..e87d391cd5f 100644
--- a/gdb/testsuite/gdb.base/step-over-syscall.exp
+++ b/gdb/testsuite/gdb.base/step-over-syscall.exp
@@ -241,12 +241,7 @@ proc step_over_syscall { syscall } {
 
 	set testfile "step-over-$syscall"
 
-	set options [list debug]
-	if { $syscall == "clone" } {
-	    lappend options "pthreads"
-	}
-
-	if [build_executable ${testfile}.exp ${testfile} ${testfile}.c $options] {
+	if [build_executable ${testfile}.exp ${testfile} ${testfile}.c {debug}] {
 	    untested "failed to compile"
 	    return -1
 	}
-- 
2.36.0