From aeb07f74175e7182bfa6cbc8bbf45bf2b8c1955c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Wed, 26 Jun 2024 10:03:22 +0900 Subject: [PATCH] Ensure static constructor is reflection-callable when NonPublicConstructors were kept (#103946) This ensures that whenever we've seen the static constructor was reflection-accessed, we make sure it's runnable with RunClassConstructor. This previously required some acrobatics because the static constructor itself could be reflection blocked and we might have still needed to RunClassConstructor the type with such constructor (notably the SR class preinitializes random ResourceManager related types to avoid recursion). Redo everything to simply use the reflectable static constructor method as a marker. --- .../Compiler/AnalysisBasedMetadataManager.cs | 10 +---- .../Compiler/Dataflow/ReflectionMarker.cs | 12 ++---- .../Compiler/MetadataManager.cs | 7 ++++ .../Compiler/UsageBasedMetadataManager.cs | 20 +--------- .../SmokeTests/Reflection/Reflection.cs | 37 +++++++++++++++++++ 5 files changed, 49 insertions(+), 37 deletions(-) diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/AnalysisBasedMetadataManager.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/AnalysisBasedMetadataManager.cs index 7b9f19c8e8be1..5ee8411d8ccac 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/AnalysisBasedMetadataManager.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/AnalysisBasedMetadataManager.cs @@ -22,7 +22,6 @@ namespace ILCompiler public sealed class AnalysisBasedMetadataManager : MetadataManager, ICompilationRootProvider { private readonly List _modulesWithMetadata; - private readonly List _typesWithRootedCctorContext; private readonly List _forcedTypes; private readonly Dictionary _reflectableTypes = new Dictionary(); @@ -36,7 +35,7 @@ public AnalysisBasedMetadataManager(CompilerTypeSystemContext typeSystemContext) new NoDynamicInvokeThunkGenerationPolicy(), Array.Empty(), Array.Empty(), Array.Empty>(), Array.Empty>(), Array.Empty>(), Array.Empty(), - Array.Empty(), default) + default) { } @@ -53,12 +52,10 @@ public AnalysisBasedMetadataManager( IEnumerable> reflectableMethods, IEnumerable> reflectableFields, IEnumerable reflectableAttributes, - IEnumerable rootedCctorContexts, MetadataManagerOptions options) : base(typeSystemContext, blockingPolicy, resourceBlockingPolicy, logFile, stackTracePolicy, invokeThunkGenerationPolicy, options) { _modulesWithMetadata = new List(modulesWithMetadata); - _typesWithRootedCctorContext = new List(rootedCctorContexts); _forcedTypes = new List(forcedTypes); foreach (var refType in reflectableTypes) @@ -205,11 +202,6 @@ void ICompilationRootProvider.AddCompilationRoots(IRootingServiceProvider rootPr rootProvider.AddReflectionRoot(field, reason); } } - - foreach (var type in _typesWithRootedCctorContext) - { - rootProvider.RootNonGCStaticBaseForType(type, reason); - } } private struct Policy : IMetadataPolicy diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMarker.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMarker.cs index f297a7cb73927..d4e81f23b0c80 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMarker.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMarker.cs @@ -206,15 +206,9 @@ internal void MarkStaticConstructor(in MessageOrigin origin, TypeDesc type, stri if (!_enabled) return; - if (type.HasStaticConstructor) - CheckAndWarnOnReflectionAccess(origin, type.GetStaticConstructor()); - - if (!type.IsGenericDefinition && !type.ContainsSignatureVariables(treatGenericParameterLikeSignatureVariable: true) && Factory.PreinitializationManager.HasLazyStaticConstructor(type)) - { - // Mark the GC static base - it contains a pointer to the class constructor, but also info - // about whether the class constructor already executed and it's what is looked at at runtime. - _dependencies.Add(Factory.TypeNonGCStaticsSymbol((MetadataType)type), "RunClassConstructor reference"); - } + MethodDesc cctor = type.GetStaticConstructor(); + if (cctor != null) + MarkMethod(origin, cctor, reason); } internal void CheckAndWarnOnReflectionAccess(in MessageOrigin origin, TypeSystemEntity entity, AccessKind accessKind = AccessKind.Unspecified) diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/MetadataManager.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/MetadataManager.cs index 96cfc811ca6be..2aaa93c9efdcc 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/MetadataManager.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/MetadataManager.cs @@ -463,6 +463,13 @@ public void GetDependenciesDueToReflectability(ref DependencyList dependencies, ReflectionInvokeSupportDependencyAlgorithm.GetDependenciesFromParamsArray(ref dependencies, factory, method); } + if (!method.IsCanonicalMethod(CanonicalFormKind.Any) && method.IsStaticConstructor) + { + // Information about the static constructor prefixes the non-GC static base + dependencies ??= new DependencyList(); + dependencies.Add(factory.TypeNonGCStaticsSymbol((MetadataType)method.OwningType), "Static constructor is reflection-callable"); + } + GenericMethodsTemplateMap.GetTemplateMethodDependencies(ref dependencies, factory, method); GenericTypesTemplateMap.GetTemplateTypeDependencies(ref dependencies, factory, method.OwningType); } diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/UsageBasedMetadataManager.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/UsageBasedMetadataManager.cs index 9c89ff530490d..991fe7aa62401 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/UsageBasedMetadataManager.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/UsageBasedMetadataManager.cs @@ -893,28 +893,10 @@ public MetadataManager ToAnalysisBasedMetadataManager() } } - var rootedCctorContexts = new List(); - foreach (NonGCStaticsNode cctorContext in GetCctorContextMapping()) - { - // If we generated a static constructor and the owning type, this might be something - // that gets fed to RuntimeHelpers.RunClassConstructor. RunClassConstructor - // also works on reflection blocked types and there is a possibility that we - // wouldn't have generated the cctor otherwise. - // - // This is a heuristic and we'll possibly root more cctor contexts than - // strictly necessary, but it's not worth introducing a new node type - // in the compiler just so we can propagate this knowledge from dataflow analysis - // (that detects RunClassConstructor usage) and this spot. - if (!TypeGeneratesEEType(cctorContext.Type)) - continue; - - rootedCctorContexts.Add(cctorContext.Type); - } - return new AnalysisBasedMetadataManager( _typeSystemContext, _blockingPolicy, _resourceBlockingPolicy, _metadataLogFile, _stackTraceEmissionPolicy, _dynamicInvokeThunkGenerationPolicy, _modulesWithMetadata, _typesWithForcedEEType, reflectableTypes.ToEnumerable(), reflectableMethods.ToEnumerable(), - reflectableFields.ToEnumerable(), _customAttributesWithMetadata, rootedCctorContexts, _options); + reflectableFields.ToEnumerable(), _customAttributesWithMetadata, _options); } private void AddDataflowDependency(ref DependencyList dependencies, NodeFactory factory, MethodIL methodIL, string reason) diff --git a/src/tests/nativeaot/SmokeTests/Reflection/Reflection.cs b/src/tests/nativeaot/SmokeTests/Reflection/Reflection.cs index 21889f70eab08..d5f64a61e9e33 100644 --- a/src/tests/nativeaot/SmokeTests/Reflection/Reflection.cs +++ b/src/tests/nativeaot/SmokeTests/Reflection/Reflection.cs @@ -2207,13 +2207,50 @@ static TypeWithNoStaticFieldsButACCtor() } } + class ClassWithCctor + { + static ClassWithCctor() => s_field = 42; + } + + class ClassWithCctor + { + static ClassWithCctor() => s_field = 11; + } + + class DynamicClassWithCctor + { + static DynamicClassWithCctor() => s_field = 1000; + } + + class Atom { } + private static bool s_cctorRan; + private static int s_field; public static void Run() { RuntimeHelpers.RunClassConstructor(typeof(TypeWithNoStaticFieldsButACCtor).TypeHandle); if (!s_cctorRan) throw new Exception(); + + RunTheCctor(typeof(ClassWithCctor)); + if (s_field != 42) + throw new Exception(); + + RunTheCctor(typeof(ClassWithCctor)); + if (s_field != 11) + throw new Exception(); + + RunTheCctor(typeof(DynamicClassWithCctor<>).MakeGenericType(GetAtom())); + if (s_field != 1000) + throw new Exception(); + + [MethodImpl(MethodImplOptions.NoInlining)] + static void RunTheCctor([DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.NonPublicConstructors)] Type t) + => RuntimeHelpers.RunClassConstructor(t.TypeHandle); + + [MethodImpl(MethodImplOptions.NoInlining)] + static Type GetAtom() => typeof(Atom); } }