Re: gdb.threads/sigthread.c

["Jim Blandy" <jimb@red-bean.com>]

[-- 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
 }