Fix crossgen2 R2R issues found by static analysis#129495
Open
jtschuster wants to merge 3 commits into
Open
Conversation
Addresses several latent bugs in the ILCompiler.ReadyToRun (crossgen2) compiler surfaced by PVS-Studio's .NET analysis: - MutableModule.CompareTo compared _index to itself instead of other._index, so the comparison always returned 0. - ModuleAndIntValueKey.Equals dereferenced Module after a check that let "this null, other non-null" fall through, throwing an NRE. - IsInheritanceChainLayoutFixedInCurrentVersionBubble had a null check after the dereference in its while condition; removed the dead check and assert that BaseType is non-null while walking to System.Object. - PropertyPseudoDesc/EventPseudoDesc operator== dereferenced operand fields without a null guard; null operands now compare correctly instead of throwing. - Removed the unreachable MethodDictionary/TypeDictionary arms of GenericLookupHelper. The sole caller never produces those helper IDs, and the TypeDictionary arm would have emitted a fixup the runtime dictionary decoder cannot parse. They now fall through to the existing NotImplementedException default. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
jtschuster
added
area-ReadyToRun
and removed
area-crossgen2-coreclr
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes several correctness issues in crossgen2/ILCompiler code paths (primarily latent null-dereferences and an always-0 comparison) and removes unreachable switch cases to align behavior with actual callers.
Changes:
- Make
PropertyPseudoDesc/EventPseudoDescequality operators null-safe. - Fix
MutableModule.CompareToto actually compare againstother(instead of itself) and adjust a null-deref in a dictionary key equality implementation. - Simplify internal logic/switch handling by removing dead/unreachable code paths and tightening an inheritance-walk loop.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/tools/Common/Compiler/PropertyPseudoDesc.cs | Make operator== handle null operands safely. |
| src/coreclr/tools/Common/Compiler/EventPseudoDesc.cs | Make operator== handle null operands safely. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/TypeSystem/Mutable/MutableModule.Sorting.cs | Fix CompareTo to compare _index with other._index. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCodegenCompilation.cs | Remove ineffective null check in inheritance-chain loop; assert expected invariant. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunSymbolNodeFactory.cs | Remove unreachable MethodDictionary / TypeDictionary arms from GenericLookupHelper. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunCodegenNodeFactory.cs | Fix ModuleAndIntValueKey.Equals to avoid dereferencing a null Module. |
Open
3 tasks
- MutableModule.CompareTo: there is only ever one MutableModule per build, so assert self-comparison and return 0, dropping the unused s_globalIndex/_index bookkeeping. - ModuleAndIntValueKey.Equals: ModuleDesc has referential identity, so compare Module references directly instead of the overcomplicated null-guarded Equals call. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
MichalStrehovsky
approved these changes
Jun 18, 2026
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
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.
Addresses latent bugs in crossgen2/ilc reported in #129042.
MutableModule.CompareTo compared _index to itself instead of other._index, so the comparison always returned 0. There are never more than 1 MutableModule in a compilation so this was never exercised.
ModuleAndIntValueKey.Equals could dereference a null Module. In practice this was never hit.
IsInheritanceChainLayoutFixedInCurrentVersionBubble had a null check after the dereference in its while condition. We can remove the dead check and assert that BaseType is non-null while walking to System.Object.
PropertyPseudoDesc/EventPseudoDesc operator== dereferenced operand fields without a null guard; null operands now compare correctly instead of throwing.
Removed the unreachable MethodDictionary/TypeDictionary arms of GenericLookupHelper. The only caller never produces those helper IDs, so they don't need to be handled. They now fall through to the existing NotImplementedException default.