diff options
author | Jackson Schuster <36744439+jtschuster@users.noreply.github.com> | 2022-04-20 00:57:55 +0300 |
---|---|---|
committer | GitHub <noreply@github.com> | 2022-04-20 00:57:55 +0300 |
commit | c2fff337c62780b2c84d5fda975a4e001bb6fe8b (patch) | |
tree | e09516a4d77ddc85756017926d75dcfeddbe5642 | |
parent | 5c69665bbb929b8decacb266dc2edeb3e32b9858 (diff) |
Move parameter annotation checks to shared code for analyzer (#2707)
* Move parameter annotation checks to shared code for analyzer
Add GetMethodThisParameterValue
Make GetParameterAnnotations return a nonnull
DynamicallyAccessedMemberTypes
* Use the shared code for parameter annotation checks in the linker
* Remove redundant handling from analyzer
* Use ReturnsVoid() extension method
* Fall back to annotations for unimplemented intrinsics in analyzer
10 files changed, 111 insertions, 56 deletions
diff --git a/src/ILLink.RoslynAnalyzer/TrimAnalysis/HandleCallAction.cs b/src/ILLink.RoslynAnalyzer/TrimAnalysis/HandleCallAction.cs index 0a4a9b62a..d46481268 100644 --- a/src/ILLink.RoslynAnalyzer/TrimAnalysis/HandleCallAction.cs +++ b/src/ILLink.RoslynAnalyzer/TrimAnalysis/HandleCallAction.cs @@ -47,6 +47,15 @@ namespace ILLink.Shared.TrimAnalysis private partial MethodThisParameterValue GetMethodThisParameterValue (MethodProxy method, DynamicallyAccessedMemberTypes dynamicallyAccessedMemberTypes) => new (method.Method, dynamicallyAccessedMemberTypes); + private partial MethodThisParameterValue GetMethodThisParameterValue (MethodProxy method) + => GetMethodThisParameterValue (method, method.Method.GetDynamicallyAccessedMemberTypes ()); + + private partial DynamicallyAccessedMemberTypes GetMethodParameterAnnotation (MethodProxy method, int parameterIndex) + { + Debug.Assert (method.Method.Parameters.Length > parameterIndex); + return FlowAnnotations.GetMethodParameterAnnotation (method.Method.Parameters[parameterIndex]); + } + private partial MethodParameterValue GetMethodParameterValue (MethodProxy method, int parameterIndex, DynamicallyAccessedMemberTypes dynamicallyAccessedMemberTypes) => new (method.Method.Parameters[parameterIndex], dynamicallyAccessedMemberTypes); @@ -62,6 +71,19 @@ namespace ILLink.Shared.TrimAnalysis yield return new SystemTypeValue (new TypeProxy (nestedType)); } + private partial bool MethodIsTypeConstructor (MethodProxy method) + { + if (!method.Method.IsConstructor ()) + return false; + var type = method.Method.ContainingType; + while (type is not null) { + if (type.IsTypeOf (WellKnownType.System_Type)) + return true; + type = type.BaseType; + } + return false; + } + private partial bool TryGetBaseType (TypeProxy type, out TypeProxy? baseType) { if (type.Type.BaseType is not null) { diff --git a/src/ILLink.RoslynAnalyzer/TrimAnalysis/TrimAnalysisVisitor.cs b/src/ILLink.RoslynAnalyzer/TrimAnalysis/TrimAnalysisVisitor.cs index 4e3cda618..13fc5ca6c 100644 --- a/src/ILLink.RoslynAnalyzer/TrimAnalysis/TrimAnalysisVisitor.cs +++ b/src/ILLink.RoslynAnalyzer/TrimAnalysis/TrimAnalysisVisitor.cs @@ -241,6 +241,7 @@ namespace ILLink.RoslynAnalyzer.TrimAnalysis var diagnosticContext = DiagnosticContext.CreateDisabled (); var handleCallAction = new HandleCallAction (diagnosticContext, Context.OwningSymbol, operation); + if (!handleCallAction.Invoke (new MethodProxy (calledMethod), instance, arguments, out MultiValue methodReturnValue, out var intrinsicId)) { switch (intrinsicId) { case IntrinsicId.Array_Empty: diff --git a/src/ILLink.Shared/TrimAnalysis/HandleCallAction.cs b/src/ILLink.Shared/TrimAnalysis/HandleCallAction.cs index 28e79650d..a7f422292 100644 --- a/src/ILLink.Shared/TrimAnalysis/HandleCallAction.cs +++ b/src/ILLink.Shared/TrimAnalysis/HandleCallAction.cs @@ -787,8 +787,16 @@ namespace ILLink.Shared.TrimAnalysis break; case IntrinsicId.None: - methodReturnValue = MultiValueLattice.Top; - return false; + // Verify the argument values match the annotations on the parameter definition + if (requiresDataFlowAnalysis) { + if (!calledMethod.IsStatic ()) { + _requireDynamicallyAccessedMembersAction.Invoke (instanceValue, GetMethodThisParameterValue (calledMethod)); + } + for (int argumentIndex = 0; argumentIndex < argumentValues.Count; argumentIndex++) { + _requireDynamicallyAccessedMembersAction.Invoke (argumentValues[argumentIndex], GetMethodParameterValue (calledMethod, argumentIndex)); + } + } + break; // Disable warnings for all unimplemented intrinsics. Some intrinsic methods have annotations, but analyzing them // would produce unnecessary warnings even for cases that are intrinsically handled. So we disable handling these calls @@ -802,6 +810,9 @@ namespace ILLink.Shared.TrimAnalysis // Note that this will be DynamicallyAccessedMembers.None for the intrinsics which don't return types. returnValue ??= calledMethod.ReturnsVoid () ? MultiValueLattice.Top : GetMethodReturnValue (calledMethod, returnValueDynamicallyAccessedMemberTypes); + if (MethodIsTypeConstructor (calledMethod)) + returnValue = UnknownValue.Instance; + // Validate that the return value has the correct annotations as per the method return value annotations if (returnValueDynamicallyAccessedMemberTypes != 0) { foreach (var uniqueValue in returnValue.Value) { @@ -918,6 +929,11 @@ namespace ILLink.Shared.TrimAnalysis private partial bool MethodRequiresDataFlowAnalysis (MethodProxy method); + /// <Summary> + /// Returns true if the method is a .ctor for System.Type or a type that derives from System.Type (i.e. fields and params of this type can have DynamicallyAccessedMembers annotations) + /// </Summary> + private partial bool MethodIsTypeConstructor (MethodProxy method); + private partial DynamicallyAccessedMemberTypes GetReturnValueAnnotation (MethodProxy method); private partial MethodReturnValue GetMethodReturnValue (MethodProxy method, DynamicallyAccessedMemberTypes dynamicallyAccessedMemberTypes); @@ -926,8 +942,15 @@ namespace ILLink.Shared.TrimAnalysis private partial MethodThisParameterValue GetMethodThisParameterValue (MethodProxy method, DynamicallyAccessedMemberTypes dynamicallyAccessedMemberTypes); + private partial MethodThisParameterValue GetMethodThisParameterValue (MethodProxy method); + + private partial DynamicallyAccessedMemberTypes GetMethodParameterAnnotation (MethodProxy method, int parameterIndex); + private partial MethodParameterValue GetMethodParameterValue (MethodProxy method, int parameterIndex, DynamicallyAccessedMemberTypes dynamicallyAccessedMemberTypes); + private MethodParameterValue GetMethodParameterValue (MethodProxy method, int parameterIndex) + => GetMethodParameterValue (method, parameterIndex, GetMethodParameterAnnotation (method, parameterIndex)!); + private partial IEnumerable<SystemReflectionMethodBaseValue> GetMethodsOnTypeHierarchy (TypeProxy type, string name, BindingFlags? bindingFlags); private partial IEnumerable<SystemTypeValue> GetNestedTypesOnType (TypeProxy type, string name, BindingFlags? bindingFlags); diff --git a/src/linker/Linker.Dataflow/HandleCallAction.cs b/src/linker/Linker.Dataflow/HandleCallAction.cs index f662cdf0b..ba3f56dc8 100644 --- a/src/linker/Linker.Dataflow/HandleCallAction.cs +++ b/src/linker/Linker.Dataflow/HandleCallAction.cs @@ -2,6 +2,7 @@ // Licensed under the MIT license. See LICENSE file in the project root for full license information. using System.Collections.Generic; +using System.Diagnostics; using System.Diagnostics.CodeAnalysis; using System.Reflection; using ILLink.Shared.TypeSystemProxy; @@ -37,6 +38,19 @@ namespace ILLink.Shared.TrimAnalysis private partial bool MethodRequiresDataFlowAnalysis (MethodProxy method) => _context.Annotations.FlowAnnotations.RequiresDataFlowAnalysis (method.Method); + private partial bool MethodIsTypeConstructor (MethodProxy method) + { + if (!method.Method.IsConstructor) + return false; + TypeDefinition? type = method.Method.DeclaringType; + while (type is not null) { + if (type.IsTypeOf (WellKnownType.System_Type)) + return true; + type = _context.Resolve (type.BaseType); + } + return false; + } + private partial DynamicallyAccessedMemberTypes GetReturnValueAnnotation (MethodProxy method) => _context.Annotations.FlowAnnotations.GetReturnParameterAnnotation (method.Method); @@ -46,9 +60,18 @@ namespace ILLink.Shared.TrimAnalysis private partial GenericParameterValue GetGenericParameterValue (GenericParameterProxy genericParameter) => new (genericParameter.GenericParameter, _context.Annotations.FlowAnnotations.GetGenericParameterAnnotation (genericParameter.GenericParameter)); + private partial MethodThisParameterValue GetMethodThisParameterValue (MethodProxy method) + => GetMethodThisParameterValue (method, _context.Annotations.FlowAnnotations.GetParameterAnnotation (method.Method, 0)); + private partial MethodThisParameterValue GetMethodThisParameterValue (MethodProxy method, DynamicallyAccessedMemberTypes dynamicallyAccessedMemberTypes) => new (method.Method, dynamicallyAccessedMemberTypes); + private partial DynamicallyAccessedMemberTypes GetMethodParameterAnnotation (MethodProxy method, int parameterIndex) + { + Debug.Assert (method.Method.Parameters.Count > parameterIndex); + return _context.Annotations.FlowAnnotations.GetParameterAnnotation (method.Method, parameterIndex + (method.IsStatic () ? 0 : 1)); + } + private partial MethodParameterValue GetMethodParameterValue (MethodProxy method, int parameterIndex, DynamicallyAccessedMemberTypes dynamicallyAccessedMemberTypes) => new ( ReflectionMethodBodyScanner.ResolveToTypeDefinition (_context, method.Method.Parameters[parameterIndex].ParameterType), diff --git a/src/linker/Linker.Dataflow/MethodBodyScanner.cs b/src/linker/Linker.Dataflow/MethodBodyScanner.cs index aab50423a..4a9e5e2f5 100644 --- a/src/linker/Linker.Dataflow/MethodBodyScanner.cs +++ b/src/linker/Linker.Dataflow/MethodBodyScanner.cs @@ -537,7 +537,7 @@ namespace Mono.Linker.Dataflow // Pop function pointer PopUnknown (currentStack, 1, methodBody, operation.Offset); - if (GetReturnTypeWithoutModifiers (signature.ReturnType).MetadataType != MetadataType.Void) + if (!signature.ReturnsVoid ()) PushUnknown (currentStack); } break; @@ -573,7 +573,7 @@ namespace Mono.Linker.Dataflow case Code.Ret: { - bool hasReturnValue = GetReturnTypeWithoutModifiers (methodBody.Method.ReturnType).MetadataType != MetadataType.Void; + bool hasReturnValue = !methodBody.Method.ReturnsVoid (); if (currentStack.Count != (hasReturnValue ? 1 : 0)) { WarnAboutInvalidILInMethod (methodBody, operation.Offset); @@ -917,13 +917,13 @@ namespace Mono.Linker.Dataflow else methodReturnValue = newObjValue; } else { - if (GetReturnTypeWithoutModifiers (calledMethod.ReturnType).MetadataType != MetadataType.Void) { + if (!calledMethod.ReturnsVoid ()) { methodReturnValue = UnknownValue.Instance; } } } - if (isNewObj || GetReturnTypeWithoutModifiers (calledMethod.ReturnType).MetadataType != MetadataType.Void) + if (isNewObj || !calledMethod.ReturnsVoid ()) currentStack.Push (new StackSlot (methodReturnValue, calledMethod.ReturnType.IsByRefOrPointer ())); foreach (var param in methodParams) { @@ -935,14 +935,6 @@ namespace Mono.Linker.Dataflow } } - protected static TypeReference GetReturnTypeWithoutModifiers (TypeReference returnType) - { - while (returnType is IModifierType) { - returnType = ((IModifierType) returnType).ElementType; - } - return returnType; - } - // Array types that are dynamically accessed should resolve to System.Array instead of its element type - which is what Cecil resolves to. // Any data flow annotations placed on a type parameter which receives an array type apply to the array itself. None of the members in its // element type should be marked. diff --git a/src/linker/Linker.Dataflow/MethodProxy.cs b/src/linker/Linker.Dataflow/MethodProxy.cs index a9b95ccf3..eacdf27a8 100644 --- a/src/linker/Linker.Dataflow/MethodProxy.cs +++ b/src/linker/Linker.Dataflow/MethodProxy.cs @@ -32,7 +32,7 @@ namespace ILLink.Shared.TypeSystemProxy internal partial bool IsStatic () => Method.IsStatic; - internal partial bool ReturnsVoid () => Method.ReturnType.MetadataType == MetadataType.Void; + internal partial bool ReturnsVoid () => Method.ReturnsVoid (); public override string ToString () => Method.ToString (); } diff --git a/src/linker/Linker.Dataflow/ReflectionMethodBodyScanner.cs b/src/linker/Linker.Dataflow/ReflectionMethodBodyScanner.cs index 440e1f562..e9e69fe47 100644 --- a/src/linker/Linker.Dataflow/ReflectionMethodBodyScanner.cs +++ b/src/linker/Linker.Dataflow/ReflectionMethodBodyScanner.cs @@ -70,7 +70,7 @@ namespace Mono.Linker.Dataflow { Scan (methodBody); - if (GetReturnTypeWithoutModifiers (methodBody.Method.ReturnType).MetadataType != MetadataType.Void) { + if (!methodBody.Method.ReturnsVoid ()) { var method = methodBody.Method; var methodReturnValue = GetMethodReturnValue (method); if (methodReturnValue.DynamicallyAccessedMemberTypes != 0) { @@ -311,6 +311,29 @@ namespace Mono.Linker.Dataflow return handleCallAction.Invoke (calledMethodDefinition, instanceValue, parameterValues, out methodReturnValue, out _); } + case IntrinsicId.None: { + if (calledMethodDefinition.IsPInvokeImpl) { + // Is the PInvoke dangerous? + bool comDangerousMethod = IsComInterop (calledMethodDefinition.MethodReturnType, calledMethodDefinition.ReturnType); + foreach (ParameterDefinition pd in calledMethodDefinition.Parameters) { + comDangerousMethod |= IsComInterop (pd, pd.ParameterType); + } + + if (comDangerousMethod) { + analysisContext.ReportWarning (DiagnosticId.CorrectnessOfCOMCannotBeGuaranteed, calledMethodDefinition.GetDisplayName ()); + } + } + _markStep.CheckAndReportRequiresUnreferencedCode (calledMethodDefinition); + + var instanceValue = MultiValueLattice.Top; + IReadOnlyList<MultiValue> parameterValues = methodParams; + if (calledMethodDefinition.HasImplicitThis ()) { + instanceValue = methodParams[0]; + parameterValues = parameterValues.Skip (1).ToImmutableList (); + } + return handleCallAction.Invoke (calledMethodDefinition, instanceValue, parameterValues, out methodReturnValue, out _); + } + case IntrinsicId.TypeDelegator_Ctor: { // This is an identity function for analysis purposes if (operation.OpCode == OpCodes.Newobj) @@ -757,46 +780,13 @@ namespace Mono.Linker.Dataflow break; default: - - if (calledMethodDefinition.IsPInvokeImpl) { - // Is the PInvoke dangerous? - bool comDangerousMethod = IsComInterop (calledMethodDefinition.MethodReturnType, calledMethodDefinition.ReturnType); - foreach (ParameterDefinition pd in calledMethodDefinition.Parameters) { - comDangerousMethod |= IsComInterop (pd, pd.ParameterType); - } - - if (comDangerousMethod) { - analysisContext.ReportWarning (DiagnosticId.CorrectnessOfCOMCannotBeGuaranteed, calledMethodDefinition.GetDisplayName ()); - } - } - - if (requiresDataFlowAnalysis) { - for (int parameterIndex = 0; parameterIndex < methodParams.Count; parameterIndex++) { - var targetValue = GetMethodParameterValue (calledMethodDefinition, parameterIndex); - - if (targetValue.DynamicallyAccessedMemberTypes != DynamicallyAccessedMemberTypes.None) { - RequireDynamicallyAccessedMembers (analysisContext, methodParams[parameterIndex], targetValue); - } - } - } - - _markStep.CheckAndReportRequiresUnreferencedCode (calledMethodDefinition); - - // To get good reporting of errors we need to track the origin of the value for all method calls - // but except Newobj as those are special. - if (GetReturnTypeWithoutModifiers (calledMethodDefinition.ReturnType).MetadataType != MetadataType.Void) { - methodReturnValue = GetMethodReturnValue (calledMethodDefinition, returnValueDynamicallyAccessedMemberTypes); - - return true; - } - - return false; + throw new NotImplementedException ("Unhandled instrinsic"); } // If we get here, we handled this as an intrinsic. As a convenience, if the code above // didn't set the return value (and the method has a return value), we will set it to be an // unknown value with the return type of the method. - bool returnsVoid = GetReturnTypeWithoutModifiers (calledMethod.ReturnType).MetadataType == MetadataType.Void; + bool returnsVoid = calledMethod.ReturnsVoid (); methodReturnValue = maybeMethodReturnValue ?? (returnsVoid ? MultiValueLattice.Top : GetMethodReturnValue (calledMethodDefinition, returnValueDynamicallyAccessedMemberTypes)); diff --git a/src/linker/Linker/MethodReferenceExtensions.cs b/src/linker/Linker/MethodReferenceExtensions.cs index d107ddf0a..be378a015 100644 --- a/src/linker/Linker/MethodReferenceExtensions.cs +++ b/src/linker/Linker/MethodReferenceExtensions.cs @@ -74,6 +74,11 @@ namespace Mono.Linker return method.ReturnType; } + public static bool ReturnsVoid (this IMethodSignature method) + { + return method.ReturnType.WithoutModifiers ().MetadataType == MetadataType.Void; + } + public static TypeReference? GetParameterType (this MethodReference method, int parameterIndex, LinkContext context) { if (method.DeclaringType is GenericInstanceType genericInstance) diff --git a/test/Mono.Linker.Tests.Cases/DataFlow/LocalDataFlow.cs b/test/Mono.Linker.Tests.Cases/DataFlow/LocalDataFlow.cs index f3da00afe..1f6347d3b 100644 --- a/test/Mono.Linker.Tests.Cases/DataFlow/LocalDataFlow.cs +++ b/test/Mono.Linker.Tests.Cases/DataFlow/LocalDataFlow.cs @@ -399,6 +399,7 @@ namespace Mono.Linker.Tests.Cases.DataFlow // https://github.com/dotnet/linker/issues/2273 // Analyzer doesn't see through foreach over array at all - will not warn [ExpectedWarning ("IL2063", ProducedBy = ProducedBy.Trimmer)] // The types loaded from the array don't have annotations, so the "return" should warn + [ExpectedWarning ("IL2073", ProducedBy = ProducedBy.Analyzer)] // Analyzer tracks resultType as the value from IEnumerable.Current.get() [return: DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] public static Type TestBackwardEdgeWithLdElem (Type[] types = null) { diff --git a/test/Mono.Linker.Tests.Cases/DataFlow/TypeBaseTypeDataFlow.cs b/test/Mono.Linker.Tests.Cases/DataFlow/TypeBaseTypeDataFlow.cs index a9174ecf5..2edf35569 100644 --- a/test/Mono.Linker.Tests.Cases/DataFlow/TypeBaseTypeDataFlow.cs +++ b/test/Mono.Linker.Tests.Cases/DataFlow/TypeBaseTypeDataFlow.cs @@ -70,11 +70,9 @@ namespace Mono.Linker.Tests.Cases.DataFlow derivedType.BaseType.RequiresAll (); } - // This is a very special case - normally there's basically no way to "new up" a Type instance via the "new" operator - // so the analyzer doesn't handle this case at all - meaning it sees it as empty value. - // Unlike linker which sees an unknown value and thus warns that it doesn't fulfill the All annotation. - // It's OK for the analyzer to be more forgiving in this case, due to the very special circumstances. - [ExpectedWarning ("IL2062", nameof (TestAllPropagated), ProducedBy = ProducedBy.Trimmer)] + // This is a very special case - normally there's basically no way to "new up" a Type instance via the "new" operator. + // The linker and analyzer see an unknown value and thus warns that it doesn't fulfill the All annotation. + [ExpectedWarning ("IL2062", nameof (TestAllPropagated))] public static void Test () { TestAllPropagated (new TestSystemTypeBase ()); |