Fix crossgen2 sometimes emitting tokens from the wrong module into R2R output #114029
Conversation
ghost
added
the
kg
added
the
NO-MERGE
|
/azp run runtime-coreclr crossgen2 outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run runtime-coreclr crossgen2 outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
…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.
kg
force-pushed
the
r2r-crossmodule-token-fix
branch
from
7cdf547 to
cf5986e
Compare
|
/azp run runtime-coreclr crossgen2 outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run runtime-coreclr crossgen2 outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run runtime-coreclr crossgen2 outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Looks like R2RDump broke again, but R2R is working fine on outerloop. |
|
There does appear to be an actual failure in the JIT on arm64, though: |
kg
added
Servicing-consider
There was a problem hiding this comment.
Pull Request Overview
This PR addresses a bug where R2R accidentally emits tokens from the wrong module by adding safety checks and updating comparison logic between tokens.
- Updated token equality checks in RuntimeDeterminedTypeHelper to include module comparisons.
- Added validation in SignatureBuilder and ModuleTokenResolver to ensure tokens are used only from modules within the version bubble.
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 | Added module equality checks for MethodWithToken and FieldWithToken comparisons. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/SignatureBuilder.cs | Introduced a validation check that throws an error if a token from a module outside the version bubble is used. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/ModuleTokenResolver.cs | Implemented a module version check similar to SignatureBuilder and suggested caching the lookup result. |
Comments suppressed due to low confidence (1)
src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/ModuleTokenResolver.cs:316
- Consider returning the previously computed 'moduleIndex' (from line 308) instead of calling _moduleIndexLookup(module) a second time to avoid redundant computations.
return _moduleIndexLookup(module);
|
Looking at the recent scheduled outerloop runs, the arm64 jit issue has been around for at least a month, but this r2rdump problem is either brand new or is somehow related to this PR. |
|
Friendly reminder that code complete is on April 14th for the May Release. If you'd like to get this change included in that release, please get a Tactics approval and merge this PR before that date. |
|
After some discussion we decided to target main with this first, because on 9.0-staging it breaks CI due to not having #107064 . |
4 participants
Description
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.)
Customer Impact
Customers rely on R2R and without this fix, users who install updated versions of the .NET runtime will experience breakage due to BCL assemblies no longer matching the ones that the application's R2R modules were compiled against
Regression
No
Testing
Manual testing by building+packaging an affected assembly (from roslyn) from source using 9.0.3 and this PR. File size did not meaningfully change and the bad tokens are no longer emitted.
Risk
If I missed a scenario where bad tokens can be emitted, R2R compilation will now fail with an error instead of silently producing a corrupted assembly. This would stop the customer from using R2R until the issue is fixed.