Issue correct Enable/Disable commands for EventSources with EventPipe (…

…#81867)

src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventPipeEventProvider.cs

@@ -1,6 +1,7 @@
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
+using System.Collections.Generic;
 using System.Runtime.InteropServices;
 
 namespace System.Diagnostics.Tracing
@@ -16,13 +17,56 @@ internal EventPipeEventProvider(EventProvider eventProvider)
             _eventProvider = new WeakReference<EventProvider>(eventProvider);
         }
 
+        protected override unsafe void HandleEnableNotification(
+                                    EventProvider target,
+                                    byte* additionalData,
+                                    byte level,
+                                    long matchAnyKeywords,
+                                    long matchAllKeywords,
+                                    Interop.Advapi32.EVENT_FILTER_DESCRIPTOR* filterData)
+        {
+            ulong id = 0;
+            if (additionalData != null)
+            {
+                id = BitConverter.ToUInt64(new ReadOnlySpan<byte>(additionalData, sizeof(ulong)));
+            }
+
+            // EventPipe issues Interop.Advapi32.EVENT_CONTROL_CODE_ENABLE_PROVIDER if a session
+            // is stopping as long as some other session is still enabled. If the session is stopping
+            // the session ID will be null, if it is a session starting it will be a non-zero value
+            bool bEnabling = id != 0;
+
+            IDictionary<string, string?>? args = null;
+            ControllerCommand command = ControllerCommand.Update;
+
+            if (bEnabling)
+            {
+                byte[]? filterDataBytes = null;
+                if (filterData != null)
+                {
+                    MarshalFilterData(filterData, out command, out filterDataBytes);
+                }
+
+                args = ParseFilterData(filterDataBytes);
+            }
+
+            // Since we are sharing logic across ETW and EventPipe in OnControllerCommand we have to set up data to
+            // mimic ETW to get the right commands sent to EventSources. perEventSourceSessionId has special meaning,
+            // if it is -1 the this command will be translated to a Disable command in EventSource.OnEventCommand. If
+            // it is 0-3 it indicates an ETW session with activities, and SessionMask.MAX (4) means legacy ETW session.
+            // We send SessionMask.MAX just to conform.
+            target.OnControllerCommand(command, args, bEnabling ? (int)SessionMask.MAX : -1);
+        }
+
         [UnmanagedCallersOnly]
         private static unsafe void Callback(byte* sourceId, int isEnabled, byte level,
             long matchAnyKeywords, long matchAllKeywords, Interop.Advapi32.EVENT_FILTER_DESCRIPTOR* filterData, void* callbackContext)
         {
             EventPipeEventProvider _this = (EventPipeEventProvider)GCHandle.FromIntPtr((IntPtr)callbackContext).Target!;
             if (_this._eventProvider.TryGetTarget(out EventProvider? target))
-                target.EnableCallback(isEnabled, level, matchAnyKeywords, matchAllKeywords, filterData);
+            {
+                _this.ProviderCallback(target, sourceId, isEnabled, level, matchAnyKeywords, matchAllKeywords, filterData);
+            }
         }
 
         // Register an event provider.
@@ -94,7 +138,7 @@ internal override int ActivityIdControl(Interop.Advapi32.ActivityControl control
 
         // Define an EventPipeEvent handle.
         internal override unsafe IntPtr DefineEventHandle(uint eventID, string eventName, long keywords, uint eventVersion, uint level,
-            byte *pMetadata, uint metadataLength)
+            byte* pMetadata, uint metadataLength)
         {
             return EventPipeInternal.DefineEvent(_provHandle, eventID, keywords, eventVersion, level, pMetadata, metadataLength);
         }

src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs

@@ -220,16 +220,6 @@ internal sealed class EventSourceAutoGenerateAttribute : Attribute
     // The EnsureDescriptorsInitialized() method might need to access EventSource and its derived type
     // members and the trimmer ensures that these members are preserved.
     [DynamicallyAccessedMembers(ManifestMemberTypes)]
-    [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2113:ReflectionToRequiresUnreferencedCode",
-        Justification = "EnsureDescriptorsInitialized's use of GetType preserves methods on Delegate and MulticastDelegate " +
-                        "because the nested type OverrideEventProvider's base type EventProvider defines a delegate. " +
-                        "This includes Delegate and MulticastDelegate methods which require unreferenced code, but " +
-                        "EnsureDescriptorsInitialized does not access these members and is safe to call.")]
-    [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2115:ReflectionToDynamicallyAccessedMembers",
-        Justification = "EnsureDescriptorsInitialized's use of GetType preserves methods on Delegate and MulticastDelegate " +
-                        "because the nested type OverrideEventProvider's base type EventProvider defines a delegate. " +
-                        "This includes Delegate and MulticastDelegate methods which have dynamically accessed members requirements, but " +
-                        "EnsureDescriptorsInitialized does not access these members and is safe to call.")]
     public partial class EventSource : IDisposable
     {
 
@@ -448,7 +438,7 @@ public static void SendCommand(EventSource eventSource, EventCommand command, ID
                 throw new ArgumentException(SR.EventSource_InvalidCommand, nameof(command));
             }
 
-            eventSource.SendCommand(null, EventProviderType.ETW, 0, 0, command, true, EventLevel.LogAlways, EventKeywords.None, commandArguments);
+            eventSource.SendCommand(null, EventProviderType.ETW, 0, command, true, EventLevel.LogAlways, EventKeywords.None, commandArguments);
         }
 
         // Error APIs.  (We don't throw by default, but you can probe for status)
@@ -745,7 +735,7 @@ private unsafe void DefineEventPipeEvents()
 
                 fixed (byte *pMetadata = metadata)
                 {
-                    IntPtr eventHandle = m_eventPipeProvider.m_eventProvider.DefineEventHandle(
+                    IntPtr eventHandle = m_eventPipeProvider._eventProvider.DefineEventHandle(
                         eventID,
                         eventName,
                         keywords,
@@ -2168,7 +2158,7 @@ static TraceLoggingEventTypes GetTrimSafeTraceLoggingEventTypes() =>
 
                                     fixed (byte* pMetadata = metadata)
                                     {
-                                        m_writeEventStringEventHandle = m_eventPipeProvider.m_eventProvider.DefineEventHandle(0, eventName, keywords, 0, (uint)level,
+                                        m_writeEventStringEventHandle = m_eventPipeProvider._eventProvider.DefineEventHandle(0, eventName, keywords, 0, (uint)level,
                                                                             pMetadata, metadataLength);
                                     }
                                 }
@@ -2367,12 +2357,12 @@ public OverrideEventProvider(EventSource eventSource, EventProviderType provider
                 this.m_eventSource = eventSource;
                 this.m_eventProviderType = providerType;
             }
-            protected override void OnControllerCommand(ControllerCommand command, IDictionary<string, string?>? arguments,
-                                                              int perEventSourceSessionId, int etwSessionId)
+            internal override void OnControllerCommand(ControllerCommand command, IDictionary<string, string?>? arguments,
+                                                              int perEventSourceSessionId)
             {
                 // We use null to represent the ETW EventListener.
                 EventListener? listener = null;
-                m_eventSource.SendCommand(listener, m_eventProviderType, perEventSourceSessionId, etwSessionId,
+                m_eventSource.SendCommand(listener, m_eventProviderType, perEventSourceSessionId,
                                           (EventCommand)command, IsEnabled(), Level, MatchAnyKeyword, arguments);
             }
             private readonly EventSource m_eventSource;
@@ -2490,7 +2480,7 @@ static Type[] GetParameterTypes(ParameterInfo[] parameters)
         //     * The 'enabled' 'level', matchAnyKeyword' arguments are ignored (must be true, 0, 0).
         //
         // dispatcher == null has special meaning. It is the 'ETW' dispatcher.
-        internal void SendCommand(EventListener? listener, EventProviderType eventProviderType, int perEventSourceSessionId, int etwSessionId,
+        internal void SendCommand(EventListener? listener, EventProviderType eventProviderType, int perEventSourceSessionId,
                                   EventCommand command, bool enable,
                                   EventLevel level, EventKeywords matchAnyKeyword,
                                   IDictionary<string, string?>? commandArguments)
@@ -2500,7 +2490,7 @@ internal void SendCommand(EventListener? listener, EventProviderType eventProvid
                 return;
             }
 
-            var commandArgs = new EventCommandEventArgs(command, commandArguments, this, listener, eventProviderType, perEventSourceSessionId, etwSessionId, enable, level, matchAnyKeyword);
+            var commandArgs = new EventCommandEventArgs(command, commandArguments, this, listener, eventProviderType, perEventSourceSessionId, enable, level, matchAnyKeyword);
             lock (EventListener.EventListenersLock)
             {
                 if (m_completelyInited)
@@ -4058,7 +4048,7 @@ public void EnableEvents(EventSource eventSource, EventLevel level, EventKeyword
         {
             ArgumentNullException.ThrowIfNull(eventSource);
 
-            eventSource.SendCommand(this, EventProviderType.None, 0, 0, EventCommand.Update, true, level, matchAnyKeyword, arguments);
+            eventSource.SendCommand(this, EventProviderType.None, 0, EventCommand.Update, true, level, matchAnyKeyword, arguments);
 
 #if FEATURE_PERFTRACING
             if (eventSource.GetType() == typeof(NativeRuntimeEventSource))
@@ -4076,7 +4066,7 @@ public void DisableEvents(EventSource eventSource)
         {
             ArgumentNullException.ThrowIfNull(eventSource);
 
-            eventSource.SendCommand(this, EventProviderType.None, 0, 0, EventCommand.Update, false, EventLevel.LogAlways, EventKeywords.None, null);
+            eventSource.SendCommand(this, EventProviderType.None, 0, EventCommand.Update, false, EventLevel.LogAlways, EventKeywords.None, null);
 
 #if FEATURE_PERFTRACING
             if (eventSource.GetType() == typeof(NativeRuntimeEventSource))
@@ -4505,15 +4495,14 @@ public bool DisableEvent(int eventId)
 #region private
 
         internal EventCommandEventArgs(EventCommand command, IDictionary<string, string?>? arguments, EventSource eventSource,
-            EventListener? listener, EventProviderType eventProviderType, int perEventSourceSessionId, int etwSessionId, bool enable, EventLevel level, EventKeywords matchAnyKeyword)
+            EventListener? listener, EventProviderType eventProviderType, int perEventSourceSessionId, bool enable, EventLevel level, EventKeywords matchAnyKeyword)
         {
             this.Command = command;
             this.Arguments = arguments;
             this.eventSource = eventSource;
             this.listener = listener;
             this.eventProviderType = eventProviderType;
             this.perEventSourceSessionId = perEventSourceSessionId;
-            this.etwSessionId = etwSessionId;
             this.enable = enable;
             this.level = level;
             this.matchAnyKeyword = matchAnyKeyword;
@@ -4526,7 +4515,6 @@ internal EventCommandEventArgs(EventCommand command, IDictionary<string, string?
         // These are the arguments of sendCommand and are only used for deferring commands until after we are fully initialized.
         internal EventListener? listener;
         internal int perEventSourceSessionId;
-        internal int etwSessionId;
         internal bool enable;
         internal EventLevel level;
         internal EventKeywords matchAnyKeyword;

src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/TraceLogging/NameInfo.cs

@@ -95,7 +95,7 @@ public IntPtr GetOrCreateEventHandle(EventProvider provider, TraceLoggingEventHa
                             fixed (byte* pMetadataBlob = metadata)
                             {
                                 // Define the event.
-                                eventHandle = provider.m_eventProvider.DefineEventHandle(
+                                eventHandle = provider._eventProvider.DefineEventHandle(
                                     (uint)descriptor.EventId,
                                     name,
                                     descriptor.Keywords,

src/native/eventpipe/ep-config.c

@@ -122,7 +122,8 @@ config_register_provider (
 					ep_session_provider_get_keywords (session_provider),
 					ep_session_provider_get_logging_level (session_provider),
 					ep_session_provider_get_filter_data (session_provider),
-					&provider_callback_data);
+					&provider_callback_data,
+					(EventPipeSessionID)session);
 				if (provider_callback_data_queue)
 					ep_provider_callback_data_queue_enqueue (provider_callback_data_queue, &provider_callback_data);
 				ep_provider_callback_data_fini (&provider_callback_data);
@@ -562,7 +563,8 @@ config_enable_disable (
 							ep_session_provider_get_keywords (session_provider),
 							ep_session_provider_get_logging_level (session_provider),
 							ep_session_provider_get_filter_data (session_provider),
-							&provider_callback_data);
+							&provider_callback_data,
+							(EventPipeSessionID)session);
 					} else {
 						provider_unset_config (
 							provider,

src/native/eventpipe/ep-provider-internals.h

@@ -21,7 +21,8 @@ provider_set_config (
 	int64_t keywords,
 	EventPipeEventLevel level,
 	const ep_char8_t *filter_data,
-	EventPipeProviderCallbackData *callback_data);
+	EventPipeProviderCallbackData *callback_data,
+	EventPipeSessionID session_id);
 
 // Unset the provider configuration for the specified session (disable sets of events).
 // _Requires_lock_held (ep)

src/native/eventpipe/ep-provider.c

@@ -28,7 +28,8 @@ provider_prepare_callback_data (
 	int64_t keywords,
 	EventPipeEventLevel provider_level,
 	const ep_char8_t *filter_data,
-	EventPipeProviderCallbackData *provider_callback_data);
+	EventPipeProviderCallbackData *provider_callback_data,
+	EventPipeSessionID session_id);
 
 // _Requires_lock_held (ep)
 static
@@ -69,7 +70,8 @@ provider_prepare_callback_data (
 	int64_t keywords,
 	EventPipeEventLevel provider_level,
 	const ep_char8_t *filter_data,
-	EventPipeProviderCallbackData *provider_callback_data)
+	EventPipeProviderCallbackData *provider_callback_data,
+	EventPipeSessionID session_id)
 {
 	EP_ASSERT (provider != NULL);
 	EP_ASSERT (provider_callback_data != NULL);
@@ -81,7 +83,8 @@ provider_prepare_callback_data (
 		provider->callback_data,
 		keywords,
 		provider_level,
-		provider->sessions != 0);
+		provider->sessions != 0,
+		session_id);
 }
 
 static
@@ -299,7 +302,8 @@ provider_set_config (
 	int64_t keywords,
 	EventPipeEventLevel level,
 	const ep_char8_t *filter_data,
-	EventPipeProviderCallbackData *callback_data)
+	EventPipeProviderCallbackData *callback_data,
+	EventPipeSessionID session_id)
 {
 	EP_ASSERT (provider != NULL);
 	EP_ASSERT ((provider->sessions & session_mask) == 0);
@@ -311,7 +315,7 @@ provider_set_config (
 	provider->provider_level = level_for_all_sessions;
 
 	provider_refresh_all_events (provider);
-	provider_prepare_callback_data (provider, provider->keywords, provider->provider_level, filter_data, callback_data);
+	provider_prepare_callback_data (provider, provider->keywords, provider->provider_level, filter_data, callback_data, session_id);
 
 	ep_requires_lock_held ();
 	return callback_data;
@@ -340,7 +344,7 @@ provider_unset_config (
 	provider->provider_level = level_for_all_sessions;
 
 	provider_refresh_all_events (provider);
-	provider_prepare_callback_data (provider, provider->keywords, provider->provider_level, filter_data, callback_data);
+	provider_prepare_callback_data (provider, provider->keywords, provider->provider_level, filter_data, callback_data, (EventPipeSessionID)0);
 
 	ep_requires_lock_held ();
 	return callback_data;
@@ -360,6 +364,7 @@ provider_invoke_callback (EventPipeProviderCallbackData *provider_callback_data)
 	int64_t keywords = ep_provider_callback_data_get_keywords (provider_callback_data);
 	EventPipeEventLevel provider_level = ep_provider_callback_data_get_provider_level (provider_callback_data);
 	void *callback_data = ep_provider_callback_data_get_callback_data (provider_callback_data);
+	EventPipeSessionID session_id = ep_provider_callback_data_get_session_id (provider_callback_data);
 
 	bool is_event_filter_desc_init = false;
 	EventFilterDescriptor event_filter_desc;
@@ -405,7 +410,7 @@ provider_invoke_callback (EventPipeProviderCallbackData *provider_callback_data)
 	if (callback_function && !ep_rt_process_shutdown ()) {
 		ep_rt_provider_invoke_callback (
 			callback_function,
-			NULL, /* provider_id */
+			(uint8_t *)&session_id, /* session_id */
 			enabled ? 1 : 0, /* ControlCode */
 			(uint8_t)provider_level,
 			(uint64_t)keywords,

src/native/eventpipe/ep-types.h

@@ -69,6 +69,7 @@ struct _EventPipeProviderCallbackData_Internal {
 	int64_t keywords;
 	EventPipeEventLevel provider_level;
 	bool enabled;
+	EventPipeSessionID session_id;
 };
 
 #if !defined(EP_INLINE_GETTER_SETTER) && !defined(EP_IMPL_EP_GETTER_SETTER)
@@ -83,6 +84,7 @@ EP_DEFINE_GETTER(EventPipeProviderCallbackData *, provider_callback_data, void *
 EP_DEFINE_GETTER(EventPipeProviderCallbackData *, provider_callback_data, int64_t, keywords)
 EP_DEFINE_GETTER(EventPipeProviderCallbackData *, provider_callback_data, EventPipeEventLevel, provider_level)
 EP_DEFINE_GETTER(EventPipeProviderCallbackData *, provider_callback_data, bool, enabled)
+EP_DEFINE_GETTER(EventPipeProviderCallbackData *, provider_callback_data, EventPipeSessionID, session_id)
 
 EventPipeProviderCallbackData *
 ep_provider_callback_data_alloc (
@@ -91,7 +93,8 @@ ep_provider_callback_data_alloc (
 	void *callback_data,
 	int64_t keywords,
 	EventPipeEventLevel provider_level,
-	bool enabled);
+	bool enabled,
+	EventPipeSessionID session_id);
 
 EventPipeProviderCallbackData *
 ep_provider_callback_data_alloc_copy (EventPipeProviderCallbackData *provider_callback_data_src);
@@ -107,7 +110,8 @@ ep_provider_callback_data_init (
 	void *callback_data,
 	int64_t keywords,
 	EventPipeEventLevel provider_level,
-	bool enabled);
+	bool enabled,
+	EventPipeSessionID session_id);
 
 EventPipeProviderCallbackData *
 ep_provider_callback_data_init_copy (

src/native/eventpipe/ep.c

@@ -217,7 +217,8 @@ ep_provider_callback_data_alloc (
 	void *callback_data,
 	int64_t keywords,
 	EventPipeEventLevel provider_level,
-	bool enabled)
+	bool enabled,
+	EventPipeSessionID session_id)
 {
 	EventPipeProviderCallbackData *instance = ep_rt_object_alloc (EventPipeProviderCallbackData);
 	ep_raise_error_if_nok (instance != NULL);
@@ -229,7 +230,8 @@ ep_provider_callback_data_alloc (
 		callback_data,
 		keywords,
 		provider_level,
-		enabled) != NULL);
+		enabled,
+		session_id) != NULL);
 
 ep_on_exit:
 	return instance;
@@ -288,7 +290,8 @@ ep_provider_callback_data_init (
 	void *callback_data,
 	int64_t keywords,
 	EventPipeEventLevel provider_level,
-	bool enabled)
+	bool enabled,
+	EventPipeSessionID session_id)
 {
 	EP_ASSERT (provider_callback_data != NULL);
 
@@ -298,6 +301,7 @@ ep_provider_callback_data_init (
 	provider_callback_data->keywords = keywords;
 	provider_callback_data->provider_level = provider_level;
 	provider_callback_data->enabled = enabled;
+	provider_callback_data->session_id = session_id;
 
 	return provider_callback_data;
 }

src/tests/issues.targets

@@ -3790,6 +3790,9 @@
         <ExcludeList Include = "$(XunitTestBinBase)/tracing/eventpipe/eventsourceerror/**">
             <Issue> needs triage </Issue>
         </ExcludeList>
+        <ExcludeList Include = "$(XunitTestBinBase)/tracing/eventpipe/enabledisable/**">
+            <Issue> needs triage </Issue>
+        </ExcludeList>
         <ExcludeList Include = "$(XunitTestBinBase)/Exceptions/ForeignThread/ForeignThreadExceptions/**">
             <Issue>needs triage</Issue>
         </ExcludeList>