Applied pthread_mutex patch

winsup/cygwin/ChangeLog

@@ -1,3 +1,38 @@
+2003-01-09  Thomas Pfaff  <tpfaff@gmx.net>
+
+	* include/pthread.h: Add define for errorchecking mutexes.
+	Change default mutex type.
+	* thread.cc (pthread_cond::TimedWait): Update mutex unlock
+	calls.
+	(pthread_mutex::pthread_mutex): New implement.
+	(pthread_mutex::~pthread_mutex): Ditto.
+	(pthread_mutex::Lock): Ditto.
+	(pthread_mutex::TryLock): Ditto.
+	(pthread_mutex::UnLock): Ditto.
+	(pthread_mutex::Destroy): Implement new method.
+	(pthread_mutex::SetOwner): Ditto.
+	(pthread_mutex::LockRecursive): Ditto.
+	(pthread_mutex::fixup_after_fork): Restore locking state after
+	fork.
+	(__pthread_mutex_lock): Return pthread_mutex::Lock errorcode.
+	(__pthread_mutex_trylock): Return pthread_mutex::TryLock
+	errorcode.
+	(__pthread_mutex_unlock): Return pthread_mutex::UnLock
+	errorcode.
+	(__pthread_mutex_destroy): Call pthread_mutex::Destroy to
+	destroy	mutex.
+	(__pthread_mutexattr_settype): Allow errorchecking and recursive
+	types.
+	* thread.h (MUTEX_LOCK_COUNTER_INITIAL): New define.
+	(pthread_mutex::criticalsection): Remove.
+	(pthread_mutex::lock_counter): New member.
+	(pthread_mutex::recursion_counter): Ditto.
+	(pthread_mutex::owner): Ditto.
+	(pthread_mutex::type): Ditto.
+	(pthread_mutex::Destroy): New method.
+	(pthread_mutex::SetOwner): Ditto.
+	(pthread_mutex::LockRecursive): Ditto.
+
 2003-01-09  Thomas Pfaff  <tpfaff@gmx.net>
 
 	* pthread.cc (pthread_cond_init): Use new pthread_cond::init.

winsup/cygwin/include/pthread.h

@@ -50,12 +50,11 @@ extern "C"
 #define PTHREAD_CREATE_JOINABLE 0
 #define PTHREAD_EXPLICIT_SCHED 1
 #define PTHREAD_INHERIT_SCHED 0
-#define PTHREAD_MUTEX_DEFAULT 0
+#define PTHREAD_MUTEX_ERRORCHECK 0
-#define PTHREAD_MUTEX_ERRORCHECK 1
+#define PTHREAD_MUTEX_RECURSIVE 1
-#define PTHREAD_MUTEX_NORMAL 2
+#define PTHREAD_MUTEX_DEFAULT PTHREAD_MUTEX_ERRORCHECK
 /* this should be too low to ever be a valid address */
 #define PTHREAD_MUTEX_INITIALIZER (void *)20
-#define PTHREAD_MUTEX_RECURSIVE 0
 #define PTHREAD_ONCE_INIT { PTHREAD_MUTEX_INITIALIZER, 0 }
 #define PTHREAD_PRIO_INHERIT
 #define PTHREAD_PRIO_NONE

winsup/cygwin/thread.cc

@@ -913,27 +913,20 @@ int
 pthread_cond::TimedWait (DWORD dwMilliseconds)
 {
   DWORD rv;
-  if (!wincap.has_signal_object_and_wait ())
-    {
-      // FIXME: race condition (potentially drop events
-      // Possible solution (single process only) - place this in a critical section.
-      ReleaseMutex (mutex->win32_obj_id);
-      rv = WaitForSingleObject (win32_obj_id, dwMilliseconds);
-    }
-  else
-    {
-      LeaveCriticalSection (&mutex->criticalsection);
-      rv = WaitForSingleObject (win32_obj_id, dwMilliseconds);
-#if 0
-    /* we need to use native win32 mutex's here, because the cygwin ones now use
-     * critical sections, which are faster, but introduce a race _here_. Until then
-     * The NT variant of the code is redundant.
-     */
 
-    rv = SignalObjectAndWait (mutex->win32_obj_id, win32_obj_id, dwMilliseconds,
+  // FIXME: race condition (potentially drop events
-			 false);
+  // Possible solution (single process only) - place this in a critical section.
+  mutex->UnLock ();
+  rv = WaitForSingleObject (win32_obj_id, dwMilliseconds);
+#if 0
+  /* we need to use native win32 mutex's here, because the cygwin ones now use
+   * critical sections, which are faster, but introduce a race _here_. Until then
+   * The NT variant of the code is redundant.
+   */
+
+  rv = SignalObjectAndWait (mutex->win32_obj_id, win32_obj_id, dwMilliseconds,
+                            false);
 #endif
-    }
   switch (rv)
     {
     case WAIT_FAILED:
@@ -1154,39 +1147,41 @@ pthread_mutex::initMutex ()
     api_fatal ("Could not create win32 Mutex for pthread mutex static initializer support.");
 }
 
-pthread_mutex::pthread_mutex (pthread_mutexattr *attr):verifyable_object (PTHREAD_MUTEX_MAGIC)
+pthread_mutex::pthread_mutex (pthread_mutexattr *attr) :
+  verifyable_object (PTHREAD_MUTEX_MAGIC),
+  lock_counter (MUTEX_LOCK_COUNTER_INITIAL),
+  win32_obj_id (NULL), recursion_counter (0),
+  condwaits (0), owner (NULL), type (PTHREAD_MUTEX_DEFAULT),
+  pshared(PTHREAD_PROCESS_PRIVATE)
 {
-  /* attr checked in the C call */
+  win32_obj_id = ::CreateSemaphore (&sec_none_nih, 0, LONG_MAX, NULL);
-  if (attr && attr->pshared == PTHREAD_PROCESS_SHARED)
+  if (!win32_obj_id)
     {
-      // fail
       magic = 0;
       return;
     }
-  if (wincap.has_try_enter_critical_section ())
+  /*attr checked in the C call */
-    InitializeCriticalSection (&criticalsection);
+  if (attr)
-  else
     {
-      this->win32_obj_id = ::CreateMutex (&sec_none_nih, false, NULL);
+      if (attr->pshared == PTHREAD_PROCESS_SHARED)
-      if (!win32_obj_id)
+        {
-	magic = 0;
+          // fail
+          magic = 0;
+          return;
+        }
+      
+      type = attr->mutextype;
     }
-  condwaits = 0;
+
-  pshared = PTHREAD_PROCESS_PRIVATE;
   /* threadsafe addition is easy */
   next = (pthread_mutex *) InterlockedExchangePointer (&MT_INTERFACE->mutexs, this);
 }
 
 pthread_mutex::~pthread_mutex ()
 {
-  if (wincap.has_try_enter_critical_section ())
+  if (win32_obj_id)
-    DeleteCriticalSection (&criticalsection);
+    CloseHandle (win32_obj_id);
-  else
+
-    {
-      if (win32_obj_id)
-	CloseHandle (win32_obj_id);
-      win32_obj_id = NULL;
-    }
   /* I'm not 100% sure the next bit is threadsafe. I think it is... */
   if (MT_INTERFACE->mutexs == this)
     /* TODO: printf an error if the return value != this */
@@ -1195,7 +1190,7 @@ pthread_mutex::~pthread_mutex ()
     {
       pthread_mutex *tempmutex = MT_INTERFACE->mutexs;
       while (tempmutex->next && tempmutex->next != this)
-	tempmutex = tempmutex->next;
+        tempmutex = tempmutex->next;
       /* but there may be a race between the loop above and this statement */
       /* TODO: printf an error if the return value != this */
       InterlockedExchangePointer (&tempmutex->next, this->next);
@@ -1205,33 +1200,96 @@ pthread_mutex::~pthread_mutex ()
 int
 pthread_mutex::Lock ()
 {
-  if (wincap.has_try_enter_critical_section ())
+  int result = 0;
+  pthread_t self = pthread::self ();
+ 
+  if (0 == InterlockedIncrement (&lock_counter))
+    SetOwner ();
+  else if (__pthread_equal (&owner, &self))
     {
-      EnterCriticalSection (&criticalsection);
+      InterlockedDecrement (&lock_counter);
-      return 0;
+      if (PTHREAD_MUTEX_RECURSIVE == type)
+        result = LockRecursive ();
+      else
+        result = EDEADLK;
     }
-  /* FIXME: Return 0 on success */
+  else
-  return WaitForSingleObject (win32_obj_id, INFINITE);
+    {
+      WaitForSingleObject (win32_obj_id, INFINITE);
+      SetOwner ();
+    }
+ 
+  return result;
 }
 
 /* returns non-zero on failure */
 int
 pthread_mutex::TryLock ()
 {
-  if (wincap.has_try_enter_critical_section ())
+  int result = 0;
-    return (!TryEnterCriticalSection (&criticalsection));
+  pthread_t self = pthread::self ();
-  return (WaitForSingleObject (win32_obj_id, 0) == WAIT_TIMEOUT);
+ 
+  if (MUTEX_LOCK_COUNTER_INITIAL ==
+      InterlockedCompareExchange (&lock_counter, 0, MUTEX_LOCK_COUNTER_INITIAL ))
+    SetOwner ();
+  else if (__pthread_equal (&owner, &self) && PTHREAD_MUTEX_RECURSIVE == type)
+    result = LockRecursive ();
+  else
+    result = EBUSY;
+ 
+  return result;
 }
 
 int
 pthread_mutex::UnLock ()
 {
-  if (wincap.has_try_enter_critical_section ())
+  pthread_t self = pthread::self ();
+ 
+  if (!__pthread_equal (&owner, &self))
+    return EPERM;
+ 
+  if (0 == --recursion_counter)
     {
-      LeaveCriticalSection (&criticalsection);
+      owner = NULL;
-      return 0;
+      if (MUTEX_LOCK_COUNTER_INITIAL != InterlockedDecrement (&lock_counter))
+        // Another thread is waiting
+        ::ReleaseSemaphore (win32_obj_id, 1, NULL);
     }
-  return (!ReleaseMutex (win32_obj_id));
+ 
+  return 0;
+}
+
+int
+pthread_mutex::Destroy ()
+{
+  if (condwaits || TryLock ())
+    // Do not destroy a condwaited or locked mutex
+    return EBUSY;
+  else if (recursion_counter != 1)
+    {
+      // Do not destroy a recursive locked mutex
+      --recursion_counter;
+      return EBUSY;
+    }
+ 
+  delete this;
+  return 0;
+}
+ 
+void
+pthread_mutex::SetOwner ()
+{
+  recursion_counter = 1;
+  owner = pthread::self ();
+}
+
+int
+pthread_mutex::LockRecursive ()
+{
+  if (UINT_MAX == recursion_counter)
+    return EAGAIN;
+  ++recursion_counter;
+  return 0;
 }
 
 void
@@ -1240,15 +1298,18 @@ pthread_mutex::fixup_after_fork ()
   debug_printf ("mutex %x in fixup_after_fork", this);
   if (pshared != PTHREAD_PROCESS_PRIVATE)
     api_fatal ("pthread_mutex::fixup_after_fork () doesn'tunderstand PROCESS_SHARED mutex's");
-  /* FIXME: duplicate code here and in the constructor. */
+
-  if (wincap.has_try_enter_critical_section ())
+  if (NULL == owner)
-    InitializeCriticalSection (&criticalsection);
+    /* mutex has no owner, reset to initial */
-  else
+    lock_counter = MUTEX_LOCK_COUNTER_INITIAL;
-    {
+  else if (lock_counter != MUTEX_LOCK_COUNTER_INITIAL)
-      win32_obj_id = ::CreateMutex (&sec_none_nih, false, NULL);
+    /* All waiting threads are gone after a fork */
-      if (!win32_obj_id)
+    lock_counter = 0;
-	api_fatal ("pthread_mutex::fixup_after_fork () failed to create new win32 mutex");
+
-    }
+  win32_obj_id = ::CreateSemaphore (&sec_none_nih, 0, LONG_MAX, NULL);
+  if (!win32_obj_id)
+    api_fatal ("pthread_mutex::fixup_after_fork () failed to recreate win32 semaphore for mutex");
+
 #if DETECT_BAD_APPS
   if (condwaits)
     api_fatal ("Forked () while a mutex has condition variables waiting on it.\nReport to cygwin@cygwin.com");
@@ -2346,8 +2407,7 @@ __pthread_mutex_lock (pthread_mutex_t *mutex)
     case VALID_OBJECT:
       break;
     }
-  (*themutex)->Lock ();
+  return (*themutex)->Lock ();
-  return 0;
 }
 
 int
@@ -2358,9 +2418,7 @@ __pthread_mutex_trylock (pthread_mutex_t *mutex)
     pthread_mutex::init (mutex, NULL);
   if (!pthread_mutex::isGoodObject (themutex))
     return EINVAL;
-  if ((*themutex)->TryLock ())
+  return (*themutex)->TryLock ();
-    return EBUSY;
-  return 0;
 }
 
 int
@@ -2370,23 +2428,23 @@ __pthread_mutex_unlock (pthread_mutex_t *mutex)
     pthread_mutex::init (mutex, NULL);
   if (!pthread_mutex::isGoodObject (mutex))
     return EINVAL;
-  (*mutex)->UnLock ();
+  return (*mutex)->UnLock ();
-  return 0;
 }
 
 int
 __pthread_mutex_destroy (pthread_mutex_t *mutex)
 {
+  int rv;
+
   if (pthread_mutex::isGoodInitializer (mutex))
     return 0;
   if (!pthread_mutex::isGoodObject (mutex))
     return EINVAL;
 
-  /* reading a word is atomic */
+  rv = (*mutex)->Destroy ();
-  if ((*mutex)->condwaits)
+  if (rv)
-    return EBUSY;
+    return rv;
 
-  delete (*mutex);
   *mutex = NULL;
   return 0;
 }
@@ -2424,10 +2482,6 @@ __pthread_mutexattr_getpshared (const pthread_mutexattr_t *attr,
   return 0;
 }
 
-/* Win32 mutex's are equivalent to posix RECURSIVE mutexs.
-   We need to put glue in place to support other types of mutex's. We map
-   PTHREAD_MUTEX_DEFAULT to PTHREAD_MUTEX_RECURSIVE and return EINVAL for
-   other types.  */
 int
 __pthread_mutexattr_gettype (const pthread_mutexattr_t *attr, int *type)
 {
@@ -2437,10 +2491,7 @@ __pthread_mutexattr_gettype (const pthread_mutexattr_t *attr, int *type)
   return 0;
 }
 
-/* Currently pthread_mutex_init ignores the attr variable, this is because
+/* FIXME: write and test process shared mutex's.  */
-   none of the variables have any impact on it's behaviour.
-
-   FIXME: write and test process shared mutex's.  */
 int
 __pthread_mutexattr_init (pthread_mutexattr_t *attr)
 {
@@ -2516,9 +2567,17 @@ __pthread_mutexattr_settype (pthread_mutexattr_t *attr, int type)
 {
   if (!pthread_mutexattr::isGoodObject (attr))
     return EINVAL;
-  if (type != PTHREAD_MUTEX_RECURSIVE)
+ 
-    return EINVAL;
+  switch (type)
-  (*attr)->mutextype = type;
+    {
+    case PTHREAD_MUTEX_ERRORCHECK:
+    case PTHREAD_MUTEX_RECURSIVE:
+      (*attr)->mutextype = type;
+      break;
+    default:
+      return EINVAL;
+    }
+
   return 0;
 }
 

winsup/cygwin/thread.h

@@ -162,6 +162,8 @@ private:
 #define SEM_MAGIC PTHREAD_MAGIC+7
 #define PTHREAD_ONCE_MAGIC PTHREAD_MAGIC+8;
 
+#define MUTEX_LOCK_COUNTER_INITIAL   (-1)
+
 /* verifyable_object should not be defined here - it's a general purpose class */
 
 class verifyable_object
@@ -305,15 +307,21 @@ public:
   static void initMutex ();
   static int init (pthread_mutex_t *, const pthread_mutexattr_t *);
 
-  CRITICAL_SECTION criticalsection;
+  LONG lock_counter;
   HANDLE win32_obj_id;
+  unsigned int recursion_counter;
   LONG condwaits;
+  pthread_t owner;
+  int type;
   int pshared;
   class pthread_mutex * next;
 
   int Lock ();
   int TryLock ();
   int UnLock ();
+  int Destroy ();
+  void SetOwner ();
+  int LockRecursive ();
   void fixup_after_fork ();
 
   pthread_mutex (pthread_mutexattr * = NULL);