Fix incorrect nativeaot event thread / sequence number on shutdown (#88941)
authorElinor Fung <elfung@microsoft.com>
Thu, 20 Jul 2023 13:44:01 +0000 (06:44 -0700)
committerGitHub <noreply@github.com>
Thu, 20 Jul 2023 13:44:01 +0000 (06:44 -0700)
src/coreclr/nativeaot/Runtime/DisabledEventPipeInterface.cpp
src/coreclr/nativeaot/Runtime/EnabledEventPipeInterface.cpp
src/coreclr/nativeaot/Runtime/EventPipeInterface.h
src/coreclr/nativeaot/Runtime/eventpipe/ep-rt-aot.cpp
src/coreclr/nativeaot/Runtime/eventpipe/ep-rt-aot.h
src/coreclr/nativeaot/Runtime/eventpipeadaptertypes.h [deleted file]
src/coreclr/nativeaot/Runtime/startup.cpp

index cb654d8..1195c7a 100644 (file)
@@ -8,5 +8,7 @@ void DiagnosticServerAdapter_PauseForDiagnosticsMonitor() {}
 
 void EventPipeAdapter_FinishInitialize() {}
 
+void EventPipe_ThreadShutdown() { }
+
 void EventPipeAdapter_Shutdown() {}
 bool DiagnosticServerAdapter_Shutdown() { return false; }
index a94cdd0..0a68a15 100644 (file)
@@ -4,19 +4,14 @@
 #include "eventpipeadapter.h"
 #include "diagnosticserveradapter.h"
 
-#include "gcenv.h"
-#include "regdisplay.h"
-#include "StackFrameIterator.h"
-#include "thread.h"
-#include "SpinLock.h"
-
 void EventPipeAdapter_Initialize() { EventPipeAdapter::Initialize(); }
 
 bool DiagnosticServerAdapter_Initialize() { return DiagnosticServerAdapter::Initialize(); }
 void DiagnosticServerAdapter_PauseForDiagnosticsMonitor() { DiagnosticServerAdapter::PauseForDiagnosticsMonitor();}
 
-
 void EventPipeAdapter_FinishInitialize() { EventPipeAdapter::FinishInitialize(); }
 
+void EventPipe_ThreadShutdown() { ep_rt_aot_thread_exited(); }
+
 void EventPipeAdapter_Shutdown() { EventPipeAdapter::Shutdown(); }
 bool DiagnosticServerAdapter_Shutdown() { return DiagnosticServerAdapter::Shutdown(); }
index 6e39f53..c8f2d31 100644 (file)
@@ -14,6 +14,8 @@ void DiagnosticServerAdapter_PauseForDiagnosticsMonitor();
 
 void EventPipeAdapter_FinishInitialize();
 
+void EventPipe_ThreadShutdown();
+
 void EventPipeAdapter_Shutdown();
 bool DiagnosticServerAdapter_Shutdown();
 
index 944d771..b1d3520 100644 (file)
 #include <unistd.h>
 #endif
 
-// The regdisplay.h, StackFrameIterator.h, and thread.h includes are present only to access the Thread
-// class and can be removed if it turns out that the required ep_rt_thread_handle_t can be
-// implemented in some manner that doesn't rely on the Thread class.
-
 #include "gcenv.h"
-#include "regdisplay.h"
-#include "StackFrameIterator.h"
-#include "thread.h"
-#include "holder.h"
-#include "SpinLock.h"
 
 #ifndef DIRECTORY_SEPARATOR_CHAR
 #ifdef TARGET_UNIX
 #endif // TARGET_UNIX
 #endif
 
-#ifdef TARGET_UNIX
-// Per module (1 for NativeAOT), key that will be used to implement TLS in Unix
-pthread_key_t eventpipe_tls_key;
-__thread EventPipeThreadHolder* eventpipe_tls_instance;
-#else
-thread_local EventPipeAotThreadHolderTLS EventPipeAotThreadHolderTLS::g_threadHolderTLS;
-#endif
-
 // Uses _rt_aot_lock_internal_t that has CrstStatic as a field
 // This is initialized at the beginning and EventPipe library requires the lock handle to be maintained by the runtime
 ep_rt_lock_handle_t _ep_rt_aot_config_lock_handle;
@@ -181,6 +164,46 @@ ep_rt_aot_diagnostics_command_line_get (void)
 #endif
 }
 
+namespace
+{
+    #ifdef TARGET_UNIX
+    __thread EventPipeThreadHolder* eventpipe_tls_instance;
+    #else
+    thread_local EventPipeThreadHolder* eventpipe_tls_instance;
+    #endif
+
+    void free_thread_holder ()
+    {
+        EventPipeThreadHolder *thread_holder = eventpipe_tls_instance;
+        if (thread_holder != NULL) {
+            thread_holder_free_func (thread_holder);
+            eventpipe_tls_instance = NULL;
+        }
+    }
+}
+
+EventPipeThread* ep_rt_aot_thread_get (void)
+{
+    EventPipeThreadHolder *thread_holder = eventpipe_tls_instance;
+    return thread_holder ? ep_thread_holder_get_thread (thread_holder) : NULL;
+}
+
+EventPipeThread* ep_rt_aot_thread_get_or_create (void)
+{
+    EventPipeThread *thread = ep_rt_thread_get ();
+    if (thread != NULL)
+        return thread;
+
+    eventpipe_tls_instance = thread_holder_alloc_func ();
+    return ep_thread_holder_get_thread (eventpipe_tls_instance);
+}
+
+void
+ep_rt_aot_thread_exited (void)
+{
+    free_thread_holder ();
+}
+
 uint32_t
 ep_rt_aot_atomic_inc_uint32_t (volatile uint32_t *value)
 {
@@ -679,17 +702,6 @@ ep_rt_aot_volatile_store_ptr_without_barrier (
     VolatileStoreWithoutBarrier<void *> ((void **)ptr, value);
 }
 
-void unix_tls_callback_fn(void *value) 
-{
-    if (value) {
-        // we need to do the unallocation here
-        EventPipeThreadHolder *thread_holder_old = static_cast<EventPipeThreadHolder*>(value);    
-        // @TODO - inline
-        thread_holder_free_func (thread_holder_old);
-        value = NULL;
-    }
-}
-
 void ep_rt_aot_init (void)
 {
     extern ep_rt_lock_handle_t _ep_rt_aot_config_lock_handle;
@@ -697,11 +709,6 @@ void ep_rt_aot_init (void)
 
     _ep_rt_aot_config_lock_handle.lock = &_ep_rt_aot_config_lock;
     _ep_rt_aot_config_lock_handle.lock->InitNoThrow (CrstType::CrstEventPipeConfig);
-
-    // Initialize the pthread key used for TLS in Unix
-    #ifdef TARGET_UNIX
-    pthread_key_create(&eventpipe_tls_key, unix_tls_callback_fn);
-    #endif
 }
 
 bool ep_rt_aot_lock_acquire (ep_rt_lock_handle_t *lock)
index 25c9666..33deac1 100644 (file)
@@ -8,7 +8,6 @@
 #include <ctype.h>  // For isspace
 #ifdef TARGET_UNIX
 #include <sys/time.h>
-#include <pthread.h>
 #endif
 
 #include <eventpipe/ep-rt-config.h>
 #define _TEXT(s) #s
 #define STRINGIFY(s) _TEXT(s)
 
-#ifdef TARGET_UNIX
-extern pthread_key_t eventpipe_tls_key;
-extern __thread EventPipeThreadHolder* eventpipe_tls_instance;
-#endif
+extern void ep_rt_aot_thread_exited (void);
 
 // shipping criteria: no EVENTPIPE-NATIVEAOT-TODO left in the codebase
 // TODO: The NativeAOT ALIGN_UP is defined in a tangled manner that generates linker errors if
@@ -1542,46 +1538,6 @@ thread_holder_free_func (EventPipeThreadHolder * thread_holder)
     }
 }
 
-class EventPipeAotThreadHolderTLS {
-public:
-    EventPipeAotThreadHolderTLS ()
-    {
-        STATIC_CONTRACT_NOTHROW;
-    }
-
-    ~EventPipeAotThreadHolderTLS ()
-    {
-        STATIC_CONTRACT_NOTHROW;
-
-        if (m_threadHolder) {
-            thread_holder_free_func (m_threadHolder);
-            m_threadHolder = NULL;
-        }
-    }
-
-    static inline EventPipeThreadHolder * getThreadHolder ()
-    {
-        STATIC_CONTRACT_NOTHROW;
-        return g_threadHolderTLS.m_threadHolder;
-    }
-
-    static inline EventPipeThreadHolder * createThreadHolder ()
-    {
-        STATIC_CONTRACT_NOTHROW;
-
-        if (g_threadHolderTLS.m_threadHolder) {
-            thread_holder_free_func (g_threadHolderTLS.m_threadHolder);
-            g_threadHolderTLS.m_threadHolder = NULL;
-        }
-        g_threadHolderTLS.m_threadHolder = thread_holder_alloc_func ();
-        return g_threadHolderTLS.m_threadHolder;
-    }
-
-private:
-    EventPipeThreadHolder *m_threadHolder;
-    static thread_local EventPipeAotThreadHolderTLS g_threadHolderTLS;
-};
-
 static
 void
 ep_rt_thread_setup (void)
@@ -1594,44 +1550,6 @@ ep_rt_thread_setup (void)
     // EP_ASSERT (thread_handle != NULL);
 }
 
-#ifdef TARGET_UNIX
-static
-inline
-EventPipeThreadHolder *
-pthread_getThreadHolder (void)
-{
-    void *value = eventpipe_tls_instance;
-    if (value) {
-        EventPipeThreadHolder *thread_holder = static_cast<EventPipeThreadHolder*>(value);    
-        return thread_holder;
-    }
-    return NULL;
-}
-
-static
-inline
-EventPipeThreadHolder *
-pthread_createThreadHolder (void)
-{
-    void *value = eventpipe_tls_instance;
-    if (value) {
-        // we need to do the unallocation here
-        EventPipeThreadHolder *thread_holder_old = static_cast<EventPipeThreadHolder*>(value);
-        thread_holder_free_func(thread_holder_old);
-        eventpipe_tls_instance = NULL;
-
-        value = NULL;
-    }
-    EventPipeThreadHolder *instance = thread_holder_alloc_func();
-    if (instance){
-        // We need to know when the thread is no longer in use to clean up EventPipeThreadHolder instance and will use pthread destructor function to get notification when that happens.
-        pthread_setspecific(eventpipe_tls_key, instance);
-        eventpipe_tls_instance = instance;
-    }
-    return instance;
-}
-#endif
-
 static
 inline
 EventPipeThread *
@@ -1639,12 +1557,8 @@ ep_rt_thread_get (void)
 {
     STATIC_CONTRACT_NOTHROW;
 
-#ifdef TARGET_UNIX
-    EventPipeThreadHolder *thread_holder = pthread_getThreadHolder ();
-#else
-    EventPipeThreadHolder *thread_holder = EventPipeAotThreadHolderTLS::getThreadHolder ();
-#endif    
-    return thread_holder ? ep_thread_holder_get_thread (thread_holder) : NULL;
+    extern EventPipeThread* ep_rt_aot_thread_get (void);
+    return ep_rt_aot_thread_get ();
 }
 
 static
@@ -1654,17 +1568,8 @@ ep_rt_thread_get_or_create (void)
 {
     STATIC_CONTRACT_NOTHROW;
 
-#ifdef TARGET_UNIX
-    EventPipeThreadHolder *thread_holder = pthread_getThreadHolder ();
-    if (!thread_holder)
-        thread_holder = pthread_createThreadHolder ();
-#else
-    EventPipeThreadHolder *thread_holder = EventPipeAotThreadHolderTLS::getThreadHolder ();    
-    if (!thread_holder)
-        thread_holder = EventPipeAotThreadHolderTLS::createThreadHolder ();
-#endif        
-
-    return ep_thread_holder_get_thread (thread_holder);
+    extern EventPipeThread* ep_rt_aot_thread_get_or_create (void);
+    return ep_rt_aot_thread_get_or_create ();
 }
 
 static
diff --git a/src/coreclr/nativeaot/Runtime/eventpipeadaptertypes.h b/src/coreclr/nativeaot/Runtime/eventpipeadaptertypes.h
deleted file mode 100644 (file)
index 9d9df45..0000000
+++ /dev/null
@@ -1,15 +0,0 @@
-// Licensed to the .NET Foundation under one or more agreements.
-// The .NET Foundation licenses this file to you under the MIT license.
-
-#ifndef EVENTPIPE_ADAPTER_TYPES_H
-#define EVENTPIPE_ADAPTER_TYPES_H
-
-#if defined(FEATURE_PERFTRACING)
-
-typedef struct _EventFilterDescriptor EventFilterDescriptor;
-typedef struct _EventPipeBufferList EventPipeBufferList;
-typedef struct _EventPipeProvider EventPipeProvider;
-typedef struct _EventPipeSession EventPipeSession;
-
-#endif // FEATURE_PERFTRACING
-#endif // EVENTPIPE_ADAPTER_TYPES_H
index b7e59f6..272dd8e 100644 (file)
@@ -342,6 +342,10 @@ void RuntimeThreadShutdown(void* thread)
 #endif
 
     ThreadStore::DetachCurrentThread();
+    
+#ifdef FEATURE_PERFTRACING
+    EventPipe_ThreadShutdown();
+#endif
 }
 
 extern "C" bool RhInitialize()