Mon Oct 12 00:00:44 1998 Ross Johnson <rpj@ixobrychus.canberra.edu.au>

        * implement.h (_pthread_tsd_key_table): New.
        * create.c (_pthread_start_call): Initialise per-thread TSD keys
        to NULL.
        * misc.c (pthread_once): Correct typo in comment.
        * implement.h (_pthread_destructor_push): Remove.
        (_pthread_destructor_pop): Remove.
        (_pthread_destructor_run_all): Rename from _pthread_destructor_pop_all.
        (_PTHREAD_TSD_KEY_DELETED): Add enum.
        (_PTHREAD_TSD_KEY_INUSE): Add enum.
        * cleanup.c (_pthread_destructor_push): Remove.
        (_pthread_destructor_pop): Remove.
        (_pthread_destructor_run_all): Totally revamped TSD.
        * dll.c (_pthread_TSD_keys_TlsIndex): Initialise.
        * tsd.c (pthread_setspecific): Totally revamped TSD.
        (pthread_getspecific): Ditto.
        (pthread_create): Ditto.
        (pthread_delete): Ditto.
Sun Oct 11 22:44:55 1998  Ross Johnson  <rpj@ixobrychus.canberra.edu.au>
        * global.c (_pthread_tsd_key_table): Add new global.
        * implement.h (_pthread_tsd_key_t and struct _pthread_tsd_key):
        Add.
        (struct _pthread): Remove destructorstack.
        * cleanup.c (_pthread_destructor_run_all): Rename from
        _pthread_destructor_pop_all. The key destructor stack was made
        global rather than per-thread. No longer removes destructor nodes
        from the stack. Comments updated.

8 files changed, 155 insertions, 120 deletions

diff --git a/ChangeLog b/ChangeLog
index 51b81cf..3da9a14 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,42 @@
+Mon Oct 12 00:00:44 1998  Ross Johnson  <rpj@ixobrychus.canberra.edu.au>
+
+	* implement.h (_pthread_tsd_key_table): New.
+
+	* create.c (_pthread_start_call): Initialise per-thread TSD keys
+	to NULL.
+
+	* misc.c (pthread_once): Correct typo in comment.
+
+	* implement.h (_pthread_destructor_push): Remove.
+	(_pthread_destructor_pop): Remove.
+	(_pthread_destructor_run_all): Rename from _pthread_destructor_pop_all.
+	(_PTHREAD_TSD_KEY_DELETED): Add enum.
+	(_PTHREAD_TSD_KEY_INUSE): Add enum.
+
+	* cleanup.c (_pthread_destructor_push): Remove.
+	(_pthread_destructor_pop): Remove.
+	(_pthread_destructor_run_all): Totally revamped TSD.
+
+	* dll.c (_pthread_TSD_keys_TlsIndex): Initialise.
+
+	* tsd.c (pthread_setspecific): Totally revamped TSD.
+	(pthread_getspecific): Ditto.
+	(pthread_create): Ditto.
+	(pthread_delete): Ditto.
+
+Sun Oct 11 22:44:55 1998  Ross Johnson  <rpj@ixobrychus.canberra.edu.au>
+
+	* global.c (_pthread_tsd_key_table): Add new global.
+
+	* implement.h (_pthread_tsd_key_t and struct _pthread_tsd_key):
+	Add.
+	(struct _pthread): Remove destructorstack.
+
+	* cleanup.c (_pthread_destructor_run_all): Rename from
+ 	_pthread_destructor_pop_all. The key destructor stack was made
+ 	global rather than per-thread. No longer removes destructor nodes
+	from the stack. Comments updated.
+
 1998-10-06  Ben Elliston  <bje@cygnus.com>
 
 	* condvar.c (cond_wait): Use POSIX, not Win32 mutex calls.
diff --git a/cleanup.c b/cleanup.c
index 7a7ea4e..c956ce7 100644
--- a/cleanup.c
+++ b/cleanup.c
@@ -120,112 +120,36 @@ _pthread_handler_pop_all(int stack, int execute)
     }
 }
 
-
-int
-_pthread_destructor_push(void (* routine)(void *), pthread_key_t key)
-{
-  return _pthread_handler_push(_PTHREAD_DESTRUCTOR_STACK, 
-			       _PTHREAD_HANDLER_POP_LIFO,
-			       routine, 
-			       (void *) key);
-}
-
-
-/* Remove all of the destructors associated with the key. */
-void
-_pthread_destructor_pop(pthread_key_t key)
-{
-  _pthread_handler_node_t ** head;
-  _pthread_handler_node_t * current;
-  _pthread_handler_node_t * next;
-
-  head = _PTHREAD_STACK(_PTHREAD_DESTRUCTOR_STACK);
-  current = *head;
-
-  while (current != NULL)
-    {
-      next = current->next;
-
-      /* The destructors associated key is in current->arg. */
-      if (current->arg == (void *) key)
-	{
-	  if (current == *head)
-	    {
-	      *head = next;
-	    }
-	  free(current);
-	}
-      current = next;
-    }
-}
-
-
-/* Run destructors for all non-NULL key values.
-
-   FIXME: Currently we only run the destructors on the calling
-   thread's key values. The way I interpret POSIX semantics is that,
-   for each key that the calling thread has a destructor for, we need
-   to look at the key values of every thread and run the destructor on
-   it if the key value is non-NULL.
-
-   The question is: how do we access the key associated values which
-   are private to other threads?
-
+/* Run destructors for all non-NULL key values for the calling thread.
  */
 void
-_pthread_destructor_pop_all()
+_pthread_destructor_run_all()
 {
-  _pthread_handler_node_t ** head;
-  _pthread_handler_node_t * current;
-  _pthread_handler_node_t * next;
-  void (* func)(void *);
+  _pthread_tsd_key_t * k;
   void * arg;
   int count;
 
-  head = _PTHREAD_STACK(_PTHREAD_DESTRUCTOR_STACK);
+  k = _pthread_tsd_key_table;
 
   /* Stop destructor execution at a finite time. POSIX allows us
      to ignore this if we like, even at the risk of an infinite loop.
    */
   for (count = 0; count < PTHREAD_DESTRUCTOR_ITERATIONS; count++)
     {
-      /* Loop through all destructors for this thread. */
-      while (current != NULL)
+      /* Loop through all keys. */
+      for (key = 0; key < _POSIX_THREAD_KEYS_MAX; key++)
 	{
-	  func = current->routine;
-
-	  /* Get the key value using the key which is in current->arg. */
-	  arg = pthread_getspecific((int) current->arg);
+	  if (k->in_use != 1)
+	    continue;
 
-	  next = current->next;
+	  arg = pthread_getspecific(key);
 
-	  /* If the key value is non-NULL run the destructor, otherwise
-	     unlink it from the list.
-	   */
-	  if (arg != NULL)
-	    {
-	      if (func != NULL)
-		{
-		  (void) func(arg);
-		}
-	    }
-	  else
+	  if (arg != NULL && k->destructor != NULL)
 	    {
-	      if (current == *head)
-		{
-		  *head = next;
-		}
-	      free(current);
+	      (void) (k->destructor)(arg);
 	    }
-	  current = next;
-	}
-    }
 
-  /* Free the destructor list even if we still have non-NULL key values. */
-  while (*head != NULL)
-    {
-      next = (*head)->next;
-      free(*head);
-      *head = next;
+	  k++;
+	}
     }
 }
diff --git a/create.c b/create.c
index d313a8a..f477585 100644
--- a/create.c
+++ b/create.c
@@ -22,13 +22,17 @@ STDCALL _pthread_start_call(void * us_arg)
      this thread's private stack so we're safe to leave data in them
      until we leave. */
   pthread_t us;
+  void * keys[PTHREAD_KEYS_MAX];
   unsigned (*func)(void *);
   void * arg;
   unsigned ret;
 
   us = (pthread_t) us_arg;
 
+  memset(keys, 0, sizeof(keys));
+
   (void) TlsSetValue(_pthread_threadID_TlsIndex, (LPVOID) us);
+  (void) TlsSetValue(_pthread_TSD_keys_TlsIndex, (LPVOID) keys);
 
   /* FIXME: For now, if priority setting fails then at least ensure
      that our records reflect true reality. */
diff --git a/dll.c b/dll.c
index 5900ed9..83edcce 100644
--- a/dll.c
+++ b/dll.c
@@ -22,6 +22,10 @@
 /* Global index for TLS data. */
 DWORD _pthread_threadID_TlsIndex;
 
+/* Global index for thread TSD key array. */
+DWORD _pthread_TSD_keys_TlsIndex;
+
+
 BOOL WINAPI PthreadsEntryPoint(HINSTANCE dllHandle,
 			  DWORD reason,
 			  LPVOID situation)
@@ -42,9 +46,18 @@ BOOL WINAPI PthreadsEntryPoint(HINSTANCE dllHandle,
 	{
 	  return FALSE;
 	}
+
+      /* Set up per thread TSD key array pointer. */
+      _pthread_TSD_keys_TlsIndex = TlsAlloc();
+
+      if (_pthread_TSD_keys_TlsIndex == 0xFFFFFFFF)
+	{
+	  return FALSE;
+	}
       break;
 
     case DLL_PROCESS_DETACH:
+      (void) TlsFree(_pthread_TSD_keys_TlsIndex);
       (void) TlsFree(_pthread_threadID_TlsIndex);
       break;
 
diff --git a/global.c b/global.c
index 6d55581..9ce662f 100644
--- a/global.c
+++ b/global.c
@@ -57,3 +57,8 @@ pthread_t _pthread_win32handle_map[_PTHREAD_MAX_THREADS];
 /* Per thread mutex locks. */
 pthread_mutex_t _pthread_threads_mutex_table[_PTHREAD_MAX_THREADS];
 
+/* Global TSD key array. */
+_pthread_tsd_key_t _pthread_tsd_key_table[_POSIX_THREAD_KEYS_MAX];
+
+/* Index to the next available TSD key. */
+int _pthread_tsd_key_next = 0;
diff --git a/implement.h b/implement.h
index bf55e91..a87edb2 100644
--- a/implement.h
+++ b/implement.h
@@ -19,6 +19,11 @@ enum {
   _PTHREAD_REUSE
 };
 
+enum {
+  _PTHREAD_TSD_KEY_DELETED,
+  _PTHREAD_TSD_KEY_INUSE
+};
+
 #define _PTHREAD_VALID(T) \
   ((T) != NULL \
    && ((T)->ptstatus == _PTHREAD_NEW \
@@ -40,6 +45,14 @@ struct _pthread_handler_node {
   void * arg;
 };
 
+/* TSD key element. */
+typedef struct _pthread_tsd_key _pthread_tsd_key_t;
+
+struct _pthread_tsd_key {
+  int in_use;
+  void (* destructor)(void *);
+};
+
 /* Stores a thread call routine and argument. */
 typedef struct {
   unsigned (*routine)(void *);
@@ -84,7 +97,6 @@ struct _pthread {
 
   /* These must be kept in this order and together. */
   _pthread_handler_node_t *   cleanupstack;
-  _pthread_handler_node_t *   destructorstack;
   _pthread_handler_node_t *   forkpreparestack;
   _pthread_handler_node_t *   forkparentstack;
   _pthread_handler_node_t *   forkchildstack;
@@ -107,12 +119,7 @@ void _pthread_handler_pop(int stack,
 void _pthread_handler_pop_all(int stack,
 			      int execute);
 
-int _pthread_destructor_push(void (*routine)(void *),
-			     pthread_key_t key);
-
-void _pthread_destructor_pop(pthread_key_t key);
-
-void _pthread_destructor_pop_all();
+void _pthread_destructor_run_all();
 
 /* Primitives to manage threads table entries. */
 
@@ -161,6 +168,9 @@ extern pthread_t _pthread_win32handle_map[];
 /* Per thread mutex locks. */
 extern pthread_mutex_t _pthread_threads_mutex_table[];
 
+/* Global TSD key array. */
+extern _pthread_tsd_key_t _pthread_tsd_key_table[];
+
 #endif /* _IMPLEMENT_H */
 
 
diff --git a/misc.c b/misc.c
index d39af50..659a4eb 100644
--- a/misc.c
+++ b/misc.c
@@ -13,7 +13,7 @@ int
 pthread_once(pthread_once_t *once_control,
 	     void (*init_routine)(void))
 {
-  /* A flag, allocated per invocation, that indicates if the amotic
+  /* A flag, allocated per invocation, that indicates if the atomic
      test-and-set occured. */
   unsigned short flag = 0;
 
diff --git a/tsd.c b/tsd.c
index 015af85..8e53b37 100644
--- a/tsd.c
+++ b/tsd.c
@@ -12,14 +12,40 @@
  * In a word: Destructors
  *
  * POSIX 1003.1 1996, Section 17 allows for optional destructor functions
- * to be associated with each key value. The destructors are called from
- * the creating thread, which means that the calling thread must have access
- * to the TSD keys of all active threads.
+ * to be associated with each key value.
  *
- * If we use Win32 TLS then this is not possible since Tls*Value()
- * functions don't allow us to access other than our own [thread's] key.
+ * This is my (revised) understanding of how destructors work:
  *
- * As a result, these routines need to be redesigned.
+ * A key is created by a single thread, which then provides in every
+ * existing thread a TSD matching the same key, but initialised
+ * to NULL. Each new thread will also get a matching key with value NULL.
+ * The creating thread can optionally associate a function, called a
+ * destructor, with the key.
+ *
+ * When each thread exits, it calls the destructor function, which
+ * will then perform an action on that threads key value
+ * only. (Previously I thought that only the key creating thread ran
+ * the destructor on the key in all threads. That proposition is
+ * sounding scarier by the minute.)
+ *
+ * SOME APPROACHES TO MANAGING TSD MEMORY
+ *
+ * We could simply allocate enough memory on process startup to hold
+ * all possible data for all possible threads.
+ *
+ * We could allocate memory for just a table to hold a single pointer
+ * for each of POSIX_THREAD_KEYS_MAX keys. pthread_key_create() could then
+ * allocate space for POSIX_THREADS_MAX key values in one hit and store
+ * the location of the array in the first table.
+ * 
+ * The standard also suggests that each thread might store key/value pairs
+ * on its private stack. This seems like a good idea. I had concerns about
+ * memory leaks and key re-use if a key was deleted, but the standard talks
+ * at length on this and basically says it's up to the application to
+ * make sure everything goes smoothly here, making sure that proper cleanup
+ * is done before a key is deleted. (section B.17.1.3 in particular)
+ *
+ * One more thing to note: destructors must never be called on deleted keys.
  */
 
 #include <errno.h>
@@ -30,21 +56,17 @@
 int
 pthread_key_create(pthread_key_t *key, void (*destructor)(void *))
 {
-  DWORD index;
+  pthread_key_t k;
+
+  if (_pthread_tsd_key_next >= PTHREAD_KEYS_MAX)
+    return EAGAIN;
 
-  index = TlsAlloc();
-  if (index == 0xFFFFFFFF)
-    {
-      return EAGAIN;
-    }
+  k = _pthread_tsd_key_next++;
 
-  /* Only modify the `key' parameter if allocation was successful. */
-  *key = index;
+  _pthread_tsd_key_table[k].in_use = _PTHREAD_TSD_KEY_INUSE;
+  _pthread_tsd_key_table[k].destructor = destructor;
 
-  if (destructor != NULL)
-    {
-      return (_pthread_destructor_push(destructor, *key));
-    }
+  *key = k;
 
   return 0;
 }
@@ -52,20 +74,38 @@ pthread_key_create(pthread_key_t *key, void (*destructor)(void *))
 int
 pthread_setspecific(pthread_key_t key, void *value)
 {
-  return (TlsSetValue(key, value) == FALSE) ? EINVAL : 0;
+  void ** keys;
+
+  if (_pthread_tsd_key_table[key].in_use != _PTHREAD_TSD_KEY_INUSE)
+    return EINVAL;
+
+  keys = (void **) TlsGetValue(_pthread_TSD_keys_TlsIndex);
+  keys[key] = value;
+
+  return 0;
 }
 
 void *
 pthread_getspecific(pthread_key_t key)
 {
-  return TlsGetValue(key);
+  void ** keys;
+
+  if (_pthread_tsd_key_table[key].in_use != _PTHREAD_TSD_KEY_INUSE)
+    return EINVAL;
+
+  keys = (void **) TlsGetValue(_pthread_TSD_keys_TlsIndex);
+  return keys[key];
 }
 
 int
 pthread_key_delete(pthread_key_t key)
 {
-  /* Remove this key's destructors. */
-  _pthread_destructor_pop(key);
+  if (_pthread_tsd_key_table[key].in_use != _PTHREAD_TSD_KEY_INUSE)
+    return EINVAL;
 
-  return (TlsFree(key) == FALSE) ? EINVAL : 0;
+  _pthread_tsd_key_table[key].in_use = _PTHREAD_TSD_KEY_DELETED;
+  _pthread_tsd_key_table[key].destructor = NULL;
+
+  return 0;
 }
+