gdb.threads/sigthread.c

gdb.threads/sigthread.c

[Mark Kettenis]

Hi Jim,

You recently changed gdb.threads/sigthread.c to use barriers.
Unfortunately the pthread_barrier_xxx() interfaces are not found in
the standard POSIX threads standard, but part of the Advanced Realtime
Threads POSIX extension.  Therefore they are not widely available.

Could you change the test to use a more standard synchronisation
mechanism?  I guess having a global variable of type volatile
sig_atomic_t, setting it to a non-zero value in main() (after the
threads have been created) and checking the value in child_two() and
thread_function() (perhaps calling sched_yield() if the variables
isn't set yet) would do the trick.

I can roll you a diff to do this if you want, but I don't think I have
access to a system that shows the problem you tried to fix.

Thanks,

Mark

Re: gdb.threads/sigthread.c

[Jim Blandy]

[-- Attachment #1: Type: text/plain, Size: 1353 bytes --]

On Feb 18, 2008 1:03 AM, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> You recently changed gdb.threads/sigthread.c to use barriers.
> Unfortunately the pthread_barrier_xxx() interfaces are not found in
> the standard POSIX threads standard, but part of the Advanced Realtime
> Threads POSIX extension.  Therefore they are not widely available.

Yeah, I mentioned this concern when I posted the patch.  I guess we know now.

> Could you change the test to use a more standard synchronisation
> mechanism?  I guess having a global variable of type volatile
> sig_atomic_t, setting it to a non-zero value in main() (after the
> threads have been created) and checking the value in child_two() and
> thread_function() (perhaps calling sched_yield() if the variables
> isn't set yet) would do the trick.

I'll bet this isn't the only test in the test suite that would like
some barriers.  How does the attached look?

> I can roll you a diff to do this if you want, but I don't think I have
> access to a system that shows the problem you tried to fix.

I developed the fix on a Core 2 Duo laptop running Linux, running
'while true; do :;done' in a few windows while running the test over
and over again as 'while runtest sigthread.exp; do :;done'.  Or
something like that.  It sometimes took a half-dozen iterations or so,
but it would fail eventually.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: jimb.gdb-barriers.patch --]
[-- Type: text/x-patch; name=jimb.gdb-barriers.patch, Size: 5700 bytes --]

gdb/testsuite/ChangeLog:
2008-02-19  Jim Blandy  <jimb@red-bean.com>

	* gdb.threads/gdb-barrier.c, gdb.threads/gdb-barrier.h: New files.
	* gdb.threads/sigthread.c: Use the gdb barrier, not the POSIX
	barriers, which are optional.
	* gdb.threads/sigthread.exp: Link the program with gdb-barrier.c.

diff -r b72cf86f9dad gdb/testsuite/gdb.threads/gdb-barrier.c
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/gdb/testsuite/gdb.threads/gdb-barrier.c	Tue Feb 19 09:49:56 2008 -0800
@@ -0,0 +1,70 @@
+/* gdb-barrier.c --- a portable implementation of barriers.  */
+
+#include <string.h>
+#include <stdlib.h>
+#include <stdio.h>
+#include <pthread.h>
+
+/* The header file has more comments and explanation.  */
+#include "gdb-barrier.h"
+
+static void
+check (int result, char *what)
+{
+  if (result)
+    {
+      fprintf (stderr, "error: %s: %s\n", what, strerror (result));
+      exit (1);
+    }
+}
+
+
+void
+gdb_barrier_init (struct gdb_barrier *b, int n)
+{
+  memset (b, 0, sizeof (*b));
+  check (pthread_mutex_init (&b->m, NULL),
+         "initializing barrier mutex");
+  check (pthread_cond_init (&b->c, NULL),
+         "initializing barrier condition");
+  b->full_count = n;
+  b->pending = n;
+}
+
+
+int
+gdb_barrier_wait (struct gdb_barrier *b)
+{
+  int result;
+
+  check (pthread_mutex_lock (&b->m),
+         "locking barrier mutex");
+  {
+    int my_cycle = b->cycle;
+
+    b->pending--;
+    if (b->pending > 0)
+      {
+        result = 0;
+
+        /* Condition variables are allowed to let threads wake up
+           spuriously, so we need to verify that the barrier has moved
+           on to the next cycle, and wait again if it hasn't.  */
+        while (b->cycle == my_cycle)
+          check (pthread_cond_wait (&b->c, &b->m),
+                 "waiting on barrier condition");
+      }
+    else
+      {
+        result = 1;
+        b->cycle++;
+        b->pending = b->full_count;
+        check (pthread_cond_broadcast (&b->c),
+               "broadcasting barrier condition");
+      }
+  }
+  check (pthread_mutex_unlock (&b->m),
+         "releasing barrier mutex");
+
+  return result;
+}
diff -r b72cf86f9dad gdb/testsuite/gdb.threads/gdb-barrier.h
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/gdb/testsuite/gdb.threads/gdb-barrier.h	Tue Feb 19 09:50:38 2008 -0800
@@ -0,0 +1,34 @@
+/* gdb-barrier.h --- barriers built on POSIX-only thread primitives.  */
+
+#ifndef GDB_BARRIER_H
+#define GDB_BARRIER_H
+
+#include <pthread.h>
+
+/* POSIX does provide barriers, but they're optional, and not all
+   systems implement them.  This is a simple implementation of
+   barriers in terms of the non-optional POSIX threads operations.  */
+
+struct gdb_barrier
+{
+  pthread_mutex_t m;            /* Mutex protecting this barrier.  */
+  pthread_cond_t c;             /* Queue of threads waiting at barrier.  */
+
+  int full_count;               /* Number of threads this barrier stops
+                                   per cycle.*/
+  int pending;                  /* Number of threads left to wait for.  */
+  int cycle;                    /* Current cycle number.  */
+};
+
+
+/* Initialize *B with a count of N (used by gdb_barrier_wait; see
+   below).  Exit on failure.  */
+extern void gdb_barrier_init (struct gdb_barrier *b, int n);
+
+/* Wait at barrier *B.  Once N threads are waiting on the barrier,
+   those N threads are allowed to continue.  This function returns
+   true in exactly one of the N threads allowed to continue, and false
+   in the other N-1 threads.  Exit on failure.  */
+extern int gdb_barrier_wait (struct gdb_barrier *b);
+
+#endif /* GDB_BARRIER_H */
diff -r b72cf86f9dad gdb/testsuite/gdb.threads/sigthread.c
--- a/gdb/testsuite/gdb.threads/sigthread.c	Tue Feb 19 09:36:34 2008 -0800
+++ b/gdb/testsuite/gdb.threads/sigthread.c	Tue Feb 19 09:37:13 2008 -0800
@@ -15,12 +15,14 @@
 #include <pthread.h>
 #include <signal.h>
 
+#include "gdb-barrier.h"
+
 /* Loop long enough for GDB to send a few signals of its own, but
    don't hang around eating CPU forever if something goes wrong during
    testing.  */
 #define NSIGS 10000000
 
-pthread_barrier_t barrier;
+struct gdb_barrier barrier;
 
 void
 handler (int sig)
@@ -36,7 +38,7 @@ child_two (void *arg)
 {
   int i;
 
-  pthread_barrier_wait (&barrier);
+  gdb_barrier_wait (&barrier);
 
   for (i = 0; i < NSIGS; i++)
     pthread_kill (child_thread, SIGUSR1);
@@ -47,7 +49,7 @@ thread_function (void *arg)
 {
   int i;
 
-  pthread_barrier_wait (&barrier);
+  gdb_barrier_wait (&barrier);
 
   for (i = 0; i < NSIGS; i++)
     pthread_kill (child_thread_two, SIGUSR2);
@@ -60,13 +62,13 @@ int main()
   signal (SIGUSR1, handler);
   signal (SIGUSR2, handler);
 
-  pthread_barrier_init (&barrier, NULL, 3);
+  gdb_barrier_init (&barrier, 3);
 
   main_thread = pthread_self ();
   pthread_create (&child_thread, NULL, thread_function, NULL);
   pthread_create (&child_thread_two, NULL, child_two, NULL);
 
-  pthread_barrier_wait (&barrier);
+  gdb_barrier_wait (&barrier);
 
   for (i = 0; i < NSIGS; i++)
     pthread_kill (child_thread_two, SIGUSR1);
diff -r b72cf86f9dad gdb/testsuite/gdb.threads/sigthread.exp
--- a/gdb/testsuite/gdb.threads/sigthread.exp	Tue Feb 19 09:36:34 2008 -0800
+++ b/gdb/testsuite/gdb.threads/sigthread.exp	Tue Feb 19 09:37:13 2008 -0800
@@ -18,7 +18,9 @@ set srcfile ${testfile}.c
 set srcfile ${testfile}.c
 set binfile ${objdir}/${subdir}/${testfile}
 
-if {[gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}" "${binfile}" \
+if {[gdb_compile_pthreads \
+         "${srcdir}/${subdir}/${srcfile} ${srcdir}/${subdir}/gdb-barrier.c" \
+         "${binfile}" \
 	 executable { debug }] != "" } {
     return -1
 }