From 25895380104a3b3ed7eb2357ffdf586133e07052 Mon Sep 17 00:00:00 2001 From: Jackson Schuster <36744439+jtschuster@users.noreply.github.com> Date: Tue, 16 Jun 2026 16:27:39 -0700 Subject: [PATCH 1/2] Fix crossgen2 R2R issues found by static analysis 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> --- .../tools/Common/Compiler/EventPseudoDesc.cs | 11 ++++++++++- .../tools/Common/Compiler/PropertyPseudoDesc.cs | 11 ++++++++++- .../ReadyToRunCodegenNodeFactory.cs | 2 +- .../ReadyToRunSymbolNodeFactory.cs | 14 -------------- .../Compiler/ReadyToRunCodegenCompilation.cs | 3 ++- .../TypeSystem/Mutable/MutableModule.Sorting.cs | 2 +- 6 files changed, 24 insertions(+), 19 deletions(-) diff --git a/src/coreclr/tools/Common/Compiler/EventPseudoDesc.cs b/src/coreclr/tools/Common/Compiler/EventPseudoDesc.cs index 287d86cdee7687..02d0cbe811ca7e 100644 --- a/src/coreclr/tools/Common/Compiler/EventPseudoDesc.cs +++ b/src/coreclr/tools/Common/Compiler/EventPseudoDesc.cs @@ -93,7 +93,16 @@ public EventPseudoDesc(EcmaType type, EventDefinitionHandle handle) public override string ToString() => $"{_type}.{Name}"; - public static bool operator ==(EventPseudoDesc a, EventPseudoDesc b) => a._type == b._type && a._handle == b._handle; + public static bool operator ==(EventPseudoDesc a, EventPseudoDesc b) + { + if (a is null) + return b is null; + + if (b is null) + return false; + + return a._type == b._type && a._handle == b._handle; + } public static bool operator !=(EventPseudoDesc a, EventPseudoDesc b) => !(a == b); } diff --git a/src/coreclr/tools/Common/Compiler/PropertyPseudoDesc.cs b/src/coreclr/tools/Common/Compiler/PropertyPseudoDesc.cs index 2ceb2d9eda75de..af03919dc4c48b 100644 --- a/src/coreclr/tools/Common/Compiler/PropertyPseudoDesc.cs +++ b/src/coreclr/tools/Common/Compiler/PropertyPseudoDesc.cs @@ -88,7 +88,16 @@ public PropertyPseudoDesc(EcmaType type, PropertyDefinitionHandle handle) public override string ToString() => $"{_type}.{Name}"; - public static bool operator ==(PropertyPseudoDesc a, PropertyPseudoDesc b) => a._type == b._type && a._handle == b._handle; + public static bool operator ==(PropertyPseudoDesc a, PropertyPseudoDesc b) + { + if (a is null) + return b is null; + + if (b is null) + return false; + + return a._type == b._type && a._handle == b._handle; + } public static bool operator !=(PropertyPseudoDesc a, PropertyPseudoDesc b) => !(a == b); } diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunCodegenNodeFactory.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunCodegenNodeFactory.cs index accf88bec29fce..dd1f5be2ce5547 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunCodegenNodeFactory.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunCodegenNodeFactory.cs @@ -208,7 +208,7 @@ public ModuleAndIntValueKey(int integer, EcmaModule module) Module = module; } - public bool Equals(ModuleAndIntValueKey other) => IntValue == other.IntValue && ((Module == null && other.Module == null) || Module.Equals(other.Module)); + public bool Equals(ModuleAndIntValueKey other) => IntValue == other.IntValue && (Module == null ? other.Module == null : Module.Equals(other.Module)); public override bool Equals(object obj) => obj is ModuleAndIntValueKey && Equals((ModuleAndIntValueKey)obj); public override int GetHashCode() { diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunSymbolNodeFactory.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunSymbolNodeFactory.cs index d0ca5431be42d0..41a70f79c87f65 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunSymbolNodeFactory.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunSymbolNodeFactory.cs @@ -633,20 +633,6 @@ public Import GenericLookupHelper( (MethodWithToken)helperArgument, methodContext); - case ReadyToRunHelperId.MethodDictionary: - return GenericLookupMethodHelper( - runtimeLookupKind, - ReadyToRunFixupKind.MethodHandle, - (MethodWithToken)helperArgument, - methodContext); - - case ReadyToRunHelperId.TypeDictionary: - return GenericLookupTypeHelper( - runtimeLookupKind, - ReadyToRunFixupKind.TypeDictionary, - (TypeDesc)helperArgument, - methodContext); - case ReadyToRunHelperId.VirtualDispatchCell: return GenericLookupMethodHelper( runtimeLookupKind, diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCodegenCompilation.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCodegenCompilation.cs index 547458296fd024..90215c273ff83e 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCodegenCompilation.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCodegenCompilation.cs @@ -655,13 +655,14 @@ public bool IsInheritanceChainLayoutFixedInCurrentVersionBubble(TypeDesc type) if (CompilationModuleGroup.TypeLayoutCompilationUnits(type).HasMultipleInexactCompilationUnits) return false; - while (!type.IsObject && type != null) + while (!type.IsObject) { if (!IsLayoutFixedInCurrentVersionBubble(type)) { return false; } type = type.BaseType; + Debug.Assert(type != null); } } diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/TypeSystem/Mutable/MutableModule.Sorting.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/TypeSystem/Mutable/MutableModule.Sorting.cs index e6676ff7d0a2d3..2d47d7e7e59a60 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/TypeSystem/Mutable/MutableModule.Sorting.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/TypeSystem/Mutable/MutableModule.Sorting.cs @@ -16,7 +16,7 @@ partial class MutableModule int _index = Interlocked.Increment(ref s_globalIndex); public int CompareTo(MutableModule other) { - return _index.CompareTo(_index); + return _index.CompareTo(other._index); } } } From c1f3c04537b1a89b4b96aa1a570bc4b3bd0ab805 Mon Sep 17 00:00:00 2001 From: Jackson Schuster <36744439+jtschuster@users.noreply.github.com> Date: Thu, 18 Jun 2026 10:15:32 -0700 Subject: [PATCH 2/2] Address review feedback - 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> --- .../ReadyToRunCodegenNodeFactory.cs | 2 +- .../TypeSystem/Mutable/MutableModule.Sorting.cs | 11 +++-------- 2 files changed, 4 insertions(+), 9 deletions(-) diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunCodegenNodeFactory.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunCodegenNodeFactory.cs index dd1f5be2ce5547..5a5a45b5240c38 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunCodegenNodeFactory.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunCodegenNodeFactory.cs @@ -208,7 +208,7 @@ public ModuleAndIntValueKey(int integer, EcmaModule module) Module = module; } - public bool Equals(ModuleAndIntValueKey other) => IntValue == other.IntValue && (Module == null ? other.Module == null : Module.Equals(other.Module)); + public bool Equals(ModuleAndIntValueKey other) => IntValue == other.IntValue && Module == other.Module; public override bool Equals(object obj) => obj is ModuleAndIntValueKey && Equals((ModuleAndIntValueKey)obj); public override int GetHashCode() { diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/TypeSystem/Mutable/MutableModule.Sorting.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/TypeSystem/Mutable/MutableModule.Sorting.cs index 2d47d7e7e59a60..28c1823b5d4ec7 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/TypeSystem/Mutable/MutableModule.Sorting.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/TypeSystem/Mutable/MutableModule.Sorting.cs @@ -1,22 +1,17 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -using System; -using System.Threading; - using Debug = System.Diagnostics.Debug; namespace Internal.TypeSystem.Ecma { partial class MutableModule { - // This isn't deterministic, but it is functional. At this time, since only 1 MutableModule is contained in a build, it will be deterministic as it will always return 0. - static int s_globalIndex = 0; - - int _index = Interlocked.Increment(ref s_globalIndex); + // There is only ever a single MutableModule in a build, so any comparison is against itself. public int CompareTo(MutableModule other) { - return _index.CompareTo(other._index); + Debug.Assert(this == other); + return 0; } } }