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

Remove static modifiers from ThreadPool events in NativeRuntimeEventSource.PortableThreadPool.NativeSinks.cs #85563

Merged
merged 7 commits into from
May 13, 2023

Conversation

nickyMcDonald
Copy link
Contributor

Here is a fix for issue #78006 that works with win11 .NET 7.

The EventPipeEventDispatcher cycles through EventPipeInternal via QCall and sends them to be processed by the NativeRuntimeEventSource.

while (!m_stopDispatchTask && EventPipeInternal.GetNextEvent(m_sessionID, &instanceData))
{
eventsReceived = true;
// Filter based on provider.
if (instanceData.ProviderID == m_RuntimeProviderID)
{
// Dispatch the event.
ReadOnlySpan<byte> payload = new ReadOnlySpan<byte>((void*)instanceData.Payload, (int)instanceData.PayloadLength);
DateTime dateTimeStamp = TimeStampToDateTime(instanceData.TimeStamp);
NativeRuntimeEventSource.Log.ProcessEvent(instanceData.EventID, instanceData.ThreadID, dateTimeStamp, instanceData.ActivityId, instanceData.ChildActivityId, payload);
}
}

From there they are sent to be decoded by the EventPipePayloadDecoder.
object[] decodedPayloadFields = EventPipePayloadDecoder.DecodePayload(ref m_eventData[eventID], payload);

There is where the NRE occurs because the metadata is uninitialized. Parameters is default null and attempting to get its length results in NRE. Looking back at the NativeRuntimeEventSource the NRE occurs with eventID 64 (ThreadPoolIODequeue).
internal static object[] DecodePayload(ref EventSource.EventMetadata metadata, ReadOnlySpan<byte> payload)
{
ParameterInfo[] parameters = metadata.Parameters;
object[] decodedFields = new object[parameters.Length];

In the EventSource. There are no BindingFlags for static methods. This is important because any static methods from the NativeRuntimeEventSource won’t get loaded. Because this data is used to initialize m_eventData values, any methods left out will leave uninitialized slots in m_eventData. Slot 64 being one of them.
MethodInfo[] methods = eventSourceType.GetMethods(BindingFlags.DeclaredOnly | BindingFlags.NonPublic | BindingFlags.Public | BindingFlags.Instance);

The fix will allow for event 64 and 63 to be loaded into m_eventData before pr #66333 again.

remove static modifiers from lines:
 - 156
 - 191
in:
src\libraries\System.Private.CoreLib\src\System\Threading\NativeRuntimeEventSource.PortableThreadPool.NativeSinks.cs
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Apr 30, 2023
@ghost
Copy link

ghost commented Apr 30, 2023

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

Issue Details

Here is a fix for issue #78006 that works with win11 .NET 7.

The EventPipeEventDispatcher cycles through EventPipeInternal via QCall and sends them to be processed by the NativeRuntimeEventSource.

while (!m_stopDispatchTask && EventPipeInternal.GetNextEvent(m_sessionID, &instanceData))
{
eventsReceived = true;
// Filter based on provider.
if (instanceData.ProviderID == m_RuntimeProviderID)
{
// Dispatch the event.
ReadOnlySpan<byte> payload = new ReadOnlySpan<byte>((void*)instanceData.Payload, (int)instanceData.PayloadLength);
DateTime dateTimeStamp = TimeStampToDateTime(instanceData.TimeStamp);
NativeRuntimeEventSource.Log.ProcessEvent(instanceData.EventID, instanceData.ThreadID, dateTimeStamp, instanceData.ActivityId, instanceData.ChildActivityId, payload);
}
}

From there they are sent to be decoded by the EventPipePayloadDecoder.
object[] decodedPayloadFields = EventPipePayloadDecoder.DecodePayload(ref m_eventData[eventID], payload);

There is where the NRE occurs because the metadata is uninitialized. Parameters is default null and attempting to get its length results in NRE. Looking back at the NativeRuntimeEventSource the NRE occurs with eventID 64 (ThreadPoolIODequeue).
internal static object[] DecodePayload(ref EventSource.EventMetadata metadata, ReadOnlySpan<byte> payload)
{
ParameterInfo[] parameters = metadata.Parameters;
object[] decodedFields = new object[parameters.Length];

In the EventSource. There are no BindingFlags for static methods. This is important because any static methods from the NativeRuntimeEventSource won’t get loaded. Because this data is used to initialize m_eventData values, any methods left out will leave uninitialized slots in m_eventData. Slot 64 being one of them.
MethodInfo[] methods = eventSourceType.GetMethods(BindingFlags.DeclaredOnly | BindingFlags.NonPublic | BindingFlags.Public | BindingFlags.Instance);

The fix will allow for event 64 and 63 to be loaded into m_eventData before pr #66333 again.

Author: n77y
Assignees: -
Labels:

area-System.Threading

Milestone: -

@jkotas
Copy link
Member

jkotas commented Apr 30, 2023

@jkotas
Copy link
Member

jkotas commented Apr 30, 2023

@hoyosjs
Copy link
Member

hoyosjs commented May 9, 2023

Hmm... tracing/runtimeeventsource/nativeruntimeeventsource/nativeruntimeeventsource.sh is now timing out and a dump was collected on macOS x64

@nickyMcDonald
Copy link
Contributor Author

Hmm... tracing/runtimeeventsource/nativeruntimeeventsource/nativeruntimeeventsource.sh is now timing out and a dump was collected on macOS x64

After adding tests to NativeRuntimeEventSourceTest.cs and returning the static modifiers to NativeRuntimeEventSource.PortableThreadPool.NativeSinks.cs the tests successfully failed. I will now remove the static modifiers and the tracing tests should now pass.

@tommcdon
Copy link
Member

@davmason ptal

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Threading community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants