From a817006bfad0cd53d539d6973188f2d4181dade2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Tue, 10 Sep 2024 16:20:10 +0900 Subject: [PATCH] Consider existence of EETypes and metadata for typeof checks (#107347) The problem was that when we optimize `typeof(Foo) == somevalue` checks, we were looking at the presence of a constructed `MethodTable` of `Foo` in the whole program view. If it didn't exist, we'd declare the equality comparison can't succeed. There is however an edge case where someone could obtain a `System.Type` representing a type by browsing the reflection metadata. Such `System.Type` might not have a workable constructed `MethodTable`, but should still compare equal to the `typeof`. An example of when this could happen is when the type is used in a custom attribute metadata blob - such type may even have no `MethodTable` at all. The fix is to look not just at `MethodTable` but also at the metadata for the type. --- .../Compiler/SubstitutedILProvider.cs | 10 +++++++- .../IL/ILImporter.Scanner.cs | 11 ++++++++- src/coreclr/tools/aot/ILCompiler/Program.cs | 2 +- .../TrimmingBehaviors/DeadCodeElimination.cs | 23 +++++++++++++++++++ 4 files changed, 43 insertions(+), 3 deletions(-) diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/SubstitutedILProvider.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/SubstitutedILProvider.cs index 00b0d1143f168..6b339f8a89d2e 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/SubstitutedILProvider.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/SubstitutedILProvider.cs @@ -23,12 +23,14 @@ public class SubstitutedILProvider : ILProvider private readonly ILProvider _nestedILProvider; private readonly SubstitutionProvider _substitutionProvider; private readonly DevirtualizationManager _devirtualizationManager; + private readonly MetadataManager _metadataManager; - public SubstitutedILProvider(ILProvider nestedILProvider, SubstitutionProvider substitutionProvider, DevirtualizationManager devirtualizationManager) + public SubstitutedILProvider(ILProvider nestedILProvider, SubstitutionProvider substitutionProvider, DevirtualizationManager devirtualizationManager, MetadataManager metadataManager = null) { _nestedILProvider = nestedILProvider; _substitutionProvider = substitutionProvider; _devirtualizationManager = devirtualizationManager; + _metadataManager = metadataManager; } public override MethodIL GetMethodIL(MethodDesc method) @@ -989,9 +991,15 @@ private bool TryExpandTypeEquality(in TypeEqualityPatternAnalyzer analyzer, Meth if (!ConstructedEETypeNode.CreationAllowed(knownType)) return false; + // If a constructed MethodTable for this type exists, the comparison could succeed. if (_devirtualizationManager.CanReferenceConstructedTypeOrCanonicalFormOfType(knownType.NormalizeInstantiation())) return false; + // If we can have metadata for the type the comparison could succeed even if no MethodTable present. + // (This is the case of metadata-only types, where we were able to optimize the MethodTable away.) + if (_metadataManager != null && knownType.GetTypeDefinition() is MetadataType mdType && _metadataManager.CanGenerateMetadata(mdType)) + return false; + constant = 0; } diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/IL/ILImporter.Scanner.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/IL/ILImporter.Scanner.cs index 7fec5bbe20d74..406649bdc60e5 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/IL/ILImporter.Scanner.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/IL/ILImporter.Scanner.cs @@ -836,7 +836,16 @@ private void ImportBranch(ILOpcode opcode, BasicBlock target, BasicBlock fallthr && ConstructedEETypeNode.CreationAllowed(typeEqualityCheckType) && !typeEqualityCheckType.ConvertToCanonForm(CanonicalFormKind.Specific).IsCanonicalSubtype(CanonicalFormKind.Any)) { - condition = _factory.MaximallyConstructableType(typeEqualityCheckType); + // If the type could generate metadata, we set the condition to the presence of the metadata. + // This covers situations where the typeof is compared against metadata-only types. + // Note this assumes a constructed MethodTable always implies having metadata. + // This will likely remain true because anyone can call Object.GetType on a constructed type. + // If the type cannot generate metadata, we only condition on the MethodTable itself. + if (!_factory.MetadataManager.IsReflectionBlocked(typeEqualityCheckType) + && typeEqualityCheckType.GetTypeDefinition() is MetadataType typeEqualityCheckMetadataType) + condition = _factory.TypeMetadata(typeEqualityCheckMetadataType); + else + condition = _factory.MaximallyConstructableType(typeEqualityCheckType); } } diff --git a/src/coreclr/tools/aot/ILCompiler/Program.cs b/src/coreclr/tools/aot/ILCompiler/Program.cs index 30fd2028f3862..589fdf8b53848 100644 --- a/src/coreclr/tools/aot/ILCompiler/Program.cs +++ b/src/coreclr/tools/aot/ILCompiler/Program.cs @@ -499,7 +499,7 @@ void RunScanner() interopStubManager = scanResults.GetInteropStubManager(interopStateManager, pinvokePolicy); - ilProvider = new SubstitutedILProvider(unsubstitutedILProvider, substitutionProvider, devirtualizationManager); + ilProvider = new SubstitutedILProvider(unsubstitutedILProvider, substitutionProvider, devirtualizationManager, metadataManager); // Use a more precise IL provider that uses whole program analysis for dead branch elimination builder.UseILProvider(ilProvider); diff --git a/src/tests/nativeaot/SmokeTests/TrimmingBehaviors/DeadCodeElimination.cs b/src/tests/nativeaot/SmokeTests/TrimmingBehaviors/DeadCodeElimination.cs index 9fe236cef1f13..0a6090f809243 100644 --- a/src/tests/nativeaot/SmokeTests/TrimmingBehaviors/DeadCodeElimination.cs +++ b/src/tests/nativeaot/SmokeTests/TrimmingBehaviors/DeadCodeElimination.cs @@ -529,13 +529,27 @@ class TestTypeEqualityDeadBranchScanRemoval { class NeverAllocated1 { } class NeverAllocated2 { } + class NeverAllocated3 { } class PossiblyAllocated1 { } class PossiblyAllocated2 { } + class MyAttribute : Attribute + { + public Type TheType; + + public MyAttribute(Type t) => TheType = t; + } + + [My(typeof(NeverAllocated3))] + class AttributeHolder { } + [MethodImpl(MethodImplOptions.NoInlining)] static Type GetNeverObject() => null; + [MethodImpl(MethodImplOptions.NoInlining)] + static Type GetNeverAllocated3Type() => typeof(AttributeHolder).GetCustomAttribute().TheType; + static volatile Type s_sink; public static void Run() @@ -557,6 +571,15 @@ public static void Run() } if (Environment.GetEnvironmentVariable("SURETHING") != null) s_sink = typeof(PossiblyAllocated1); + + if (GetNeverAllocated3Type() == typeof(NeverAllocated3)) + { + Console.WriteLine($"{nameof(NeverAllocated3)} check succeeded"); + } + else + { + throw new Exception(); + } } }