-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Diagnostic changes to R2R for issues #106675 and #108255 #118370
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
…ureImportSections or Initialize
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.
Pull Request Overview
This PR adds diagnostic output to the ReadyToRun (R2R) reader to help troubleshoot failures reported in issues #106675 and #108255. The changes focus on providing detailed information about PE image structure when BadImageFormatException occurs.
Key changes:
- Added exception handling with diagnostic output in ReadyToRunReader initialization methods
- Enhanced PEExportTable with additional diagnostic information and console dumping capabilities
- Added image information dumping including PE header details, export table data, and file metadata
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
File | Description |
---|---|
ReadyToRunReader.cs | Added try-catch blocks with diagnostic output for Initialize() and EnsureImportSections() methods, plus DumpImageInformation() method |
PEReaderExtensions.cs | Enhanced PEExportTable with diagnostic fields, console error logging for zero RVAs, and DumpToConsoleError() method |
Comments suppressed due to low confidence (2)
src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/ReadyToRunReader.cs:490
- [nitpick] The opening brace for the try block should be on the same line as the try statement according to C# conventions.
try
src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/ReadyToRunReader.cs:520
- [nitpick]
{
Interestingly, this seems to break for composite r2r locally:
EDIT: Based on DependenciesGui and dumpbin this looks like a valid PE executable, but we're failing to parse its export table for some reason. I think this may be a bug in R2RDump. |
Successfully verified this locally so it's ready for review. |
/azp run runtime-coreclr crossgen2 outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
This fails if the CORHeader is missing ATM, I have a fix for that I'll push once I see the test results. |
…xceptions thrown inside of it
Hopefully these changes should produce more diagnostic output when this fails on outerloop to help track down what's happening. I haven't been able to reproduce the failures locally on linux or windows yet.