Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix incorrect nativeaot event thread / sequence number on shutdown #88941

Merged
merged 3 commits into from
Jul 20, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -8,5 +8,7 @@ void DiagnosticServerAdapter_PauseForDiagnosticsMonitor() {}

void EventPipeAdapter_FinishInitialize() {}

void EventPipe_ThreadShutdown() { }

void EventPipeAdapter_Shutdown() {}
bool DiagnosticServerAdapter_Shutdown() { return false; }
9 changes: 2 additions & 7 deletions src/coreclr/nativeaot/Runtime/EnabledEventPipeInterface.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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(); }
2 changes: 2 additions & 0 deletions src/coreclr/nativeaot/Runtime/EventPipeInterface.h
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ void DiagnosticServerAdapter_PauseForDiagnosticsMonitor();

void EventPipeAdapter_FinishInitialize();

void EventPipe_ThreadShutdown();

void EventPipeAdapter_Shutdown();
bool DiagnosticServerAdapter_Shutdown();

Expand Down
90 changes: 57 additions & 33 deletions src/coreclr/nativeaot/Runtime/eventpipe/ep-rt-aot.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -17,16 +17,7 @@
#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
Expand All @@ -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;
Expand Down Expand Up @@ -181,6 +164,63 @@ 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;
}
}

EventPipeThreadHolder* get_thread_holder ()
{
return eventpipe_tls_instance;
}

EventPipeThreadHolder* create_thread_holder ()
{
STATIC_CONTRACT_NOTHROW;

// Free any existing holder
free_thread_holder ();

eventpipe_tls_instance = thread_holder_alloc_func ();
return eventpipe_tls_instance;
}
}

EventPipeThread* ep_rt_aot_thread_get (void)
{
EventPipeThreadHolder *thread_holder = get_thread_holder ();
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;

EventPipeThreadHolder *thread_holder = create_thread_holder ();

return ep_thread_holder_get_thread (thread_holder);
}

void
ep_rt_aot_thread_exited (void)
{
free_thread_holder ();
}

uint32_t
ep_rt_aot_atomic_inc_uint32_t (volatile uint32_t *value)
{
Expand Down Expand Up @@ -679,29 +719,13 @@ 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;
extern CrstStatic _ep_rt_aot_config_lock;

_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)
Expand Down
105 changes: 5 additions & 100 deletions src/coreclr/nativeaot/Runtime/eventpipe/ep-rt-aot.h
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand All @@ -1594,57 +1550,15 @@ 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 *
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
Expand All @@ -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
Expand Down
15 changes: 0 additions & 15 deletions src/coreclr/nativeaot/Runtime/eventpipeadaptertypes.h

This file was deleted.

4 changes: 4 additions & 0 deletions src/coreclr/nativeaot/Runtime/startup.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -540,6 +540,10 @@ void RuntimeThreadShutdown(void* thread)
#endif

ThreadStore::DetachCurrentThread();

#ifdef FEATURE_PERFTRACING
EventPipe_ThreadShutdown();
#endif
}

extern "C" bool RhInitialize()
Expand Down