From 58091524639f81a4733dcd66510d79341d3aa1b1 Mon Sep 17 00:00:00 2001 From: Sven Boemer Date: Thu, 21 Jul 2022 14:30:01 -0700 Subject: Fix re-entrancy when marking nested functions (#2907) This fixes endless recursion introduced by the change which scans compiler-generated methods as a group. This change fixes the re-entrancy instead of putting a temporary value in the cache, because this will make it easier to fix the behavior of CheckRequiresReflectionMethodBodyScanner to not consider type parameter annotations. --- src/linker/Linker.Steps/MarkStep.cs | 58 +++++++++- .../CompilerGeneratedCodeAccessedViaReflection.cs | 127 ++++++++++++++++++--- 2 files changed, 167 insertions(+), 18 deletions(-) diff --git a/src/linker/Linker.Steps/MarkStep.cs b/src/linker/Linker.Steps/MarkStep.cs index fb71b5470..6d19bcc76 100644 --- a/src/linker/Linker.Steps/MarkStep.cs +++ b/src/linker/Linker.Steps/MarkStep.cs @@ -2885,7 +2885,7 @@ namespace Mono.Linker.Steps // the reflection scanner. Checking this will also mark direct dependencies of the method body, if it // hasn't been marked already. A cache ensures this only happens once for the method, whether or not // it is accessed via reflection. - return MarkAndCheckRequiresReflectionMethodBodyScanner (method.Body); + return CheckRequiresReflectionMethodBodyScanner (method.Body); } void ProcessAnalysisAnnotationsForMethod (MethodDefinition method, DependencyKind dependencyKind, in MessageOrigin origin) @@ -3413,6 +3413,11 @@ namespace Mono.Linker.Steps return; } + // Note: we mark the method body of every method here including compiler-generated methods, + // whether they are accessed from the user method or via reflection. + // But for compiler-generated methods we only do dataflow analysis if they're used through their + // corresponding user method, so we will skip dataflow for compiler-generated methods which + // are only accessed via reflection. bool requiresReflectionMethodBodyScanner = MarkAndCheckRequiresReflectionMethodBodyScanner (body); // Data-flow (reflection scanning) for compiler-generated methods will happen as part of the @@ -3423,11 +3428,58 @@ namespace Mono.Linker.Steps MarkReflectionLikeDependencies (body, requiresReflectionMethodBodyScanner); } + bool CheckRequiresReflectionMethodBodyScanner (MethodBody body) + { + // This method is only called on reflection access to compiler-generated methods. + // This should be uncommon, so don't cache the result. + if (ReflectionMethodBodyScanner.RequiresReflectionMethodBodyScannerForMethodBody (Context, body.Method)) + return true; + + foreach (Instruction instruction in body.Instructions) { + switch (instruction.OpCode.OperandType) { + case OperandType.InlineField: + switch (instruction.OpCode.Code) { + case Code.Stfld: + case Code.Stsfld: + case Code.Ldflda: + case Code.Ldsflda: + if (ReflectionMethodBodyScanner.RequiresReflectionMethodBodyScannerForAccess (Context, (FieldReference) instruction.Operand)) + return true; + break; + + default: + break; + } + break; + + case OperandType.InlineMethod: + if (ReflectionMethodBodyScanner.RequiresReflectionMethodBodyScannerForCallSite (Context, (MethodReference) instruction.Operand)) + return true; + break; + } + } + return false; + } + + // Keep the return value of this method in sync with that of CheckRequiresReflectionMethodBodyScanner. + // It computes the same value, while also marking as it goes, as an optimization. + // This should only be called behind a check to IsProcessed for the method or corresponding user method, + // to avoid recursion. bool MarkAndCheckRequiresReflectionMethodBodyScanner (MethodBody body) { +#if DEBUG + if (!Annotations.IsProcessed (body.Method)) { + Debug.Assert (CompilerGeneratedState.IsNestedFunctionOrStateMachineMember (body.Method)); + MethodDefinition owningMethod = body.Method; + while (Context.CompilerGeneratedState.TryGetOwningMethodForCompilerGeneratedMember (owningMethod, out var owner)) + owningMethod = owner; + Debug.Assert (owningMethod != body.Method); + Debug.Assert (Annotations.IsProcessed (owningMethod)); + } +#endif // This may get called multiple times for compiler-generated code: once for // reflection access, and once as part of the interprocedural scan of the user method. - // This check ensures that we only do the work once. + // This check ensures that we only do the work and produce warnings once. if (_compilerGeneratedMethodRequiresScanner.TryGetValue (body, out bool requiresReflectionMethodBodyScanner)) return requiresReflectionMethodBodyScanner; @@ -3451,6 +3503,7 @@ namespace Mono.Linker.Steps PostMarkMethodBody (body); + Debug.Assert (requiresReflectionMethodBodyScanner == CheckRequiresReflectionMethodBodyScanner (body)); return requiresReflectionMethodBodyScanner; } @@ -3620,6 +3673,7 @@ namespace Mono.Linker.Steps // protected virtual void MarkReflectionLikeDependencies (MethodBody body, bool requiresReflectionMethodBodyScanner) { + Debug.Assert (!CompilerGeneratedState.IsNestedFunctionOrStateMachineMember (body.Method)); // requiresReflectionMethodBodyScanner tells us whether the method body itself requires a dataflow scan. // If the method body owns any compiler-generated code, we might still need to do a scan of it together with diff --git a/test/Mono.Linker.Tests.Cases/DataFlow/CompilerGeneratedCodeAccessedViaReflection.cs b/test/Mono.Linker.Tests.Cases/DataFlow/CompilerGeneratedCodeAccessedViaReflection.cs index 416254981..a05e25439 100644 --- a/test/Mono.Linker.Tests.Cases/DataFlow/CompilerGeneratedCodeAccessedViaReflection.cs +++ b/test/Mono.Linker.Tests.Cases/DataFlow/CompilerGeneratedCodeAccessedViaReflection.cs @@ -27,7 +27,7 @@ namespace Mono.Linker.Tests.Cases.DataFlow Lambdas.Test (); LocalFunctions.Test (); - LambdaWhichMarksItself.Test (); + SelfMarkingMethods.Test (); } class BaseTypeWithIteratorStateMachines @@ -398,21 +398,6 @@ namespace Mono.Linker.Tests.Cases.DataFlow } } - class LambdaWhichMarksItself - { - static void RequiresAllOnT<[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.All)] T> () { } - - public static void Test () - { - var a = () => { - // https://github.com/dotnet/linker/issues/2903 - //RequiresAllOnT (); - }; - - a (); - } - } - [DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.All)] class LocalFunctions { @@ -538,6 +523,116 @@ namespace Mono.Linker.Tests.Cases.DataFlow } } + class SelfMarkingMethods + { + static void RequiresAllOnT<[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.All)] T> () { } + + static void RequiresNonPublicMethodsOnT<[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.NonPublicMethods)] T> () { } + + class LambdaWhichMarksItself + { + public static void Test () + { + var a = + [ExpectedWarning ("IL2118", nameof (LambdaWhichMarksItself), "", + ProducedBy = ProducedBy.Trimmer)] + () => { + RequiresAllOnT (); + }; + + a (); + } + } + + class LocalFunctionWhichMarksItself + { + public static void Test () + { + [ExpectedWarning ("IL2118", nameof (LocalFunctionWhichMarksItself), "", + ProducedBy = ProducedBy.Trimmer)] + void LocalFunction () + { + RequiresAllOnT (); + }; + + LocalFunction (); + } + } + + class IteratorWhichMarksItself + { + [ExpectedWarning ("IL2118", ProducedBy = ProducedBy.Trimmer, CompilerGeneratedCode = true)] + public static IEnumerable Test () + { + yield return 0; + + RequiresAllOnT (); + + yield return 1; + } + } + + class AsyncWhichMarksItself + { + [ExpectedWarning ("IL2118", ProducedBy = ProducedBy.Trimmer, CompilerGeneratedCode = true)] + public static async void Test () + { + await MethodAsync (); + + RequiresAllOnT (); + + await MethodAsync (); + } + } + + + class MethodWhichMarksItself + { + static void RequiresAllOnT<[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.All)] T> () { } + + public static void Test () + { + RequiresAllOnT (); + } + } + + + class LocalFunctionWhichMarksItselfOnlyAccessedViaReflection + { + [ExpectedWarning ("IL2118", nameof (LocalFunctionWhichMarksItselfOnlyAccessedViaReflection), "<" + nameof (ClassWithLocalFunction.MethodWithLocalFunction) + ">", "LocalFunction", + ProducedBy = ProducedBy.Trimmer)] + public static void Test () + { + RequiresNonPublicMethodsOnT (); + } + + public class ClassWithLocalFunction + { + public static void MethodWithLocalFunction () + { + [ExpectedWarning ("IL2118", nameof (LocalFunctionWhichMarksItselfOnlyAccessedViaReflection), "<" + nameof (MethodWithLocalFunction) + ">", nameof (LocalFunction), + ProducedBy = ProducedBy.Trimmer)] + static void LocalFunction () + { + RequiresNonPublicMethodsOnT (); + }; + + LocalFunction (); + } + } + } + + public static void Test () + { + LambdaWhichMarksItself.Test (); + LocalFunctionWhichMarksItself.Test (); + IteratorWhichMarksItself.Test (); + AsyncWhichMarksItself.Test (); + MethodWhichMarksItself.Test (); + LocalFunctionWhichMarksItselfOnlyAccessedViaReflection.Test (); + } + } + [RequiresUnreferencedCode ("--MethodWithRequires--")] [RequiresAssemblyFiles ("--MethodWithRequires--")] [RequiresDynamicCode ("--MethodWithRequires--")] -- cgit v1.2.3