Make ThreadLocalStorage more posix pthread compliant

base/threading/thread_local_storage_win.cc

Index: base/threading/thread_local_storage_win.cc
===================================================================
--- base/threading/thread_local_storage_win.cc	(revision 111815)
+++ base/threading/thread_local_storage_win.cc	(working copy)
@@ -8,8 +8,31 @@
 
 #include "base/logging.h"
 
+
 namespace base {
 
+namespace {
+// The maximum number of 'slots' in our thread local storage stack.
+const int kThreadLocalStorageSize = 64;
+
+// The maximum number of times to try to clear slots by calling destructors.
+// Use pthread naming convention for clarity.
+const int kMaxDestructorIterations = kThreadLocalStorageSize;
+
+// An array of destructor function pointers for the slots.  If a slot has a
+// destructor, it will be stored in its corresponding entry in this array.
+// The elements are volatile to ensure that when the compiler reads the value
+// to potentially call the destructor, it does so once, and that value is tested
+// for null-ness and then used. Yes, that would be a weird de-optimization,
+// but I can imagine some register machines where it was just as easy to
+// re-fetch an array element, and I want to be sure a call to free the key
+// (i.e., null out the destructor entry) that happens on a separate thread can't
+// hurt the racy calls to the destructors on another thread.
+volatile ThreadLocalStorage::TLSDestructorFunc

[willchan no longer on Chromium, 2011/11/29 18:52:14]

I'm always super wary of use of volatile. Are you sure it's doing what you think
it's doing? Please refer to
http://en.wikipedia.org/wiki/Volatile_variable#In_C_and_C.2B.2B.

[jar (doing other things), 2011/11/29 19:53:22]

I'm beyond super wary, and almost tend to think of it as useless.  This is one
of the rare cases where it seems applicable, and that point seems to be agreed
to by the article you reference.  Search for the text:

"However, foo might represent a location that can be changed by other elements
of the computer system at any time,... [use volatile]"

That is indeed the situation we are put in with an asynchronous change via the
tls-free call on another thread.  We wish to preclude any compiler
optimizations, including reading it form memory more than once (per use).

They also summarize it as:

"prevent the (pseudo)compiler from applying any optimizations on the code that
assume values of variables cannot change "on their own."  "

These variables can change, and hence should not be read more than once, when
the value of the first read is tested and re-used.

On 2011/11/29 18:52:14, willchan wrote:

[rvargas (doing something else), 2011/11/29 19:55:22]

Given that this is Windows specific code (and assuming that we're not going to
move away from VS), this version is actually more strict than the previous one
(the one that was using the atomic type but nobarrier calls). Atomic implies
memory barriers and no optimizations:

http://msdn.microsoft.com/en-us/library/12a04hfd(VS.80).aspx

That said, we probably want to revisit the whole issue soon, because chatting
with Jim I'm now wary that at least the TLS API is not right.

+    g_tls_destructors[kThreadLocalStorageSize];
+
+}  // namespace anonymous
+
 // In order to make TLS destructors work, we need to keep function
 // pointers to the destructor for each TLS that we allocate.
 // We make this work by allocating a single OS-level TLS, which
@@ -26,12 +49,6 @@
 // unallocated TLS slot.
 long ThreadLocalStorage::tls_max_ = 1;
 
-// An array of destructor function pointers for the slots.  If
-// a slot has a destructor, it will be stored in its corresponding
-// entry in this array.
-ThreadLocalStorage::TLSDestructorFunc
-  ThreadLocalStorage::tls_destructors_[kThreadLocalStorageSize];
-
 void** ThreadLocalStorage::Initialize() {
   if (tls_key_ == TLS_OUT_OF_INDEXES) {
     long value = TlsAlloc();
@@ -49,9 +66,22 @@
   }
   DCHECK(!TlsGetValue(tls_key_));
 
-  // Create an array to store our data.
+  // Some allocators, such as TCMalloc, make use of thread local storage.
+  // As a result, any attempt to call new (or malloc) will lazily cause such a
+  // system to initialize, which will include registering for a TLS key.  If we
+  // are not careful here, then that request to create a key will call new back,
+  // and we'll have an infinite loop.  We avoid that as follows:
+  // Use a stack allocated vector, so that we don't have dependence on our
+  // allocator until our service is in place.  (i.e., don't even call new until
+  // after we're setup)
+  void* stack_allocated_tls_data[kThreadLocalStorageSize];
+  memset(stack_allocated_tls_data, 0, sizeof(stack_allocated_tls_data));
+  // Ensure that any rentrant calls change the temp version.
+  TlsSetValue(tls_key_, stack_allocated_tls_data);
+
+  // Allocate an array to store our data.
   void** tls_data = new void*[kThreadLocalStorageSize];
-  memset(tls_data, 0, sizeof(void*[kThreadLocalStorageSize]));
+  memcpy(tls_data, stack_allocated_tls_data, sizeof(stack_allocated_tls_data));
   TlsSetValue(tls_key_, tls_data);
   return tls_data;
 }
@@ -68,13 +98,14 @@
 
   // Grab a new slot.
   slot_ = InterlockedIncrement(&tls_max_) - 1;
+  DCHECK_GT(slot_, 0);
   if (slot_ >= kThreadLocalStorageSize) {
     NOTREACHED();
     return false;
   }
 
   // Setup our destructor.
-  tls_destructors_[slot_] = destructor;
+  g_tls_destructors[slot_] = destructor;
   initialized_ = true;
   return true;
 }
@@ -82,7 +113,10 @@
 void ThreadLocalStorage::Slot::Free() {
   // At this time, we don't reclaim old indices for TLS slots.
   // So all we need to do is wipe the destructor.
-  tls_destructors_[slot_] = NULL;
+  DCHECK_GT(slot_, 0);
+  DCHECK_LT(slot_, kThreadLocalStorageSize);
+  g_tls_destructors[slot_] =  NULL;

[willchan no longer on Chromium, 2011/11/29 18:52:14]

extra horizontal whitespace?

[jar (doing other things), 2011/11/29 19:53:22]

Done.

+  slot_ = 0;
   initialized_ = false;
 }
 
@@ -90,7 +124,7 @@
   void** tls_data = static_cast<void**>(TlsGetValue(tls_key_));
   if (!tls_data)
     tls_data = ThreadLocalStorage::Initialize();
-  DCHECK_GE(slot_, 0);
+  DCHECK_GT(slot_, 0);
   DCHECK_LT(slot_, kThreadLocalStorageSize);
   return tls_data[slot_];
 }
@@ -99,7 +133,7 @@
   void** tls_data = static_cast<void**>(TlsGetValue(tls_key_));
   if (!tls_data)
     tls_data = ThreadLocalStorage::Initialize();
-  DCHECK_GE(slot_, 0);
+  DCHECK_GT(slot_, 0);
   DCHECK_LT(slot_, kThreadLocalStorageSize);
   tls_data[slot_] = value;
 }
@@ -109,21 +143,56 @@
     return;
 
   void** tls_data = static_cast<void**>(TlsGetValue(tls_key_));
-
   // Maybe we have never initialized TLS for this thread.
   if (!tls_data)
     return;
 
-  for (int slot = 0; slot < tls_max_; slot++) {
-    if (tls_destructors_[slot] != NULL) {
-      void* value = tls_data[slot];
-      tls_destructors_[slot](value);
+  // Some allocators, such as TCMalloc, use TLS.  As a result, when a thread
+  // terminates, one of the destructor calls we make may be to shut down an
+  // allocator.  We have to be careful that after we've shutdown all of the
+  // known destructors (perchance including an allocator), that we don't call
+  // the allocator and cause it to resurrect itself (with no possibly destructor
+  // call to follow).  We handle this problem as follows:
+  // Switch to using a stack allocated vector, so that we don't have dependence
+  // on our allocator after we have called all g_tls_destructors.  (i.e., don't
+  // even call delete[] after we're done with destructors.)
+  void* stack_allocated_tls_data[kThreadLocalStorageSize];
+  memcpy(stack_allocated_tls_data, tls_data, sizeof(stack_allocated_tls_data));
+  // Ensure that any re-entrant calls change the temp version.
+  TlsSetValue(tls_key_, stack_allocated_tls_data);
+  delete[] tls_data;  // Our last dependence on an allocator.
+
+  int remaining_attempts = kMaxDestructorIterations;
+  bool need_to_scan_destructors = true;
+  while (need_to_scan_destructors) {
+    need_to_scan_destructors = false;
+    // Try to destroy the first-created-slot (which is slot 1) in our last
+    // destructor call.  That user was able to function, and define a slot with
+    // no other services running, so perhaps it is a basic service (like an
+    // allocator) and should also be destroyed last.  If we get the order wrong,
+    // then we'll itterate several more times, so it is really not that
+    // critical (but it might help).
+    for (int slot = tls_max_ - 1; slot > 0; --slot) {
+      void* value = stack_allocated_tls_data[slot];
+      if (value == NULL)
+        continue;
+      TLSDestructorFunc destructor = g_tls_destructors[slot];
+      if (destructor == NULL)
+        continue;
+      stack_allocated_tls_data[slot] = NULL;  // pre-clear the slot.
+      destructor(value);
+      // Any destructor might have called a different service, which then set
+      // a different slot to a non-NULL value.  Hence we need to check
+      // the whole vector again.  This is a pthread standard.
+      need_to_scan_destructors = true;
     }
+    if (--remaining_attempts <= 0) {
+      NOTREACHED();  // Destructors might not have been called.
+      break;
+    }
   }
 
-  delete[] tls_data;
-
-  // In case there are other "onexit" handlers...
+  // Remove our stack allocated vector.
   TlsSetValue(tls_key_, NULL);
 }