From a9c69c93b9666e79f6c13afc0704f58a8197be1d Mon Sep 17 00:00:00 2001 From: Jan Kotas Date: Tue, 16 Jan 2024 16:44:05 -0800 Subject: [PATCH 1/6] Fix compareTypesForEquality Fixes #96876 --- src/coreclr/vm/jitinterface.cpp | 77 ++++++++++++++----- .../InteropServices/CollectionsMarshal.cs | 3 +- 2 files changed, 57 insertions(+), 23 deletions(-) diff --git a/src/coreclr/vm/jitinterface.cpp b/src/coreclr/vm/jitinterface.cpp index ca1dd982e1cbb..a9074578bfe7e 100644 --- a/src/coreclr/vm/jitinterface.cpp +++ b/src/coreclr/vm/jitinterface.cpp @@ -4441,6 +4441,59 @@ TypeCompareState CEEInfo::compareTypesForCast( return result; } +// Returns true if hnd1 and hnd2 are guaranteed to represent different types. Returns false if hnd1 and hnd2 may represent the same type. +static bool AreGuaranteedToRepresentDifferentTypes(TypeHandle hnd1, TypeHandle hnd2) +{ + STANDARD_VM_CONTRACT; + + CorElementType et1 = hnd1.GetSignatureCorElementType(); + CorElementType et2 = hnd2.GetSignatureCorElementType(); + + TypeHandle canonHnd = TypeHandle(g_pCanonMethodTableClass); + if ((hnd1 == canonHnd) || (hnd2 == canonHnd)) + { + return CorTypeInfo::IsObjRef(et1) != CorTypeInfo::IsObjRef(et2); + } + + if (et1 != et2) + { + return true; + } + + switch (et1) + { + case ELEMENT_TYPE_ARRAY: + if (hnd1.GetRank() != hnd2.GetRank()) + return true; + case ELEMENT_TYPE_SZARRAY: + case ELEMENT_TYPE_BYREF: + case ELEMENT_TYPE_PTR: + return AreGuaranteedToRepresentDifferentTypes(hnd1.GetTypeParam(), hnd2.GetTypeParam()); + + default: + if (!hnd1.IsTypeDesc()) + { + if (!hnd1.AsMethodTable()->HasSameTypeDefAs(hnd2.AsMethodTable())) + return true; + + if (hnd1.AsMethodTable()->HasInstantiation()) + { + Instantiation inst1 = hnd1.AsMethodTable()->GetInstantiation(); + Instantiation inst2 = hnd2.AsMethodTable()->GetInstantiation(); + + for (DWORD i = 0; i < inst1.GetNumArgs(); i++) + { + if (AreGuaranteedToRepresentDifferentTypes(inst1[i], inst2[i])) + return true; + } + } + } + break; + } + + return false; +} + /*********************************************************************/ // See if types represented by cls1 and cls2 compare equal, not // equal, or the comparison needs to be resolved at runtime. @@ -4466,30 +4519,12 @@ TypeCompareState CEEInfo::compareTypesForEquality( { result = (hnd1 == hnd2 ? TypeCompareState::Must : TypeCompareState::MustNot); } - // If either or both types are canonical subtypes, we can sometimes prove inequality. else { - // If either is a value type then the types cannot - // be equal unless the type defs are the same. - if (hnd1.IsValueType() || hnd2.IsValueType()) + // If either or both types are canonical subtypes, we can sometimes prove inequality. + if (AreGuaranteedToRepresentDifferentTypes(hnd1, hnd2)) { - if (!hnd1.GetMethodTable()->HasSameTypeDefAs(hnd2.GetMethodTable())) - { - result = TypeCompareState::MustNot; - } - } - // If we have two ref types that are not __Canon, then the - // types cannot be equal unless the type defs are the same. - else - { - TypeHandle canonHnd = TypeHandle(g_pCanonMethodTableClass); - if ((hnd1 != canonHnd) && (hnd2 != canonHnd)) - { - if (!hnd1.GetMethodTable()->HasSameTypeDefAs(hnd2.GetMethodTable())) - { - result = TypeCompareState::MustNot; - } - } + result = TypeCompareState::MustNot; } } diff --git a/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/CollectionsMarshal.cs b/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/CollectionsMarshal.cs index ec3d0f08c8e1e..e3cecd684abf6 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/CollectionsMarshal.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/CollectionsMarshal.cs @@ -34,8 +34,7 @@ public static Span AsSpan(List? list) ThrowHelper.ThrowInvalidOperationException_ConcurrentOperationsNotSupported(); } - // Commented out to workaround https://github.com/dotnet/runtime/issues/96876 - // Debug.Assert(typeof(T[]) == list._items.GetType(), "Implementation depends on List always using a T[] and not U[] where U : T."); + Debug.Assert(typeof(T[]) == list._items.GetType(), "Implementation depends on List always using a T[] and not U[] where U : T."); span = new Span(ref MemoryMarshal.GetArrayDataReference(items), size); } From e6214490259b6cae31ceec3a5b29283809d3d25e Mon Sep 17 00:00:00 2001 From: Jan Kotas Date: Tue, 16 Jan 2024 20:07:49 -0800 Subject: [PATCH 2/6] Fix build break --- src/coreclr/vm/jitinterface.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/coreclr/vm/jitinterface.cpp b/src/coreclr/vm/jitinterface.cpp index a9074578bfe7e..99f37a749e127 100644 --- a/src/coreclr/vm/jitinterface.cpp +++ b/src/coreclr/vm/jitinterface.cpp @@ -4465,6 +4465,7 @@ static bool AreGuaranteedToRepresentDifferentTypes(TypeHandle hnd1, TypeHandle h case ELEMENT_TYPE_ARRAY: if (hnd1.GetRank() != hnd2.GetRank()) return true; + return AreGuaranteedToRepresentDifferentTypes(hnd1.GetTypeParam(), hnd2.GetTypeParam()); case ELEMENT_TYPE_SZARRAY: case ELEMENT_TYPE_BYREF: case ELEMENT_TYPE_PTR: @@ -4480,6 +4481,7 @@ static bool AreGuaranteedToRepresentDifferentTypes(TypeHandle hnd1, TypeHandle h { Instantiation inst1 = hnd1.AsMethodTable()->GetInstantiation(); Instantiation inst2 = hnd2.AsMethodTable()->GetInstantiation(); + _ASSERTE(inst1.GetNumArgs() == inst2.GetNumArgs()); for (DWORD i = 0; i < inst1.GetNumArgs(); i++) { From e00f6b130ac88ea586bca00023d411991a89a1a4 Mon Sep 17 00:00:00 2001 From: Jan Kotas Date: Wed, 17 Jan 2024 11:36:40 -0800 Subject: [PATCH 3/6] Add test --- .../coreclr/GitHub_96876/test96876.cs | 20 +++++++++++++++++++ .../coreclr/GitHub_96876/test96876.csproj | 12 +++++++++++ 2 files changed, 32 insertions(+) create mode 100644 src/tests/Regressions/coreclr/GitHub_96876/test96876.cs create mode 100644 src/tests/Regressions/coreclr/GitHub_96876/test96876.csproj diff --git a/src/tests/Regressions/coreclr/GitHub_96876/test96876.cs b/src/tests/Regressions/coreclr/GitHub_96876/test96876.cs new file mode 100644 index 0000000000000..d09e5f04b100b --- /dev/null +++ b/src/tests/Regressions/coreclr/GitHub_96876/test96876.cs @@ -0,0 +1,20 @@ +// 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.Runtime.CompilerServices; +using Xunit; + +public class Test96876 +{ + [Fact] + public static void TestEntryPoint() + { + Assert.True(foo(new string[1])); + Assert.False(foo(new string[1])); + } + + // Validate that the type equality involving shared array types is handled correctly + // in shared generic code. + [MethodImpl(MethodImplOptions.NoInlining)] + static bool foo(string[] list) => typeof(T[]) == list.GetType(); +} diff --git a/src/tests/Regressions/coreclr/GitHub_96876/test96876.csproj b/src/tests/Regressions/coreclr/GitHub_96876/test96876.csproj new file mode 100644 index 0000000000000..e189796704d87 --- /dev/null +++ b/src/tests/Regressions/coreclr/GitHub_96876/test96876.csproj @@ -0,0 +1,12 @@ + + + + 0 + + + + + + + + From da7f0bdd0866d06612b6cb26e895330238cb0563 Mon Sep 17 00:00:00 2001 From: Jan Kotas Date: Wed, 17 Jan 2024 15:57:39 -0800 Subject: [PATCH 4/6] Fix managed type system implementation --- .../tools/Common/Compiler/TypeExtensions.cs | 73 +++++++++++++------ 1 file changed, 49 insertions(+), 24 deletions(-) diff --git a/src/coreclr/tools/Common/Compiler/TypeExtensions.cs b/src/coreclr/tools/Common/Compiler/TypeExtensions.cs index 04e8e65f61f3b..f16b643e0cbb4 100644 --- a/src/coreclr/tools/Common/Compiler/TypeExtensions.cs +++ b/src/coreclr/tools/Common/Compiler/TypeExtensions.cs @@ -182,43 +182,68 @@ public static bool IsArrayTypeWithoutGenericInterfaces(this TypeDesc type) public static bool? CompareTypesForEquality(TypeDesc type1, TypeDesc type2) { - bool? result = null; - // If neither type is a canonical subtype, type handle comparison suffices if (!type1.IsCanonicalSubtype(CanonicalFormKind.Any) && !type2.IsCanonicalSubtype(CanonicalFormKind.Any)) { - result = type1 == type2; + return type1 == type2; } + // If either or both types are canonical subtypes, we can sometimes prove inequality. - else + if (AreGuaranteedToRepresentDifferentTypes(type1, type2)) + { + return false; + } + + return null; + + static bool AreGuaranteedToRepresentDifferentTypes(TypeDesc type1, TypeDesc type2) { - // If either is a value type then the types cannot - // be equal unless the type defs are the same. - if (type1.IsValueType || type2.IsValueType) + if (type1.IsCanonicalDefinitionType(CanonicalFormKind.Any) || type2.IsCanonicalDefinitionType(CanonicalFormKind.Any)) { - if (!type1.IsCanonicalDefinitionType(CanonicalFormKind.Universal) && !type2.IsCanonicalDefinitionType(CanonicalFormKind.Universal)) - { - if (!type1.HasSameTypeDefinition(type2)) - { - result = false; - } - } + // Universal canonical definition can match any type. We can't prove inequality. + if (type1.IsCanonicalDefinitionType(CanonicalFormKind.Universal) || type2.IsCanonicalDefinitionType(CanonicalFormKind.Universal)) + return false; + + return type1.IsGCPointer != type2.IsGCPointer; } - // If we have two ref types that are not __Canon, then the - // types cannot be equal unless the type defs are the same. - else + + TypeFlags category = type1.Category; + if (category != type2.Category) + return true; + + switch (category) { - if (!type1.IsCanonicalDefinitionType(CanonicalFormKind.Any) && !type2.IsCanonicalDefinitionType(CanonicalFormKind.Any)) - { - if (!type1.HasSameTypeDefinition(type2)) + case TypeFlags.Array: + if (((ArrayType)type1).Rank != ((ArrayType)type2).Rank) + return true; + return AreGuaranteedToRepresentDifferentTypes(((ArrayType)type1).ElementType, ((ArrayType)type2).ElementType); + case TypeFlags.SzArray: + case TypeFlags.ByRef: + case TypeFlags.Pointer: + return AreGuaranteedToRepresentDifferentTypes(((ParameterizedType)type1).ParameterType, ((ParameterizedType)type2).ParameterType); + + default: + if (type1.IsDefType || type2.IsDefType) { - result = false; + if (!type1.HasSameTypeDefinition(type2)) + return true; + + Instantiation inst1 = type1.Instantiation; + if (inst1.Length != 0) + { + var inst2 = type2.Instantiation; + Debug.Assert(inst1.Length == inst2.Length); + for (int i = 0; i < inst1.Length; i++) + { + if (AreGuaranteedToRepresentDifferentTypes(inst1[i], inst2[i])) + return true; + } + } } - } + break; } + return false; } - - return result; } public static TypeDesc MergeTypesToCommonParent(TypeDesc ta, TypeDesc tb) From b9b0d667eb58af57b553d7ab1213f379008b60d2 Mon Sep 17 00:00:00 2001 From: Jan Kotas Date: Wed, 17 Jan 2024 19:37:58 -0800 Subject: [PATCH 5/6] Additional test --- src/tests/Regressions/coreclr/GitHub_96876/test96876.cs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/tests/Regressions/coreclr/GitHub_96876/test96876.cs b/src/tests/Regressions/coreclr/GitHub_96876/test96876.cs index d09e5f04b100b..3408854470b73 100644 --- a/src/tests/Regressions/coreclr/GitHub_96876/test96876.cs +++ b/src/tests/Regressions/coreclr/GitHub_96876/test96876.cs @@ -11,10 +11,16 @@ public static void TestEntryPoint() { Assert.True(foo(new string[1])); Assert.False(foo(new string[1])); + + Assert.True(foo2()); + Assert.False(foo2()); } // Validate that the type equality involving shared array types is handled correctly // in shared generic code. [MethodImpl(MethodImplOptions.NoInlining)] static bool foo(string[] list) => typeof(T[]) == list.GetType(); + + [MethodImpl(MethodImplOptions.NoInlining)] + static bool foo2() => typeof(T[]) == typeof(string[]); } From 50eb486a9f41d33b338de42b6bcfdf9038430fca Mon Sep 17 00:00:00 2001 From: Jan Kotas Date: Thu, 18 Jan 2024 10:39:03 -0800 Subject: [PATCH 6/6] Update src/tests/Regressions/coreclr/GitHub_96876/test96876.csproj --- src/tests/Regressions/coreclr/GitHub_96876/test96876.csproj | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/tests/Regressions/coreclr/GitHub_96876/test96876.csproj b/src/tests/Regressions/coreclr/GitHub_96876/test96876.csproj index e189796704d87..6eeaf93f848b5 100644 --- a/src/tests/Regressions/coreclr/GitHub_96876/test96876.csproj +++ b/src/tests/Regressions/coreclr/GitHub_96876/test96876.csproj @@ -1,7 +1,6 @@ - - 0 + 1