Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Tighten GC mode assertion for EventPipe::Enable()#24124

Merged
jkotas merged 1 commit into
dotnet:masterfrom
cshung:dev/andrewau/tighten-assert-1
Apr 20, 2019
Merged

Tighten GC mode assertion for EventPipe::Enable()#24124
jkotas merged 1 commit into
dotnet:masterfrom
cshung:dev/andrewau/tighten-assert-1

Conversation

@cshung
Copy link
Copy Markdown

@cshung cshung commented Apr 19, 2019

According to the documentation, QCALL involves a proper PInvoke transition frame, which means unless explicitly switched, the native code paths are in preemptive mode. That gives us an opportunity to tighten the asserts. Tightening the assert allows us to catch current bugs, prevent future regressions and better (live) document what the mode is supposed to be.

As far as I understand, EventPipe::Enable() could happen only if it is triggered in managed code through QCALL or from IPC (which is purely native code), therefore we can assert native code that is used only for EventPipe::Enable() has to be in preemptive mode. This analysis forms the basis of this PR, so if this is incorrect, feel free to comment on this analysis.

I proved (though tedious manual static code analysis) that the functions I changed are called only in the EventPipe::Enable(). Feel free to comment if you found the code I changed could be called by other code-path, because if that's the case, my assumption might be wrong. It might not capture all calls that are involved only in EventPipe::Enable(), so feel free to point those out as well.

For calls that are used in EventPipe::Write() cannot be tightened because we knew it might be called directly (through the code generator) in random places in the runtime where it might already be in cooperative mode (I was able to prove in some case EventPipe::Write() is indeed called by thread in cooperative mode. (Those calls should be scrutinized for their potential to block GC).

@cshung cshung requested review from jorive, noahfalk and sywhang April 19, 2019 22:27
@jorive jorive added area-Tracing EventPipe enhancement Product code improvement that does NOT require public API changes/additions labels Apr 19, 2019
@cshung
Copy link
Copy Markdown
Author

cshung commented Apr 20, 2019

@dotnet-bot Test Windows_NT x64 Checked CoreFX Tests please

@jkotas jkotas merged commit b269345 into dotnet:master Apr 20, 2019
@cshung cshung deleted the dev/andrewau/tighten-assert-1 branch April 20, 2019 20:52
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-Tracing enhancement Product code improvement that does NOT require public API changes/additions EventPipe

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants