From 33a30bc0b011960e66fdf86d5833bfdd7f60102f Mon Sep 17 00:00:00 2001 From: Vitek Karas <10670590+vitek-karas@users.noreply.github.com> Date: Wed, 10 May 2023 14:01:12 -0700 Subject: [PATCH] ILLink should keep parameter names for methods which are used to create a delagate (#85831) This is a fix for https://github.com/dotnet/linker/issues/3061. If the app creates a delegate over a method and later on access the `Delegate.Method` property it can get a `MethodInfo` which it can use to inspect some aspects of the method, specifically parameter names. NativeAOT already handles this correctly, but ILLink doesn't and will trim the parameter names away. The correct behavior is to treat such method as reflection accessible (we really don't want to mark the `Delegate.Method` property as trim incompatible). The ideal way to implement this is to perform full data flow on method addresses, which would allow us to detect which method is the delegate created for. (this is what NativeAOT ends up doing through JIT) But that is rather complex, an easier solution is to treat any method which address is accessed as reflection enabled. This covers all delegates. It might mark more methods as reflection enabled, but it's very uncommon. So this change does that, it will mark all methods accessed via `ldftn` (or similar) as reflection enabled. Additionally this cleans up marking of reflection enabled members in the MarkStep. Specifically there was a distinction between reflection accessed and indirect accessed methods, but the latter only kicked in for the obsolete preserve attributes. It's better to reflection mark those as well. This might cause a small size regression, but it's very unlikely to be even observable. This requires lot more test changes to enable sane testing: * In the ILC test infra, track if method was kept with or without reflection metadata (as that is the difference in NativeAOT which will provide parameter names or not) * Teach both the ILLink and ILC test infras how to handle compiler generated code with the `Kept` attribute validation * This is in no way complete, but it's an improvement * Mostly it means that we don't fail the test if a compiler generated code is kept without it being marked with the `Kept` attribute (as that's not always possible) * Note that we have some support for this for things like backing fields and private implementation details, but nothing which can handle state machines, and closure types for lambdas. These should now be workable, without the ability to verify that they were indeed kept (and how) * Finally adds tests verifying the new functionality. --- .../Mono.Linker.Tests/TestCases/TestSuites.cs | 1 + .../TestCasesRunner/AssemblyChecker.cs | 218 +++++++++++++----- .../src/linker/Linker.Steps/MarkStep.cs | 50 ++-- .../Reflection/ParametersUsedViaReflection.cs | 36 ++- .../TestCasesRunner/AssemblyChecker.cs | 123 ++++++---- 5 files changed, 307 insertions(+), 121 deletions(-) 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 4e3901f2697..a3985de579b 100644 --- a/src/coreclr/tools/aot/Mono.Linker.Tests/TestCases/TestSuites.cs +++ b/src/coreclr/tools/aot/Mono.Linker.Tests/TestCases/TestSuites.cs @@ -43,6 +43,7 @@ namespace Mono.Linker.Tests.TestCases { switch (t) { case "TypeHierarchyReflectionWarnings": + case "ParametersUsedViaReflection": Run (t); break; default: diff --git a/src/coreclr/tools/aot/Mono.Linker.Tests/TestCasesRunner/AssemblyChecker.cs b/src/coreclr/tools/aot/Mono.Linker.Tests/TestCasesRunner/AssemblyChecker.cs index 961e4b6a9c0..c0c17cf4d31 100644 --- a/src/coreclr/tools/aot/Mono.Linker.Tests/TestCasesRunner/AssemblyChecker.cs +++ b/src/coreclr/tools/aot/Mono.Linker.Tests/TestCasesRunner/AssemblyChecker.cs @@ -21,12 +21,28 @@ namespace Mono.Linker.Tests.TestCasesRunner { public class AssemblyChecker { + class LinkedEntity + { + public TypeSystemEntity Entity { get; init; } + + public LinkedEntity(TypeSystemEntity entity) => Entity = entity; + } + + class LinkedMethodEntity : LinkedEntity + { + public bool IsReflected { get; init; } + + public MethodDesc Method { get => (MethodDesc) Entity; } + + public LinkedMethodEntity (MethodDesc method, bool isReflected) : base (method) => IsReflected = isReflected; + } + private readonly BaseAssemblyResolver originalsResolver; private readonly ReaderParameters originalReaderParameters; private readonly AssemblyDefinition originalAssembly; private readonly ILCompilerTestCaseResult testResult; - private readonly Dictionary linkedMembers; + private readonly Dictionary linkedMembers; private readonly HashSet verifiedGeneratedFields = new HashSet (); private readonly HashSet verifiedEventMethods = new HashSet (); private readonly HashSet verifiedGeneratedTypes = new HashSet (); @@ -95,9 +111,9 @@ namespace Mono.Linker.Tests.TestCasesRunner linkedMembers.TryGetValue ( token, - out TypeSystemEntity? linkedMember); + out LinkedEntity? linkedMember); - VerifyTypeDefinition (td, linkedMember as TypeDesc); + VerifyTypeDefinition (td, linkedMember); linkedMembers.Remove (token); continue; @@ -109,7 +125,7 @@ namespace Mono.Linker.Tests.TestCasesRunner // Filter out all members which are not from the main assembly // The Kept attributes are "optional" for non-main assemblies string mainModuleName = originalAssembly.Name.Name; - List externalMembers = linkedMembers.Where (m => GetModuleName (m.Value) != mainModuleName).Select (m => m.Key).ToList (); + List externalMembers = linkedMembers.Where (m => GetModuleName (m.Value.Entity) != mainModuleName).Select (m => m.Key).ToList (); foreach (var externalMember in externalMembers) { linkedMembers.Remove (externalMember); } @@ -118,9 +134,29 @@ namespace Mono.Linker.Tests.TestCasesRunner Assert.True ( false, "Linked output includes unexpected member:\n " + - string.Join ("\n ", linkedMembers.Values.Select (e => e.GetDisplayName ()))); + string.Join ("\n ", linkedMembers.Values.Select (e => e.Entity.GetDisplayName ()))); + } + + static bool IsCompilerGeneratedMemberName (string memberName) + { + return memberName.Length > 0 && memberName[0] == '<'; + } + + static bool IsCompilerGeneratedMember (IMemberDefinition member) + { + if (IsCompilerGeneratedMemberName (member.Name)) + return true; + + if (member.DeclaringType != null) + return IsCompilerGeneratedMember (member.DeclaringType); + + return false; } + static bool IsDelegateBackingFieldsType (TypeDefinition type) => type.Name == "<>O"; + + static bool IsPrivateImplementationDetailsType (TypeDefinition type) => string.IsNullOrEmpty (type.Namespace) && type.Name.StartsWith (""); + private void PopulateLinkedMembers () { foreach (TypeDesc type in testResult.TrimmingResults.AllEETypes) { @@ -132,10 +168,10 @@ namespace Mono.Linker.Tests.TestCasesRunner } foreach (MethodDesc method in testResult.TrimmingResults.ReflectedMethods) { - AddMethod (method); + AddMethod (method, isReflected: true); } - void AddMethod (MethodDesc method) + void AddMethod (MethodDesc method, bool isReflected = false) { MethodDesc methodDef = method.GetTypicalMethodDefinition (); @@ -149,7 +185,7 @@ namespace Mono.Linker.Tests.TestCasesRunner if (owningType?.IsDelegate == true) return; - if (!AddMember (methodDef)) + if (!AddTrimmedMethod (methodDef, isReflected)) return; if (owningType is not null) { @@ -182,7 +218,7 @@ namespace Mono.Linker.Tests.TestCasesRunner // So to simplify this, we're going to automatically "mark" all of the delegate's methods foreach (MethodDesc m in typeDef.GetMethods ()) { if (ShouldIncludeEntityByDisplayName (m)) { - AddMember (m); + AddTrimmedMethod (m, isReflected: false); } } } @@ -208,7 +244,28 @@ namespace Mono.Linker.Tests.TestCasesRunner AddMember (@event); } - bool AddMember (TypeSystemEntity entity) => linkedMembers.TryAdd (new AssemblyQualifiedToken (entity), entity); + bool AddMember (TypeSystemEntity entity) + { + Assert.False (entity is MethodDesc, "Use AddTrimmedMethod for all methods instead"); + return linkedMembers.TryAdd (new AssemblyQualifiedToken (entity), new LinkedEntity(entity)); + } + + bool AddTrimmedMethod (MethodDesc method, bool isReflected = false) + { + var token = new AssemblyQualifiedToken (method); + bool addedNew = true; + if (linkedMembers.TryGetValue(token, out var existingValue)) { + addedNew = false; + LinkedMethodEntity existingMethod = (LinkedMethodEntity) existingValue; + if (existingMethod.IsReflected || !isReflected) + return addedNew; + + linkedMembers.Remove (token); + } + + linkedMembers.Add (token, new LinkedMethodEntity (method, isReflected)); + return addedNew; + } static bool ShouldIncludeEntityByDisplayName (TypeSystemEntity entity) => !ExcludeDisplayNames.Contains (entity.GetDisplayName ()); @@ -268,8 +325,9 @@ namespace Mono.Linker.Tests.TestCasesRunner VerifyCustomAttributes (original, linked); } - protected virtual void VerifyTypeDefinition (TypeDefinition original, TypeDesc? linked) + void VerifyTypeDefinition (TypeDefinition original, LinkedEntity? linkedEntity) { + TypeDesc? linked = linkedEntity?.Entity as TypeDesc; if (linked != null && NameUtils.GetActualOriginDisplayName (linked) is string linkedDisplayName && verifiedGeneratedTypes.Contains (linkedDisplayName)) return; @@ -287,10 +345,19 @@ namespace Mono.Linker.Tests.TestCasesRunner original.AllMembers ().Any (HasActiveKeptDerivedAttribute); if (!expectedKept) { - if (linked != null) - Assert.True (false, $"Type `{original}' should have been removed"); + if (linked == null) + return; - return; + // Compiler generated members can't be annotated with `Kept` attributes directly + // For some of them we have special attributes (backing fields for example), but it's impractical to define + // special attributes for all types of compiler generated members (there are quite a few of them and they're + // going to change/increase over time). + // So we're effectively disabling Kept validation on compiler generated members + // Note that we still want to go "inside" each such member, as it might have additional attributes + // we do want to validate. There's no specific use case right now, but I can easily imagine one + // for more detailed testing of for example custom attributes on local functions, or similar. + if (!IsCompilerGeneratedMember (original)) + Assert.True (false, $"Type `{original}' should have been removed"); } bool prev = checkNames; @@ -319,31 +386,41 @@ namespace Mono.Linker.Tests.TestCasesRunner protected virtual void VerifyTypeDefinitionKept (TypeDefinition original, TypeDesc? linked) { + // NativeAOT will not keep delegate backing field type information, it's compiled down to a set of static fields + // this infra currently doesn't track fields in any way. + // Same goes for private implementation detail type. + if (IsDelegateBackingFieldsType (original) || IsPrivateImplementationDetailsType(original)) + return; + if (linked == null) { Assert.True (false, $"Type `{original}' should have been kept"); return; } #if false - if (!original.IsInterface) - VerifyBaseType (original, linked); + // Skip verification of type metadata for compiler generated types (we don't currently need it yet) + if (!IsCompilerGeneratedMember (original)) { + VerifyKeptByAttributes (original, linked); + if (!original.IsInterface) + VerifyBaseType (original, linked); - VerifyInterfaces (original, linked); - VerifyPseudoAttributes (original, linked); - VerifyGenericParameters (original, linked); - VerifyCustomAttributes (original, linked); - VerifySecurityAttributes (original, linked); + VerifyInterfaces (original, linked); + VerifyPseudoAttributes (original, linked); + VerifyGenericParameters (original, linked); + VerifyCustomAttributes (original, linked); + VerifySecurityAttributes (original, linked); - VerifyFixedBufferFields (original, linked); + VerifyFixedBufferFields (original, linked); + } #endif foreach (var td in original.NestedTypes) { AssemblyQualifiedToken token = new (td); linkedMembers.TryGetValue ( token, - out TypeSystemEntity? linkedMember); + out LinkedEntity? linkedMember); - VerifyTypeDefinition (td, linkedMember as TypeDesc); + VerifyTypeDefinition (td, linkedMember); linkedMembers.Remove (token); } @@ -353,8 +430,8 @@ namespace Mono.Linker.Tests.TestCasesRunner linkedMembers.TryGetValue ( token, - out TypeSystemEntity? linkedMember); - VerifyProperty (p, linkedMember as PropertyPseudoDesc, linked); + out LinkedEntity? linkedMember); + VerifyProperty (p, linkedMember, linked); linkedMembers.Remove (token); } // Need to check events before fields so that the KeptBackingFieldAttribute is handled correctly @@ -363,8 +440,8 @@ namespace Mono.Linker.Tests.TestCasesRunner linkedMembers.TryGetValue ( token, - out TypeSystemEntity? linkedMember); - VerifyEvent (e, linkedMember as EventPseudoDesc, linked); + out LinkedEntity? linkedMember); + VerifyEvent (e, linkedMember, linked); linkedMembers.Remove (token); } @@ -387,9 +464,9 @@ namespace Mono.Linker.Tests.TestCasesRunner AssemblyQualifiedToken token = new (m); linkedMembers.TryGetValue ( token, - out TypeSystemEntity? linkedMember); + out LinkedEntity? linkedMember); - VerifyMethod (m, linkedMember as MethodDesc); + VerifyMethod (m, linkedMember); linkedMembers.Remove (token); } } @@ -454,7 +531,8 @@ namespace Mono.Linker.Tests.TestCasesRunner private void VerifyField (FieldDefinition src, FieldDesc? linked) { - bool expectedKept = ShouldBeKept (src); + bool compilerGenerated = IsCompilerGeneratedMember (src); + bool expectedKept = ShouldBeKept (src) | compilerGenerated; if (!expectedKept) { if (linked != null) @@ -463,10 +541,10 @@ namespace Mono.Linker.Tests.TestCasesRunner return; } - VerifyFieldKept (src, linked); + VerifyFieldKept (src, linked, compilerGenerated); } - private static void VerifyFieldKept (FieldDefinition src, FieldDesc? linked) + private static void VerifyFieldKept (FieldDefinition src, FieldDesc? linked, bool compilerGenerated) { if (linked == null) { Assert.True (false, $"Field `{src}' should have been kept"); @@ -484,15 +562,18 @@ namespace Mono.Linker.Tests.TestCasesRunner #if false VerifyPseudoAttributes (src, linked); - VerifyCustomAttributes (src, linked); + if (!compilerGenerated) + VerifyCustomAttributes (src, linked); #endif } - private void VerifyProperty (PropertyDefinition src, PropertyPseudoDesc? linked, TypeDesc linkedType) + private void VerifyProperty (PropertyDefinition src, LinkedEntity? linkedEntity, TypeDesc linkedType) { + PropertyPseudoDesc? linked = linkedEntity?.Entity as PropertyPseudoDesc; VerifyMemberBackingField (src, linkedType); - bool expectedKept = ShouldBeKept (src); + bool compilerGenerated = IsCompilerGeneratedMember (src); + bool expectedKept = ShouldBeKept (src) || compilerGenerated; if (!expectedKept) { if (linked is not null) @@ -516,15 +597,18 @@ namespace Mono.Linker.Tests.TestCasesRunner #if false VerifyPseudoAttributes (src, linked); - VerifyCustomAttributes (src, linked); + if (!compilerGenerated) + VerifyCustomAttributes (src, linked); #endif } - private void VerifyEvent (EventDefinition src, EventPseudoDesc? linked, TypeDesc linkedType) + private void VerifyEvent (EventDefinition src, LinkedEntity? linkedEntity, TypeDesc linkedType) { + EventPseudoDesc? linked = linkedEntity?.Entity as EventPseudoDesc; VerifyMemberBackingField (src, linkedType); - bool expectedKept = ShouldBeKept (src); + bool compilerGenerated = IsCompilerGeneratedMember (src); + bool expectedKept = ShouldBeKept (src) | compilerGenerated; if (!expectedKept) { if (linked is not null) @@ -539,39 +623,50 @@ namespace Mono.Linker.Tests.TestCasesRunner } if (src.CustomAttributes.Any (attr => attr.AttributeType.Name == nameof (KeptEventAddMethodAttribute))) { - VerifyMethodInternal (src.AddMethod, linked.AddMethod, true); + // TODO: This is wrong - we can't validate that the method is present by looking at linked (as that is not actually linked) + // we need to look into linkedMembers to see if the method was actually preserved by the compiler (and has an entry point) + VerifyMethodInternal (src.AddMethod, new LinkedMethodEntity(linked.AddMethod, false), true, compilerGenerated); verifiedEventMethods.Add (src.AddMethod.FullName); linkedMembers.Remove (new AssemblyQualifiedToken (src.AddMethod)); } if (src.CustomAttributes.Any (attr => attr.AttributeType.Name == nameof (KeptEventRemoveMethodAttribute))) { - VerifyMethodInternal (src.RemoveMethod, linked.RemoveMethod, true); + // TODO: This is wrong - we can't validate that the method is present by looking at linked (as that is not actually linked) + // we need to look into linkedMembers to see if the method was actually preserved by the compiler (and has an entry point) + VerifyMethodInternal (src.RemoveMethod, new LinkedMethodEntity(linked.RemoveMethod, false), true, compilerGenerated); verifiedEventMethods.Add (src.RemoveMethod.FullName); linkedMembers.Remove (new AssemblyQualifiedToken (src.RemoveMethod)); } #if false VerifyPseudoAttributes (src, linked); - VerifyCustomAttributes (src, linked); + if (!compilerGenerated) + VerifyCustomAttributes (src, linked); #endif } - private void VerifyMethod (MethodDefinition src, MethodDesc? linked) + private void VerifyMethod (MethodDefinition src, LinkedEntity? linkedEntity) { + LinkedMethodEntity? linked = linkedEntity as LinkedMethodEntity; + bool compilerGenerated = IsCompilerGeneratedMember (src); bool expectedKept = ShouldMethodBeKept (src); - VerifyMethodInternal (src, linked, expectedKept); + VerifyMethodInternal (src, linked, expectedKept, compilerGenerated); } - private void VerifyMethodInternal (MethodDefinition src, MethodDesc? linked, bool expectedKept) + private void VerifyMethodInternal (MethodDefinition src, LinkedMethodEntity? linked, bool expectedKept, bool compilerGenerated) { if (!expectedKept) { - if (linked != null) - Assert.True (false, $"Method `{NameUtils.GetExpectedOriginDisplayName (src)}' should have been removed"); + if (linked == null) + return; - return; + // Similar to comment on types, compiler-generated methods can't be annotated with Kept attribute directly + // so we're not going to validate kept/remove on them. Note that we're still going to go validate "into" them + // to check for other properties (like parameter name presence/removal for example) + if (!compilerGenerated) + Assert.True (false, $"Method `{NameUtils.GetExpectedOriginDisplayName (src)}' should have been removed"); } - VerifyMethodKept (src, linked); + VerifyMethodKept (src, linked, compilerGenerated); } private void VerifyMemberBackingField (IMemberDefinition src, TypeDesc? linkedType) @@ -598,12 +693,12 @@ namespace Mono.Linker.Tests.TestCasesRunner return; } - VerifyFieldKept (srcField, linkedType?.GetFields ()?.FirstOrDefault (l => srcField.Name == l.Name)); + VerifyFieldKept (srcField, linkedType?.GetFields ()?.FirstOrDefault (l => srcField.Name == l.Name), compilerGenerated: true); verifiedGeneratedFields.Add (srcField.FullName); linkedMembers.Remove (new AssemblyQualifiedToken (srcField)); } - protected virtual void VerifyMethodKept (MethodDefinition src, MethodDesc? linked) + void VerifyMethodKept (MethodDefinition src, LinkedMethodEntity? linked, bool compilerGenerated) { if (linked == null) { Assert.True (false, $"Method `{NameUtils.GetExpectedOriginDisplayName (src)}' should have been kept"); @@ -613,14 +708,19 @@ namespace Mono.Linker.Tests.TestCasesRunner #if false VerifyPseudoAttributes (src, linked); VerifyGenericParameters (src, linked); - VerifyCustomAttributes (src, linked); - VerifyCustomAttributes (src.MethodReturnType, linked.MethodReturnType); + if (!compilerGenerated) { + VerifyCustomAttributes (src, linked); + VerifyCustomAttributes (src.MethodReturnType, linked.MethodReturnType); + } +#endif VerifyParameters (src, linked); +#if false VerifySecurityAttributes (src, linked); VerifyArrayInitializers (src, linked); // Method bodies are not very different in Native AOT VerifyMethodBody (src, linked); + VerifyKeptByAttributes (src, linked); #endif } @@ -1117,21 +1217,23 @@ namespace Mono.Linker.Tests.TestCasesRunner } } - private void VerifyParameters (IMethodSignature src, IMethodSignature linked) + private void VerifyParameters (IMethodSignature src, LinkedMethodEntity linked) { - Assert.Equal (src.HasParameters, linked.HasParameters); + Assert.Equal (src.HasParameters, linked.Method.Signature.Length > 0); if (src.HasParameters) { for (int i = 0; i < src.Parameters.Count; ++i) { var srcp = src.Parameters[i]; - var lnkp = linked.Parameters[i]; + //var lnkp = linked.Parameters[i]; +#if false VerifyCustomAttributes (srcp, lnkp); +#endif if (checkNames) { if (srcp.CustomAttributes.Any (attr => attr.AttributeType.Name == nameof (RemovedNameValueAttribute))) - lnkp.Name.Should ().BeEmpty ("Expected empty parameter name"); + linked.IsReflected.Should ().BeFalse ($"Expected no parameter name (non-reflectable). Parameter {i} of {(src as MethodDefinition)}"); else - lnkp.Name.Should ().Be (srcp.Name, "Mismatch in parameter name"); + linked.IsReflected.Should ().BeTrue ($"Expected accessible parameter name (reflectable). Parameter {i} of {(src as MethodDefinition)}"); } } } diff --git a/src/tools/illink/src/linker/Linker.Steps/MarkStep.cs b/src/tools/illink/src/linker/Linker.Steps/MarkStep.cs index 515a3018fc4..0db018aeb1a 100644 --- a/src/tools/illink/src/linker/Linker.Steps/MarkStep.cs +++ b/src/tools/illink/src/linker/Linker.Steps/MarkStep.cs @@ -1088,7 +1088,7 @@ namespace Mono.Linker.Steps continue; if (signature == null) { - MarkIndirectlyCalledMethod (m, reason, ScopeStack.CurrentScope.Origin); + MarkMethodVisibleToReflection (m, reason, ScopeStack.CurrentScope.Origin); marked = true; continue; } @@ -1107,7 +1107,7 @@ namespace Mono.Linker.Steps if (!matched) continue; - MarkIndirectlyCalledMethod (m, reason, ScopeStack.CurrentScope.Origin); + MarkMethodVisibleToReflection (m, reason, ScopeStack.CurrentScope.Origin); marked = true; } @@ -1871,13 +1871,16 @@ namespace Mono.Linker.Steps return MarkType (type, reason, origin); } - internal void MarkMethodVisibleToReflection (MethodDefinition method, in DependencyInfo reason, in MessageOrigin origin) + internal void MarkMethodVisibleToReflection (MethodReference method, in DependencyInfo reason, in MessageOrigin origin) { - MarkIndirectlyCalledMethod (method, reason, origin); - Annotations.MarkReflectionUsed (method); + MarkMethod (method, reason, origin); + if (Context.Resolve (method) is MethodDefinition methodDefinition) { + Annotations.MarkReflectionUsed (methodDefinition); + Annotations.MarkIndirectlyCalledMethod (methodDefinition); + } } - internal void MarkFieldVisibleToReflection (FieldDefinition field, in DependencyInfo reason, in MessageOrigin origin) + internal void MarkFieldVisibleToReflection (FieldReference field, in DependencyInfo reason, in MessageOrigin origin) { MarkField (field, reason, origin); } @@ -2914,12 +2917,6 @@ namespace Mono.Linker.Steps MarkMethod (method, reason, ScopeStack.CurrentScope.Origin); } - protected internal void MarkIndirectlyCalledMethod (MethodDefinition method, in DependencyInfo reason, in MessageOrigin origin) - { - MarkMethod (method, reason, origin); - Annotations.MarkIndirectlyCalledMethod (method); - } - protected virtual MethodDefinition? MarkMethod (MethodReference reference, DependencyInfo reason, in MessageOrigin origin) { DependencyKind originalReasonKind = reason.Kind; @@ -3692,21 +3689,28 @@ namespace Mono.Linker.Steps break; case OperandType.InlineMethod: { - DependencyKind dependencyKind = instruction.OpCode.Code switch { - Code.Jmp => DependencyKind.DirectCall, - Code.Call => DependencyKind.DirectCall, - Code.Callvirt => DependencyKind.VirtualCall, - Code.Newobj => DependencyKind.Newobj, - Code.Ldvirtftn => DependencyKind.Ldvirtftn, - Code.Ldftn => DependencyKind.Ldftn, + (DependencyKind dependencyKind, bool markForReflectionAccess) = instruction.OpCode.Code switch { + Code.Jmp => (DependencyKind.DirectCall, false), + Code.Call => (DependencyKind.DirectCall, false), + Code.Callvirt => (DependencyKind.VirtualCall, false), + Code.Newobj => (DependencyKind.Newobj, false), + Code.Ldvirtftn => (DependencyKind.Ldvirtftn, true), + Code.Ldftn => (DependencyKind.Ldftn, true), _ => throw new InvalidOperationException ($"unexpected opcode {instruction.OpCode}") }; + MethodReference methodReference = (MethodReference) instruction.Operand; + requiresReflectionMethodBodyScanner |= - ReflectionMethodBodyScanner.RequiresReflectionMethodBodyScannerForCallSite (Context, (MethodReference) instruction.Operand); + ReflectionMethodBodyScanner.RequiresReflectionMethodBodyScannerForCallSite (Context, methodReference); ScopeStack.UpdateCurrentScopeInstructionOffset (instruction.Offset); - MarkMethod ((MethodReference) instruction.Operand, new DependencyInfo (dependencyKind, method), ScopeStack.CurrentScope.Origin); + if (markForReflectionAccess) { + MarkMethodVisibleToReflection (methodReference, new DependencyInfo (dependencyKind, method), ScopeStack.CurrentScope.Origin); + } + else { + MarkMethod (methodReference, new DependencyInfo (dependencyKind, method), ScopeStack.CurrentScope.Origin); + } break; } @@ -3721,9 +3725,9 @@ namespace Mono.Linker.Steps if (Context.TryResolve (typeReference) is TypeDefinition type) MarkTypeVisibleToReflection (typeReference, type, reason, ScopeStack.CurrentScope.Origin); } else if (token is MethodReference methodReference) { - MarkMethod (methodReference, reason, ScopeStack.CurrentScope.Origin); + MarkMethodVisibleToReflection (methodReference, reason, ScopeStack.CurrentScope.Origin); } else { - MarkField ((FieldReference) token, reason, ScopeStack.CurrentScope.Origin); + MarkFieldVisibleToReflection ((FieldReference) token, reason, ScopeStack.CurrentScope.Origin); } break; } diff --git a/src/tools/illink/test/Mono.Linker.Tests.Cases/Reflection/ParametersUsedViaReflection.cs b/src/tools/illink/test/Mono.Linker.Tests.Cases/Reflection/ParametersUsedViaReflection.cs index 57b16569acb..272e44a4114 100644 --- a/src/tools/illink/test/Mono.Linker.Tests.Cases/Reflection/ParametersUsedViaReflection.cs +++ b/src/tools/illink/test/Mono.Linker.Tests.Cases/Reflection/ParametersUsedViaReflection.cs @@ -1,10 +1,12 @@ using System; +using System.Linq.Expressions; using System.Reflection; using Mono.Linker.Tests.Cases.Expectations.Assertions; using Mono.Linker.Tests.Cases.Expectations.Metadata; namespace Mono.Linker.Tests.Cases.Reflection { + [ExpectedNoWarnings] public class ParametersUsedViaReflection { public static void Main () @@ -21,17 +23,28 @@ namespace Mono.Linker.Tests.Cases.Reflection GetMethod_Name.CalledDirectly (11); GetMethod_Name.CalledDirectly2 (1); + + Action action = GetMethod_Name.OnlyUsedViaDelegate; + name = action.Method.GetParameters ()[0].Name; + + GetMethod_Name instance = new GetMethod_Name (); + action = instance.OnlyUseViaDelegateVirt; + + Expression ex = () => GetMethod_Name.OnlyUsedViaLdToken (42); + + TestLambdaUsage ((int firstName) => { firstName.ToString (); }); } [Kept] static void TestClassParameters () { - var type = Type.GetType ("Mono.Linker.Tests.Cases.Reflection.ParametersUsedViaReflection/GenericClass1`1"); + var type = Type.GetType ("Mono.Linker.Tests.Cases.Reflection.ParametersUsedViaReflection+GenericClass1`1"); var type2 = new GenericClass2 (); } [Kept] + [KeptMember (".ctor()")] class GetMethod_Name { [Kept] @@ -55,6 +68,27 @@ namespace Mono.Linker.Tests.Cases.Reflection public static void CalledDirectly2 ([RemovedNameValue] int firstArg) { } + + [Kept] + public static void OnlyUsedViaDelegate (int firstName) + { + } + + [Kept] + public virtual void OnlyUseViaDelegateVirt (int firstName) + { + } + + [Kept] + public static void OnlyUsedViaLdToken (int firstName) + { + } + } + + [Kept] + public static void TestLambdaUsage ([RemovedNameValue] Delegate action) + { + var n = action.Method.GetParameters ()[0].Name; } [Kept] diff --git a/src/tools/illink/test/Mono.Linker.Tests/TestCasesRunner/AssemblyChecker.cs b/src/tools/illink/test/Mono.Linker.Tests/TestCasesRunner/AssemblyChecker.cs index 9c9e9dd3314..363c089f67f 100644 --- a/src/tools/illink/test/Mono.Linker.Tests/TestCasesRunner/AssemblyChecker.cs +++ b/src/tools/illink/test/Mono.Linker.Tests/TestCasesRunner/AssemblyChecker.cs @@ -7,6 +7,8 @@ using System.Linq; using System.Text; using Mono.Cecil; using Mono.Cecil.Cil; +using Mono.Linker.Dataflow; +using Mono.Linker.Tests.Cases.CppCLI; using Mono.Linker.Tests.Cases.Expectations.Assertions; using Mono.Linker.Tests.Extensions; using NUnit.Framework; @@ -82,6 +84,24 @@ namespace Mono.Linker.Tests.TestCasesRunner Assert.IsEmpty (linkedMembers, "Linked output includes unexpected member"); } + static bool IsCompilerGeneratedMemberName (string memberName) + { + return memberName.Length > 0 && memberName[0] == '<'; + } + + static bool IsCompilerGeneratedMember (IMemberDefinition member) + { + if (IsCompilerGeneratedMemberName (member.Name)) + return true; + + if (member.DeclaringType != null) + return IsCompilerGeneratedMember (member.DeclaringType); + + return false; + } + + static bool IsBackingField (FieldDefinition field) => field.Name.StartsWith ("<") && field.Name.EndsWith (">k__BackingField"); + protected virtual void VerifyModule (ModuleDefinition original, ModuleDefinition linked) { // We never link away a module today so let's make sure the linked one isn't null @@ -120,10 +140,19 @@ namespace Mono.Linker.Tests.TestCasesRunner original.AllMembers ().Any (HasActiveKeptDerivedAttribute); if (!expectedKept) { - if (linked != null) - Assert.Fail ($"Type `{original}' should have been removed"); + if (linked == null) + return; - return; + // Compiler generated members can't be annotated with `Kept` attributes directly + // For some of them we have special attributes (backing fields for example), but it's impractical to define + // special attributes for all types of compiler generated members (there are quite a few of them and they're + // going to change/increase over time). + // So we're effectively disabling Kept validation on compiler generated members + // Note that we still want to go "inside" each such member, as it might have additional attributes + // we do want to validate. There's no specific use case right now, but I can easily imagine one + // for more detailed testing of for example custom attributes on local functions, or similar. + if (!IsCompilerGeneratedMember (original)) + Assert.Fail ($"Type `{original}' should have been removed"); } bool prev = checkNames; @@ -211,17 +240,20 @@ namespace Mono.Linker.Tests.TestCasesRunner if (linked == null) Assert.Fail ($"Type `{original}' should have been kept"); - VerifyKeptByAttributes (original, linked); - if (!original.IsInterface) - VerifyBaseType (original, linked); + // Skip verification of type metadata for compiler generated types (we don't currently need it yet) + if (!IsCompilerGeneratedMember (original)) { + VerifyKeptByAttributes (original, linked); + if (!original.IsInterface) + VerifyBaseType (original, linked); - VerifyInterfaces (original, linked); - VerifyPseudoAttributes (original, linked); - VerifyGenericParameters (original, linked); - VerifyCustomAttributes (original, linked); - VerifySecurityAttributes (original, linked); + VerifyInterfaces (original, linked); + VerifyPseudoAttributes (original, linked); + VerifyGenericParameters (original, linked); + VerifyCustomAttributes (original, linked); + VerifySecurityAttributes (original, linked); - VerifyFixedBufferFields (original, linked); + VerifyFixedBufferFields (original, linked); + } // Need to check delegate cache fields before the normal field check VerifyDelegateBackingFields (original, linked); @@ -362,7 +394,9 @@ namespace Mono.Linker.Tests.TestCasesRunner void VerifyField (FieldDefinition src, FieldDefinition linked) { - bool expectedKept = ShouldBeKept (src); + bool compilerGenerated = IsCompilerGeneratedMember (src); + bool expectedKept = ShouldBeKept (src) || + (compilerGenerated ? !IsBackingField (src) : false); if (!expectedKept) { if (linked != null) @@ -371,10 +405,10 @@ namespace Mono.Linker.Tests.TestCasesRunner return; } - VerifyFieldKept (src, linked); + VerifyFieldKept (src, linked, compilerGenerated); } - void VerifyFieldKept (FieldDefinition src, FieldDefinition linked) + void VerifyFieldKept (FieldDefinition src, FieldDefinition linked, bool compilerGenerated) { if (linked == null) Assert.Fail ($"Field `{src}' should have been kept"); @@ -383,14 +417,16 @@ namespace Mono.Linker.Tests.TestCasesRunner VerifyKeptByAttributes (src, linked); VerifyPseudoAttributes (src, linked); - VerifyCustomAttributes (src, linked); + if (!compilerGenerated) + VerifyCustomAttributes (src, linked); } void VerifyProperty (PropertyDefinition src, PropertyDefinition linked, TypeDefinition linkedType) { VerifyMemberBackingField (src, linkedType); - bool expectedKept = ShouldBeKept (src); + bool compilerGenerated = IsCompilerGeneratedMember (src); + bool expectedKept = ShouldBeKept (src) || compilerGenerated; if (!expectedKept) { if (linked != null) @@ -406,14 +442,16 @@ namespace Mono.Linker.Tests.TestCasesRunner VerifyKeptByAttributes (src, linked); VerifyPseudoAttributes (src, linked); - VerifyCustomAttributes (src, linked); + if (!compilerGenerated) + VerifyCustomAttributes (src, linked); } void VerifyEvent (EventDefinition src, EventDefinition linked, TypeDefinition linkedType) { VerifyMemberBackingField (src, linkedType); - bool expectedKept = ShouldBeKept (src); + bool compilerGenerated = IsCompilerGeneratedMember (src); + bool expectedKept = ShouldBeKept (src) || compilerGenerated; if (!expectedKept) { if (linked != null) @@ -426,39 +464,44 @@ namespace Mono.Linker.Tests.TestCasesRunner Assert.Fail ($"Event `{src}' should have been kept"); if (src.CustomAttributes.Any (attr => attr.AttributeType.Name == nameof (KeptEventAddMethodAttribute))) { - VerifyMethodInternal (src.AddMethod, linked.AddMethod, true); + VerifyMethodInternal (src.AddMethod, linked.AddMethod, true, compilerGenerated); verifiedEventMethods.Add (src.AddMethod.FullName); linkedMembers.Remove (src.AddMethod.FullName); } if (src.CustomAttributes.Any (attr => attr.AttributeType.Name == nameof (KeptEventRemoveMethodAttribute))) { - VerifyMethodInternal (src.RemoveMethod, linked.RemoveMethod, true); + VerifyMethodInternal (src.RemoveMethod, linked.RemoveMethod, true, compilerGenerated); verifiedEventMethods.Add (src.RemoveMethod.FullName); linkedMembers.Remove (src.RemoveMethod.FullName); } VerifyKeptByAttributes (src, linked); VerifyPseudoAttributes (src, linked); - VerifyCustomAttributes (src, linked); + if (!compilerGenerated) + VerifyCustomAttributes (src, linked); } void VerifyMethod (MethodDefinition src, MethodDefinition linked) { + bool compilerGenerated = IsCompilerGeneratedMember (src); bool expectedKept = ShouldMethodBeKept (src); - VerifyMethodInternal (src, linked, expectedKept); + VerifyMethodInternal (src, linked, expectedKept, compilerGenerated); } - - void VerifyMethodInternal (MethodDefinition src, MethodDefinition linked, bool expectedKept) + void VerifyMethodInternal (MethodDefinition src, MethodDefinition linked, bool expectedKept, bool compilerGenerated) { if (!expectedKept) { - if (linked != null) - Assert.Fail ($"Method `{src.FullName}' should have been removed"); + if (linked == null) + return; - return; + // Similar to comment on types, compiler-generated methods can't be annotated with Kept attribute directly + // so we're not going to validate kept/remove on them. Note that we're still going to go validate "into" them + // to check for other properties (like parameter name presence/removal for example) + if (!compilerGenerated) + Assert.Fail ($"Method `{src.FullName}' should have been removed"); } - VerifyMethodKept (src, linked); + VerifyMethodKept (src, linked, compilerGenerated); } void VerifyMemberBackingField (IMemberDefinition src, TypeDefinition linkedType) @@ -483,20 +526,22 @@ namespace Mono.Linker.Tests.TestCasesRunner if (srcField == null) Assert.Fail ($"{src.MetadataToken.TokenType} `{src}', could not locate the expected backing field {backingFieldName}"); - VerifyFieldKept (srcField, linkedType?.Fields.FirstOrDefault (l => srcField.Name == l.Name)); + VerifyFieldKept (srcField, linkedType?.Fields.FirstOrDefault (l => srcField.Name == l.Name), compilerGenerated: true); verifiedGeneratedFields.Add (srcField.FullName); linkedMembers.Remove (srcField.FullName); } - protected virtual void VerifyMethodKept (MethodDefinition src, MethodDefinition linked) + protected virtual void VerifyMethodKept (MethodDefinition src, MethodDefinition linked, bool compilerGenerated) { if (linked == null) Assert.Fail ($"Method `{src.FullName}' should have been kept"); VerifyPseudoAttributes (src, linked); VerifyGenericParameters (src, linked); - VerifyCustomAttributes (src, linked); - VerifyCustomAttributes (src.MethodReturnType, linked.MethodReturnType); + if (!compilerGenerated) { + VerifyCustomAttributes (src, linked); + VerifyCustomAttributes (src.MethodReturnType, linked.MethodReturnType); + } VerifyParameters (src, linked); VerifySecurityAttributes (src, linked); VerifyArrayInitializers (src, linked); @@ -807,7 +852,7 @@ namespace Mono.Linker.Tests.TestCasesRunner Assert.Fail ($"Could not locate original private implementation details method {methodName}"); var linkedMethod = linkedImplementationDetails.Methods.FirstOrDefault (m => m.Name == methodName); - VerifyMethodKept (originalMethod, linkedMethod); + VerifyMethodKept (originalMethod, linkedMethod, compilerGenerated: true); linkedMembers.Remove (linkedMethod.FullName); } verifiedGeneratedTypes.Add (srcImplementationDetails.FullName); @@ -870,7 +915,7 @@ namespace Mono.Linker.Tests.TestCasesRunner void VerifyInitializerField (FieldDefinition src, FieldDefinition linked) { - VerifyFieldKept (src, linked); + VerifyFieldKept (src, linked, compilerGenerated: true); verifiedGeneratedFields.Add (linked.FullName); linkedMembers.Remove (linked.FullName); VerifyTypeDefinitionKept (src.FieldType.Resolve (), linked.FieldType.Resolve ()); @@ -970,7 +1015,7 @@ namespace Mono.Linker.Tests.TestCasesRunner Assert.Fail ($"Could not locate original compiler generated FixedElementField on {originalCompilerGeneratedBufferType}"); var linkedField = linkedCompilerGeneratedBufferType?.Fields.FirstOrDefault (); - VerifyFieldKept (originalElementField, linkedField); + VerifyFieldKept (originalElementField, linkedField, compilerGenerated: true); verifiedGeneratedFields.Add (originalElementField.FullName); linkedMembers.Remove (linkedField.FullName); @@ -1001,7 +1046,7 @@ namespace Mono.Linker.Tests.TestCasesRunner Assert.Fail ($"Invalid expected delegate backing field {expectedFieldName} in {src}. This member was not in the unlinked assembly"); var linkedField = linkedNestedType?.Fields.FirstOrDefault (f => f.Name == expectedFieldName); - VerifyFieldKept (originalField, linkedField); + VerifyFieldKept (originalField, linkedField, compilerGenerated: true); verifiedGeneratedFields.Add (linkedField.FullName); linkedMembers.Remove (linkedField.FullName); } @@ -1045,9 +1090,9 @@ namespace Mono.Linker.Tests.TestCasesRunner if (checkNames) { if (srcp.CustomAttributes.Any (attr => attr.AttributeType.Name == nameof (RemovedNameValueAttribute))) - Assert.IsEmpty (lnkp.Name, "Expected empty parameter name"); + Assert.IsEmpty (lnkp.Name, $"Expected empty parameter name. Parameter {i} of {(src as MethodDefinition)}"); else - Assert.AreEqual (srcp.Name, lnkp.Name, "Mismatch in parameter name"); + Assert.AreEqual (srcp.Name, lnkp.Name, $"Mismatch in parameter name. Parameter {i} of {(src as MethodDefinition)}"); } } } -- 2.34.1