Fixes to EtwCallback and EtwCallbackCommon: #35793
Conversation
PeterSolMS
commented
May 4, 2020
PeterSolMS
commented
- Only call GCHeapUtilities::RecordEventStateChange for EVENT_CONTROL_CODE_ENABLE_PROVIDER or EVENT_CONTROL_CODE_DISABLE_PROVIDER, not for EVENT_CONTROL_CODE_CAPTURE_STATE.
- Only call GCHeapUtilities::RecordEventStateChange for DotNETRuntime and DotNETRuntimePrivate providers
- Consolidate level and keywords between EventPipe and ETW, so that disabling an event on one side does not disable it for the other side, if the other side is still interested in it.
- Only call GCHeapUtilities::RecordEventStateChange for EVENT_CONTROL_CODE_ENABLE_PROVIDER or EVENT_CONTROL_CODE_DISABLE_PROVIDER, *not* for EVENT_CONTROL_CODE_CAPTURE_STATE. - Only call GCHeapUtilities::RecordEventStateChange for DotNETRuntime and DotNETRuntimePrivate providers - Consolidate level and keywords between EventPipe and ETW, so that disabling an event on one side does not disable it for the other side, if the other side is still interested in it.
src/coreclr/src/vm/eventtrace.cpp
Outdated
| @@ -4272,6 +4269,18 @@ VOID EtwCallbackCommon( | |||
| ctxToUpdate->EventPipeProvider.EnabledKeywordsBitmask = MatchAnyKeyword; | |||
| } | |||
|
|
|||
| if ((ControlCode == EVENT_CONTROL_CODE_ENABLE_PROVIDER || ControlCode == EVENT_CONTROL_CODE_DISABLE_PROVIDER) && | |||
There was a problem hiding this comment.
I think you probably want to also maintain the g_fEEStarted && !g_fEEShutdown conditions here as well.
There was a problem hiding this comment.
Probably not because we really want the settings on the GC side to mimic what's set on the VM side, and the VM side does not check g_fEEStarted and !g_fEEShutdown before setting level and keywords.
Also, I noticed that the first call to EtwCallbackCommon happens when g_fEEStarted is still FALSE, so we'd miss that first call.
There was a problem hiding this comment.
I see. As long as the call is safe to make during startup and shutdown, then that's OK. I know that a bunch of the other calls that are inside the if statement below are unsafe and will throw.
|
Fix for #34423 |
|
Thanks Brian and Maoni! |
ghost
locked as resolved and limited conversation to collaborators
