Enable IlcTrimMetadata by default (dotnet#70201)

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 dotnet/runtimelab#656 (resolves dotnet/runtimelab#656).
* Update tests
* Do more marking where it was necessary

src/coreclr/nativeaot/BuildIntegration/Microsoft.NETCore.Native.targets

@@ -232,14 +232,14 @@ The .NET Foundation licenses this file to you under the MIT license.
       <IlcArg Condition="$(Optimize) == 'true' and $(IlcOptimizationPreference) == 'Size'" Include="--Os" />
       <IlcArg Condition="$(Optimize) == 'true' and $(IlcOptimizationPreference) == 'Speed'" Include="--Ot" />
       <IlcArg Condition="$(IlcInstructionSet) != ''" Include="--instructionset:$(IlcInstructionSet)" />
-      <IlcArg Condition="$(IlcDisableReflection) == 'true'" Include="--disablereflection" />
+      <IlcArg Condition="$(IlcDisableReflection) == 'true'" Include="--reflectiondata:none" />
       <IlcArg Condition="$(IlcDisableReflection) == 'true'" Include="--feature:System.Collections.Generic.DefaultComparers=false" />
       <IlcArg Condition="$(IlcSingleThreaded) == 'true'" Include="--parallelism:1" />
       <IlcArg Condition="$(IlcSystemModule) != ''" Include="--systemmodule:$(IlcSystemModule)" />
       <IlcArg Condition="$(IlcDumpIL) == 'true'" Include="--ildump:$(NativeIntermediateOutputPath)%(ManagedBinary.Filename).il" />
       <IlcArg Condition="$(NoWarn) != ''" Include='--nowarn:"$([MSBuild]::Escape($(NoWarn)))"' />
       <IlcArg Condition="$(TrimmerSingleWarn) == 'true'" Include="--singlewarn" />
-      <IlcArg Condition="$(IlcTrimMetadata) == 'true'" Include="--reflectedonly" />
+      <IlcArg Condition="$(IlcTrimMetadata) == 'false'" Include="--reflectiondata:all" />
       <IlcArg Condition="'$(ControlFlowGuard)' == 'Guard' and '$(TargetOS)' == 'windows'" Include="--guard:cf" />
       <IlcArg Include="@(_IlcRootedAssemblies->'--root:%(Identity)')" />
       <IlcArg Include="@(_IlcConditionallyRootedAssemblies->'--conditionalroot:%(Identity)')" />

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 `<Project>` 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 `<InvariantGlobalization>true</InvariantGlobalization>` 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 `<TrimMode>` property to `link`.
+To enable more aggressive removal of unreferenced code, set the `<TrimmerDefaultAction>` property to `link`.
 
 To aid in troubleshooting some of the most common problems related to trimming add `<IlcGenerateCompleteTypeMetadata>true</IlcGenerateCompleteTypeMetadata>` 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 `<IlcTrimMetadata>` 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 `<InvariantGlobalization>true</InvariantGlobalization>` 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
 
 * `<IlcGenerateStackTraceData>false</IlcGenerateStackTraceData>`: 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).
-* `<IlcTrimMetadata>true</IlcTrimMetadata>`: 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
 * `<IlcOptimizationPreference>Speed</IlcOptimizationPreference>`: when generating optimized code, favor code execution speed.

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

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

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.

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
             {

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/UsageBasedMetadataManager.cs

@@ -187,7 +187,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,
@@ -527,7 +527,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);
             }
@@ -538,7 +538,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();
@@ -549,7 +549,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
@@ -600,8 +600,40 @@ public override void GetDependenciesDueToAccess(ref DependencyList dependencies,
                 dependencies.Add(factory.DataflowAnalyzedMethod(methodIL.GetMethodILDefinition()), "Access to interesting field");
             }
 
-            if ((_generationOptions & UsageBasedMetadataGenerationOptions.ReflectedMembersOnly) == 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);
             }
         }
 
@@ -1039,9 +1071,9 @@ public enum UsageBasedMetadataGenerationOptions
         ReflectionILScanning = 4,
 
         /// <summary>
-        /// Only members that were seen as reflected on will be reflectable.
+        /// Consider all native artifacts (native method bodies, etc) visible from reflection.
         /// </summary>
-        ReflectedMembersOnly = 8,
+        CreateReflectableArtifacts = 8,
 
         /// <summary>
         /// Fully root used assemblies that are not marked IsTrimmable in metadata.

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 _reflectedOnly;
         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<string> inputFiles = Array.Empty<string>();
             IReadOnlyList<string> referenceFiles = Array.Empty<string>();
 
@@ -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("reflectedonly", ref _reflectedOnly, "Generate metadata only for reflected members");
+                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,8 +763,8 @@ static string ILLinkify(string rootedAssembly)
                     metadataGenerationOptions |= UsageBasedMetadataGenerationOptions.CompleteTypesOnly;
                 if (_scanReflection)
                     metadataGenerationOptions |= UsageBasedMetadataGenerationOptions.ReflectionILScanning;
-                if (_reflectedOnly)
-                    metadataGenerationOptions |= UsageBasedMetadataGenerationOptions.ReflectedMembersOnly;
+                if (_reflectionData == "all")
+                    metadataGenerationOptions |= UsageBasedMetadataGenerationOptions.CreateReflectableArtifacts;
                 if (_rootDefaultAssemblies)
                     metadataGenerationOptions |= UsageBasedMetadataGenerationOptions.RootDefaultAssemblies;
             }