From a303a070c3f4df330dc275d6ab20f6dbd8f382bf Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Wed, 13 Sep 2023 16:52:26 -0700 Subject: [PATCH] [release/8.0] Revert "Emit less metadata for not-reflection-visible types (#91660)" (#91989) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Revert "Emit less metadata for not-reflection-visible types (#91660)" This reverts commit 0bfc0c9e838193b699415a015504b4aede1f7f0a. * Regression test --------- Co-authored-by: Michal Strehovský Co-authored-by: Carlos Sánchez López <1175054+carlossanlop@users.noreply.github.com> --- .../Compiler/DependencyAnalysis/EETypeNode.cs | 3 +- .../GenericDefinitionEETypeNode.cs | 2 +- .../DependencyAnalysis/NodeFactory.cs | 17 +------ .../DependencyAnalysis/TypeMetadataNode.cs | 49 +++++++++---------- .../Compiler/MetadataManager.cs | 6 +-- .../Compiler/UsageBasedMetadataManager.cs | 10 ++-- .../SmokeTests/Reflection/Reflection.cs | 30 ++++++++++++ .../TrimmingBehaviors/DeadCodeElimination.cs | 46 ----------------- 8 files changed, 63 insertions(+), 100 deletions(-) diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/EETypeNode.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/EETypeNode.cs index cd1ef49a22147..61520b4bfadaf 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/EETypeNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/EETypeNode.cs @@ -625,8 +625,7 @@ protected override DependencyList ComputeNonRelocationBasedDependencies(NodeFact // Ask the metadata manager // if we have any dependencies due to presence of the EEType. - bool isFullType = factory.MaximallyConstructableType(_type) == this; - factory.MetadataManager.GetDependenciesDueToEETypePresence(ref dependencies, factory, _type, isFullType); + factory.MetadataManager.GetDependenciesDueToEETypePresence(ref dependencies, factory, _type); if (_type is MetadataType mdType) ModuleUseBasedDependencyAlgorithm.AddDependenciesDueToModuleUse(ref dependencies, factory, mdType.Module); diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/GenericDefinitionEETypeNode.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/GenericDefinitionEETypeNode.cs index 2bf7672884de0..b42d93273e468 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/GenericDefinitionEETypeNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/GenericDefinitionEETypeNode.cs @@ -25,7 +25,7 @@ protected override DependencyList ComputeNonRelocationBasedDependencies(NodeFact DependencyList dependencyList = null; // Ask the metadata manager if we have any dependencies due to the presence of the EEType. - factory.MetadataManager.GetDependenciesDueToEETypePresence(ref dependencyList, factory, _type, isFullType: true); + factory.MetadataManager.GetDependenciesDueToEETypePresence(ref dependencyList, factory, _type); return dependencyList; } diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/NodeFactory.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/NodeFactory.cs index 5bc64c02d2886..fbb6c08e04006 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/NodeFactory.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/NodeFactory.cs @@ -458,12 +458,7 @@ private void CreateNodeCaches() _typesWithMetadata = new NodeCache(type => { - return new TypeMetadataNode(type, includeCustomAttributes: true); - }); - - _typesWithMetadataWithoutCustomAttributes = new NodeCache(type => - { - return new TypeMetadataNode(type, includeCustomAttributes: false); + return new TypeMetadataNode(type); }); _methodsWithMetadata = new NodeCache(method => @@ -1161,16 +1156,6 @@ internal TypeMetadataNode TypeMetadata(MetadataType type) return _typesWithMetadata.GetOrAdd(type); } - private NodeCache _typesWithMetadataWithoutCustomAttributes; - - internal TypeMetadataNode TypeMetadataWithoutCustomAttributes(MetadataType type) - { - // These are only meaningful for UsageBasedMetadataManager. We should not have them - // in the dependency graph otherwise. - Debug.Assert(MetadataManager is UsageBasedMetadataManager); - return _typesWithMetadataWithoutCustomAttributes.GetOrAdd(type); - } - private NodeCache _methodsWithMetadata; internal MethodMetadataNode MethodMetadata(MethodDesc method) diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/TypeMetadataNode.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/TypeMetadataNode.cs index 8979feb1061ca..de162987d508b 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/TypeMetadataNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/TypeMetadataNode.cs @@ -24,13 +24,11 @@ namespace ILCompiler.DependencyAnalysis internal sealed class TypeMetadataNode : DependencyNodeCore { private readonly MetadataType _type; - private readonly bool _includeCustomAttributes; - public TypeMetadataNode(MetadataType type, bool includeCustomAttributes) + public TypeMetadataNode(MetadataType type) { Debug.Assert(type.IsTypeDefinition); _type = type; - _includeCustomAttributes = includeCustomAttributes; } public MetadataType Type => _type; @@ -39,21 +37,13 @@ public override IEnumerable GetStaticDependencies(NodeFacto { DependencyList dependencies = new DependencyList(); - if (_includeCustomAttributes) - CustomAttributeBasedDependencyAlgorithm.AddDependenciesDueToCustomAttributes(ref dependencies, factory, ((EcmaType)_type)); + CustomAttributeBasedDependencyAlgorithm.AddDependenciesDueToCustomAttributes(ref dependencies, factory, ((EcmaType)_type)); DefType containingType = _type.ContainingType; if (containingType != null) - { - TypeMetadataNode metadataNode = _includeCustomAttributes - ? factory.TypeMetadata((MetadataType)containingType) - : factory.TypeMetadataWithoutCustomAttributes((MetadataType)containingType); - dependencies.Add(metadataNode, "Containing type of a reflectable type"); - } + dependencies.Add(factory.TypeMetadata((MetadataType)containingType), "Containing type of a reflectable type"); else - { dependencies.Add(factory.ModuleMetadata(_type.Module), "Containing module of a reflectable type"); - } var mdManager = (UsageBasedMetadataManager)factory.MetadataManager; @@ -110,7 +100,7 @@ public override IEnumerable GetStaticDependencies(NodeFacto /// Decomposes a constructed type into individual units that will be needed to /// express the constructed type in metadata. /// - public static void GetMetadataDependencies(ref DependencyList dependencies, NodeFactory nodeFactory, TypeDesc type, string reason, bool isFullType = true) + public static void GetMetadataDependencies(ref DependencyList dependencies, NodeFactory nodeFactory, TypeDesc type, string reason) { MetadataManager mdManager = nodeFactory.MetadataManager; @@ -120,13 +110,13 @@ public static void GetMetadataDependencies(ref DependencyList dependencies, Node case TypeFlags.SzArray: case TypeFlags.ByRef: case TypeFlags.Pointer: - GetMetadataDependencies(ref dependencies, nodeFactory, ((ParameterizedType)type).ParameterType, reason, isFullType); + GetMetadataDependencies(ref dependencies, nodeFactory, ((ParameterizedType)type).ParameterType, reason); break; case TypeFlags.FunctionPointer: var pointerType = (FunctionPointerType)type; - GetMetadataDependencies(ref dependencies, nodeFactory, pointerType.Signature.ReturnType, reason, isFullType); + GetMetadataDependencies(ref dependencies, nodeFactory, pointerType.Signature.ReturnType, reason); foreach (TypeDesc paramType in pointerType.Signature) - GetMetadataDependencies(ref dependencies, nodeFactory, paramType, reason, isFullType); + GetMetadataDependencies(ref dependencies, nodeFactory, paramType, reason); break; case TypeFlags.SignatureMethodVariable: @@ -136,22 +126,27 @@ public static void GetMetadataDependencies(ref DependencyList dependencies, Node default: Debug.Assert(type.IsDefType); - var typeDefinition = (MetadataType)type.GetTypeDefinition(); + TypeDesc typeDefinition = type.GetTypeDefinition(); if (typeDefinition != type) { + if (mdManager.CanGenerateMetadata((MetadataType)typeDefinition)) + { + dependencies ??= new DependencyList(); + dependencies.Add(nodeFactory.TypeMetadata((MetadataType)typeDefinition), reason); + } + foreach (TypeDesc typeArg in type.Instantiation) { - GetMetadataDependencies(ref dependencies, nodeFactory, typeArg, reason, isFullType); + GetMetadataDependencies(ref dependencies, nodeFactory, typeArg, reason); } } - - if (mdManager.CanGenerateMetadata(typeDefinition)) + else { - dependencies ??= new DependencyList(); - TypeMetadataNode node = isFullType - ? nodeFactory.TypeMetadata(typeDefinition) - : nodeFactory.TypeMetadataWithoutCustomAttributes(typeDefinition); - dependencies.Add(node, reason); + if (mdManager.CanGenerateMetadata((MetadataType)type)) + { + dependencies ??= new DependencyList(); + dependencies.Add(nodeFactory.TypeMetadata((MetadataType)type), reason); + } } break; } @@ -159,7 +154,7 @@ public static void GetMetadataDependencies(ref DependencyList dependencies, Node protected override string GetName(NodeFactory factory) { - return $"Reflectable type: {_type}{(!_includeCustomAttributes ? " (No custom attributes)" : "")}"; + return "Reflectable type: " + _type.ToString(); } protected override void OnMarked(NodeFactory factory) diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/MetadataManager.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/MetadataManager.cs index b62304f362c16..8658068c1ebe4 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/MetadataManager.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/MetadataManager.cs @@ -478,13 +478,13 @@ protected virtual void GetMetadataDependenciesDueToReflectability(ref Dependency /// /// This method is an extension point that can provide additional metadata-based dependencies to generated EETypes. /// - public virtual void GetDependenciesDueToEETypePresence(ref DependencyList dependencies, NodeFactory factory, TypeDesc type, bool isFullType) + public virtual void GetDependenciesDueToEETypePresence(ref DependencyList dependencies, NodeFactory factory, TypeDesc type) { MetadataCategory category = GetMetadataCategory(type); if ((category & MetadataCategory.Description) != 0) { - GetMetadataDependenciesDueToReflectability(ref dependencies, factory, type, isFullType); + GetMetadataDependenciesDueToReflectability(ref dependencies, factory, type); } } @@ -493,7 +493,7 @@ internal virtual void GetDependenciesDueToModuleUse(ref DependencyList dependenc // MetadataManagers can override this to provide additional dependencies caused by using a module } - protected virtual void GetMetadataDependenciesDueToReflectability(ref DependencyList dependencies, NodeFactory factory, TypeDesc type, bool isFullType) + protected virtual void GetMetadataDependenciesDueToReflectability(ref DependencyList dependencies, NodeFactory factory, TypeDesc type) { // MetadataManagers can override this to provide additional dependencies caused by the emission of metadata // (E.g. dependencies caused by the type having custom attributes applied to it: making sure we compile the attribute constructor diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/UsageBasedMetadataManager.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/UsageBasedMetadataManager.cs index 45be574e031ea..0d4a855e736ed 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/UsageBasedMetadataManager.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/UsageBasedMetadataManager.cs @@ -271,9 +271,9 @@ internal override void GetDependenciesDueToModuleUse(ref DependencyList dependen } } - protected override void GetMetadataDependenciesDueToReflectability(ref DependencyList dependencies, NodeFactory factory, TypeDesc type, bool isFullType) + protected override void GetMetadataDependenciesDueToReflectability(ref DependencyList dependencies, NodeFactory factory, TypeDesc type) { - TypeMetadataNode.GetMetadataDependencies(ref dependencies, factory, type, "Reflectable type", isFullType); + TypeMetadataNode.GetMetadataDependencies(ref dependencies, factory, type, "Reflectable type"); if (type.IsDelegate) { @@ -385,9 +385,9 @@ private static bool IsTrimmableAssembly(ModuleDesc assembly) return false; } - public override void GetDependenciesDueToEETypePresence(ref DependencyList dependencies, NodeFactory factory, TypeDesc type, bool isFullType) + public override void GetDependenciesDueToEETypePresence(ref DependencyList dependencies, NodeFactory factory, TypeDesc type) { - base.GetDependenciesDueToEETypePresence(ref dependencies, factory, type, isFullType); + base.GetDependenciesDueToEETypePresence(ref dependencies, factory, type); DataflowAnalyzedTypeDefinitionNode.GetDependencies(ref dependencies, factory, FlowAnnotations, type); } @@ -970,7 +970,7 @@ public bool GeneratesMetadata(MethodDesc methodDef) public bool GeneratesMetadata(MetadataType typeDef) { - return _factory.TypeMetadata(typeDef).Marked || _factory.TypeMetadataWithoutCustomAttributes(typeDef).Marked; + return _factory.TypeMetadata(typeDef).Marked; } public bool GeneratesMetadata(EcmaModule module, CustomAttributeHandle caHandle) diff --git a/src/tests/nativeaot/SmokeTests/Reflection/Reflection.cs b/src/tests/nativeaot/SmokeTests/Reflection/Reflection.cs index 8a77223bb3feb..df29defbdfe29 100644 --- a/src/tests/nativeaot/SmokeTests/Reflection/Reflection.cs +++ b/src/tests/nativeaot/SmokeTests/Reflection/Reflection.cs @@ -67,6 +67,7 @@ private static int Main() TestByRefTypeLoad.Run(); TestGenericLdtoken.Run(); TestAbstractGenericLdtoken.Run(); + TestTypeHandlesVisibleFromIDynamicInterfaceCastable.Run(); // // Mostly functionality tests @@ -2457,6 +2458,35 @@ public static void Run() } } + class TestTypeHandlesVisibleFromIDynamicInterfaceCastable + { + class MyAttribute : Attribute { } + + [My] + interface IUnusedInterface { } + + class Foo : IDynamicInterfaceCastable + { + public RuntimeTypeHandle GetInterfaceImplementation(RuntimeTypeHandle interfaceType) => throw new NotImplementedException(); + + public bool IsInterfaceImplemented(RuntimeTypeHandle interfaceType, bool throwIfNotImplemented) + { + Type t = Type.GetTypeFromHandle(interfaceType); + if (t.GetCustomAttribute() == null) + throw new Exception(); + + return true; + } + } + + public static void Run() + { + object myObject = new Foo(); + if (myObject is not IUnusedInterface) + throw new Exception(); + } + } + class TestEntryPoint { public static void Run() diff --git a/src/tests/nativeaot/SmokeTests/TrimmingBehaviors/DeadCodeElimination.cs b/src/tests/nativeaot/SmokeTests/TrimmingBehaviors/DeadCodeElimination.cs index e030b3a1de99c..152c851455666 100644 --- a/src/tests/nativeaot/SmokeTests/TrimmingBehaviors/DeadCodeElimination.cs +++ b/src/tests/nativeaot/SmokeTests/TrimmingBehaviors/DeadCodeElimination.cs @@ -21,7 +21,6 @@ public static int Run() TestStaticVirtualMethodOptimizations.Run(); TestTypeEquals.Run(); TestBranchesInGenericCodeRemoval.Run(); - TestLimitedMetadataBlobs.Run(); return 100; } @@ -379,51 +378,6 @@ public static void Run() } } - class TestLimitedMetadataBlobs - { - class MyAttribute : Attribute - { - public MyAttribute([DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] Type t) { } - } - - class ShouldNotBeNeeded - { - } - - [My(typeof(ShouldNotBeNeeded))] - interface INeverImplemented - { - void Never(); - } - - static INeverImplemented s_instance; -#if !DEBUG - static Type s_type; -#endif - - internal static void Run() - { - Console.WriteLine("Testing generation of limited metadata blobs"); - - // Force a reference to the interface from a dispatch cell - if (s_instance != null) - s_instance.Never(); - - // Following is for release only since it relies on optimizing the typeof into an unconstructed - // MethodTable. -#if !DEBUG - // Force another reference from an LDTOKEN - if (s_type == typeof(INeverImplemented)) - s_type = typeof(object); -#endif - - ThrowIfPresent(typeof(TestLimitedMetadataBlobs), nameof(ShouldNotBeNeeded)); - ThrowIfPresent(typeof(TestLimitedMetadataBlobs), nameof(MyAttribute)); - ThrowIfNotPresent(typeof(TestLimitedMetadataBlobs), nameof(INeverImplemented)); - } - } - - [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2070:UnrecognizedReflectionPattern", Justification = "That's the point")] private static Type GetTypeSecretly(Type testType, string typeName) => testType.GetNestedType(typeName, BindingFlags.NonPublic | BindingFlags.Public);