Implement support for UnsafeAccessor in the trimmer

eng/pipelines/runtime-linker-tests.yml

@@ -74,7 +74,7 @@ extends:
           - linux_x64
           jobParameters:
             testGroup: innerloop
-            testResultsFormat: 'vstest'
+            enablePublishTestResults: true
             timeoutInMinutes: 120
             nameSuffix: ILLink_Runtime_Testing
             condition:

src/coreclr/tools/Common/TypeSystem/IL/UnsafeAccessors.cs

@@ -91,6 +91,15 @@ public static MethodIL TryGetIL(EcmaMethod method)
                         return GenerateAccessorBadImageFailure(method);
                     }
 
+                    // If the non-static method access is for a
+                    // value type, the instance must be byref.
+                    if (kind == UnsafeAccessorKind.Method
+                        && firstArgType.IsValueType
+                        && !firstArgType.IsByRef)
+                    {
+                        return GenerateAccessorBadImageFailure(method);
+                    }
+
                     context.TargetType = ValidateTargetType(firstArgType);
                     if (context.TargetType == null)
                     {

src/coreclr/tools/aot/Mono.Linker.Tests/TestCases/TestSuites.cs

@@ -44,6 +44,7 @@ public void Reflection (string t)
 			switch (t) {
 			case "TypeHierarchyReflectionWarnings":
 			case "ParametersUsedViaReflection":
+			case "UnsafeAccessor":
 				Run (t);
 				break;
 			default:

src/coreclr/vm/prestub.cpp

@@ -1503,6 +1503,15 @@ bool MethodDesc::TryGenerateUnsafeAccessor(DynamicResolver** resolver, COR_ILMET
         if (firstArgType.IsNull())
             ThrowHR(COR_E_BADIMAGEFORMAT, BFA_INVALID_UNSAFEACCESSOR);
 
+        // If the non-static method access is for a
+        // value type, the instance must be byref.
+        if (kind == UnsafeAccessorKind::Method
+            && firstArgType.IsValueType()
+            && !firstArgType.IsByRef())
+        {
+            ThrowHR(COR_E_BADIMAGEFORMAT, BFA_INVALID_UNSAFEACCESSOR);
+        }
+
         context.TargetType = ValidateTargetType(firstArgType);
         context.IsTargetStatic = kind == UnsafeAccessorKind::StaticMethod;
         if (!TrySetTargetMethod(context, name.GetUTF8()))

src/tests/baseservices/compilerservices/UnsafeAccessors/UnsafeAccessorsTests.cs

@@ -79,6 +79,8 @@ struct UserDataValue
 
         private static string _M(string s, ref string sr, in string si) => s;
         private string _m(string s, ref string sr, in string si) => s;
+
+        public string GetFieldValue() => _f;
     }
 
     [UnsafeAccessor(UnsafeAccessorKind.Constructor)]
@@ -119,6 +121,18 @@ public static void Verify_CallCtorValue()
         Assert.Equal(PrivateArg, local.Value);
     }
 
+    [Fact]
+    public static void Verify_CallCtorWithEmptyNotNullName()
+    {
+        Console.WriteLine($"Running {nameof(Verify_CallCtorWithEmptyNotNullName)}");
+
+        var local = CallPrivateConstructorWithEmptyName();
+        Assert.Equal(nameof(UserDataClass), local.GetType().Name);
+
+        [UnsafeAccessor(UnsafeAccessorKind.Constructor, Name="")]
+        extern static UserDataClass CallPrivateConstructorWithEmptyName();
+    }
+
     [Fact]
     public static void Verify_CallCtorAsMethod()
     {
@@ -191,6 +205,10 @@ public static void Verify_AccessFieldValue()
         UserDataValue local = new();
         Assert.Equal(Private, GetPrivateField(ref local));
 
+        const string newValue = "__NewValue__";
+        GetPrivateField(ref local) = newValue;
+        Assert.Equal(newValue, local.GetFieldValue());
+
         [UnsafeAccessor(UnsafeAccessorKind.Field, Name=UserDataValue.FieldName)]
         extern static ref string GetPrivateField(ref UserDataValue d);
     }
@@ -355,6 +373,50 @@ public static void Verify_ManagedUnmanagedFunctionPointersDontMatch()
         extern static string CallManagedMethod(UserDataClass d, delegate* unmanaged[Cdecl]<void> fptr);
     }
 
+    class InheritanceBase
+    {
+        private static string OnBase() => nameof(OnBase);
+        private static string FieldOnBase = nameof(FieldOnBase);
+    }
+
+    class InheritanceDerived : InheritanceBase
+    {
+        private static string OnDerived() => nameof(OnDerived);
+        private static string FieldOnDerived = nameof(FieldOnDerived);
+    }
+
+    [Fact]
+    public static void Verify_InheritanceMethodResolution()
+    {
+        Console.WriteLine($"Running {nameof(Verify_InheritanceMethodResolution)}");
+
+        var instance = new InheritanceDerived();
+        Assert.Throws<MissingMethodException>(() => OnBase(instance));
+        Assert.Equal(nameof(OnDerived), OnDerived(instance));
+
+        [UnsafeAccessor(UnsafeAccessorKind.StaticMethod, Name = nameof(OnBase))]
+        extern static string OnBase(InheritanceDerived i);
+
+        [UnsafeAccessor(UnsafeAccessorKind.StaticMethod, Name = nameof(OnDerived))]
+        extern static string OnDerived(InheritanceDerived i);
+    }
+
+    [Fact]
+    public static void Verify_InheritanceFieldResolution()
+    {
+        Console.WriteLine($"Running {nameof(Verify_InheritanceFieldResolution)}");
+
+        var instance = new InheritanceDerived();
+        Assert.Throws<MissingFieldException>(() => FieldOnBase(instance));
+        Assert.Equal(nameof(FieldOnDerived), FieldOnDerived(instance));
+
+        [UnsafeAccessor(UnsafeAccessorKind.StaticField, Name = nameof(FieldOnBase))]
+        extern static ref string FieldOnBase(InheritanceDerived i);
+
+        [UnsafeAccessor(UnsafeAccessorKind.StaticField, Name = nameof(FieldOnDerived))]
+        extern static ref string FieldOnDerived(InheritanceDerived i);
+    }
+
     [Fact]
     public static void Verify_InvalidTargetUnsafeAccessor()
     {
@@ -389,6 +451,13 @@ public static void Verify_InvalidTargetUnsafeAccessor()
         AssertExtensions.ThrowsMissingMemberException<MissingMethodException>(
             isNativeAot ? null : UserDataClass.MethodPointerName,
             () => CallPointerMethod(null, null));
+        AssertExtensions.ThrowsMissingMemberException<MissingMethodException>(
+            isNativeAot ? null : UserDataClass.StaticMethodName,
+            () => { string sr = string.Empty; StaticMethodWithDifferentReturnType(null, null, ref sr, string.Empty); });
+
+        AssertExtensions.ThrowsMissingMemberException<MissingMethodException>(
+            isNativeAot ? null : UserDataClass.StaticMethodName,
+            () => { string sr = string.Empty; StaticMethodWithDifferentReturnType(null, null, ref sr, string.Empty); });
 
         [UnsafeAccessor(UnsafeAccessorKind.Method, Name=DoesNotExist)]
         extern static void MethodNotFound(UserDataClass d);
@@ -412,6 +481,8 @@ public static void Verify_InvalidTargetUnsafeAccessor()
         [UnsafeAccessor(UnsafeAccessorKind.StaticMethod, Name=UserDataClass.MethodPointerName)]
         extern static string CallPointerMethod(UserDataClass d, delegate* unmanaged[Stdcall]<void> fptr);
 
+        [UnsafeAccessor(UnsafeAccessorKind.StaticMethod, Name=UserDataClass.StaticMethodName)]
+        extern static int StaticMethodWithDifferentReturnType(UserDataClass d, string s, ref string sr, in string si);
     }
 
     [Fact]
@@ -467,6 +538,12 @@ public static void Verify_InvalidUseUnsafeAccessor()
         Assert.Throws<BadImageFormatException>(() => new Invalid().NonStatic(string.Empty));
         Assert.Throws<BadImageFormatException>(() => Invalid.CallToString<string>(string.Empty));
         Assert.Throws<BadImageFormatException>(() => Invalid<string>.CallToString(string.Empty));
+        Assert.Throws<BadImageFormatException>(() =>
+        {
+            string str = string.Empty;
+            UserDataValue local = new();
+            InvokeMethodOnValueWithoutRef(local, null, ref str, str);
+        });
 
         [UnsafeAccessor(UnsafeAccessorKind.Field, Name=UserDataValue.FieldName)]
         extern static string FieldReturnMustBeByRefClass(UserDataClass d);
@@ -503,5 +580,8 @@ public static void Verify_InvalidUseUnsafeAccessor()
 
         [UnsafeAccessor(UnsafeAccessorKind.Method, Name=nameof(ToString))]
         extern static string LookUpFailsOnFunctionPointers(delegate* <void> fptr);
+
+        [UnsafeAccessor(UnsafeAccessorKind.Method, Name = UserDataValue.MethodName)]
+        extern static string InvokeMethodOnValueWithoutRef(UserDataValue target, string s, ref string sr, in string si);
     }
 }

src/tools/illink/src/linker/Linker.Steps/MarkStep.cs

@@ -36,6 +36,7 @@
 using System.Diagnostics.CodeAnalysis;
 using System.Linq;
 using System.Reflection.Runtime.TypeParsing;
+using System.Runtime.CompilerServices;
 using System.Text.RegularExpressions;
 using ILLink.Shared;
 using ILLink.Shared.TrimAnalysis;
@@ -127,6 +128,7 @@ internal DynamicallyAccessedMembersTypeHierarchy DynamicallyAccessedMembersTypeH
 			DependencyKind.ReferencedBySpecialAttribute,
 			DependencyKind.TypePreserve,
 			DependencyKind.XmlDescriptor,
+			DependencyKind.UnsafeAccessorTarget,
 		};
 
 		static readonly DependencyKind[] _typeReasons = new DependencyKind[] {
@@ -211,6 +213,7 @@ internal DynamicallyAccessedMembersTypeHierarchy DynamicallyAccessedMembersTypeH
 			DependencyKind.FieldMarshalSpec,
 			DependencyKind.ReturnTypeMarshalSpec,
 			DependencyKind.XmlDescriptor,
+			DependencyKind.UnsafeAccessorTarget,
 		};
 #endif
 
@@ -1870,6 +1873,7 @@ void ProcessAnalysisAnnotationsForField (FieldDefinition field, DependencyKind d
 			case DependencyKind.DynamicallyAccessedMember:
 			case DependencyKind.InteropMethodDependency:
 			case DependencyKind.Ldtoken:
+			case DependencyKind.UnsafeAccessorTarget:
 				if (isReflectionAccessCoveredByDAM = Annotations.FlowAnnotations.ShouldWarnWhenAccessedForReflection (field))
 					Context.LogWarning (origin, DiagnosticId.DynamicallyAccessedMembersFieldAccessedViaReflection, field.GetDisplayName ());
 
@@ -3264,6 +3268,10 @@ protected virtual void ProcessMethod (MethodDefinition method, in DependencyInfo
 				ProcessInteropMethod (method);
 			}
 
+			if (!method.HasBody || method.Body.CodeSize == 0) {
+				ProcessUnsafeAccessorMethod (method);
+			}
+
 			if (ShouldParseMethodBody (method))
 				MarkMethodBody (method.Body);
 
@@ -3494,6 +3502,11 @@ void ProcessInteropMethod (MethodDefinition method)
 #pragma warning restore RS0030
 		}
 
+		void ProcessUnsafeAccessorMethod (MethodDefinition method)
+		{
+			(new UnsafeAccessorMarker (Context, this)).ProcessUnsafeAccessorMethod (method);
+		}
+
 		protected virtual bool ShouldParseMethodBody (MethodDefinition method)
 		{
 			if (!method.HasBody)

src/tools/illink/src/linker/Linker.Steps/UnsafeAccessorMarker.cs

@@ -0,0 +1,141 @@
+// Copyright (c) .NET Foundation and contributors. All rights reserved.
+// Licensed under the MIT license. See LICENSE file in the project root for full license information.
+
+using System.Runtime.CompilerServices;
+using Mono.Cecil;
+
+namespace Mono.Linker.Steps
+{
+	// This class only handles static methods (all the unsafe accessors should be static)
+	// so there's no problem with forgetting the implicit "this".
+#pragma warning disable RS0030 // MethodReference.Parameters is banned
+
+	readonly struct UnsafeAccessorMarker (LinkContext context, MarkStep markStep)
+	{
+		readonly LinkContext _context = context;
+		readonly MarkStep _markStep = markStep;
+
+		// We don't perform method overload resolution based on list of parameters (or return type) for now
+		// Mono.Cecil's method resolution is problematic and has bugs. It's also not extensible
+		// and we would need that to correctly implement the desired behavior around custom modifiers. So for now we decided to not
+		// duplicate the logic to tweak it and will just mark entire method groups.
+
+		public void ProcessUnsafeAccessorMethod (MethodDefinition method)
+		{
+			if (!method.IsStatic || !method.HasCustomAttributes)
+				return;
+
+			foreach (CustomAttribute customAttribute in method.CustomAttributes) {
+				if (customAttribute.Constructor.DeclaringType.FullName == "System.Runtime.CompilerServices.UnsafeAccessorAttribute") {
+					if (customAttribute.HasConstructorArguments && customAttribute.ConstructorArguments[0].Value is int kindValue) {
+						UnsafeAccessorKind kind = (UnsafeAccessorKind) kindValue;
+						string? name = null;
+						if (customAttribute.HasProperties) {
+							foreach (CustomAttributeNamedArgument prop in customAttribute.Properties) {
+								if (prop.Name == "Name") {
+									name = prop.Argument.Value as string;
+									break;
+								}
+							}
+						}
+
+						switch (kind) {
+						case UnsafeAccessorKind.Constructor:
+							ProcessConstructorAccessor (method, name);
+							break;
+						case UnsafeAccessorKind.StaticMethod:
+							ProcessMethodAccessor (method, name, isStatic: true);
+							break;
+						case UnsafeAccessorKind.Method:
+							ProcessMethodAccessor (method, name, isStatic: false);
+							break;
+						case UnsafeAccessorKind.StaticField:
+							ProcessFieldAccessor (method, name, isStatic: true);
+							break;
+						case UnsafeAccessorKind.Field:
+							ProcessFieldAccessor (method, name, isStatic: false);
+							break;
+						default:
+							break;
+						}
+
+						// Intentionally only process the first such attribute
+						// if there's more than one runtime will fail on it anyway.
+						break;
+					}
+				}
+			}
+		}
+
+		void ProcessConstructorAccessor (MethodDefinition method, string? name)
+		{
+			// A return type is required for a constructor, otherwise
+			// we don't know the type to construct.
+			// Types should not be parameterized (that is, by-ref).
+			// The name is defined by the runtime and should be empty.
+			if (method.ReturnsVoid () || method.ReturnType.IsByRefOrPointer () || !string.IsNullOrEmpty (name))
+				return;
+
+			if (_context.TryResolve (method.ReturnType) is not TypeDefinition targetType)
+				return;
+
+			foreach (MethodDefinition targetMethod in targetType.Methods) {
+				if (!targetMethod.IsConstructor || targetMethod.IsStatic)
+					continue;
+
+				_markStep.MarkMethodVisibleToReflection (targetMethod, new DependencyInfo (DependencyKind.UnsafeAccessorTarget, method), new MessageOrigin (method));
+			}
+		}
+
+		void ProcessMethodAccessor (MethodDefinition method, string? name, bool isStatic)
+		{
+			// Method access requires a target type.
+			if (method.Parameters.Count == 0)
+				return;
+
+			if (string.IsNullOrEmpty (name))
+				name = method.Name;
+
+			TypeReference targetTypeReference = method.Parameters[0].ParameterType;
+			if (_context.TryResolve (targetTypeReference) is not TypeDefinition targetType)
+				return;
+
+			if (!isStatic && targetType.IsValueType && !targetTypeReference.IsByReference)
+				return;
+
+			foreach (MethodDefinition targetMethod in targetType.Methods) {
+				if (targetMethod.Name != name || targetMethod.IsStatic != isStatic)
+					continue;
+
+				_markStep.MarkMethodVisibleToReflection (targetMethod, new DependencyInfo (DependencyKind.UnsafeAccessorTarget, method), new MessageOrigin (method));
+			}
+		}
+
+		void ProcessFieldAccessor (MethodDefinition method, string? name, bool isStatic)
+		{
+			// Field access requires exactly one parameter
+			if (method.Parameters.Count != 1)
+				return;
+
+			if (string.IsNullOrEmpty (name))
+				name = method.Name;
+
+			if (!method.ReturnType.IsByReference)
+				return;
+
+			TypeReference targetTypeReference = method.Parameters[0].ParameterType;
+			if (_context.TryResolve (targetTypeReference) is not TypeDefinition targetType)
+				return;
+
+			if (!isStatic && targetType.IsValueType && !targetTypeReference.IsByReference)
+				return;
+
+			foreach (FieldDefinition targetField in targetType.Fields) {
+				if (targetField.Name != name || targetField.IsStatic != isStatic)
+					continue;
+
+				_markStep.MarkFieldVisibleToReflection (targetField, new DependencyInfo (DependencyKind.UnsafeAccessorTarget, method), new MessageOrigin (method));
+			}
+		}
+	}
+}

src/tools/illink/src/linker/Linker/DependencyInfo.cs

@@ -143,6 +143,8 @@ public enum DependencyKind
 		PreservedOperator = 87, // operator method preserved on a type
 
 		DynamicallyAccessedMemberOnType = 88, // type with DynamicallyAccessedMembers annotations (including those inherited from base types and interfaces)
+
+		UnsafeAccessorTarget = 89, // the member is referenced via UnsafeAccessor attribute
 	}
 
 	public readonly struct DependencyInfo : IEquatable<DependencyInfo>