Fix crossgen2 sometimes emitting tokens from the wrong module #114172
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ldWithToken to ensure that the tokens in question are from the same module. Prevents us from erroneously using another module's token in a way that will produce bad R2R modules.
|
/azp run runtime-coreclr crossgen2 outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull Request Overview
This PR aims to prevent crossgen2 from emitting tokens from the wrong module by adding a safety check during R2R generation and updating token comparers.
- Updated equality comparisons in RuntimeDeterminedTypeHelper to include module tokens.
- Added a safety check in SignatureBuilder to throw an error for tokens from modules outside the version bubble.
- Added a similar safety check in ModuleTokenResolver for validating module tokens.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/RuntimeDeterminedTypeHelper.cs | Adds module token comparisons in equality checks for methods and fields. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/SignatureBuilder.cs | Introduces a safety check ensuring that tokens are only used from modules within the version bubble. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/ModuleTokenResolver.cs | Adds a version-bubble check for module tokens and raises an error if the module is not valid. |
Comments suppressed due to low confidence (1)
src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/ModuleTokenResolver.cs:316
- Consider caching the result of _moduleIndexLookup(module) after the initial call to avoid duplicate lookups later in the method.
return _moduleIndexLookup(module);
|
The arm64 outerloop failure looks like it would be fixed by #106099. |
This was referenced Apr 3, 2025
davidwrighton
approved these changes
May 23, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Under very specific circumstances, R2R can accidentally allow tokens from other modules into its output. These tokens are valid as long as none of the assemblies in use change, but if an assembly changes (i.e. due to a runtime update) tokens pointing into that assembly are no longer valid, and you'll get crashes at runtime from the tokens no longer being what the R2R module expects them to be.
This PR adds a safety check to detect this happening during R2R generation so that we won't quietly produce broken R2R modules, and it also fixes a couple comparers to detect the case where tokens are from different modules (previously, tokens from different modules would compare as equal if they were otherwise equivalent.)
Fixes #114183