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 111452)
+++ base/threading/thread_local_storage_win.cc	(working copy)
@@ -29,8 +29,8 @@
 // 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];
+subtle::AtomicWord
+    ThreadLocalStorage::tls_destructors_[kThreadLocalStorageSize];
 
 void** ThreadLocalStorage::Initialize() {
   if (tls_key_ == TLS_OUT_OF_INDEXES) {
@@ -49,9 +49,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 lazilly cause such a

[willchan no longer on Chromium, 2011/11/29 01:46:49]

s/lazilly/lazily/g

[jar (doing other things), 2011/11/29 02:32:45]

Done.

+  // 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 use a stack allocated vector, so that we don't have dependence

[willchan no longer on Chromium, 2011/11/29 01:46:49]

s/Use use/Use/g

[jar (doing other things), 2011/11/29 02:32:45]

Done.

+  // 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));

[willchan no longer on Chromium, 2011/11/29 01:46:49]

s/sizeof/arraysize/

[jar (doing other things), 2011/11/29 02:32:45]

I think I want the size of the array.  Array size would only give me a count
(ratio of array size to element size), and not give me number of bytes.  I could
equally well have done:
kThreadLocalStorageSize * sizeof (stack_allocated_tls_data[0])

...but the above is clearer (subtle, in that one of the few times that an array
is not equivalent to a pointer is when used as an arg to sizeof).

+  // Ensure that any rentrant calls change the temp version.

[willchan no longer on Chromium, 2011/11/29 01:46:49]

re-entrant

[jar (doing other things), 2011/11/29 02:32:45]

Done.

+  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));

[willchan no longer on Chromium, 2011/11/29 01:46:49]

s/sizeof/arraysize/

[jar (doing other things), 2011/11/29 02:32:45]

Again... I think I have the right form.

[willchan no longer on Chromium, 2011/11/29 02:47:04]

Oops, you're right. Ignore me.

   TlsSetValue(tls_key_, tls_data);
   return tls_data;
 }
@@ -68,13 +81,16 @@
 
   // 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;
+  // Setup our destructor.  Make sure that any thread that is dostroyed can see

[rvargas (doing something else), 2011/11/29 01:26:31]

nit: typo

[jar (doing other things), 2011/11/29 02:32:45]

Done.

+  // this new setting.
+  subtle::NoBarrier_Store(&tls_destructors_[slot_],

[rvargas (doing something else), 2011/11/29 01:26:31]

Isn't it more readable to do a regular assignment? (assuming we don't want
barriers)

[jar (doing other things), 2011/11/29 02:32:45]

Yeah... but I think it is a bit wrong.  Perhaps if windows was only x86 it would
be ok... but I'm not sure if I should count on that.

[rvargas (doing something else), 2011/11/29 02:46:46]

The way I'm reading this code (I may be wrong) is that we don't need a memory
barrier here, regardless of the platform where this code is compiled. So I see
this basically as a documentation line, telling the future reader of the code
that the implications were already considered.

If that's the case, I'd prefer a simple assignment and a comment saying that a
barrier is not needed, that way the comment is clear, and the code is also
clear.

Your call :)

+                          reinterpret_cast<subtle::AtomicWord>(destructor));
   initialized_ = true;
   return true;
 }
@@ -82,7 +98,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);
+  subtle::NoBarrier_Store(&tls_destructors_[slot_], NULL);
+  slot_ = 0;
   initialized_ = false;
 }
 
@@ -90,7 +109,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 +118,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 +128,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 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 rentrant calls change the temp version.
+  TlsSetValue(tls_key_, stack_allocated_tls_data);
+  delete[] tls_data;  // Our last dependence on an allocator.
+
+  int remaining_attempts = PTHREAD_DESTRUCTOR_ITERATIONS;
+  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.
+    for (int slot = tls_max_ - 1; slot > 0; --slot) {
+      void* value = stack_allocated_tls_data[slot];
+      if (value == NULL)
+        continue;
+      TLSDestructorFunc destructor =
+          reinterpret_cast<ThreadLocalStorage::TLSDestructorFunc>(
+              subtle::NoBarrier_Load(&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);
 }