Handle PInvoke analysis similar to other methods (#2091)

* PInvoke fix

* FB

* FB

* FB2

Author: LakshanF

src/linker/Linker.Dataflow/MethodBodyScanner.cs

@@ -526,13 +526,13 @@ public void Scan (MethodBody methodBody)
 						}
 
 						// Pop arguments
-						PopUnknown (currentStack, signature.Parameters.Count, methodBody, operation.Offset);
+						if (signature.Parameters.Count > 0)
+							PopUnknown (currentStack, signature.Parameters.Count, methodBody, operation.Offset);
 
 						// Pop function pointer
 						PopUnknown (currentStack, 1, methodBody, operation.Offset);
 
-						// Push return value
-						if (signature.ReturnType.MetadataType != MetadataType.Void)
+						if (GetReturnTypeWithoutModifiers (signature.ReturnType).MetadataType != MetadataType.Void)
 							PushUnknown (currentStack);
 					}
 					break;
@@ -567,7 +567,9 @@ public void Scan (MethodBody methodBody)
 					break;
 
 				case Code.Ret: {
-						bool hasReturnValue = methodBody.Method.ReturnType.MetadataType != MetadataType.Void;
+
+						bool hasReturnValue = GetReturnTypeWithoutModifiers (methodBody.Method.ReturnType).MetadataType != MetadataType.Void;
+
 						if (currentStack.Count != (hasReturnValue ? 1 : 0)) {
 							WarnAboutInvalidILInMethod (methodBody, operation.Offset);
 						}
@@ -892,7 +894,7 @@ private void HandleCall (
 					else
 						methodReturnValue = newObjValue;
 				} else {
-					if (calledMethod.ReturnType.MetadataType != MetadataType.Void) {
+					if (GetReturnTypeWithoutModifiers (calledMethod.ReturnType).MetadataType != MetadataType.Void) {
 						methodReturnValue = UnknownValue.Instance;
 					}
 				}
@@ -908,6 +910,14 @@ private void HandleCall (
 			}
 		}
 
+		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.

src/linker/Linker.Dataflow/ReflectionMethodBodyScanner.cs

@@ -37,10 +37,10 @@ public static bool RequiresReflectionMethodBodyScannerForCallSite (LinkContext c
 			if (methodDefinition == null)
 				return false;
 
-			return
-				GetIntrinsicIdForMethod (methodDefinition) > IntrinsicId.RequiresReflectionBodyScanner_Sentinel ||
+			return GetIntrinsicIdForMethod (methodDefinition) > IntrinsicId.RequiresReflectionBodyScanner_Sentinel ||
 				context.Annotations.FlowAnnotations.RequiresDataFlowAnalysis (methodDefinition) ||
-				context.Annotations.HasLinkerAttribute<RequiresUnreferencedCodeAttribute> (methodDefinition);
+				context.Annotations.HasLinkerAttribute<RequiresUnreferencedCodeAttribute> (methodDefinition)
+				|| methodDefinition.IsPInvokeImpl;
 		}
 
 		public static bool RequiresReflectionMethodBodyScannerForMethodBody (FlowAnnotations flowAnnotations, MethodDefinition methodDefinition)
@@ -75,7 +75,7 @@ public void ScanAndProcessReturnValue (MethodBody methodBody)
 		{
 			Scan (methodBody);
 
-			if (methodBody.Method.ReturnType.MetadataType != MetadataType.Void) {
+			if (GetReturnTypeWithoutModifiers (methodBody.Method.ReturnType).MetadataType != MetadataType.Void) {
 				var method = methodBody.Method;
 				var requiredMemberTypes = _context.Annotations.FlowAnnotations.GetReturnParameterAnnotation (method);
 				if (requiredMemberTypes != 0) {
@@ -1729,6 +1729,20 @@ methodParams[argsParam] is ArrayValue arrayValue &&
 					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) {
+							reflectionContext.AnalyzingPattern ();
+							reflectionContext.RecordUnrecognizedPattern (2050, $"P/invoke method '{calledMethodDefinition.GetDisplayName ()}' declares a parameter with COM marshalling. Correctness of COM interop cannot be guaranteed after trimming. Interfaces and interface members might be removed.");
+						}
+					}
+
 					if (requiresDataFlowAnalysis) {
 						reflectionContext.AnalyzingPattern ();
 						for (int parameterIndex = 0; parameterIndex < methodParams.Count; parameterIndex++) {
@@ -1755,7 +1769,7 @@ methodParams[argsParam] is ArrayValue arrayValue &&
 
 					// 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 (calledMethodDefinition.ReturnType.MetadataType != MetadataType.Void) {
+					if (GetReturnTypeWithoutModifiers (calledMethodDefinition.ReturnType).MetadataType != MetadataType.Void) {
 						methodReturnValue = CreateMethodReturnValue (calledMethodDefinition, returnValueDynamicallyAccessedMemberTypes);
 
 						return true;
@@ -1771,7 +1785,7 @@ methodParams[argsParam] is ArrayValue arrayValue &&
 			// 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.
 			if (methodReturnValue == null) {
-				if (calledMethod.ReturnType.MetadataType != MetadataType.Void) {
+				if (GetReturnTypeWithoutModifiers (calledMethod.ReturnType).MetadataType != MetadataType.Void) {
 					methodReturnValue = CreateMethodReturnValue (calledMethodDefinition, returnValueDynamicallyAccessedMemberTypes);
 				}
 			}
@@ -1794,6 +1808,64 @@ methodParams[argsParam] is ArrayValue arrayValue &&
 			return true;
 		}
 
+		bool IsComInterop (IMarshalInfoProvider marshalInfoProvider, TypeReference parameterType)
+		{
+			// This is best effort. One can likely find ways how to get COM without triggering these alarms.
+			// AsAny marshalling of a struct with an object-typed field would be one, for example.
+
+			// This logic roughly corresponds to MarshalInfo::MarshalInfo in CoreCLR,
+			// not trying to handle invalid cases and distinctions that are not interesting wrt
+			// "is this COM?" question.
+
+			NativeType nativeType = NativeType.None;
+			if (marshalInfoProvider.HasMarshalInfo) {
+				nativeType = marshalInfoProvider.MarshalInfo.NativeType;
+			}
+
+			if (nativeType == NativeType.IUnknown || nativeType == NativeType.IDispatch || nativeType == NativeType.IntF) {
+				// This is COM by definition
+				return true;
+			}
+
+			if (nativeType == NativeType.None) {
+				// Resolve will look at the element type
+				var parameterTypeDef = _context.TryResolve (parameterType);
+
+				if (parameterTypeDef != null) {
+					if (parameterTypeDef.IsTypeOf ("System", "Array")) {
+						// System.Array marshals as IUnknown by default
+						return true;
+					} else if (parameterTypeDef.IsTypeOf ("System", "String") ||
+						parameterTypeDef.IsTypeOf ("System.Text", "StringBuilder")) {
+						// String and StringBuilder are special cased by interop
+						return false;
+					}
+
+					if (parameterTypeDef.IsValueType) {
+						// Value types don't marshal as COM
+						return false;
+					} else if (parameterTypeDef.IsInterface) {
+						// Interface types marshal as COM by default
+						return true;
+					} else if (parameterTypeDef.IsMulticastDelegate ()) {
+						// Delegates are special cased by interop
+						return false;
+					} else if (parameterTypeDef.IsSubclassOf ("System.Runtime.InteropServices", "CriticalHandle")) {
+						// Subclasses of CriticalHandle are special cased by interop
+						return false;
+					} else if (parameterTypeDef.IsSubclassOf ("System.Runtime.InteropServices", "SafeHandle")) {
+						// Subclasses of SafeHandle are special cased by interop
+						return false;
+					} else if (!parameterTypeDef.IsSequentialLayout && !parameterTypeDef.IsExplicitLayout) {
+						// Rest of classes that don't have layout marshal as COM
+						return true;
+					}
+				}
+			}
+
+			return false;
+		}
+
 		bool AnalyzeGenericInstatiationTypeArray (ValueNode arrayParam, ref ReflectionPatternContext reflectionContext, MethodReference calledMethod, IList<GenericParameter> genericParameters)
 		{
 			bool hasRequirements = false;

src/linker/Linker.Steps/MarkStep.cs

@@ -3042,28 +3042,13 @@ void ProcessInteropMethod (MethodDefinition method)
 
 			TypeDefinition returnTypeDefinition = _context.TryResolve (method.ReturnType);
 
-			bool didWarnAboutCom = false;
-
 			const bool includeStaticFields = false;
 			if (returnTypeDefinition != null) {
 				if (!returnTypeDefinition.IsImport) {
 					// What we keep here is correct most of the time, but not every time. Fine for now.
 					MarkDefaultConstructor (returnTypeDefinition, new DependencyInfo (DependencyKind.InteropMethodDependency, method));
 					MarkFields (returnTypeDefinition, includeStaticFields, new DependencyInfo (DependencyKind.InteropMethodDependency, method));
 				}
-
-				// Best-effort attempt to find COM marshalling.
-				// It might seem reasonable to allow out parameters, but once we get a foreign object back (an RCW), it's going to look
-				// like a regular managed class/interface, but every method invocation that takes a class/interface implies COM
-				// marshalling. We can't detect that once we have an RCW.
-				if (method.IsPInvokeImpl) {
-					if (IsComInterop (method.MethodReturnType, method.ReturnType) && !didWarnAboutCom) {
-						_context.LogWarning (
-							$"P/invoke method '{method.GetDisplayName ()}' declares a parameter with COM marshalling. Correctness of COM interop cannot be guaranteed after trimming. Interfaces and interface members might be removed.",
-							2050, method, subcategory: MessageSubCategory.TrimAnalysis);
-						didWarnAboutCom = true;
-					}
-				}
 			}
 
 			if (method.HasThis && !method.DeclaringType.IsImport) {
@@ -3085,75 +3070,8 @@ void ProcessInteropMethod (MethodDefinition method)
 							MarkDefaultConstructor (paramTypeDefinition, new DependencyInfo (DependencyKind.InteropMethodDependency, method));
 						}
 					}
-
-					// Best-effort attempt to find COM marshalling.
-					// It might seem reasonable to allow out parameters, but once we get a foreign object back (an RCW), it's going to look
-					// like a regular managed class/interface, but every method invocation that takes a class/interface implies COM
-					// marshalling. We can't detect that once we have an RCW.
-					if (method.IsPInvokeImpl) {
-						if (IsComInterop (pd, paramTypeReference) && !didWarnAboutCom) {
-							_context.LogWarning ($"P/invoke method '{method.GetDisplayName ()}' declares a parameter with COM marshalling. Correctness of COM interop cannot be guaranteed after trimming. Interfaces and interface members might be removed.", 2050, method);
-							didWarnAboutCom = true;
-						}
-					}
-				}
-			}
-		}
-
-		bool IsComInterop (IMarshalInfoProvider marshalInfoProvider, TypeReference parameterType)
-		{
-			// This is best effort. One can likely find ways how to get COM without triggering these alarms.
-			// AsAny marshalling of a struct with an object-typed field would be one, for example.
-
-			// This logic roughly corresponds to MarshalInfo::MarshalInfo in CoreCLR,
-			// not trying to handle invalid cases and distinctions that are not interesting wrt
-			// "is this COM?" question.
-
-			NativeType nativeType = NativeType.None;
-			if (marshalInfoProvider.HasMarshalInfo) {
-				nativeType = marshalInfoProvider.MarshalInfo.NativeType;
-			}
-
-			if (nativeType == NativeType.IUnknown || nativeType == NativeType.IDispatch || nativeType == NativeType.IntF) {
-				// This is COM by definition
-				return true;
-			}
-
-			if (nativeType == NativeType.None) {
-				if (parameterType.IsTypeOf ("System", "Array")) {
-					// System.Array marshals as IUnknown by default
-					return true;
-				} else if (parameterType.IsTypeOf ("System", "String") ||
-					parameterType.IsTypeOf ("System.Text", "StringBuilder")) {
-					// String and StringBuilder are special cased by interop
-					return false;
-				}
-
-				var parameterTypeDef = _context.TryResolve (parameterType);
-				if (parameterTypeDef != null) {
-					if (parameterTypeDef.IsValueType) {
-						// Value types don't marshal as COM
-						return false;
-					} else if (parameterTypeDef.IsInterface) {
-						// Interface types marshal as COM by default
-						return true;
-					} else if (parameterTypeDef.IsMulticastDelegate ()) {
-						// Delegates are special cased by interop
-						return false;
-					} else if (parameterTypeDef.IsSubclassOf ("System.Runtime.InteropServices", "CriticalHandle")) {
-						// Subclasses of CriticalHandle are special cased by interop
-						return false;
-					} else if (parameterTypeDef.IsSubclassOf ("System.Runtime.InteropServices", "SafeHandle")) {
-						// Subclasses of SafeHandle are special cased by interop
-						return false;
-					} else if (!parameterTypeDef.IsSequentialLayout && !parameterTypeDef.IsExplicitLayout) {
-						// Rest of classes that don't have layout marshal as COM
-						return true;
-					}
 				}
 			}
-
-			return false;
 		}
 
 		protected virtual bool ShouldParseMethodBody (MethodDefinition method)