From 79c79d8c8c57eaf94c8d5b3b10f44923f8e3557e Mon Sep 17 00:00:00 2001 From: Elinor Fung Date: Thu, 8 Feb 2024 15:28:18 -0800 Subject: [PATCH 1/3] Fix polluted CompareState when comparing element types in a signature We were propagating state from each type in the method signature to the comparisons for the next type. This resulted in type equivalence checks being disabled for any parameters that came after a generic parameter. --- src/coreclr/vm/siginfo.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/coreclr/vm/siginfo.cpp b/src/coreclr/vm/siginfo.cpp index 719073867caa63..115b5454bf5943 100644 --- a/src/coreclr/vm/siginfo.cpp +++ b/src/coreclr/vm/siginfo.cpp @@ -4201,8 +4201,6 @@ MetaSig::CompareTypeDefsUnderSubstitutions( SigPointer inst1 = pSubst1->GetInst(); SigPointer inst2 = pSubst2->GetInst(); - TokenPairList visited { pVisited }; - CompareState state{ &visited }; for (DWORD i = 0; i < pTypeDef1->GetNumGenericArgs(); i++) { PCCOR_SIGNATURE startInst1 = inst1.GetPtr(); @@ -4211,6 +4209,8 @@ MetaSig::CompareTypeDefsUnderSubstitutions( PCCOR_SIGNATURE startInst2 = inst2.GetPtr(); IfFailThrow(inst2.SkipExactlyOne()); PCCOR_SIGNATURE endInst2ptr = inst2.GetPtr(); + TokenPairList visited{ pVisited }; + CompareState state{ &visited }; if (!CompareElementType( startInst1, startInst2, @@ -4379,8 +4379,6 @@ MetaSig::CompareMethodSigs( IfFailThrow(CorSigUncompressData_EndPtr(pSig1, pEndSig1, &ArgCount1)); IfFailThrow(CorSigUncompressData_EndPtr(pSig2, pEndSig2, &ArgCount2)); - TokenPairList visited{ pVisited }; - if (ArgCount1 != ArgCount2) { if ((callConv & IMAGE_CEE_CS_CALLCONV_MASK) != IMAGE_CEE_CS_CALLCONV_VARARG) @@ -4402,7 +4400,6 @@ MetaSig::CompareMethodSigs( // to correctly handle overloads, where there are a number of varargs methods // to pick from, like m1(int,...) and m2(int,int,...), etc. - CompareState state{ &visited }; // <= because we want to include a check of the return value! for (i = 0; i <= ArgCount1; i++) { @@ -4434,6 +4431,8 @@ MetaSig::CompareMethodSigs( else { // We are in bounds on both sides. Compare the element. + TokenPairList visited{ pVisited }; + CompareState state{ &visited }; if (!CompareElementType( pSig1, pSig2, @@ -4458,7 +4457,6 @@ MetaSig::CompareMethodSigs( } // do return type as well - CompareState state{ &visited }; for (i = 0; i <= ArgCount1; i++) { if (i == 0 && skipReturnTypeSig) @@ -4473,6 +4471,8 @@ MetaSig::CompareMethodSigs( } else { + TokenPairList visited{ pVisited }; + CompareState state{ &visited }; if (!CompareElementType( pSig1, pSig2, From 32618365e6f98f38d301bc01e9e345be1bdb23a4 Mon Sep 17 00:00:00 2001 From: Elinor Fung Date: Thu, 8 Feb 2024 20:31:05 -0800 Subject: [PATCH 2/3] Add test --- .../typeequivalence/impl/Impls.cs | 12 +++++++++++ .../typeequivalence/simple/Simple.cs | 20 ++++++++++++++++++- 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/src/tests/baseservices/typeequivalence/impl/Impls.cs b/src/tests/baseservices/typeequivalence/impl/Impls.cs index f3472ed1076300..c1a16fc215c815 100644 --- a/src/tests/baseservices/typeequivalence/impl/Impls.cs +++ b/src/tests/baseservices/typeequivalence/impl/Impls.cs @@ -135,3 +135,15 @@ public static class TestsExactTypeOptimizationsHelper { public static TestValueType[] s_arrayInstance; } + +public static class MethodCall +{ + // Include a generic type in the method signature before the type using type equivalence to ensure that + // processing of the generic type does not affect subsequent type processing during signature comparison. + public static System.Collections.Generic.List InterfaceAfterGeneric(IEmptyType t) => null; + public static System.Collections.Generic.List ValueTypeAfterGeneric(TestValueType t) => null; + + // Generic type after the type using type equivalence should also not affect processing. + public static void InterfaceBeforeGeneric(IEmptyType t, System.Collections.Generic.List l) { } + public static void ValueTypeBeforeGeneric(TestValueType t, System.Collections.Generic.List l) { } +} \ No newline at end of file diff --git a/src/tests/baseservices/typeequivalence/simple/Simple.cs b/src/tests/baseservices/typeequivalence/simple/Simple.cs index afe4ba62777680..ecd9bbcb637f4e 100644 --- a/src/tests/baseservices/typeequivalence/simple/Simple.cs +++ b/src/tests/baseservices/typeequivalence/simple/Simple.cs @@ -262,7 +262,7 @@ private static unsafe void TestTypeEquivalenceWithTypePunning() } [MethodImpl (MethodImplOptions.NoInlining)] - private static void TestLoadingValueTypesWithMethod() + private static void TestLoadingValueTypesWithMethod() { Console.WriteLine($"{nameof(TestLoadingValueTypesWithMethod)}"); Console.WriteLine($"-- {typeof(ValueTypeWithStaticMethod).Name}"); @@ -293,6 +293,23 @@ private static void TestsExactTypeOptimizations() Assert.True(typeof(TestValueType[]) == TestsExactTypeOptimizationsHelper.s_arrayInstance.GetType()); } + private static void MethodCallSignature() + { + Console.WriteLine($"{nameof(MethodCallSignature)}"); + + Console.WriteLine($"-- {nameof(MethodCall.InterfaceAfterGeneric)}"); + MethodCall.InterfaceAfterGeneric((IEmptyType)EmptyType2.Create()); + + Console.WriteLine($"-- {nameof(MethodCall.ValueTypeAfterGeneric)}"); + MethodCall.ValueTypeAfterGeneric(new TestValueType()); + + Console.WriteLine($"-- {nameof(MethodCall.InterfaceBeforeGeneric)}"); + MethodCall.InterfaceBeforeGeneric((IEmptyType)EmptyType2.Create(), null); + + Console.WriteLine($"-- {nameof(MethodCall.ValueTypeBeforeGeneric)}"); + MethodCall.ValueTypeBeforeGeneric(new TestValueType(), null); + } + [Fact] public static int TestEntryPoint() { @@ -314,6 +331,7 @@ public static int TestEntryPoint() TestLoadingValueTypesWithMethod(); TestCastsOptimizations(); TestsExactTypeOptimizations(); + MethodCallSignature(); } catch (Exception e) { From 94ee677a77078c5bcde8ed325a4ec5781f7023bc Mon Sep 17 00:00:00 2001 From: Elinor Fung Date: Fri, 9 Feb 2024 09:21:49 -0800 Subject: [PATCH 3/3] Switch test methods to separate facts --- .../typeequivalence/simple/Simple.cs | 86 ++++++++----------- 1 file changed, 34 insertions(+), 52 deletions(-) diff --git a/src/tests/baseservices/typeequivalence/simple/Simple.cs b/src/tests/baseservices/typeequivalence/simple/Simple.cs index ecd9bbcb637f4e..e937edc5c3076d 100644 --- a/src/tests/baseservices/typeequivalence/simple/Simple.cs +++ b/src/tests/baseservices/typeequivalence/simple/Simple.cs @@ -18,6 +18,7 @@ public struct EquivalentValueType public int A; } +[PlatformSpecific(TestPlatforms.Windows)] public class Simple { private class EmptyType2 : IEmptyType @@ -31,7 +32,8 @@ public static object Create() } } - private static void InterfaceTypesFromDifferentAssembliesAreEquivalent() + [Fact] + public static void InterfaceTypesFromDifferentAssembliesAreEquivalent() { Console.WriteLine($"{nameof(InterfaceTypesFromDifferentAssembliesAreEquivalent)}"); var inAsm = EmptyType.Create(); @@ -45,9 +47,10 @@ void AreNotSameObject(IEmptyType a, IEmptyType b) } } - private static void ValidateTypeInstanceEquality() + [Fact] + public static void TypeInstanceEquality() { - Console.WriteLine($"{nameof(ValidateTypeInstanceEquality)}"); + Console.WriteLine($"{nameof(TypeInstanceEquality)}"); var inAsm = EmptyType.Create(); var otherAsm = EmptyType2.Create(); @@ -111,7 +114,8 @@ public override string ScaleString(string s) } } - private static void InterfaceTypesMethodOperations() + [Fact] + public static void InterfaceTypesMethodOperations() { Console.WriteLine($"{nameof(InterfaceTypesMethodOperations)}"); @@ -142,7 +146,8 @@ private static void InterfaceTypesMethodOperations() } } - private static void CallSparseInterface() + [Fact] + public static void CallSparseInterface() { Console.WriteLine($"{nameof(CallSparseInterface)}"); @@ -157,9 +162,10 @@ private static void CallSparseInterface() Assert.Equal(input * 18, sparseType.MultiplyBy18(input)); } - private static void TestArrayEquivalence() + [Fact] + public static void ArrayEquivalence() { - Console.WriteLine($"{nameof(TestArrayEquivalence)}"); + Console.WriteLine($"{nameof(ArrayEquivalence)}"); var inAsm = EmptyType.Create(); var otherAsm = EmptyType2.Create(); @@ -174,9 +180,10 @@ private static void TestArrayEquivalence() Assert.False(inAsmInterfaceType.MakeArrayType(1).IsEquivalentTo(otherAsmInterfaceType.MakeArrayType(2))); } - private static void TestByRefEquivalence() + [Fact] + public static void ByRefEquivalence() { - Console.WriteLine($"{nameof(TestByRefEquivalence)}"); + Console.WriteLine($"{nameof(ByRefEquivalence)}"); var inAsm = EmptyType.Create(); var otherAsm = EmptyType2.Create(); @@ -198,9 +205,10 @@ public void Method(V input) } } - private static void TestGenericClassNonEquivalence() + [Fact] + public static void GenericClassNonEquivalence() { - Console.WriteLine($"{nameof(TestGenericClassNonEquivalence)}"); + Console.WriteLine($"{nameof(GenericClassNonEquivalence)}"); var inAsm = EmptyType.Create(); var otherAsm = EmptyType2.Create(); @@ -210,9 +218,10 @@ private static void TestGenericClassNonEquivalence() Assert.False(typeof(Generic<>).MakeGenericType(inAsmInterfaceType).IsEquivalentTo(typeof(Generic<>).MakeGenericType(otherAsmInterfaceType))); } - private static void TestGenericInterfaceEquivalence() + [Fact] + public static void GenericInterfaceEquivalence() { - Console.WriteLine($"{nameof(TestGenericInterfaceEquivalence)}"); + Console.WriteLine($"{nameof(GenericInterfaceEquivalence)}"); var inAsm = EmptyType.Create(); var otherAsm = EmptyType2.Create(); @@ -222,9 +231,10 @@ private static void TestGenericInterfaceEquivalence() Assert.True(typeof(IGeneric<>).MakeGenericType(inAsmInterfaceType).IsEquivalentTo(typeof(IGeneric<>).MakeGenericType(otherAsmInterfaceType))); } - private static unsafe void TestTypeEquivalenceWithTypePunning() + [Fact] + public static unsafe void TypeEquivalenceWithTypePunning() { - Console.WriteLine($"{nameof(TestTypeEquivalenceWithTypePunning)}"); + Console.WriteLine($"{nameof(TypeEquivalenceWithTypePunning)}"); { Console.WriteLine($"-- GetFunctionPointer()"); @@ -261,10 +271,11 @@ private static unsafe void TestTypeEquivalenceWithTypePunning() } } + [Fact] [MethodImpl (MethodImplOptions.NoInlining)] - private static void TestLoadingValueTypesWithMethod() + public static void LoadValueTypesWithMethod() { - Console.WriteLine($"{nameof(TestLoadingValueTypesWithMethod)}"); + Console.WriteLine($"{nameof(LoadValueTypesWithMethod)}"); Console.WriteLine($"-- {typeof(ValueTypeWithStaticMethod).Name}"); Assert.Throws(() => LoadInvalidType()); } @@ -275,7 +286,8 @@ private static void LoadInvalidType() Console.WriteLine($"-- {typeof(ValueTypeWithInstanceMethod).Name}"); } - private static void TestCastsOptimizations() + [Fact] + public static void CastsOptimizations() { string otherTypeName = $"{typeof(EquivalentValueType).FullName},{typeof(EmptyType).Assembly.GetName().Name}"; Type otherEquivalentValueType = Type.GetType(otherTypeName); @@ -286,14 +298,16 @@ private static void TestCastsOptimizations() EquivalentValueType inst = (EquivalentValueType)otherEquivalentValueTypeInstance; } - private static void TestsExactTypeOptimizations() + [Fact] + public static void ExactTypeOptimizations() { TestsExactTypeOptimizationsHelper.s_arrayInstance = new TestValueType[1]; Thread.Yield(); Assert.True(typeof(TestValueType[]) == TestsExactTypeOptimizationsHelper.s_arrayInstance.GetType()); } - private static void MethodCallSignature() + [Fact] + public static void MethodCallSignature() { Console.WriteLine($"{nameof(MethodCallSignature)}"); @@ -309,36 +323,4 @@ private static void MethodCallSignature() Console.WriteLine($"-- {nameof(MethodCall.ValueTypeBeforeGeneric)}"); MethodCall.ValueTypeBeforeGeneric(new TestValueType(), null); } - - [Fact] - public static int TestEntryPoint() - { - if (!OperatingSystem.IsWindows()) - { - return 100; - } - try - { - InterfaceTypesFromDifferentAssembliesAreEquivalent(); - ValidateTypeInstanceEquality(); - InterfaceTypesMethodOperations(); - CallSparseInterface(); - TestByRefEquivalence(); - TestArrayEquivalence(); - TestGenericClassNonEquivalence(); - TestGenericInterfaceEquivalence(); - TestTypeEquivalenceWithTypePunning(); - TestLoadingValueTypesWithMethod(); - TestCastsOptimizations(); - TestsExactTypeOptimizations(); - MethodCallSignature(); - } - catch (Exception e) - { - Console.WriteLine($"Test Failure: {e}"); - return 101; - } - - return 100; - } }