Merge pull request #2531 from janvorli/fix-unix-thread-shutdown

There was a problem with the secondary thread shutdown on Unix. First,
when the TlsObjectDestructor is called, it is passed the key that
was set using the pthread_setspecific before. But I was missing the fact
that before the TlsObjectDestructor is called, the key is set to NULL.
So we cannot use pthread_getspecific to get the key anymore and we can
only use the value passed as the TlsObjectDestructor parameter.

The second part of the problem was that the at the time the TlsObjectDestructor
is called, the tls_CurrentThread to which the key points was already removed
from the TLS and so an attempt to access tls_CurrentThread results in creation
of a fresh new tls_CurrentThread. And our checks that we have at a few places
that verify that the argument passed to the TlsObjectDestructor was actually
the current thread pointer cannot be done.

The fix is to use __cxa_thread_atexit to register a destruction callback for the
ThreadStore object and use it instead of the pthread callback functionality.

* Enable workaround for build tools that don't support thread_local

Apple started to support thread_local in xcode starting from version 8.0,
this workaround enables the thread shutdown detection to compile even on
devices that don't have the latest xcode.

5 files changed, 55 insertions, 50 deletions

diff --git a/buildscripts/buildvars-setup.sh b/buildscripts/buildvars-setup.sh
index ead93b646..588bcd626 100755
--- a/buildscripts/buildvars-setup.sh
+++ b/buildscripts/buildvars-setup.sh
@@ -190,10 +190,6 @@ while [ "$1" != "" ]; do
         verbose)
             export __VerboseBuild=1
             ;;
-        clang3.5)
-            export __ClangMajorVersion=3
-            export __ClangMinorVersion=5
-            ;;
         clang3.6)
             export __ClangMajorVersion=3
             export __ClangMinorVersion=6
diff --git a/src/Native/Runtime/startup.cpp b/src/Native/Runtime/startup.cpp
index 8a7b0c56c..3a368191b 100644
--- a/src/Native/Runtime/startup.cpp
+++ b/src/Native/Runtime/startup.cpp
@@ -268,8 +268,13 @@ void __stdcall RuntimeThreadShutdown(void* thread)
     // that is made for the single thread that runs the final stages of orderly process
     // shutdown (i.e., the thread that delivers the DLL_PROCESS_DETACH notifications when the
     // process is being torn down via an ExitProcess call).
+#if HAVE_THREAD_LOCAL
+    // If the current Unix platform doesn't support thread_local, we don't get the thread pointer
+    // as the parameter, we just get NULL, so we can check the thread validity only if the
+    // thread_local is supported
     UNREFERENCED_PARAMETER(thread);
     ASSERT((Thread*)thread == ThreadStore::GetCurrentThread());
+#endif
 
     if (!g_processShutdownHasStarted)
     {
diff --git a/src/Native/Runtime/unix/PalRedhawkUnix.cpp b/src/Native/Runtime/unix/PalRedhawkUnix.cpp
index 91c5842cc..c9f8ea4a8 100644
--- a/src/Native/Runtime/unix/PalRedhawkUnix.cpp
+++ b/src/Native/Runtime/unix/PalRedhawkUnix.cpp
@@ -158,9 +158,6 @@ static uint8_t g_helperPage[OS_PAGE_SIZE] __attribute__((aligned(OS_PAGE_SIZE)))
 // Mutex to make the FlushProcessWriteBuffersMutex thread safe
 pthread_mutex_t g_flushProcessWriteBuffersMutex;
 
-// Key for the thread local storage of the attached thread pointer
-static pthread_key_t g_threadKey;
-
 extern bool PalQueryProcessorTopology();
 bool InitializeFlushProcessWriteBuffers();
 
@@ -413,14 +410,10 @@ public:
 
 typedef UnixHandle<UnixHandleType::Thread, pthread_t> ThreadUnixHandle;
 
-// Destructor of the thread local object represented by the g_threadKey,
-// called when a thread is shut down
-void TlsObjectDestructor(void* data)
-{
-    ASSERT(data == pthread_getspecific(g_threadKey));
-
-    RuntimeThreadShutdown(data);
-}
+#if !HAVE_THREAD_LOCAL
+extern "C" int __cxa_thread_atexit(void (*)(void*), void*, void *);
+extern "C" void *__dso_handle;
+#endif
 
 // The Redhawk PAL must be initialized before any of its exports can be called. Returns true for a successful
 // initialization and false on failure.
@@ -449,11 +442,10 @@ REDHAWK_PALEXPORT bool REDHAWK_PALAPI PalInit()
         return false;
     }
 #endif // !USE_PORTABLE_HELPERS
-    int status = pthread_key_create(&g_threadKey, TlsObjectDestructor);
-    if (status != 0)
-    {
-        return false;
-    }
+
+#if !HAVE_THREAD_LOCAL
+    __cxa_thread_atexit(RuntimeThreadShutdown, NULL, &__dso_handle);
+#endif
 
     return true;
 }
@@ -464,6 +456,32 @@ REDHAWK_PALEXPORT bool REDHAWK_PALAPI PalHasCapability(PalCapability capability)
     return (g_dwPALCapabilities & (uint32_t)capability) == (uint32_t)capability;
 }
 
+#if HAVE_THREAD_LOCAL
+
+struct TlsDestructionMonitor
+{
+    void* m_thread = nullptr;
+
+    void SetThread(void* thread)
+    {
+        m_thread = thread;
+    }
+
+    ~TlsDestructionMonitor()
+    {
+        if (m_thread != nullptr)
+        {
+            RuntimeThreadShutdown(m_thread);
+        }
+    }
+};
+
+// This thread local object is used to detect thread shutdown. Its destructor
+// is called when a thread is being shut down.
+thread_local TlsDestructionMonitor tls_destructionMonitor;
+
+#endif // HAVE_THREAD_LOCAL
+
 // Attach thread to PAL. 
 // It can be called multiple times for the same thread.
 // It fails fast if a different thread was already registered.
@@ -471,16 +489,9 @@ REDHAWK_PALEXPORT bool REDHAWK_PALAPI PalHasCapability(PalCapability capability)
 //  thread        - thread to attach
 extern "C" void PalAttachThread(void* thread)
 {
-    void* attachedThread = pthread_getspecific(g_threadKey);
-
-    ASSERT_MSG(attachedThread == NULL, "PalAttachThread called multiple times for the same thread");
-
-    int status = pthread_setspecific(g_threadKey, thread);
-    if (status != 0)
-    {
-        ASSERT_UNCONDITIONALLY("PalAttachThread failed to store thread pointer in thread local storage");
-        RhFailFast();
-    }
+#if HAVE_THREAD_LOCAL
+    tls_destructionMonitor.SetThread(thread);
+#endif
 }
 
 // Detach thread from PAL.
@@ -491,26 +502,8 @@ extern "C" void PalAttachThread(void* thread)
 //  true if the thread was detached, false if there was no attached thread
 extern "C" bool PalDetachThread(void* thread)
 {
-    void* attachedThread = pthread_getspecific(g_threadKey);
-
-    if (attachedThread == thread)
-    {
-        int status = pthread_setspecific(g_threadKey, NULL);
-        if (status != 0)
-        {
-            ASSERT_UNCONDITIONALLY("PalDetachThread failed to clear thread pointer in thread local storage");
-            RhFailFast();
-        }
-        return true;
-    }
-
-    if (attachedThread != NULL)
-    {
-        ASSERT_UNCONDITIONALLY("PalDetachThread called with different thread pointer than PalAttachThread");
-        RhFailFast();
-    }
-
-    return false;
+    UNREFERENCED_PARAMETER(thread);
+    return true;
 }
 
 REDHAWK_PALEXPORT unsigned int REDHAWK_PALAPI PalGetCurrentProcessorNumber()
diff --git a/src/Native/Runtime/unix/config.h.in b/src/Native/Runtime/unix/config.h.in
index 085ceaa3a..ca5f5aa71 100644
--- a/src/Native/Runtime/unix/config.h.in
+++ b/src/Native/Runtime/unix/config.h.in
@@ -29,4 +29,6 @@
 #cmakedefine01 HAVE_CLOCK_MONOTONIC_COARSE
 #cmakedefine01 HAVE_MACH_ABSOLUTE_TIME
 
+#cmakedefine01 HAVE_THREAD_LOCAL
+
 #endif
diff --git a/src/Native/Runtime/unix/configure.cmake b/src/Native/Runtime/unix/configure.cmake
index d58e1219e..a8fd29d18 100644
--- a/src/Native/Runtime/unix/configure.cmake
+++ b/src/Native/Runtime/unix/configure.cmake
@@ -105,4 +105,13 @@ int main()
   exit(ret);
 }" HAVE_MACH_ABSOLUTE_TIME)
 
+check_cxx_source_compiles("
+thread_local int x;
+
+int main(int argc, char **argv)
+{
+    x = 1;
+    return 0;
+}" HAVE_THREAD_LOCAL)
+
 configure_file(${CMAKE_CURRENT_LIST_DIR}/config.h.in ${CMAKE_CURRENT_BINARY_DIR}/config.h)