From 5723747b7af24706a53fb8a1a5956469b35dbe38 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Thu, 2 Jun 2022 15:44:09 +0900 Subject: [PATCH 1/9] Enable IlcTrimMetadata by default Enables more aggressive metadata stripping by default. In this mode, the AOT compiler only generates reflection metadata for artifacts that were deemed reflection-visible by either dataflow analysis, or user inputs. Previously we would consider everything that got generated reflection-visible. * Change the defaults within the compiler (making the conservative mode opt-in). * Add tests for scenarios in https://github.com/dotnet/runtimelab/issues/656 (resolves dotnet/runtimelab#656). * Update tests --- .../Microsoft.NETCore.Native.targets | 2 +- .../Compiler/UsageBasedMetadataManager.cs | 12 +-- src/coreclr/tools/aot/ILCompiler/Program.cs | 8 +- .../nativeaot/SmokeTests/Dataflow/Dataflow.cs | 8 +- .../SmokeTests/DynamicGenerics/rd.xml | 21 ++++ .../Preinitialization.csproj | 6 ++ .../SmokeTests/Reflection/Reflection.cs | 95 +++++++++++++++++++ .../SmokeTests/Reflection/Reflection.csproj | 1 - ...nly.csproj => Reflection_FromUsage.csproj} | 7 +- 9 files changed, 142 insertions(+), 18 deletions(-) rename src/tests/nativeaot/SmokeTests/Reflection/{Reflection_ReflectedOnly.csproj => Reflection_FromUsage.csproj} (84%) diff --git a/src/coreclr/nativeaot/BuildIntegration/Microsoft.NETCore.Native.targets b/src/coreclr/nativeaot/BuildIntegration/Microsoft.NETCore.Native.targets index ef10a3e41d768..e72f44e6e24a0 100644 --- a/src/coreclr/nativeaot/BuildIntegration/Microsoft.NETCore.Native.targets +++ b/src/coreclr/nativeaot/BuildIntegration/Microsoft.NETCore.Native.targets @@ -238,7 +238,7 @@ The .NET Foundation licenses this file to you under the MIT license. - + diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/UsageBasedMetadataManager.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/UsageBasedMetadataManager.cs index ef615ac558bd1..ff393b426e2a1 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/UsageBasedMetadataManager.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/UsageBasedMetadataManager.cs @@ -173,7 +173,7 @@ protected override MetadataCategory GetMetadataCategory(TypeDesc type) return category; } - protected override bool AllMethodsCanBeReflectable => (_generationOptions & UsageBasedMetadataGenerationOptions.ReflectedMembersOnly) == 0; + protected override bool AllMethodsCanBeReflectable => (_generationOptions & UsageBasedMetadataGenerationOptions.CreateReflectableArtifacts) != 0; protected override void ComputeMetadata(NodeFactory factory, out byte[] metadataBlob, @@ -494,7 +494,7 @@ protected override void GetDependenciesDueToMethodCodePresenceInternal(ref Depen } // Presence of code might trigger the reflectability dependencies. - if ((_generationOptions & UsageBasedMetadataGenerationOptions.ReflectedMembersOnly) == 0) + if ((_generationOptions & UsageBasedMetadataGenerationOptions.CreateReflectableArtifacts) != 0) { GetDependenciesDueToReflectability(ref dependencies, factory, method); } @@ -505,7 +505,7 @@ public override void GetConditionalDependenciesDueToMethodCodePresence(ref Combi MethodDesc typicalMethod = method.GetTypicalMethodDefinition(); // Ensure methods with genericness have the same reflectability by injecting a conditional dependency. - if ((_generationOptions & UsageBasedMetadataGenerationOptions.ReflectedMembersOnly) != 0 + if ((_generationOptions & UsageBasedMetadataGenerationOptions.CreateReflectableArtifacts) == 0 && method != typicalMethod) { dependencies ??= new CombinedDependencyList(); @@ -516,7 +516,7 @@ public override void GetConditionalDependenciesDueToMethodCodePresence(ref Combi public override void GetDependenciesDueToVirtualMethodReflectability(ref DependencyList dependencies, NodeFactory factory, MethodDesc method) { - if ((_generationOptions & UsageBasedMetadataGenerationOptions.ReflectedMembersOnly) == 0) + if ((_generationOptions & UsageBasedMetadataGenerationOptions.CreateReflectableArtifacts) != 0) { // If we have a use of an abstract method, GetDependenciesDueToReflectability is not going to see the method // as being used since there's no body. We inject a dependency on a new node that serves as a logical method body @@ -1028,9 +1028,9 @@ public enum UsageBasedMetadataGenerationOptions ReflectionILScanning = 4, /// - /// Only members that were seen as reflected on will be reflectable. + /// Consider all native artifacts (native method bodies, etc) visible from reflection. /// - ReflectedMembersOnly = 8, + CreateReflectableArtifacts = 8, /// /// Fully root used assemblies that are not marked IsTrimmable in metadata. diff --git a/src/coreclr/tools/aot/ILCompiler/Program.cs b/src/coreclr/tools/aot/ILCompiler/Program.cs index 647ee1f7a9eeb..033ed85ed455a 100644 --- a/src/coreclr/tools/aot/ILCompiler/Program.cs +++ b/src/coreclr/tools/aot/ILCompiler/Program.cs @@ -55,7 +55,7 @@ internal class Program private bool _noMetadataBlocking; private bool _disableReflection; private bool _completeTypesMetadata; - private bool _reflectedOnly; + private bool _reflectableartifacts; private bool _scanReflection; private bool _methodBodyFolding; private int _parallelism = Environment.ProcessorCount; @@ -203,7 +203,7 @@ private ArgumentSyntax ParseCommandLine(string[] args) syntax.DefineOption("nometadatablocking", ref _noMetadataBlocking, "Ignore metadata blocking for internal implementation details"); syntax.DefineOption("disablereflection", ref _disableReflection, "Disable generation of reflection metadata"); syntax.DefineOption("completetypemetadata", ref _completeTypesMetadata, "Generate complete metadata for types"); - syntax.DefineOption("reflectedonly", ref _reflectedOnly, "Generate metadata only for reflected members"); + syntax.DefineOption("reflectableartifacts", ref _reflectableartifacts, "Consider generated native arifacts visible from reflection"); syntax.DefineOption("scanreflection", ref _scanReflection, "Scan IL for reflection patterns"); syntax.DefineOption("scan", ref _useScanner, "Use IL scanner to generate optimized code (implied by -O)"); syntax.DefineOption("noscan", ref _noScanner, "Do not use IL scanner to generate optimized code"); @@ -758,8 +758,8 @@ static string ILLinkify(string rootedAssembly) metadataGenerationOptions |= UsageBasedMetadataGenerationOptions.CompleteTypesOnly; if (_scanReflection) metadataGenerationOptions |= UsageBasedMetadataGenerationOptions.ReflectionILScanning; - if (_reflectedOnly) - metadataGenerationOptions |= UsageBasedMetadataGenerationOptions.ReflectedMembersOnly; + if (_reflectableartifacts) + metadataGenerationOptions |= UsageBasedMetadataGenerationOptions.CreateReflectableArtifacts; if (_rootDefaultAssemblies) metadataGenerationOptions |= UsageBasedMetadataGenerationOptions.RootDefaultAssemblies; } diff --git a/src/tests/nativeaot/SmokeTests/Dataflow/Dataflow.cs b/src/tests/nativeaot/SmokeTests/Dataflow/Dataflow.cs index da7578fed1bf2..45bedc5a2165b 100644 --- a/src/tests/nativeaot/SmokeTests/Dataflow/Dataflow.cs +++ b/src/tests/nativeaot/SmokeTests/Dataflow/Dataflow.cs @@ -389,7 +389,10 @@ public static void Run() Assert.Equal(1, typeof(TypeWithSpecificMethodKept).CountMethods()); Assert.Equal(1, typeof(TypeWithSpecificOverloadKept).CountMethods()); Assert.Equal(2, typeof(TypeWithAllOverloadsKept).CountMethods()); - Assert.Equal(2, typeof(TestDynamicDependency).CountMethods()); + + // We only expect DependentMethod. We specifically don't expect to see the Run method (current method). + Assert.Equal(1, typeof(TestDynamicDependency).CountMethods()); + Assert.Equal(1, typeof(TypeWithPublicPropertiesKept).CountProperties()); } } @@ -577,7 +580,8 @@ public static void Run() { // Check we detect these as intrinsics IntPtr pBytes = Marshal.AllocHGlobal(Marshal.SizeOf(typeof(ClassWithLayout1))); - object o = Marshal.PtrToStructure(pBytes, typeof(ClassWithLayout2)); + // https://github.com/dotnet/runtime/issues/70072 + //object o = Marshal.PtrToStructure(pBytes, typeof(ClassWithLayout2)); Marshal.DestroyStructure(pBytes, typeof(ClassWithLayout3)); Marshal.OffsetOf(typeof(ClassWithLayout4), "Field"); diff --git a/src/tests/nativeaot/SmokeTests/DynamicGenerics/rd.xml b/src/tests/nativeaot/SmokeTests/DynamicGenerics/rd.xml index 5583cf52b7e91..92d116b2a139f 100644 --- a/src/tests/nativeaot/SmokeTests/DynamicGenerics/rd.xml +++ b/src/tests/nativeaot/SmokeTests/DynamicGenerics/rd.xml @@ -113,6 +113,27 @@ + + + + + + + + + + + + + + + + + + + + + diff --git a/src/tests/nativeaot/SmokeTests/Preinitialization/Preinitialization.csproj b/src/tests/nativeaot/SmokeTests/Preinitialization/Preinitialization.csproj index f0f876b0bb4ad..8f2a5aa349d58 100644 --- a/src/tests/nativeaot/SmokeTests/Preinitialization/Preinitialization.csproj +++ b/src/tests/nativeaot/SmokeTests/Preinitialization/Preinitialization.csproj @@ -5,6 +5,12 @@ 0 true true + + + false diff --git a/src/tests/nativeaot/SmokeTests/Reflection/Reflection.cs b/src/tests/nativeaot/SmokeTests/Reflection/Reflection.cs index ab19c6f35d510..9e8107fadcec5 100644 --- a/src/tests/nativeaot/SmokeTests/Reflection/Reflection.cs +++ b/src/tests/nativeaot/SmokeTests/Reflection/Reflection.cs @@ -52,6 +52,10 @@ private static int Main() TestNotReflectedIsNotReflectable.Run(); TestGenericInstantiationsAreEquallyReflectable.Run(); #endif + TestAttributeInheritance2.Run(); + TestInvokeMethodMetadata.Run(); + TestVTableOfNullableUnderlyingTypes.Run(); + TestInterfaceLists.Run(); // // Mostly functionality tests @@ -1676,6 +1680,97 @@ public static void Run() } } + class TestAttributeInheritance2 + { + [AttributeUsage(AttributeTargets.All, Inherited = true)] + class AttAttribute : Attribute { } + + class Base + { + [Att] + public virtual void VirtualMethodWithAttribute() { } + } + + class Derived : Base + { + public override void VirtualMethodWithAttribute() { } + } + + public static void Run() + { + object[] attrs = typeof(Derived).GetMethod(nameof(Derived.VirtualMethodWithAttribute)).GetCustomAttributes(inherit: true); + if (attrs.Length != 1 || attrs[0].GetType().Name != nameof(AttAttribute)) + { + throw new Exception(); + } + } + } + + class TestInvokeMethodMetadata + { + delegate int WithDefaultParameter1(int value = 1234); + delegate DateTime WithDefaultParameter2(DateTime value); + + public static int Method(int value) => value; + + public static DateTime Method(DateTime value) => value; + + public static void Run() + { + // Check that we have metadata for the Invoke method to convert Type.Missing to the actual value. + WithDefaultParameter1 del1 = Method; + int val = (int)del1.DynamicInvoke(new object[] { Type.Missing }); + if (val != 1234) + throw new Exception(); + + // Check that we have metadata for the Invoke method to find a matching method + Delegate del2 = Delegate.CreateDelegate(typeof(WithDefaultParameter2), typeof(TestInvokeMethodMetadata), nameof(Method)); + if (del2.Method.ReturnType != typeof(DateTime)) + throw new Exception(); + } + } + + class TestVTableOfNullableUnderlyingTypes + { + struct NeverAllocated + { + public override string ToString() => "Never allocated"; + } + + static Type s_hidden = typeof(Nullable); + + public static void Run() + { + // Underlying type of a Nullable needs to be considered constructed. + // Trimming warning suppressions in the libraries depend on this invariant. + var instance = RuntimeHelpers.GetUninitializedObject(s_hidden); + if (instance.ToString() != "Never allocated") + throw new Exception(); + } + } + + class TestInterfaceLists + { + interface IGeneric { } + + class Class : IGeneric { } + + static Type s_hidden = typeof(Class); + + public static void Run() + { + // Can't drop an interface from the interface list if the interface is referenced. + // Trimming warning suppressions in the libraries depend on this invariant. + foreach (var intface in s_hidden.GetInterfaces()) + { + if (intface.HasSameMetadataDefinitionAs(typeof(IGeneric))) + return; + } + + throw new Exception(); + } + } + #region Helpers private static Type GetTestType(string testName, string typeName) diff --git a/src/tests/nativeaot/SmokeTests/Reflection/Reflection.csproj b/src/tests/nativeaot/SmokeTests/Reflection/Reflection.csproj index c386cd514b05f..8d424f6fc1213 100644 --- a/src/tests/nativeaot/SmokeTests/Reflection/Reflection.csproj +++ b/src/tests/nativeaot/SmokeTests/Reflection/Reflection.csproj @@ -4,7 +4,6 @@ BuildAndRun 0 true - $(DefineConstants);REFLECTION_FROM_USAGE true diff --git a/src/tests/nativeaot/SmokeTests/Reflection/Reflection_ReflectedOnly.csproj b/src/tests/nativeaot/SmokeTests/Reflection/Reflection_FromUsage.csproj similarity index 84% rename from src/tests/nativeaot/SmokeTests/Reflection/Reflection_ReflectedOnly.csproj rename to src/tests/nativeaot/SmokeTests/Reflection/Reflection_FromUsage.csproj index 166979bc9c369..584e823c5e0d0 100644 --- a/src/tests/nativeaot/SmokeTests/Reflection/Reflection_ReflectedOnly.csproj +++ b/src/tests/nativeaot/SmokeTests/Reflection/Reflection_FromUsage.csproj @@ -4,6 +4,7 @@ BuildAndRun 0 true + $(DefineConstants);REFLECTION_FROM_USAGE true @@ -11,12 +12,10 @@ true + + false - - - - From 7c5d4db255b02bd49d038f62c164d9a7d261fa2f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Wed, 8 Jun 2022 15:53:12 +0900 Subject: [PATCH 2/9] FB --- .../Microsoft.NETCore.Native.targets | 4 ++-- src/coreclr/nativeaot/docs/optimizing.md | 19 +++++++++---------- src/coreclr/tools/aot/ILCompiler/Program.cs | 17 +++++++++++------ 3 files changed, 22 insertions(+), 18 deletions(-) diff --git a/src/coreclr/nativeaot/BuildIntegration/Microsoft.NETCore.Native.targets b/src/coreclr/nativeaot/BuildIntegration/Microsoft.NETCore.Native.targets index 4caf03d0395f5..a49cccedb8379 100644 --- a/src/coreclr/nativeaot/BuildIntegration/Microsoft.NETCore.Native.targets +++ b/src/coreclr/nativeaot/BuildIntegration/Microsoft.NETCore.Native.targets @@ -235,14 +235,14 @@ The .NET Foundation licenses this file to you under the MIT license. - + - + diff --git a/src/coreclr/nativeaot/docs/optimizing.md b/src/coreclr/nativeaot/docs/optimizing.md index aeee66fa2f41d..cc31c98df1539 100644 --- a/src/coreclr/nativeaot/docs/optimizing.md +++ b/src/coreclr/nativeaot/docs/optimizing.md @@ -12,12 +12,6 @@ To specify a switch, add a new property to your project file with one or more of under the `` node of your project file. -## Options related to library features - -Native AOT supports enabling and disabling all [documented framework library features](https://docs.microsoft.com/en-us/dotnet/core/deploying/trimming-options#trimming-framework-library-features). For example, to remove globalization specific code and data, add a `true` property to your project. Disabling a framework feature (or enabling a minimal mode of the feature) can result in significant size savings. - -🛈 Native AOT difference: The `EnableUnsafeBinaryFormatterSerialization` framework switch is already set to the optimal value of `false` (removing the support for [obsolete](https://github.com/dotnet/designs/blob/21b274dbc21e4ae54b7e4c5dbd5ef31e439e78db/accepted/2020/better-obsoletion/binaryformatter-obsoletion.md) binary serialization). - ## Options related to trimming The Native AOT compiler supports the [documented options](https://docs.microsoft.com/en-us/dotnet/core/deploying/trim-self-contained) for removing unused code (trimming). By default, the compiler tries to very conservatively remove some of the unused code. @@ -26,16 +20,21 @@ The Native AOT compiler supports the [documented options](https://docs.microsoft By default, the compiler tries to maximize compatibility with existing .NET code at the expense of compilation speed and size of the output executable. This allows people to use their existing code that worked well in a fully dynamic mode without hitting issues caused by trimming. To read more about reflection, see the [Reflection in AOT mode](reflection-in-aot-mode.md) document. -🛈 Native AOT difference: the `TrimMode` of framework assemblies is set to `link` by default. To compile entire framework assemblies, use `TrimmerRootAssembly` to root the selected assemblies. It's not recommended to root the entire framework. - -To enable more aggressive removal of unreferenced code, set the `` property to `link`. +To enable more aggressive removal of unreferenced code, set the `` property to `link`. To aid in troubleshooting some of the most common problems related to trimming add `true` to your project. This ensures types are preserved in their entirety, but the extra members that would otherwise be trimmed cannot be used in runtime reflection. This mode can turn some spurious `NullReferenceExceptions` (caused by reflection APIs returning a null) caused by trimming into more actionable exceptions. +The Native AOT compiler can remove unused metadata more effectively than non-Native deployment models. For example, it's possible to remove names and metadata for methods while keeping the native code of the method. The higher efficiency of trimming in Native AOT can result in differences in what's visible to reflection at runtime in trimming-unfriendly code. To increase compatibility with the less efficient non-Native trimming, set the `` property to `false`. This compatibility mode is not necessary if there are no trimming warnings. + +## Options related to library features + +Native AOT supports enabling and disabling all [documented framework library features](https://docs.microsoft.com/en-us/dotnet/core/deploying/trimming-options#trimming-framework-library-features). For example, to remove globalization specific code and data, add a `true` property to your project. Disabling a framework feature (or enabling a minimal mode of the feature) can result in significant size savings. + +Since `PublishTrimmed` is implied to be true with Native AOT, some framework features such as binary serialization are disabled by default. + ## Options related to metadata generation * `false`: this disables generation of stack trace metadata that provides textual names in stack traces. This is for example the text string one gets by calling `Exception.ToString()` on a caught exception. With this option disabled, stack traces will still be generated, but will be based on reflection metadata alone (they might be less complete). -* `true`: allows the compiler to remove reflection metadata from things that were not visible targets of reflection. By default, the compiler keeps metadata for everything that was compiled. With this option turned on, reflection metadata (and therefore reflection) will only be available for visible targets of reflection. Visible targets of reflection are things like assemblies rooted from the project file, RD.XML, ILLinkTrim descriptors, DynamicallyAccessedMembers annotations or DynamicDependency annotations. ## Options related to code generation * `Speed`: when generating optimized code, favor code execution speed. diff --git a/src/coreclr/tools/aot/ILCompiler/Program.cs b/src/coreclr/tools/aot/ILCompiler/Program.cs index bab03035eda9e..eb3643851fd50 100644 --- a/src/coreclr/tools/aot/ILCompiler/Program.cs +++ b/src/coreclr/tools/aot/ILCompiler/Program.cs @@ -53,9 +53,8 @@ internal class Program private string _mapFileName; private string _metadataLogFileName; private bool _noMetadataBlocking; - private bool _disableReflection; + private string _reflectionData; private bool _completeTypesMetadata; - private bool _reflectableartifacts; private bool _scanReflection; private bool _methodBodyFolding; private int _parallelism = Environment.ProcessorCount; @@ -159,6 +158,8 @@ private void InitializeDefaultOptions() private ArgumentSyntax ParseCommandLine(string[] args) { + var validReflectionDataOptions = new string[] { "all", "none" }; + IReadOnlyList inputFiles = Array.Empty(); IReadOnlyList referenceFiles = Array.Empty(); @@ -201,9 +202,8 @@ private ArgumentSyntax ParseCommandLine(string[] args) syntax.DefineOption("map", ref _mapFileName, "Generate a map file"); syntax.DefineOption("metadatalog", ref _metadataLogFileName, "Generate a metadata log file"); syntax.DefineOption("nometadatablocking", ref _noMetadataBlocking, "Ignore metadata blocking for internal implementation details"); - syntax.DefineOption("disablereflection", ref _disableReflection, "Disable generation of reflection metadata"); syntax.DefineOption("completetypemetadata", ref _completeTypesMetadata, "Generate complete metadata for types"); - syntax.DefineOption("reflectableartifacts", ref _reflectableartifacts, "Consider generated native arifacts visible from reflection"); + syntax.DefineOption("reflectiondata", ref _reflectionData, $"Reflection data to generate (one of: {string.Join(", ", validReflectionDataOptions)})"); syntax.DefineOption("scanreflection", ref _scanReflection, "Scan IL for reflection patterns"); syntax.DefineOption("scan", ref _useScanner, "Use IL scanner to generate optimized code (implied by -O)"); syntax.DefineOption("noscan", ref _noScanner, "Do not use IL scanner to generate optimized code"); @@ -342,6 +342,11 @@ private ArgumentSyntax ParseCommandLine(string[] args) Helpers.MakeReproPackage(_makeReproPath, _outputFilePath, args, argSyntax, new[] { "-r", "-m", "--rdxml", "--directpinvokelist" }); } + if (_reflectionData != null && Array.IndexOf(validReflectionDataOptions, _reflectionData) < 0) + { + Console.WriteLine($"Warning: option '{_reflectionData}' not recognized"); + } + return argSyntax; } @@ -527,7 +532,7 @@ private int Run(string[] args) InstructionSetSupportBuilder.GetNonSpecifiableInstructionSetsForArch(_targetArchitecture), _targetArchitecture); - bool supportsReflection = !_disableReflection && _systemModuleName == DefaultSystemModule; + bool supportsReflection = _reflectionData != "none" && _systemModuleName == DefaultSystemModule; // // Initialize type system context @@ -758,7 +763,7 @@ static string ILLinkify(string rootedAssembly) metadataGenerationOptions |= UsageBasedMetadataGenerationOptions.CompleteTypesOnly; if (_scanReflection) metadataGenerationOptions |= UsageBasedMetadataGenerationOptions.ReflectionILScanning; - if (_reflectableartifacts) + if (_reflectionData == "all") metadataGenerationOptions |= UsageBasedMetadataGenerationOptions.CreateReflectableArtifacts; if (_rootDefaultAssemblies) metadataGenerationOptions |= UsageBasedMetadataGenerationOptions.RootDefaultAssemblies; From 887876ab176afb6ec696f0b4deee91f7c107a1e6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Fri, 10 Jun 2022 19:39:29 +0900 Subject: [PATCH 3/9] Fix PtrToStructure --- .../Compiler/Dataflow/ReflectionMethodBodyScanner.cs | 6 ++++++ src/tests/nativeaot/SmokeTests/Dataflow/Dataflow.cs | 3 +-- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMethodBodyScanner.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMethodBodyScanner.cs index f584f7c5b2023..4343e5352da3b 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMethodBodyScanner.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMethodBodyScanner.cs @@ -597,6 +597,12 @@ public override bool HandleCall(MethodIL callingMethodBody, MethodDesc calledMet if (systemTypeValue.RepresentedType.Type.IsDefType) { _reflectionMarker.Dependencies.Add(_factory.StructMarshallingData((DefType)systemTypeValue.RepresentedType.Type), "Marshal API"); + if (intrinsicId == IntrinsicId.Marshal_PtrToStructure + && systemTypeValue.RepresentedType.Type.GetParameterlessConstructor() is MethodDesc ctorMethod + && !_factory.MetadataManager.IsReflectionBlocked(ctorMethod)) + { + _reflectionMarker.Dependencies.Add(_factory.ReflectableMethod(ctorMethod), "Marshal API"); + } } } else diff --git a/src/tests/nativeaot/SmokeTests/Dataflow/Dataflow.cs b/src/tests/nativeaot/SmokeTests/Dataflow/Dataflow.cs index 45bedc5a2165b..02c00131e3513 100644 --- a/src/tests/nativeaot/SmokeTests/Dataflow/Dataflow.cs +++ b/src/tests/nativeaot/SmokeTests/Dataflow/Dataflow.cs @@ -580,8 +580,7 @@ public static void Run() { // Check we detect these as intrinsics IntPtr pBytes = Marshal.AllocHGlobal(Marshal.SizeOf(typeof(ClassWithLayout1))); - // https://github.com/dotnet/runtime/issues/70072 - //object o = Marshal.PtrToStructure(pBytes, typeof(ClassWithLayout2)); + object o = Marshal.PtrToStructure(pBytes, typeof(ClassWithLayout2)); Marshal.DestroyStructure(pBytes, typeof(ClassWithLayout3)); Marshal.OffsetOf(typeof(ClassWithLayout4), "Field"); From dd2738b625595e13838dfa1815c5c51d60a8e832 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Mon, 13 Jun 2022 13:39:32 +0900 Subject: [PATCH 4/9] Merge from main --- .../ILCompiler.Compiler/Compiler/UsageBasedMetadataManager.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/UsageBasedMetadataManager.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/UsageBasedMetadataManager.cs index 4aba8932e58d1..36125cd01943f 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/UsageBasedMetadataManager.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/UsageBasedMetadataManager.cs @@ -600,7 +600,7 @@ public override void GetDependenciesDueToAccess(ref DependencyList dependencies, dependencies.Add(factory.DataflowAnalyzedMethod(methodIL.GetMethodILDefinition()), "Access to interesting field"); } - if ((_generationOptions & UsageBasedMetadataGenerationOptions.ReflectedMembersOnly) == 0 + if ((_generationOptions & UsageBasedMetadataGenerationOptions.CreateReflectableArtifacts) != 0 && !IsReflectionBlocked(writtenField)) { FieldDesc fieldToReport = writtenField; From a2def8c423d5a1a7086d411f2dc60a03ac0a04d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Tue, 14 Jun 2022 10:10:22 +0900 Subject: [PATCH 5/9] Consider some array element types constructed --- .../DependencyAnalysis/CanonicalEETypeNode.cs | 5 ++ .../ConstructedEETypeNode.cs | 5 ++ .../Compiler/DependencyAnalysis/EETypeNode.cs | 19 +++++- .../DeadCodeElimination.cs | 63 +++++++++++++++++++ 4 files changed, 90 insertions(+), 2 deletions(-) diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/CanonicalEETypeNode.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/CanonicalEETypeNode.cs index 845fe201486d3..e2df7d38798e3 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/CanonicalEETypeNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/CanonicalEETypeNode.cs @@ -77,6 +77,11 @@ protected override ISymbolNode GetBaseTypeNode(NodeFactory factory) return _type.BaseType != null ? factory.NecessaryTypeSymbol(_type.BaseType.NormalizeInstantiation()) : null; } + protected override ISymbolNode GetNonNullableValueTypeArrayElementTypeNode(NodeFactory factory) + { + return factory.ConstructedTypeSymbol(((ArrayType)_type).ElementType); + } + protected override int GCDescSize { get diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ConstructedEETypeNode.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ConstructedEETypeNode.cs index 4c5d630437a3b..be5ce0d0dc80b 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ConstructedEETypeNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ConstructedEETypeNode.cs @@ -101,6 +101,11 @@ protected override ISymbolNode GetBaseTypeNode(NodeFactory factory) return _type.BaseType != null ? factory.ConstructedTypeSymbol(_type.BaseType) : null; } + protected override ISymbolNode GetNonNullableValueTypeArrayElementTypeNode(NodeFactory factory) + { + return factory.ConstructedTypeSymbol(((ArrayType)_type).ElementType); + } + protected override IEETypeNode GetInterfaceTypeNode(NodeFactory factory, TypeDesc interfaceType) { // The interface type will be visible to reflection and should be considered constructed. 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 0ec69a4f29f76..54b6424032c5b 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/EETypeNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/EETypeNode.cs @@ -757,14 +757,29 @@ protected virtual ISymbolNode GetBaseTypeNode(NodeFactory factory) return _type.BaseType != null ? factory.NecessaryTypeSymbol(_type.BaseType) : null; } + protected virtual ISymbolNode GetNonNullableValueTypeArrayElementTypeNode(NodeFactory factory) + { + return factory.NecessaryTypeSymbol(((ArrayType)_type).ElementType); + } + private ISymbolNode GetRelatedTypeNode(NodeFactory factory) { ISymbolNode relatedTypeNode = null; - if (_type.IsArray || _type.IsPointer || _type.IsByRef) + if (_type.IsParameterizedType) { var parameterType = ((ParameterizedType)_type).ParameterType; - relatedTypeNode = factory.NecessaryTypeSymbol(parameterType); + if (_type.IsArray && parameterType.IsValueType && !parameterType.IsNullable) + { + // This might be a constructed type symbol. There are APIs on Array that allow allocating element + // types through runtime magic ("((Array)new NeverAllocated[1]).GetValue(0)" or IEnumerable) and we don't have + // visibility into that. Conservatively assume element types of constructed arrays are also constructed. + relatedTypeNode = GetNonNullableValueTypeArrayElementTypeNode(factory); + } + else + { + relatedTypeNode = factory.NecessaryTypeSymbol(parameterType); + } } else { diff --git a/src/tests/nativeaot/SmokeTests/DeadCodeElimination/DeadCodeElimination.cs b/src/tests/nativeaot/SmokeTests/DeadCodeElimination/DeadCodeElimination.cs index f133eb28f5d2b..b371e2469c24b 100644 --- a/src/tests/nativeaot/SmokeTests/DeadCodeElimination/DeadCodeElimination.cs +++ b/src/tests/nativeaot/SmokeTests/DeadCodeElimination/DeadCodeElimination.cs @@ -17,6 +17,7 @@ static int Main() TestAbstractNeverDerivedWithDevirtualizedCall.Run(); TestAbstractDerivedByUnrelatedTypeWithDevirtualizedCall.Run(); TestUnusedDefaultInterfaceMethod.Run(); + TestArrayElementTypeOperations.Run(); return 100; } @@ -219,6 +220,60 @@ public static void Run() } } + class TestArrayElementTypeOperations + { + public static void Run() + { + Console.WriteLine("Testing array element type optimizations"); + + // We consider valuetype elements of arrays constructed... + { + Array arr = new NeverAllocated1[1]; + ThrowIfNotPresent(typeof(TestArrayElementTypeOperations), nameof(Marker1)); + + // The reason they're considered constructed is runtime magic here + // Make sure that works too. + object o = arr.GetValue(0); + if (!o.ToString().Contains(nameof(Marker1))) + throw new Exception(); + } + + // ...but not nullable... + { + Array arr = new Nullable[1]; + arr.GetValue(0); + ThrowIfPresent(typeof(TestArrayElementTypeOperations), nameof(Marker2)); + } + + + // ...or reference type element types + { + Array arr = new NeverAllocated3[1]; + arr.GetValue(0); + ThrowIfPresent(typeof(TestArrayElementTypeOperations), nameof(Marker3)); + } + } + + class Marker1 { } + struct NeverAllocated1 + { + public override string ToString() => typeof(Marker1).ToString(); + } + + class Marker2 { } + struct NeverAllocated2 + { + public override string ToString() => typeof(Marker2).ToString(); + } + + class Marker3 { } + class NeverAllocated3 + { + public override string ToString() => typeof(Marker3).ToString(); + } + } + + [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2070:UnrecognizedReflectionPattern", Justification = "That's the point")] private static bool IsTypePresent(Type testType, string typeName) => testType.GetNestedType(typeName, BindingFlags.NonPublic | BindingFlags.Public) != null; @@ -230,4 +285,12 @@ private static void ThrowIfPresent(Type testType, string typeName) throw new Exception(typeName); } } + + private static void ThrowIfNotPresent(Type testType, string typeName) + { + if (!IsTypePresent(testType, typeName)) + { + throw new Exception(typeName); + } + } } From e64bb3111883f55392b7b06b2573c2b8c25a8a30 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Tue, 14 Jun 2022 10:33:20 +0900 Subject: [PATCH 6/9] Fix the tests reflecting on collection privates --- src/libraries/System.Collections/tests/default.rd.xml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/libraries/System.Collections/tests/default.rd.xml b/src/libraries/System.Collections/tests/default.rd.xml index eaeb64ebc3137..cff6d074fe9ae 100644 --- a/src/libraries/System.Collections/tests/default.rd.xml +++ b/src/libraries/System.Collections/tests/default.rd.xml @@ -150,5 +150,11 @@ + + + + + + From c8eafd2c5705bdae64c4d311cb946e720b6813f7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Wed, 15 Jun 2022 10:14:42 +0900 Subject: [PATCH 7/9] Keep referenced fields on System.Attribute --- .../Compiler/UsageBasedMetadataManager.cs | 38 +++++++++++++++++-- 1 file changed, 35 insertions(+), 3 deletions(-) diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/UsageBasedMetadataManager.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/UsageBasedMetadataManager.cs index 36125cd01943f..d11c8c8b92886 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/UsageBasedMetadataManager.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/UsageBasedMetadataManager.cs @@ -600,8 +600,40 @@ public override void GetDependenciesDueToAccess(ref DependencyList dependencies, dependencies.Add(factory.DataflowAnalyzedMethod(methodIL.GetMethodILDefinition()), "Access to interesting field"); } - if ((_generationOptions & UsageBasedMetadataGenerationOptions.CreateReflectableArtifacts) != 0 - && !IsReflectionBlocked(writtenField)) + string reason = "Use of a field"; + + bool generatesMetadata = false; + if (!IsReflectionBlocked(writtenField)) + { + if ((_generationOptions & UsageBasedMetadataGenerationOptions.CreateReflectableArtifacts) != 0) + { + // If access to the field should trigger metadata generation, we should generate the field + generatesMetadata = true; + } + else + { + // There's an invalid suppression in the CoreLib that assumes used fields on attributes will be kept. + // It's used in the reflection-based implementation of Attribute.Equals and Attribute.GetHashCode. + // .NET Native used to have a non-reflection based implementation of Equals/GetHashCode to get around + // this problem. We could explore that as well, but for now, emulate the fact that accessed fields + // on custom attributes will be visible in reflection metadata. + MetadataType currentType = (MetadataType)writtenField.OwningType.BaseType; + while (currentType != null) + { + if (currentType.Module == factory.TypeSystemContext.SystemModule + && currentType.Name == "Attribute" && currentType.Namespace == "System") + { + generatesMetadata = true; + reason = "Field of an attribute"; + break; + } + + currentType = currentType.MetadataBaseType; + } + } + } + + if (generatesMetadata) { FieldDesc fieldToReport = writtenField; @@ -619,7 +651,7 @@ public override void GetDependenciesDueToAccess(ref DependencyList dependencies, } dependencies = dependencies ?? new DependencyList(); - dependencies.Add(factory.ReflectableField(fieldToReport), "Use of a field"); + dependencies.Add(factory.ReflectableField(fieldToReport), reason); } } From 94617ddd70347150d844844e055bb64dfcf009d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Wed, 15 Jun 2022 10:14:59 +0900 Subject: [PATCH 8/9] Fix one more test that reflects on implementation details --- src/libraries/System.Runtime/tests/default.rd.xml | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libraries/System.Runtime/tests/default.rd.xml b/src/libraries/System.Runtime/tests/default.rd.xml index b100128a1bd61..b78da0bb4022b 100644 --- a/src/libraries/System.Runtime/tests/default.rd.xml +++ b/src/libraries/System.Runtime/tests/default.rd.xml @@ -501,6 +501,7 @@ + From ef1831dd89819d12f2760d6bae7e8cf2d40a3ccf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Wed, 15 Jun 2022 17:25:40 +0900 Subject: [PATCH 9/9] Fix the tests More reflection that only worked by accident. The code is not trim-safe. --- .../tests/default.rd.xml | 18 ++++++++++++++++++ .../System.Reflection/tests/default.rd.xml | 4 ++++ 2 files changed, 22 insertions(+) diff --git a/src/libraries/System.Linq.Expressions/tests/default.rd.xml b/src/libraries/System.Linq.Expressions/tests/default.rd.xml index ab2f88341472c..0babcd014cec3 100644 --- a/src/libraries/System.Linq.Expressions/tests/default.rd.xml +++ b/src/libraries/System.Linq.Expressions/tests/default.rd.xml @@ -1,8 +1,18 @@ + + + + + + + + + + @@ -32,6 +42,13 @@ + + + + + + + @@ -45,6 +62,7 @@ + diff --git a/src/libraries/System.Reflection/tests/default.rd.xml b/src/libraries/System.Reflection/tests/default.rd.xml index de87619639313..31a940f6e8540 100644 --- a/src/libraries/System.Reflection/tests/default.rd.xml +++ b/src/libraries/System.Reflection/tests/default.rd.xml @@ -47,5 +47,9 @@ + + + +