Fix covariant returns validation (#41874)

* Fix covariant returns validation

Recent fix to a problem with covariant returns validation has introduced
another problem related to code versioning. The validation method is now
called too late in the type load process which causes problem in
crossgening or in case code versioning is disabled.

This change fixes it by splitting the return type verification and
mutating changes like vtable slot updates into two separate phases.

* Fix missing precondition for calling SetRequiresCovariantReturnTypeChecking

diff --git a/src/coreclr/src/vm/class.cpp b/src/coreclr/src/vm/class.cpp
index a620a27..c4fb3ed 100644 (file)
--- a/src/coreclr/src/vm/class.cpp
+++ b/src/coreclr/src/vm/class.cpp
@@ -963,6 +963,8 @@ void ClassLoader::LoadExactParents(MethodTable *pMT)
 
     MethodTableBuilder::CopyExactParentSlots(pMT, pApproxParentMT);
 
+    PropagateCovariantReturnMethodImplSlots(pMT);
+
     // We can now mark this type as having exact parents
     pMT->SetHasExactParent();
 
@@ -1133,43 +1135,10 @@ void ClassLoader::ValidateMethodsWithCovariantReturnTypes(MethodTable* pMT)
     }
     CONTRACTL_END;
 
-    //
-    // Validation for methods with covariant return types is a two step process.
-    //
-    // The first step is to validate that the return types on overriding methods with covariant return types are
+    // Validate that the return types on overriding methods with covariant return types are
     // compatible with the return type of the method being overridden. Compatibility rules are defined by
     // ECMA I.8.7.1, which is what the CanCastTo() API checks.
     //
-    // The second step is to propagate an overriding MethodImpl to all applicable vtable slots if the MethodImpl
-    // has the PreserveBaseOverrides attribute. This is to ensure that if we use the signature of one of
-    // the base type methods to call the overriding method, we still execute the overriding method.
-    //
-    // Consider this case:
-    //
-    //      class A {
-    //          RetType VirtualFunction() { }
-    //      }
-    //      class B : A {
-    //          [PreserveBaseOverrides]
-    //          DerivedRetType VirtualFunction() { .override A.VirtualFuncion }
-    //      }
-    //      class C : B {
-    //          MoreDerivedRetType VirtualFunction() { .override A.VirtualFunction }
-    //      }
-    //
-    // NOTE: Typically the attribute would be added to the MethodImpl on C, but was omitted in this example to
-    //       illustrate how its presence on a MethodImpl on the base type can propagate as well. In other words,
-    //       think of it as applying to the vtable slot itself, so any MethodImpl that overrides this slot on a
-    //       derived type will propagate to all other applicable vtable slots.
-    //
-    // Given an object of type C, the attribute will ensure that:
-    //      callvirt RetType A::VirtualFunc()               -> executes the MethodImpl on C
-    //      callvirt DerivedRetType B::VirtualFunc()        -> executes the MethodImpl on C
-    //      callvirt MoreDerivedRetType C::VirtualFunc()    -> executes the MethodImpl on C
-    //
-    // Without the attribute, the second callvirt would normally execute the MethodImpl on B (the MethodImpl on
-    // C does not override the vtable slot of B's MethodImpl, but only overrides the declaring method's vtable slot.
-    //
 
     // Validation not applicable to interface types and value types, since these are not currently
     // supported with the covariant return feature
@@ -1202,11 +1171,6 @@ void ClassLoader::ValidateMethodsWithCovariantReturnTypes(MethodTable* pMT)
             if (!pMD->RequiresCovariantReturnTypeChecking() && !pParentMD->RequiresCovariantReturnTypeChecking())
                 continue;
 
-            // If the bit is not set on this method, but we reach here because it's been set on the method at the same slot on
-            // the base type, set the bit for the current method to ensure any future overriding method down the chain gets checked.
-            if (!pMD->RequiresCovariantReturnTypeChecking())
-                pMD->SetRequiresCovariantReturnTypeChecking();
-
             // The context used to load the return type of the parent method has to use the generic method arguments
             // of the overriding method, otherwise the type comparison below will not work correctly
             SigTypeContext context1(pParentMD->GetClassInstantiation(), pMD->GetMethodInstantiation());
@@ -1243,8 +1207,60 @@ void ClassLoader::ValidateMethodsWithCovariantReturnTypes(MethodTable* pMT)
             }
         }
     }
+}
 
-    // Step 2: propate overriding MethodImpls to applicable vtable slots if the declaring method has the attribute
+/*static*/
+void ClassLoader::PropagateCovariantReturnMethodImplSlots(MethodTable* pMT)
+{
+    CONTRACTL
+    {
+        STANDARD_VM_CHECK;
+        PRECONDITION(CheckPointer(pMT));
+    }
+    CONTRACTL_END;
+
+    // Propagate an overriding MethodImpl to all applicable vtable slots if the MethodImpl
+    // has the PreserveBaseOverrides attribute. This is to ensure that if we use the signature of one of
+    // the base type methods to call the overriding method, we still execute the overriding method.
+    //
+    // Consider this case:
+    //
+    //      class A {
+    //          RetType VirtualFunction() { }
+    //      }
+    //      class B : A {
+    //          [PreserveBaseOverrides]
+    //          DerivedRetType VirtualFunction() { .override A.VirtualFuncion }
+    //      }
+    //      class C : B {
+    //          MoreDerivedRetType VirtualFunction() { .override A.VirtualFunction }
+    //      }
+    //
+    // NOTE: Typically the attribute would be added to the MethodImpl on C, but was omitted in this example to
+    //       illustrate how its presence on a MethodImpl on the base type can propagate as well. In other words,
+    //       think of it as applying to the vtable slot itself, so any MethodImpl that overrides this slot on a
+    //       derived type will propagate to all other applicable vtable slots.
+    //
+    // Given an object of type C, the attribute will ensure that:
+    //      callvirt RetType A::VirtualFunc()               -> executes the MethodImpl on C
+    //      callvirt DerivedRetType B::VirtualFunc()        -> executes the MethodImpl on C
+    //      callvirt MoreDerivedRetType C::VirtualFunc()    -> executes the MethodImpl on C
+    //
+    // Without the attribute, the second callvirt would normally execute the MethodImpl on B (the MethodImpl on
+    // C does not override the vtable slot of B's MethodImpl, but only overrides the declaring method's vtable slot.
+    //
+
+    // Validation not applicable to interface types and value types, since these are not currently
+    // supported with the covariant return feature
+
+    if (pMT->IsInterface() || pMT->IsValueType())
+        return;
+
+    MethodTable* pParentMT = pMT->GetParentMethodTable();
+    if (pParentMT == NULL)
+        return;
+
+    // Propagate overriding MethodImpls to applicable vtable slots if the declaring method has the attribute
 
     if (pMT->GetClass()->HasVTableMethodImpl())
     {
@@ -1265,6 +1281,11 @@ void ClassLoader::ValidateMethodsWithCovariantReturnTypes(MethodTable* pMT)
             if (pMD == pParentMD)
                 continue;
 
+            // If the bit is not set on this method, but we reach here because it's been set on the method at the same slot on
+            // the base type, set the bit for the current method to ensure any future overriding method down the chain gets checked.
+            if (!pMD->RequiresCovariantReturnTypeChecking() && pParentMD->RequiresCovariantReturnTypeChecking())
+                pMD->SetRequiresCovariantReturnTypeChecking();
+
             // The attribute is only applicable to MethodImpls. For anything else, it will be treated as a no-op
             if (!pMD->IsMethodImpl())
                 continue;
@@ -1310,6 +1331,7 @@ void ClassLoader::ValidateMethodsWithCovariantReturnTypes(MethodTable* pMT)
     }
 }
 
+
 //*******************************************************************************
 // This is the routine that computes the internal type of a given type.  It normalizes
 // structs that have only one field (of int/ptr sized values), to be that underlying type.

diff --git a/src/coreclr/src/vm/clsload.cpp b/src/coreclr/src/vm/clsload.cpp
index 6ce4aac..a72d151 100644 (file)
--- a/src/coreclr/src/vm/clsload.cpp
+++ b/src/coreclr/src/vm/clsload.cpp
@@ -3523,11 +3523,6 @@ static void PushFinalLevels(TypeHandle typeHnd, ClassLoadLevel targetLevel, cons
     // final state, sans the IsFullyLoaded bit.
     if (targetLevel >= CLASS_DEPENDENCIES_LOADED)
     {
-        if (!typeHnd.IsTypeDesc())
-        {
-            ClassLoader::ValidateMethodsWithCovariantReturnTypes(typeHnd.AsMethodTable());
-        }
-
         BOOL fBailed = FALSE;
         typeHnd.DoFullyLoad(NULL, CLASS_DEPENDENCIES_LOADED, NULL, &fBailed, pInstContext);
     }
@@ -3536,6 +3531,11 @@ 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;
 

diff --git a/src/coreclr/src/vm/clsload.hpp b/src/coreclr/src/vm/clsload.hpp
index 550fa1e..b947d4a 100644 (file)
--- a/src/coreclr/src/vm/clsload.hpp
+++ b/src/coreclr/src/vm/clsload.hpp
@@ -971,6 +971,8 @@ private:
 
     static void LoadExactParentAndInterfacesTransitively(MethodTable *pMT);
 
+    static void PropagateCovariantReturnMethodImplSlots(MethodTable* pMT);
+
     static bool IsCompatibleWith(TypeHandle hType1, TypeHandle hType2);
     static CorElementType GetReducedTypeElementType(TypeHandle hType);
     static CorElementType GetVerificationTypeElementType(TypeHandle hType);