Mon Mar 8 11:18:59 1999 Ross Johnson <rpj@ixobrychus.canberra.edu.au>

        * misc.c (CancelableWait): Ensure cancelEvent handle is the lowest
        indexed element in the handles array. Enhance test for abandoned
        objects.
        * pthread.h (PTHREAD_MUTEX_INITIALIZER): Trailing elements not
        initialised are set to zero by the compiler. This avoids the
        problem of initialising the opaque critical section element in it.
        (PTHREAD_COND_INITIALIZER): Ditto.
        * semaphore.c (_pthread_sem_timedwait): Check sem == NULL earlier.
Sun Mar  7 12:31:14 1999  Ross Johnson  <rpj@ixobrychus.canberra.edu.au>
        * condvar.c (pthread_cond_init): set semaphore initial value
        to 0, not 1. cond_timedwait was returning signaled immediately.
        * misc.c (CancelableWait): Place the cancel event handle first
        in the handle table for WaitForMultipleObjects. This ensures that
        the cancel event is recognised and acted apon if both objects
        happen to be signaled together.
        * private.c (_pthread_cond_test_init_lock): Initialise and destroy.
        * implement.h (_pthread_cond_test_init_lock): Add extern.
        * global.c (_pthread_cond_test_init_lock): Add declaration.
        * condvar.c (pthread_cond_destroy): check for valid initialised CV;
        flag destroyed CVs as invalid.
        (pthread_cond_init): pthread_cond_t is no longer just a pointer.
        This is because PTHREAD_COND_INITIALIZER needs state info to reside
        in pthread_cond_t so that it can initialise on first use. Will work on
        making pthread_cond_t (and other objects like it) opaque again, if
        possible, later.
        (cond_timedwait): add check for statically initialisation of
        CV; initialise on first use.
        (pthread_cond_signal): check for valid CV.
        (pthread_cond_broadcast): check for valid CV.
        (_cond_check_need_init): Add.
        * pthread.h (PTHREAD_COND_INITIALIZER): Fix.
        (pthread_cond_t): no longer a pointer to pthread_cond_t_.
        (pthread_cond_t_): add 'staticinit' and 'valid' elements.
tests/ChangeLog
Sun Mar  7 10:41:52 1999  Ross Johnson  <rpj@ixobrychus.canberra.edu.au>
        * Makefile (condvar3, condvar4): Add tests.
        * condvar4.c (General): Reduce to simple test case; prerequisite
        is condvar3.c; add description.
        * condvar3.c (General): Reduce to simple test case; prerequisite
        is condvar2.c; add description.
        * condvar2.c (General): Reduce to simple test case; prerequisite
        is condvar1.c; add description.
        * condvar1.c (General): Reduce to simple test case; add
        description.
        * Template.c (Comments): Add generic test detail.

1 files changed, 157 insertions, 53 deletions

diff --git a/condvar.c b/condvar.c
index eb549b5..8fca94c 100644
--- a/condvar.c
+++ b/condvar.c
@@ -12,6 +12,67 @@
 #include "pthread.h"
 #include "implement.h"
 
+static int
+_cond_check_need_init(pthread_cond_t *cond)
+{
+  int result = 0;
+
+  /*
+   * The following guarded test is specifically for statically
+   * initialised condition variables (via PTHREAD_COND_INITIALIZER).
+   *
+   * Note that by not providing this synchronisation we risk
+   * introducing race conditions into applications which are
+   * correctly written.
+   *
+   * Approach
+   * --------
+   * We know that static condition variables will not be PROCESS_SHARED
+   * so we can serialise access to internal state using
+   * Win32 Critical Sections rather than Win32 Mutexes.
+   *
+   * We still have a problem in that we would like a per-mutex
+   * lock, but attempting to create one will again lead
+   * to a race condition. We are forced to use a global
+   * lock in this instance.
+   *
+   * If using a single global lock slows applications down too much,
+   * multiple global locks could be created and hashed on some random
+   * value associated with each mutex, the pointer perhaps. At a guess,
+   * a good value for the optimal number of global locks might be
+   * the number of processors + 1.
+   *
+   * We need to maintain the following per-cv state independently:
+   *   - cv staticinit (true iff cv is static and still
+   *                    needs to be initialised)
+   *   - cv valid (false iff cv has been destroyed)
+   *
+   * For example, in this implementation a cv initialised by
+   * PTHREAD_COND_INITIALIZER will be 'valid' but uninitialised until
+   * the thread attempts to use it. It can also be destroyed (made invalid)
+   * before ever being used.
+   */
+  EnterCriticalSection(&_pthread_cond_test_init_lock);
+
+  /*
+   * We got here because staticinit tested true, possibly under race
+   * conditions. Check staticinit again inside the critical section
+   * and only initialise if the cv is valid (not been destroyed).
+   * If a static cv has been destroyed, the application can
+   * re-initialise it only by calling pthread_cond_init()
+   * explicitly.
+   */
+  if (cond->staticinit && cond->valid)
+    {
+      result = pthread_cond_init(cond, NULL);
+    }
+
+  LeaveCriticalSection(&_pthread_cond_test_init_lock);
+
+  return(result);
+}
+
+
 int
 pthread_condattr_init (pthread_condattr_t * attr)
      /*
@@ -281,7 +342,19 @@ pthread_cond_init (pthread_cond_t * cond, const pthread_condattr_t * attr)
       */
 {
   int result = EAGAIN;
-  pthread_cond_t cv = NULL;
+
+  if (cond == NULL)
+    {
+      return EINVAL;
+    }
+
+  /* 
+   * Assuming any race condition here is harmless.
+   */
+  if (cond->valid && !cond->staticinit)
+    {
+      return EBUSY;
+    }
 
   if ((attr != NULL && *attr != NULL) &&
       ((*attr)->pshared == PTHREAD_PROCESS_SHARED))
@@ -295,37 +368,34 @@ pthread_cond_init (pthread_cond_t * cond, const pthread_condattr_t * attr)
       goto FAIL0;
     }
 
-  cv = (pthread_cond_t) calloc (1, sizeof (*cv));
-
-  if (cv == NULL)
-    {
-      result = ENOMEM;
-      goto FAIL0;
-    }
-
-  cv->waiters = 0;
-  cv->wasBroadcast = FALSE;
+  cond->waiters = 0;
+  cond->wasBroadcast = FALSE;
 
-  if (_pthread_sem_init (&(cv->sema), 0, 1) != 0)
+  if (_pthread_sem_init (&(cond->sema), 0, 0) != 0)
     {
       goto FAIL0;
     }
-  if (pthread_mutex_init (&(cv->waitersLock), NULL) != 0)
+  if (pthread_mutex_init (&(cond->waitersLock), NULL) != 0)
     {
       goto FAIL1;
     }
 
-  cv->waitersDone = CreateEvent (
+  cond->waitersDone = CreateEvent (
                                   0,
                                   (int) FALSE,  /* manualReset  */
                                   (int) FALSE,  /* setSignaled  */
                                   NULL);
 
-  if (cv->waitersDone == NULL)
+  if (cond->waitersDone == NULL)
     {
       goto FAIL2;
     }
 
+  cond->staticinit = 0;
+
+  /* Mark as valid. */
+  cond->valid = 1;
+
   result = 0;
 
   goto DONE;
@@ -336,16 +406,13 @@ pthread_cond_init (pthread_cond_t * cond, const pthread_condattr_t * attr)
    * -------------
    */
 FAIL2:
-  (void) pthread_mutex_destroy (&(cv->waitersLock));
+  (void) pthread_mutex_destroy (&(cond->waitersLock));
 
 FAIL1:
-  (void) _pthread_sem_destroy (&(cv->sema));
-  free (cv);
-  cv = NULL;
+  (void) _pthread_sem_destroy (&(cond->sema));
 
 FAIL0:
 DONE:
-  *cond = cv;
   return (result);
 
 }                               /* pthread_cond_init */
@@ -380,21 +447,26 @@ pthread_cond_destroy (pthread_cond_t * cond)
       */
 {
   int result = 0;
-  pthread_cond_t cv;
 
-  if (cond != NULL && *cond != NULL)
+  /*
+   * Assuming any race condition here is harmless.
+   */
+  if (cond == NULL || cond->valid == 0 || cond->staticinit == 1)
     {
-      cv = *cond;
-
-      (void) _pthread_sem_destroy (&(cv->sema));
-      (void) pthread_mutex_destroy (&(cv->waitersLock));
-      (void) CloseHandle (cv->waitersDone);
-
-      free (cv);
+      return EINVAL;
+    }
 
-      *cond = NULL;
+  if (cond->waiters > 0)
+    {
+      return EBUSY;
     }
 
+  (void) _pthread_sem_destroy (&(cond->sema));
+  (void) pthread_mutex_destroy (&(cond->waitersLock));
+  (void) CloseHandle (cond->waitersDone);
+      
+  cond->valid = 0;
+
   return (result);
 }
 
@@ -404,15 +476,24 @@ cond_timedwait (pthread_cond_t * cond,
 		const struct timespec *abstime)
 {
   int result = 0;
-  pthread_cond_t cv;
-  int lastWaiter;
+  int internal_result = 0;
+  int lastWaiter = FALSE;
 
-  cv = *cond;
+  /*
+   * We do a quick check to see if we need to do more work
+   * to initialise a static condition variable. We check 'staticinit'
+   * again inside the guarded section of _cond_check_need_init()
+   * to avoid race conditions.
+   */
+  if (cond->staticinit == 1)
+    {
+      result = _cond_check_need_init(cond);
+    }
 
   /*
-   * OK to increment  cv->waiters because the caller locked 'mutex'
+   * OK to increment  cond->waiters because the caller locked 'mutex'
    */
-  cv->waiters++;
+  cond->waiters++;
 
   /*
    * We keep the lock held just long enough to increment the count of
@@ -433,30 +514,30 @@ cond_timedwait (pthread_cond_t * cond,
        *      hence providing the
        *      mechanism for making pthread_cond_wait a cancellation
        *      point. We use the cleanup mechanism to ensure we
-       *  re-lock the mutex if we are cancelled.
+       *      re-lock the mutex if we are cancelled.
        */
       pthread_cleanup_push (pthread_mutex_lock, mutex);
 
-      result = _pthread_sem_timedwait (&(cv->sema), abstime);
+      result = _pthread_sem_timedwait (&(cond->sema), abstime);
 
       pthread_cleanup_pop (0);
     }
 
-  if ((result = pthread_mutex_lock (&(cv->waitersLock))) == 0)
+  if ((internal_result = pthread_mutex_lock (&(cond->waitersLock))) == 0)
     {
       /*
        * By making the waiter responsible for decrementing
        * its count we don't have to worry about having an internal
        * mutex.
        */
-      cv->waiters--;
+      cond->waiters--;
 
-      lastWaiter = cv->wasBroadcast && (cv->waiters == 0);
+      lastWaiter = cond->wasBroadcast && (cond->waiters == 0);
 
-      result = pthread_mutex_unlock (&(cv->waitersLock));
+      internal_result = pthread_mutex_unlock (&(cond->waitersLock));
     }
 
-  if (result == 0)
+  if (result == 0 && internal_result == 0)
     {
       if (lastWaiter)
         {
@@ -464,7 +545,7 @@ cond_timedwait (pthread_cond_t * cond,
            * If we are the last waiter on this broadcast
            * let the thread doing the broadcast proceed
            */
-          if (!SetEvent (cv->waitersDone))
+          if (!SetEvent (cond->waitersDone))
             {
               result = EINVAL;
             }
@@ -646,15 +727,26 @@ pthread_cond_signal (pthread_cond_t * cond)
       */
 {
   int result = 0;
-  pthread_cond_t cv = *cond;
+
+  if (cond == NULL || cond->valid == 0)
+    {
+      return EINVAL;
+    }
 
   /*
-   * If there aren't any waiters, then this is a no-op.
+   * No-op if the CV is static and hasn't been initialised yet.
    */
-  if (cv->waiters > 0)
+  if (cond->staticinit == 1)
     {
+      return 0;
+    }
 
-      result = _pthread_sem_post (&(cv->sema));
+  /*
+   * If there aren't any waiters, then this is a no-op.
+   */
+  if (cond->waiters > 0)
+    {
+      result = _pthread_sem_post (&(cond->sema));
     }
 
   return (result);
@@ -700,27 +792,39 @@ pthread_cond_broadcast (pthread_cond_t * cond)
       */
 {
   int result = 0;
-  pthread_cond_t cv = *cond;
   int i;
 
-  cv->wasBroadcast = TRUE;
+  if (cond == NULL || cond->valid == 0)
+    {
+      return EINVAL;
+    }
+
+  cond->wasBroadcast = TRUE;
+
+  /*
+   * No-op if the CV is static and hasn't been initialised yet.
+   */
+  if (cond->staticinit == 1)
+    {
+      return 0;
+    }
 
   /*
    * Wake up all waiters
    */
-  for (i = cv->waiters; i > 0 && result == 0; i--)
+  for (i = cond->waiters; i > 0 && result == 0; i--)
     {
 
-      result = _pthread_sem_post (&(cv->sema));
+      result = _pthread_sem_post (&(cond->sema));
     }
 
-  if (cv->waiters > 0 && result == 0)
+  if (cond->waiters > 0 && result == 0)
     {
       /*
        * Wait for all the awakened threads to acquire their part of
        * the counting semaphore
        */
-      if (WaitForSingleObject (cv->waitersDone, INFINITE) !=
+      if (WaitForSingleObject (cond->waitersDone, INFINITE) !=
           WAIT_OBJECT_0)
         {