-
Notifications
You must be signed in to change notification settings - Fork 388
Remove reliance on s_isTracking to detect recursion #297
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
Conversation
Fixes dotnet/coreclr#21994
Codecov Report
@@ Coverage Diff @@
## master #297 +/- ##
==========================================
+ Coverage 88.85% 89.06% +0.21%
==========================================
Files 16 16
Lines 1875 1912 +37
==========================================
+ Hits 1666 1703 +37
Misses 209 209
Continue to review full report at Codecov.
|
@sharwell Is this PR ready to be merged? |
@codemzs No, I'm working with @ViktorHofer to figure this out still |
367315f
to
db5dbc5
Compare
OK this should be ready now 😄 |
db5dbc5
to
be7a5d6
Compare
@@ -76,6 +77,7 @@ private void InstrumentModule() | |||
|
|||
using (var module = ModuleDefinition.ReadModule(stream, parameters)) | |||
{ | |||
var containsAppContext = module.GetType(nameof(System), nameof(AppContext)) != null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📝 This could be modified to also check the name of the containing assembly, but it didn't seem necessary.
I validated the fix together with @sharwell offline and we finally got our code coverage for System.Private.CoreLib back hooray.
|
be7a5d6
to
96b1594
Compare
cc @tonerdo |
I see you all have been busy 😄. Thanks for all the help guys |
Fixes dotnet/coreclr#21994
The stack overflow was caused when a call to
RecordHit
occurred prior to the initialization ofHitsArray
, which in turn called an instrumented constructor forNullReferenceException
.