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

[Pedro Alves <pedro@palves.net>]

I'm dropping this patch from a v2 of this series, because I've since remembered
that current glibc is eliminating separte libpthread, merging it into the main library, and
gdb will load libthread_db.so even with non-threaded programs, so that printing errno
always works...  This all means that with new enough glibc, I think we'd get back the
warnings...

Pedro Alves

On 2022-06-20 11:53 p.m., Pedro Alves wrote:
> 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
>  	}
>