From fbb3a12753da085717e516bdf75a74149f670583 Mon Sep 17 00:00:00 2001 From: Vitek Karas <10670590+vitek-karas@users.noreply.github.com> Date: Wed, 8 Mar 2023 05:11:11 -0800 Subject: [PATCH] Implement correct handling of new constraint in ilc (#82818) This is done by treating the new constraint as a data flow annotation `PublicParameterlessConstructor` (which are supposed to be identical). The rest falls out from this change since all of the validation and marking will automatically kick in. This change causes more methods to go through data flow (since that's what will actually perform the constraint validation/marking). I tested this on the ASP.NET API AOT template. Before this change it ran ~620 methods through data flow. With this change that number goes up to ~2100. The problem is tracked by https://github.com/dotnet/runtime/issues/82603. I measured compiler perf but didn't see any noticeable changes. Current thinking is that ~2100 is still not that much and most of those methods are pretty small and thus cheap to run data flow on. Fixes https://github.com/dotnet/runtime/issues/81720 - note that the repro still fails on AOT with this fix, but the failure is different (`System.InvalidOperationException: Sequence contains no matching element`). I verified that the missing .ctor is present in the app with the fix. --- .../Compiler/Dataflow/ArrayValue.cs | 9 ++ .../Compiler/Dataflow/AttributeDataFlow.cs | 2 +- .../Compiler/Dataflow/FlowAnnotations.cs | 49 ++++---- .../Compiler/Dataflow/MethodBodyScanner.cs | 3 +- .../Dataflow/ReflectionMethodBodyScanner.cs | 11 +- .../Compiler/UsageBasedMetadataManager.cs | 23 ++++ .../ILLink.Shared/TrimAnalysis/ArrayValue.cs | 28 +++++ .../TestCases/TestDatabase.cs | 5 + .../Mono.Linker.Tests/TestCases/TestSuites.cs | 7 ++ .../DependencyInjectionPattern.cs | 49 ++++++++ .../TrimAnalysis/ArrayValue.cs | 9 ++ .../TrimAnalysis/TrimAnalysisVisitor.cs | 6 +- .../ILLink.Shared/TrimAnalysis/ArrayValue.cs | 28 +++++ .../src/linker/Linker.Dataflow/ArrayValue.cs | 9 ++ .../Linker.Dataflow/MethodBodyScanner.cs | 2 +- .../DataFlow/ArrayDataFlow.cs | 15 +++ ...rloadedMethodGetsStrippedInGenericClass.cs | 2 + ...ericParametersUnusedBaseWillGetStripped.cs | 4 +- ...ntGenericArgumentNameDoesNotGetStripped.cs | 4 +- ...deUsesADifferentNameForGenericParameter.cs | 6 +- ...ferentNameForGenericParameterNestedCase.cs | 6 +- ...erentNameForGenericParameterNestedCase2.cs | 6 +- .../Generics/NewConstraintOnClass.cs | 108 ++++++++++++++++-- ...hodOfSameNameWithDifferentParameterType.cs | 4 +- .../RequiresAccessedThrough.cs | 16 +-- 25 files changed, 341 insertions(+), 70 deletions(-) diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ArrayValue.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ArrayValue.cs index c6b8010e5077e..f39e83d289817 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ArrayValue.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ArrayValue.cs @@ -84,6 +84,15 @@ public override SingleValue DeepCopy() var newValue = new ArrayValue(Size.DeepCopy(), ElementType); foreach (var kvp in IndexValues) { +#if DEBUG + // Since it's possible to store a reference to array as one of its own elements + // simple deep copy could lead to endless recursion. + // So instead we simply disallow arrays as element values completely - and treat that case as "too complex to analyze". + foreach (SingleValue v in kvp.Value.Value) + { + System.Diagnostics.Debug.Assert(v is not ArrayValue); + } +#endif newValue.IndexValues.Add(kvp.Key, new ValueBasicBlockPair(kvp.Value.Value.Clone(), kvp.Value.BasicBlockIndex)); } diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/AttributeDataFlow.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/AttributeDataFlow.cs index 25ad48c6a98cd..c37e954d15f8a 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/AttributeDataFlow.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/AttributeDataFlow.cs @@ -42,7 +42,7 @@ public AttributeDataFlow(Logger logger, NodeFactory factory, FlowAnnotations ann DependencyList? result = null; // First do the dataflow for the constructor parameters if necessary. - if (_annotations.RequiresDataflowAnalysis(method)) + if (_annotations.RequiresDataflowAnalysisDueToSignature(method)) { var builder = ImmutableArray.CreateBuilder<object?>(arguments.FixedArguments.Length); foreach (var argument in arguments.FixedArguments) diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/FlowAnnotations.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/FlowAnnotations.cs index ead8ced640675..b9312b0e10969 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/FlowAnnotations.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/FlowAnnotations.cs @@ -41,13 +41,18 @@ public FlowAnnotations(Logger logger, ILProvider ilProvider, CompilerGeneratedSt CompilerGeneratedState = compilerGeneratedState; } - public bool RequiresDataflowAnalysis(MethodDesc method) + /// <summary> + /// Determines if the method has any annotations on its parameters or return values. + /// </summary> + public bool RequiresDataflowAnalysisDueToSignature(MethodDesc method) { try { method = method.GetTypicalMethodDefinition(); TypeAnnotations typeAnnotations = GetAnnotations(method.OwningType); - return typeAnnotations.HasGenericParameterAnnotation() || typeAnnotations.TryGetAnnotation(method, out _); + // This will return true even if the method has annotations on generic parameters, + // but the callers don't rely on that - it's just easier to leave it this way (and it makes very little difference) + return typeAnnotations.TryGetAnnotation(method, out _); } catch (TypeSystemException) { @@ -68,37 +73,16 @@ public bool RequiresVirtualMethodDataflowAnalysis(MethodDesc method) } } - public bool RequiresDataflowAnalysis(FieldDesc field) + /// <summary> + /// Determines if the field has any annotations on itself (not looking at owning type). + /// </summary> + public bool RequiresDataflowAnalysisDueToSignature(FieldDesc field) { try { field = field.GetTypicalFieldDefinition(); TypeAnnotations typeAnnotations = GetAnnotations(field.OwningType); - return typeAnnotations.HasGenericParameterAnnotation() || typeAnnotations.TryGetAnnotation(field, out _); - } - catch (TypeSystemException) - { - return false; - } - } - - public bool RequiresGenericArgumentDataFlowAnalysis(GenericParameterDesc genericParameter) - { - try - { - return GetGenericParameterAnnotation(genericParameter) != DynamicallyAccessedMemberTypes.None; - } - catch (TypeSystemException) - { - return false; - } - } - - public bool HasAnyAnnotations(TypeDesc type) - { - try - { - return !GetAnnotations(type.GetTypeDefinition()).IsDefault; + return typeAnnotations.TryGetAnnotation(field, out _); } catch (TypeSystemException) { @@ -330,6 +314,11 @@ private static DynamicallyAccessedMemberTypes GetMemberTypesForDynamicallyAccess return (DynamicallyAccessedMemberTypes)blobReader.ReadUInt32(); } + private static DynamicallyAccessedMemberTypes GetMemberTypesForConstraints(GenericParameterDesc genericParameter) + => genericParameter.HasDefaultConstructorConstraint ? + DynamicallyAccessedMemberTypes.PublicParameterlessConstructor : + DynamicallyAccessedMemberTypes.None; + protected override bool CompareKeyToValue(TypeDesc key, TypeAnnotations value) => key == value.Type; protected override bool CompareValueToValue(TypeAnnotations value1, TypeAnnotations value2) => value1.Type == value2.Type; protected override int GetKeyHashCode(TypeDesc key) => key.GetHashCode(); @@ -478,6 +467,7 @@ protected override TypeAnnotations CreateValueFromKey(TypeDesc key) { GenericParameter genericParameterDef = reader.GetGenericParameter(genericParameter.Handle); var annotation = GetMemberTypesForDynamicallyAccessedMembersAttribute(reader, genericParameterDef.GetCustomAttributes()); + annotation |= GetMemberTypesForConstraints(genericParameter); if (annotation != DynamicallyAccessedMemberTypes.None) { genericParameterAnnotations ??= new DynamicallyAccessedMemberTypes[method.Instantiation.Length]; @@ -639,6 +629,7 @@ protected override TypeAnnotations CreateValueFromKey(TypeDesc key) genericParameter = (attrs?[genericParameterIndex] as EcmaGenericParameter) ?? genericParameter; GenericParameter genericParameterDef = reader.GetGenericParameter(genericParameter.Handle); var annotation = GetMemberTypesForDynamicallyAccessedMembersAttribute(reader, genericParameterDef.GetCustomAttributes()); + annotation |= GetMemberTypesForConstraints(genericParameter); if (annotation != DynamicallyAccessedMemberTypes.None) { typeGenericParameterAnnotations ??= new DynamicallyAccessedMemberTypes[ecmaType.Instantiation.Length]; @@ -959,7 +950,7 @@ public FieldAnnotation(FieldDesc field, DynamicallyAccessedMemberTypes annotatio } internal partial bool MethodRequiresDataFlowAnalysis(MethodProxy method) - => RequiresDataflowAnalysis(method.Method); + => RequiresDataflowAnalysisDueToSignature(method.Method); #pragma warning disable CA1822 // Other partial implementations are not in the ilc project internal partial MethodReturnValue GetMethodReturnValue(MethodProxy method, DynamicallyAccessedMemberTypes dynamicallyAccessedMemberTypes) diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/MethodBodyScanner.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/MethodBodyScanner.cs index ecc3f12f649a4..d6d4526e5c771 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/MethodBodyScanner.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/MethodBodyScanner.cs @@ -5,7 +5,6 @@ using System.Collections.Generic; using System.Diagnostics; using System.Linq; -using System.Reflection.Metadata; using ILCompiler.Logging; @@ -1378,7 +1377,7 @@ private void ScanStelem( else { // When we know the index, we can record the value at that index. - StoreMethodLocalValue(arrValue.IndexValues, valueToStore.Value, indexToStoreAtInt.Value, curBasicBlock, MaxTrackedArrayValues); + StoreMethodLocalValue(arrValue.IndexValues, ArrayValue.SanitizeArrayElementValue(valueToStore.Value), indexToStoreAtInt.Value, curBasicBlock, MaxTrackedArrayValues); } } } 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 c6a808d3187de..625a0f4195ed1 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMethodBodyScanner.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMethodBodyScanner.cs @@ -33,7 +33,7 @@ internal sealed class ReflectionMethodBodyScanner : MethodBodyScanner public static bool RequiresReflectionMethodBodyScannerForCallSite(FlowAnnotations flowAnnotations, MethodDesc method) { return Intrinsics.GetIntrinsicIdForMethod(method) > IntrinsicId.RequiresReflectionBodyScanner_Sentinel || - flowAnnotations.RequiresDataflowAnalysis(method) || + flowAnnotations.RequiresDataflowAnalysisDueToSignature(method) || GenericArgumentDataFlow.RequiresGenericArgumentDataFlow(flowAnnotations, method) || method.DoesMethodRequire(DiagnosticUtilities.RequiresUnreferencedCodeAttribute, out _) || method.DoesMethodRequire(DiagnosticUtilities.RequiresDynamicCodeAttribute, out _) || @@ -43,12 +43,12 @@ public static bool RequiresReflectionMethodBodyScannerForCallSite(FlowAnnotation public static bool RequiresReflectionMethodBodyScannerForMethodBody(FlowAnnotations flowAnnotations, MethodDesc methodDefinition) { return Intrinsics.GetIntrinsicIdForMethod(methodDefinition) > IntrinsicId.RequiresReflectionBodyScanner_Sentinel || - flowAnnotations.RequiresDataflowAnalysis(methodDefinition); + flowAnnotations.RequiresDataflowAnalysisDueToSignature(methodDefinition); } public static bool RequiresReflectionMethodBodyScannerForAccess(FlowAnnotations flowAnnotations, FieldDesc field) { - return flowAnnotations.RequiresDataflowAnalysis(field) || + return flowAnnotations.RequiresDataflowAnalysisDueToSignature(field) || GenericArgumentDataFlow.RequiresGenericArgumentDataFlow(flowAnnotations, field) || field.DoesFieldRequire(DiagnosticUtilities.RequiresUnreferencedCodeAttribute, out _) || field.DoesFieldRequire(DiagnosticUtilities.RequiresDynamicCodeAttribute, out _); @@ -259,9 +259,10 @@ public static bool HandleCall( var callingMethodDefinition = callingMethodBody.OwningMethod; Debug.Assert(callingMethodDefinition == diagnosticContext.Origin.MemberDefinition); - bool requiresDataFlowAnalysis = reflectionMarker.Annotations.RequiresDataflowAnalysis(calledMethod); var annotatedMethodReturnValue = reflectionMarker.Annotations.GetMethodReturnValue(calledMethod); - Debug.Assert(requiresDataFlowAnalysis || annotatedMethodReturnValue.DynamicallyAccessedMemberTypes == DynamicallyAccessedMemberTypes.None); + Debug.Assert( + RequiresReflectionMethodBodyScannerForCallSite(reflectionMarker.Annotations, calledMethod) || + annotatedMethodReturnValue.DynamicallyAccessedMemberTypes == DynamicallyAccessedMemberTypes.None); MultiValue? maybeMethodReturnValue = null; diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/UsageBasedMetadataManager.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/UsageBasedMetadataManager.cs index c29a30a02c397..eca94cc204a51 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/UsageBasedMetadataManager.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/UsageBasedMetadataManager.cs @@ -896,6 +896,9 @@ public MetadataManager ToAnalysisBasedMetadataManager() private void AddDataflowDependency(ref DependencyList dependencies, NodeFactory factory, MethodIL methodIL, string reason) { + if (ShouldSkipDataflowForMethod(methodIL)) + return; + MethodIL methodILDefinition = methodIL.GetMethodILDefinition(); if (FlowAnnotations.CompilerGeneratedState.TryGetUserMethodForCompilerGeneratedMember(methodILDefinition.OwningMethod, out var userMethod)) { @@ -904,6 +907,8 @@ private void AddDataflowDependency(ref DependencyList dependencies, NodeFactory // It is possible that this will try to add the DatadlowAnalyzedMethod node multiple times for the same method // but that's OK since the node factory will only add actually one node. methodILDefinition = FlowAnnotations.ILProvider.GetMethodIL(userMethod); + if (ShouldSkipDataflowForMethod(methodILDefinition)) + return; } // Data-flow (reflection scanning) for compiler-generated methods will happen as part of the @@ -911,8 +916,26 @@ private void AddDataflowDependency(ref DependencyList dependencies, NodeFactory if (CompilerGeneratedState.IsNestedFunctionOrStateMachineMember(methodILDefinition.OwningMethod)) return; + // Currently we add data flow for reasons which are not directly related to data flow + // or don't need full data flow processing. + // The prime example is that we run data flow for any method which contains an access + // to a member on a generic type with annotated generic parameters. These are relatively common + // as there are quite a few types which use the new/struct/unmanaged constraints (which are all treated as annotations). + // Technically we don't need to run data flow only because of this, since the result of analysis + // will not depend on stack modeling and of the other data flow functionality. + // See https://github.com/dotnet/runtime/issues/82603 for more details and some ideas. + dependencies ??= new DependencyList(); dependencies.Add(factory.DataflowAnalyzedMethod(methodILDefinition), reason); + + // Some MethodIL implementations can't/don't provide the method definition version of the IL + // for example if the method is a stub generated by the compiler, the IL may look very different + // between each instantiation (and definition version may not even make sense). + // We will skip data flow for such methods for now, since currently dataflow can't handle instantiated + // generics. + static bool ShouldSkipDataflowForMethod(MethodIL method) + => method.GetMethodILDefinition() == method && + method.OwningMethod.GetTypicalMethodDefinition() != method.OwningMethod; } private struct ReflectableEntityBuilder<T> diff --git a/src/coreclr/tools/aot/ILLink.Shared/TrimAnalysis/ArrayValue.cs b/src/coreclr/tools/aot/ILLink.Shared/TrimAnalysis/ArrayValue.cs index 342e3a5c51a63..258d38ad14766 100644 --- a/src/coreclr/tools/aot/ILLink.Shared/TrimAnalysis/ArrayValue.cs +++ b/src/coreclr/tools/aot/ILLink.Shared/TrimAnalysis/ArrayValue.cs @@ -1,6 +1,7 @@ // Copyright (c) .NET Foundation and contributors. All rights reserved. // Licensed under the MIT license. See LICENSE file in the project root for full license information. +using System.Linq; using ILLink.Shared.DataFlow; using MultiValue = ILLink.Shared.DataFlow.ValueSet<ILLink.Shared.DataFlow.SingleValue>; @@ -16,5 +17,32 @@ internal sealed partial record ArrayValue : SingleValue public readonly SingleValue Size; public partial bool TryGetValueByIndex (int index, out MultiValue value); + + public static MultiValue SanitizeArrayElementValue (MultiValue input) + { + // We need to be careful about self-referencing arrays. It's easy to have an array which has one of the elements as itself: + // var arr = new object[1]; + // arr[0] = arr; + // + // We can't deep copy this, as it's an infinite recursion. And we can't easily guard against it with checks to self-referencing + // arrays - as this could be two or more levels deep. + // + // We need to deep copy arrays because we don't have a good way to track references (we treat everything as a value type) + // and thus we could get bad results if the array is involved in multiple branches for example. + // That said, it only matters for us to track arrays to be able to get integers or Type values (and we really only need relatively simple cases) + // + // So we will simply treat array value as an element value as "too complex to analyze" and give up by storing Unknown instead + + bool needSanitization = false; + foreach (var v in input) { + if (v is ArrayValue) + needSanitization = true; + } + + if (!needSanitization) + return input; + + return new (input.Select (v => v is ArrayValue ? UnknownValue.Instance : v)); + } } } diff --git a/src/coreclr/tools/aot/Mono.Linker.Tests/TestCases/TestDatabase.cs b/src/coreclr/tools/aot/Mono.Linker.Tests/TestCases/TestDatabase.cs index aef1f75e5188b..ec395f21795a3 100644 --- a/src/coreclr/tools/aot/Mono.Linker.Tests/TestCases/TestDatabase.cs +++ b/src/coreclr/tools/aot/Mono.Linker.Tests/TestCases/TestDatabase.cs @@ -24,6 +24,11 @@ public static IEnumerable<object[]> DynamicDependencies () return TestNamesBySuiteName (); } + public static IEnumerable<object[]> Generics () + { + return TestNamesBySuiteName (); + } + public static IEnumerable<object[]> LinkXml() { return TestNamesBySuiteName(); diff --git a/src/coreclr/tools/aot/Mono.Linker.Tests/TestCases/TestSuites.cs b/src/coreclr/tools/aot/Mono.Linker.Tests/TestCases/TestSuites.cs index aff281da97b59..4fe12fa6bac3d 100644 --- a/src/coreclr/tools/aot/Mono.Linker.Tests/TestCases/TestSuites.cs +++ b/src/coreclr/tools/aot/Mono.Linker.Tests/TestCases/TestSuites.cs @@ -23,6 +23,13 @@ public void DynamicDependencies (string t) Run (t); } + [Theory] + [MemberData (nameof (TestDatabase.Generics), MemberType = typeof (TestDatabase))] + public void Generics (string t) + { + Run (t); + } + [Theory] [MemberData (nameof (TestDatabase.LinkXml), MemberType = typeof (TestDatabase))] public void LinkXml (string t) diff --git a/src/tests/nativeaot/SmokeTests/TrimmingBehaviors/DependencyInjectionPattern.cs b/src/tests/nativeaot/SmokeTests/TrimmingBehaviors/DependencyInjectionPattern.cs index 0c88b5e2b2186..ebfbf0b4fffb9 100644 --- a/src/tests/nativeaot/SmokeTests/TrimmingBehaviors/DependencyInjectionPattern.cs +++ b/src/tests/nativeaot/SmokeTests/TrimmingBehaviors/DependencyInjectionPattern.cs @@ -24,6 +24,13 @@ public static int Run() var created = customFactory.Create(); Assert.Equal(42, created.GetValue()); + services.RegisterService(typeof(ICustomFactoryWithConstraint<>), typeof(CustomFactoryWithConstraintImpl<>)); + services.RegisterService(typeof(ICustomFactoryWithConstraintWrapper), typeof(CustomFactoryWithConstraintWrapperImpl)); + + var customFactoryWithConstraintWrapper = services.GetService<ICustomFactoryWithConstraintWrapper>(); + var createdWithConstraint = customFactoryWithConstraintWrapper.Create(); + Assert.Equal(42, createdWithConstraint.GetValue()); + return 100; } } @@ -173,5 +180,47 @@ public FactoryCreated() _value = 42; } + public int GetValue() => _value; +} + +interface ICustomFactoryWithConstraintWrapper +{ + FactoryWithConstraintCreated Create(); +} + +class CustomFactoryWithConstraintWrapperImpl : ICustomFactoryWithConstraintWrapper +{ + private FactoryWithConstraintCreated _value; + + public CustomFactoryWithConstraintWrapperImpl(ICustomFactoryWithConstraint<FactoryWithConstraintCreated> factory) + { + _value = factory.Create(); + } + + public FactoryWithConstraintCreated Create() => _value; +} + +interface ICustomFactoryWithConstraint<T> where T : new() +{ + T Create(); +} + +class CustomFactoryWithConstraintImpl<T> : ICustomFactoryWithConstraint<T> where T : new() +{ + public T Create() + { + return Activator.CreateInstance<T>(); + } +} + +class FactoryWithConstraintCreated +{ + int _value; + + public FactoryWithConstraintCreated() + { + _value = 42; + } + public int GetValue() => _value; } \ No newline at end of file diff --git a/src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/ArrayValue.cs b/src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/ArrayValue.cs index 835a60e22dfaf..3df35757310ea 100644 --- a/src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/ArrayValue.cs +++ b/src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/ArrayValue.cs @@ -64,6 +64,15 @@ public override SingleValue DeepCopy () { var newArray = new ArrayValue (Size); foreach (var kvp in IndexValues) { +#if DEBUG + // Since it's possible to store a reference to array as one of its own elements + // simple deep copy could lead to endless recursion. + // So instead we simply disallow arrays as element values completely - and treat that case as "too complex to analyze". + foreach (SingleValue v in kvp.Value) { + System.Diagnostics.Debug.Assert (v is not ArrayValue); + } +#endif + newArray.IndexValues.Add (kvp.Key, kvp.Value.Clone ()); } diff --git a/src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/TrimAnalysisVisitor.cs b/src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/TrimAnalysisVisitor.cs index 78874c103fbe5..dccf85a2c5b24 100644 --- a/src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/TrimAnalysisVisitor.cs +++ b/src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/TrimAnalysisVisitor.cs @@ -79,7 +79,7 @@ public override MultiValue VisitArrayCreation (IArrayCreationOperation operation var elements = operation.Initializer?.ElementValues.Select (val => Visit (val, state)).ToArray () ?? System.Array.Empty<MultiValue> (); foreach (var array in arrayValue.Cast<ArrayValue> ()) { for (int i = 0; i < elements.Length; i++) { - array.IndexValues.Add (i, elements[i]); + array.IndexValues.Add (i, ArrayValue.SanitizeArrayElementValue(elements[i])); } } @@ -229,9 +229,9 @@ public override void HandleArrayElementWrite (MultiValue arrayValue, MultiValue arr.IndexValues.Clear (); } else { if (arr.IndexValues.TryGetValue (index.Value, out _)) { - arr.IndexValues[index.Value] = valueToWrite; + arr.IndexValues[index.Value] = ArrayValue.SanitizeArrayElementValue(valueToWrite); } else if (arr.IndexValues.Count < MaxTrackedArrayValues) { - arr.IndexValues[index.Value] = valueToWrite; + arr.IndexValues[index.Value] = ArrayValue.SanitizeArrayElementValue(valueToWrite); } } } diff --git a/src/tools/illink/src/ILLink.Shared/TrimAnalysis/ArrayValue.cs b/src/tools/illink/src/ILLink.Shared/TrimAnalysis/ArrayValue.cs index 342e3a5c51a63..9c699baea9125 100644 --- a/src/tools/illink/src/ILLink.Shared/TrimAnalysis/ArrayValue.cs +++ b/src/tools/illink/src/ILLink.Shared/TrimAnalysis/ArrayValue.cs @@ -1,6 +1,7 @@ // Copyright (c) .NET Foundation and contributors. All rights reserved. // Licensed under the MIT license. See LICENSE file in the project root for full license information. +using System.Linq; using ILLink.Shared.DataFlow; using MultiValue = ILLink.Shared.DataFlow.ValueSet<ILLink.Shared.DataFlow.SingleValue>; @@ -16,5 +17,32 @@ internal sealed partial record ArrayValue : SingleValue public readonly SingleValue Size; public partial bool TryGetValueByIndex (int index, out MultiValue value); + + public static MultiValue SanitizeArrayElementValue (MultiValue input) + { + // We need to be careful about self-referencing arrays. It's easy to have an array which has one of the elements as itself: + // var arr = new object[1]; + // arr[0] = arr; + // + // We can't deep copy this, as it's an infinite recursion. And we can't easily guard against it with checks to self-referencing + // arrays - as this could be two or more levels deep. + // + // We need to deep copy arrays because we don't have a good way to track references (we treat everything as a value type) + // and thus we could get bad results if the array is involved in multiple branches for example. + // That said, it only matters for us to track arrays to be able to get integers or Type values (and we really only need relatively simple cases) + // + // So we will simply treat array value as an element value as "too complex to analyze" and give up by storing Unknown instead + + bool needSanitization = false; + foreach (var v in input) { + if (v is ArrayValue) + needSanitization = true; + } + + if (!needSanitization) + return input; + + return new(input.Select (v => v is ArrayValue ? UnknownValue.Instance : v)); + } } } diff --git a/src/tools/illink/src/linker/Linker.Dataflow/ArrayValue.cs b/src/tools/illink/src/linker/Linker.Dataflow/ArrayValue.cs index 3a42e37a85648..9e816bf93eb12 100644 --- a/src/tools/illink/src/linker/Linker.Dataflow/ArrayValue.cs +++ b/src/tools/illink/src/linker/Linker.Dataflow/ArrayValue.cs @@ -79,6 +79,15 @@ public override SingleValue DeepCopy () { var newValue = new ArrayValue (Size.DeepCopy (), ElementType); foreach (var kvp in IndexValues) { +#if DEBUG + // Since it's possible to store a reference to array as one of its own elements + // simple deep copy could lead to endless recursion. + // So instead we simply disallow arrays as element values completely - and treat that case as "too complex to analyze". + foreach (SingleValue v in kvp.Value.Value) { + System.Diagnostics.Debug.Assert (v is not ArrayValue); + } +#endif + newValue.IndexValues.Add (kvp.Key, new ValueBasicBlockPair (kvp.Value.Value.Clone (), kvp.Value.BasicBlockIndex)); } diff --git a/src/tools/illink/src/linker/Linker.Dataflow/MethodBodyScanner.cs b/src/tools/illink/src/linker/Linker.Dataflow/MethodBodyScanner.cs index c85838f644c19..5b946d199ce3c 100644 --- a/src/tools/illink/src/linker/Linker.Dataflow/MethodBodyScanner.cs +++ b/src/tools/illink/src/linker/Linker.Dataflow/MethodBodyScanner.cs @@ -1169,7 +1169,7 @@ private void ScanStelem ( MarkArrayValuesAsUnknown (arrValue, curBasicBlock); } else { // When we know the index, we can record the value at that index. - StoreMethodLocalValue (arrValue.IndexValues, valueToStore.Value, indexToStoreAtInt.Value, curBasicBlock, MaxTrackedArrayValues); + StoreMethodLocalValue (arrValue.IndexValues, ArrayValue.SanitizeArrayElementValue (valueToStore.Value), indexToStoreAtInt.Value, curBasicBlock, MaxTrackedArrayValues); } } } diff --git a/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/ArrayDataFlow.cs b/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/ArrayDataFlow.cs index ad3fd24577ac2..1f06afb2174b6 100644 --- a/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/ArrayDataFlow.cs +++ b/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/ArrayDataFlow.cs @@ -40,6 +40,9 @@ public static void Main () TestArrayResetGetElementOnByRefArray (); TestArrayResetAfterCall (); TestArrayResetAfterAssignment (); + + TestArrayRecursion (); + TestMultiDimensionalArray.Test (); WriteCapturedArrayElement.Test (); @@ -273,6 +276,18 @@ static void TestArrayResetAfterAssignment () arr[0].RequiresPublicFields (); // Should warn } + static void TestArrayRecursion () + { + typeof (TestType).RequiresAll (); // Force data flow on this method + + object[] arr = new object[3]; + arr[0] = arr; // Recursive reference + + ConsumeArray (arr); + + static void ConsumeArray (object[] a) { } + } + static Type[] _externalArray; static class TestMultiDimensionalArray diff --git a/src/tools/illink/test/Mono.Linker.Tests.Cases/Generics/CorrectOverloadedMethodGetsStrippedInGenericClass.cs b/src/tools/illink/test/Mono.Linker.Tests.Cases/Generics/CorrectOverloadedMethodGetsStrippedInGenericClass.cs index 0da7862c2f7c9..e673b3f75b927 100644 --- a/src/tools/illink/test/Mono.Linker.Tests.Cases/Generics/CorrectOverloadedMethodGetsStrippedInGenericClass.cs +++ b/src/tools/illink/test/Mono.Linker.Tests.Cases/Generics/CorrectOverloadedMethodGetsStrippedInGenericClass.cs @@ -2,6 +2,8 @@ namespace Mono.Linker.Tests.Cases.Generics { + [IgnoreTestCase ("Ignore in NativeAOT, see https://github.com/dotnet/runtime/issues/82447", IgnoredBy = Tool.NativeAot)] + [KeptAttributeAttribute (typeof (IgnoreTestCaseAttribute), By = Tool.Trimmer)] class CorrectOverloadedMethodGetsStrippedInGenericClass { public static void Main () diff --git a/src/tools/illink/test/Mono.Linker.Tests.Cases/Generics/DerivedClassWithMethodOfSameNameAsBaseButDifferentNumberOfGenericParametersUnusedBaseWillGetStripped.cs b/src/tools/illink/test/Mono.Linker.Tests.Cases/Generics/DerivedClassWithMethodOfSameNameAsBaseButDifferentNumberOfGenericParametersUnusedBaseWillGetStripped.cs index 9a58584d9a682..62e706ba899a7 100644 --- a/src/tools/illink/test/Mono.Linker.Tests.Cases/Generics/DerivedClassWithMethodOfSameNameAsBaseButDifferentNumberOfGenericParametersUnusedBaseWillGetStripped.cs +++ b/src/tools/illink/test/Mono.Linker.Tests.Cases/Generics/DerivedClassWithMethodOfSameNameAsBaseButDifferentNumberOfGenericParametersUnusedBaseWillGetStripped.cs @@ -4,7 +4,7 @@ namespace Mono.Linker.Tests.Cases.Generics { class DerivedClassWithMethodOfSameNameAsBaseButDifferentNumberOfGenericParametersUnusedBaseWillGetStripped { - public static void Main (string[] args) + public static void Main () { MyDerived obj = new MyDerived (); obj.Method<int, int> (1); @@ -31,4 +31,4 @@ public virtual T Method<T, K> (T arg1) } } } -} \ No newline at end of file +} diff --git a/src/tools/illink/test/Mono.Linker.Tests.Cases/Generics/GenericInstanceInterfaceMethodImplementedWithDifferentGenericArgumentNameDoesNotGetStripped.cs b/src/tools/illink/test/Mono.Linker.Tests.Cases/Generics/GenericInstanceInterfaceMethodImplementedWithDifferentGenericArgumentNameDoesNotGetStripped.cs index 06818969451a7..c3e112ab677ae 100644 --- a/src/tools/illink/test/Mono.Linker.Tests.Cases/Generics/GenericInstanceInterfaceMethodImplementedWithDifferentGenericArgumentNameDoesNotGetStripped.cs +++ b/src/tools/illink/test/Mono.Linker.Tests.Cases/Generics/GenericInstanceInterfaceMethodImplementedWithDifferentGenericArgumentNameDoesNotGetStripped.cs @@ -2,6 +2,8 @@ namespace Mono.Linker.Tests.Cases.Generics { + [IgnoreTestCase ("Ignore in NativeAOT, see https://github.com/dotnet/runtime/issues/82447", IgnoredBy = Tool.NativeAot)] + [KeptAttributeAttribute (typeof (IgnoreTestCaseAttribute), By = Tool.Trimmer)] class GenericInstanceInterfaceMethodImplementedWithDifferentGenericArgumentNameDoesNotGetStripped { public static void Main () @@ -32,4 +34,4 @@ public GenericType<TInConcrete> ShouldNotGetStripped<TInConcrete> () } } } -} \ No newline at end of file +} diff --git a/src/tools/illink/test/Mono.Linker.Tests.Cases/Generics/MethodWithParameterWhichHasGenericParametersAndOverrideUsesADifferentNameForGenericParameter.cs b/src/tools/illink/test/Mono.Linker.Tests.Cases/Generics/MethodWithParameterWhichHasGenericParametersAndOverrideUsesADifferentNameForGenericParameter.cs index da2d562b0e808..fe567f305076c 100644 --- a/src/tools/illink/test/Mono.Linker.Tests.Cases/Generics/MethodWithParameterWhichHasGenericParametersAndOverrideUsesADifferentNameForGenericParameter.cs +++ b/src/tools/illink/test/Mono.Linker.Tests.Cases/Generics/MethodWithParameterWhichHasGenericParametersAndOverrideUsesADifferentNameForGenericParameter.cs @@ -3,9 +3,11 @@ namespace Mono.Linker.Tests.Cases.Generics { + [IgnoreTestCase ("Ignore in NativeAOT, see https://github.com/dotnet/runtime/issues/82447", IgnoredBy = Tool.NativeAot)] + [KeptAttributeAttribute (typeof (IgnoreTestCaseAttribute), By = Tool.Trimmer)] class MethodWithParameterWhichHasGenericParametersAndOverrideUsesADifferentNameForGenericParameter { - public static void Main (string[] args) + public static void Main () { Derived<int, int> tmp = new Derived<int, int> (); tmp.Method<int> (null); @@ -29,4 +31,4 @@ public override TResult2 Method<TResult2> (IDictionary<TResult1, TResult2> arg) } } } -} \ No newline at end of file +} diff --git a/src/tools/illink/test/Mono.Linker.Tests.Cases/Generics/MethodWithParameterWhichHasGenericParametersAndOverrideUsesADifferentNameForGenericParameterNestedCase.cs b/src/tools/illink/test/Mono.Linker.Tests.Cases/Generics/MethodWithParameterWhichHasGenericParametersAndOverrideUsesADifferentNameForGenericParameterNestedCase.cs index f737196ded0cf..fb94b78e26996 100644 --- a/src/tools/illink/test/Mono.Linker.Tests.Cases/Generics/MethodWithParameterWhichHasGenericParametersAndOverrideUsesADifferentNameForGenericParameterNestedCase.cs +++ b/src/tools/illink/test/Mono.Linker.Tests.Cases/Generics/MethodWithParameterWhichHasGenericParametersAndOverrideUsesADifferentNameForGenericParameterNestedCase.cs @@ -3,9 +3,11 @@ namespace Mono.Linker.Tests.Cases.Generics { + [IgnoreTestCase ("Ignore in NativeAOT, see https://github.com/dotnet/runtime/issues/82447", IgnoredBy = Tool.NativeAot)] + [KeptAttributeAttribute (typeof (IgnoreTestCaseAttribute), By = Tool.Trimmer)] class MethodWithParameterWhichHasGenericParametersAndOverrideUsesADifferentNameForGenericParameterNestedCase { - public static void Main (string[] args) + public static void Main () { Derived<int, int> tmp = new Derived<int, int> (); tmp.Method<int> (null); @@ -34,4 +36,4 @@ public override TResult2 Method<TResult2> (IDictionary<TResult1, TResult2> arg) } } } -} \ No newline at end of file +} diff --git a/src/tools/illink/test/Mono.Linker.Tests.Cases/Generics/MethodWithParameterWhichHasGenericParametersAndOverrideUsesADifferentNameForGenericParameterNestedCase2.cs b/src/tools/illink/test/Mono.Linker.Tests.Cases/Generics/MethodWithParameterWhichHasGenericParametersAndOverrideUsesADifferentNameForGenericParameterNestedCase2.cs index 8bd0af0dded01..7b84e9368a6bd 100644 --- a/src/tools/illink/test/Mono.Linker.Tests.Cases/Generics/MethodWithParameterWhichHasGenericParametersAndOverrideUsesADifferentNameForGenericParameterNestedCase2.cs +++ b/src/tools/illink/test/Mono.Linker.Tests.Cases/Generics/MethodWithParameterWhichHasGenericParametersAndOverrideUsesADifferentNameForGenericParameterNestedCase2.cs @@ -3,9 +3,11 @@ namespace Mono.Linker.Tests.Cases.Generics { + [IgnoreTestCase ("Ignore in NativeAOT, see https://github.com/dotnet/runtime/issues/82447", IgnoredBy = Tool.NativeAot)] + [KeptAttributeAttribute (typeof (IgnoreTestCaseAttribute), By = Tool.Trimmer)] class MethodWithParameterWhichHasGenericParametersAndOverrideUsesADifferentNameForGenericParameterNestedCase2 { - public static void Main (string[] args) + public static void Main () { Derived<int, int> tmp = new Derived<int, int> (); tmp.Method<int> (null); @@ -34,4 +36,4 @@ public override TResult2 Method<TResult2> (IDictionary<TResult1, TResult2> arg) } } } -} \ No newline at end of file +} diff --git a/src/tools/illink/test/Mono.Linker.Tests.Cases/Generics/NewConstraintOnClass.cs b/src/tools/illink/test/Mono.Linker.Tests.Cases/Generics/NewConstraintOnClass.cs index ae4991e16e813..177a4ace8a515 100644 --- a/src/tools/illink/test/Mono.Linker.Tests.Cases/Generics/NewConstraintOnClass.cs +++ b/src/tools/illink/test/Mono.Linker.Tests.Cases/Generics/NewConstraintOnClass.cs @@ -1,37 +1,129 @@ using System; +using System.Runtime.CompilerServices; using Mono.Linker.Tests.Cases.Expectations.Assertions; namespace Mono.Linker.Tests.Cases.Generics { + [ExpectedNoWarnings] public class NewConstraintOnClass { public static void Main () { - var g1 = new G1<G1Ctor> (); + NewConstraint.Test (); + StructConstraint.Test (); + UnmanagedConstraint.Test (); } - class G1Ctor + [Kept] + class NewConstraint { - static readonly int field = 1; + class TestClass + { + static readonly int field = 1; + + [Kept] + public TestClass () + { + } + + public TestClass (int a) + { + } + + public void Foo () + { + } + } + + [Kept] + [KeptMember (".ctor()")] + class WithConstraint<T> where T : new() + { + } [Kept] - public G1Ctor () + public static void Test () { + var a = new WithConstraint<TestClass> (); } + } - public G1Ctor (int a) + [Kept] + class StructConstraint + { + struct TestStruct { + static readonly int field = 1; + + [Kept] + public TestStruct () + { + } + + public TestStruct (int a) + { + } + + public void Foo () + { + } } - public void Foo () + [Kept] + [KeptMember (".ctor()")] + class WithConstraint<T> where T : struct { } + + [Kept] + public static void Test () + { + var a = new WithConstraint<TestStruct> (); + } } [Kept] - [KeptMember (".ctor()")] - class G1<T> where T : G1Ctor, new() + class UnmanagedConstraint { + struct TestStruct + { + static readonly int field = 1; + + [Kept] + public TestStruct () + { + } + + public TestStruct (int a) + { + } + + public void Foo () + { + } + } + + [Kept] + [KeptMember (".ctor()")] + class WithConstraint<[KeptAttributeAttribute (typeof (IsUnmanagedAttribute))] T> where T : unmanaged + { + } + + [Kept] + public static void Test () + { + var a = new WithConstraint<TestStruct> (); + } } } } + +namespace System.Runtime.CompilerServices +{ + // NativeAOT test infra filters out System.* members from validation for now + [Kept (By = Tool.Trimmer)] + [KeptMember (".ctor()", By = Tool.Trimmer)] + public partial class IsUnmanagedAttribute + { + } +} diff --git a/src/tools/illink/test/Mono.Linker.Tests.Cases/Generics/OverrideWithAnotherVirtualMethodOfSameNameWithDifferentParameterType.cs b/src/tools/illink/test/Mono.Linker.Tests.Cases/Generics/OverrideWithAnotherVirtualMethodOfSameNameWithDifferentParameterType.cs index 8b67e179d95c2..3ba7549b60daf 100644 --- a/src/tools/illink/test/Mono.Linker.Tests.Cases/Generics/OverrideWithAnotherVirtualMethodOfSameNameWithDifferentParameterType.cs +++ b/src/tools/illink/test/Mono.Linker.Tests.Cases/Generics/OverrideWithAnotherVirtualMethodOfSameNameWithDifferentParameterType.cs @@ -4,7 +4,7 @@ namespace Mono.Linker.Tests.Cases.Generics { class OverrideWithAnotherVirtualMethodOfSameNameWithDifferentParameterType { - public static void Main (string[] args) + public static void Main () { new Derived<double, int> ().Method (1.0); } @@ -35,4 +35,4 @@ public virtual S Method (S arg) } } } -} \ No newline at end of file +} diff --git a/src/tools/illink/test/Mono.Linker.Tests.Cases/RequiresCapability/RequiresAccessedThrough.cs b/src/tools/illink/test/Mono.Linker.Tests.Cases/RequiresCapability/RequiresAccessedThrough.cs index 094ff710afbb1..a531f94d5f42e 100644 --- a/src/tools/illink/test/Mono.Linker.Tests.Cases/RequiresCapability/RequiresAccessedThrough.cs +++ b/src/tools/illink/test/Mono.Linker.Tests.Cases/RequiresCapability/RequiresAccessedThrough.cs @@ -129,10 +129,8 @@ class NewConstraintTestAnnotatedType static void GenericMethod<T> () where T : new() { } - // NativeAOT doesnt generate warnings when marking generic constraints - // https://github.com/dotnet/runtime/issues/68688 - [ExpectedWarning ("IL2026", "--NewConstraintTestType.ctor--", ProducedBy = Tool.Analyzer | Tool.Trimmer)] - [ExpectedWarning ("IL2026", "--NewConstraintTestAnnotatedType--", ProducedBy = Tool.Analyzer | Tool.Trimmer)] + [ExpectedWarning ("IL2026", "--NewConstraintTestType.ctor--")] + [ExpectedWarning ("IL2026", "--NewConstraintTestAnnotatedType--")] [ExpectedWarning ("IL3002", "--NewConstraintTestType.ctor--", ProducedBy = Tool.Analyzer)] [ExpectedWarning ("IL3050", "--NewConstraintTestType.ctor--", ProducedBy = Tool.Analyzer)] public static void Test<T> () where T : new() @@ -151,10 +149,8 @@ public static void DoNothing () { } { } - // NativeAOT doesnt generate warnings when marking generic constraints - // https://github.com/dotnet/runtime/issues/68688 - [ExpectedWarning ("IL2026", "--NewConstraintTestType.ctor--", ProducedBy = Tool.Analyzer | Tool.Trimmer)] - [ExpectedWarning ("IL2026", "--NewConstraintTestAnnotatedType--", ProducedBy = Tool.Analyzer | Tool.Trimmer)] + [ExpectedWarning ("IL2026", "--NewConstraintTestType.ctor--")] + [ExpectedWarning ("IL2026", "--NewConstraintTestAnnotatedType--")] [ExpectedWarning ("IL3002", "--NewConstraintTestType.ctor--", ProducedBy = Tool.Analyzer)] [ExpectedWarning ("IL3050", "--NewConstraintTestType.ctor--", ProducedBy = Tool.Analyzer)] public static void TestNewConstraintOnTypeParameter<T> () where T : new() @@ -199,8 +195,8 @@ public static void Method () } } - [ExpectedWarning ("IL2026", "--NewConstraintTestType.ctor--", ProducedBy = Tool.Analyzer | Tool.Trimmer)] - [ExpectedWarning ("IL2026", "--NewConstraintTestAnnotatedType--", ProducedBy = Tool.Analyzer | Tool.Trimmer)] + [ExpectedWarning ("IL2026", "--NewConstraintTestType.ctor--")] + [ExpectedWarning ("IL2026", "--NewConstraintTestAnnotatedType--")] [ExpectedWarning ("IL3002", "--NewConstraintTestType.ctor--", ProducedBy = Tool.Analyzer)] [ExpectedWarning ("IL3050", "--NewConstraintTestType.ctor--", ProducedBy = Tool.Analyzer)] public static void TestNewConstraintOnTypeParameterOfStaticType<T> () where T : new()