From 636eb226c552e4a1c821717279556a01cf6550e2 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?=
 <MichalStrehovsky@users.noreply.github.com>
Date: Fri, 12 Aug 2022 16:35:18 +0900
Subject: [PATCH 1/2] Prevent devirtualization into unallocated types

If we were in a situation like in the regression test, we would devirtualize the `GrabObject` call to `SomeUnallocatedClass.GrabObject`. But because `SomeUnallocatedClass` was never allocated, the scanner didn't scan it, and bad things would happen.

Prevent devirtualizing into types that were not seen as allocated.

This is not a real issue for non-generic (non-shareable) types because the tentative instance method optimization generates throwing bodies for these. But tentative method optimization doesn't run for shared generics:

https://github.com/dotnet/runtime/blob/4cbe6f99d23e04c56a89251d49de1b0f14000427/src/coreclr/tools/Common/Compiler/MethodExtensions.cs#L115

This was rare enough that we haven't seen it until I did #73683 and there was one useless constructor that we stopped generating and triggered this.

This also includes what is essentially a rollback of https://github.com/dotnet/runtimelab/pull/1700. This should have been rolled back with https://github.com/dotnet/runtime/pull/66145 but I forgot we had this. It was not needed.
---
 .../ILCompiler.Compiler/Compiler/ILScanner.cs | 25 +++++--------------
 .../SmokeTests/UnitTests/Devirtualization.cs  | 25 +++++++++++++++++++
 2 files changed, 31 insertions(+), 19 deletions(-)

diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ILScanner.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ILScanner.cs
index c2caeb24b3341f..da72ea35cb05b0 100644
--- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ILScanner.cs
+++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ILScanner.cs
@@ -365,8 +365,8 @@ public override DictionaryLayoutNode GetLayout(TypeSystemEntity methodOrType)
         private class ScannedDevirtualizationManager : DevirtualizationManager
         {
             private HashSet<TypeDesc> _constructedTypes = new HashSet<TypeDesc>();
+            private HashSet<TypeDesc> _canonConstructedTypes = new HashSet<TypeDesc>();
             private HashSet<TypeDesc> _unsealedTypes = new HashSet<TypeDesc>();
-            private HashSet<TypeDesc> _abstractButNonabstractlyOverriddenTypes = new HashSet<TypeDesc>();
 
             public ScannedDevirtualizationManager(ImmutableArray<DependencyNodeCore<NodeFactory>> markedNodes)
             {
@@ -390,16 +390,13 @@ public ScannedDevirtualizationManager(ImmutableArray<DependencyNodeCore<NodeFact
                             // 2. What types are the base types of other types
                             //    This is needed for optimizations. We use this information to effectively
                             //    seal types that are not base types for any other type.
-                            // 3. What abstract types got derived by non-abstract types.
-                            //    This is needed for correctness. Abstract types that were never derived
-                            //    by non-abstract types should never be devirtualized into - we probably
-                            //    didn't scan the virtual methods on them.
                             //
 
                             if (!type.IsCanonicalSubtype(CanonicalFormKind.Any))
                                 _constructedTypes.Add(type);
 
                             TypeDesc canonType = type.ConvertToCanonForm(CanonicalFormKind.Specific);
+                            _canonConstructedTypes.Add(canonType.GetClosestDefType());
 
                             bool hasNonAbstractTypeInHierarchy = canonType is not MetadataType mdType || !mdType.IsAbstract;
                             TypeDesc baseType = canonType.BaseType;
@@ -408,12 +405,6 @@ public ScannedDevirtualizationManager(ImmutableArray<DependencyNodeCore<NodeFact
                             {
                                 baseType = baseType.ConvertToCanonForm(CanonicalFormKind.Specific);
                                 added = _unsealedTypes.Add(baseType);
-
-                                bool currentTypeIsAbstract = ((MetadataType)baseType).IsAbstract;
-                                if (currentTypeIsAbstract && hasNonAbstractTypeInHierarchy)
-                                    added |= _abstractButNonabstractlyOverriddenTypes.Add(baseType);
-                                hasNonAbstractTypeInHierarchy |= !currentTypeIsAbstract;
-
                                 baseType = baseType.BaseType;
                             }
                         }
@@ -449,15 +440,11 @@ public override bool IsEffectivelySealed(TypeDesc type)
             protected override MethodDesc ResolveVirtualMethod(MethodDesc declMethod, DefType implType, out CORINFO_DEVIRTUALIZATION_DETAIL devirtualizationDetail)
             {
                 MethodDesc result = base.ResolveVirtualMethod(declMethod, implType, out devirtualizationDetail);
-                if (result != null && result.IsFinal && result.OwningType is MetadataType mdType && mdType.IsAbstract)
+                if (result != null)
                 {
-                    // If this type is abstract check that we saw a non-abstract type deriving from it.
-                    // We don't look at virtual methods introduced by abstract classes unless there's a non-abstract
-                    // class that needs them (i.e. the non-abstract class doesn't immediately override them).
-                    // This lets us optimize out some unused virtual method implementations.
-                    // Allowing this to devirtualize would cause trouble because we didn't scan the method
-                    // and expected it would be optimized out.
-                    if (!_abstractButNonabstractlyOverriddenTypes.Contains(mdType.ConvertToCanonForm(CanonicalFormKind.Specific)))
+                    // If we would resolve into a type that wasn't seen as allocated, don't allow devirtualization.
+                    // It would go past what we scanned in the scanner and that doesn't lead to good things.
+                    if (!_canonConstructedTypes.Contains(result.OwningType.ConvertToCanonForm(CanonicalFormKind.Specific)))
                     {
                         // FAILED_BUBBLE_IMPL_NOT_REFERENCEABLE is close enough...
                         devirtualizationDetail = CORINFO_DEVIRTUALIZATION_DETAIL.CORINFO_DEVIRTUALIZATION_FAILED_BUBBLE_IMPL_NOT_REFERENCEABLE;
diff --git a/src/tests/nativeaot/SmokeTests/UnitTests/Devirtualization.cs b/src/tests/nativeaot/SmokeTests/UnitTests/Devirtualization.cs
index 73e3ab4d3064fb..5954c6118e6696 100644
--- a/src/tests/nativeaot/SmokeTests/UnitTests/Devirtualization.cs
+++ b/src/tests/nativeaot/SmokeTests/UnitTests/Devirtualization.cs
@@ -12,6 +12,7 @@ internal static int Run()
     {
         RegressionBug73076.Run();
         DevirtualizationCornerCaseTests.Run();
+        DevirtualizeIntoUnallocatedGenericType.Run();
 
         return 100;
     }
@@ -111,4 +112,28 @@ public static void Run()
             TestIntf2((IIntf2)Activator.CreateInstance(typeof(Intf2Impl2<>).MakeGenericType(typeof(object))), 456);
         }
     }
+
+    class DevirtualizeIntoUnallocatedGenericType
+    {
+        class Never { }
+
+        class SomeGeneric<T>
+        {
+            public virtual object GrabObject() => null;
+        }
+
+        sealed class SomeUnallocatedClass<T> : SomeGeneric<T>
+        {
+            public override object GrabObject() => new Never();
+        }
+
+        [MethodImpl(MethodImplOptions.NoInlining)]
+        static SomeUnallocatedClass<object> GrabInst() => null;
+
+        public static void Run()
+        {
+            if (GrabInst() != null)
+                GrabInst().GrabObject();
+        }
+    }
 }

From 92d9b7506dc511959be3e538d2140ddfe4e4ff22 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?=
 <MichalStrehovsky@users.noreply.github.com>
Date: Fri, 12 Aug 2022 19:07:04 +0900
Subject: [PATCH 2/2] Update tests.proj

---
 src/libraries/tests.proj | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/libraries/tests.proj b/src/libraries/tests.proj
index 9820d280330719..da787c4710b61c 100644
--- a/src/libraries/tests.proj
+++ b/src/libraries/tests.proj
@@ -490,6 +490,7 @@
     <ProjectExclusions Include="$(MSBuildThisFileDirectory)System.Net.Mail\tests\Functional\System.Net.Mail.Functional.Tests.csproj"
                       Condition="'$(TargetOS)' == 'windows'"/>
 
+    <ProjectExclusions Include="$(MSBuildThisFileDirectory)System.Reflection.Metadata\tests\System.Reflection.Metadata.Tests.csproj" />
     <ProjectExclusions Include="$(MSBuildThisFileDirectory)System.Net.Http.Json\tests\FunctionalTests\System.Net.Http.Json.Functional.Tests.csproj" />
     <ProjectExclusions Include="$(MSBuildThisFileDirectory)System.Linq.Parallel\tests\System.Linq.Parallel.Tests.csproj" />
     <ProjectExclusions Include="$(MSBuildThisFileDirectory)System.Text.Json\tests\System.Text.Json.Tests\System.Text.Json.Tests.csproj" />