From 4312aeb47447039574e9e905cdcb2bae6388e101 Mon Sep 17 00:00:00 2001 From: =?utf8?q?Jos=C3=A9=20Rivero?= Date: Wed, 3 Jul 2019 20:05:22 -0700 Subject: [PATCH] Fix getting rundown events during EventPipe::Shutdown (#25450) --- src/vm/eventpipe.cpp | 57 +++++++++++++++++++++++++--------------------------- src/vm/eventpipe.h | 11 ++++++++-- 2 files changed, 36 insertions(+), 32 deletions(-) diff --git a/src/vm/eventpipe.cpp b/src/vm/eventpipe.cpp index 09463d2..acb4d39 100644 --- a/src/vm/eventpipe.cpp +++ b/src/vm/eventpipe.cpp @@ -29,7 +29,7 @@ #ifdef FEATURE_PERFTRACING CrstStatic EventPipe::s_configCrst; -Volatile EventPipe::s_tracingInitialized(false); +Volatile EventPipe::s_state(EventPipeState::NotInitialized); EventPipeConfiguration EventPipe::s_config; EventPipeEventSource *EventPipe::s_pEventSource = nullptr; VolatilePtr EventPipe::s_pSessions[MaxNumberOfSessions]; @@ -49,7 +49,7 @@ void EventPipe::Initialize() { STANDARD_VM_CONTRACT; - if (s_tracingInitialized) + if (s_state != EventPipeState::NotInitialized) { _ASSERTE(!"EventPipe::Initialize was already initialized."); return; @@ -97,7 +97,8 @@ void EventPipe::Initialize() { CrstHolder _crst(GetLock()); - s_tracingInitialized = tracingInitialized; + if (tracingInitialized) + s_state = EventPipeState::Initialized; } } @@ -108,7 +109,7 @@ void EventPipe::Shutdown() NOTHROW; GC_TRIGGERS; MODE_ANY; - PRECONDITION(s_tracingInitialized); + PRECONDITION(s_state != EventPipeState::ShuttingDown); } CONTRACTL_END; @@ -126,37 +127,33 @@ void EventPipe::Shutdown() return; } + if (s_state != EventPipeState::Initialized) + return; + // We are shutting down, so if disabling EventPipe throws, we need to move along anyway. EX_TRY { - bool tracingInitialized = false; { CrstHolder _crst(GetLock()); - tracingInitialized = s_tracingInitialized; - - // Mark tracing as no longer initialized. - s_tracingInitialized = false; + s_state = EventPipeState::ShuttingDown; } - if (tracingInitialized) + for (uint32_t i = 0; i < MaxNumberOfSessions; ++i) { - for (uint32_t i = 0; i < MaxNumberOfSessions; ++i) - { - EventPipeSession *pSession = s_pSessions[i].Load(); - if (pSession) - Disable(reinterpret_cast(pSession)); - } + EventPipeSession *pSession = s_pSessions[i].Load(); + if (pSession) + Disable(reinterpret_cast(pSession)); + } - // dotnet/coreclr: issue 24850: EventPipe shutdown race conditions - // Deallocating providers/events here might cause AV if a WriteEvent - // was to occur. Thus, we are not doing this cleanup. + // dotnet/coreclr: issue 24850: EventPipe shutdown race conditions + // Deallocating providers/events here might cause AV if a WriteEvent + // was to occur. Thus, we are not doing this cleanup. - // // Remove EventPipeEventSource first since it tries to use the data structures that we remove below. - // // We need to do this after disabling sessions since those try to write to EventPipeEventSource. - // delete s_pEventSource; - // s_pEventSource = nullptr; - // s_config.Shutdown(); - } + // // Remove EventPipeEventSource first since it tries to use the data structures that we remove below. + // // We need to do this after disabling sessions since those try to write to EventPipeEventSource. + // delete s_pEventSource; + // s_pEventSource = nullptr; + // s_config.Shutdown(); } EX_CATCH {} EX_END_CATCH(SwallowAllExceptions); @@ -191,7 +188,7 @@ EventPipeSessionID EventPipe::Enable( EventPipeSessionID sessionId = 0; RunWithCallbackPostponed([&](EventPipeProviderCallbackDataQueue *pEventPipeProviderCallbackDataQueue) { - if (!s_tracingInitialized) + if (s_state != EventPipeState::Initialized) return; const uint32_t SessionIndex = GenerateSessionIndex(); @@ -228,7 +225,7 @@ bool EventPipe::EnableInternal( THROWS; GC_TRIGGERS; MODE_PREEMPTIVE; - PRECONDITION(s_tracingInitialized); + PRECONDITION(s_state == EventPipeState::Initialized); PRECONDITION(pSession != nullptr && pSession->IsValid()); PRECONDITION(IsLockOwnedByCurrentThread()); } @@ -402,7 +399,7 @@ EventPipeSession *EventPipe::GetSession(EventPipeSessionID id) { CrstHolder _crst(GetLock()); - if (!s_tracingInitialized) + if (s_state == EventPipeState::NotInitialized) { _ASSERTE(!"EventPipe::GetSession invoked before EventPipe was initialized."); return nullptr; @@ -532,7 +529,7 @@ void EventPipe::WriteEventInternal(EventPipeEvent &event, EventPipeEventPayload CONTRACTL_END; // We can't proceed if tracing is not initialized. - if (!s_tracingInitialized) + if (s_state == EventPipeState::NotInitialized) return; // Exit early if the event is not enabled. @@ -573,7 +570,7 @@ void EventPipe::WriteEventInternal( CONTRACTL_END; // We can't proceed if tracing is not initialized. - if (!s_tracingInitialized) + if (s_state == EventPipeState::NotInitialized) return; EventPipeThread *const pEventPipeThread = EventPipeThread::GetOrCreate(); diff --git a/src/vm/eventpipe.h b/src/vm/eventpipe.h index 610dc39..23b1acf 100644 --- a/src/vm/eventpipe.h +++ b/src/vm/eventpipe.h @@ -27,6 +27,13 @@ struct EventData; typedef uint64_t EventPipeSessionID; +enum class EventPipeState : uint32_t +{ + NotInitialized, + Initialized, + ShuttingDown, +}; + class EventPipe { // Declare friends. @@ -64,7 +71,7 @@ public: static bool Enabled() { LIMITED_METHOD_CONTRACT; - return s_tracingInitialized && (s_numberOfSessions.LoadWithoutBarrier() > 0); + return (s_state.LoadWithoutBarrier() >= EventPipeState::Initialized) && (s_numberOfSessions.LoadWithoutBarrier() > 0); } // Create a provider. @@ -196,7 +203,7 @@ private: } static CrstStatic s_configCrst; - static Volatile s_tracingInitialized; + static Volatile s_state; static EventPipeConfiguration s_config; static VolatilePtr s_pSessions[MaxNumberOfSessions]; static Volatile s_allowWrite; -- 2.7.4