Skip to content

Commit

Permalink
Fix polluted CompareState when comparing element types in a signature (…
Browse files Browse the repository at this point in the history 
…#98249)

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.
  • Loading branch information
@elinor-fung
elinor-fung authored Feb 12, 2024
1 parent ba68cf1 commit 6dec8dd
Show file tree
Hide file tree
Showing 4 changed files with 68 additions and 49 deletions.
12 changes: 6 additions & 6 deletions src/coreclr/vm/siginfo.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4204,8 +4204,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();
Expand All @@ -4214,6 +4212,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,
Expand Down Expand Up @@ -4382,8 +4382,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)
Expand All @@ -4405,7 +4403,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++)
{
Expand Down Expand Up @@ -4437,6 +4434,8 @@ MetaSig::CompareMethodSigs(
else
{
// We are in bounds on both sides. Compare the element.
TokenPairList visited{ pVisited };
CompareState state{ &visited };
if (!CompareElementType(
pSig1,
pSig2,
Expand All @@ -4461,7 +4460,6 @@ MetaSig::CompareMethodSigs(
}

// do return type as well
CompareState state{ &visited };
for (i = 0; i <= ArgCount1; i++)
{
if (i == 0 && skipReturnTypeSig)
Expand All @@ -4476,6 +4474,8 @@ MetaSig::CompareMethodSigs(
}
else
{
TokenPairList visited{ pVisited };
CompareState state{ &visited };
if (!CompareElementType(
pSig1,
pSig2,
Expand Down
6 changes: 6 additions & 0 deletions src/tests/baseservices/typeequivalence/contracts/Types.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,12 @@ public interface ISparseType
int MultiplyBy20(int a);
}

[Guid("BD752276-52DF-4CD1-8C62-49D202F15C8D")]
public struct TestValueType
{
public int Field;
}

//
// Below types are used in type punning tests and shouldn't be used anywhere else.
//
Expand Down
12 changes: 12 additions & 0 deletions src/tests/baseservices/typeequivalence/impl/Impls.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -130,3 +130,15 @@ public static int GetField_3(OnlyLoadOnce_3 s)
return s.Field;
}
}

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<int> InterfaceAfterGeneric(IEmptyType t) => null;
public static System.Collections.Generic.List<int> 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<int> l) { }
public static void ValueTypeBeforeGeneric(TestValueType t, System.Collections.Generic.List<int> l) { }
}
87 changes: 44 additions & 43 deletions src/tests/baseservices/typeequivalence/simple/Simple.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ public struct EquivalentValueType
public int A;
}

[PlatformSpecific(TestPlatforms.Windows)]
public class Simple
{
private class EmptyType2 : IEmptyType
Expand All @@ -30,7 +31,8 @@ public static object Create()
}
}

private static void InterfaceTypesFromDifferentAssembliesAreEquivalent()
[Fact]
public static void InterfaceTypesFromDifferentAssembliesAreEquivalent()
{
Console.WriteLine($"{nameof(InterfaceTypesFromDifferentAssembliesAreEquivalent)}");
var inAsm = EmptyType.Create();
Expand All @@ -44,9 +46,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();

Expand Down Expand Up @@ -110,7 +113,8 @@ public override string ScaleString(string s)
}
}

private static void InterfaceTypesMethodOperations()
[Fact]
public static void InterfaceTypesMethodOperations()
{
Console.WriteLine($"{nameof(InterfaceTypesMethodOperations)}");

Expand Down Expand Up @@ -141,7 +145,8 @@ private static void InterfaceTypesMethodOperations()
}
}

private static void CallSparseInterface()
[Fact]
public static void CallSparseInterface()
{
Console.WriteLine($"{nameof(CallSparseInterface)}");

Expand All @@ -156,9 +161,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();

Expand All @@ -173,9 +179,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();

Expand All @@ -197,9 +204,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();

Expand All @@ -209,9 +217,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();

Expand All @@ -221,9 +230,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()");
Expand Down Expand Up @@ -260,10 +270,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<TypeLoadException>(() => LoadInvalidType());
}
Expand All @@ -274,7 +285,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);
Expand All @@ -285,32 +297,21 @@ private static void TestCastsOptimizations()
EquivalentValueType inst = (EquivalentValueType)otherEquivalentValueTypeInstance;
}

public static int Main()
[Fact]
public static void MethodCallSignature()
{
if (!OperatingSystem.IsWindows())
{
return 100;
}
try
{
InterfaceTypesFromDifferentAssembliesAreEquivalent();
ValidateTypeInstanceEquality();
InterfaceTypesMethodOperations();
CallSparseInterface();
TestByRefEquivalence();
TestArrayEquivalence();
TestGenericClassNonEquivalence();
TestGenericInterfaceEquivalence();
TestTypeEquivalenceWithTypePunning();
TestLoadingValueTypesWithMethod();
TestCastsOptimizations();
}
catch (Exception e)
{
Console.WriteLine($"Test Failure: {e}");
return 101;
}
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);

return 100;
Console.WriteLine($"-- {nameof(MethodCall.ValueTypeBeforeGeneric)}");
MethodCall.ValueTypeBeforeGeneric(new TestValueType(), null);
}
}

0 comments on commit 6dec8dd

Please sign in to comment.