From cab9b02d9d72a9b41af5a2def0799674d3dc9584 Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Fri, 27 Nov 2020 12:26:12 +0100 Subject: [PATCH 1/3] Fix covariant returns with generic return types When a covariant return type was an uninstantiated generic type, the ClassLoader::IsCompatibleWith was not working properly. In debug builds, it was asserting because there was no MethodTable for that type and in release builds, it resulted in ExecutionEngineException or an internal CLR error. This change fixes it by using TypeHandle::CanCastTo instead of MethodTable::CanCastTo and adds a regression test for two cases where the problem was observed (Assembly.GetTypes() and creating an instance of a type with a covariant return with a problematic kind of type). --- src/coreclr/src/vm/class.cpp | 20 ++++----- .../coreclr/GitHub_45037/test45037.cs | 43 +++++++++++++++++++ .../coreclr/GitHub_45037/test45037.csproj | 10 +++++ 3 files changed, 62 insertions(+), 11 deletions(-) create mode 100644 src/tests/Regressions/coreclr/GitHub_45037/test45037.cs create mode 100644 src/tests/Regressions/coreclr/GitHub_45037/test45037.csproj diff --git a/src/coreclr/src/vm/class.cpp b/src/coreclr/src/vm/class.cpp index 43586ccc719b62..d7af8668c7eba2 100644 --- a/src/coreclr/src/vm/class.cpp +++ b/src/coreclr/src/vm/class.cpp @@ -1109,20 +1109,18 @@ bool ClassLoader::IsCompatibleWith(TypeHandle hType1, TypeHandle hType2) return false; } - _ASSERTE(hType1.GetMethodTable() != NULL); - _ASSERTE(hType2.GetMethodTable() != NULL); - - // Nullable can be cast to T, but this is not compatible according to ECMA I.8.7.1 - bool isCastFromNullableOfTtoT = hType1.GetMethodTable()->IsNullable() && hType2.IsEquivalentTo(hType1.GetMethodTable()->GetInstantiation()[0]); - if (isCastFromNullableOfTtoT) + MethodTable* pMT1 = hType1.GetMethodTable(); + if (pMT1 != NULL) { - return false; + // Nullable can be cast to T, but this is not compatible according to ECMA I.8.7.1 + bool isCastFromNullableOfTtoT = pMT1->IsNullable() && hType2.IsEquivalentTo(pMT1->GetInstantiation()[0]); + if (isCastFromNullableOfTtoT) + { + return false; + } } - { - GCX_COOP(); - return hType2.GetMethodTable()->CanCastTo(hType1.GetMethodTable(), NULL); - } + return hType2.CanCastTo(hType1, NULL); } /*static*/ diff --git a/src/tests/Regressions/coreclr/GitHub_45037/test45037.cs b/src/tests/Regressions/coreclr/GitHub_45037/test45037.cs new file mode 100644 index 00000000000000..99e310ee0e153c --- /dev/null +++ b/src/tests/Regressions/coreclr/GitHub_45037/test45037.cs @@ -0,0 +1,43 @@ +using System; +using System.Reflection; + + +public abstract class ABase +{ + public abstract object this[int index] { get; } +} + +public sealed class Concrete : ABase + where T : class +{ + public override T this[int index] + { + get + { + throw null; + } + } +} + +class Parent +{ + public virtual object Value { get; } +} + +class Child : Parent where T : class +{ + public override T Value { get => (T)base.Value; } +} + +class Foo { } + +class Program +{ + static int Main() + { + Type[] t = Assembly.GetExecutingAssembly().GetTypes(); + new Child(); + + return 100; + } +} diff --git a/src/tests/Regressions/coreclr/GitHub_45037/test45037.csproj b/src/tests/Regressions/coreclr/GitHub_45037/test45037.csproj new file mode 100644 index 00000000000000..4a1f50c8346cf6 --- /dev/null +++ b/src/tests/Regressions/coreclr/GitHub_45037/test45037.csproj @@ -0,0 +1,10 @@ + + + Exe + BuildAndRun + 1 + + + + + From aec401179099d5735ec03cacbb61f23427cf3e2b Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Tue, 1 Dec 2020 15:29:53 +0100 Subject: [PATCH 2/3] Fix issue 45082 too There were two issues. First, the ClassLoader::ValidateMethodsWithCovariantReturnTypes was called before typeHnd.DoFullyLoad and that resulted in an assert down the call chain of TypeDesc::CanCastTo due to a wrong load level. Second, the SigTypeContext generation for the current MD in the ClassLoader::ValidateMethodsWithCovariantReturnTypes requires the same change for class instantiation as the one that we had for the parent MD. --- src/coreclr/src/vm/class.cpp | 15 +++++++---- src/coreclr/src/vm/clsload.cpp | 9 +++---- .../coreclr/GitHub_45037/test45037.cs | 13 +++++++++- .../coreclr/GitHub_45082/test45082.cs | 26 +++++++++++++++++++ .../coreclr/GitHub_45082/test45082.csproj | 10 +++++++ 5 files changed, 62 insertions(+), 11 deletions(-) create mode 100644 src/tests/Regressions/coreclr/GitHub_45082/test45082.cs create mode 100644 src/tests/Regressions/coreclr/GitHub_45082/test45082.csproj diff --git a/src/coreclr/src/vm/class.cpp b/src/coreclr/src/vm/class.cpp index d7af8668c7eba2..6baa328700702e 100644 --- a/src/coreclr/src/vm/class.cpp +++ b/src/coreclr/src/vm/class.cpp @@ -1169,16 +1169,21 @@ void ClassLoader::ValidateMethodsWithCovariantReturnTypes(MethodTable* pMT) if (!pMD->RequiresCovariantReturnTypeChecking() && !pParentMD->RequiresCovariantReturnTypeChecking()) continue; - Instantiation classInst = pParentMD->GetClassInstantiation(); - if (ClassLoader::IsTypicalSharedInstantiation(classInst)) + Instantiation parentClassInst = pParentMD->GetClassInstantiation(); + if (ClassLoader::IsTypicalSharedInstantiation(parentClassInst)) { - classInst = pParentMT->GetInstantiation(); + parentClassInst = pParentMT->GetInstantiation(); } - SigTypeContext context1(classInst, pMD->GetMethodInstantiation()); + SigTypeContext context1(parentClassInst, pMD->GetMethodInstantiation()); MetaSig methodSig1(pParentMD); TypeHandle hType1 = methodSig1.GetReturnProps().GetTypeHandleThrowing(pParentMD->GetModule(), &context1, ClassLoader::LoadTypesFlag::LoadTypes, CLASS_LOAD_EXACTPARENTS); - SigTypeContext context2(pMD); + Instantiation classInst = pMD->GetClassInstantiation(); + if (ClassLoader::IsTypicalSharedInstantiation(classInst)) + { + classInst = pMT->GetInstantiation(); + } + SigTypeContext context2(classInst, pMD->GetMethodInstantiation()); MetaSig methodSig2(pMD); TypeHandle hType2 = methodSig2.GetReturnProps().GetTypeHandleThrowing(pMD->GetModule(), &context2, ClassLoader::LoadTypesFlag::LoadTypes, CLASS_LOAD_EXACTPARENTS); diff --git a/src/coreclr/src/vm/clsload.cpp b/src/coreclr/src/vm/clsload.cpp index a72d151ffe7ae4..156fc187968af3 100644 --- a/src/coreclr/src/vm/clsload.cpp +++ b/src/coreclr/src/vm/clsload.cpp @@ -3531,16 +3531,15 @@ static void PushFinalLevels(TypeHandle typeHnd, ClassLoadLevel targetLevel, cons // and on its transitive dependencies. if (targetLevel == CLASS_LOADED) { - if (!typeHnd.IsTypeDesc()) - { - ClassLoader::ValidateMethodsWithCovariantReturnTypes(typeHnd.AsMethodTable()); - } - DFLPendingList pendingList; BOOL fBailed = FALSE; typeHnd.DoFullyLoad(NULL, CLASS_LOADED, &pendingList, &fBailed, pInstContext); + if (!typeHnd.IsTypeDesc()) + { + ClassLoader::ValidateMethodsWithCovariantReturnTypes(typeHnd.AsMethodTable()); + } // In the case of a circular dependency, one or more types will have // had their promotions deferred. diff --git a/src/tests/Regressions/coreclr/GitHub_45037/test45037.cs b/src/tests/Regressions/coreclr/GitHub_45037/test45037.cs index 99e310ee0e153c..61dfeedec558da 100644 --- a/src/tests/Regressions/coreclr/GitHub_45037/test45037.cs +++ b/src/tests/Regressions/coreclr/GitHub_45037/test45037.cs @@ -1,6 +1,16 @@ -using System; +using System; +using System.Collections.Generic; using System.Reflection; +public abstract class Base +{ + public virtual T Get() => throw new NotImplementedException(); +} + +public sealed class CovariantReturn : Base +{ + public override string Get() => throw new NotImplementedException(); +} public abstract class ABase { @@ -37,6 +47,7 @@ static int Main() { Type[] t = Assembly.GetExecutingAssembly().GetTypes(); new Child(); + new CovariantReturn(); return 100; } diff --git a/src/tests/Regressions/coreclr/GitHub_45082/test45082.cs b/src/tests/Regressions/coreclr/GitHub_45082/test45082.cs new file mode 100644 index 00000000000000..e035fa3475f34d --- /dev/null +++ b/src/tests/Regressions/coreclr/GitHub_45082/test45082.cs @@ -0,0 +1,26 @@ +using System; +using System.Collections.Generic; + +public abstract class AComponent { } +public class Component : AComponent { } + +public abstract class Abstract +{ + public abstract IReadOnlyList New { get; } +} + +public sealed class Concrete : Abstract + where T : AComponent +{ + public override IReadOnlyList New => throw null; +} + +class Program +{ + static int Main() + { + new Concrete(); + + return 100; + } +} diff --git a/src/tests/Regressions/coreclr/GitHub_45082/test45082.csproj b/src/tests/Regressions/coreclr/GitHub_45082/test45082.csproj new file mode 100644 index 00000000000000..c025440f3f9d98 --- /dev/null +++ b/src/tests/Regressions/coreclr/GitHub_45082/test45082.csproj @@ -0,0 +1,10 @@ + + + Exe + BuildAndRun + 1 + + + + + From d9e8297c7dea7d961e4d3d0fd08f003ce79af712 Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Wed, 2 Dec 2020 21:42:58 +0100 Subject: [PATCH 3/3] Fix the issue 45082 in a correct way The call to ClassLoader::ValidateMethodsWithCovariantReturnTypes is now in MethodTable::DoFullyLoad. I have also added a test case that verifies a case that David Wrighton has suggested offline, where there are 3 types... A, B and C. C derives from B which derives from A. B has a bad override which should produce an error. Then, cause C to be fully loaded without otherwise triggering a load of B. --- src/coreclr/src/vm/clsload.cpp | 5 --- src/coreclr/src/vm/methodtable.cpp | 6 +++ src/coreclr/src/vm/typehandle.cpp | 4 +- .../UnitTest/CompatibleWithTest.il | 41 ++++++++++++++++++- 4 files changed, 48 insertions(+), 8 deletions(-) diff --git a/src/coreclr/src/vm/clsload.cpp b/src/coreclr/src/vm/clsload.cpp index 156fc187968af3..a7152d3400a31d 100644 --- a/src/coreclr/src/vm/clsload.cpp +++ b/src/coreclr/src/vm/clsload.cpp @@ -3536,11 +3536,6 @@ static void PushFinalLevels(TypeHandle typeHnd, ClassLoadLevel targetLevel, cons typeHnd.DoFullyLoad(NULL, CLASS_LOADED, &pendingList, &fBailed, pInstContext); - if (!typeHnd.IsTypeDesc()) - { - ClassLoader::ValidateMethodsWithCovariantReturnTypes(typeHnd.AsMethodTable()); - } - // In the case of a circular dependency, one or more types will have // had their promotions deferred. // diff --git a/src/coreclr/src/vm/methodtable.cpp b/src/coreclr/src/vm/methodtable.cpp index 437162fa98f720..4cd29f2042946c 100644 --- a/src/coreclr/src/vm/methodtable.cpp +++ b/src/coreclr/src/vm/methodtable.cpp @@ -5282,6 +5282,12 @@ void MethodTable::DoFullyLoad(Generics::RecursionGraph * const pVisited, const CONSISTENCY_CHECK(IsRestored_NoLogging()); CONSISTENCY_CHECK(!HasApproxParent()); + if ((level == CLASS_LOADED) && !IsSharedByGenericInstantiations()) + { + _ASSERTE(GetLoadLevel() >= CLASS_DEPENDENCIES_LOADED); + ClassLoader::ValidateMethodsWithCovariantReturnTypes(this); + } + if (IsArray()) { Generics::RecursionGraph newVisited(pVisited, TypeHandle(this)); diff --git a/src/coreclr/src/vm/typehandle.cpp b/src/coreclr/src/vm/typehandle.cpp index 7883071fee28ea..8b494f07eb2d07 100644 --- a/src/coreclr/src/vm/typehandle.cpp +++ b/src/coreclr/src/vm/typehandle.cpp @@ -560,7 +560,7 @@ BOOL TypeHandle::IsBoxedAndCanCastTo(TypeHandle type, TypeHandlePairList *pPairL GC_TRIGGERS; INJECT_FAULT(COMPlusThrowOM()); - LOADS_TYPE(CLASS_LOAD_EXACTPARENTS); + LOADS_TYPE(CLASS_DEPENDENCIES_LOADED); // The caller should check for an exact match. // That will cover the cast of a (unboxed) valuetype to itself. @@ -607,7 +607,7 @@ BOOL TypeHandle::CanCastTo(TypeHandle type, TypeHandlePairList *pVisited) const MODE_ANY; INJECT_FAULT(COMPlusThrowOM()); - LOADS_TYPE(CLASS_LOAD_EXACTPARENTS); + LOADS_TYPE(CLASS_DEPENDENCIES_LOADED); } CONTRACTL_END diff --git a/src/tests/Loader/classloader/MethodImpl/CovariantReturns/UnitTest/CompatibleWithTest.il b/src/tests/Loader/classloader/MethodImpl/CovariantReturns/UnitTest/CompatibleWithTest.il index eafb6b89202371..75e580398676bb 100644 --- a/src/tests/Loader/classloader/MethodImpl/CovariantReturns/UnitTest/CompatibleWithTest.il +++ b/src/tests/Loader/classloader/MethodImpl/CovariantReturns/UnitTest/CompatibleWithTest.il @@ -124,6 +124,17 @@ } } +.class public auto ansi beforefieldinit D2 extends C2 +{ + .method public hidebysig specialname rtspecialname instance void .ctor() cil managed { ret } + + .method public hidebysig newslot virtual instance int32 MD2() + { + ldc.i4.0 + ret + } +} + .class public auto ansi beforefieldinit C3 extends C1 { .method public hidebysig specialname rtspecialname instance void .ctor() cil managed { ret } @@ -491,6 +502,16 @@ ret } + .method public static void RunTestD2() noinlining + { + newobj instance void class D2::.ctor() + callvirt instance int32 class D2::MD2() + pop + ldstr "Unexpectedly succeeded" + call void [System.Console]System.Console::WriteLine(string) + ret + } + .method public static void RunTestC3() noinlining { newobj instance void class C3::.ctor() @@ -796,6 +817,24 @@ CC2: call void Main::RunTestC2() ldc.i4.0 stloc.0 + leave.s CD2 + } + catch [mscorlib]System.TypeLoadException + { + ldstr "Caught expected TypeLoadException:" + call void [System.Console]System.Console::WriteLine(string) + call void [System.Console]System.Console::WriteLine(object) + leave.s CD2 + } +CD2: + ldstr "D2: call non-overriding method MD2 when base class of D2 has invalid covariant override" + call void [System.Console]System.Console::WriteLine(string) + + .try + { + call void Main::RunTestD2() + ldc.i4.0 + stloc.0 leave.s CC3 } catch [mscorlib]System.TypeLoadException @@ -804,7 +843,7 @@ CC2: call void [System.Console]System.Console::WriteLine(string) call void [System.Console]System.Console::WriteLine(object) leave.s CC3 - } + } CC3: ldstr "C3: override IList by int32[]" call void [System.Console]System.Console::WriteLine(string)