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

Improve logging of GC ref map mismatches #80219

Merged
merged 4 commits into from
Jan 6, 2023

Conversation

trylek
Copy link
Member

@trylek trylek commented Jan 5, 2023

As part of investigation of the GC refmap mismatch I'm fixing with

#80218

I have improved CoreCLR runtime behavior in the presence of GC refmap mismatches by adding debug-only logic to dump both the GC ref map emitted by Crossgen2 and the one calculated by the native runtime. The dump format matches the way R2RDump outputs GC ref maps next to method import cells in the R2R file.

Thanks

Tomas

/cc @dotnet/crossgen-contrib

C:\git\runtime>C:\triage\wpf\bin\Debug\net7.0-windows\win-arm64\publish\wpf.exe
GC ref map mismatch detected for method: System.Windows.Media.PathGeometry::GetPathBoundsAsRB
Runtime GC ref map: I(70) R(78) I(80)
Crossgen2 GC ref map: I(68 70) R(78) I(80)

Assert failure(PID 12596 [0x00003134], Thread: 9568 [0x2560]): false

CORECLR! CheckGCRefMapEqual + 0x1DC (0x00007fffe66fa72c) CORECLR! ExternalMethodFixupWorker + 0xA98 (0x00007fffe681b3c8)
CORECLR! DelayLoad_MethodCall + 0x58 (0x00007fffe6581a18) PRESENTATIONCORE! <no symbol> + 0x0 (0x00007fffe401ecac)
PRESENTATIONCORE! + 0x0 (0x00007fffe401eaa4) PRESENTATIONFRAMEWORK! <no symbol> + 0x0 (0x00007fffdbd34db4)
PRESENTATIONFRAMEWORK! + 0x0 (0x00007fffdbd33d6c) PRESENTATIONFRAMEWORK! <no symbol> + 0x0 (0x00007fffdbc9a85c)
PRESENTATIONCORE! + 0x0 (0x00007fffe3ecde14) PRESENTATIONFRAMEWORK! <no symbol> + 0x0 (0x00007fffdc13ce5c)
File: C:\git\runtime\src\coreclr\vm\frames.cpp Line: 2177
Image: C:\triage\wpf\bin\Debug\net7.0-windows\win-arm64\publish\wpf.exe

@trylek
Copy link
Member Author

trylek commented Jan 5, 2023

(apologies for updating the bug description, I originally copied an earlier version of the CoreCLR runtime dump where I had reversed the tags for Crossgen2 vs. Runtime)

@janvorli
Copy link
Member

janvorli commented Jan 5, 2023

The Unix builds are failing because of the wprintf usage. We have recently removed support for wprintf from PAL, see #77852.

@trylek
Copy link
Member Author

trylek commented Jan 5, 2023

@janvorli - thanks for the heads-up, so does that mean that I should be using printf instead?

@janvorli
Copy link
Member

janvorli commented Jan 5, 2023

so does that mean that I should be using printf instead?

Yes

@janvorli
Copy link
Member

janvorli commented Jan 5, 2023

And also not to use %S in printf, as on Unix it would expect 32 bit char.

src/coreclr/vm/frames.cpp Outdated Show resolved Hide resolved
Copy link
Member

@janvorli janvorli left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@trylek trylek merged commit afc963e into dotnet:main Jan 6, 2023
@trylek trylek deleted the GCRefMapMismatchReporting branch January 6, 2023 22:48
@AntonLapounov
Copy link
Member

Great improvement. I have a question regarding dumping: we skip GCREFMAP_SKIP tokens when we dump; however, in theory it is possible that the two GCRefMaps differ exactly in one of GCREFMAP_SKIP tokens, in which case the difference may not be easily seen from the output.

Nit: this may need a trailing space.

printf("POP(0x%x)", stackPop);

@trylek
Copy link
Member Author

trylek commented Jan 17, 2023

Thanks Anton for your feedback. I think that GCREFMAP_SKIP can only become an issue at the end of the GC refmap dump, otherwise it would distort the offset and trigger a mismatch for the subsequent non-skip tokens. From this viewpoint I guess we might want to log trailing GCREFMAP_SKIP entries loosely akin to how Roslyn now reports stray whitespace at line ends. I think you're also right about the POP, I "copied" that bug from R2RDump which behaves the same I believe. I'll put up a PR to fix these two issues, thanks for pointing them out!

@ghost ghost locked as resolved and limited conversation to collaborators Feb 17, 2023
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.

3 participants