From d672bccdcf1d9037b1fd1ab87549662dd793ef27 Mon Sep 17 00:00:00 2001 From: Elinor Fung Date: Thu, 20 Jul 2023 06:44:01 -0700 Subject: [PATCH] Fix incorrect nativeaot event thread / sequence number on shutdown (#88941) --- .../Runtime/DisabledEventPipeInterface.cpp | 2 + .../Runtime/EnabledEventPipeInterface.cpp | 9 +- src/coreclr/nativeaot/Runtime/EventPipeInterface.h | 2 + .../nativeaot/Runtime/eventpipe/ep-rt-aot.cpp | 73 +++++++------- .../nativeaot/Runtime/eventpipe/ep-rt-aot.h | 105 +-------------------- .../nativeaot/Runtime/eventpipeadaptertypes.h | 15 --- src/coreclr/nativeaot/Runtime/startup.cpp | 4 + 7 files changed, 55 insertions(+), 155 deletions(-) delete mode 100644 src/coreclr/nativeaot/Runtime/eventpipeadaptertypes.h diff --git a/src/coreclr/nativeaot/Runtime/DisabledEventPipeInterface.cpp b/src/coreclr/nativeaot/Runtime/DisabledEventPipeInterface.cpp index cb654d8..1195c7a 100644 --- a/src/coreclr/nativeaot/Runtime/DisabledEventPipeInterface.cpp +++ b/src/coreclr/nativeaot/Runtime/DisabledEventPipeInterface.cpp @@ -8,5 +8,7 @@ void DiagnosticServerAdapter_PauseForDiagnosticsMonitor() {} void EventPipeAdapter_FinishInitialize() {} +void EventPipe_ThreadShutdown() { } + void EventPipeAdapter_Shutdown() {} bool DiagnosticServerAdapter_Shutdown() { return false; } diff --git a/src/coreclr/nativeaot/Runtime/EnabledEventPipeInterface.cpp b/src/coreclr/nativeaot/Runtime/EnabledEventPipeInterface.cpp index a94cdd0..0a68a15 100644 --- a/src/coreclr/nativeaot/Runtime/EnabledEventPipeInterface.cpp +++ b/src/coreclr/nativeaot/Runtime/EnabledEventPipeInterface.cpp @@ -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(); } diff --git a/src/coreclr/nativeaot/Runtime/EventPipeInterface.h b/src/coreclr/nativeaot/Runtime/EventPipeInterface.h index 6e39f53..c8f2d31 100644 --- a/src/coreclr/nativeaot/Runtime/EventPipeInterface.h +++ b/src/coreclr/nativeaot/Runtime/EventPipeInterface.h @@ -14,6 +14,8 @@ void DiagnosticServerAdapter_PauseForDiagnosticsMonitor(); void EventPipeAdapter_FinishInitialize(); +void EventPipe_ThreadShutdown(); + void EventPipeAdapter_Shutdown(); bool DiagnosticServerAdapter_Shutdown(); diff --git a/src/coreclr/nativeaot/Runtime/eventpipe/ep-rt-aot.cpp b/src/coreclr/nativeaot/Runtime/eventpipe/ep-rt-aot.cpp index 944d771..b1d3520 100644 --- a/src/coreclr/nativeaot/Runtime/eventpipe/ep-rt-aot.cpp +++ b/src/coreclr/nativeaot/Runtime/eventpipe/ep-rt-aot.cpp @@ -17,16 +17,7 @@ #include #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 @@ -36,14 +27,6 @@ #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 **)ptr, value); } -void unix_tls_callback_fn(void *value) -{ - if (value) { - // we need to do the unallocation here - EventPipeThreadHolder *thread_holder_old = static_cast(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) diff --git a/src/coreclr/nativeaot/Runtime/eventpipe/ep-rt-aot.h b/src/coreclr/nativeaot/Runtime/eventpipe/ep-rt-aot.h index 25c9666..33deac1 100644 --- a/src/coreclr/nativeaot/Runtime/eventpipe/ep-rt-aot.h +++ b/src/coreclr/nativeaot/Runtime/eventpipe/ep-rt-aot.h @@ -8,7 +8,6 @@ #include // For isspace #ifdef TARGET_UNIX #include -#include #endif #include @@ -51,10 +50,7 @@ #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(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(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 index 9d9df45..0000000 --- a/src/coreclr/nativeaot/Runtime/eventpipeadaptertypes.h +++ /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 diff --git a/src/coreclr/nativeaot/Runtime/startup.cpp b/src/coreclr/nativeaot/Runtime/startup.cpp index b7e59f6..272dd8e 100644 --- a/src/coreclr/nativeaot/Runtime/startup.cpp +++ b/src/coreclr/nativeaot/Runtime/startup.cpp @@ -342,6 +342,10 @@ void RuntimeThreadShutdown(void* thread) #endif ThreadStore::DetachCurrentThread(); + +#ifdef FEATURE_PERFTRACING + EventPipe_ThreadShutdown(); +#endif } extern "C" bool RhInitialize() -- 2.7.4