From e6a09695e71cdc975f77b142ec99b2a9a2adad85 Mon Sep 17 00:00:00 2001 From: Sven Boemer Date: Fri, 23 Aug 2024 16:37:14 -0700 Subject: [PATCH 1/3] Fix edge case in object.GetType dataflow --- .../Compiler/Dataflow/HandleCallAction.cs | 5 ++ .../TrimAnalysis/HandleCallAction.cs | 5 ++ .../Linker.Dataflow/HandleCallAction.cs | 5 ++ .../DataFlow/ObjectGetTypeDataflow.cs | 64 +++++++++++++++++++ .../Reflection/ObjectGetType.cs | 58 ----------------- 5 files changed, 79 insertions(+), 58 deletions(-) diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/HandleCallAction.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/HandleCallAction.cs index 3e9b8a1fa7d58b..6f9988295b6605 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/HandleCallAction.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/HandleCallAction.cs @@ -373,6 +373,11 @@ private partial bool TryHandleIntrinsic ( // case IntrinsicId.Object_GetType: { + if (instanceValue.IsEmpty ()) { + AddReturnValue (MultiValueLattice.Top); + break; + } + foreach (var valueNode in instanceValue.AsEnumerable ()) { // Note that valueNode can be statically typed in IL as some generic argument type. diff --git a/src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/HandleCallAction.cs b/src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/HandleCallAction.cs index 7a13c46e7fd2c2..620e9fa582bb17 100644 --- a/src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/HandleCallAction.cs +++ b/src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/HandleCallAction.cs @@ -66,6 +66,11 @@ private partial bool TryHandleIntrinsic ( break; case IntrinsicId.Object_GetType: { + if (instanceValue.IsEmpty ()) { + AddReturnValue (MultiValueLattice.Top); + break; + } + foreach (var valueNode in instanceValue.AsEnumerable ()) { // Note that valueNode can be statically typed as some generic argument type. // For example: diff --git a/src/tools/illink/src/linker/Linker.Dataflow/HandleCallAction.cs b/src/tools/illink/src/linker/Linker.Dataflow/HandleCallAction.cs index a337e999506f8b..45b262a5818a89 100644 --- a/src/tools/illink/src/linker/Linker.Dataflow/HandleCallAction.cs +++ b/src/tools/illink/src/linker/Linker.Dataflow/HandleCallAction.cs @@ -100,6 +100,11 @@ private partial bool TryHandleIntrinsic ( // GetType() // case IntrinsicId.Object_GetType: { + if (instanceValue.IsEmpty ()) { + AddReturnValue (MultiValueLattice.Top); + break; + } + foreach (var valueNode in instanceValue.AsEnumerable ()) { // Note that valueNode can be statically typed in IL as some generic argument type. // For example: diff --git a/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/ObjectGetTypeDataflow.cs b/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/ObjectGetTypeDataflow.cs index 3116bae3c0de7e..9a69c365311af1 100644 --- a/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/ObjectGetTypeDataflow.cs +++ b/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/ObjectGetTypeDataflow.cs @@ -4,6 +4,7 @@ using System; using System.Collections.Generic; using System.Diagnostics.CodeAnalysis; +using System.Reflection; using Mono.Linker.Tests.Cases.Expectations.Assertions; using Mono.Linker.Tests.Cases.Expectations.Helpers; @@ -21,6 +22,9 @@ public static void Main () EnumConstraintSatisfiesPublicFields.Test (); InstantiatedTypeParameterAsSource.Test (); EnumerationOverInstances.Test (); + NullValue.Test (); + NoValue.Test (); + UnknownValue.Test (); } class SealedConstructorAsSource @@ -412,5 +416,65 @@ public static void Test () } } } + + class NullValue + { + class TestType + { + } + + [ExpectedWarning ("IL2072", nameof (DataFlowTypeExtensions.RequiresAll) + "(Type)", nameof (Object.GetType) + "()")] + public static void Test () + { + TestType nullInstance = null; + // Even though this throws at runtime, we warn about the return value of GetType + nullInstance.GetType ().RequiresAll (); + } + } + + class NoValue + { + [ExpectedWarning ("IL2072", nameof (DataFlowTypeExtensions.RequiresAll) + "(Type)", nameof (Object.GetType) + "()")] + static void TestGetTypeOfType () + { + Type t = null; + Type noValue = Type.GetTypeFromHandle (t.TypeHandle); + // Even though the above throws at runtime, we warn about the return value of GetType. + // This doesn't go through the intrinsic handling because this goes to the newslot method Type.GetType, + // instead of the intrinsically handled object.GetType. + noValue.GetType ().RequiresAll (); + } + + static void TestGetTypeOfMethod () + { + Type t = null; + MethodInfo noValue = t.GetMethod ("Method"); + noValue.GetType ().RequiresAll (); + } + + public static void Test () + { + TestGetTypeOfType (); + TestGetTypeOfMethod (); + } + } + + class UnknownValue + { + [KeptMember (".ctor()")] + class TestType + { + } + + static TestType GetInstance () => new TestType (); + + [ExpectedWarning ("IL2072", nameof (DataFlowTypeExtensions.RequiresAll) + "(Type)", nameof (Object.GetType) + "()")] + public static void Test () + { + TestType unknownValue = GetInstance (); + // Should warn about the return value of GetType + unknownValue.GetType ().RequiresAll (); + } + } } } diff --git a/src/tools/illink/test/Mono.Linker.Tests.Cases/Reflection/ObjectGetType.cs b/src/tools/illink/test/Mono.Linker.Tests.Cases/Reflection/ObjectGetType.cs index 52032b740c4918..a9fa5d4e07b261 100644 --- a/src/tools/illink/test/Mono.Linker.Tests.Cases/Reflection/ObjectGetType.cs +++ b/src/tools/illink/test/Mono.Linker.Tests.Cases/Reflection/ObjectGetType.cs @@ -53,10 +53,6 @@ public static void Main () DataFlowUnusedGetType.Test (); - NullValue.Test (); - NoValue.Test (); - UnknownValue.Test (); - PrivateMembersOnBaseTypesAppliedToDerived.Test (); IsInstOf.Test (); @@ -1467,60 +1463,6 @@ public static void Test () } } - [Kept] - class NullValue - { - [Kept] - class TestType - { - } - - [Kept] - [ExpectedWarning ("IL2072", nameof (DataFlowTypeExtensions.RequiresAll) + "(Type)", nameof (Object.GetType) + "()")] - public static void Test () - { - TestType nullInstance = null; - // Even though this throws at runtime, we warn about the return value of GetType - nullInstance.GetType ().RequiresAll (); - } - } - - [Kept] - class NoValue - { - [Kept] - [ExpectedWarning ("IL2072", nameof (DataFlowTypeExtensions.RequiresAll) + "(Type)", nameof (Object.GetType) + "()")] - public static void Test () - { - Type t = null; - Type noValue = Type.GetTypeFromHandle (t.TypeHandle); - // Even though the above throws at runtime, we warn about the return value of GetType - noValue.GetType ().RequiresAll (); - } - } - - [Kept] - class UnknownValue - { - [Kept] - [KeptMember (".ctor()")] - class TestType - { - } - - [Kept] - static TestType GetInstance () => new TestType (); - - [Kept] - [ExpectedWarning ("IL2072", nameof (DataFlowTypeExtensions.RequiresAll) + "(Type)", nameof (Object.GetType) + "()")] - public static void Test () - { - TestType unknownValue = GetInstance (); - // Should warn about the return value of GetType - unknownValue.GetType ().RequiresAll (); - } - } - [Kept] class PrivateMembersOnBaseTypesAppliedToDerived { From 3aecd752f960f5b458ff68062cd89483821efc24 Mon Sep 17 00:00:00 2001 From: Sven Boemer Date: Fri, 23 Aug 2024 16:37:20 -0700 Subject: [PATCH 2/3] Clean up testcase --- .../DataFlow/ObjectGetTypeDataflow.cs | 50 ------------------- 1 file changed, 50 deletions(-) diff --git a/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/ObjectGetTypeDataflow.cs b/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/ObjectGetTypeDataflow.cs index 9a69c365311af1..5c45c718c9fa4a 100644 --- a/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/ObjectGetTypeDataflow.cs +++ b/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/ObjectGetTypeDataflow.cs @@ -29,16 +29,12 @@ public static void Main () class SealedConstructorAsSource { - [KeptMember (".ctor()")] public class Base { } - [KeptMember (".ctor()")] - [KeptBaseType (typeof (Base))] public sealed class Derived : Base { - [KeptAttributeAttribute (typeof (RequiresUnreferencedCodeAttribute))] [RequiresUnreferencedCode (nameof (Method))] public void Method () { } } @@ -50,48 +46,35 @@ public static void Test () } } - [Kept] class InstantiatedGenericAsSource { - [Kept] - [KeptAttributeAttribute (typeof (DynamicallyAccessedMembersAttribute))] [DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] class Generic { - [Kept] [ExpectedWarning ("IL2112")] - [KeptAttributeAttribute (typeof (RequiresUnreferencedCodeAttribute))] [RequiresUnreferencedCode (nameof (KeptForMethodParameter))] public void KeptForMethodParameter () {} - [Kept] [ExpectedWarning ("IL2112")] - [KeptAttributeAttribute (typeof (RequiresUnreferencedCodeAttribute))] [RequiresUnreferencedCode (nameof (KeptForField))] public void KeptForField () {} - [Kept] [ExpectedWarning ("IL2112")] - [KeptAttributeAttribute (typeof (RequiresUnreferencedCodeAttribute))] [RequiresUnreferencedCode (nameof (KeptJustBecause))] public void KeptJustBecause () {} } - [Kept] static void TestMethodParameter (Generic instance) { instance.GetType ().GetMethod ("KeptForMethodParameter"); } - [Kept] static Generic field = null; - [Kept] static void TestField () { field.GetType ().GetMethod ("KeptForField"); } - [Kept] public static void Test () { TestMethodParameter (null); @@ -99,57 +82,45 @@ public static void Test () } } - [Kept] class EnumTypeSatisfiesPublicFields { - [Kept] static void ParameterType (Enum instance) { instance.GetType ().RequiresPublicFields (); } - [Kept] class FieldType { - [Kept] Enum field; - [Kept] public FieldType (Enum instance) => field = instance; - [Kept] public void Test () { field.GetType ().RequiresPublicFields (); } } - [KeptMember ("value__")] enum EnumType { - [Kept] Value } - [Kept] static Enum ReturnType () { return EnumType.Value; } - [Kept] static void TestReturnType () { ReturnType ().GetType ().RequiresPublicFields (); } - [Kept] static void OutParameter (out Enum value) { value = EnumType.Value; } - [Kept] // Analyzer doesn't assign a value to the out parameter after calling the OutParameter method, // so when it looks up the value of the local 'value', it returns an empty value, and the // GetType intrinsic handling can't see that the out param satisfies the public fields requirement. @@ -161,7 +132,6 @@ static void TestOutParameter () value.GetType ().RequiresPublicFields (); } - [Kept] public static void Test () { ParameterType (EnumType.Value); @@ -171,22 +141,17 @@ public static void Test () } } - [Kept] class EnumConstraintSatisfiesPublicFields { - [Kept] static void MethodGenericParameterAsParameter (T instance) where T : Enum { instance.GetType ().RequiresPublicFields (); } - [Kept] class TypeGenericParameterAsField where T : Enum { - [Kept] public static T field; - [Kept] public static void TestAccessFromType () { field.GetType ().RequiresPublicFields (); @@ -198,32 +163,26 @@ static void TestAccessTypeGenericParameterAsField () TypeGenericParameterAsField.field.GetType ().RequiresPublicFields (); } - [Kept] class TypeGenericParameterAsParameter where T : Enum { - [Kept] public static void Method (T instance) { instance.GetType ().RequiresPublicFields (); } - [Kept] public static void Test () { Method (default); } } - [Kept] class TypeGenericParameterAsReturnType where T : Enum { - [Kept] public static T Method () { return default; } - [Kept] public static void TestAccessFromType () { Method ().GetType ().RequiresPublicFields (); @@ -235,16 +194,13 @@ static void TestAccessTypeGenericParameterAsReturnType () TypeGenericParameterAsReturnType.Method ().GetType ().RequiresPublicFields (); } - [Kept] class TypeGenericParameterAsOutParam where T : Enum { - [Kept] public static void Method (out T instance) { instance = default; } - [Kept] [ExpectedWarning ("IL2072", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/101734")] public static void TestAccessFromType () { @@ -260,26 +216,21 @@ static void TestAccessTypeGenericParameterAsOutParam () instance.GetType ().RequiresPublicFields (); } - [KeptMember ("value__")] enum EnumType { - [Kept] Value } - [Kept] static T MethodGenericParameterAsReturnType () where T : Enum { return default; } - [Kept] static void TestMethodGenericParameterAsReturnType () { MethodGenericParameterAsReturnType ().GetType ().RequiresPublicFields (); } - [Kept] public static void Test () { TypeGenericParameterAsParameter.Test (); @@ -461,7 +412,6 @@ public static void Test () class UnknownValue { - [KeptMember (".ctor()")] class TestType { } From 88c660f5d6077c6dc7b92452c69b4ceab45434c1 Mon Sep 17 00:00:00 2001 From: Sven Boemer Date: Mon, 26 Aug 2024 13:26:36 -0700 Subject: [PATCH 3/3] Fix up tests --- .../DataFlow/ObjectGetTypeDataflow.cs | 61 ++++++++++++------- .../Reflection/ObjectGetType.cs | 12 +++- 2 files changed, 51 insertions(+), 22 deletions(-) diff --git a/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/ObjectGetTypeDataflow.cs b/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/ObjectGetTypeDataflow.cs index 5c45c718c9fa4a..71c1999c9600f8 100644 --- a/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/ObjectGetTypeDataflow.cs +++ b/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/ObjectGetTypeDataflow.cs @@ -121,11 +121,6 @@ static void OutParameter (out Enum value) value = EnumType.Value; } - // Analyzer doesn't assign a value to the out parameter after calling the OutParameter method, - // so when it looks up the value of the local 'value', it returns an empty value, and the - // GetType intrinsic handling can't see that the out param satisfies the public fields requirement. - // Similar for the other cases below. - [ExpectedWarning ("IL2072", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/101734")] static void TestOutParameter () { OutParameter (out var value); @@ -201,7 +196,6 @@ public static void Method (out T instance) instance = default; } - [ExpectedWarning ("IL2072", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/101734")] public static void TestAccessFromType () { Method (out var instance); @@ -209,7 +203,6 @@ public static void TestAccessFromType () } } - [ExpectedWarning ("IL2072", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/101734")] static void TestAccessTypeGenericParameterAsOutParam () { TypeGenericParameterAsOutParam.Method (out var instance); @@ -248,24 +241,35 @@ public static void Test () class InstantiatedTypeParameterAsSource { [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicFields)] - class Annotated {} + class AnnotatedPublicFields {} static T GenericReturnType () => default; static void TestGenericMethodReturnType () { - GenericReturnType ().GetType ().RequiresPublicFields (); + GenericReturnType ().GetType ().RequiresPublicFields (); } static void GenericOutParameter (out T value) => value = default; - [UnexpectedWarning ("IL2072", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/101734")] static void TestGenericMethodOutParameter () { - GenericOutParameter (out Annotated value); + GenericOutParameter (out AnnotatedPublicFields value); value.GetType ().RequiresPublicFields (); } + // Analyzer doesn't assign a value to the out parameter after calling the OutParameter method, + // so when it looks up the value of the local 'value', it returns an empty value, and the + // GetType intrinsic handling propagates the empty value so the analyzer + // can't see that the out parameter violates the public methods requirement. + // Similar for the other cases below. + [ExpectedWarning ("IL2072", Tool.Trimmer | Tool.NativeAot, "https://github.com/dotnet/runtime/issues/101734")] + static void TestGenericMethodOutParameterMismatch () + { + GenericOutParameter (out AnnotatedPublicFields value); + value.GetType ().RequiresPublicMethods (); + } + class Generic { public static T ReturnType () => default; @@ -289,58 +293,73 @@ public class Nested { static void TestGenericClassReturnType () { - Generic.ReturnType ().GetType ().RequiresPublicFields (); + Generic.ReturnType ().GetType ().RequiresPublicFields (); } - [UnexpectedWarning ("IL2072", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/101734")] static void TestGenericClassOutParameter () { - Generic.OutParameter (out var value); + Generic.OutParameter (out var value); value.GetType ().RequiresPublicFields (); } + [ExpectedWarning ("IL2072", Tool.Trimmer | Tool.NativeAot, "https://github.com/dotnet/runtime/issues/101734")] + static void TestGenericClassOutParameterMismatch () + { + Generic.OutParameter (out var value); + value.GetType ().RequiresPublicMethods (); + } + static void TestGenericClassField () { - Generic.field.GetType ().RequiresPublicFields (); + Generic.field.GetType ().RequiresPublicFields (); } static void TestGenericClassProperty () { - Generic.Property.GetType ().RequiresPublicFields (); + Generic.Property.GetType ().RequiresPublicFields (); } static void TestNestedGenericClassReturnType () { - Generic.Nested.ReturnType ().GetType ().RequiresPublicFields (); + Generic.Nested.ReturnType ().GetType ().RequiresPublicFields (); } - [UnexpectedWarning ("IL2072", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/101734")] static void TestNestedGenericClassOutParameter () { - Generic.Nested.OutParameter (out var value); + Generic.Nested.OutParameter (out var value); value.GetType ().RequiresPublicFields (); } + [ExpectedWarning ("IL2072", Tool.Trimmer | Tool.NativeAot, "https://github.com/dotnet/runtime/issues/101734")] + static void TestNestedGenericClassOutParameterMismatch () + { + Generic.Nested.OutParameter (out var value); + value.GetType ().RequiresPublicMethods (); + } + static void TestNestedGenericClassField () { - Generic.Nested.field.GetType ().RequiresPublicFields (); + Generic.Nested.field.GetType ().RequiresPublicFields (); } static void TestNestedGenericClassProperty () { - Generic.Nested.Property.GetType ().RequiresPublicFields (); + Generic.Nested.Property.GetType ().RequiresPublicFields (); } public static void Test () { TestGenericMethodReturnType (); TestGenericMethodOutParameter (); + TestGenericMethodOutParameterMismatch (); TestGenericClassReturnType (); TestGenericClassOutParameter (); + TestGenericClassOutParameterMismatch (); TestGenericClassField (); TestGenericClassProperty (); TestNestedGenericClassReturnType (); TestNestedGenericClassOutParameter (); + TestNestedGenericClassOutParameterMismatch (); TestNestedGenericClassField (); TestNestedGenericClassProperty (); } diff --git a/src/tools/illink/test/Mono.Linker.Tests.Cases/Reflection/ObjectGetType.cs b/src/tools/illink/test/Mono.Linker.Tests.Cases/Reflection/ObjectGetType.cs index a9fa5d4e07b261..3eaa5e496901c6 100644 --- a/src/tools/illink/test/Mono.Linker.Tests.Cases/Reflection/ObjectGetType.cs +++ b/src/tools/illink/test/Mono.Linker.Tests.Cases/Reflection/ObjectGetType.cs @@ -1526,7 +1526,7 @@ class Target } [Kept] - [UnexpectedWarning ("IL2072", Tool.TrimmerAnalyzerAndNativeAot, "https://github.com/dotnet/runtime/issues/93720")] + [UnexpectedWarning ("IL2072", Tool.Trimmer | Tool.NativeAot, "https://github.com/dotnet/runtime/issues/93720")] static void TestIsInstOf (object o) { if (o is Target t) { @@ -1534,11 +1534,21 @@ static void TestIsInstOf (object o) } } + [Kept] + [ExpectedWarning ("IL2072", Tool.Trimmer | Tool.NativeAot, "https://github.com/dotnet/runtime/issues/93720")] + static void TestIsInstOfMismatch (object o) + { + if (o is Target t) { + t.GetType ().RequiresPublicMethods (); + } + } + [Kept] public static void Test () { var target = new Target (); TestIsInstOf (target); + TestIsInstOfMismatch (target); } }