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 AV in NativeRuntimeEventSource QCalls #48414

Merged
merged 5 commits into from
Feb 18, 2021

Conversation

sywhang
Copy link
Contributor

@sywhang sywhang commented Feb 17, 2021

Fix #48407 and #48368.

There was a error in the last commit I made as part of #47829 where I moved around some of the QCalls from XplatEventLogger to NativeRuntimeEventSource that wasn't properly reflected in ecalllist.h that causes an AV to occur.

This PR fixes ecalllist.h to properly reflect where the QCalls are getting called from, and adds a regression test that checks for the ThreadPool events.

cc @brianrob

Copy link
Contributor

@josalem josalem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yay for new tests! This looks good to me if we get a clean CI back.

@jkotas
Copy link
Member

jkotas commented Feb 17, 2021

Is this also a problem on Mono? I do not see where these QCalls come from when running on Mono.

@sywhang
Copy link
Contributor Author

sywhang commented Feb 17, 2021

@jkotas I don't think these methods can get invoked on Mono because Mono is built with EventSourceSupport feature flag set to false and that prevents the EventSource from getting enabled and the ThreadPool checks whether the events are enabled before emitting them.

@josalem do you know if Mono is planning to support emitting runtime events from .NET 6?

@josalem
Copy link
Contributor

josalem commented Feb 17, 2021

@lateralusX & @lambdageek would know (or know who would know) whether they plan to turn it on in 6.

@jkotas
Copy link
Member

jkotas commented Feb 17, 2021

I don't think these methods can get invoked on Mono because Mono is built with EventSourceSupport feature flag set to false

Mono sets EventSourceSupport feature flag to false for wasm, but I do not see it set for other platforms.

Mono sets FeaturePerfTracing here: https://github.com/dotnet/runtime/blob/master/src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj#L112

It will cause NativeRuntimeEventSource.cs to be included here: https://github.com/dotnet/runtime/blob/master/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems#L1201

NativeEventSource.cs has the managed QCall declarations: https://github.com/dotnet/runtime/blob/master/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/NativeRuntimeEventSource.cs#L111

@sywhang
Copy link
Contributor Author

sywhang commented Feb 17, 2021

@jkotas thanks for pointing that out. I checked again and it seems like the best way to move forward is to just emit these events using the WriteEventCore APIs for Mono instead of the QCalls since Mono doesn't have the same native event stubs that CoreCLR does, and there's doesn't seem to be a concept of native runtime events from Mono. (Please correct me on this if I'm wrong @lateralusX @lambdageek)

jkotas added a commit to jkotas/runtimelab that referenced this pull request Feb 17, 2021
We should eventually switch to the same plan as Mono once it actually works (more context in dotnet/runtime#48414 (comment))
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@lambdageek
Copy link
Member

and there's doesn't seem to be a concept of native runtime events from Mono. (Please correct me on this if I'm wrong @lateralusX @lambdageek)

@lateralusX is first working on managed event sources on mobile. native events are going to be added later. and I believe we discussed adding a few events at a time ad hoc by hand, rather than generating all the ones CoreCLR has defined.

jkotas added a commit to jkotas/runtimelab that referenced this pull request Feb 18, 2021
We should eventually switch to the same plan as Mono once it actually works (more context in dotnet/runtime#48414 (comment))
jkotas added a commit to jkotas/runtimelab that referenced this pull request Feb 18, 2021
We should eventually switch to the same plan as Mono once it actually works (more context in dotnet/runtime#48414 (comment))
jkotas added a commit to jkotas/runtimelab that referenced this pull request Feb 18, 2021
We should eventually switch to the same plan as Mono once it actually works (more context in dotnet/runtime#48414 (comment))
@sywhang
Copy link
Contributor Author

sywhang commented Feb 18, 2021

@lambdageek Thanks for that info - the patch as of now will keep these events flowing to Mono runtime's EventPipe, as well as any EventListeners so I think we're unblocked on these. For future events, there may be a path for a cleaner solution both on CoreCLR and Mono. @brianrob talked about potentially generating these managed events in NativeRuntimeEventSource from ClrEtwAll.man but given the context, that may be a little difficult since not all events are shared across the runtimes.

Copy link
Member

@noahfalk noahfalk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM : )

@sywhang sywhang merged commit ae40145 into dotnet:master Feb 18, 2021
@sywhang sywhang deleted the dev/suwhang/48407 branch February 18, 2021 22:38
@lateralusX
Copy link
Member

We will revisit this on Mono side when we look more broadly to support native events and see if it can be unified (not currently implemented). The interop between managed and native EventSource -> EventPipe is icalls on Mono (due to a couple of reasons), but since we added support for qcalls in Mono since, we might look into switching, but if not, we can always make a small shim as done for all other EventPipe interop in https://github.com/dotnet/runtime/blob/master/src/mono/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventPipe.Mono.cs if needed.

@ghost ghost locked as resolved and limited conversation to collaborators Mar 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash When Enabling Threading Events on Windows
6 participants