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,17 @@
   }
   DCHECK(!TlsGetValue(tls_key_));
 
-  // Create an array to store our data.
+  // Use 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)

[willchan no longer on Chromium, 2011/11/28 20:58:28]

You should explain that this is because an allocator may use thread local
storage.

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

Done.

+  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 +76,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

[willchan no longer on Chromium, 2011/11/28 21:04:32]

s/dostroyed/destroyed/

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

Done.

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

[willchan no longer on Chromium, 2011/11/28 21:04:32]

It's not obvious to me that the NoBarrier variants are what you want to use
here. I would expect to do an Acquire_Load() and Release_Store(). Can you
explain why it's ok to use NoBarrier_Store() and NoBarrier_Load()?

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

As per our discussion, I added a pile of comments justifying this.

On 2011/11/28 21:04:32, willchan wrote:

+                          reinterpret_cast<subtle::AtomicWord>(destructor));
   initialized_ = true;
   return true;
 }
@@ -82,7 +93,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 +104,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 +113,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 +123,50 @@
     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);
+  // 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.)

[willchan no longer on Chromium, 2011/11/28 20:58:28]

You should again explain that this is because the allocator may depend on thread
local data structures that may no longer exist.

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

Done.

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