From d4f6b519302b266ec1f6b5bd699e7a8a16904125 Mon Sep 17 00:00:00 2001 From: Steve Harter Date: Mon, 11 Apr 2022 09:25:00 -0500 Subject: [PATCH 01/21] Add IL Emit support for MethodInfo.Invoke() --- .../Reflection/RuntimeMethodInfo.CoreCLR.cs | 20 +- .../System.Private.CoreLib.Shared.projitems | 1 + .../src/System/Reflection/InvokerEmitUtil.cs | 331 ++++++++++++++++++ .../src/System/Reflection/MethodInvoker.cs | 65 +++- .../tests/MethodInfoTests.cs | 42 +++ .../Reflection/RuntimeMethodInfo.Mono.cs | 4 + .../src/System/RuntimeType.Mono.cs | 2 + 7 files changed, 448 insertions(+), 17 deletions(-) create mode 100644 src/libraries/System.Private.CoreLib/src/System/Reflection/InvokerEmitUtil.cs diff --git a/src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeMethodInfo.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeMethodInfo.CoreCLR.cs index 8cd33129735fd..babc1854d98f4 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeMethodInfo.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeMethodInfo.CoreCLR.cs @@ -368,25 +368,15 @@ public override MethodImplAttributes GetMethodImplementationFlags() return retValue; } +#pragma warning disable CA1822 // Mark members as static + internal bool SupportsNewInvoke => true; +#pragma warning restore CA1822 // Mark members as static + [DebuggerHidden] [DebuggerStepThrough] internal unsafe object? InvokeNonEmitUnsafe(object? obj, IntPtr* arguments, Span argsForTemporaryMonoSupport, BindingFlags invokeAttr) { - if ((invokeAttr & BindingFlags.DoNotWrapExceptions) == 0) - { - try - { - return RuntimeMethodHandle.InvokeMethod(obj, (void**)arguments, Signature, isConstructor: false); - } - catch (Exception e) - { - throw new TargetInvocationException(e); - } - } - else - { - return RuntimeMethodHandle.InvokeMethod(obj, (void**)arguments, Signature, isConstructor: false); - } + return RuntimeMethodHandle.InvokeMethod(obj, (void**)arguments, Signature, isConstructor: false); } #endregion diff --git a/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems b/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems index 83f6d219275bb..37fcf2aa4b7af 100644 --- a/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems +++ b/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems @@ -630,6 +630,7 @@ + diff --git a/src/libraries/System.Private.CoreLib/src/System/Reflection/InvokerEmitUtil.cs b/src/libraries/System.Private.CoreLib/src/System/Reflection/InvokerEmitUtil.cs new file mode 100644 index 0000000000000..0b46dd78ab566 --- /dev/null +++ b/src/libraries/System.Private.CoreLib/src/System/Reflection/InvokerEmitUtil.cs @@ -0,0 +1,331 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Diagnostics; +using System.Diagnostics.CodeAnalysis; +using System.Reflection.Emit; +using System.Runtime.CompilerServices; + +namespace System.Reflection +{ + internal static class InvokerEmitUtil + { + // This is an instance method to avoid overhead of shuffle thunk. + internal unsafe delegate object? InvokeFunc(T obj, object? target, IntPtr* arguments); + + // Avoid high arg count due to increased stack. + // This also allows the local variables to use that more smaller encoded _S opcode variants. + public const int MaxArgumentCount = 64; + + public static unsafe InvokeFunc CreateInvokeDelegate(MethodBase method) + { + Debug.Assert(!method.ContainsGenericParameters); + + ParameterInfo[] parameters = method.GetParametersNoCopy(); + Debug.Assert(parameters.Length <= MaxArgumentCount); + + Type[] delegateParameters = new Type[3] { typeof(T), typeof(object), typeof(IntPtr*) }; + + // We could use the overload with 'owner' in order to associate to this module. + var dm = new DynamicMethod( + "InvokeStub_" + method.DeclaringType!.Name + "." + method.Name, + returnType: typeof(object), + delegateParameters, + owner: typeof(T), + skipVisibility: true); + + ILGenerator il = dm.GetILGenerator(); + + if (parameters.Length == 0) + { + HandleThisPointer(il, method); + Invoke(il, method); + } + else + { + HandleThisPointer(il, method); + PushArguments(il, parameters, out NullableRefInfo[]? byRefNullables, out int byRefNullableCount); + Invoke(il, method); + + if (byRefNullableCount > 0) + { + Debug.Assert(byRefNullables != null); ; + UpdateNullables(il, parameters, byRefNullables, byRefNullableCount); + } + } + + HandleReturn(il, method); + + return (InvokeFunc)dm.CreateDelegate(typeof(InvokeFunc)); + } + + private static void HandleThisPointer(ILGenerator il, MethodBase method) + { + bool emitNew = method is RuntimeConstructorInfo; + bool hasThis = !(emitNew || method.IsStatic); + + if (hasThis) + { + il.Emit(OpCodes.Ldarg_1); + if (method.DeclaringType!.IsValueType) + { + il.Emit(OpCodes.Unbox, method.DeclaringType); + } + } + } + + /// + /// Push each argument. + /// + private static void PushArguments( + ILGenerator il, + ParameterInfo[] parameters, + out NullableRefInfo[]? byRefNullables, + out int byRefNullablesCount + ) + { + byRefNullables = null; + byRefNullablesCount = 0; + + LocalBuilder? local_pArg = il.DeclareLocal(typeof(IntPtr*)); + il.Emit(OpCodes.Ldarg_2); + il.Emit(OpCodes.Stloc_S, local_pArg); + + for (int i = 0; i < parameters.Length; i++) + { + RuntimeType parameterType = (RuntimeType)parameters[i].ParameterType; + + // Get the argument as a ref + il.Emit(OpCodes.Ldloc_S, local_pArg); + il.Emit(OpCodes.Ldind_Ref); + + if (parameterType.IsByRef) + { + RuntimeType elementType = (RuntimeType)parameterType.GetElementType(); + + if (elementType.IsNullableOfT) + { + byRefNullables ??= new NullableRefInfo[MaxArgumentCount - i]; + + LocalBuilder tmp = il.DeclareLocal(typeof(object).MakeByRefType()); + il.Emit(OpCodes.Stloc_S, tmp); + il.Emit(OpCodes.Ldloc_S, tmp); + + // Get the raw pointer. + il.Emit(OpCodes.Ldobj, typeof(object)); + il.Emit(OpCodes.Unbox, elementType); + + // Copy the pointer to the temp variable and load as a ref. + LocalBuilder byRefPtr = il.DeclareLocal(elementType.MakePointerType()); + il.Emit(OpCodes.Stloc_S, byRefPtr); + il.Emit(OpCodes.Ldloca_S, byRefPtr); + il.Emit(OpCodes.Ldind_Ref); + + byRefNullables[byRefNullablesCount++] = new NullableRefInfo { ParameterIndex = i, LocalIndex = byRefPtr.LocalIndex }; + } + else if (elementType.IsPointer) + { + LocalBuilder tmp = il.DeclareLocal(typeof(IntPtr).MakeByRefType()); + il.Emit(OpCodes.Stloc_S, tmp); + il.Emit(OpCodes.Ldloc_S, tmp); + il.Emit(OpCodes.Ldobj, typeof(IntPtr).MakeByRefType()); + } + else + { + LocalBuilder tmp = il.DeclareLocal(parameterType); + il.Emit(OpCodes.Stloc_S, tmp); + il.Emit(OpCodes.Ldloca_S, tmp); + il.Emit(OpCodes.Ldind_Ref); //keep this or remove and use ldloca? + } + } + else if (parameterType.IsNullableOfT) + { + // Nullable is the only incoming value type that is boxed. + LocalBuilder tmp = il.DeclareLocal(typeof(object).MakeByRefType()); + il.Emit(OpCodes.Stloc_S, tmp); + il.Emit(OpCodes.Ldloc_S, tmp); + + il.Emit(OpCodes.Ldobj, typeof(object)); + il.Emit(OpCodes.Unbox, parameterType); + il.Emit(OpCodes.Ldobj, parameterType); + } + else if (parameterType.IsPointer) + { + LocalBuilder tmp = il.DeclareLocal(typeof(IntPtr).MakeByRefType()); + il.Emit(OpCodes.Stloc_S, tmp); + il.Emit(OpCodes.Ldloc_S, tmp); + il.Emit(OpCodes.Ldobj, typeof(IntPtr)); + } + else + { + LocalBuilder tmp = il.DeclareLocal(parameterType.MakeByRefType()); + il.Emit(OpCodes.Stloc_S, tmp); + il.Emit(OpCodes.Ldloc_S, tmp); + il.Emit(OpCodes.Ldobj, parameterType); + } + + // Move to the next argument. + if (i < parameters.Length - 1) + { + il.Emit(OpCodes.Ldloc_S, local_pArg); + il.Emit(OpCodes.Sizeof, typeof(IntPtr)); + il.Emit(OpCodes.Add); + il.Emit(OpCodes.Stloc_S, local_pArg); + } + } + } + + /// + /// Update any nullables that were passed by reference. + /// + private static void UpdateNullables( + ILGenerator il, + ParameterInfo[] parameters, + NullableRefInfo[] byRefNullables, + int byRefNullablesCount) + { + for (int i = 0; i < byRefNullablesCount; i++) + { + NullableRefInfo info = byRefNullables[i]; + + RuntimeType parameterType = (RuntimeType)parameters[info.ParameterIndex].ParameterType; + Debug.Assert(parameterType.IsByRef); + + RuntimeType? elementType = (RuntimeType)parameterType.GetElementType()!; + Debug.Assert(elementType.IsNullableOfT); + + // Get the original byref location. + il.Emit(OpCodes.Ldc_I4_S, info.ParameterIndex); + il.Emit(OpCodes.Conv_I); + il.Emit(OpCodes.Sizeof, typeof(IntPtr)); + il.Emit(OpCodes.Mul); + il.Emit(OpCodes.Ldarg_2); + il.Emit(OpCodes.Add); + il.Emit(OpCodes.Ldind_I); + + // Get the Nullable& value and update the original. + il.Emit(OpCodes.Ldloc_S, info.LocalIndex); + il.Emit(OpCodes.Ldobj, elementType); + il.Emit(OpCodes.Box, elementType); + il.Emit(OpCodes.Stind_Ref); + } + } + + private static void Invoke(ILGenerator il, MethodBase method) + { + // todo: once we return the value without boxing add support for OpCodes.Tailcall for perf. + + bool emitNew = method is RuntimeConstructorInfo; + if (emitNew) + { + Debug.Assert(method!.IsStatic); + il.Emit(OpCodes.Newobj, (ConstructorInfo)method); + } + else if (method.IsStatic || method.DeclaringType!.IsValueType) + { + il.Emit(OpCodes.Call, (RuntimeMethodInfo)method); + } + else + { + il.Emit(OpCodes.Callvirt, (RuntimeMethodInfo)method); + } + } + + private static void HandleReturn(ILGenerator il, MethodBase method) + { + bool emitNew = method is RuntimeConstructorInfo; + Type returnType = emitNew ? method.DeclaringType! : ((RuntimeMethodInfo)method).ReturnType; + + if (returnType == typeof(void)) + { + il.Emit(OpCodes.Ldnull); + il.Emit(OpCodes.Ret); + return; + } + + if (returnType.IsByRef) + { + // Check for null ref return. + Type elementType = returnType.GetElementType()!; + Label retValueOk = il.DefineLabel(); + + il.Emit(OpCodes.Dup); + il.Emit(OpCodes.Brtrue_S, retValueOk); + il.Emit(OpCodes.Call, Methods.ThrowHelper_Throw_NullReference_InvokeNullRefReturned()); + il.MarkLabel(retValueOk); + + if (elementType.IsValueType) + { + LocalBuilder? local_return = il.DeclareLocal(typeof(object)); + il.Emit(OpCodes.Ldobj, elementType); + il.Emit(OpCodes.Box, elementType); + il.Emit(OpCodes.Stloc_S, local_return); + il.Emit(OpCodes.Ldloc_S, local_return); + } + else if (elementType.IsPointer) + { + LocalBuilder? local_return = il.DeclareLocal(elementType); + il.Emit(OpCodes.Ldind_Ref); + il.Emit(OpCodes.Stloc_S, local_return); + il.Emit(OpCodes.Ldloc_S, local_return); + il.Emit(OpCodes.Ldtoken, elementType); + il.Emit(OpCodes.Call, Methods.Type_GetTypeFromHandle()); + il.Emit(OpCodes.Call, Methods.Pointer_Box()); + } + else + { + LocalBuilder? local_return = il.DeclareLocal(elementType); + il.Emit(OpCodes.Ldind_Ref); + il.Emit(OpCodes.Stloc_S, local_return); + il.Emit(OpCodes.Ldloc_S, local_return); + } + } + else if (returnType.IsPointer) + { + il.Emit(OpCodes.Ldtoken, returnType); + il.Emit(OpCodes.Call, Methods.Type_GetTypeFromHandle()); + il.Emit(OpCodes.Call, Methods.Pointer_Box()); + } + else if (returnType.IsValueType) + { + il.Emit(OpCodes.Box, returnType); + } + + il.Emit(OpCodes.Ret); + } + + private static class ThrowHelper + { + public static void Throw_NullReference_InvokeNullRefReturned() + { + throw new NullReferenceException(SR.NullReference_InvokeNullRefReturned); + } + } + + private struct NullableRefInfo + { + public int ParameterIndex; + public int LocalIndex; + } + + private static class Methods + { + private static MethodInfo? s_ThrowHelper_Throw_NullReference_InvokeNullRefReturned; + [DynamicDependency(DynamicallyAccessedMemberTypes.PublicMethods, typeof(ThrowHelper))] + public static MethodInfo ThrowHelper_Throw_NullReference_InvokeNullRefReturned() => + s_ThrowHelper_Throw_NullReference_InvokeNullRefReturned ?? + (s_ThrowHelper_Throw_NullReference_InvokeNullRefReturned = typeof(ThrowHelper).GetMethod(nameof(ThrowHelper.Throw_NullReference_InvokeNullRefReturned))!); + + private static MethodInfo? s_Pointer_Box; + [DynamicDependency(DynamicallyAccessedMemberTypes.PublicMethods, typeof(Pointer))] + public static MethodInfo Pointer_Box() => + s_Pointer_Box ?? + (s_Pointer_Box = typeof(Pointer).GetMethod(nameof(Pointer.Box), new[] { typeof(void*), typeof(Type) })!); + + private static MethodInfo? s_Type_GetTypeFromHandle; + public static MethodInfo Type_GetTypeFromHandle() => + s_Type_GetTypeFromHandle ?? + (s_Type_GetTypeFromHandle = typeof(Type).GetMethod(nameof(Type.GetTypeFromHandle), new[] { typeof(RuntimeTypeHandle) })!); + } + } +} diff --git a/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodInvoker.cs b/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodInvoker.cs index 65c4e2bac2e74..0da25be2ddee9 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodInvoker.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodInvoker.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. using System.Diagnostics; +using System.Runtime.CompilerServices; namespace System.Reflection { @@ -9,6 +10,9 @@ internal sealed partial class MethodInvoker { internal InvocationFlags _invocationFlags; private readonly RuntimeMethodInfo _methodInfo; + private bool _invoked; + private bool _strategyDetermined; + private InvokerEmitUtil.InvokeFunc? _emitInvoke; public MethodInvoker(RuntimeMethodInfo methodInfo) { @@ -19,8 +23,65 @@ public MethodInvoker(RuntimeMethodInfo methodInfo) [DebuggerHidden] public unsafe object? InvokeUnsafe(object? obj, IntPtr* args, Span argsForTemporaryMonoSupport, BindingFlags invokeAttr) { - // Todo: add strategy for calling IL Emit-based version - return _methodInfo.InvokeNonEmitUnsafe(obj, args, argsForTemporaryMonoSupport, invokeAttr); + if (!_invoked) + { + // Note: disabled for test reasons: + // The first time, ignoring race conditions, use the slow path. + // return _methodInfo.InvokeNonEmitUnsafe(obj, args, argsForTemporaryMonoSupport, invokeAttr); + + _invoked = true; + } + + if (!_strategyDetermined) + { + if (RuntimeFeature.IsDynamicCodeCompiled && + _methodInfo.GetParametersNoCopy().Length <= InvokerEmitUtil.MaxArgumentCount && + _methodInfo.DeclaringType != typeof(Type) // Avoid stack crawl issue with GetType(). + ) + { + _emitInvoke = InvokerEmitUtil.CreateInvokeDelegate(_methodInfo); + } + + _strategyDetermined = true; + } + + if (_methodInfo.SupportsNewInvoke) + { + if ((invokeAttr & BindingFlags.DoNotWrapExceptions) == 0) + { + try + { + if (_emitInvoke != null) + { + return _emitInvoke(this, obj, args); + } + else + { + return _methodInfo.InvokeNonEmitUnsafe(obj, args, argsForTemporaryMonoSupport, invokeAttr); + } + } + catch (Exception e) + { + throw new TargetInvocationException(e); + } + } + else + { + if (_emitInvoke != null) + { + return _emitInvoke(this, obj, args); + } + else + { + return _methodInfo.InvokeNonEmitUnsafe(obj, args, argsForTemporaryMonoSupport, invokeAttr); + } + } + } + else + { + // Remove this branch once Mono has the same exception handling and managed conversion logic. + return _methodInfo.InvokeNonEmitUnsafe(obj, args, argsForTemporaryMonoSupport, invokeAttr); + } } } } diff --git a/src/libraries/System.Reflection/tests/MethodInfoTests.cs b/src/libraries/System.Reflection/tests/MethodInfoTests.cs index bdc8cfd07a0e8..b574f2c819ada 100644 --- a/src/libraries/System.Reflection/tests/MethodInfoTests.cs +++ b/src/libraries/System.Reflection/tests/MethodInfoTests.cs @@ -708,6 +708,35 @@ static MethodInfo GetMethod(string name) => typeof(EnumMethods).GetMethod( name, BindingFlags.Public | BindingFlags.Static)!; } + [Fact] + public void ValueTypeMembers() + { + ValueTypeWithOverrides valueTypeWithOverrides = default; + valueTypeWithOverrides.Id = 1; + + // ToString is overridden. + Assert.Equal("Hello", (string)GetMethod(nameof(ValueTypeWithOverrides.ToString)). + Invoke(valueTypeWithOverrides, null)); + + // Ensure a normal method works. + Assert.Equal(1, (int)GetMethod(nameof(ValueTypeWithOverrides.GetId)). + Invoke(valueTypeWithOverrides, null)); + + ValueTypeWithoutOverrides valueTypeWithoutOverrides = default; + valueTypeWithoutOverrides.Id = 1; + + // ToString is not overridden. + Assert.Equal(typeof(ValueTypeWithoutOverrides).ToString(), (string)GetMethod(nameof(ValueTypeWithoutOverrides.ToString)). + Invoke(valueTypeWithoutOverrides, null )); + + // Ensure a normal method works. + Assert.Equal(1, (int)GetMethod(nameof(ValueTypeWithoutOverrides.GetId)). + Invoke(valueTypeWithOverrides, null)); + + static MethodInfo GetMethod(string name) => typeof(T).GetMethod( + name, BindingFlags.Public | BindingFlags.Instance)!; + } + //Methods for Reflection Metadata private void DummyMethod1(string str, int iValue, long lValue) { @@ -1015,6 +1044,19 @@ public enum OtherColorsInt : int Red = 1 } + public struct ValueTypeWithOverrides + { + public int Id; + public override string ToString() => "Hello"; + public int GetId() => Id; + } + + public struct ValueTypeWithoutOverrides + { + public int Id; + public int GetId() => Id; + } + public static class EnumMethods { public static bool PassColorsInt(ColorsInt color) diff --git a/src/mono/System.Private.CoreLib/src/System/Reflection/RuntimeMethodInfo.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Reflection/RuntimeMethodInfo.Mono.cs index c8c67cdd62944..f78082da3dfb0 100644 --- a/src/mono/System.Private.CoreLib/src/System/Reflection/RuntimeMethodInfo.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Reflection/RuntimeMethodInfo.Mono.cs @@ -384,6 +384,10 @@ internal RuntimeType[] ArgumentTypes [MethodImplAttribute(MethodImplOptions.InternalCall)] internal extern object? InternalInvoke(object? obj, in Span parameters, out Exception? exc); +#pragma warning disable CA1822 // Mark members as static + internal bool SupportsNewInvoke => false; +#pragma warning restore CA1822 // Mark members as static + [MethodImpl(MethodImplOptions.AggressiveInlining)] internal unsafe object? InvokeNonEmitUnsafe(object? obj, IntPtr* byrefParameters, Span argsForTemporaryMonoSupport, BindingFlags invokeAttr) { diff --git a/src/mono/System.Private.CoreLib/src/System/RuntimeType.Mono.cs b/src/mono/System.Private.CoreLib/src/System/RuntimeType.Mono.cs index 16d19f3bed4c5..1a2365567a634 100644 --- a/src/mono/System.Private.CoreLib/src/System/RuntimeType.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/RuntimeType.Mono.cs @@ -2363,6 +2363,8 @@ public override string? FullName public sealed override bool HasSameMetadataDefinitionAs(MemberInfo other) => HasSameMetadataDefinitionAsCore(other); + internal bool IsNullableOfT => Nullable.GetUnderlyingType(this) != null; + public override bool IsSZArray { get From 023299c659e074bfa97a725e63b43e068f874e80 Mon Sep 17 00:00:00 2001 From: Steve Harter Date: Wed, 13 Apr 2022 14:29:16 -0500 Subject: [PATCH 02/21] Test updates --- .../tests/StackFrameTests.cs | 5 +-- .../tests/MethodInfoTests.cs | 31 +++++++++---------- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/src/libraries/System.Diagnostics.StackTrace/tests/StackFrameTests.cs b/src/libraries/System.Diagnostics.StackTrace/tests/StackFrameTests.cs index 2f541d9e2a851..67ba8fd483eb1 100644 --- a/src/libraries/System.Diagnostics.StackTrace/tests/StackFrameTests.cs +++ b/src/libraries/System.Diagnostics.StackTrace/tests/StackFrameTests.cs @@ -186,7 +186,8 @@ private static void VerifyStackFrameSkipFrames(StackFrame stackFrame, bool isFil } // GetNativeOffset returns StackFrame.OFFSET_UNKNOWN for unknown frames. - // GetNativeOffset returns 0 for known frames with a positive skipFrames. + // For a positive skipFrame, the GetNativeOffset return value is dependent upon the implementation of reflection + // Invoke() which can be native (where the value would be zero) or managed (where the value is likely non-zero). if (skipFrames == int.MaxValue || skipFrames == int.MinValue) { Assert.Equal(StackFrame.OFFSET_UNKNOWN, stackFrame.GetNativeOffset()); @@ -198,7 +199,7 @@ private static void VerifyStackFrameSkipFrames(StackFrame stackFrame, bool isFil } else { - Assert.Equal(0, stackFrame.GetNativeOffset()); + Assert.True(stackFrame.GetNativeOffset() >= 0); } } } diff --git a/src/libraries/System.Reflection/tests/MethodInfoTests.cs b/src/libraries/System.Reflection/tests/MethodInfoTests.cs index b574f2c819ada..c2a66bad643a5 100644 --- a/src/libraries/System.Reflection/tests/MethodInfoTests.cs +++ b/src/libraries/System.Reflection/tests/MethodInfoTests.cs @@ -709,32 +709,31 @@ static MethodInfo GetMethod(string name) => typeof(EnumMethods).GetMethod( } [Fact] - public void ValueTypeMembers() + public void ValueTypeMembers_WithOverrides() { - ValueTypeWithOverrides valueTypeWithOverrides = default; - valueTypeWithOverrides.Id = 1; + ValueTypeWithOverrides obj = new() { Id = 1 }; // ToString is overridden. - Assert.Equal("Hello", (string)GetMethod(nameof(ValueTypeWithOverrides.ToString)). - Invoke(valueTypeWithOverrides, null)); + Assert.Equal("Hello", (string)GetMethod(typeof(ValueTypeWithOverrides), nameof(ValueTypeWithOverrides.ToString)). + Invoke(obj, null)); // Ensure a normal method works. - Assert.Equal(1, (int)GetMethod(nameof(ValueTypeWithOverrides.GetId)). - Invoke(valueTypeWithOverrides, null)); + Assert.Equal(1, (int)GetMethod(typeof(ValueTypeWithOverrides), nameof(ValueTypeWithOverrides.GetId)). + Invoke(obj, null)); + } - ValueTypeWithoutOverrides valueTypeWithoutOverrides = default; - valueTypeWithoutOverrides.Id = 1; + [Fact] + public void ValueTypeMembers_WithoutOverrides() + { + ValueTypeWithoutOverrides obj = new() { Id = 1 }; // ToString is not overridden. - Assert.Equal(typeof(ValueTypeWithoutOverrides).ToString(), (string)GetMethod(nameof(ValueTypeWithoutOverrides.ToString)). - Invoke(valueTypeWithoutOverrides, null )); + Assert.Equal(typeof(ValueTypeWithoutOverrides).ToString(), (string)GetMethod(typeof(ValueTypeWithoutOverrides), nameof(ValueTypeWithoutOverrides.ToString)). + Invoke(obj, null)); // Ensure a normal method works. - Assert.Equal(1, (int)GetMethod(nameof(ValueTypeWithoutOverrides.GetId)). - Invoke(valueTypeWithOverrides, null)); - - static MethodInfo GetMethod(string name) => typeof(T).GetMethod( - name, BindingFlags.Public | BindingFlags.Instance)!; + Assert.Equal(1, (int)GetMethod(typeof(ValueTypeWithoutOverrides), nameof(ValueTypeWithoutOverrides.GetId)). + Invoke(obj, null)); } //Methods for Reflection Metadata From 19959c04f7ec3bc3123dd5ff8e1c041146586dba Mon Sep 17 00:00:00 2001 From: Steve Harter Date: Tue, 19 Apr 2022 09:16:21 -0500 Subject: [PATCH 03/21] Create true Nullables to simplify native and IL --- .../System/Reflection/Emit/DynamicMethod.cs | 4 +- .../src/System/RuntimeHandles.cs | 6 + .../src/System/RuntimeType.CoreCLR.cs | 147 ++++++-------- src/coreclr/vm/ecalllist.h | 2 + src/coreclr/vm/invokeutil.cpp | 26 +-- src/coreclr/vm/methodtable.h | 1 - src/coreclr/vm/methodtable.inl | 25 --- src/coreclr/vm/object.cpp | 72 +------ src/coreclr/vm/object.h | 1 - src/coreclr/vm/reflectioninvocation.cpp | 186 ++++++++---------- src/coreclr/vm/reflectioninvocation.h | 2 +- src/coreclr/vm/runtimehandles.h | 3 + .../src/System/Reflection/InvokeUtils.cs | 4 +- .../src/System/Reflection/MethodBase.cs | 3 +- .../src/System/Reflection/MethodInvoker.cs | 3 +- .../Reflection/RuntimeConstructorInfo.cs | 8 +- .../System/Reflection/RuntimeMethodInfo.cs | 4 +- 17 files changed, 183 insertions(+), 314 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/DynamicMethod.cs b/src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/DynamicMethod.cs index 0942a3a031200..f5992994effce 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/DynamicMethod.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/DynamicMethod.cs @@ -510,7 +510,7 @@ Signature LazyCreateSignature() { if (shouldCopyBackParameters[i]) { - parameters[i] = copyOfParameters[i]; + parameters[i] = RuntimeMethodHandle.ReboxFromNullable(copyOfParameters[i]); } } } @@ -570,7 +570,7 @@ Signature LazyCreateSignature() { if (shouldCopyBackParameters[i]) { - parameters[i] = copyOfParameters[i]; + parameters[i] = RuntimeMethodHandle.ReboxFromNullable(copyOfParameters[i]); } } diff --git a/src/coreclr/System.Private.CoreLib/src/System/RuntimeHandles.cs b/src/coreclr/System.Private.CoreLib/src/System/RuntimeHandles.cs index 450f9a3441d86..edc8e58a52a5e 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/RuntimeHandles.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/RuntimeHandles.cs @@ -981,6 +981,12 @@ internal static MdUtf8String GetUtf8Name(RuntimeMethodHandleInternal method) [MethodImpl(MethodImplOptions.InternalCall)] internal static extern object? InvokeMethod(object? target, void** arguments, Signature sig, bool isConstructor); + [MethodImpl(MethodImplOptions.InternalCall)] + internal static extern object? ReboxFromNullable(object? src); + + [MethodImpl(MethodImplOptions.InternalCall)] + internal static extern object ReboxToNullable(object? src, RuntimeType destNullableType); + [LibraryImport(RuntimeHelpers.QCall, EntryPoint = "RuntimeMethodHandle_GetMethodInstantiation")] private static partial void GetMethodInstantiation(RuntimeMethodHandleInternal method, ObjectHandleOnStack types, Interop.BOOL fAsRuntimeTypeArray); diff --git a/src/coreclr/System.Private.CoreLib/src/System/RuntimeType.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/RuntimeType.CoreCLR.cs index 0670e4660a8eb..c5424474b05d6 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/RuntimeType.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/RuntimeType.CoreCLR.cs @@ -3475,7 +3475,7 @@ public override Type MakeArrayType(int rank) private static extern bool CanValueSpecialCast(RuntimeType valueType, RuntimeType targetType); [MethodImpl(MethodImplOptions.InternalCall)] - private static extern object AllocateValueType(RuntimeType type, object? value, bool fForceTypeChange); + private static extern object AllocateValueType(RuntimeType type, object? value); private enum CheckValueStatus { @@ -3500,7 +3500,7 @@ internal void VerifyValueType(object? value) /// /// Verify and optionally convert the value for special cases. /// - /// True if the value should be considered a value type, False otherwise + /// True if should be considered a value type, False otherwise internal bool CheckValue( ref object? value, ref bool copyBack, @@ -3511,21 +3511,23 @@ internal bool CheckValue( // Already fast-pathed by the caller. Debug.Assert(!ReferenceEquals(value?.GetType(), this)); + // Since this cannot be a generic parameter, we use RuntimeTypeHandle.IsValueType here + // because it is faster than IsValueType + Debug.Assert(!IsGenericParameter); + // Fast path to whether a value can be assigned without conversion. if (IsInstanceOfType(value)) { - // Since this cannot be a generic parameter, we use RuntimeTypeHandle.IsValueType here - // because it is faster than IsValueType - Debug.Assert(!IsGenericParameter); - - if (RuntimeTypeHandle.IsValueType(this)) + if (IsNullableOfT) { - // Nullable is the only value type that will get here. - Debug.Assert(IsNullableOfT); - - // Fall through and treat as a reference type. + // Pass as a true boxed Nullable, not as a T or null. + value = RuntimeMethodHandle.ReboxToNullable(value, this); + return true; } + // Other value types won't get here since Type equality was previous checked. + Debug.Assert(!RuntimeTypeHandle.IsValueType(this)); + return false; } @@ -3547,7 +3549,7 @@ internal bool CheckValue( if (IsInstanceOfType(value)) { copyBack = true; - return IsValueType; + return IsValueType; // Note the call to IsValueType, not the variable. } result = TryChangeType(ref value, out copyBack, out isValueType); @@ -3567,7 +3569,7 @@ internal bool CheckValue( } Debug.Fail("Error result not expected"); - return false; + return default; } private CheckValueStatus TryChangeType( @@ -3575,113 +3577,89 @@ private CheckValueStatus TryChangeType( out bool copyBack, out bool isValueType) { - // If this is a ByRef get the element type and check if it's compatible if (IsByRef) { + copyBack = true; RuntimeType sigElementType = RuntimeTypeHandle.GetElementType(this); - - // If a nullable, pass the object even though it's a value type. - if (sigElementType.IsNullableOfT) - { - // Treat as a reference type since a null value may be replaced with T or vise-versa. - isValueType = false; - copyBack = true; - return CheckValueStatus.Success; - } + Debug.Assert(!sigElementType.IsGenericParameter); if (sigElementType.IsInstanceOfType(value)) { - // Need to create an instance of the ByRef if null was provided, but only if primitive, enum or value type - value = AllocateValueType(sigElementType, value, fForceTypeChange: false); - isValueType = sigElementType.IsValueType; - copyBack = true; - return CheckValueStatus.Success; - } - - if (value == null) - { - if (IsByRefLike) + Debug.Assert(!sigElementType.IsGenericParameter); + isValueType = RuntimeTypeHandle.IsValueType(sigElementType); + if (sigElementType.IsValueType) { - isValueType = copyBack = default; - return CheckValueStatus.NotSupported_ByRefLike; + if (sigElementType.IsNullableOfT) + { + // Pass as a true boxed Nullable, not as a T or null. + value = RuntimeMethodHandle.ReboxToNullable(value, sigElementType); + } + else + { + // Need to create an instance of the ByRef if null was provided, but only for value types. + value = AllocateValueType(sigElementType, value: value); + } } - value = AllocateValueType(sigElementType, value, fForceTypeChange: false); - isValueType = sigElementType.IsValueType; - copyBack = true; return CheckValueStatus.Success; } - if (NeedsSpecialCast()) + if (value == null) { - if (SpecialCast(sigElementType, ref value) == CheckValueStatus.Success) + isValueType = RuntimeTypeHandle.IsValueType(sigElementType); + if (!isValueType) { - isValueType = true; - copyBack = false; + // Normally we don't get here since 'null' was previosuly checked, but due to binders we can. return CheckValueStatus.Success; } + + if (sigElementType.IsByRefLike) + { + return CheckValueStatus.NotSupported_ByRefLike; + } + + // Allocate default. + value = AllocateValueType(sigElementType, value: null); + return CheckValueStatus.Success; } - isValueType = copyBack = default; + isValueType = default; return CheckValueStatus.ArgumentException; } if (value == null) { - if (!RuntimeTypeHandle.IsValueType(this)) - { - isValueType = false; - copyBack = false; - return CheckValueStatus.Success; - } - - if (IsNullableOfT) + copyBack = false; + isValueType = RuntimeTypeHandle.IsValueType(this); + if (!isValueType) { - // Treat as a boxed value. - isValueType = false; - copyBack = false; + // Normally we don't get here since 'null' was previosuly checked, but due to binders we can. return CheckValueStatus.Success; } - if (RuntimeTypeHandle.IsByRefLike(this)) + if (IsByRefLike) { - isValueType = copyBack = default; return CheckValueStatus.NotSupported_ByRefLike; } - // Need to create a default instance of the value type. - value = AllocateValueType(this, value: null, fForceTypeChange: false); - isValueType = true; - copyBack = false; + // Allocate default. + value = AllocateValueType(this, value: null); return CheckValueStatus.Success; } - if (NeedsSpecialCast()) - { - if (SpecialCast(this, ref value) == CheckValueStatus.Success) - { - isValueType = true; - copyBack = false; - return CheckValueStatus.Success; - } - } - - isValueType = copyBack = default; - return CheckValueStatus.ArgumentException; - // Check the strange ones courtesy of reflection: - // - implicit cast between primitives - // - enum treated as underlying type - // - IntPtr and System.Reflection.Pointer to pointer types - bool NeedsSpecialCast() => IsPointer || IsEnum || IsPrimitive; - - static CheckValueStatus SpecialCast(RuntimeType type, ref object value) + // - Implicit cast between primitives + // - Enum treated as underlying type + // - Pointer (*) types to IntPtr (if dest is IntPtr) + // - System.Reflection.Pointer to appropriate pointer (*) type (if dest is pointer type) + if (IsPointer || IsEnum || IsPrimitive) { Pointer? pointer = value as Pointer; RuntimeType srcType = pointer != null ? pointer.GetPointerType() : (RuntimeType)value.GetType(); - if (!CanValueSpecialCast(srcType, type)) + if (!CanValueSpecialCast(srcType, this)) { + isValueType = copyBack = default; return CheckValueStatus.ArgumentException; } @@ -3692,15 +3670,20 @@ static CheckValueStatus SpecialCast(RuntimeType type, ref object value) else { CorElementType srcElementType = GetUnderlyingType(srcType); - CorElementType dstElementType = GetUnderlyingType(type); + CorElementType dstElementType = GetUnderlyingType(this); if (dstElementType != srcElementType) { - value = InvokeUtils.ConvertOrWiden(srcType, srcElementType, value, type, dstElementType); + value = InvokeUtils.ConvertOrWiden(srcType, srcElementType, value, this, dstElementType); } } + isValueType = true; + copyBack = false; return CheckValueStatus.Success; } + + copyBack = isValueType = default; + return CheckValueStatus.ArgumentException; } private static CorElementType GetUnderlyingType(RuntimeType type) diff --git a/src/coreclr/vm/ecalllist.h b/src/coreclr/vm/ecalllist.h index 0c0525eedfc89..19a96f78a094c 100644 --- a/src/coreclr/vm/ecalllist.h +++ b/src/coreclr/vm/ecalllist.h @@ -217,6 +217,8 @@ FCFuncEnd() FCFuncStart(gRuntimeMethodHandle) FCFuncElement("_GetCurrentMethod", RuntimeMethodHandle::GetCurrentMethod) FCFuncElement("InvokeMethod", RuntimeMethodHandle::InvokeMethod) + FCFuncElement("ReboxFromNullable", RuntimeMethodHandle::ReboxFromNullable) + FCFuncElement("ReboxToNullable", RuntimeMethodHandle::ReboxToNullable) FCFuncElement("GetImplAttributes", RuntimeMethodHandle::GetImplAttributes) FCFuncElement("GetAttributes", RuntimeMethodHandle::GetAttributes) FCFuncElement("GetDeclaringType", RuntimeMethodHandle::GetDeclaringType) diff --git a/src/coreclr/vm/invokeutil.cpp b/src/coreclr/vm/invokeutil.cpp index e50cdc7f1511d..944490f280b72 100644 --- a/src/coreclr/vm/invokeutil.cpp +++ b/src/coreclr/vm/invokeutil.cpp @@ -180,20 +180,8 @@ void InvokeUtil::CopyArg(TypeHandle th, PVOID argRef, ArgDestination *argDest) { case ELEMENT_TYPE_VALUETYPE: { - if (Nullable::IsNullableType(th)) - { - // ASSUMPTION: we only receive T or NULL values, not Nullable values - // and the values are boxed, unlike other value types. - MethodTable* pMT = th.AsMethodTable(); - OBJECTREF src = (OBJECTREF)(Object*)*(PVOID*)argRef; - if (!pMT->UnBoxIntoArg(argDest, src)) - COMPlusThrow(kArgumentException, W("Arg_ObjObj")); - } - else - { - MethodTable* pMT = th.GetMethodTable(); - CopyValueClassArg(argDest, argRef, pMT, 0); - } + MethodTable* pMT = th.GetMethodTable(); + CopyValueClassArg(argDest, argRef, pMT, 0); break; } @@ -213,10 +201,6 @@ void InvokeUtil::CopyArg(TypeHandle th, PVOID argRef, ArgDestination *argDest) { case ELEMENT_TYPE_BYREF: { - // We should never get here for nullable types. Instead invoke - // heads these off and morphs the type handle to not be byref anymore - _ASSERTE(!Nullable::IsNullableType(th.AsTypeDesc()->GetTypeParam())); - *(PVOID *)pArgDst = argRef; break; } @@ -1076,8 +1060,8 @@ OBJECTREF InvokeUtil::GetFieldValue(FieldDesc* pField, TypeHandle fieldType, OBJ CopyValueClass(obj->GetData(), p, fieldType.AsMethodTable()); } - // If it is a Nullable, box it using Nullable conventions. - // TODO: this double allocates on constructions which is wastefull + // If it is a Nullable, box it using Nullable conventions. + // TODO: this double allocates on constructions which is wastefull obj = Nullable::NormalizeBox(obj); break; } @@ -1115,5 +1099,3 @@ OBJECTREF InvokeUtil::GetFieldValue(FieldDesc* pField, TypeHandle fieldType, OBJ return obj; } - - diff --git a/src/coreclr/vm/methodtable.h b/src/coreclr/vm/methodtable.h index 039b5c06e14c8..4426f517d7524 100644 --- a/src/coreclr/vm/methodtable.h +++ b/src/coreclr/vm/methodtable.h @@ -2386,7 +2386,6 @@ class MethodTable OBJECTREF FastBox(void** data); #ifndef DACCESS_COMPILE BOOL UnBoxInto(void *dest, OBJECTREF src); - BOOL UnBoxIntoArg(ArgDestination *argDest, OBJECTREF src); void UnBoxIntoUnchecked(void *dest, OBJECTREF src); #endif diff --git a/src/coreclr/vm/methodtable.inl b/src/coreclr/vm/methodtable.inl index 72b628dae9545..049e4da57f3e1 100644 --- a/src/coreclr/vm/methodtable.inl +++ b/src/coreclr/vm/methodtable.inl @@ -1282,31 +1282,6 @@ inline BOOL MethodTable::UnBoxInto(void *dest, OBJECTREF src) return TRUE; } -//========================================================================================== -// unbox src into argument, making sure src is of the correct type. - -inline BOOL MethodTable::UnBoxIntoArg(ArgDestination *argDest, OBJECTREF src) -{ - CONTRACTL - { - NOTHROW; - GC_NOTRIGGER; - MODE_COOPERATIVE; - } - CONTRACTL_END; - - if (Nullable::IsNullableType(TypeHandle(this))) - return Nullable::UnBoxIntoArgNoGC(argDest, src, this); - else - { - if (src == NULL || src->GetMethodTable() != this) - return FALSE; - - CopyValueClassArg(argDest, src->UnBox(), this, 0); - } - return TRUE; -} - //========================================================================================== // unbox src into dest, No checks are done diff --git a/src/coreclr/vm/object.cpp b/src/coreclr/vm/object.cpp index c52c46dbd7e9a..992d72793c9f4 100644 --- a/src/coreclr/vm/object.cpp +++ b/src/coreclr/vm/object.cpp @@ -911,7 +911,6 @@ BOOL StringObject::CaseInsensitiveCompHelper(_In_reads_(aLength) WCHAR *strAChar // Next char strAChars++; strBChars++; } - } /*============================InternalTrailByteCheck============================ @@ -1599,7 +1598,7 @@ void* Nullable::ValueAddr(MethodTable* nullableMT) { } //=============================================================================== -// Special Logic to box a nullable as a boxed +// Special logic to box a nullable as a boxed OBJECTREF Nullable::Box(void* srcPtr, MethodTable* nullableMT) { @@ -1616,7 +1615,7 @@ OBJECTREF Nullable::Box(void* srcPtr, MethodTable* nullableMT) Nullable* src = (Nullable*) srcPtr; _ASSERTE(IsNullableType(nullableMT)); - // We better have a concrete instantiation, or our field offset asserts are not useful + // We better have a concrete instantiation, or our field offset asserts are not useful _ASSERTE(!nullableMT->ContainsGenericVariables()); if (!*src->HasValueAddr(nullableMT)) @@ -1647,10 +1646,10 @@ BOOL Nullable::UnBox(void* destPtr, OBJECTREF boxedVal, MethodTable* destMT) Nullable* dest = (Nullable*) destPtr; BOOL fRet = TRUE; - // We should only get here if we are unboxing a T as a Nullable + // We should only get here if we are unboxing a T as a Nullable _ASSERTE(IsNullableType(destMT)); - // We better have a concrete instantiation, or our field offset asserts are not useful + // We better have a concrete instantiation, or our field offset asserts are not useful _ASSERTE(!destMT->ContainsGenericVariables()); if (boxedVal == NULL) @@ -1722,7 +1721,7 @@ BOOL Nullable::UnBoxNoGC(void* destPtr, OBJECTREF boxedVal, MethodTable* destMT) { // For safety's sake, also allow true nullables to be unboxed normally. // This should not happen normally, but we want to be robust - if (destMT == boxedVal->GetMethodTable()) + if (destMT->IsEquivalentTo(boxedVal->GetMethodTable())) { CopyValueClass(dest, boxedVal->GetData(), destMT); return TRUE; @@ -1736,63 +1735,6 @@ BOOL Nullable::UnBoxNoGC(void* destPtr, OBJECTREF boxedVal, MethodTable* destMT) return TRUE; } -//=============================================================================== -// Special Logic to unbox a boxed T as a nullable into an argument -// specified by the argDest. -// Does not handle type equivalence (may conservatively return FALSE) -BOOL Nullable::UnBoxIntoArgNoGC(ArgDestination *argDest, OBJECTREF boxedVal, MethodTable* destMT) -{ - CONTRACTL - { - NOTHROW; - GC_NOTRIGGER; - MODE_COOPERATIVE; - } - CONTRACTL_END; - -#if defined(UNIX_AMD64_ABI) - if (argDest->IsStructPassedInRegs()) - { - // We should only get here if we are unboxing a T as a Nullable - _ASSERTE(IsNullableType(destMT)); - - // We better have a concrete instantiation, or our field offset asserts are not useful - _ASSERTE(!destMT->ContainsGenericVariables()); - - if (boxedVal == NULL) - { - // Logically we are doing *dest->HasValueAddr(destMT) = false; - // We zero out the whole structure because it may contain GC references - // and these need to be initialized to zero. (could optimize in the non-GC case) - InitValueClassArg(argDest, destMT); - } - else - { - if (!IsNullableForTypeNoGC(destMT, boxedVal->GetMethodTable())) - { - // For safety's sake, also allow true nullables to be unboxed normally. - // This should not happen normally, but we want to be robust - if (destMT == boxedVal->GetMethodTable()) - { - CopyValueClassArg(argDest, boxedVal->GetData(), destMT, 0); - return TRUE; - } - return FALSE; - } - - Nullable* dest = (Nullable*)argDest->GetStructGenRegDestinationAddress(); - *dest->HasValueAddr(destMT) = true; - int destOffset = (BYTE*)dest->ValueAddr(destMT) - (BYTE*)dest; - CopyValueClassArg(argDest, boxedVal->UnBox(), boxedVal->GetMethodTable(), destOffset); - } - return TRUE; - } - -#endif // UNIX_AMD64_ABI - - return UnBoxNoGC(argDest->GetDestinationAddress(), boxedVal, destMT); -} - //=============================================================================== // Special Logic to unbox a boxed T as a nullable // Does not do any type checks. @@ -1807,10 +1749,10 @@ void Nullable::UnBoxNoCheck(void* destPtr, OBJECTREF boxedVal, MethodTable* dest CONTRACTL_END; Nullable* dest = (Nullable*) destPtr; - // We should only get here if we are unboxing a T as a Nullable + // We should only get here if we are unboxing a T as a Nullable _ASSERTE(IsNullableType(destMT)); - // We better have a concrete instantiation, or our field offset asserts are not useful + // We better have a concrete instantiation, or our field offset asserts are not useful _ASSERTE(!destMT->ContainsGenericVariables()); if (boxedVal == NULL) diff --git a/src/coreclr/vm/object.h b/src/coreclr/vm/object.h index 92e04d55e3291..629e29e5cb393 100644 --- a/src/coreclr/vm/object.h +++ b/src/coreclr/vm/object.h @@ -2669,7 +2669,6 @@ class Nullable { static OBJECTREF Box(void* src, MethodTable* nullable); static BOOL UnBox(void* dest, OBJECTREF boxedVal, MethodTable* destMT); static BOOL UnBoxNoGC(void* dest, OBJECTREF boxedVal, MethodTable* destMT); - static BOOL UnBoxIntoArgNoGC(ArgDestination *argDest, OBJECTREF boxedVal, MethodTable* destMT); static void UnBoxNoCheck(void* dest, OBJECTREF boxedVal, MethodTable* destMT); static OBJECTREF BoxedNullableNull(TypeHandle nullableType) { return 0; } diff --git a/src/coreclr/vm/reflectioninvocation.cpp b/src/coreclr/vm/reflectioninvocation.cpp index 197d4ac026232..f1a5fa17c1d97 100644 --- a/src/coreclr/vm/reflectioninvocation.cpp +++ b/src/coreclr/vm/reflectioninvocation.cpp @@ -25,29 +25,6 @@ #include "dbginterface.h" #include "argdestination.h" -/**************************************************************************/ -/* if the type handle 'th' is a byref to a nullable type, return the - type handle to the nullable type in the byref. Otherwise return - the null type handle */ -static TypeHandle NullableTypeOfByref(TypeHandle th) { - CONTRACTL - { - NOTHROW; - GC_NOTRIGGER; - MODE_ANY; - } - CONTRACTL_END; - - if (th.GetVerifierCorElementType() != ELEMENT_TYPE_BYREF) - return TypeHandle(); - - TypeHandle subType = th.AsTypeDesc()->GetTypeParam(); - if (!Nullable::IsNullableType(subType)) - return TypeHandle(); - - return subType; -} - FCIMPL5(Object*, RuntimeFieldHandle::GetValue, ReflectFieldObject *pFieldUNSAFE, Object *instanceUNSAFE, ReflectClassBaseObject *pFieldTypeUNSAFE, ReflectClassBaseObject *pDeclaringTypeUNSAFE, CLR_BOOL *pDomainInitialized) { CONTRACTL { FCALL_CHECK; @@ -145,7 +122,10 @@ FCIMPL2(FC_BOOL_RET, ReflectionInvocation::CanValueSpecialCast, ReflectClassBase } FCIMPLEND -FCIMPL3(Object*, ReflectionInvocation::AllocateValueType, ReflectClassBaseObject *pTargetTypeUNSAFE, Object *valueUNSAFE, CLR_BOOL fForceTypeChange) { +/// +/// Allocate the value type and copy the optional value into it. +/// +FCIMPL2(Object*, ReflectionInvocation::AllocateValueType, ReflectClassBaseObject *pTargetTypeUNSAFE, Object *valueUNSAFE) { CONTRACTL { FCALL_CHECK; PRECONDITION(CheckPointer(pTargetTypeUNSAFE)); @@ -164,41 +144,24 @@ FCIMPL3(Object*, ReflectionInvocation::AllocateValueType, ReflectClassBaseObject gc.obj = gc.value; gc.refTargetType = (REFLECTCLASSBASEREF)ObjectToOBJECTREF(pTargetTypeUNSAFE); + HELPER_METHOD_FRAME_BEGIN_RET_PROTECT(gc); + TypeHandle targetType = gc.refTargetType->GetType(); - HELPER_METHOD_FRAME_BEGIN_RET_PROTECT(gc); - CorElementType targetElementType = targetType.GetSignatureCorElementType(); - if (InvokeUtil::IsPrimitiveType(targetElementType) || targetElementType == ELEMENT_TYPE_VALUETYPE) - { - MethodTable* allocMT = targetType.AsMethodTable(); + // This method is only intended for value types; it is not called directly by any public APIs + // so we don't expect validation issues here. + _ASSERTE(InvokeUtil::IsPrimitiveType(targetType.GetSignatureCorElementType()) || + targetType.GetSignatureCorElementType() == ELEMENT_TYPE_VALUETYPE); - if (allocMT->IsByRefLike()) { - COMPlusThrow(kNotSupportedException, W("NotSupported_ByRefLike")); - } + MethodTable* allocMT = targetType.AsMethodTable(); + _ASSERTE(!allocMT->IsByRefLike()); - if (gc.value != NULL) - { - // ignore the type of the incoming box if fForceTypeChange is set - // and the target type is not nullable - if (!fForceTypeChange || Nullable::IsNullableType(targetType)) - allocMT = gc.value->GetMethodTable(); - } + gc.obj = allocMT->Allocate(); + _ASSERTE(gc.obj != NULL); - // for null Nullable we don't want a default value being created. - // just allow the null value to be passed, as it will be converted to - // a true nullable - if (!(gc.value == NULL && Nullable::IsNullableType(targetType))) - { - // boxed value type are 'read-only' in the sence that you can't - // only the implementor of the value type can expose mutators. - // To insure byrefs don't mutate value classes in place, we make - // a copy (and if we were not given one, we create a null value type - // instance. - gc.obj = allocMT->Allocate(); - - if (gc.value != NULL) - CopyValueClass(gc.obj->UnBox(), gc.value->UnBox(), allocMT); - } + if (gc.value != NULL) { + _ASSERTE(allocMT->IsEquivalentTo(gc.value->GetMethodTable())); + CopyValueClass(gc.obj->UnBox(), gc.value->UnBox(), allocMT); } HELPER_METHOD_FRAME_END(); @@ -346,29 +309,6 @@ FCIMPL2(FC_BOOL_RET, RuntimeTypeHandle::IsInstanceOfType, ReflectClassBaseObject } FCIMPLEND -/****************************************************************************/ -/* boxed Nullable are represented as a boxed T, so there is no unboxed - Nullable inside to point at by reference. Because of this a byref - parameters of type Nullable are copied out of the boxed instance - (to a place on the stack), before the call is made (and this copy is - pointed at). After the call returns, this copy must be copied back to - the original argument array. ByRefToNullable, is a simple linked list - that remembers what copy-backs are needed */ - -struct ByRefToNullable { - unsigned argNum; // The argument number for this byrefNullable argument - void* data; // The data to copy back to the ByRefNullable. This points to the stack - TypeHandle type; // The type of Nullable for this argument - ByRefToNullable* next; // list of these - - ByRefToNullable(unsigned aArgNum, void* aData, TypeHandle aType, ByRefToNullable* aNext) { - argNum = aArgNum; - data = aData; - type = aType; - next = aNext; - } -}; - static OBJECTREF InvokeArrayConstructor(TypeHandle th, PVOID* args, int argCnt) { CONTRACTL { @@ -641,7 +581,6 @@ FCIMPL4(Object*, RuntimeMethodHandle::InvokeMethod, FrameWithCookie *pProtectValueClassFrame = NULL; ValueClassInfo *pValueClasses = NULL; - ByRefToNullable* byRefToNullables = NULL; // if we have the magic Value Class return, we need to allocate that class // and place a pointer to it on the stack. @@ -692,6 +631,7 @@ FCIMPL4(Object*, RuntimeMethodHandle::InvokeMethod, if (pMeth->IsUnboxingStub()) pThisPtr = OBJECTREFToObject(gc.target); else { + //todo: add tests for this (calling a nullable) // Create a true boxed Nullable and use that as the 'this' pointer. // since what is passed in is just a boxed T MethodTable* pMT = pMeth->GetMethodTable(); @@ -761,23 +701,8 @@ FCIMPL4(Object*, RuntimeMethodHandle::InvokeMethod, bool needsStackCopy = false; ArgDestination argDest(pTransitionBlock, ofs, argit.GetArgLocDescForStructInRegs()); - TypeHandle nullableType = NullableTypeOfByref(th); - if (!nullableType.IsNull()) { - // A boxed Nullable is represented as boxed T. So to pass a Nullable by reference, - // we have to create a Nullable on stack, copy the T into it, then pass it to the callee and - // after returning from the call, copy the T out of the Nullable back to the boxed T. - th = nullableType; - structSize = th.GetSize(); - needsStackCopy = true; - } #ifdef ENREGISTERED_PARAMTYPE_MAXSIZE - else if (argit.IsArgPassedByRef()) - { - needsStackCopy = true; - } -#endif - - if (needsStackCopy) + if (argit.IsArgPassedByRef()) { MethodTable* pMT = th.GetMethodTable(); _ASSERTE(pMT && pMT->IsValueType()); @@ -787,11 +712,6 @@ FCIMPL4(Object*, RuntimeMethodHandle::InvokeMethod, PVOID pStackCopy = _alloca(structSize); *(PVOID *)pArgDst = pStackCopy; - if (!nullableType.IsNull()) - { - byRefToNullables = new(_alloca(sizeof(ByRefToNullable))) ByRefToNullable(i, pStackCopy, nullableType, byRefToNullables); - } - // save the info into ValueClassInfo if (pMT->ContainsPointers()) { @@ -801,6 +721,7 @@ FCIMPL4(Object*, RuntimeMethodHandle::InvokeMethod, // We need a new ArgDestination that points to the stack copy argDest = ArgDestination(pStackCopy, 0, NULL); } +#endif InvokeUtil::CopyArg(th, args[i], &argDest); } @@ -874,12 +795,6 @@ FCIMPL4(Object*, RuntimeMethodHandle::InvokeMethod, gc.retVal = InvokeUtil::CreateObjectAfterInvoke(retTH, &callDescrData.returnValue); } - while (byRefToNullables != NULL) { - OBJECTREF obj = Nullable::Box(byRefToNullables->data, byRefToNullables->type.GetMethodTable()); - SetObjectReference((OBJECTREF*)args[byRefToNullables->argNum], obj); - byRefToNullables = byRefToNullables->next; - } - if (pProtectValueClassFrame != NULL) pProtectValueClassFrame->Pop(pThread); @@ -893,6 +808,67 @@ FCIMPL4(Object*, RuntimeMethodHandle::InvokeMethod, } FCIMPLEND +/// +/// Convert a boxed value of {T} (which is either {T} or null) to a true boxed Nullable{T}. +/// +FCIMPL2(Object*, RuntimeMethodHandle::ReboxToNullable, Object* pBoxedValUNSAFE, ReflectClassBaseObject *pDestUNSAFE) +{ + FCALL_CONTRACT; + + struct { + OBJECTREF pBoxed; + REFLECTCLASSBASEREF destType; + OBJECTREF retVal; + } gc; + + gc.pBoxed = ObjectToOBJECTREF(pBoxedValUNSAFE); + gc.destType = (REFLECTCLASSBASEREF)ObjectToOBJECTREF(pDestUNSAFE); + gc.retVal = NULL; + + HELPER_METHOD_FRAME_BEGIN_RET_PROTECT(gc); + + MethodTable* destMT = gc.destType->GetType().AsMethodTable(); + + gc.retVal = destMT->Allocate(); + void* buffer = gc.retVal->GetData(); + BOOL result = Nullable::UnBox(buffer, gc.pBoxed, destMT); + _ASSERTE(result == TRUE); + + HELPER_METHOD_FRAME_END(); + + return OBJECTREFToObject(gc.retVal); +} +FCIMPLEND + +/// +/// For a true boxed Nullable{T}, re-box to a boxed {T} or null, otherwise just return the input. +/// +FCIMPL1(Object*, RuntimeMethodHandle::ReboxFromNullable, Object* pBoxedValUNSAFE) +{ + FCALL_CONTRACT; + + struct { + OBJECTREF pBoxed; + OBJECTREF retVal; + } gc; + + gc.pBoxed = ObjectToOBJECTREF(pBoxedValUNSAFE); + gc.retVal = gc.pBoxed; + + HELPER_METHOD_FRAME_BEGIN_RET_PROTECT(gc); + + if (gc.pBoxed != NULL) { + MethodTable* retMT = gc.pBoxed->GetMethodTable(); + if (Nullable::IsNullableType(retMT)) + gc.retVal = Nullable::Box(gc.pBoxed->GetData(), retMT); + } + + HELPER_METHOD_FRAME_END(); + + return OBJECTREFToObject(gc.retVal); +} +FCIMPLEND + struct SkipStruct { StackCrawlMark* pStackMark; MethodDesc* pMeth; diff --git a/src/coreclr/vm/reflectioninvocation.h b/src/coreclr/vm/reflectioninvocation.h index 76eef427a7969..5e909b6b4c51e 100644 --- a/src/coreclr/vm/reflectioninvocation.h +++ b/src/coreclr/vm/reflectioninvocation.h @@ -62,7 +62,7 @@ class ReflectionInvocation { // helper fcalls for invocation static FCDECL2(FC_BOOL_RET, CanValueSpecialCast, ReflectClassBaseObject *valueType, ReflectClassBaseObject *targetType); - static FCDECL3(Object*, AllocateValueType, ReflectClassBaseObject *targetType, Object *valueUNSAFE, CLR_BOOL fForceTypeChange); + static FCDECL2(Object*, AllocateValueType, ReflectClassBaseObject *targetType, Object *valueUNSAFE); }; extern "C" void QCALLTYPE ReflectionInvocation_CompileMethod(MethodDesc * pMD); diff --git a/src/coreclr/vm/runtimehandles.h b/src/coreclr/vm/runtimehandles.h index 29bdfa08a846f..02ec1e078f4c9 100644 --- a/src/coreclr/vm/runtimehandles.h +++ b/src/coreclr/vm/runtimehandles.h @@ -224,6 +224,9 @@ class RuntimeMethodHandle { static FCDECL4(Object*, InvokeMethod, Object *target, PVOID* args, SignatureNative* pSig, CLR_BOOL fConstructor); + static FCDECL2(Object*, ReboxToNullable, Object *pBoxedValUNSAFE, ReflectClassBaseObject *pDestUNSAFE); + static FCDECL1(Object*, ReboxFromNullable, Object *pBoxedValUNSAFE); + struct StreamingContextData { Object * additionalContext; // additionalContex was changed from OBJECTREF to Object to avoid having a INT32 contextStates; // constructor in this struct. GCC doesn't allow structs with constructors to be diff --git a/src/libraries/System.Private.CoreLib/src/System/Reflection/InvokeUtils.cs b/src/libraries/System.Private.CoreLib/src/System/Reflection/InvokeUtils.cs index 74cf3c8181781..c5b26694eebb1 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Reflection/InvokeUtils.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Reflection/InvokeUtils.cs @@ -16,7 +16,7 @@ public static object ConvertOrWiden(Type srcType, CorElementType srcElementType, if (dstType.IsPointer) { - if (TryConvertPointer(srcObject, srcElementType, dstElementType, out IntPtr dstIntPtr)) + if (TryConvertPointer(srcObject, out IntPtr dstIntPtr)) { return dstIntPtr; } @@ -109,7 +109,7 @@ public static object ConvertOrWiden(Type srcType, CorElementType srcElementType, return dstObject; } - private static bool TryConvertPointer(object srcObject, CorElementType srcEEType, CorElementType dstEEType, out IntPtr dstIntPtr) + private static bool TryConvertPointer(object srcObject, out IntPtr dstIntPtr) { if (srcObject is IntPtr srcIntPtr) { diff --git a/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodBase.cs b/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodBase.cs index 65b371a630191..38ecb5c6ef0cb 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodBase.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodBase.cs @@ -163,7 +163,7 @@ BindingFlags invokeAttr if (arg is null) { - // Fast path that avoids calling CheckValue() for reference types. + // A fast path that avoids calling CheckValue() for null reference types. isValueType = RuntimeTypeHandle.IsValueType(sigType); if (isValueType || RuntimeTypeHandle.IsByRef(sigType)) { @@ -185,6 +185,7 @@ BindingFlags invokeAttr } else { + // Convert Type.Missing to the default value. if (paramInfo.DefaultValue == DBNull.Value) { throw new ArgumentException(SR.Arg_VarMissNull, nameof(parameters)); diff --git a/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodInvoker.cs b/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodInvoker.cs index 0da25be2ddee9..28c0c2f9337ce 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodInvoker.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodInvoker.cs @@ -39,7 +39,8 @@ public MethodInvoker(RuntimeMethodInfo methodInfo) _methodInfo.DeclaringType != typeof(Type) // Avoid stack crawl issue with GetType(). ) { - _emitInvoke = InvokerEmitUtil.CreateInvokeDelegate(_methodInfo); + // For testing slow path, disable Emit for now + //_emitInvoke = InvokerEmitUtil.CreateInvokeDelegate(_methodInfo); } _strategyDetermined = true; diff --git a/src/libraries/System.Private.CoreLib/src/System/Reflection/RuntimeConstructorInfo.cs b/src/libraries/System.Private.CoreLib/src/System/Reflection/RuntimeConstructorInfo.cs index c5f4d7b1345c7..c2302f7efc7a2 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Reflection/RuntimeConstructorInfo.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Reflection/RuntimeConstructorInfo.cs @@ -162,7 +162,7 @@ internal void ThrowNoInvokeException() { if (shouldCopyBackParameters[i]) { - parameters[i] = copyOfParameters[i]; + parameters[i] = RuntimeMethodHandle.ReboxFromNullable(copyOfParameters[i]); } } } @@ -222,7 +222,7 @@ private static unsafe void InvokeWithManyArguments( { if (shouldCopyBackParameters[i]) { - parameters[i] = copyOfParameters[i]; + parameters[i] = RuntimeMethodHandle.ReboxFromNullable(copyOfParameters[i]); } } } @@ -285,7 +285,7 @@ public override object Invoke(BindingFlags invokeAttr, Binder? binder, object?[] { if (shouldCopyBackParameters[i]) { - parameters[i] = copyOfParameters[i]; + parameters[i] = RuntimeMethodHandle.ReboxFromNullable(copyOfParameters[i]); } } } @@ -348,7 +348,7 @@ public override object Invoke(BindingFlags invokeAttr, Binder? binder, object?[] { if (shouldCopyBackParameters[i]) { - parameters[i] = copyOfParameters[i]; + parameters[i] = RuntimeMethodHandle.ReboxFromNullable(copyOfParameters[i]); } } diff --git a/src/libraries/System.Private.CoreLib/src/System/Reflection/RuntimeMethodInfo.cs b/src/libraries/System.Private.CoreLib/src/System/Reflection/RuntimeMethodInfo.cs index 87404f4723de1..70c81e8693c73 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Reflection/RuntimeMethodInfo.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Reflection/RuntimeMethodInfo.cs @@ -160,7 +160,7 @@ private void ThrowNoInvokeException() { if (shouldCopyBackParameters[i]) { - parameters[i] = copyOfParameters[i]; + parameters[i] = RuntimeMethodHandle.ReboxFromNullable(copyOfParameters[i]); } } } @@ -221,7 +221,7 @@ private void ThrowNoInvokeException() { if (shouldCopyBackParameters[i]) { - parameters[i] = copyOfParameters[i]; + parameters[i] = RuntimeMethodHandle.ReboxFromNullable(copyOfParameters[i]); } } From 40eb3914fc3e2fe2f1f389402f975c5c3dc89dbe Mon Sep 17 00:00:00 2001 From: Steve Harter Date: Tue, 19 Apr 2022 16:14:24 -0500 Subject: [PATCH 04/21] Update Emit code for new Nullability; still running non-emit for tests --- .../src/System/Reflection/InvokerEmitUtil.cs | 150 ++---------------- .../src/System/Reflection/MethodInvoker.cs | 6 +- .../tests/System/Reflection/PointerTests.cs | 74 +++++++++ .../src/System/RuntimeMethodHandle.cs | 4 + 4 files changed, 97 insertions(+), 137 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Reflection/InvokerEmitUtil.cs b/src/libraries/System.Private.CoreLib/src/System/Reflection/InvokerEmitUtil.cs index 0b46dd78ab566..4ff4046f230a6 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Reflection/InvokerEmitUtil.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Reflection/InvokerEmitUtil.cs @@ -13,16 +13,11 @@ internal static class InvokerEmitUtil // This is an instance method to avoid overhead of shuffle thunk. internal unsafe delegate object? InvokeFunc(T obj, object? target, IntPtr* arguments); - // Avoid high arg count due to increased stack. - // This also allows the local variables to use that more smaller encoded _S opcode variants. - public const int MaxArgumentCount = 64; - public static unsafe InvokeFunc CreateInvokeDelegate(MethodBase method) { Debug.Assert(!method.ContainsGenericParameters); ParameterInfo[] parameters = method.GetParametersNoCopy(); - Debug.Assert(parameters.Length <= MaxArgumentCount); Type[] delegateParameters = new Type[3] { typeof(T), typeof(object), typeof(IntPtr*) }; @@ -44,14 +39,8 @@ public static unsafe InvokeFunc CreateInvokeDelegate(MethodBase method) else { HandleThisPointer(il, method); - PushArguments(il, parameters, out NullableRefInfo[]? byRefNullables, out int byRefNullableCount); + PushArguments(il, parameters); Invoke(il, method); - - if (byRefNullableCount > 0) - { - Debug.Assert(byRefNullables != null); ; - UpdateNullables(il, parameters, byRefNullables, byRefNullableCount); - } } HandleReturn(il, method); @@ -77,144 +66,33 @@ private static void HandleThisPointer(ILGenerator il, MethodBase method) /// /// Push each argument. /// - private static void PushArguments( - ILGenerator il, - ParameterInfo[] parameters, - out NullableRefInfo[]? byRefNullables, - out int byRefNullablesCount - ) + private static void PushArguments(ILGenerator il, ParameterInfo[] parameters) { - byRefNullables = null; - byRefNullablesCount = 0; - - LocalBuilder? local_pArg = il.DeclareLocal(typeof(IntPtr*)); - il.Emit(OpCodes.Ldarg_2); - il.Emit(OpCodes.Stloc_S, local_pArg); - for (int i = 0; i < parameters.Length; i++) { RuntimeType parameterType = (RuntimeType)parameters[i].ParameterType; - - // Get the argument as a ref - il.Emit(OpCodes.Ldloc_S, local_pArg); - il.Emit(OpCodes.Ldind_Ref); - - if (parameterType.IsByRef) + if (parameterType.IsPointer) { - RuntimeType elementType = (RuntimeType)parameterType.GetElementType(); - - if (elementType.IsNullableOfT) - { - byRefNullables ??= new NullableRefInfo[MaxArgumentCount - i]; - - LocalBuilder tmp = il.DeclareLocal(typeof(object).MakeByRefType()); - il.Emit(OpCodes.Stloc_S, tmp); - il.Emit(OpCodes.Ldloc_S, tmp); - - // Get the raw pointer. - il.Emit(OpCodes.Ldobj, typeof(object)); - il.Emit(OpCodes.Unbox, elementType); - - // Copy the pointer to the temp variable and load as a ref. - LocalBuilder byRefPtr = il.DeclareLocal(elementType.MakePointerType()); - il.Emit(OpCodes.Stloc_S, byRefPtr); - il.Emit(OpCodes.Ldloca_S, byRefPtr); - il.Emit(OpCodes.Ldind_Ref); - - byRefNullables[byRefNullablesCount++] = new NullableRefInfo { ParameterIndex = i, LocalIndex = byRefPtr.LocalIndex }; - } - else if (elementType.IsPointer) - { - LocalBuilder tmp = il.DeclareLocal(typeof(IntPtr).MakeByRefType()); - il.Emit(OpCodes.Stloc_S, tmp); - il.Emit(OpCodes.Ldloc_S, tmp); - il.Emit(OpCodes.Ldobj, typeof(IntPtr).MakeByRefType()); - } - else - { - LocalBuilder tmp = il.DeclareLocal(parameterType); - il.Emit(OpCodes.Stloc_S, tmp); - il.Emit(OpCodes.Ldloca_S, tmp); - il.Emit(OpCodes.Ldind_Ref); //keep this or remove and use ldloca? - } + parameterType = (RuntimeType)typeof(IntPtr); } - else if (parameterType.IsNullableOfT) - { - // Nullable is the only incoming value type that is boxed. - LocalBuilder tmp = il.DeclareLocal(typeof(object).MakeByRefType()); - il.Emit(OpCodes.Stloc_S, tmp); - il.Emit(OpCodes.Ldloc_S, tmp); - il.Emit(OpCodes.Ldobj, typeof(object)); - il.Emit(OpCodes.Unbox, parameterType); - il.Emit(OpCodes.Ldobj, parameterType); - } - else if (parameterType.IsPointer) - { - LocalBuilder tmp = il.DeclareLocal(typeof(IntPtr).MakeByRefType()); - il.Emit(OpCodes.Stloc_S, tmp); - il.Emit(OpCodes.Ldloc_S, tmp); - il.Emit(OpCodes.Ldobj, typeof(IntPtr)); - } - else + il.Emit(OpCodes.Ldarg_2); + if (i != 0) { - LocalBuilder tmp = il.DeclareLocal(parameterType.MakeByRefType()); - il.Emit(OpCodes.Stloc_S, tmp); - il.Emit(OpCodes.Ldloc_S, tmp); - il.Emit(OpCodes.Ldobj, parameterType); + il.Emit(OpCodes.Ldc_I4, i * IntPtr.Size); + il.Emit(OpCodes.Add); } - // Move to the next argument. - if (i < parameters.Length - 1) + il.Emit(OpCodes.Call, Methods.ByReferenceOfByte_Value()); // This can be replaced by ldfld once byref fields are available in C# + if (!parameterType.IsByRef) { - il.Emit(OpCodes.Ldloc_S, local_pArg); - il.Emit(OpCodes.Sizeof, typeof(IntPtr)); - il.Emit(OpCodes.Add); - il.Emit(OpCodes.Stloc_S, local_pArg); + il.Emit(OpCodes.Ldobj, parameterType); } } } - /// - /// Update any nullables that were passed by reference. - /// - private static void UpdateNullables( - ILGenerator il, - ParameterInfo[] parameters, - NullableRefInfo[] byRefNullables, - int byRefNullablesCount) - { - for (int i = 0; i < byRefNullablesCount; i++) - { - NullableRefInfo info = byRefNullables[i]; - - RuntimeType parameterType = (RuntimeType)parameters[info.ParameterIndex].ParameterType; - Debug.Assert(parameterType.IsByRef); - - RuntimeType? elementType = (RuntimeType)parameterType.GetElementType()!; - Debug.Assert(elementType.IsNullableOfT); - - // Get the original byref location. - il.Emit(OpCodes.Ldc_I4_S, info.ParameterIndex); - il.Emit(OpCodes.Conv_I); - il.Emit(OpCodes.Sizeof, typeof(IntPtr)); - il.Emit(OpCodes.Mul); - il.Emit(OpCodes.Ldarg_2); - il.Emit(OpCodes.Add); - il.Emit(OpCodes.Ldind_I); - - // Get the Nullable& value and update the original. - il.Emit(OpCodes.Ldloc_S, info.LocalIndex); - il.Emit(OpCodes.Ldobj, elementType); - il.Emit(OpCodes.Box, elementType); - il.Emit(OpCodes.Stind_Ref); - } - } - private static void Invoke(ILGenerator il, MethodBase method) { - // todo: once we return the value without boxing add support for OpCodes.Tailcall for perf. - bool emitNew = method is RuntimeConstructorInfo; if (emitNew) { @@ -310,6 +188,12 @@ private struct NullableRefInfo private static class Methods { + private static MethodInfo? s_ByReferenceOfByte_Value; + [DynamicDependency(DynamicallyAccessedMemberTypes.PublicMethods, typeof(ByReference<>))] + public static MethodInfo ByReferenceOfByte_Value() => + s_ByReferenceOfByte_Value ?? + (s_ByReferenceOfByte_Value = typeof(ByReference).GetMethod("get_Value")!); + private static MethodInfo? s_ThrowHelper_Throw_NullReference_InvokeNullRefReturned; [DynamicDependency(DynamicallyAccessedMemberTypes.PublicMethods, typeof(ThrowHelper))] public static MethodInfo ThrowHelper_Throw_NullReference_InvokeNullRefReturned() => diff --git a/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodInvoker.cs b/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodInvoker.cs index 28c0c2f9337ce..e78d78fbb8dee 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodInvoker.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodInvoker.cs @@ -35,12 +35,10 @@ public MethodInvoker(RuntimeMethodInfo methodInfo) if (!_strategyDetermined) { if (RuntimeFeature.IsDynamicCodeCompiled && - _methodInfo.GetParametersNoCopy().Length <= InvokerEmitUtil.MaxArgumentCount && - _methodInfo.DeclaringType != typeof(Type) // Avoid stack crawl issue with GetType(). - ) + _methodInfo.DeclaringType != typeof(Type)) // Avoid stack crawl issue with GetType(). { // For testing slow path, disable Emit for now - //_emitInvoke = InvokerEmitUtil.CreateInvokeDelegate(_methodInfo); + // _emitInvoke = InvokerEmitUtil.CreateInvokeDelegate(_methodInfo); } _strategyDetermined = true; diff --git a/src/libraries/System.Runtime/tests/System/Reflection/PointerTests.cs b/src/libraries/System.Runtime/tests/System/Reflection/PointerTests.cs index cfe5f5b20ace8..f89f19ec5d19e 100644 --- a/src/libraries/System.Runtime/tests/System/Reflection/PointerTests.cs +++ b/src/libraries/System.Runtime/tests/System/Reflection/PointerTests.cs @@ -16,15 +16,27 @@ public void Method(byte* ptr, int expected) Assert.Equal(expected, unchecked((int)ptr)); } + public void MethodWithSystemPointer(Pointer ptr, int expected) + { + Assert.Equal(expected, unchecked((int)Pointer.Unbox(ptr))); + } + public bool* Return(int expected) { return unchecked((bool*)expected); } + + public object ReturnWithSystemPointer(int expected) + { + return Pointer.Box((byte*)expected, typeof(byte*)); + } } unsafe delegate void MethodDelegate(byte* ptr, int expected); + unsafe delegate void MethodDelegateWithSystemPointer(Pointer ptr, int expected); unsafe delegate bool* ReturnDelegate(int expected); + unsafe delegate object ReturnDelegateWithSystemPointer(int expected); public unsafe class PointerTests { @@ -242,6 +254,15 @@ public void PointerMethodDelegateParameter(int value) d.DynamicInvoke(Pointer.Box(unchecked((void*)value), typeof(byte*)), value); } + [Theory] + [MemberData(nameof(Pointers))] + public void MethodDelegateParameter_SystemPointer(int value) + { + var obj = new PointerHolder(); + MethodDelegateWithSystemPointer d = obj.MethodWithSystemPointer; + d.DynamicInvoke(Pointer.Box(unchecked((void*)value), typeof(byte*)), value); + } + [Fact] public void PointerNullMethodDelegateParameter() { @@ -250,6 +271,24 @@ public void PointerNullMethodDelegateParameter() d.DynamicInvoke(null, 0); } + [Fact] + public void PointerNullMethodDelegateParameter_InvalidType_SystemPointer() + { + // An null is not converted to a System.Pointer. + var obj = new PointerHolder(); + MethodDelegateWithSystemPointer d = obj.MethodWithSystemPointer; + try + { + d.DynamicInvoke(null, 0); + } + catch (TargetInvocationException e) when (e.InnerException is ArgumentException) + { + return; + } + + Assert.Fail("Inner exception should be ArgumentException."); + } + [Theory] [MemberData(nameof(Pointers))] public void IntPtrMethodDelegateParameter(int value) @@ -259,6 +298,19 @@ public void IntPtrMethodDelegateParameter(int value) d.DynamicInvoke((IntPtr)value, value); } + [Theory] + [MemberData(nameof(Pointers))] + public void IntPtrMethodDelegateParameter_InvalidType_SystemPointer(int value) + { + // An IntPtr is not converted to a System.Pointer. + var obj = new PointerHolder(); + MethodDelegateWithSystemPointer d = obj.MethodWithSystemPointer; + AssertExtensions.Throws(null, () => + { + d.DynamicInvoke((IntPtr)value, value); + }); + } + [Theory] [MemberData(nameof(Pointers))] public void PointerMethodDelegateParameter_InvalidType(int value) @@ -271,6 +323,16 @@ public void PointerMethodDelegateParameter_InvalidType(int value) }); } + [Theory] + [MemberData(nameof(Pointers))] + public void PointerMethodDelegateParameter_InvalidType_SystemPointer(int value) + { + // Although the type boxed doesn't match, when unboxing void* is returned. + var obj = new PointerHolder(); + MethodDelegateWithSystemPointer d = obj.MethodWithSystemPointer; + d.DynamicInvoke(Pointer.Box(unchecked((void*)value), typeof(long*)), value); + } + [Theory] [MemberData(nameof(Pointers))] public void PointerMethodDelegateReturn(int value) @@ -282,5 +344,17 @@ public void PointerMethodDelegateReturn(int value) void* actualPointer = Pointer.Unbox(actualValue); Assert.Equal(value, unchecked((int)actualPointer)); } + + [Theory] + [MemberData(nameof(Pointers))] + public void PointerMethodDelegateReturn_SystemPointer(int value) + { + var obj = new PointerHolder(); + ReturnDelegateWithSystemPointer d = obj.ReturnWithSystemPointer; + object actualValue = d.DynamicInvoke(value); + Assert.IsType(actualValue); + void* actualPointer = Pointer.Unbox(actualValue); + Assert.Equal(value, unchecked((int)actualPointer)); + } } } diff --git a/src/mono/System.Private.CoreLib/src/System/RuntimeMethodHandle.cs b/src/mono/System.Private.CoreLib/src/System/RuntimeMethodHandle.cs index 2f0e1b726082f..768aaa4dc6271 100644 --- a/src/mono/System.Private.CoreLib/src/System/RuntimeMethodHandle.cs +++ b/src/mono/System.Private.CoreLib/src/System/RuntimeMethodHandle.cs @@ -85,5 +85,9 @@ internal bool IsNullHandle() { return value == IntPtr.Zero; } + + + // Temporary placeholder until Mono adds support for supporting boxing true Nullables. + internal static object? ReboxFromNullable(object? src) => src; } } From a1f7a5cddc3427d81d8e8fe042bddba768e5c8c7 Mon Sep 17 00:00:00 2001 From: Steve Harter Date: Wed, 20 Apr 2022 14:40:51 -0500 Subject: [PATCH 05/21] Enable IL for testing; improve perf for ByRef param validation --- .../System/Reflection/Emit/DynamicMethod.cs | 18 +- .../Reflection/Emit/DynamicMethodInvoker.cs | 22 ++- .../RuntimeConstructorInfo.CoreCLR.cs | 19 +-- src/coreclr/vm/reflectioninvocation.cpp | 4 +- .../System/Reflection/ConstructorInvoker.cs | 76 ++++++++- .../src/System/Reflection/InvokerEmitUtil.cs | 158 ++++++++---------- .../src/System/Reflection/MethodBase.cs | 51 +++--- .../src/System/Reflection/MethodInvoker.cs | 48 +++--- .../tests/MethodInfoTests.cs | 8 + .../Reflection/RuntimeMethodInfo.Mono.cs | 4 + .../src/System/RuntimeMethodHandle.cs | 1 - 11 files changed, 234 insertions(+), 175 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/DynamicMethod.cs b/src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/DynamicMethod.cs index f5992994effce..a3bb2740c4cd1 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/DynamicMethod.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/DynamicMethod.cs @@ -579,23 +579,9 @@ Signature LazyCreateSignature() [DebuggerHidden] [DebuggerStepThrough] - internal unsafe object? InvokeNonEmitUnsafe(object? obj, IntPtr* arguments, BindingFlags invokeAttr) + internal unsafe object? InvokeNonEmitUnsafe(object? obj, IntPtr* arguments) { - if ((invokeAttr & BindingFlags.DoNotWrapExceptions) == 0) - { - try - { - return RuntimeMethodHandle.InvokeMethod(obj, (void**)arguments, Signature, isConstructor: false); - } - catch (Exception e) - { - throw new TargetInvocationException(e); - } - } - else - { - return RuntimeMethodHandle.InvokeMethod(obj, (void**)arguments, Signature, isConstructor: false); - } + return RuntimeMethodHandle.InvokeMethod(obj, (void**)arguments, Signature, isConstructor: false); } public override object[] GetCustomAttributes(Type attributeType, bool inherit) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/DynamicMethodInvoker.cs b/src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/DynamicMethodInvoker.cs index 9e5a5e17be4e8..99715b7fb4074 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/DynamicMethodInvoker.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/DynamicMethodInvoker.cs @@ -1,9 +1,6 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -using System.Diagnostics; -using System.Runtime.CompilerServices; - namespace System.Reflection.Emit { internal sealed partial class DynamicMethodInvoker @@ -17,8 +14,23 @@ public DynamicMethodInvoker(DynamicMethod dynamicMethod) public unsafe object? InvokeUnsafe(object? obj, IntPtr* args, BindingFlags invokeAttr) { - // Todo: add strategy for calling IL Emit-based version - return _dynamicMethod.InvokeNonEmitUnsafe(obj, args, invokeAttr); + // Always use the slow path; the Emit-based fast path can be added but in general dynamic + // methods are invoked through a direct delegate, not through Invoke(). + if ((invokeAttr & BindingFlags.DoNotWrapExceptions) == 0) + { + try + { + return _dynamicMethod.InvokeNonEmitUnsafe(obj, args); + } + catch (Exception e) + { + throw new TargetInvocationException(e); + } + } + else + { + return _dynamicMethod.InvokeNonEmitUnsafe(obj, args); + } } } } diff --git a/src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeConstructorInfo.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeConstructorInfo.CoreCLR.cs index 35e14b8ebfee4..3fead3cf78810 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeConstructorInfo.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeConstructorInfo.CoreCLR.cs @@ -95,26 +95,15 @@ Signature LazyCreateSignature() internal BindingFlags BindingFlags => m_bindingFlags; +#pragma warning disable CA1822 // Mark members as static + internal bool SupportsNewInvoke => true; +#pragma warning restore CA1822 // Mark members as static [DebuggerStepThrough] [DebuggerHidden] internal unsafe object InvokeNonEmitUnsafe(object? obj, IntPtr* args, Span argsForTemporaryMonoSupport, BindingFlags invokeAttr) { - if ((invokeAttr & BindingFlags.DoNotWrapExceptions) == 0) - { - try - { - return RuntimeMethodHandle.InvokeMethod(obj, (void**)args, Signature, isConstructor: obj is null)!; - } - catch (Exception ex) - { - throw new TargetInvocationException(ex); - } - } - else - { - return RuntimeMethodHandle.InvokeMethod(obj, (void**)args, Signature, isConstructor: obj is null)!; - } + return RuntimeMethodHandle.InvokeMethod(obj, (void**)args, Signature, isConstructor: obj is null)!; } #endregion diff --git a/src/coreclr/vm/reflectioninvocation.cpp b/src/coreclr/vm/reflectioninvocation.cpp index f1a5fa17c1d97..0f4b947f387aa 100644 --- a/src/coreclr/vm/reflectioninvocation.cpp +++ b/src/coreclr/vm/reflectioninvocation.cpp @@ -624,14 +624,12 @@ FCIMPL4(Object*, RuntimeMethodHandle::InvokeMethod, else pThisPtr = OBJECTREFToObject(gc.retVal); } - else - if (!pMeth->GetMethodTable()->IsValueType()) + else if (!pMeth->GetMethodTable()->IsValueType()) pThisPtr = OBJECTREFToObject(gc.target); else { if (pMeth->IsUnboxingStub()) pThisPtr = OBJECTREFToObject(gc.target); else { - //todo: add tests for this (calling a nullable) // Create a true boxed Nullable and use that as the 'this' pointer. // since what is passed in is just a boxed T MethodTable* pMT = pMeth->GetMethodTable(); diff --git a/src/libraries/System.Private.CoreLib/src/System/Reflection/ConstructorInvoker.cs b/src/libraries/System.Private.CoreLib/src/System/Reflection/ConstructorInvoker.cs index 48fe08e491026..7fe23875c22dc 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Reflection/ConstructorInvoker.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Reflection/ConstructorInvoker.cs @@ -2,25 +2,93 @@ // The .NET Foundation licenses this file to you under the MIT license. using System.Diagnostics; +using System.Runtime.CompilerServices; namespace System.Reflection { internal sealed partial class ConstructorInvoker { - private readonly RuntimeConstructorInfo _constructorInfo; + private readonly RuntimeConstructorInfo _method; public InvocationFlags _invocationFlags; + private bool _invoked; + private bool _strategyDetermined; + private InvokerEmitUtil.InvokeFunc? _emitInvoke; public ConstructorInvoker(RuntimeConstructorInfo constructorInfo) { - _constructorInfo = constructorInfo; + _method = constructorInfo; } [DebuggerStepThrough] [DebuggerHidden] public unsafe object? InvokeUnsafe(object? obj, IntPtr* args, Span argsForTemporaryMonoSupport, BindingFlags invokeAttr) { - // Todo: add strategy for calling IL Emit-based version - return _constructorInfo.InvokeNonEmitUnsafe(obj, args, argsForTemporaryMonoSupport, invokeAttr); + if (!_strategyDetermined) + { + if (!_invoked) + { + // The first time, ignoring race conditions, use the slow path. + _invoked = true; + +#if true + // TEMP HACK FOR FORCING IL ON FIRST TIME: + if (RuntimeFeature.IsDynamicCodeCompiled) + { + _emitInvoke = InvokerEmitUtil.CreateInvokeDelegate(_method); + } + _strategyDetermined = true; +#endif + } + else + { + if (RuntimeFeature.IsDynamicCodeCompiled) + { + _emitInvoke = InvokerEmitUtil.CreateInvokeDelegate(_method); + } + + _strategyDetermined = true; + } + } + + if (_method.SupportsNewInvoke) + { + if ((invokeAttr & BindingFlags.DoNotWrapExceptions) == 0) + { + try + { + // For the rare and broken scenario of calling the constructor directly through MethodBase.Invoke() + // with a non-null 'obj', we use the slow path to avoid having two emit-based delegates. + if (_emitInvoke != null && obj == null) + { + return _emitInvoke(this, obj, args); + } + else + { + return _method.InvokeNonEmitUnsafe(obj, args, argsForTemporaryMonoSupport, invokeAttr); + } + } + catch (Exception e) + { + throw new TargetInvocationException(e); + } + } + else + { + if (_emitInvoke != null && obj == null) + { + return _emitInvoke(this, obj, args); + } + else + { + return _method.InvokeNonEmitUnsafe(obj, args, argsForTemporaryMonoSupport, invokeAttr); + } + } + } + else + { + // Remove this branch once Mono has the same exception handling and managed conversion logic. + return _method.InvokeNonEmitUnsafe(obj, args, argsForTemporaryMonoSupport, invokeAttr); + } } } } diff --git a/src/libraries/System.Private.CoreLib/src/System/Reflection/InvokerEmitUtil.cs b/src/libraries/System.Private.CoreLib/src/System/Reflection/InvokerEmitUtil.cs index 4ff4046f230a6..ea104cb3c4245 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Reflection/InvokerEmitUtil.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Reflection/InvokerEmitUtil.cs @@ -17,11 +17,11 @@ public static unsafe InvokeFunc CreateInvokeDelegate(MethodBase method) { Debug.Assert(!method.ContainsGenericParameters); - ParameterInfo[] parameters = method.GetParametersNoCopy(); + bool emitNew = method is RuntimeConstructorInfo; + bool hasThis = !(emitNew || method.IsStatic); Type[] delegateParameters = new Type[3] { typeof(T), typeof(object), typeof(IntPtr*) }; - // We could use the overload with 'owner' in order to associate to this module. var dm = new DynamicMethod( "InvokeStub_" + method.DeclaringType!.Name + "." + method.Name, returnType: typeof(object), @@ -31,28 +31,7 @@ public static unsafe InvokeFunc CreateInvokeDelegate(MethodBase method) ILGenerator il = dm.GetILGenerator(); - if (parameters.Length == 0) - { - HandleThisPointer(il, method); - Invoke(il, method); - } - else - { - HandleThisPointer(il, method); - PushArguments(il, parameters); - Invoke(il, method); - } - - HandleReturn(il, method); - - return (InvokeFunc)dm.CreateDelegate(typeof(InvokeFunc)); - } - - private static void HandleThisPointer(ILGenerator il, MethodBase method) - { - bool emitNew = method is RuntimeConstructorInfo; - bool hasThis = !(emitNew || method.IsStatic); - + // Handle instance methods. if (hasThis) { il.Emit(OpCodes.Ldarg_1); @@ -61,13 +40,9 @@ private static void HandleThisPointer(ILGenerator il, MethodBase method) il.Emit(OpCodes.Unbox, method.DeclaringType); } } - } - /// - /// Push each argument. - /// - private static void PushArguments(ILGenerator il, ParameterInfo[] parameters) - { + // Push the arguments. + ParameterInfo[] parameters = method.GetParametersNoCopy(); for (int i = 0; i < parameters.Length; i++) { RuntimeType parameterType = (RuntimeType)parameters[i].ParameterType; @@ -75,6 +50,10 @@ private static void PushArguments(ILGenerator il, ParameterInfo[] parameters) { parameterType = (RuntimeType)typeof(IntPtr); } + else if (parameterType.IsByRef && RuntimeTypeHandle.GetElementType(parameterType).IsPointer) + { + parameterType = (RuntimeType)typeof(IntPtr).MakeByRefType(); + } il.Emit(OpCodes.Ldarg_2); if (i != 0) @@ -89,87 +68,90 @@ private static void PushArguments(ILGenerator il, ParameterInfo[] parameters) il.Emit(OpCodes.Ldobj, parameterType); } } - } - private static void Invoke(ILGenerator il, MethodBase method) - { - bool emitNew = method is RuntimeConstructorInfo; + // Invoke the method. if (emitNew) { - Debug.Assert(method!.IsStatic); il.Emit(OpCodes.Newobj, (ConstructorInfo)method); } else if (method.IsStatic || method.DeclaringType!.IsValueType) { - il.Emit(OpCodes.Call, (RuntimeMethodInfo)method); + il.Emit(OpCodes.Call, (MethodInfo)method); } else { - il.Emit(OpCodes.Callvirt, (RuntimeMethodInfo)method); + il.Emit(OpCodes.Callvirt, (MethodInfo)method); } - } - - private static void HandleReturn(ILGenerator il, MethodBase method) - { - bool emitNew = method is RuntimeConstructorInfo; - Type returnType = emitNew ? method.DeclaringType! : ((RuntimeMethodInfo)method).ReturnType; - if (returnType == typeof(void)) + // Handle the return. + if (emitNew) { - il.Emit(OpCodes.Ldnull); - il.Emit(OpCodes.Ret); - return; + Type returnType = method.DeclaringType!; + if (returnType.IsValueType) + { + il.Emit(OpCodes.Box, returnType); + } } - - if (returnType.IsByRef) + else { - // Check for null ref return. - Type elementType = returnType.GetElementType()!; - Label retValueOk = il.DefineLabel(); - - il.Emit(OpCodes.Dup); - il.Emit(OpCodes.Brtrue_S, retValueOk); - il.Emit(OpCodes.Call, Methods.ThrowHelper_Throw_NullReference_InvokeNullRefReturned()); - il.MarkLabel(retValueOk); - - if (elementType.IsValueType) + Type returnType = ((RuntimeMethodInfo)method).ReturnType; + if (returnType == typeof(void)) { - LocalBuilder? local_return = il.DeclareLocal(typeof(object)); - il.Emit(OpCodes.Ldobj, elementType); - il.Emit(OpCodes.Box, elementType); - il.Emit(OpCodes.Stloc_S, local_return); - il.Emit(OpCodes.Ldloc_S, local_return); + il.Emit(OpCodes.Ldnull); } - else if (elementType.IsPointer) + else if (returnType.IsValueType) { - LocalBuilder? local_return = il.DeclareLocal(elementType); - il.Emit(OpCodes.Ldind_Ref); - il.Emit(OpCodes.Stloc_S, local_return); - il.Emit(OpCodes.Ldloc_S, local_return); - il.Emit(OpCodes.Ldtoken, elementType); + il.Emit(OpCodes.Box, returnType); + } + else if (returnType.IsPointer) + { + il.Emit(OpCodes.Ldtoken, returnType); il.Emit(OpCodes.Call, Methods.Type_GetTypeFromHandle()); il.Emit(OpCodes.Call, Methods.Pointer_Box()); } - else + else if (returnType.IsByRef) { - LocalBuilder? local_return = il.DeclareLocal(elementType); - il.Emit(OpCodes.Ldind_Ref); - il.Emit(OpCodes.Stloc_S, local_return); - il.Emit(OpCodes.Ldloc_S, local_return); + // Check for null ref return. + Type elementType = returnType.GetElementType()!; + Label retValueOk = il.DefineLabel(); + il.Emit(OpCodes.Dup); + il.Emit(OpCodes.Brtrue_S, retValueOk); + il.Emit(OpCodes.Call, Methods.ThrowHelper_Throw_NullReference_InvokeNullRefReturned()); + il.MarkLabel(retValueOk); + + // Handle per-type differences. + if (elementType.IsValueType) + { + LocalBuilder? local_return = il.DeclareLocal(typeof(object)); + il.Emit(OpCodes.Ldobj, elementType); + il.Emit(OpCodes.Box, elementType); + il.Emit(OpCodes.Stloc_S, local_return); + il.Emit(OpCodes.Ldloc_S, local_return); + } + else if (elementType.IsPointer) + { + LocalBuilder? local_return = il.DeclareLocal(elementType); + il.Emit(OpCodes.Ldind_Ref); + il.Emit(OpCodes.Stloc_S, local_return); + il.Emit(OpCodes.Ldloc_S, local_return); + il.Emit(OpCodes.Ldtoken, elementType); + il.Emit(OpCodes.Call, Methods.Type_GetTypeFromHandle()); + il.Emit(OpCodes.Call, Methods.Pointer_Box()); + } + else + { + LocalBuilder? local_return = il.DeclareLocal(elementType); + il.Emit(OpCodes.Ldind_Ref); + il.Emit(OpCodes.Stloc_S, local_return); + il.Emit(OpCodes.Ldloc_S, local_return); + } } } - else if (returnType.IsPointer) - { - il.Emit(OpCodes.Ldtoken, returnType); - il.Emit(OpCodes.Call, Methods.Type_GetTypeFromHandle()); - il.Emit(OpCodes.Call, Methods.Pointer_Box()); - } - else if (returnType.IsValueType) - { - il.Emit(OpCodes.Box, returnType); - } il.Emit(OpCodes.Ret); + + // Create the delegate. + return (InvokeFunc)dm.CreateDelegate(typeof(InvokeFunc)); } private static class ThrowHelper @@ -180,12 +162,6 @@ public static void Throw_NullReference_InvokeNullRefReturned() } } - private struct NullableRefInfo - { - public int ParameterIndex; - public int LocalIndex; - } - private static class Methods { private static MethodInfo? s_ByReferenceOfByte_Value; diff --git a/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodBase.cs b/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodBase.cs index 38ecb5c6ef0cb..5470a9a43a03d 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodBase.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodBase.cs @@ -163,42 +163,53 @@ BindingFlags invokeAttr if (arg is null) { - // A fast path that avoids calling CheckValue() for null reference types. + // Fast path for null reference types. isValueType = RuntimeTypeHandle.IsValueType(sigType); if (isValueType || RuntimeTypeHandle.IsByRef(sigType)) { isValueType = sigType.CheckValue(ref arg, ref copyBackArg, binder, culture, invokeAttr); } } - else if (ReferenceEquals(arg.GetType(), sigType)) - { - // Fast path that avoids calling CheckValue() when argument value matches the signature type. - isValueType = RuntimeTypeHandle.IsValueType(sigType); - } else { - paramInfos ??= GetParametersNoCopy(); - ParameterInfo paramInfo = paramInfos[i]; - if (!ReferenceEquals(arg, Type.Missing)) + RuntimeType argType = (RuntimeType)arg.GetType(); + + if (ReferenceEquals(argType, sigType)) { - isValueType = sigType.CheckValue(ref arg, ref copyBackArg, binder, culture, invokeAttr); + // Fast path when the value type matches the signature type. + isValueType = RuntimeTypeHandle.IsValueType(argType); + } + else if (RuntimeTypeHandle.IsByRef(sigType) && ReferenceEquals(argType, RuntimeTypeHandle.GetElementType(sigType))) + { + // Fast path when the value type matches the signature type. + isValueType = RuntimeTypeHandle.IsValueType(argType); + copyBackArg = true; } else { - // Convert Type.Missing to the default value. - if (paramInfo.DefaultValue == DBNull.Value) - { - throw new ArgumentException(SR.Arg_VarMissNull, nameof(parameters)); - } - - arg = paramInfo.DefaultValue; - if (ReferenceEquals(arg?.GetType(), sigType)) + paramInfos ??= GetParametersNoCopy(); + ParameterInfo paramInfo = paramInfos[i]; + if (!ReferenceEquals(arg, Type.Missing)) { - isValueType = RuntimeTypeHandle.IsValueType(sigType); + isValueType = sigType.CheckValue(ref arg, ref copyBackArg, binder, culture, invokeAttr); } else { - isValueType = sigType.CheckValue(ref arg, ref copyBackArg, binder, culture, invokeAttr); + // Convert Type.Missing to the default value. + if (paramInfo.DefaultValue == DBNull.Value) + { + throw new ArgumentException(SR.Arg_VarMissNull, nameof(parameters)); + } + + arg = paramInfo.DefaultValue; + if (ReferenceEquals(arg?.GetType(), sigType)) + { + isValueType = RuntimeTypeHandle.IsValueType(sigType); + } + else + { + isValueType = sigType.CheckValue(ref arg, ref copyBackArg, binder, culture, invokeAttr); + } } } } diff --git a/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodInvoker.cs b/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodInvoker.cs index e78d78fbb8dee..ed1fe961446ec 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodInvoker.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodInvoker.cs @@ -9,42 +9,50 @@ namespace System.Reflection internal sealed partial class MethodInvoker { internal InvocationFlags _invocationFlags; - private readonly RuntimeMethodInfo _methodInfo; + private readonly RuntimeMethodInfo _method; private bool _invoked; private bool _strategyDetermined; private InvokerEmitUtil.InvokeFunc? _emitInvoke; public MethodInvoker(RuntimeMethodInfo methodInfo) { - _methodInfo = methodInfo; + _method = methodInfo; } [DebuggerStepThrough] [DebuggerHidden] public unsafe object? InvokeUnsafe(object? obj, IntPtr* args, Span argsForTemporaryMonoSupport, BindingFlags invokeAttr) { - if (!_invoked) - { - // Note: disabled for test reasons: - // The first time, ignoring race conditions, use the slow path. - // return _methodInfo.InvokeNonEmitUnsafe(obj, args, argsForTemporaryMonoSupport, invokeAttr); - - _invoked = true; - } - if (!_strategyDetermined) { - if (RuntimeFeature.IsDynamicCodeCompiled && - _methodInfo.DeclaringType != typeof(Type)) // Avoid stack crawl issue with GetType(). + if (!_invoked) { - // For testing slow path, disable Emit for now - // _emitInvoke = InvokerEmitUtil.CreateInvokeDelegate(_methodInfo); + // The first time, ignoring race conditions, use the slow path. + _invoked = true; + +#if true + // TEMP HACK FOR FORCING IL ON FIRST TIME: + if (RuntimeFeature.IsDynamicCodeCompiled && + _method.DeclaringType != typeof(Type)) // Avoid stack crawl issue with Type.GetType(). + { + _emitInvoke = InvokerEmitUtil.CreateInvokeDelegate(_method); + } + _strategyDetermined = true; +#endif } + else + { + if (RuntimeFeature.IsDynamicCodeCompiled && + _method.DeclaringType != typeof(Type)) // Avoid stack crawl issue with Type.GetType(). + { + _emitInvoke = InvokerEmitUtil.CreateInvokeDelegate(_method); + } - _strategyDetermined = true; + _strategyDetermined = true; + } } - if (_methodInfo.SupportsNewInvoke) + if (_method.SupportsNewInvoke) { if ((invokeAttr & BindingFlags.DoNotWrapExceptions) == 0) { @@ -56,7 +64,7 @@ public MethodInvoker(RuntimeMethodInfo methodInfo) } else { - return _methodInfo.InvokeNonEmitUnsafe(obj, args, argsForTemporaryMonoSupport, invokeAttr); + return _method.InvokeNonEmitUnsafe(obj, args, argsForTemporaryMonoSupport, invokeAttr); } } catch (Exception e) @@ -72,14 +80,14 @@ public MethodInvoker(RuntimeMethodInfo methodInfo) } else { - return _methodInfo.InvokeNonEmitUnsafe(obj, args, argsForTemporaryMonoSupport, invokeAttr); + return _method.InvokeNonEmitUnsafe(obj, args, argsForTemporaryMonoSupport, invokeAttr); } } } else { // Remove this branch once Mono has the same exception handling and managed conversion logic. - return _methodInfo.InvokeNonEmitUnsafe(obj, args, argsForTemporaryMonoSupport, invokeAttr); + return _method.InvokeNonEmitUnsafe(obj, args, argsForTemporaryMonoSupport, invokeAttr); } } } diff --git a/src/libraries/System.Reflection/tests/MethodInfoTests.cs b/src/libraries/System.Reflection/tests/MethodInfoTests.cs index c2a66bad643a5..9e6325ba0f713 100644 --- a/src/libraries/System.Reflection/tests/MethodInfoTests.cs +++ b/src/libraries/System.Reflection/tests/MethodInfoTests.cs @@ -736,6 +736,14 @@ public void ValueTypeMembers_WithoutOverrides() Invoke(obj, null)); } + [Fact] + public void NullableOfTMembers() + { + // Ensure calling a method on Nullable works. + MethodInfo mi = GetMethod(typeof(int?), nameof(Nullable.GetValueOrDefault)); + Assert.Equal(42, mi.Invoke(42, null)); + } + //Methods for Reflection Metadata private void DummyMethod1(string str, int iValue, long lValue) { diff --git a/src/mono/System.Private.CoreLib/src/System/Reflection/RuntimeMethodInfo.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Reflection/RuntimeMethodInfo.Mono.cs index f78082da3dfb0..c712e7f0b4df4 100644 --- a/src/mono/System.Private.CoreLib/src/System/Reflection/RuntimeMethodInfo.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Reflection/RuntimeMethodInfo.Mono.cs @@ -863,6 +863,10 @@ private static void InvokeClassConstructor() [MethodImplAttribute(MethodImplOptions.InternalCall)] internal extern object InternalInvoke(object? obj, in Span parameters, out Exception exc); +#pragma warning disable CA1822 // Mark members as static + internal bool SupportsNewInvoke => false; +#pragma warning restore CA1822 // Mark members as static + [DebuggerHidden] [DebuggerStepThrough] internal unsafe object? InvokeNonEmitUnsafe(object? obj, IntPtr* byrefParameters, Span argsForTemporaryMonoSupport, BindingFlags invokeAttr) diff --git a/src/mono/System.Private.CoreLib/src/System/RuntimeMethodHandle.cs b/src/mono/System.Private.CoreLib/src/System/RuntimeMethodHandle.cs index 768aaa4dc6271..d57633d1a52a8 100644 --- a/src/mono/System.Private.CoreLib/src/System/RuntimeMethodHandle.cs +++ b/src/mono/System.Private.CoreLib/src/System/RuntimeMethodHandle.cs @@ -86,7 +86,6 @@ internal bool IsNullHandle() return value == IntPtr.Zero; } - // Temporary placeholder until Mono adds support for supporting boxing true Nullables. internal static object? ReboxFromNullable(object? src) => src; } From ba141cebec62625dbdf7024d132dd1bc7abd3ad9 Mon Sep 17 00:00:00 2001 From: Steve Harter Date: Thu, 21 Apr 2022 15:34:55 -0500 Subject: [PATCH 06/21] Fixup merge --- src/coreclr/vm/object.cpp | 99 --------------------------------------- 1 file changed, 99 deletions(-) diff --git a/src/coreclr/vm/object.cpp b/src/coreclr/vm/object.cpp index 2079e6c6d7e5a..55547666eca04 100644 --- a/src/coreclr/vm/object.cpp +++ b/src/coreclr/vm/object.cpp @@ -1753,105 +1753,6 @@ BOOL Nullable::UnBoxNoGC(void* destPtr, OBJECTREF boxedVal, MethodTable* destMT) return TRUE; } -//=============================================================================== -<<<<<<< HEAD -======= -// Special Logic to unbox a boxed T as a nullable into an argument -// specified by the argDest. -// Does not handle type equivalence (may conservatively return FALSE) -BOOL Nullable::UnBoxIntoArgNoGC(ArgDestination *argDest, OBJECTREF boxedVal, MethodTable* destMT) -{ - CONTRACTL - { - NOTHROW; - GC_NOTRIGGER; - MODE_COOPERATIVE; - } - CONTRACTL_END; - -#if defined(UNIX_AMD64_ABI) - if (argDest->IsStructPassedInRegs()) - { - // We should only get here if we are unboxing a T as a Nullable - _ASSERTE(IsNullableType(destMT)); - - // We better have a concrete instantiation, or our field offset asserts are not useful - _ASSERTE(!destMT->ContainsGenericVariables()); - - if (boxedVal == NULL) - { - // Logically we are doing *dest->HasValueAddr(destMT) = false; - // We zero out the whole structure because it may contain GC references - // and these need to be initialized to zero. (could optimize in the non-GC case) - InitValueClassArg(argDest, destMT); - } - else - { - if (!IsNullableForTypeNoGC(destMT, boxedVal->GetMethodTable())) - { - // For safety's sake, also allow true nullables to be unboxed normally. - // This should not happen normally, but we want to be robust - if (destMT == boxedVal->GetMethodTable()) - { - CopyValueClassArg(argDest, boxedVal->GetData(), destMT, 0); - return TRUE; - } - return FALSE; - } - - Nullable* dest = (Nullable*)argDest->GetStructGenRegDestinationAddress(); - *dest->HasValueAddr(destMT) = true; - int destOffset = (BYTE*)dest->ValueAddr(destMT) - (BYTE*)dest; - CopyValueClassArg(argDest, boxedVal->UnBox(), boxedVal->GetMethodTable(), destOffset); - } - return TRUE; - } - -#endif // UNIX_AMD64_ABI - -#if defined(TARGET_LOONGARCH64) - if (argDest->IsStructPassedInRegs()) - { - // We should only get here if we are unboxing a T as a Nullable - _ASSERTE(IsNullableType(destMT)); - - // We better have a concrete instantiation, or our field offset asserts are not useful - _ASSERTE(!destMT->ContainsGenericVariables()); - - if (boxedVal == NULL) - { - // Logically we are doing *dest->HasValueAddr(destMT) = false; - // We zero out the whole structure becasue it may contain GC references - // and these need to be initialized to zero. (could optimize in the non-GC case) - InitValueClassArg(argDest, destMT); - } - else - { - if (!IsNullableForTypeNoGC(destMT, boxedVal->GetMethodTable())) - { - // For safety's sake, also allow true nullables to be unboxed normally. - // This should not happen normally, but we want to be robust - if (destMT == boxedVal->GetMethodTable()) - { - CopyValueClassArg(argDest, boxedVal->GetData(), destMT, 0); - return TRUE; - } - return FALSE; - } - - CopyValueClassArg(argDest, boxedVal->UnBox(), boxedVal->GetMethodTable(), 0); - *(UINT64*)(argDest->GetStructGenRegDestinationAddress()) = 1; - } - return TRUE; - } - -#endif - - return UnBoxNoGC(argDest->GetDestinationAddress(), boxedVal, destMT); -} - -//=============================================================================== ->>>>>>> e09903f9b1bee58e5def96007e9fd743685ef8e2 // Special Logic to unbox a boxed T as a nullable // Does not do any type checks. void Nullable::UnBoxNoCheck(void* destPtr, OBJECTREF boxedVal, MethodTable* destMT) From 79571d9b56985f5ae5240cdfd00e36c3da5a5d28 Mon Sep 17 00:00:00 2001 From: Steve Harter Date: Fri, 22 Apr 2022 13:44:58 -0500 Subject: [PATCH 07/21] Ensure a copy is made for byref value types --- .../src/System/RuntimeHandles.cs | 13 +++++ .../src/System/RuntimeType.CoreCLR.cs | 26 ++++++++-- src/coreclr/vm/object.cpp | 1 + .../src/System/Reflection/InvokerEmitUtil.cs | 29 ++++++----- .../src/System/Reflection/MethodBase.cs | 47 +++++++++--------- .../tests/MethodInfoTests.cs | 49 +++++++++++++++++++ .../src/System/RuntimeType.Mono.cs | 5 ++ 7 files changed, 130 insertions(+), 40 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/RuntimeHandles.cs b/src/coreclr/System.Private.CoreLib/src/System/RuntimeHandles.cs index edc8e58a52a5e..f7086bd10591d 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/RuntimeHandles.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/RuntimeHandles.cs @@ -138,6 +138,19 @@ internal static bool IsByRef(RuntimeType type) return corElemType == CorElementType.ELEMENT_TYPE_BYREF; } + internal static bool TryGetByRefElementType(RuntimeType type, [NotNullWhen(true)] out RuntimeType? elementType) + { + CorElementType corElemType = GetCorElementType(type); + if (corElemType == CorElementType.ELEMENT_TYPE_BYREF) + { + elementType = GetElementType(type); + return true; + } + + elementType = null; + return false; + } + internal static bool IsPointer(RuntimeType type) { CorElementType corElemType = GetCorElementType(type); diff --git a/src/coreclr/System.Private.CoreLib/src/System/RuntimeType.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/RuntimeType.CoreCLR.cs index 7225c973d95a3..47d119e95b642 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/RuntimeType.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/RuntimeType.CoreCLR.cs @@ -3597,10 +3597,10 @@ private CheckValueStatus TryChangeType( out bool copyBack, out bool isValueType) { - if (IsByRef) + RuntimeType? sigElementType; + if (RuntimeTypeHandle.TryGetByRefElementType(this, out sigElementType)) { copyBack = true; - RuntimeType sigElementType = RuntimeTypeHandle.GetElementType(this); Debug.Assert(!sigElementType.IsGenericParameter); if (sigElementType.IsInstanceOfType(value)) @@ -3616,8 +3616,8 @@ private CheckValueStatus TryChangeType( } else { - // Need to create an instance of the ByRef if null was provided, but only for value types. - value = AllocateValueType(sigElementType, value: value); + // Make a copy to prevent the boxed instance from being directly modified by the method. + value = AllocateValueType(sigElementType, value); } } @@ -3706,6 +3706,24 @@ private CheckValueStatus TryChangeType( return CheckValueStatus.ArgumentException; } + internal bool TryByRefFastPath(ref object arg, ref bool isValueType) + { + if (RuntimeTypeHandle.TryGetByRefElementType(this, out RuntimeType? sigElementType) && + ReferenceEquals(sigElementType, arg.GetType())) + { + isValueType = sigElementType.IsValueType; + if (isValueType) + { + // Make a copy to prevent the boxed instance from being directly modified by the method. + arg = AllocateValueType(sigElementType, arg); + } + + return true; + } + + return false; + } + private static CorElementType GetUnderlyingType(RuntimeType type) { if (type.IsEnum) diff --git a/src/coreclr/vm/object.cpp b/src/coreclr/vm/object.cpp index 55547666eca04..995cf608737b9 100644 --- a/src/coreclr/vm/object.cpp +++ b/src/coreclr/vm/object.cpp @@ -1753,6 +1753,7 @@ BOOL Nullable::UnBoxNoGC(void* destPtr, OBJECTREF boxedVal, MethodTable* destMT) return TRUE; } +//=============================================================================== // Special Logic to unbox a boxed T as a nullable // Does not do any type checks. void Nullable::UnBoxNoCheck(void* destPtr, OBJECTREF boxedVal, MethodTable* destMT) diff --git a/src/libraries/System.Private.CoreLib/src/System/Reflection/InvokerEmitUtil.cs b/src/libraries/System.Private.CoreLib/src/System/Reflection/InvokerEmitUtil.cs index ea104cb3c4245..23aacaed588f2 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Reflection/InvokerEmitUtil.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Reflection/InvokerEmitUtil.cs @@ -4,7 +4,6 @@ using System.Diagnostics; using System.Diagnostics.CodeAnalysis; using System.Reflection.Emit; -using System.Runtime.CompilerServices; namespace System.Reflection { @@ -45,15 +44,7 @@ public static unsafe InvokeFunc CreateInvokeDelegate(MethodBase method) ParameterInfo[] parameters = method.GetParametersNoCopy(); for (int i = 0; i < parameters.Length; i++) { - RuntimeType parameterType = (RuntimeType)parameters[i].ParameterType; - if (parameterType.IsPointer) - { - parameterType = (RuntimeType)typeof(IntPtr); - } - else if (parameterType.IsByRef && RuntimeTypeHandle.GetElementType(parameterType).IsPointer) - { - parameterType = (RuntimeType)typeof(IntPtr).MakeByRefType(); - } + RuntimeType parameterType = NormalizePointerType((RuntimeType)parameters[i].ParameterType); il.Emit(OpCodes.Ldarg_2); if (i != 0) @@ -94,7 +85,7 @@ public static unsafe InvokeFunc CreateInvokeDelegate(MethodBase method) } else { - Type returnType = ((RuntimeMethodInfo)method).ReturnType; + RuntimeType returnType = (RuntimeType)((RuntimeMethodInfo)method).ReturnType; if (returnType == typeof(void)) { il.Emit(OpCodes.Ldnull); @@ -130,7 +121,7 @@ public static unsafe InvokeFunc CreateInvokeDelegate(MethodBase method) } else if (elementType.IsPointer) { - LocalBuilder? local_return = il.DeclareLocal(elementType); + LocalBuilder? local_return = il.DeclareLocal(NormalizePointerType(returnType)); // IntPtr.MakeByRefType() il.Emit(OpCodes.Ldind_Ref); il.Emit(OpCodes.Stloc_S, local_return); il.Emit(OpCodes.Ldloc_S, local_return); @@ -154,6 +145,20 @@ public static unsafe InvokeFunc CreateInvokeDelegate(MethodBase method) return (InvokeFunc)dm.CreateDelegate(typeof(InvokeFunc)); } + private static RuntimeType NormalizePointerType(RuntimeType type) + { + if (type.IsPointer) + { + type = (RuntimeType)typeof(IntPtr); + } + else if (type.IsByRef && RuntimeTypeHandle.GetElementType(type).IsPointer) + { + type = (RuntimeType)typeof(IntPtr).MakeByRefType(); + } + + return type; + } + private static class ThrowHelper { public static void Throw_NullReference_InvokeNullRefReturned() diff --git a/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodBase.cs b/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodBase.cs index 5470a9a43a03d..a4d06b71960fe 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodBase.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodBase.cs @@ -157,7 +157,7 @@ BindingFlags invokeAttr for (int i = 0; i < parameters.Length; i++) { bool copyBackArg = false; - bool isValueType; + bool isValueType = false; object? arg = parameters[i]; RuntimeType sigType = sigTypes[i]; @@ -176,40 +176,39 @@ BindingFlags invokeAttr if (ReferenceEquals(argType, sigType)) { - // Fast path when the value type matches the signature type. + // Fast path when the value's type matches the signature type. isValueType = RuntimeTypeHandle.IsValueType(argType); } - else if (RuntimeTypeHandle.IsByRef(sigType) && ReferenceEquals(argType, RuntimeTypeHandle.GetElementType(sigType))) + else if (sigType.TryByRefFastPath(ref arg, ref isValueType)) { - // Fast path when the value type matches the signature type. - isValueType = RuntimeTypeHandle.IsValueType(argType); + // Fast path when the value's type matches the signature type of a byref parameter. copyBackArg = true; } + else if (!ReferenceEquals(arg, Type.Missing)) + { + // Slow path that supports type conversions. + isValueType = sigType.CheckValue(ref arg, ref copyBackArg, binder, culture, invokeAttr); + } else { + // Convert Type.Missing to the default value. paramInfos ??= GetParametersNoCopy(); ParameterInfo paramInfo = paramInfos[i]; - if (!ReferenceEquals(arg, Type.Missing)) + + if (paramInfo.DefaultValue == DBNull.Value) { - isValueType = sigType.CheckValue(ref arg, ref copyBackArg, binder, culture, invokeAttr); + throw new ArgumentException(SR.Arg_VarMissNull, nameof(parameters)); + } + + arg = paramInfo.DefaultValue; + if (ReferenceEquals(arg?.GetType(), sigType)) + { + // Fast path when the default value's type matches the signature type. + isValueType = RuntimeTypeHandle.IsValueType(sigType); } else { - // Convert Type.Missing to the default value. - if (paramInfo.DefaultValue == DBNull.Value) - { - throw new ArgumentException(SR.Arg_VarMissNull, nameof(parameters)); - } - - arg = paramInfo.DefaultValue; - if (ReferenceEquals(arg?.GetType(), sigType)) - { - isValueType = RuntimeTypeHandle.IsValueType(sigType); - } - else - { - isValueType = sigType.CheckValue(ref arg, ref copyBackArg, binder, culture, invokeAttr); - } + isValueType = sigType.CheckValue(ref arg, ref copyBackArg, binder, culture, invokeAttr); } } } @@ -221,8 +220,8 @@ BindingFlags invokeAttr // as we validate them. n.b. This disallows use of ArrayPool, as ArrayPool-rented arrays are // considered user-visible to threads which may still be holding on to returned instances. // This separate array is also used to hold default values when 'null' is specified for value - // types, and also used to hold the results from conversions such as from Int16 to Int32; these - // default values and conversions should not be applied to the incoming arguments. + // types, and also used to hold the results from conversions such as from Int16 to Int32. For + // compat, these default values and conversions are not be applied to the incoming arguments. shouldCopyBack[i] = copyBackArg; copyOfParameters[i] = arg; diff --git a/src/libraries/System.Reflection/tests/MethodInfoTests.cs b/src/libraries/System.Reflection/tests/MethodInfoTests.cs index 9e6325ba0f713..adf59fde57ab5 100644 --- a/src/libraries/System.Reflection/tests/MethodInfoTests.cs +++ b/src/libraries/System.Reflection/tests/MethodInfoTests.cs @@ -744,6 +744,32 @@ public void NullableOfTMembers() Assert.Equal(42, mi.Invoke(42, null)); } + [Fact] + public void CopyBackWithByRefArgs() + { + object i = 42; + object[] args = new object[] { i }; + GetMethod(typeof(CopyBackMethods), nameof(CopyBackMethods.IncrementByRef)).Invoke(null, args); + Assert.Equal(43, (int)args[0]); + Assert.NotSame(i, args[0]); // A copy should be made; a boxed instance should never be directly updated. + + i = 42; + args = new object[] { i }; + GetMethod(typeof(CopyBackMethods), nameof(CopyBackMethods.IncrementByNullableRef)).Invoke(null, args); + Assert.Equal(43, (int)args[0]); + Assert.NotSame(i, args[0]); + + object o = null; + args = new object[] { o }; + GetMethod(typeof(CopyBackMethods), nameof(CopyBackMethods.SetToNonNullByRef)).Invoke(null, args); + Assert.NotNull(args[0]); + + o = new object(); + args = new object[] { o }; + GetMethod(typeof(CopyBackMethods), nameof(CopyBackMethods.SetToNullByRef)).Invoke(null, args); + Assert.Null(args[0]); + } + //Methods for Reflection Metadata private void DummyMethod1(string str, int iValue, long lValue) { @@ -1036,6 +1062,29 @@ public static bool ValueToNullBoxed(ref int? i, int expected) } } + public static class CopyBackMethods + { + public static void IncrementByRef(ref int i) + { + i++; + } + + public static void IncrementByNullableRef(ref int? i) + { + i++; + } + + public static void SetToNullByRef(ref object o) + { + o = null; + } + + public static void SetToNonNullByRef(ref object o) + { + o = new object(); + } + } + public enum ColorsInt : int { Red = 1 diff --git a/src/mono/System.Private.CoreLib/src/System/RuntimeType.Mono.cs b/src/mono/System.Private.CoreLib/src/System/RuntimeType.Mono.cs index 1bba9f6203c96..28e67798be2e7 100644 --- a/src/mono/System.Private.CoreLib/src/System/RuntimeType.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/RuntimeType.Mono.cs @@ -1787,6 +1787,11 @@ private CheckValueStatus TryConvertToType(ref object? value) return CheckValueStatus.ArgumentException; } + // Stub method to allow for shared code with CoreClr. +#pragma warning disable CA1822 + internal bool TryByRefFastPath(ref object arg, ref bool isValueType) => false; +#pragma warning restore CA1822 + // Binder uses some incompatible conversion rules. For example // int value cannot be used with decimal parameter but in other // ways it's more flexible than normal convertor, for example From c2ae6e4fbae5f2cc5ea27f68408d827d81b23536 Mon Sep 17 00:00:00 2001 From: Steve Harter Date: Sun, 24 Apr 2022 09:26:10 -0500 Subject: [PATCH 08/21] Remove temps in IL --- .../System.Private.CoreLib.sln | 42 ++++++++++++++++--- .../src/System/Reflection/InvokerEmitUtil.cs | 12 +----- 2 files changed, 38 insertions(+), 16 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/System.Private.CoreLib.sln b/src/coreclr/System.Private.CoreLib/System.Private.CoreLib.sln index 66c65e680d469..fedce21be494b 100644 --- a/src/coreclr/System.Private.CoreLib/System.Private.CoreLib.sln +++ b/src/coreclr/System.Private.CoreLib/System.Private.CoreLib.sln @@ -1,6 +1,6 @@ Microsoft Visual Studio Solution File, Format Version 12.00 -# Visual Studio Version 16 -VisualStudioVersion = 16.0.28902.138 +# Visual Studio Version 17 +VisualStudioVersion = 17.2.32317.152 MinimumVisualStudioVersion = 10.0.40219.1 Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "System.Private.CoreLib", "System.Private.CoreLib.csproj", "{3DA06C3A-2E7B-4CB7-80ED-9B12916013F9}" EndProject @@ -9,10 +9,6 @@ EndProject Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "System.Private.CoreLib.Generators", "..\..\libraries\System.Private.CoreLib\gen\System.Private.CoreLib.Generators.csproj", "{7196828B-5E00-4BC6-9A1E-492C948E41A3}" EndProject Global - GlobalSection(SharedMSBuildProjectFiles) = preSolution - ..\..\libraries\System.Private.CoreLib\src\System.Private.CoreLib.Shared.projitems*{3da06c3a-2e7b-4cb7-80ed-9b12916013f9}*SharedItemsImports = 5 - ..\..\libraries\System.Private.CoreLib\src\System.Private.CoreLib.Shared.projitems*{845c8b26-350b-4e63-bd11-2c8150444e28}*SharedItemsImports = 13 - EndGlobalSection GlobalSection(SolutionConfigurationPlatforms) = preSolution Checked|amd64 = Checked|amd64 Checked|Any CPU = Checked|Any CPU @@ -58,6 +54,36 @@ Global {3DA06C3A-2E7B-4CB7-80ED-9B12916013F9}.Release|arm64.Build.0 = Release|arm64 {3DA06C3A-2E7B-4CB7-80ED-9B12916013F9}.Release|x86.ActiveCfg = Release|x86 {3DA06C3A-2E7B-4CB7-80ED-9B12916013F9}.Release|x86.Build.0 = Release|x86 + {845C8B26-350B-4E63-BD11-2C8150444E28}.Checked|amd64.ActiveCfg = Checked|x64 + {845C8B26-350B-4E63-BD11-2C8150444E28}.Checked|amd64.Build.0 = Checked|x64 + {845C8B26-350B-4E63-BD11-2C8150444E28}.Checked|Any CPU.ActiveCfg = Checked|x64 + {845C8B26-350B-4E63-BD11-2C8150444E28}.Checked|Any CPU.Build.0 = Checked|x64 + {845C8B26-350B-4E63-BD11-2C8150444E28}.Checked|arm.ActiveCfg = Checked|arm + {845C8B26-350B-4E63-BD11-2C8150444E28}.Checked|arm.Build.0 = Checked|arm + {845C8B26-350B-4E63-BD11-2C8150444E28}.Checked|arm64.ActiveCfg = Checked|arm64 + {845C8B26-350B-4E63-BD11-2C8150444E28}.Checked|arm64.Build.0 = Checked|arm64 + {845C8B26-350B-4E63-BD11-2C8150444E28}.Checked|x86.ActiveCfg = Checked|x86 + {845C8B26-350B-4E63-BD11-2C8150444E28}.Checked|x86.Build.0 = Checked|x86 + {845C8B26-350B-4E63-BD11-2C8150444E28}.Debug|amd64.ActiveCfg = Debug|x64 + {845C8B26-350B-4E63-BD11-2C8150444E28}.Debug|amd64.Build.0 = Debug|x64 + {845C8B26-350B-4E63-BD11-2C8150444E28}.Debug|Any CPU.ActiveCfg = Debug|x64 + {845C8B26-350B-4E63-BD11-2C8150444E28}.Debug|Any CPU.Build.0 = Debug|x64 + {845C8B26-350B-4E63-BD11-2C8150444E28}.Debug|arm.ActiveCfg = Debug|arm + {845C8B26-350B-4E63-BD11-2C8150444E28}.Debug|arm.Build.0 = Debug|arm + {845C8B26-350B-4E63-BD11-2C8150444E28}.Debug|arm64.ActiveCfg = Debug|arm64 + {845C8B26-350B-4E63-BD11-2C8150444E28}.Debug|arm64.Build.0 = Debug|arm64 + {845C8B26-350B-4E63-BD11-2C8150444E28}.Debug|x86.ActiveCfg = Debug|x86 + {845C8B26-350B-4E63-BD11-2C8150444E28}.Debug|x86.Build.0 = Debug|x86 + {845C8B26-350B-4E63-BD11-2C8150444E28}.Release|amd64.ActiveCfg = Release|x64 + {845C8B26-350B-4E63-BD11-2C8150444E28}.Release|amd64.Build.0 = Release|x64 + {845C8B26-350B-4E63-BD11-2C8150444E28}.Release|Any CPU.ActiveCfg = Release|x64 + {845C8B26-350B-4E63-BD11-2C8150444E28}.Release|Any CPU.Build.0 = Release|x64 + {845C8B26-350B-4E63-BD11-2C8150444E28}.Release|arm.ActiveCfg = Release|arm + {845C8B26-350B-4E63-BD11-2C8150444E28}.Release|arm.Build.0 = Release|arm + {845C8B26-350B-4E63-BD11-2C8150444E28}.Release|arm64.ActiveCfg = Release|arm64 + {845C8B26-350B-4E63-BD11-2C8150444E28}.Release|arm64.Build.0 = Release|arm64 + {845C8B26-350B-4E63-BD11-2C8150444E28}.Release|x86.ActiveCfg = Release|x86 + {845C8B26-350B-4E63-BD11-2C8150444E28}.Release|x86.Build.0 = Release|x86 {7196828B-5E00-4BC6-9A1E-492C948E41A3}.Checked|amd64.ActiveCfg = Debug|Any CPU {7196828B-5E00-4BC6-9A1E-492C948E41A3}.Checked|amd64.Build.0 = Debug|Any CPU {7196828B-5E00-4BC6-9A1E-492C948E41A3}.Checked|Any CPU.ActiveCfg = Debug|Any CPU @@ -95,4 +121,8 @@ Global GlobalSection(ExtensibilityGlobals) = postSolution SolutionGuid = {DA05075A-7CDA-4F65-AF6A-CB5DB6CF936F} EndGlobalSection + GlobalSection(SharedMSBuildProjectFiles) = preSolution + ..\..\libraries\System.Private.CoreLib\src\System.Private.CoreLib.Shared.projitems*{3da06c3a-2e7b-4cb7-80ed-9b12916013f9}*SharedItemsImports = 5 + ..\..\libraries\System.Private.CoreLib\src\System.Private.CoreLib.Shared.projitems*{845c8b26-350b-4e63-bd11-2c8150444e28}*SharedItemsImports = 13 + EndGlobalSection EndGlobal diff --git a/src/libraries/System.Private.CoreLib/src/System/Reflection/InvokerEmitUtil.cs b/src/libraries/System.Private.CoreLib/src/System/Reflection/InvokerEmitUtil.cs index 23aacaed588f2..39f72d811d804 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Reflection/InvokerEmitUtil.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Reflection/InvokerEmitUtil.cs @@ -113,28 +113,20 @@ public static unsafe InvokeFunc CreateInvokeDelegate(MethodBase method) // Handle per-type differences. if (elementType.IsValueType) { - LocalBuilder? local_return = il.DeclareLocal(typeof(object)); il.Emit(OpCodes.Ldobj, elementType); il.Emit(OpCodes.Box, elementType); - il.Emit(OpCodes.Stloc_S, local_return); - il.Emit(OpCodes.Ldloc_S, local_return); } else if (elementType.IsPointer) { - LocalBuilder? local_return = il.DeclareLocal(NormalizePointerType(returnType)); // IntPtr.MakeByRefType() il.Emit(OpCodes.Ldind_Ref); - il.Emit(OpCodes.Stloc_S, local_return); - il.Emit(OpCodes.Ldloc_S, local_return); + il.Emit(OpCodes.Conv_U); il.Emit(OpCodes.Ldtoken, elementType); il.Emit(OpCodes.Call, Methods.Type_GetTypeFromHandle()); il.Emit(OpCodes.Call, Methods.Pointer_Box()); } else { - LocalBuilder? local_return = il.DeclareLocal(elementType); - il.Emit(OpCodes.Ldind_Ref); - il.Emit(OpCodes.Stloc_S, local_return); - il.Emit(OpCodes.Ldloc_S, local_return); + il.Emit(OpCodes.Ldobj, elementType); } } } From 97dc203fdbe287f13d1ecfc2ebfab4bac644afdd Mon Sep 17 00:00:00 2001 From: Steve Harter Date: Sun, 24 Apr 2022 17:36:59 -0500 Subject: [PATCH 09/21] Remove hard check for Type.GetType() --- src/coreclr/vm/appdomain.cpp | 13 +++++++++---- .../src/System/Reflection/MethodInvoker.cs | 6 ++---- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/src/coreclr/vm/appdomain.cpp b/src/coreclr/vm/appdomain.cpp index 7c6f55a242a8c..e43250e583f73 100644 --- a/src/coreclr/vm/appdomain.cpp +++ b/src/coreclr/vm/appdomain.cpp @@ -1580,6 +1580,15 @@ bool SystemDomain::IsReflectionInvocationMethod(MethodDesc* pMeth) if (CoreLibBinder::GetExistingClass(reflectionInvocationTypes[i]) == pCaller) return true; } + + // Check for dynamically generate Invoke methods. + if (pMeth->IsDynamicMethod()) + { + if (strncmp(pMeth->GetName(), "InvokeStub_", 11) == 0) + { + return true; + } + } } return false; @@ -1744,8 +1753,6 @@ StackWalkAction SystemDomain::CallersMethodCallbackWithStackMark(CrawlFrame* pCf Frame* frame = pCf->GetFrame(); _ASSERTE(pCf->IsFrameless() || frame); - - // Skipping reflection frames. We don't need to be quite as exhaustive here // as the security or reflection stack walking code since we know this logic // is only invoked for selected methods in CoreLib itself. So we're @@ -1819,10 +1826,8 @@ StackWalkAction SystemDomain::CallersMethodCallback(CrawlFrame* pCf, VOID* data) pCaller->skip--; return SWA_CONTINUE; } - } - void AppDomain::Create() { STANDARD_VM_CONTRACT; diff --git a/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodInvoker.cs b/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodInvoker.cs index ed1fe961446ec..a6afada9b047c 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodInvoker.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodInvoker.cs @@ -32,8 +32,7 @@ public MethodInvoker(RuntimeMethodInfo methodInfo) #if true // TEMP HACK FOR FORCING IL ON FIRST TIME: - if (RuntimeFeature.IsDynamicCodeCompiled && - _method.DeclaringType != typeof(Type)) // Avoid stack crawl issue with Type.GetType(). + if (RuntimeFeature.IsDynamicCodeCompiled) { _emitInvoke = InvokerEmitUtil.CreateInvokeDelegate(_method); } @@ -42,8 +41,7 @@ public MethodInvoker(RuntimeMethodInfo methodInfo) } else { - if (RuntimeFeature.IsDynamicCodeCompiled && - _method.DeclaringType != typeof(Type)) // Avoid stack crawl issue with Type.GetType(). + if (RuntimeFeature.IsDynamicCodeCompiled) { _emitInvoke = InvokerEmitUtil.CreateInvokeDelegate(_method); } From 69d467d447777c5166bdb088a1df82797584290a Mon Sep 17 00:00:00 2001 From: Steve Harter Date: Mon, 25 Apr 2022 16:29:31 -0500 Subject: [PATCH 10/21] Active issue some tests; misc other --- .../System/Reflection/ConstructorInvoker.cs | 64 ++++++++----------- .../src/System/Reflection/InvokerEmitUtil.cs | 7 +- .../src/System/Reflection/MethodInvoker.cs | 61 +++++++----------- .../tests/BinaryFormatterTests.cs | 15 ++++- .../BinderTracingTest.EventHandlers.cs | 4 +- .../BinderTracingTest.ResolutionFlow.cs | 4 +- .../binding/tracing/BinderTracingTest.cs | 13 +++- 7 files changed, 84 insertions(+), 84 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Reflection/ConstructorInvoker.cs b/src/libraries/System.Private.CoreLib/src/System/Reflection/ConstructorInvoker.cs index 7fe23875c22dc..13875a76ef383 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Reflection/ConstructorInvoker.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Reflection/ConstructorInvoker.cs @@ -1,6 +1,8 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +#define USE_EMIT_INVOKE // Temporary for testing + using System.Diagnostics; using System.Runtime.CompilerServices; @@ -23,25 +25,22 @@ public ConstructorInvoker(RuntimeConstructorInfo constructorInfo) [DebuggerHidden] public unsafe object? InvokeUnsafe(object? obj, IntPtr* args, Span argsForTemporaryMonoSupport, BindingFlags invokeAttr) { +#if USE_NATIVE_INVOKE + _strategyDetermined = true; // always use the native invoke. +#elif USE_EMIT_INVOKE + _invoked = true; // use the emit invoke on all invokes (assuming it is compatible) +#endif if (!_strategyDetermined) { if (!_invoked) { // The first time, ignoring race conditions, use the slow path. _invoked = true; - -#if true - // TEMP HACK FOR FORCING IL ON FIRST TIME: - if (RuntimeFeature.IsDynamicCodeCompiled) - { - _emitInvoke = InvokerEmitUtil.CreateInvokeDelegate(_method); - } - _strategyDetermined = true; -#endif } else { - if (RuntimeFeature.IsDynamicCodeCompiled) + if (RuntimeFeature.IsDynamicCodeCompiled && + _method.SupportsNewInvoke) // Remove check for SupportsNewInvoke once Mono is updated. { _emitInvoke = InvokerEmitUtil.CreateInvokeDelegate(_method); } @@ -50,45 +49,32 @@ public ConstructorInvoker(RuntimeConstructorInfo constructorInfo) } } - if (_method.SupportsNewInvoke) + // Remove check for SupportsNewInvoke once Mono is updated (Mono's InvokeNonEmitUnsafe has its own exception handling) + if (_method.SupportsNewInvoke && (invokeAttr & BindingFlags.DoNotWrapExceptions) == 0) { - if ((invokeAttr & BindingFlags.DoNotWrapExceptions) == 0) - { - try - { - // For the rare and broken scenario of calling the constructor directly through MethodBase.Invoke() - // with a non-null 'obj', we use the slow path to avoid having two emit-based delegates. - if (_emitInvoke != null && obj == null) - { - return _emitInvoke(this, obj, args); - } - else - { - return _method.InvokeNonEmitUnsafe(obj, args, argsForTemporaryMonoSupport, invokeAttr); - } - } - catch (Exception e) - { - throw new TargetInvocationException(e); - } - } - else + try { + // For the rarely used and broken scenario of calling the constructor directly through MethodBase.Invoke() + // with a non-null 'obj', we use the slow path to avoid having two emit-based delegates. if (_emitInvoke != null && obj == null) { return _emitInvoke(this, obj, args); } - else - { - return _method.InvokeNonEmitUnsafe(obj, args, argsForTemporaryMonoSupport, invokeAttr); - } + + return _method.InvokeNonEmitUnsafe(obj, args, argsForTemporaryMonoSupport, invokeAttr); + } + catch (Exception e) + { + throw new TargetInvocationException(e); } } - else + + if (_emitInvoke != null && obj == null) { - // Remove this branch once Mono has the same exception handling and managed conversion logic. - return _method.InvokeNonEmitUnsafe(obj, args, argsForTemporaryMonoSupport, invokeAttr); + return _emitInvoke(this, obj, args); } + + return _method.InvokeNonEmitUnsafe(obj, args, argsForTemporaryMonoSupport, invokeAttr); } } } diff --git a/src/libraries/System.Private.CoreLib/src/System/Reflection/InvokerEmitUtil.cs b/src/libraries/System.Private.CoreLib/src/System/Reflection/InvokerEmitUtil.cs index 39f72d811d804..b972ed406ba8b 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Reflection/InvokerEmitUtil.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Reflection/InvokerEmitUtil.cs @@ -9,6 +9,9 @@ namespace System.Reflection { internal static class InvokerEmitUtil { + // If changed, update native stack walking code that also uses this prefix to ignore reflection frames. + private const string InvokeStubPrefix = "InvokeStub_"; + // This is an instance method to avoid overhead of shuffle thunk. internal unsafe delegate object? InvokeFunc(T obj, object? target, IntPtr* arguments); @@ -22,7 +25,7 @@ public static unsafe InvokeFunc CreateInvokeDelegate(MethodBase method) Type[] delegateParameters = new Type[3] { typeof(T), typeof(object), typeof(IntPtr*) }; var dm = new DynamicMethod( - "InvokeStub_" + method.DeclaringType!.Name + "." + method.Name, + InvokeStubPrefix + method.DeclaringType!.Name + "." + method.Name, returnType: typeof(object), delegateParameters, owner: typeof(T), @@ -133,7 +136,7 @@ public static unsafe InvokeFunc CreateInvokeDelegate(MethodBase method) il.Emit(OpCodes.Ret); - // Create the delegate. + // Create the delegate; it is also compiled at this point due to skipVisibility=true. return (InvokeFunc)dm.CreateDelegate(typeof(InvokeFunc)); } diff --git a/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodInvoker.cs b/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodInvoker.cs index a6afada9b047c..db8fda635f6e8 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodInvoker.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodInvoker.cs @@ -1,6 +1,8 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +#define USE_EMIT_INVOKE // Temporary for testing + using System.Diagnostics; using System.Runtime.CompilerServices; @@ -23,25 +25,23 @@ public MethodInvoker(RuntimeMethodInfo methodInfo) [DebuggerHidden] public unsafe object? InvokeUnsafe(object? obj, IntPtr* args, Span argsForTemporaryMonoSupport, BindingFlags invokeAttr) { +#if USE_NATIVE_INVOKE + _strategyDetermined = true; // always use the native invoke. +#elif USE_EMIT_INVOKE + _invoked = true; // use the emit invoke on all invokes (assuming it is compatible) +#endif + if (!_strategyDetermined) { if (!_invoked) { // The first time, ignoring race conditions, use the slow path. _invoked = true; - -#if true - // TEMP HACK FOR FORCING IL ON FIRST TIME: - if (RuntimeFeature.IsDynamicCodeCompiled) - { - _emitInvoke = InvokerEmitUtil.CreateInvokeDelegate(_method); - } - _strategyDetermined = true; -#endif } else { - if (RuntimeFeature.IsDynamicCodeCompiled) + if (RuntimeFeature.IsDynamicCodeCompiled && + _method.SupportsNewInvoke) // Remove check for SupportsNewInvoke once Mono is updated. { _emitInvoke = InvokerEmitUtil.CreateInvokeDelegate(_method); } @@ -50,43 +50,30 @@ public MethodInvoker(RuntimeMethodInfo methodInfo) } } - if (_method.SupportsNewInvoke) + // Remove check for SupportsNewInvoke once Mono is updated (Mono's InvokeNonEmitUnsafe has its own exception handling) + if (_method.SupportsNewInvoke && (invokeAttr & BindingFlags.DoNotWrapExceptions) == 0) { - if ((invokeAttr & BindingFlags.DoNotWrapExceptions) == 0) - { - try - { - if (_emitInvoke != null) - { - return _emitInvoke(this, obj, args); - } - else - { - return _method.InvokeNonEmitUnsafe(obj, args, argsForTemporaryMonoSupport, invokeAttr); - } - } - catch (Exception e) - { - throw new TargetInvocationException(e); - } - } - else + try { if (_emitInvoke != null) { return _emitInvoke(this, obj, args); } - else - { - return _method.InvokeNonEmitUnsafe(obj, args, argsForTemporaryMonoSupport, invokeAttr); - } + + return _method.InvokeNonEmitUnsafe(obj, args, argsForTemporaryMonoSupport, invokeAttr); + } + catch (Exception e) + { + throw new TargetInvocationException(e); } } - else + + if (_emitInvoke != null) { - // Remove this branch once Mono has the same exception handling and managed conversion logic. - return _method.InvokeNonEmitUnsafe(obj, args, argsForTemporaryMonoSupport, invokeAttr); + return _emitInvoke(this, obj, args); } + + return _method.InvokeNonEmitUnsafe(obj, args, argsForTemporaryMonoSupport, invokeAttr); } } } diff --git a/src/libraries/System.Runtime.Serialization.Formatters/tests/BinaryFormatterTests.cs b/src/libraries/System.Runtime.Serialization.Formatters/tests/BinaryFormatterTests.cs index 5c698d2ec4f46..229cd08ada4f1 100644 --- a/src/libraries/System.Runtime.Serialization.Formatters/tests/BinaryFormatterTests.cs +++ b/src/libraries/System.Runtime.Serialization.Formatters/tests/BinaryFormatterTests.cs @@ -566,10 +566,23 @@ private static void CheckObjectTypeIntegrity(ISerializable serializable) private static void SanityCheckBlob(object obj, TypeSerializableValue[] blobs) { // These types are unstable during serialization and produce different blobs. + string name = obj.GetType().FullName; if (obj is WeakReference || obj is Collections.Specialized.HybridDictionary || obj is Color || - obj.GetType().FullName == "System.Collections.SortedList+SyncSortedList") + name == "System.Collections.SortedList+SyncSortedList" || + // Due to non-deterministic field ordering the types below will fail when using IL Emit-based Invoke. + // The types above may also be failing for the same reason. + // Remove these cases once https://github.com/dotnet/runtime/issues/46272 is fixed. + name == "System.Collections.Comparer" || + name == "System.Collections.Hashtable" || + name == "System.Collections.SortedList" || + name == "System.Collections.Specialized.ListDictionary" || + name == "System.CultureAwareComparer" || + name == "System.Globalization.CompareInfo" || + name == "System.Net.Cookie" || + name == "System.Net.CookieCollection" || + name == "System.Net.CookieContainer") { return; } diff --git a/src/tests/Loader/binding/tracing/BinderTracingTest.EventHandlers.cs b/src/tests/Loader/binding/tracing/BinderTracingTest.EventHandlers.cs index 9596b84510f50..21561ec3ff151 100644 --- a/src/tests/Loader/binding/tracing/BinderTracingTest.EventHandlers.cs +++ b/src/tests/Loader/binding/tracing/BinderTracingTest.EventHandlers.cs @@ -275,7 +275,9 @@ public static BindOperation AssemblyLoadFromResolveHandler_LoadDependency() }; } - [BinderTest(isolate: true, additionalLoadsToTrack: new string[] { "AssemblyToLoadDependency" })] + [BinderTest(isolate: true, + additionalLoadsToTrack: new string[] { "AssemblyToLoadDependency" }, + activeIssue: "https://github.com/dotnet/runtime/issues/68521")] // Emit-based Invoke causes an extra load. public static BindOperation AssemblyLoadFromResolveHandler_MissingDependency() { string appPath = Path.GetDirectoryName(Assembly.GetExecutingAssembly().Location); diff --git a/src/tests/Loader/binding/tracing/BinderTracingTest.ResolutionFlow.cs b/src/tests/Loader/binding/tracing/BinderTracingTest.ResolutionFlow.cs index 780162821f508..63376afa07263 100644 --- a/src/tests/Loader/binding/tracing/BinderTracingTest.ResolutionFlow.cs +++ b/src/tests/Loader/binding/tracing/BinderTracingTest.ResolutionFlow.cs @@ -180,7 +180,9 @@ public static BindOperation ApplicationAssemblies_IncompatibleVersion() // ResolutionAttempted : ApplicationAssemblies (DefaultALC) [MismatchedAssemblyName] // ResolutionAttempted : AssemblyLoadContextResolvingEvent (DefaultALC) [AssemblyNotFound] // ResolutionAttempted : AppDomainAssemblyResolveEvent (DefaultALC) [AssemblyNotFound] - [BinderTest(isolate: true, additionalLoadsToTrack: new string[] { DependentAssemblyName + "_Copy" } )] + [BinderTest(isolate: true, + additionalLoadsToTrack: new string[] { DependentAssemblyName + "_Copy" }, + activeIssue: "https://github.com/dotnet/runtime/issues/68521")] // Emit-based Invoke causes AssemblyNotFound instead of MismatchedAssemblyName. public static BindOperation ApplicationAssemblies_MismatchedAssemblyName() { var assemblyName = new AssemblyName($"{DependentAssemblyName}_Copy, Culture=neutral, PublicKeyToken=null"); diff --git a/src/tests/Loader/binding/tracing/BinderTracingTest.cs b/src/tests/Loader/binding/tracing/BinderTracingTest.cs index d3f1fd8eee3db..bbafdf07db1f1 100644 --- a/src/tests/Loader/binding/tracing/BinderTracingTest.cs +++ b/src/tests/Loader/binding/tracing/BinderTracingTest.cs @@ -18,13 +18,15 @@ namespace BinderTracingTests class BinderTestAttribute : Attribute { public bool Isolate { get; private set; } + public string ActiveIssue { get; private set; } public string TestSetup { get; private set; } public string[] AdditionalLoadsToTrack { get; private set; } - public BinderTestAttribute(bool isolate = false, string testSetup = null, string[] additionalLoadsToTrack = null) + public BinderTestAttribute(bool isolate = false, string testSetup = null, string[] additionalLoadsToTrack = null, string activeIssue = null) { Isolate = isolate; TestSetup = testSetup; AdditionalLoadsToTrack = additionalLoadsToTrack; + ActiveIssue = activeIssue; } } @@ -75,7 +77,9 @@ public static bool RunAllTests() { MethodInfo[] methods = typeof(BinderTracingTest) .GetMethods(BindingFlags.Public | BindingFlags.Static) - .Where(m => m.GetCustomAttribute() != null && m.ReturnType == typeof(BindOperation)) + .Where(m => m.GetCustomAttribute() != null && + m.ReturnType == typeof(BindOperation) && + m.GetCustomAttribute().ActiveIssue == null) .ToArray(); foreach (var method in methods) @@ -110,7 +114,10 @@ public static int Main(string[] args) // Run specific test - first argument should be the test method name MethodInfo method = typeof(BinderTracingTest) .GetMethod(args[0], BindingFlags.Public | BindingFlags.Static); - Assert.True(method != null && method.GetCustomAttribute() != null && method.ReturnType == typeof(BindOperation)); + Assert.True(method != null && + method.GetCustomAttribute() != null && + method.ReturnType == typeof(BindOperation) && + method.GetCustomAttribute().ActiveIssue == null); success = RunSingleTest(method); } } From 259c4e05ccfd2970338fb9e026ad51c6e775c7ec Mon Sep 17 00:00:00 2001 From: Steve Harter Date: Wed, 27 Apr 2022 13:30:35 -0500 Subject: [PATCH 11/21] Remove IL-only test hack; ready for review --- .../System.Private.CoreLib.sln | 42 +++---------------- .../src/System/RuntimeType.CoreCLR.cs | 2 +- src/coreclr/vm/appdomain.cpp | 4 +- .../System/Reflection/ConstructorInvoker.cs | 15 +++---- .../src/System/Reflection/MethodInvoker.cs | 16 +++---- 5 files changed, 24 insertions(+), 55 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/System.Private.CoreLib.sln b/src/coreclr/System.Private.CoreLib/System.Private.CoreLib.sln index fedce21be494b..66c65e680d469 100644 --- a/src/coreclr/System.Private.CoreLib/System.Private.CoreLib.sln +++ b/src/coreclr/System.Private.CoreLib/System.Private.CoreLib.sln @@ -1,6 +1,6 @@ Microsoft Visual Studio Solution File, Format Version 12.00 -# Visual Studio Version 17 -VisualStudioVersion = 17.2.32317.152 +# Visual Studio Version 16 +VisualStudioVersion = 16.0.28902.138 MinimumVisualStudioVersion = 10.0.40219.1 Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "System.Private.CoreLib", "System.Private.CoreLib.csproj", "{3DA06C3A-2E7B-4CB7-80ED-9B12916013F9}" EndProject @@ -9,6 +9,10 @@ EndProject Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "System.Private.CoreLib.Generators", "..\..\libraries\System.Private.CoreLib\gen\System.Private.CoreLib.Generators.csproj", "{7196828B-5E00-4BC6-9A1E-492C948E41A3}" EndProject Global + GlobalSection(SharedMSBuildProjectFiles) = preSolution + ..\..\libraries\System.Private.CoreLib\src\System.Private.CoreLib.Shared.projitems*{3da06c3a-2e7b-4cb7-80ed-9b12916013f9}*SharedItemsImports = 5 + ..\..\libraries\System.Private.CoreLib\src\System.Private.CoreLib.Shared.projitems*{845c8b26-350b-4e63-bd11-2c8150444e28}*SharedItemsImports = 13 + EndGlobalSection GlobalSection(SolutionConfigurationPlatforms) = preSolution Checked|amd64 = Checked|amd64 Checked|Any CPU = Checked|Any CPU @@ -54,36 +58,6 @@ Global {3DA06C3A-2E7B-4CB7-80ED-9B12916013F9}.Release|arm64.Build.0 = Release|arm64 {3DA06C3A-2E7B-4CB7-80ED-9B12916013F9}.Release|x86.ActiveCfg = Release|x86 {3DA06C3A-2E7B-4CB7-80ED-9B12916013F9}.Release|x86.Build.0 = Release|x86 - {845C8B26-350B-4E63-BD11-2C8150444E28}.Checked|amd64.ActiveCfg = Checked|x64 - {845C8B26-350B-4E63-BD11-2C8150444E28}.Checked|amd64.Build.0 = Checked|x64 - {845C8B26-350B-4E63-BD11-2C8150444E28}.Checked|Any CPU.ActiveCfg = Checked|x64 - {845C8B26-350B-4E63-BD11-2C8150444E28}.Checked|Any CPU.Build.0 = Checked|x64 - {845C8B26-350B-4E63-BD11-2C8150444E28}.Checked|arm.ActiveCfg = Checked|arm - {845C8B26-350B-4E63-BD11-2C8150444E28}.Checked|arm.Build.0 = Checked|arm - {845C8B26-350B-4E63-BD11-2C8150444E28}.Checked|arm64.ActiveCfg = Checked|arm64 - {845C8B26-350B-4E63-BD11-2C8150444E28}.Checked|arm64.Build.0 = Checked|arm64 - {845C8B26-350B-4E63-BD11-2C8150444E28}.Checked|x86.ActiveCfg = Checked|x86 - {845C8B26-350B-4E63-BD11-2C8150444E28}.Checked|x86.Build.0 = Checked|x86 - {845C8B26-350B-4E63-BD11-2C8150444E28}.Debug|amd64.ActiveCfg = Debug|x64 - {845C8B26-350B-4E63-BD11-2C8150444E28}.Debug|amd64.Build.0 = Debug|x64 - {845C8B26-350B-4E63-BD11-2C8150444E28}.Debug|Any CPU.ActiveCfg = Debug|x64 - {845C8B26-350B-4E63-BD11-2C8150444E28}.Debug|Any CPU.Build.0 = Debug|x64 - {845C8B26-350B-4E63-BD11-2C8150444E28}.Debug|arm.ActiveCfg = Debug|arm - {845C8B26-350B-4E63-BD11-2C8150444E28}.Debug|arm.Build.0 = Debug|arm - {845C8B26-350B-4E63-BD11-2C8150444E28}.Debug|arm64.ActiveCfg = Debug|arm64 - {845C8B26-350B-4E63-BD11-2C8150444E28}.Debug|arm64.Build.0 = Debug|arm64 - {845C8B26-350B-4E63-BD11-2C8150444E28}.Debug|x86.ActiveCfg = Debug|x86 - {845C8B26-350B-4E63-BD11-2C8150444E28}.Debug|x86.Build.0 = Debug|x86 - {845C8B26-350B-4E63-BD11-2C8150444E28}.Release|amd64.ActiveCfg = Release|x64 - {845C8B26-350B-4E63-BD11-2C8150444E28}.Release|amd64.Build.0 = Release|x64 - {845C8B26-350B-4E63-BD11-2C8150444E28}.Release|Any CPU.ActiveCfg = Release|x64 - {845C8B26-350B-4E63-BD11-2C8150444E28}.Release|Any CPU.Build.0 = Release|x64 - {845C8B26-350B-4E63-BD11-2C8150444E28}.Release|arm.ActiveCfg = Release|arm - {845C8B26-350B-4E63-BD11-2C8150444E28}.Release|arm.Build.0 = Release|arm - {845C8B26-350B-4E63-BD11-2C8150444E28}.Release|arm64.ActiveCfg = Release|arm64 - {845C8B26-350B-4E63-BD11-2C8150444E28}.Release|arm64.Build.0 = Release|arm64 - {845C8B26-350B-4E63-BD11-2C8150444E28}.Release|x86.ActiveCfg = Release|x86 - {845C8B26-350B-4E63-BD11-2C8150444E28}.Release|x86.Build.0 = Release|x86 {7196828B-5E00-4BC6-9A1E-492C948E41A3}.Checked|amd64.ActiveCfg = Debug|Any CPU {7196828B-5E00-4BC6-9A1E-492C948E41A3}.Checked|amd64.Build.0 = Debug|Any CPU {7196828B-5E00-4BC6-9A1E-492C948E41A3}.Checked|Any CPU.ActiveCfg = Debug|Any CPU @@ -121,8 +95,4 @@ Global GlobalSection(ExtensibilityGlobals) = postSolution SolutionGuid = {DA05075A-7CDA-4F65-AF6A-CB5DB6CF936F} EndGlobalSection - GlobalSection(SharedMSBuildProjectFiles) = preSolution - ..\..\libraries\System.Private.CoreLib\src\System.Private.CoreLib.Shared.projitems*{3da06c3a-2e7b-4cb7-80ed-9b12916013f9}*SharedItemsImports = 5 - ..\..\libraries\System.Private.CoreLib\src\System.Private.CoreLib.Shared.projitems*{845c8b26-350b-4e63-bd11-2c8150444e28}*SharedItemsImports = 13 - EndGlobalSection EndGlobal diff --git a/src/coreclr/System.Private.CoreLib/src/System/RuntimeType.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/RuntimeType.CoreCLR.cs index 47d119e95b642..a6080078df9e6 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/RuntimeType.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/RuntimeType.CoreCLR.cs @@ -3520,7 +3520,7 @@ internal void VerifyValueType(object? value) /// /// Verify and optionally convert the value for special cases. /// - /// True if should be considered a value type, False otherwise + /// True if is a value type, False otherwise internal bool CheckValue( ref object? value, ref bool copyBack, diff --git a/src/coreclr/vm/appdomain.cpp b/src/coreclr/vm/appdomain.cpp index e43250e583f73..bebf8e9d6973a 100644 --- a/src/coreclr/vm/appdomain.cpp +++ b/src/coreclr/vm/appdomain.cpp @@ -1581,13 +1581,11 @@ bool SystemDomain::IsReflectionInvocationMethod(MethodDesc* pMeth) return true; } - // Check for dynamically generate Invoke methods. + // Check for dynamically generated Invoke methods. if (pMeth->IsDynamicMethod()) { if (strncmp(pMeth->GetName(), "InvokeStub_", 11) == 0) - { return true; - } } } diff --git a/src/libraries/System.Private.CoreLib/src/System/Reflection/ConstructorInvoker.cs b/src/libraries/System.Private.CoreLib/src/System/Reflection/ConstructorInvoker.cs index 13875a76ef383..e953e1600a4c9 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Reflection/ConstructorInvoker.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Reflection/ConstructorInvoker.cs @@ -1,8 +1,6 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -#define USE_EMIT_INVOKE // Temporary for testing - using System.Diagnostics; using System.Runtime.CompilerServices; @@ -19,17 +17,20 @@ internal sealed partial class ConstructorInvoker public ConstructorInvoker(RuntimeConstructorInfo constructorInfo) { _method = constructorInfo; + +#if USE_NATIVE_INVOKE + // always use the native invoke. + _strategyDetermined = true; +#elif USE_EMIT_INVOKE + // always use emit invoke (if it is compatible). + _invoked = true; +#endif } [DebuggerStepThrough] [DebuggerHidden] public unsafe object? InvokeUnsafe(object? obj, IntPtr* args, Span argsForTemporaryMonoSupport, BindingFlags invokeAttr) { -#if USE_NATIVE_INVOKE - _strategyDetermined = true; // always use the native invoke. -#elif USE_EMIT_INVOKE - _invoked = true; // use the emit invoke on all invokes (assuming it is compatible) -#endif if (!_strategyDetermined) { if (!_invoked) diff --git a/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodInvoker.cs b/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodInvoker.cs index db8fda635f6e8..9d1f9590d6083 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodInvoker.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodInvoker.cs @@ -1,8 +1,6 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -#define USE_EMIT_INVOKE // Temporary for testing - using System.Diagnostics; using System.Runtime.CompilerServices; @@ -19,18 +17,20 @@ internal sealed partial class MethodInvoker public MethodInvoker(RuntimeMethodInfo methodInfo) { _method = methodInfo; + +#if USE_NATIVE_INVOKE + // always use the native invoke. + _strategyDetermined = true; +#elif USE_EMIT_INVOKE + // always use emit invoke (if it is compatible). + _invoked = true; +#endif } [DebuggerStepThrough] [DebuggerHidden] public unsafe object? InvokeUnsafe(object? obj, IntPtr* args, Span argsForTemporaryMonoSupport, BindingFlags invokeAttr) { -#if USE_NATIVE_INVOKE - _strategyDetermined = true; // always use the native invoke. -#elif USE_EMIT_INVOKE - _invoked = true; // use the emit invoke on all invokes (assuming it is compatible) -#endif - if (!_strategyDetermined) { if (!_invoked) From 9dab8f7501b11cdfb6d391c67e30c6f99ccc40ac Mon Sep 17 00:00:00 2001 From: Steve Harter Date: Thu, 28 Apr 2022 15:28:27 -0500 Subject: [PATCH 12/21] Add emit to DynamicMethod; other misc feedback --- .../System.Private.CoreLib.csproj | 5 +- .../Reflection/ConstructorInvoker.CoreCLR.cs | 16 ++++ .../System/Reflection/Emit/DynamicMethod.cs | 14 +-- .../Reflection/Emit/DynamicMethodInvoker.cs | 36 -------- .../Reflection/MethodInvoker.CoreCLR.cs | 18 ++++ .../RuntimeConstructorInfo.CoreCLR.cs | 10 --- .../Reflection/RuntimeMethodInfo.CoreCLR.cs | 19 ++-- .../src/System/RuntimeType.CoreCLR.cs | 11 ++- src/coreclr/vm/appdomain.cpp | 3 +- src/coreclr/vm/corelib.h | 1 - .../System/Reflection/ConstructorInvoker.cs | 27 +++--- .../src/System/Reflection/InvokerEmitUtil.cs | 2 +- .../src/System/Reflection/MethodInvoker.cs | 34 ++++--- .../Reflection/RuntimeConstructorInfo.cs | 28 ++++-- .../System/Reflection/RuntimeMethodInfo.cs | 14 ++- .../System.Private.CoreLib.csproj | 4 +- .../Reflection/ConstructorInvoker.Mono.cs | 46 ++++++++++ .../System/Reflection/MethodInvoker.Mono.cs | 53 +++++++++++ .../Reflection/RuntimeMethodInfo.Mono.cs | 88 ------------------- .../src/System/RuntimeType.Mono.cs | 3 +- 20 files changed, 227 insertions(+), 205 deletions(-) create mode 100644 src/coreclr/System.Private.CoreLib/src/System/Reflection/ConstructorInvoker.CoreCLR.cs delete mode 100644 src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/DynamicMethodInvoker.cs create mode 100644 src/coreclr/System.Private.CoreLib/src/System/Reflection/MethodInvoker.CoreCLR.cs create mode 100644 src/mono/System.Private.CoreLib/src/System/Reflection/ConstructorInvoker.Mono.cs create mode 100644 src/mono/System.Private.CoreLib/src/System/Reflection/MethodInvoker.Mono.cs diff --git a/src/coreclr/System.Private.CoreLib/System.Private.CoreLib.csproj b/src/coreclr/System.Private.CoreLib/System.Private.CoreLib.csproj index 36611ae871642..28ad43af18b66 100644 --- a/src/coreclr/System.Private.CoreLib/System.Private.CoreLib.csproj +++ b/src/coreclr/System.Private.CoreLib/System.Private.CoreLib.csproj @@ -1,4 +1,4 @@ - + false @@ -155,12 +155,12 @@ + - @@ -187,6 +187,7 @@ + diff --git a/src/coreclr/System.Private.CoreLib/src/System/Reflection/ConstructorInvoker.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/Reflection/ConstructorInvoker.CoreCLR.cs new file mode 100644 index 0000000000000..b50f7ccc27e22 --- /dev/null +++ b/src/coreclr/System.Private.CoreLib/src/System/Reflection/ConstructorInvoker.CoreCLR.cs @@ -0,0 +1,16 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Runtime.CompilerServices; + +namespace System.Reflection +{ + internal partial class ConstructorInvoker + { + [MethodImpl(MethodImplOptions.AggressiveInlining)] + private unsafe object? InvokeNonEmitUnsafe(object? obj, IntPtr* arguments) + { + return RuntimeMethodHandle.InvokeMethod(obj, (void**)arguments, _method.Signature, isConstructor: obj is null)!; + } + } +} diff --git a/src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/DynamicMethod.cs b/src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/DynamicMethod.cs index aab01d992f1e8..b655fbae400af 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/DynamicMethod.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/DynamicMethod.cs @@ -23,7 +23,7 @@ public sealed class DynamicMethod : MethodInfo private RuntimeModule m_module = null!; internal bool m_skipVisibility; internal RuntimeType? m_typeOwner; // can be null - private DynamicMethodInvoker? _invoker; + private MethodInvoker? _invoker; private Signature? _signature; // We want the creator of the DynamicMethod to control who has access to the @@ -434,13 +434,12 @@ internal RuntimeMethodHandle GetMethodDescriptor() public override bool IsSecurityTransparent => false; - private DynamicMethodInvoker Invoker + private MethodInvoker Invoker { [MethodImpl(MethodImplOptions.AggressiveInlining)] get { - _invoker ??= new DynamicMethodInvoker(this); - + _invoker ??= new MethodInvoker(this, Signature); return _invoker; } } @@ -591,13 +590,6 @@ Signature LazyCreateSignature() return retValue; } - [DebuggerHidden] - [DebuggerStepThrough] - internal unsafe object? InvokeNonEmitUnsafe(object? obj, IntPtr* arguments) - { - return RuntimeMethodHandle.InvokeMethod(obj, (void**)arguments, Signature, isConstructor: false); - } - public override object[] GetCustomAttributes(Type attributeType, bool inherit) { return m_dynMethod.GetCustomAttributes(attributeType, inherit); diff --git a/src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/DynamicMethodInvoker.cs b/src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/DynamicMethodInvoker.cs deleted file mode 100644 index 99715b7fb4074..0000000000000 --- a/src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/DynamicMethodInvoker.cs +++ /dev/null @@ -1,36 +0,0 @@ -// Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license. - -namespace System.Reflection.Emit -{ - internal sealed partial class DynamicMethodInvoker - { - private readonly DynamicMethod _dynamicMethod; - - public DynamicMethodInvoker(DynamicMethod dynamicMethod) - { - _dynamicMethod = dynamicMethod; - } - - public unsafe object? InvokeUnsafe(object? obj, IntPtr* args, BindingFlags invokeAttr) - { - // Always use the slow path; the Emit-based fast path can be added but in general dynamic - // methods are invoked through a direct delegate, not through Invoke(). - if ((invokeAttr & BindingFlags.DoNotWrapExceptions) == 0) - { - try - { - return _dynamicMethod.InvokeNonEmitUnsafe(obj, args); - } - catch (Exception e) - { - throw new TargetInvocationException(e); - } - } - else - { - return _dynamicMethod.InvokeNonEmitUnsafe(obj, args); - } - } - } -} diff --git a/src/coreclr/System.Private.CoreLib/src/System/Reflection/MethodInvoker.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/Reflection/MethodInvoker.CoreCLR.cs new file mode 100644 index 0000000000000..e38892b492b55 --- /dev/null +++ b/src/coreclr/System.Private.CoreLib/src/System/Reflection/MethodInvoker.CoreCLR.cs @@ -0,0 +1,18 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Diagnostics; +using System.Runtime.CompilerServices; + +namespace System.Reflection +{ + internal partial class MethodInvoker + { + [MethodImpl(MethodImplOptions.AggressiveInlining)] + private unsafe object? InvokeNonEmitUnsafe(object? obj, IntPtr* arguments) + { + Debug.Assert(_signature != null); + return RuntimeMethodHandle.InvokeMethod(obj, (void**)arguments, _signature, isConstructor: false); + } + } +} diff --git a/src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeConstructorInfo.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeConstructorInfo.CoreCLR.cs index eb211018d1d1e..51ee0061420e0 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeConstructorInfo.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeConstructorInfo.CoreCLR.cs @@ -95,16 +95,6 @@ Signature LazyCreateSignature() internal BindingFlags BindingFlags => m_bindingFlags; -#pragma warning disable CA1822 // Mark members as static - internal bool SupportsNewInvoke => true; -#pragma warning restore CA1822 // Mark members as static - - [DebuggerStepThrough] - [DebuggerHidden] - internal unsafe object InvokeNonEmitUnsafe(object? obj, IntPtr* args, Span argsForTemporaryMonoSupport, BindingFlags invokeAttr) - { - return RuntimeMethodHandle.InvokeMethod(obj, (void**)args, Signature, isConstructor: obj is null)!; - } #endregion #region Object Overrides diff --git a/src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeMethodInfo.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeMethodInfo.CoreCLR.cs index 00e035d4ebbd5..e779376940359 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeMethodInfo.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeMethodInfo.CoreCLR.cs @@ -48,7 +48,7 @@ private MethodInvoker Invoker [MethodImpl(MethodImplOptions.AggressiveInlining)] get { - m_invoker ??= new MethodInvoker(this); + m_invoker ??= new MethodInvoker(this, Signature); return m_invoker; } } @@ -366,23 +366,16 @@ public override MethodImplAttributes GetMethodImplementationFlags() culture, invokeAttr); - retValue = Invoker.InvokeUnsafe(obj, pByRefStorage, copyOfParameters, invokeAttr); +#if MONO // Temporary until Mono is updated. + retValue = Invoker.InvokeUnsafe(obj, copyOfParameters, invokeAttr); +#else + retValue = Invoker.InvokeUnsafe(obj, pByRefStorage, invokeAttr); +#endif } return retValue; } -#pragma warning disable CA1822 // Mark members as static - internal bool SupportsNewInvoke => true; -#pragma warning restore CA1822 // Mark members as static - - [DebuggerHidden] - [DebuggerStepThrough] - internal unsafe object? InvokeNonEmitUnsafe(object? obj, IntPtr* arguments, Span argsForTemporaryMonoSupport, BindingFlags invokeAttr) - { - return RuntimeMethodHandle.InvokeMethod(obj, (void**)arguments, Signature, isConstructor: false); - } - #endregion #region MethodInfo Overrides diff --git a/src/coreclr/System.Private.CoreLib/src/System/RuntimeType.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/RuntimeType.CoreCLR.cs index a6080078df9e6..e7f987b7b7fe3 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/RuntimeType.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/RuntimeType.CoreCLR.cs @@ -3589,7 +3589,7 @@ internal bool CheckValue( } Debug.Fail("Error result not expected"); - return default; + return false; } private CheckValueStatus TryChangeType( @@ -3605,9 +3605,8 @@ private CheckValueStatus TryChangeType( if (sigElementType.IsInstanceOfType(value)) { - Debug.Assert(!sigElementType.IsGenericParameter); isValueType = RuntimeTypeHandle.IsValueType(sigElementType); - if (sigElementType.IsValueType) + if (isValueType) { if (sigElementType.IsNullableOfT) { @@ -3643,7 +3642,7 @@ private CheckValueStatus TryChangeType( return CheckValueStatus.Success; } - isValueType = default; + isValueType = false; return CheckValueStatus.ArgumentException; } @@ -3679,7 +3678,7 @@ private CheckValueStatus TryChangeType( if (!CanValueSpecialCast(srcType, this)) { - isValueType = copyBack = default; + isValueType = copyBack = false; return CheckValueStatus.ArgumentException; } @@ -3702,7 +3701,7 @@ private CheckValueStatus TryChangeType( return CheckValueStatus.Success; } - copyBack = isValueType = default; + copyBack = isValueType = false; return CheckValueStatus.ArgumentException; } diff --git a/src/coreclr/vm/appdomain.cpp b/src/coreclr/vm/appdomain.cpp index bebf8e9d6973a..d98b8844aba4a 100644 --- a/src/coreclr/vm/appdomain.cpp +++ b/src/coreclr/vm/appdomain.cpp @@ -1557,7 +1557,6 @@ bool SystemDomain::IsReflectionInvocationMethod(MethodDesc* pMeth) CLASS__MULTICAST_DELEGATE, CLASS__METHOD_INVOKER, CLASS__CONSTRUCTOR_INVOKER, - CLASS__DYNAMIC_METHOD_INVOKER }; static bool fInited = false; @@ -1584,7 +1583,7 @@ bool SystemDomain::IsReflectionInvocationMethod(MethodDesc* pMeth) // Check for dynamically generated Invoke methods. if (pMeth->IsDynamicMethod()) { - if (strncmp(pMeth->GetName(), "InvokeStub_", 11) == 0) + if (strncmp(pMeth->GetName(), "InvokeStub_", ARRAY_SIZE("InvokeStub_") - 1) == 0) return true; } } diff --git a/src/coreclr/vm/corelib.h b/src/coreclr/vm/corelib.h index 05dbee2e21f01..c8ec4aec7a819 100644 --- a/src/coreclr/vm/corelib.h +++ b/src/coreclr/vm/corelib.h @@ -507,7 +507,6 @@ DEFINE_CLASS(MEMBER, Reflection, MemberInfo) DEFINE_CLASS(METHOD_INVOKER, Reflection, MethodInvoker) DEFINE_CLASS(CONSTRUCTOR_INVOKER, Reflection, ConstructorInvoker) -DEFINE_CLASS(DYNAMIC_METHOD_INVOKER,ReflectionEmit, DynamicMethodInvoker) DEFINE_CLASS_U(Reflection, RuntimeMethodInfo, NoClass) DEFINE_FIELD_U(m_handle, ReflectMethodObject, m_pMD) diff --git a/src/libraries/System.Private.CoreLib/src/System/Reflection/ConstructorInvoker.cs b/src/libraries/System.Private.CoreLib/src/System/Reflection/ConstructorInvoker.cs index e953e1600a4c9..3dfa6045450d6 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Reflection/ConstructorInvoker.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Reflection/ConstructorInvoker.cs @@ -19,17 +19,26 @@ public ConstructorInvoker(RuntimeConstructorInfo constructorInfo) _method = constructorInfo; #if USE_NATIVE_INVOKE - // always use the native invoke. + // Always use the native invoke; useful for testing. _strategyDetermined = true; #elif USE_EMIT_INVOKE - // always use emit invoke (if it is compatible). + // Always use emit invoke (if IsDynamicCodeCompiled == true); useful for testing. _invoked = true; #endif + } +#if MONO // Temporary until Mono is updated. [DebuggerStepThrough] [DebuggerHidden] - public unsafe object? InvokeUnsafe(object? obj, IntPtr* args, Span argsForTemporaryMonoSupport, BindingFlags invokeAttr) + public unsafe object? InvokeUnsafe(object? obj, Span args, BindingFlags invokeAttr) + { + return InvokeNonEmitUnsafe(obj, args, invokeAttr); + } +#else + [DebuggerStepThrough] + [DebuggerHidden] + public unsafe object? InvokeUnsafe(object? obj, IntPtr* args, BindingFlags invokeAttr) { if (!_strategyDetermined) { @@ -40,18 +49,15 @@ public ConstructorInvoker(RuntimeConstructorInfo constructorInfo) } else { - if (RuntimeFeature.IsDynamicCodeCompiled && - _method.SupportsNewInvoke) // Remove check for SupportsNewInvoke once Mono is updated. + if (RuntimeFeature.IsDynamicCodeCompiled) { _emitInvoke = InvokerEmitUtil.CreateInvokeDelegate(_method); } - _strategyDetermined = true; } } - // Remove check for SupportsNewInvoke once Mono is updated (Mono's InvokeNonEmitUnsafe has its own exception handling) - if (_method.SupportsNewInvoke && (invokeAttr & BindingFlags.DoNotWrapExceptions) == 0) + if ((invokeAttr & BindingFlags.DoNotWrapExceptions) == 0) { try { @@ -62,7 +68,7 @@ public ConstructorInvoker(RuntimeConstructorInfo constructorInfo) return _emitInvoke(this, obj, args); } - return _method.InvokeNonEmitUnsafe(obj, args, argsForTemporaryMonoSupport, invokeAttr); + return InvokeNonEmitUnsafe(obj, args); } catch (Exception e) { @@ -75,7 +81,8 @@ public ConstructorInvoker(RuntimeConstructorInfo constructorInfo) return _emitInvoke(this, obj, args); } - return _method.InvokeNonEmitUnsafe(obj, args, argsForTemporaryMonoSupport, invokeAttr); + return InvokeNonEmitUnsafe(obj, args); } +#endif } } diff --git a/src/libraries/System.Private.CoreLib/src/System/Reflection/InvokerEmitUtil.cs b/src/libraries/System.Private.CoreLib/src/System/Reflection/InvokerEmitUtil.cs index b972ed406ba8b..a5e061815b835 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Reflection/InvokerEmitUtil.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Reflection/InvokerEmitUtil.cs @@ -12,7 +12,7 @@ internal static class InvokerEmitUtil // If changed, update native stack walking code that also uses this prefix to ignore reflection frames. private const string InvokeStubPrefix = "InvokeStub_"; - // This is an instance method to avoid overhead of shuffle thunk. + // The 'obj' parameter allows the DynamicMethod to be treated as an instance method which is slightly faster than a static. internal unsafe delegate object? InvokeFunc(T obj, object? target, IntPtr* arguments); public static unsafe InvokeFunc CreateInvokeDelegate(MethodBase method) diff --git a/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodInvoker.cs b/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodInvoker.cs index 9d1f9590d6083..69523937bf4a8 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodInvoker.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodInvoker.cs @@ -9,27 +9,37 @@ namespace System.Reflection internal sealed partial class MethodInvoker { internal InvocationFlags _invocationFlags; - private readonly RuntimeMethodInfo _method; + private readonly MethodBase _method; + private readonly Signature? _signature; private bool _invoked; private bool _strategyDetermined; private InvokerEmitUtil.InvokeFunc? _emitInvoke; - public MethodInvoker(RuntimeMethodInfo methodInfo) + public MethodInvoker(MethodBase method, Signature? signature = null) { - _method = methodInfo; + _method = method; + _signature = signature; #if USE_NATIVE_INVOKE - // always use the native invoke. + // Always use the native invoke; useful for testing. _strategyDetermined = true; #elif USE_EMIT_INVOKE - // always use emit invoke (if it is compatible). + // Always use emit invoke (if IsDynamicCodeCompiled == true); useful for testing. _invoked = true; #endif } +#if MONO // Temporary until Mono is updated. [DebuggerStepThrough] [DebuggerHidden] - public unsafe object? InvokeUnsafe(object? obj, IntPtr* args, Span argsForTemporaryMonoSupport, BindingFlags invokeAttr) + public unsafe object? InvokeUnsafe(object? obj, Span args, BindingFlags invokeAttr) + { + return InvokeNonEmitUnsafe(obj, args, invokeAttr); + } +#else + [DebuggerStepThrough] + [DebuggerHidden] + public unsafe object? InvokeUnsafe(object? obj, IntPtr* args, BindingFlags invokeAttr) { if (!_strategyDetermined) { @@ -40,18 +50,15 @@ public MethodInvoker(RuntimeMethodInfo methodInfo) } else { - if (RuntimeFeature.IsDynamicCodeCompiled && - _method.SupportsNewInvoke) // Remove check for SupportsNewInvoke once Mono is updated. + if (RuntimeFeature.IsDynamicCodeCompiled) { _emitInvoke = InvokerEmitUtil.CreateInvokeDelegate(_method); } - _strategyDetermined = true; } } - // Remove check for SupportsNewInvoke once Mono is updated (Mono's InvokeNonEmitUnsafe has its own exception handling) - if (_method.SupportsNewInvoke && (invokeAttr & BindingFlags.DoNotWrapExceptions) == 0) + if ((invokeAttr & BindingFlags.DoNotWrapExceptions) == 0) { try { @@ -60,7 +67,7 @@ public MethodInvoker(RuntimeMethodInfo methodInfo) return _emitInvoke(this, obj, args); } - return _method.InvokeNonEmitUnsafe(obj, args, argsForTemporaryMonoSupport, invokeAttr); + return InvokeNonEmitUnsafe(obj, args); } catch (Exception e) { @@ -73,7 +80,8 @@ public MethodInvoker(RuntimeMethodInfo methodInfo) return _emitInvoke(this, obj, args); } - return _method.InvokeNonEmitUnsafe(obj, args, argsForTemporaryMonoSupport, invokeAttr); + return InvokeNonEmitUnsafe(obj, args); } +#endif } } diff --git a/src/libraries/System.Private.CoreLib/src/System/Reflection/RuntimeConstructorInfo.cs b/src/libraries/System.Private.CoreLib/src/System/Reflection/RuntimeConstructorInfo.cs index 4d0188e7745ea..c6db516298ca9 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Reflection/RuntimeConstructorInfo.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Reflection/RuntimeConstructorInfo.cs @@ -130,7 +130,7 @@ internal void ThrowNoInvokeException() { if (argCount == 0) { - Invoker.InvokeUnsafe(obj, args: default, argsForTemporaryMonoSupport: default, invokeAttr); + Invoker.InvokeUnsafe(obj, args: default, invokeAttr); } else if (argCount > MaxStackAllocArgCount) { @@ -157,7 +157,11 @@ internal void ThrowNoInvokeException() culture, invokeAttr); - Invoker.InvokeUnsafe(obj, pByRefStorage, copyOfParameters, invokeAttr); +#if MONO // Temporary until Mono is updated. + Invoker.InvokeUnsafe(obj, copyOfParameters, invokeAttr); +#else + Invoker.InvokeUnsafe(obj, pByRefStorage, invokeAttr); +#endif // Copy modified values out. This should be done only with ByRef or Type.Missing parameters. for (int i = 0; i < argCount; i++) @@ -212,7 +216,11 @@ private static unsafe void InvokeWithManyArguments( culture, invokeAttr); - ci.Invoker.InvokeUnsafe(obj, pByRefStorage, copyOfParameters, invokeAttr); +#if MONO // Temporary until Mono is updated. + ci.Invoker.InvokeUnsafe(obj, copyOfParameters, invokeAttr); +#else + ci.Invoker.InvokeUnsafe(obj, pByRefStorage, invokeAttr); +#endif } finally { @@ -254,7 +262,7 @@ public override object Invoke(BindingFlags invokeAttr, Binder? binder, object?[] { if (argCount == 0) { - retValue = Invoker.InvokeUnsafe(obj: null, args: default, argsForTemporaryMonoSupport: default, invokeAttr); + retValue = Invoker.InvokeUnsafe(obj: null, args: default, invokeAttr); } else if (argCount > MaxStackAllocArgCount) { @@ -280,7 +288,11 @@ public override object Invoke(BindingFlags invokeAttr, Binder? binder, object?[] culture, invokeAttr); - retValue = Invoker.InvokeUnsafe(obj: null, pByRefStorage, copyOfParameters, invokeAttr); +#if MONO // Temporary until Mono is updated. + retValue = Invoker.InvokeUnsafe(obj: null, copyOfParameters, invokeAttr); +#else + retValue = Invoker.InvokeUnsafe(obj: null, pByRefStorage, invokeAttr); +#endif // Copy modified values out. This should be done only with ByRef or Type.Missing parameters. for (int i = 0; i < argCount; i++) @@ -338,7 +350,11 @@ public override object Invoke(BindingFlags invokeAttr, Binder? binder, object?[] culture, invokeAttr); - retValue = ci.Invoker.InvokeUnsafe(obj: null, pByRefStorage, copyOfParameters, invokeAttr); +#if MONO // Temporary until Mono is updated. + retValue = ci.Invoker.InvokeUnsafe(obj: null, copyOfParameters, invokeAttr); +#else + retValue = ci.Invoker.InvokeUnsafe(obj: null, pByRefStorage, invokeAttr); +#endif } finally { diff --git a/src/libraries/System.Private.CoreLib/src/System/Reflection/RuntimeMethodInfo.cs b/src/libraries/System.Private.CoreLib/src/System/Reflection/RuntimeMethodInfo.cs index 70c81e8693c73..5d2032ff2f8de 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Reflection/RuntimeMethodInfo.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Reflection/RuntimeMethodInfo.cs @@ -126,7 +126,7 @@ private void ThrowNoInvokeException() { if (argCount == 0) { - retValue = Invoker.InvokeUnsafe(obj, args: default, argsForTemporaryMonoSupport: default, invokeAttr); + retValue = Invoker.InvokeUnsafe(obj, args: default, invokeAttr); } else if (argCount > MaxStackAllocArgCount) { @@ -153,7 +153,11 @@ private void ThrowNoInvokeException() culture, invokeAttr); - retValue = Invoker.InvokeUnsafe(obj, pByRefStorage, copyOfParameters, invokeAttr); +#if MONO // Temporary until Mono is updated. + retValue = Invoker.InvokeUnsafe(obj, copyOfParameters, invokeAttr); +#else + retValue = Invoker.InvokeUnsafe(obj, pByRefStorage, invokeAttr); +#endif // Copy modified values out. This should be done only with ByRef or Type.Missing parameters. for (int i = 0; i < argCount; i++) @@ -209,7 +213,11 @@ private void ThrowNoInvokeException() culture, invokeAttr); - retValue = mi.Invoker.InvokeUnsafe(obj, pByRefStorage, copyOfParameters, invokeAttr); +#if MONO // Temporary until Mono is updated. + retValue = mi.Invoker.InvokeUnsafe(obj, copyOfParameters, invokeAttr); +#else + retValue = mi.Invoker.InvokeUnsafe(obj, pByRefStorage, invokeAttr); +#endif } finally { diff --git a/src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj b/src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj index 8a16000e3f5e1..0a836329ba26f 100644 --- a/src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj +++ b/src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj @@ -1,4 +1,4 @@ - + false true @@ -196,10 +196,12 @@ + + diff --git a/src/mono/System.Private.CoreLib/src/System/Reflection/ConstructorInvoker.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Reflection/ConstructorInvoker.Mono.cs new file mode 100644 index 0000000000000..55c7d5d0a5079 --- /dev/null +++ b/src/mono/System.Private.CoreLib/src/System/Reflection/ConstructorInvoker.Mono.cs @@ -0,0 +1,46 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Runtime.CompilerServices; + +namespace System.Reflection +{ + internal partial class ConstructorInvoker + { + [MethodImpl(MethodImplOptions.AggressiveInlining)] + private unsafe object? InvokeNonEmitUnsafe(object? obj, Span args, BindingFlags invokeAttr) + { + Exception exc; + object? o; + + if ((invokeAttr & BindingFlags.DoNotWrapExceptions) == 0) + { + try + { + o = _method.InternalInvoke(obj, args, out exc); + } + catch (MethodAccessException) + { + throw; + } + catch (OverflowException) + { + throw; + } + catch (Exception e) + { + throw new TargetInvocationException(e); + } + } + else + { + o = _method.InternalInvoke(obj, args, out exc); + } + + if (exc != null) + throw exc; + + return obj == null ? o : null; + } + } +} diff --git a/src/mono/System.Private.CoreLib/src/System/Reflection/MethodInvoker.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Reflection/MethodInvoker.Mono.cs new file mode 100644 index 0000000000000..2593cb22c4976 --- /dev/null +++ b/src/mono/System.Private.CoreLib/src/System/Reflection/MethodInvoker.Mono.cs @@ -0,0 +1,53 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Runtime.CompilerServices; + +namespace System.Reflection +{ + internal partial class MethodInvoker + { + [MethodImpl(MethodImplOptions.AggressiveInlining)] + private unsafe object? InvokeNonEmitUnsafe(object? obj, Span args, BindingFlags invokeAttr) + { + Exception? exc; + object? o; + + if ((invokeAttr & BindingFlags.DoNotWrapExceptions) == 0) + { + try + { + o = ((RuntimeMethodInfo)_method).InternalInvoke(obj, args, out exc); + } + catch (Mono.NullByRefReturnException) + { + throw new NullReferenceException(); + } + catch (OverflowException) + { + throw; + } + catch (Exception e) + { + throw new TargetInvocationException(e); + } + } + else + { + try + { + o = ((RuntimeMethodInfo)_method).InternalInvoke(obj, args, out exc); + } + catch (Mono.NullByRefReturnException) + { + throw new NullReferenceException(); + } + } + + if (exc != null) + throw exc; + + return o; + } + } +} diff --git a/src/mono/System.Private.CoreLib/src/System/Reflection/RuntimeMethodInfo.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Reflection/RuntimeMethodInfo.Mono.cs index 0fdaf987822df..146739163199b 100644 --- a/src/mono/System.Private.CoreLib/src/System/Reflection/RuntimeMethodInfo.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Reflection/RuntimeMethodInfo.Mono.cs @@ -384,53 +384,6 @@ internal RuntimeType[] ArgumentTypes [MethodImplAttribute(MethodImplOptions.InternalCall)] internal extern object? InternalInvoke(object? obj, in Span parameters, out Exception? exc); -#pragma warning disable CA1822 // Mark members as static - internal bool SupportsNewInvoke => false; -#pragma warning restore CA1822 // Mark members as static - - [MethodImpl(MethodImplOptions.AggressiveInlining)] - internal unsafe object? InvokeNonEmitUnsafe(object? obj, IntPtr* byrefParameters, Span argsForTemporaryMonoSupport, BindingFlags invokeAttr) - { - Exception? exc; - object? o; - - if ((invokeAttr & BindingFlags.DoNotWrapExceptions) == 0) - { - try - { - o = InternalInvoke(obj, argsForTemporaryMonoSupport, out exc); - } - catch (Mono.NullByRefReturnException) - { - throw new NullReferenceException(); - } - catch (OverflowException) - { - throw; - } - catch (Exception e) - { - throw new TargetInvocationException(e); - } - } - else - { - try - { - o = InternalInvoke(obj, argsForTemporaryMonoSupport, out exc); - } - catch (Mono.NullByRefReturnException) - { - throw new NullReferenceException(); - } - } - - if (exc != null) - throw exc; - - return o; - } - public override RuntimeMethodHandle MethodHandle { get @@ -862,47 +815,6 @@ private static void InvokeClassConstructor() [MethodImplAttribute(MethodImplOptions.InternalCall)] internal extern object InternalInvoke(object? obj, in Span parameters, out Exception exc); -#pragma warning disable CA1822 // Mark members as static - internal bool SupportsNewInvoke => false; -#pragma warning restore CA1822 // Mark members as static - - [DebuggerHidden] - [DebuggerStepThrough] - internal unsafe object? InvokeNonEmitUnsafe(object? obj, IntPtr* byrefParameters, Span argsForTemporaryMonoSupport, BindingFlags invokeAttr) - { - Exception exc; - object? o; - - if ((invokeAttr & BindingFlags.DoNotWrapExceptions) == 0) - { - try - { - o = InternalInvoke(obj, argsForTemporaryMonoSupport, out exc); - } - catch (MethodAccessException) - { - throw; - } - catch (OverflowException) - { - throw; - } - catch (Exception e) - { - throw new TargetInvocationException(e); - } - } - else - { - o = InternalInvoke(obj, argsForTemporaryMonoSupport, out exc); - } - - if (exc != null) - throw exc; - - return obj == null ? o : null; - } - public override RuntimeMethodHandle MethodHandle { get diff --git a/src/mono/System.Private.CoreLib/src/System/RuntimeType.Mono.cs b/src/mono/System.Private.CoreLib/src/System/RuntimeType.Mono.cs index 28e67798be2e7..2781a4968c07d 100644 --- a/src/mono/System.Private.CoreLib/src/System/RuntimeType.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/RuntimeType.Mono.cs @@ -1656,7 +1656,6 @@ internal override FieldInfo GetField(FieldInfo fromNoninstanciated) return ctor.Invoker.InvokeUnsafe( obj: null, args: default, - argsForTemporaryMonoSupport: default, wrapExceptions ? BindingFlags.Default : BindingFlags.DoNotWrapExceptions); } } @@ -2052,7 +2051,7 @@ internal static object CreateInstanceForAnotherGenericParameter( unsafe { - return ctor.Invoker.InvokeUnsafe(obj: null, args: default, argsForTemporaryMonoSupport: default, BindingFlags.Default)!; + return ctor.Invoker.InvokeUnsafe(obj: null, args: default, BindingFlags.Default)!; } } From e7878ffff4cde8720a80d3b5bb1c622652e3e186 Mon Sep 17 00:00:00 2001 From: Steve Harter Date: Thu, 28 Apr 2022 16:39:43 -0500 Subject: [PATCH 13/21] Fix Mono compile issue --- .../Reflection/ConstructorInvoker.CoreCLR.cs | 2 ++ .../Reflection/MethodInvoker.CoreCLR.cs | 19 +++++++++++++-- .../System/Reflection/ConstructorInvoker.cs | 5 ++-- .../src/System/Reflection/MethodInvoker.cs | 23 ++++--------------- .../System/Reflection/MethodInvoker.Mono.cs | 13 +++++++++++ 5 files changed, 39 insertions(+), 23 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Reflection/ConstructorInvoker.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/Reflection/ConstructorInvoker.CoreCLR.cs index b50f7ccc27e22..a0d938c36afcc 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Reflection/ConstructorInvoker.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Reflection/ConstructorInvoker.CoreCLR.cs @@ -7,6 +7,8 @@ namespace System.Reflection { internal partial class ConstructorInvoker { + public InvocationFlags _invocationFlags; + [MethodImpl(MethodImplOptions.AggressiveInlining)] private unsafe object? InvokeNonEmitUnsafe(object? obj, IntPtr* arguments) { diff --git a/src/coreclr/System.Private.CoreLib/src/System/Reflection/MethodInvoker.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/Reflection/MethodInvoker.CoreCLR.cs index e38892b492b55..7bd5147889c90 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Reflection/MethodInvoker.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Reflection/MethodInvoker.CoreCLR.cs @@ -1,17 +1,32 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -using System.Diagnostics; using System.Runtime.CompilerServices; namespace System.Reflection { internal partial class MethodInvoker { + private readonly Signature _signature; + internal InvocationFlags _invocationFlags; + + public MethodInvoker(MethodBase method, Signature signature) + { + _method = method; + _signature = signature; + +#if USE_NATIVE_INVOKE + // Always use the native invoke; useful for testing. + _strategyDetermined = true; +#elif USE_EMIT_INVOKE + // Always use emit invoke (if IsDynamicCodeCompiled == true); useful for testing. + _invoked = true; +#endif + } + [MethodImpl(MethodImplOptions.AggressiveInlining)] private unsafe object? InvokeNonEmitUnsafe(object? obj, IntPtr* arguments) { - Debug.Assert(_signature != null); return RuntimeMethodHandle.InvokeMethod(obj, (void**)arguments, _signature, isConstructor: false); } } diff --git a/src/libraries/System.Private.CoreLib/src/System/Reflection/ConstructorInvoker.cs b/src/libraries/System.Private.CoreLib/src/System/Reflection/ConstructorInvoker.cs index 3dfa6045450d6..e441196a0bdcc 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Reflection/ConstructorInvoker.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Reflection/ConstructorInvoker.cs @@ -9,10 +9,12 @@ namespace System.Reflection internal sealed partial class ConstructorInvoker { private readonly RuntimeConstructorInfo _method; - public InvocationFlags _invocationFlags; + +#if !MONO // Temporary until Mono is updated. private bool _invoked; private bool _strategyDetermined; private InvokerEmitUtil.InvokeFunc? _emitInvoke; +#endif public ConstructorInvoker(RuntimeConstructorInfo constructorInfo) { @@ -25,7 +27,6 @@ public ConstructorInvoker(RuntimeConstructorInfo constructorInfo) // Always use emit invoke (if IsDynamicCodeCompiled == true); useful for testing. _invoked = true; #endif - } #if MONO // Temporary until Mono is updated. diff --git a/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodInvoker.cs b/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodInvoker.cs index 69523937bf4a8..f21bdd8a099de 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodInvoker.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodInvoker.cs @@ -8,26 +8,7 @@ namespace System.Reflection { internal sealed partial class MethodInvoker { - internal InvocationFlags _invocationFlags; private readonly MethodBase _method; - private readonly Signature? _signature; - private bool _invoked; - private bool _strategyDetermined; - private InvokerEmitUtil.InvokeFunc? _emitInvoke; - - public MethodInvoker(MethodBase method, Signature? signature = null) - { - _method = method; - _signature = signature; - -#if USE_NATIVE_INVOKE - // Always use the native invoke; useful for testing. - _strategyDetermined = true; -#elif USE_EMIT_INVOKE - // Always use emit invoke (if IsDynamicCodeCompiled == true); useful for testing. - _invoked = true; -#endif - } #if MONO // Temporary until Mono is updated. [DebuggerStepThrough] @@ -37,6 +18,10 @@ public MethodInvoker(MethodBase method, Signature? signature = null) return InvokeNonEmitUnsafe(obj, args, invokeAttr); } #else + private bool _invoked; + private bool _strategyDetermined; + private InvokerEmitUtil.InvokeFunc? _emitInvoke; + [DebuggerStepThrough] [DebuggerHidden] public unsafe object? InvokeUnsafe(object? obj, IntPtr* args, BindingFlags invokeAttr) diff --git a/src/mono/System.Private.CoreLib/src/System/Reflection/MethodInvoker.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Reflection/MethodInvoker.Mono.cs index 2593cb22c4976..1bfcceec8eb82 100644 --- a/src/mono/System.Private.CoreLib/src/System/Reflection/MethodInvoker.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Reflection/MethodInvoker.Mono.cs @@ -7,6 +7,19 @@ namespace System.Reflection { internal partial class MethodInvoker { + public MethodInvoker(MethodBase method) + { + _method = method; + +#if USE_NATIVE_INVOKE + // Always use the native invoke; useful for testing. + _strategyDetermined = true; +#elif USE_EMIT_INVOKE + // Always use emit invoke (if IsDynamicCodeCompiled == true); useful for testing. + _invoked = true; +#endif + } + [MethodImpl(MethodImplOptions.AggressiveInlining)] private unsafe object? InvokeNonEmitUnsafe(object? obj, Span args, BindingFlags invokeAttr) { From 13f913ae764802b10b29c93e7939485df6fa5161 Mon Sep 17 00:00:00 2001 From: Steve Harter Date: Fri, 29 Apr 2022 14:16:56 -0500 Subject: [PATCH 14/21] Fix shuffle thunk; other misc non-functional --- .../src/System/RuntimeType.CoreCLR.cs | 15 +----------- .../System/Reflection/ConstructorInvoker.cs | 24 ++++++++++++------- .../src/System/Reflection/InvokerEmitUtil.cs | 15 ++++++------ .../src/System/Reflection/MethodBase.cs | 10 +++++--- .../src/System/Reflection/MethodInvoker.cs | 24 ++++++++++++------- .../src/System/RuntimeType.Mono.cs | 8 ------- 6 files changed, 45 insertions(+), 51 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/RuntimeType.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/RuntimeType.CoreCLR.cs index e7f987b7b7fe3..9beb95dabd083 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/RuntimeType.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/RuntimeType.CoreCLR.cs @@ -3504,19 +3504,6 @@ private enum CheckValueStatus NotSupported_ByRefLike } -#if DEBUG - internal void VerifyValueType(object? value) - { - Debug.Assert(value != null); - Debug.Assert( - value.GetType() == this || - (IsPointer && value.GetType() == typeof(IntPtr)) || - (IsByRef && value.GetType() == RuntimeTypeHandle.GetElementType(this)) || - (value.GetType().IsEnum && GetUnderlyingType((RuntimeType)value.GetType()) == GetUnderlyingType(this)) || - (IsEnum && GetUnderlyingType((RuntimeType)value.GetType()) == GetUnderlyingType(this))); - } -#endif - /// /// Verify and optionally convert the value for special cases. /// @@ -3723,7 +3710,7 @@ internal bool TryByRefFastPath(ref object arg, ref bool isValueType) return false; } - private static CorElementType GetUnderlyingType(RuntimeType type) + internal static CorElementType GetUnderlyingType(RuntimeType type) { if (type.IsEnum) { diff --git a/src/libraries/System.Private.CoreLib/src/System/Reflection/ConstructorInvoker.cs b/src/libraries/System.Private.CoreLib/src/System/Reflection/ConstructorInvoker.cs index e441196a0bdcc..0c85d8595ee4f 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Reflection/ConstructorInvoker.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Reflection/ConstructorInvoker.cs @@ -13,7 +13,7 @@ internal sealed partial class ConstructorInvoker #if !MONO // Temporary until Mono is updated. private bool _invoked; private bool _strategyDetermined; - private InvokerEmitUtil.InvokeFunc? _emitInvoke; + private InvokerEmitUtil.InvokeFunc? _emitInvoke; #endif public ConstructorInvoker(RuntimeConstructorInfo constructorInfo) @@ -52,12 +52,13 @@ public ConstructorInvoker(RuntimeConstructorInfo constructorInfo) { if (RuntimeFeature.IsDynamicCodeCompiled) { - _emitInvoke = InvokerEmitUtil.CreateInvokeDelegate(_method); + _emitInvoke = InvokerEmitUtil.CreateInvokeDelegate(_method); } _strategyDetermined = true; } } + object? ret; if ((invokeAttr & BindingFlags.DoNotWrapExceptions) == 0) { try @@ -66,23 +67,28 @@ public ConstructorInvoker(RuntimeConstructorInfo constructorInfo) // with a non-null 'obj', we use the slow path to avoid having two emit-based delegates. if (_emitInvoke != null && obj == null) { - return _emitInvoke(this, obj, args); + ret = _emitInvoke(target: null, args); + } + else + { + ret = InvokeNonEmitUnsafe(obj, args); } - - return InvokeNonEmitUnsafe(obj, args); } catch (Exception e) { throw new TargetInvocationException(e); } } - - if (_emitInvoke != null && obj == null) + else if (_emitInvoke != null && obj == null) + { + ret = _emitInvoke(target: null, args); + } + else { - return _emitInvoke(this, obj, args); + ret = InvokeNonEmitUnsafe(obj, args); } - return InvokeNonEmitUnsafe(obj, args); + return ret; } #endif } diff --git a/src/libraries/System.Private.CoreLib/src/System/Reflection/InvokerEmitUtil.cs b/src/libraries/System.Private.CoreLib/src/System/Reflection/InvokerEmitUtil.cs index a5e061815b835..ee105697b7bc5 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Reflection/InvokerEmitUtil.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Reflection/InvokerEmitUtil.cs @@ -12,24 +12,23 @@ internal static class InvokerEmitUtil // If changed, update native stack walking code that also uses this prefix to ignore reflection frames. private const string InvokeStubPrefix = "InvokeStub_"; - // The 'obj' parameter allows the DynamicMethod to be treated as an instance method which is slightly faster than a static. - internal unsafe delegate object? InvokeFunc(T obj, object? target, IntPtr* arguments); + internal unsafe delegate object? InvokeFunc(object? target, IntPtr* arguments); - public static unsafe InvokeFunc CreateInvokeDelegate(MethodBase method) + public static unsafe InvokeFunc CreateInvokeDelegate(MethodBase method) { Debug.Assert(!method.ContainsGenericParameters); bool emitNew = method is RuntimeConstructorInfo; bool hasThis = !(emitNew || method.IsStatic); - Type[] delegateParameters = new Type[3] { typeof(T), typeof(object), typeof(IntPtr*) }; + // The first element allows the DynamicMethod to be treated as an instance method which is slightly faster than a static. + Type[] delegateParameters = new Type[3] { typeof(object), typeof(object), typeof(IntPtr*) }; var dm = new DynamicMethod( InvokeStubPrefix + method.DeclaringType!.Name + "." + method.Name, returnType: typeof(object), delegateParameters, - owner: typeof(T), - skipVisibility: true); + restrictedSkipVisibility: true); ILGenerator il = dm.GetILGenerator(); @@ -136,8 +135,8 @@ public static unsafe InvokeFunc CreateInvokeDelegate(MethodBase method) il.Emit(OpCodes.Ret); - // Create the delegate; it is also compiled at this point due to skipVisibility=true. - return (InvokeFunc)dm.CreateDelegate(typeof(InvokeFunc)); + // Create the delegate; it is also compiled at this point due to restrictedSkipVisibility=true. + return (InvokeFunc)dm.CreateDelegate(typeof(InvokeFunc), target: null); } private static RuntimeType NormalizePointerType(RuntimeType type) diff --git a/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodBase.cs b/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodBase.cs index a4d06b71960fe..a5c09ee0c40a6 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodBase.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodBase.cs @@ -227,9 +227,13 @@ BindingFlags invokeAttr if (isValueType) { -#if DEBUG - // Once Mono has managed conversion logic, VerifyValueType() can be lifted here as Asserts. - sigType.VerifyValueType(arg); +#if !MONO // Temporary until Mono is updated. + Debug.Assert(arg != null); + Debug.Assert( + arg.GetType() == sigType || + (sigType.IsPointer && arg.GetType() == typeof(IntPtr)) || + (sigType.IsByRef && arg.GetType() == RuntimeTypeHandle.GetElementType(sigType)) || + ((sigType.IsEnum || arg.GetType().IsEnum) && RuntimeType.GetUnderlyingType((RuntimeType)arg.GetType()) == RuntimeType.GetUnderlyingType(sigType))); #endif ByReference valueTypeRef = new(ref copyOfParameters[i]!.GetRawData()); *(ByReference*)(byrefParameters + i) = valueTypeRef; diff --git a/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodInvoker.cs b/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodInvoker.cs index f21bdd8a099de..9257bb03b32cb 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodInvoker.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodInvoker.cs @@ -20,7 +20,7 @@ internal sealed partial class MethodInvoker #else private bool _invoked; private bool _strategyDetermined; - private InvokerEmitUtil.InvokeFunc? _emitInvoke; + private InvokerEmitUtil.InvokeFunc? _emitInvoke; [DebuggerStepThrough] [DebuggerHidden] @@ -37,35 +37,41 @@ internal sealed partial class MethodInvoker { if (RuntimeFeature.IsDynamicCodeCompiled) { - _emitInvoke = InvokerEmitUtil.CreateInvokeDelegate(_method); + _emitInvoke = InvokerEmitUtil.CreateInvokeDelegate(_method); } _strategyDetermined = true; } } + object? ret; if ((invokeAttr & BindingFlags.DoNotWrapExceptions) == 0) { try { if (_emitInvoke != null) { - return _emitInvoke(this, obj, args); + ret = _emitInvoke(obj, args); + } + else + { + ret = InvokeNonEmitUnsafe(obj, args); } - - return InvokeNonEmitUnsafe(obj, args); } catch (Exception e) { throw new TargetInvocationException(e); } } - - if (_emitInvoke != null) + else if (_emitInvoke != null) + { + ret = _emitInvoke(obj, args); + } + else { - return _emitInvoke(this, obj, args); + ret = InvokeNonEmitUnsafe(obj, args); } - return InvokeNonEmitUnsafe(obj, args); + return ret; } #endif } diff --git a/src/mono/System.Private.CoreLib/src/System/RuntimeType.Mono.cs b/src/mono/System.Private.CoreLib/src/System/RuntimeType.Mono.cs index 2781a4968c07d..cc68bf29eda1f 100644 --- a/src/mono/System.Private.CoreLib/src/System/RuntimeType.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/RuntimeType.Mono.cs @@ -1660,14 +1660,6 @@ internal override FieldInfo GetField(FieldInfo fromNoninstanciated) } } - // Once Mono has managed conversion logic, this method can be removed and the Core - // implementation of this method moved to RuntimeMethod.Invoke(). -#if DEBUG -#pragma warning disable CA1822 - internal void VerifyValueType(object? value) { } -#pragma warning restore CA1822 -#endif - /// /// Verify and optionally convert the value for special cases. /// From 1516fbf807714c7bf1c4744f2898410eea2cfbb8 Mon Sep 17 00:00:00 2001 From: Steve Harter Date: Mon, 2 May 2022 17:29:01 -0500 Subject: [PATCH 15/21] Fix possible null method.DeclaringType; binding test failures with Checked build --- .../src/System/Reflection/InvokerEmitUtil.cs | 3 ++- .../tracing/BinderTracingTest.ResolutionFlow.cs | 15 ++++++++++----- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Reflection/InvokerEmitUtil.cs b/src/libraries/System.Private.CoreLib/src/System/Reflection/InvokerEmitUtil.cs index ee105697b7bc5..2e21d55c17a13 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Reflection/InvokerEmitUtil.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Reflection/InvokerEmitUtil.cs @@ -24,8 +24,9 @@ public static unsafe InvokeFunc CreateInvokeDelegate(MethodBase method) // The first element allows the DynamicMethod to be treated as an instance method which is slightly faster than a static. Type[] delegateParameters = new Type[3] { typeof(object), typeof(object), typeof(IntPtr*) }; + string declaringTypeName = method.DeclaringType != null ? method.DeclaringType.Name + "." : string.Empty; var dm = new DynamicMethod( - InvokeStubPrefix + method.DeclaringType!.Name + "." + method.Name, + InvokeStubPrefix + declaringTypeName + method.Name, returnType: typeof(object), delegateParameters, restrictedSkipVisibility: true); diff --git a/src/tests/Loader/binding/tracing/BinderTracingTest.ResolutionFlow.cs b/src/tests/Loader/binding/tracing/BinderTracingTest.ResolutionFlow.cs index 63376afa07263..08efd20131636 100644 --- a/src/tests/Loader/binding/tracing/BinderTracingTest.ResolutionFlow.cs +++ b/src/tests/Loader/binding/tracing/BinderTracingTest.ResolutionFlow.cs @@ -73,7 +73,8 @@ public static BindOperation FindInLoadContext_DefaultALC() // ResolutionAttempted : DefaultAssemblyLoadContextFallback (CustomALC) [AssemblyNotFound] // ResolutionAttempted : AssemblyLoadContextResolvingEvent (CustomALC) [AssemblyNotFound] // ResolutionAttempted : AppDomainAssemblyResolveEvent (CustomALC) [AssemblyNotFound] - [BinderTest(isolate: true, testSetup: nameof(LoadSubdirectoryAssembly_InstanceALC))] + [BinderTest(isolate: true, testSetup: nameof(LoadSubdirectoryAssembly_InstanceALC), + activeIssue: "https://github.com/dotnet/runtime/issues/68521")] // Emit-based Invoke causes an extra load. public static BindOperation FindInLoadContext_CustomALC_IncompatibleVersion() { var assemblyName = new AssemblyName($"{SubdirectoryAssemblyName}, Version=4.3.2.1"); @@ -338,7 +339,8 @@ public static BindOperation ResolveSatelliteAssembly() // ResolutionAttempted : ApplicationAssemblies (DefaultALC) [AssemblyNotFound] // ResolutionAttempted : DefaultAssemblyLoadContextFallback (CustomALC) [AssemblyNotFound] // ResolutionAttempted : AssemblyLoadContextResolvingEvent (CustomALC) [Success] - [BinderTest(isolate: true)] + [BinderTest(isolate: true, + activeIssue: "https://github.com/dotnet/runtime/issues/68521")] // Emit-based Invoke causes an extra load. public static BindOperation AssemblyLoadContextResolvingEvent_CustomALC() { var assemblyName = new AssemblyName(SubdirectoryAssemblyName); @@ -407,7 +409,8 @@ public static BindOperation AssemblyLoadContextResolvingEvent_DefaultALC() // ResolutionAttempted : ApplicationAssemblies (DefaultALC) [AssemblyNotFound] // ResolutionAttempted : DefaultAssemblyLoadContextFallback (CustomALC) [AssemblyNotFound] // ResolutionAttempted : AssemblyLoadContextResolvingEvent (CustomALC) [Exception] - [BinderTest(isolate: true)] + [BinderTest(isolate: true, + activeIssue: "https://github.com/dotnet/runtime/issues/68521")] // Emit-based Invoke causes an extra load. public static BindOperation AssemblyLoadContextResolvingEvent_CustomALC_Exception() { var assemblyName = new AssemblyName(SubdirectoryAssemblyName); @@ -473,7 +476,8 @@ public static BindOperation AssemblyLoadContextResolvingEvent_DefaultALC_Excepti // ResolutionAttempted : DefaultAssemblyLoadContextFallback (CustomALC) [AssemblyNotFound] // ResolutionAttempted : AssemblyLoadContextResolvingEvent (CustomALC) [AssemblyNotFound] // ResolutionAttempted : AppDomainAssemblyResolveEvent (CustomALC) [Success] - [BinderTest(isolate: true)] + [BinderTest(isolate: true, + activeIssue: "https://github.com/dotnet/runtime/issues/68521")] // Emit-based Invoke causes an extra load. public static BindOperation AppDomainAssemblyResolveEvent_CustomALC() { var assemblyName = new AssemblyName(SubdirectoryAssemblyName); @@ -546,7 +550,8 @@ public static BindOperation AppDomainAssemblyResolveEvent_DefaultALC() // ResolutionAttempted : DefaultAssemblyLoadContextFallback (CustomALC) [AssemblyNotFound] // ResolutionAttempted : AssemblyLoadContextResolvingEvent (CustomALC) [AssemblyNotFound] // ResolutionAttempted : AppDomainAssemblyResolveEvent (CustomALC) [Exception] - [BinderTest(isolate: true)] + [BinderTest(isolate: true, + activeIssue: "https://github.com/dotnet/runtime/issues/68521")] // Emit-based Invoke causes an extra load. public static BindOperation AppDomainAssemblyResolveEvent_Exception() { var assemblyName = new AssemblyName(SubdirectoryAssemblyName); From 54e0a7736dd7e4ac260c3d537d48ca15cac87a45 Mon Sep 17 00:00:00 2001 From: Steve Harter Date: Tue, 3 May 2022 08:51:26 -0500 Subject: [PATCH 16/21] Fix DynamicMethod.Invoke when using Emit-based invoke --- .../src/System/Reflection/InvokerEmitUtil.cs | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Reflection/InvokerEmitUtil.cs b/src/libraries/System.Private.CoreLib/src/System/Reflection/InvokerEmitUtil.cs index 2e21d55c17a13..762e0271bfbae 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Reflection/InvokerEmitUtil.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Reflection/InvokerEmitUtil.cs @@ -88,7 +88,17 @@ public static unsafe InvokeFunc CreateInvokeDelegate(MethodBase method) } else { - RuntimeType returnType = (RuntimeType)((RuntimeMethodInfo)method).ReturnType; + RuntimeType returnType; + if (method is RuntimeMethodInfo rmi) + { + returnType = (RuntimeType)rmi.ReturnType; + } + else + { + Debug.Assert(method is DynamicMethod); + returnType = (RuntimeType)((DynamicMethod)method).ReturnType; + } + if (returnType == typeof(void)) { il.Emit(OpCodes.Ldnull); From 89a04834c5f6abe9cd88245c2f0ff525bbdf60bb Mon Sep 17 00:00:00 2001 From: Steve Harter Date: Tue, 3 May 2022 16:20:36 -0500 Subject: [PATCH 17/21] Disable some JIT Stress tests on arm32 linux --- src/tests/issues.targets | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/tests/issues.targets b/src/tests/issues.targets index a0d6a2460e93d..c5d1ef803bc03 100644 --- a/src/tests/issues.targets +++ b/src/tests/issues.targets @@ -666,6 +666,18 @@ https://github.com/dotnet/runtime/issues/57856 + + https://github.com/dotnet/runtime/issues/68837 + + + https://github.com/dotnet/runtime/issues/68837 + + + https://github.com/dotnet/runtime/issues/68837 + + + https://github.com/dotnet/runtime/issues/68837 + https://github.com/dotnet/runtime/issues/57875 From 9e5096051609b083412d662a1e8bc5e7a7725efc Mon Sep 17 00:00:00 2001 From: Steve Harter Date: Thu, 5 May 2022 10:18:03 -0500 Subject: [PATCH 18/21] renaming; native perf; other feedback --- .../Reflection/ConstructorInvoker.CoreCLR.cs | 2 +- .../System/Reflection/Emit/DynamicMethod.cs | 6 +-- .../Reflection/MethodInvoker.CoreCLR.cs | 2 +- .../Reflection/RuntimeMethodInfo.CoreCLR.cs | 4 +- src/coreclr/vm/object.cpp | 2 +- src/coreclr/vm/reflectioninvocation.cpp | 20 +++++----- .../System/Reflection/ConstructorInvoker.cs | 37 +++++++++++-------- .../src/System/Reflection/InvokerEmitUtil.cs | 22 +++-------- .../src/System/Reflection/MethodInvoker.cs | 35 +++++++++++------- .../Reflection/RuntimeConstructorInfo.cs | 20 +++++----- .../System/Reflection/RuntimeMethodInfo.cs | 10 ++--- .../Reflection/ConstructorInvoker.Mono.cs | 2 +- .../System/Reflection/MethodInvoker.Mono.cs | 2 +- .../src/System/RuntimeType.Mono.cs | 4 +- 14 files changed, 86 insertions(+), 82 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Reflection/ConstructorInvoker.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/Reflection/ConstructorInvoker.CoreCLR.cs index a0d938c36afcc..05f69d2670e3e 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Reflection/ConstructorInvoker.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Reflection/ConstructorInvoker.CoreCLR.cs @@ -10,7 +10,7 @@ internal partial class ConstructorInvoker public InvocationFlags _invocationFlags; [MethodImpl(MethodImplOptions.AggressiveInlining)] - private unsafe object? InvokeNonEmitUnsafe(object? obj, IntPtr* arguments) + private unsafe object? InterpretedInvoke(object? obj, IntPtr* arguments) { return RuntimeMethodHandle.InvokeMethod(obj, (void**)arguments, _method.Signature, isConstructor: obj is null)!; } diff --git a/src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/DynamicMethod.cs b/src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/DynamicMethod.cs index b655fbae400af..1824eb89b2dde 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/DynamicMethod.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/DynamicMethod.cs @@ -489,7 +489,7 @@ Signature LazyCreateSignature() { if (argCount == 0) { - retValue = Invoker.InvokeUnsafe(obj, args: default, invokeAttr); + retValue = Invoker.InlinedInvoke(obj, args: default, invokeAttr); } else if (argCount > MaxStackAllocArgCount) { @@ -516,7 +516,7 @@ Signature LazyCreateSignature() culture, invokeAttr); - retValue = Invoker.InvokeUnsafe(obj, pByRefStorage, invokeAttr); + retValue = Invoker.InlinedInvoke(obj, pByRefStorage, invokeAttr); // Copy modified values out. This should be done only with ByRef or Type.Missing parameters. for (int i = 0; i < argCount; i++) @@ -571,7 +571,7 @@ Signature LazyCreateSignature() culture, invokeAttr); - retValue = mi.Invoker.InvokeUnsafe(obj, pByRefStorage, invokeAttr); + retValue = mi.Invoker.InlinedInvoke(obj, pByRefStorage, invokeAttr); } finally { diff --git a/src/coreclr/System.Private.CoreLib/src/System/Reflection/MethodInvoker.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/Reflection/MethodInvoker.CoreCLR.cs index 7bd5147889c90..6acf77be549c0 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Reflection/MethodInvoker.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Reflection/MethodInvoker.CoreCLR.cs @@ -25,7 +25,7 @@ public MethodInvoker(MethodBase method, Signature signature) } [MethodImpl(MethodImplOptions.AggressiveInlining)] - private unsafe object? InvokeNonEmitUnsafe(object? obj, IntPtr* arguments) + private unsafe object? InterpretedInvoke(object? obj, IntPtr* arguments) { return RuntimeMethodHandle.InvokeMethod(obj, (void**)arguments, _signature, isConstructor: false); } diff --git a/src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeMethodInfo.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeMethodInfo.CoreCLR.cs index e779376940359..df67c35c56495 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeMethodInfo.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeMethodInfo.CoreCLR.cs @@ -367,9 +367,9 @@ public override MethodImplAttributes GetMethodImplementationFlags() invokeAttr); #if MONO // Temporary until Mono is updated. - retValue = Invoker.InvokeUnsafe(obj, copyOfParameters, invokeAttr); + retValue = Invoker.InlinedInvoke(obj, copyOfParameters, invokeAttr); #else - retValue = Invoker.InvokeUnsafe(obj, pByRefStorage, invokeAttr); + retValue = Invoker.InlinedInvoke(obj, pByRefStorage, invokeAttr); #endif } diff --git a/src/coreclr/vm/object.cpp b/src/coreclr/vm/object.cpp index 995cf608737b9..83ac8e718c337 100644 --- a/src/coreclr/vm/object.cpp +++ b/src/coreclr/vm/object.cpp @@ -1739,7 +1739,7 @@ BOOL Nullable::UnBoxNoGC(void* destPtr, OBJECTREF boxedVal, MethodTable* destMT) { // For safety's sake, also allow true nullables to be unboxed normally. // This should not happen normally, but we want to be robust - if (destMT->IsEquivalentTo(boxedVal->GetMethodTable())) + if (destMT == boxedVal->GetMethodTable()) { CopyValueClass(dest, boxedVal->GetData(), destMT); return TRUE; diff --git a/src/coreclr/vm/reflectioninvocation.cpp b/src/coreclr/vm/reflectioninvocation.cpp index 0f4b947f387aa..2b6e75b6817fb 100644 --- a/src/coreclr/vm/reflectioninvocation.cpp +++ b/src/coreclr/vm/reflectioninvocation.cpp @@ -150,8 +150,7 @@ FCIMPL2(Object*, ReflectionInvocation::AllocateValueType, ReflectClassBaseObject // This method is only intended for value types; it is not called directly by any public APIs // so we don't expect validation issues here. - _ASSERTE(InvokeUtil::IsPrimitiveType(targetType.GetSignatureCorElementType()) || - targetType.GetSignatureCorElementType() == ELEMENT_TYPE_VALUETYPE); + _ASSERTE(targetType.IsValueType()); MethodTable* allocMT = targetType.AsMethodTable(); _ASSERTE(!allocMT->IsByRefLike()); @@ -850,17 +849,18 @@ FCIMPL1(Object*, RuntimeMethodHandle::ReboxFromNullable, Object* pBoxedValUNSAFE OBJECTREF retVal; } gc; - gc.pBoxed = ObjectToOBJECTREF(pBoxedValUNSAFE); - gc.retVal = gc.pBoxed; + if (pBoxedValUNSAFE == NULL) + return NULL; - HELPER_METHOD_FRAME_BEGIN_RET_PROTECT(gc); + gc.pBoxed = ObjectToOBJECTREF(pBoxedValUNSAFE); + MethodTable* retMT = gc.pBoxed->GetMethodTable(); + if (!Nullable::IsNullableType(retMT)) + return pBoxedValUNSAFE; - if (gc.pBoxed != NULL) { - MethodTable* retMT = gc.pBoxed->GetMethodTable(); - if (Nullable::IsNullableType(retMT)) - gc.retVal = Nullable::Box(gc.pBoxed->GetData(), retMT); - } + gc.retVal = NULL; + HELPER_METHOD_FRAME_BEGIN_RET_PROTECT(gc); + gc.retVal = Nullable::Box(gc.pBoxed->GetData(), retMT); HELPER_METHOD_FRAME_END(); return OBJECTREFToObject(gc.retVal); diff --git a/src/libraries/System.Private.CoreLib/src/System/Reflection/ConstructorInvoker.cs b/src/libraries/System.Private.CoreLib/src/System/Reflection/ConstructorInvoker.cs index 0c85d8595ee4f..c79036380b8e7 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Reflection/ConstructorInvoker.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Reflection/ConstructorInvoker.cs @@ -13,7 +13,7 @@ internal sealed partial class ConstructorInvoker #if !MONO // Temporary until Mono is updated. private bool _invoked; private bool _strategyDetermined; - private InvokerEmitUtil.InvokeFunc? _emitInvoke; + private InvokerEmitUtil.InvokeFunc? _invokeFunc; #endif public ConstructorInvoker(RuntimeConstructorInfo constructorInfo) @@ -29,17 +29,24 @@ public ConstructorInvoker(RuntimeConstructorInfo constructorInfo) #endif } + [MethodImpl(MethodImplOptions.AggressiveInlining)] #if MONO // Temporary until Mono is updated. - [DebuggerStepThrough] - [DebuggerHidden] - public unsafe object? InvokeUnsafe(object? obj, Span args, BindingFlags invokeAttr) + public unsafe object? InlinedInvoke(object? obj, Span args, BindingFlags invokeAttr) => InterpretedInvoke(obj, args, invokeAttr); +#else + public unsafe object? InlinedInvoke(object? obj, IntPtr* args, BindingFlags invokeAttr) { - return InvokeNonEmitUnsafe(obj, args, invokeAttr); + if (_invokeFunc != null && (invokeAttr & BindingFlags.DoNotWrapExceptions) != 0 && obj == null) + { + return _invokeFunc(target: null, args); + } + return Invoke(obj, args, invokeAttr); } -#else +#endif + +#if !MONO // Temporary until Mono is updated. [DebuggerStepThrough] [DebuggerHidden] - public unsafe object? InvokeUnsafe(object? obj, IntPtr* args, BindingFlags invokeAttr) + private unsafe object? Invoke(object? obj, IntPtr* args, BindingFlags invokeAttr) { if (!_strategyDetermined) { @@ -52,7 +59,7 @@ public ConstructorInvoker(RuntimeConstructorInfo constructorInfo) { if (RuntimeFeature.IsDynamicCodeCompiled) { - _emitInvoke = InvokerEmitUtil.CreateInvokeDelegate(_method); + _invokeFunc = InvokerEmitUtil.CreateInvokeDelegate(_method); } _strategyDetermined = true; } @@ -63,15 +70,15 @@ public ConstructorInvoker(RuntimeConstructorInfo constructorInfo) { try { - // For the rarely used and broken scenario of calling the constructor directly through MethodBase.Invoke() + // For the rarely used scenario of calling the constructor directly through MethodBase.Invoke() // with a non-null 'obj', we use the slow path to avoid having two emit-based delegates. - if (_emitInvoke != null && obj == null) + if (_invokeFunc != null && obj == null) { - ret = _emitInvoke(target: null, args); + ret = _invokeFunc(target: null, args); } else { - ret = InvokeNonEmitUnsafe(obj, args); + ret = InterpretedInvoke(obj, args); } } catch (Exception e) @@ -79,13 +86,13 @@ public ConstructorInvoker(RuntimeConstructorInfo constructorInfo) throw new TargetInvocationException(e); } } - else if (_emitInvoke != null && obj == null) + else if (_invokeFunc != null && obj == null) { - ret = _emitInvoke(target: null, args); + ret = _invokeFunc(target: null, args); } else { - ret = InvokeNonEmitUnsafe(obj, args); + ret = InterpretedInvoke(obj, args); } return ret; diff --git a/src/libraries/System.Private.CoreLib/src/System/Reflection/InvokerEmitUtil.cs b/src/libraries/System.Private.CoreLib/src/System/Reflection/InvokerEmitUtil.cs index 762e0271bfbae..21226c25d2ffb 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Reflection/InvokerEmitUtil.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Reflection/InvokerEmitUtil.cs @@ -21,7 +21,7 @@ public static unsafe InvokeFunc CreateInvokeDelegate(MethodBase method) bool emitNew = method is RuntimeConstructorInfo; bool hasThis = !(emitNew || method.IsStatic); - // The first element allows the DynamicMethod to be treated as an instance method which is slightly faster than a static. + // The first parameter is unused and allows the DynamicMethod to be treated as an instance method which is slightly faster than a static. Type[] delegateParameters = new Type[3] { typeof(object), typeof(object), typeof(IntPtr*) }; string declaringTypeName = method.DeclaringType != null ? method.DeclaringType.Name + "." : string.Empty; @@ -47,7 +47,11 @@ public static unsafe InvokeFunc CreateInvokeDelegate(MethodBase method) ParameterInfo[] parameters = method.GetParametersNoCopy(); for (int i = 0; i < parameters.Length; i++) { - RuntimeType parameterType = NormalizePointerType((RuntimeType)parameters[i].ParameterType); + RuntimeType parameterType = (RuntimeType)parameters[i].ParameterType; + if (parameterType.IsPointer) + { + parameterType = (RuntimeType)typeof(IntPtr); + } il.Emit(OpCodes.Ldarg_2); if (i != 0) @@ -150,20 +154,6 @@ public static unsafe InvokeFunc CreateInvokeDelegate(MethodBase method) return (InvokeFunc)dm.CreateDelegate(typeof(InvokeFunc), target: null); } - private static RuntimeType NormalizePointerType(RuntimeType type) - { - if (type.IsPointer) - { - type = (RuntimeType)typeof(IntPtr); - } - else if (type.IsByRef && RuntimeTypeHandle.GetElementType(type).IsPointer) - { - type = (RuntimeType)typeof(IntPtr).MakeByRefType(); - } - - return type; - } - private static class ThrowHelper { public static void Throw_NullReference_InvokeNullRefReturned() diff --git a/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodInvoker.cs b/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodInvoker.cs index 9257bb03b32cb..3adbad39eb21c 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodInvoker.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodInvoker.cs @@ -10,21 +10,28 @@ internal sealed partial class MethodInvoker { private readonly MethodBase _method; + [MethodImpl(MethodImplOptions.AggressiveInlining)] #if MONO // Temporary until Mono is updated. - [DebuggerStepThrough] - [DebuggerHidden] - public unsafe object? InvokeUnsafe(object? obj, Span args, BindingFlags invokeAttr) + public unsafe object? InlinedInvoke(object? obj, Span args, BindingFlags invokeAttr) => InterpretedInvoke(obj, args, invokeAttr); +#else + public unsafe object? InlinedInvoke(object? obj, IntPtr* args, BindingFlags invokeAttr) { - return InvokeNonEmitUnsafe(obj, args, invokeAttr); + if (_invokeFunc != null && (invokeAttr & BindingFlags.DoNotWrapExceptions) != 0) + { + return _invokeFunc(obj, args); + } + return Invoke(obj, args, invokeAttr); } -#else +#endif + +#if !MONO // Temporary until Mono is updated. private bool _invoked; private bool _strategyDetermined; - private InvokerEmitUtil.InvokeFunc? _emitInvoke; + private InvokerEmitUtil.InvokeFunc? _invokeFunc; [DebuggerStepThrough] [DebuggerHidden] - public unsafe object? InvokeUnsafe(object? obj, IntPtr* args, BindingFlags invokeAttr) + private unsafe object? Invoke(object? obj, IntPtr* args, BindingFlags invokeAttr) { if (!_strategyDetermined) { @@ -37,7 +44,7 @@ internal sealed partial class MethodInvoker { if (RuntimeFeature.IsDynamicCodeCompiled) { - _emitInvoke = InvokerEmitUtil.CreateInvokeDelegate(_method); + _invokeFunc = InvokerEmitUtil.CreateInvokeDelegate(_method); } _strategyDetermined = true; } @@ -48,13 +55,13 @@ internal sealed partial class MethodInvoker { try { - if (_emitInvoke != null) + if (_invokeFunc != null) { - ret = _emitInvoke(obj, args); + ret = _invokeFunc(obj, args); } else { - ret = InvokeNonEmitUnsafe(obj, args); + ret = InterpretedInvoke(obj, args); } } catch (Exception e) @@ -62,13 +69,13 @@ internal sealed partial class MethodInvoker throw new TargetInvocationException(e); } } - else if (_emitInvoke != null) + else if (_invokeFunc != null) { - ret = _emitInvoke(obj, args); + ret = _invokeFunc(obj, args); } else { - ret = InvokeNonEmitUnsafe(obj, args); + ret = InterpretedInvoke(obj, args); } return ret; diff --git a/src/libraries/System.Private.CoreLib/src/System/Reflection/RuntimeConstructorInfo.cs b/src/libraries/System.Private.CoreLib/src/System/Reflection/RuntimeConstructorInfo.cs index c6db516298ca9..49b59196b2251 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Reflection/RuntimeConstructorInfo.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Reflection/RuntimeConstructorInfo.cs @@ -130,7 +130,7 @@ internal void ThrowNoInvokeException() { if (argCount == 0) { - Invoker.InvokeUnsafe(obj, args: default, invokeAttr); + Invoker.InlinedInvoke(obj, args: default, invokeAttr); } else if (argCount > MaxStackAllocArgCount) { @@ -158,9 +158,9 @@ internal void ThrowNoInvokeException() invokeAttr); #if MONO // Temporary until Mono is updated. - Invoker.InvokeUnsafe(obj, copyOfParameters, invokeAttr); + Invoker.InlinedInvoke(obj, copyOfParameters, invokeAttr); #else - Invoker.InvokeUnsafe(obj, pByRefStorage, invokeAttr); + Invoker.InlinedInvoke(obj, pByRefStorage, invokeAttr); #endif // Copy modified values out. This should be done only with ByRef or Type.Missing parameters. @@ -217,9 +217,9 @@ private static unsafe void InvokeWithManyArguments( invokeAttr); #if MONO // Temporary until Mono is updated. - ci.Invoker.InvokeUnsafe(obj, copyOfParameters, invokeAttr); + ci.Invoker.InlinedInvoke(obj, copyOfParameters, invokeAttr); #else - ci.Invoker.InvokeUnsafe(obj, pByRefStorage, invokeAttr); + ci.Invoker.InlinedInvoke(obj, pByRefStorage, invokeAttr); #endif } finally @@ -262,7 +262,7 @@ public override object Invoke(BindingFlags invokeAttr, Binder? binder, object?[] { if (argCount == 0) { - retValue = Invoker.InvokeUnsafe(obj: null, args: default, invokeAttr); + retValue = Invoker.InlinedInvoke(obj: null, args: default, invokeAttr); } else if (argCount > MaxStackAllocArgCount) { @@ -289,9 +289,9 @@ public override object Invoke(BindingFlags invokeAttr, Binder? binder, object?[] invokeAttr); #if MONO // Temporary until Mono is updated. - retValue = Invoker.InvokeUnsafe(obj: null, copyOfParameters, invokeAttr); + retValue = Invoker.InlinedInvoke(obj: null, copyOfParameters, invokeAttr); #else - retValue = Invoker.InvokeUnsafe(obj: null, pByRefStorage, invokeAttr); + retValue = Invoker.InlinedInvoke(obj: null, pByRefStorage, invokeAttr); #endif // Copy modified values out. This should be done only with ByRef or Type.Missing parameters. @@ -351,9 +351,9 @@ public override object Invoke(BindingFlags invokeAttr, Binder? binder, object?[] invokeAttr); #if MONO // Temporary until Mono is updated. - retValue = ci.Invoker.InvokeUnsafe(obj: null, copyOfParameters, invokeAttr); + retValue = ci.Invoker.InlinedInvoke(obj: null, copyOfParameters, invokeAttr); #else - retValue = ci.Invoker.InvokeUnsafe(obj: null, pByRefStorage, invokeAttr); + retValue = ci.Invoker.InlinedInvoke(obj: null, pByRefStorage, invokeAttr); #endif } finally diff --git a/src/libraries/System.Private.CoreLib/src/System/Reflection/RuntimeMethodInfo.cs b/src/libraries/System.Private.CoreLib/src/System/Reflection/RuntimeMethodInfo.cs index 5d2032ff2f8de..72d22089d7e08 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Reflection/RuntimeMethodInfo.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Reflection/RuntimeMethodInfo.cs @@ -126,7 +126,7 @@ private void ThrowNoInvokeException() { if (argCount == 0) { - retValue = Invoker.InvokeUnsafe(obj, args: default, invokeAttr); + retValue = Invoker.InlinedInvoke(obj, args: default, invokeAttr); } else if (argCount > MaxStackAllocArgCount) { @@ -154,9 +154,9 @@ private void ThrowNoInvokeException() invokeAttr); #if MONO // Temporary until Mono is updated. - retValue = Invoker.InvokeUnsafe(obj, copyOfParameters, invokeAttr); + retValue = Invoker.InlinedInvoke(obj, copyOfParameters, invokeAttr); #else - retValue = Invoker.InvokeUnsafe(obj, pByRefStorage, invokeAttr); + retValue = Invoker.InlinedInvoke(obj, pByRefStorage, invokeAttr); #endif // Copy modified values out. This should be done only with ByRef or Type.Missing parameters. @@ -214,9 +214,9 @@ private void ThrowNoInvokeException() invokeAttr); #if MONO // Temporary until Mono is updated. - retValue = mi.Invoker.InvokeUnsafe(obj, copyOfParameters, invokeAttr); + retValue = mi.Invoker.InlinedInvoke(obj, copyOfParameters, invokeAttr); #else - retValue = mi.Invoker.InvokeUnsafe(obj, pByRefStorage, invokeAttr); + retValue = mi.Invoker.InlinedInvoke(obj, pByRefStorage, invokeAttr); #endif } finally diff --git a/src/mono/System.Private.CoreLib/src/System/Reflection/ConstructorInvoker.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Reflection/ConstructorInvoker.Mono.cs index 55c7d5d0a5079..49b13afc66511 100644 --- a/src/mono/System.Private.CoreLib/src/System/Reflection/ConstructorInvoker.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Reflection/ConstructorInvoker.Mono.cs @@ -8,7 +8,7 @@ namespace System.Reflection internal partial class ConstructorInvoker { [MethodImpl(MethodImplOptions.AggressiveInlining)] - private unsafe object? InvokeNonEmitUnsafe(object? obj, Span args, BindingFlags invokeAttr) + private unsafe object? InterpretedInvoke(object? obj, Span args, BindingFlags invokeAttr) { Exception exc; object? o; diff --git a/src/mono/System.Private.CoreLib/src/System/Reflection/MethodInvoker.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Reflection/MethodInvoker.Mono.cs index 1bfcceec8eb82..2ea61086c7a4f 100644 --- a/src/mono/System.Private.CoreLib/src/System/Reflection/MethodInvoker.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Reflection/MethodInvoker.Mono.cs @@ -21,7 +21,7 @@ public MethodInvoker(MethodBase method) } [MethodImpl(MethodImplOptions.AggressiveInlining)] - private unsafe object? InvokeNonEmitUnsafe(object? obj, Span args, BindingFlags invokeAttr) + private unsafe object? InterpretedInvoke(object? obj, Span args, BindingFlags invokeAttr) { Exception? exc; object? o; diff --git a/src/mono/System.Private.CoreLib/src/System/RuntimeType.Mono.cs b/src/mono/System.Private.CoreLib/src/System/RuntimeType.Mono.cs index cc68bf29eda1f..3ecb3b5b0289f 100644 --- a/src/mono/System.Private.CoreLib/src/System/RuntimeType.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/RuntimeType.Mono.cs @@ -1653,7 +1653,7 @@ internal override FieldInfo GetField(FieldInfo fromNoninstanciated) unsafe { - return ctor.Invoker.InvokeUnsafe( + return ctor.Invoker.InlinedInvoke( obj: null, args: default, wrapExceptions ? BindingFlags.Default : BindingFlags.DoNotWrapExceptions); @@ -2043,7 +2043,7 @@ internal static object CreateInstanceForAnotherGenericParameter( unsafe { - return ctor.Invoker.InvokeUnsafe(obj: null, args: default, BindingFlags.Default)!; + return ctor.Invoker.InlinedInvoke(obj: null, args: default, BindingFlags.Default)!; } } From 53a4167f8e948c54e5e1c5d92ffd9f9c76602eb2 Mon Sep 17 00:00:00 2001 From: Steve Harter Date: Thu, 5 May 2022 13:00:18 -0500 Subject: [PATCH 19/21] Nit feedback --- .../src/System/Reflection/InvokerEmitUtil.cs | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Reflection/InvokerEmitUtil.cs b/src/libraries/System.Private.CoreLib/src/System/Reflection/InvokerEmitUtil.cs index 21226c25d2ffb..f79b802b62cd4 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Reflection/InvokerEmitUtil.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Reflection/InvokerEmitUtil.cs @@ -21,7 +21,7 @@ public static unsafe InvokeFunc CreateInvokeDelegate(MethodBase method) bool emitNew = method is RuntimeConstructorInfo; bool hasThis = !(emitNew || method.IsStatic); - // The first parameter is unused and allows the DynamicMethod to be treated as an instance method which is slightly faster than a static. + // The first parameter is unused but supports treating the DynamicMethod as an instance method which is slightly faster than a static. Type[] delegateParameters = new Type[3] { typeof(object), typeof(object), typeof(IntPtr*) }; string declaringTypeName = method.DeclaringType != null ? method.DeclaringType.Name + "." : string.Empty; @@ -47,12 +47,6 @@ public static unsafe InvokeFunc CreateInvokeDelegate(MethodBase method) ParameterInfo[] parameters = method.GetParametersNoCopy(); for (int i = 0; i < parameters.Length; i++) { - RuntimeType parameterType = (RuntimeType)parameters[i].ParameterType; - if (parameterType.IsPointer) - { - parameterType = (RuntimeType)typeof(IntPtr); - } - il.Emit(OpCodes.Ldarg_2); if (i != 0) { @@ -61,9 +55,11 @@ public static unsafe InvokeFunc CreateInvokeDelegate(MethodBase method) } il.Emit(OpCodes.Call, Methods.ByReferenceOfByte_Value()); // This can be replaced by ldfld once byref fields are available in C# + + RuntimeType parameterType = (RuntimeType)parameters[i].ParameterType; if (!parameterType.IsByRef) { - il.Emit(OpCodes.Ldobj, parameterType); + il.Emit(OpCodes.Ldobj, parameterType.IsPointer ? typeof(IntPtr) : parameterType); } } From 8efba3da88970358f71fa8eb0f0636104f548d0d Mon Sep 17 00:00:00 2001 From: Steve Harter Date: Fri, 6 May 2022 13:34:47 -0500 Subject: [PATCH 20/21] For perf, avoid calling ReboxFromNullable when not necessary --- .../System/Reflection/Emit/DynamicMethod.cs | 40 +++++++--- .../src/System/Reflection/RtFieldInfo.cs | 2 +- .../Reflection/RuntimeMethodInfo.CoreCLR.cs | 2 +- .../src/System/RuntimeType.CoreCLR.cs | 20 +++-- .../System.Private.CoreLib.Shared.projitems | 1 + .../src/System/Reflection/MethodBase.cs | 14 ++-- .../Reflection/ParameterCopyBackAction.cs | 15 ++++ .../Reflection/RuntimeConstructorInfo.cs | 80 ++++++++++++++----- .../System/Reflection/RuntimeMethodInfo.cs | 40 +++++++--- 9 files changed, 161 insertions(+), 53 deletions(-) create mode 100644 src/libraries/System.Private.CoreLib/src/System/Reflection/ParameterCopyBackAction.cs diff --git a/src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/DynamicMethod.cs b/src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/DynamicMethod.cs index 1824eb89b2dde..bc1966d1e3bfa 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/DynamicMethod.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/DynamicMethod.cs @@ -500,8 +500,8 @@ Signature LazyCreateSignature() { Debug.Assert(parameters != null); StackAllocedArguments argStorage = default; - Span copyOfParameters = new Span(ref argStorage._arg0, argCount); - Span shouldCopyBackParameters = new Span(ref argStorage._copyBack0, argCount); + Span copyOfParameters = new(ref argStorage._arg0, argCount); + Span shouldCopyBackParameters = new(ref argStorage._copyBack0, argCount); StackAllocatedByRefs byrefStorage = default; IntPtr* pByRefStorage = (IntPtr*)&byrefStorage; @@ -521,9 +521,20 @@ Signature LazyCreateSignature() // Copy modified values out. This should be done only with ByRef or Type.Missing parameters. for (int i = 0; i < argCount; i++) { - if (shouldCopyBackParameters[i]) + ParameterCopyBackAction action = shouldCopyBackParameters[i]; + if (action != ParameterCopyBackAction.None) { - parameters[i] = RuntimeMethodHandle.ReboxFromNullable(copyOfParameters[i]); + if (action == ParameterCopyBackAction.Copy) + { + parameters[i] = copyOfParameters[i]; + } + else + { + Debug.Assert(action == ParameterCopyBackAction.CopyNullable); + Debug.Assert(copyOfParameters[i] != null); + Debug.Assert(((RuntimeType)copyOfParameters[i]!.GetType()).IsNullableOfT); + parameters[i] = RuntimeMethodHandle.ReboxFromNullable(copyOfParameters[i]); + } } } } @@ -545,15 +556,15 @@ Signature LazyCreateSignature() CultureInfo? culture) { object[] objHolder = new object[argCount]; - Span copyOfParameters = new Span(objHolder, 0, argCount); + Span copyOfParameters = new(objHolder, 0, argCount); // We don't check a max stack size since we are invoking a method which // naturally requires a stack size that is dependent on the arg count\size. IntPtr* pByRefStorage = stackalloc IntPtr[argCount]; Buffer.ZeroMemory((byte*)pByRefStorage, (uint)(argCount * sizeof(IntPtr))); - bool* boolHolder = stackalloc bool[argCount]; - Span shouldCopyBackParameters = new Span(boolHolder, argCount); + ParameterCopyBackAction* copyBackActions = stackalloc ParameterCopyBackAction[argCount]; + Span shouldCopyBackParameters = new(copyBackActions, argCount); GCFrameRegistration reg = new(pByRefStorage, (uint)argCount, areByRefs: true); @@ -581,9 +592,20 @@ Signature LazyCreateSignature() // Copy modified values out. This should be done only with ByRef or Type.Missing parameters. for (int i = 0; i < argCount; i++) { - if (shouldCopyBackParameters[i]) + ParameterCopyBackAction action = shouldCopyBackParameters[i]; + if (action != ParameterCopyBackAction.None) { - parameters[i] = RuntimeMethodHandle.ReboxFromNullable(copyOfParameters[i]); + if (action == ParameterCopyBackAction.Copy) + { + parameters[i] = copyOfParameters[i]; + } + else + { + Debug.Assert(action == ParameterCopyBackAction.CopyNullable); + Debug.Assert(copyOfParameters[i] != null); + Debug.Assert(((RuntimeType)copyOfParameters[i]!.GetType()).IsNullableOfT); + parameters[i] = RuntimeMethodHandle.ReboxFromNullable(copyOfParameters[i]); + } } } diff --git a/src/coreclr/System.Private.CoreLib/src/System/Reflection/RtFieldInfo.cs b/src/coreclr/System.Private.CoreLib/src/System/Reflection/RtFieldInfo.cs index 8eb09a7756e8f..5831eca455adc 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Reflection/RtFieldInfo.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Reflection/RtFieldInfo.cs @@ -236,7 +236,7 @@ public override void SetValue(object? obj, object? value, BindingFlags invokeAtt CheckConsistency(obj); - bool _ref = false; + ParameterCopyBackAction _ref = default; RuntimeType fieldType = (RuntimeType)FieldType; if (value is null) { diff --git a/src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeMethodInfo.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeMethodInfo.CoreCLR.cs index df67c35c56495..229e2981bb6f2 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeMethodInfo.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeMethodInfo.CoreCLR.cs @@ -351,7 +351,7 @@ public override MethodImplAttributes GetMethodImplementationFlags() StackAllocedArguments argStorage = default; Span copyOfParameters = new(ref argStorage._arg0, 1); ReadOnlySpan parameters = new(in parameter); - Span shouldCopyBackParameters = new(ref argStorage._copyBack0, 1); + Span shouldCopyBackParameters = new(ref argStorage._copyBack0, 1); StackAllocatedByRefs byrefStorage = default; IntPtr* pByRefStorage = (IntPtr*)&byrefStorage; diff --git a/src/coreclr/System.Private.CoreLib/src/System/RuntimeType.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/RuntimeType.CoreCLR.cs index 9beb95dabd083..2ca4dd8f3744b 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/RuntimeType.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/RuntimeType.CoreCLR.cs @@ -3510,7 +3510,7 @@ private enum CheckValueStatus /// True if is a value type, False otherwise internal bool CheckValue( ref object? value, - ref bool copyBack, + ref ParameterCopyBackAction copyBack, Binder? binder, CultureInfo? culture, BindingFlags invokeAttr) @@ -3555,7 +3555,7 @@ internal bool CheckValue( value = binder.ChangeType(value, this, culture); if (IsInstanceOfType(value)) { - copyBack = true; + copyBack = ParameterCopyBackAction.Copy; return IsValueType; // Note the call to IsValueType, not the variable. } @@ -3581,13 +3581,13 @@ internal bool CheckValue( private CheckValueStatus TryChangeType( ref object? value, - out bool copyBack, + out ParameterCopyBackAction copyBack, out bool isValueType) { RuntimeType? sigElementType; if (RuntimeTypeHandle.TryGetByRefElementType(this, out sigElementType)) { - copyBack = true; + copyBack = ParameterCopyBackAction.Copy; Debug.Assert(!sigElementType.IsGenericParameter); if (sigElementType.IsInstanceOfType(value)) @@ -3599,6 +3599,7 @@ private CheckValueStatus TryChangeType( { // Pass as a true boxed Nullable, not as a T or null. value = RuntimeMethodHandle.ReboxToNullable(value, sigElementType); + copyBack = ParameterCopyBackAction.CopyNullable; } else { @@ -3626,6 +3627,7 @@ private CheckValueStatus TryChangeType( // Allocate default. value = AllocateValueType(sigElementType, value: null); + copyBack = sigElementType.IsNullableOfT ? ParameterCopyBackAction.CopyNullable : ParameterCopyBackAction.Copy; return CheckValueStatus.Success; } @@ -3635,7 +3637,7 @@ private CheckValueStatus TryChangeType( if (value == null) { - copyBack = false; + copyBack = ParameterCopyBackAction.None; isValueType = RuntimeTypeHandle.IsValueType(this); if (!isValueType) { @@ -3665,7 +3667,8 @@ private CheckValueStatus TryChangeType( if (!CanValueSpecialCast(srcType, this)) { - isValueType = copyBack = false; + isValueType = false; + copyBack = ParameterCopyBackAction.None; return CheckValueStatus.ArgumentException; } @@ -3684,11 +3687,12 @@ private CheckValueStatus TryChangeType( } isValueType = true; - copyBack = false; + copyBack = ParameterCopyBackAction.None; return CheckValueStatus.Success; } - copyBack = isValueType = false; + isValueType = false; + copyBack = ParameterCopyBackAction.None; return CheckValueStatus.ArgumentException; } diff --git a/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems b/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems index e044e7e984b86..57eb374430e02 100644 --- a/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems +++ b/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems @@ -641,6 +641,7 @@ + diff --git a/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodBase.cs b/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodBase.cs index a5c09ee0c40a6..cc89e57832384 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodBase.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodBase.cs @@ -143,7 +143,7 @@ private protected void ValidateInvokeTarget(object? target) private protected unsafe void CheckArguments( Span copyOfParameters, IntPtr* byrefParameters, - Span shouldCopyBack, + Span shouldCopyBack, ReadOnlySpan parameters, RuntimeType[] sigTypes, Binder? binder, @@ -156,7 +156,7 @@ BindingFlags invokeAttr ParameterInfo[]? paramInfos = null; for (int i = 0; i < parameters.Length; i++) { - bool copyBackArg = false; + ParameterCopyBackAction copyBackArg = default; bool isValueType = false; object? arg = parameters[i]; RuntimeType sigType = sigTypes[i]; @@ -182,7 +182,7 @@ BindingFlags invokeAttr else if (sigType.TryByRefFastPath(ref arg, ref isValueType)) { // Fast path when the value's type matches the signature type of a byref parameter. - copyBackArg = true; + copyBackArg = ParameterCopyBackAction.Copy; } else if (!ReferenceEquals(arg, Type.Missing)) { @@ -262,11 +262,11 @@ private protected ref struct StackAllocedArguments private object? _arg2; private object? _arg3; #pragma warning restore CA1823, CS0169, IDE0051 - internal bool _copyBack0; + internal ParameterCopyBackAction _copyBack0; #pragma warning disable CA1823, CS0169, IDE0051 // accessed via 'CheckArguments' ref arithmetic - private bool _copyBack1; - private bool _copyBack2; - private bool _copyBack3; + private ParameterCopyBackAction _copyBack1; + private ParameterCopyBackAction _copyBack2; + private ParameterCopyBackAction _copyBack3; #pragma warning restore CA1823, CS0169, IDE0051 } diff --git a/src/libraries/System.Private.CoreLib/src/System/Reflection/ParameterCopyBackAction.cs b/src/libraries/System.Private.CoreLib/src/System/Reflection/ParameterCopyBackAction.cs new file mode 100644 index 0000000000000..e7a9692a7449a --- /dev/null +++ b/src/libraries/System.Private.CoreLib/src/System/Reflection/ParameterCopyBackAction.cs @@ -0,0 +1,15 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +namespace System.Reflection +{ + /// + /// Determines how an invoke parameter needs to be copied back to the caller's object[] parameters. + /// + internal enum ParameterCopyBackAction : byte + { + None = 0, + Copy = 1, + CopyNullable = 2 + } +} diff --git a/src/libraries/System.Private.CoreLib/src/System/Reflection/RuntimeConstructorInfo.cs b/src/libraries/System.Private.CoreLib/src/System/Reflection/RuntimeConstructorInfo.cs index 49b59196b2251..0ad353cfd38e9 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Reflection/RuntimeConstructorInfo.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Reflection/RuntimeConstructorInfo.cs @@ -141,8 +141,8 @@ internal void ThrowNoInvokeException() { Debug.Assert(parameters != null); StackAllocedArguments argStorage = default; - Span copyOfParameters = new Span(ref argStorage._arg0, argCount); - Span shouldCopyBackParameters = new Span(ref argStorage._copyBack0, argCount); + Span copyOfParameters = new(ref argStorage._arg0, argCount); + Span shouldCopyBackParameters = new(ref argStorage._copyBack0, argCount); StackAllocatedByRefs byrefStorage = default; IntPtr* pByRefStorage = (IntPtr*)&byrefStorage; @@ -166,9 +166,20 @@ internal void ThrowNoInvokeException() // Copy modified values out. This should be done only with ByRef or Type.Missing parameters. for (int i = 0; i < argCount; i++) { - if (shouldCopyBackParameters[i]) + ParameterCopyBackAction action = shouldCopyBackParameters[i]; + if (action != ParameterCopyBackAction.None) { - parameters[i] = RuntimeMethodHandle.ReboxFromNullable(copyOfParameters[i]); + if (action == ParameterCopyBackAction.Copy) + { + parameters[i] = copyOfParameters[i]; + } + else + { + Debug.Assert(action == ParameterCopyBackAction.CopyNullable); + Debug.Assert(copyOfParameters[i] != null); + Debug.Assert(((RuntimeType)copyOfParameters[i]!.GetType()).IsNullableOfT); + parameters[i] = RuntimeMethodHandle.ReboxFromNullable(copyOfParameters[i]); + } } } } @@ -191,15 +202,15 @@ private static unsafe void InvokeWithManyArguments( CultureInfo? culture) { object[] objHolder = new object[argCount]; - Span copyOfParameters = new Span(objHolder, 0, argCount); + Span copyOfParameters = new(objHolder, 0, argCount); // We don't check a max stack size since we are invoking a method which // naturally requires a stack size that is dependent on the arg count\size. IntPtr* pByRefStorage = stackalloc IntPtr[argCount]; Buffer.ZeroMemory((byte*)pByRefStorage, (uint)(argCount * sizeof(IntPtr))); - bool* boolHolder = stackalloc bool[argCount]; - Span shouldCopyBackParameters = new Span(boolHolder, argCount); + ParameterCopyBackAction* copyBackActions = stackalloc ParameterCopyBackAction[argCount]; + Span shouldCopyBackParameters = new(copyBackActions, argCount); GCFrameRegistration reg = new(pByRefStorage, (uint)argCount, areByRefs: true); @@ -230,9 +241,20 @@ private static unsafe void InvokeWithManyArguments( // Copy modified values out. This should be done only with ByRef or Type.Missing parameters. for (int i = 0; i < argCount; i++) { - if (shouldCopyBackParameters[i]) + ParameterCopyBackAction action = shouldCopyBackParameters[i]; + if (action != ParameterCopyBackAction.None) { - parameters[i] = RuntimeMethodHandle.ReboxFromNullable(copyOfParameters[i]); + if (action == ParameterCopyBackAction.Copy) + { + parameters[i] = copyOfParameters[i]; + } + else + { + Debug.Assert(action == ParameterCopyBackAction.CopyNullable); + Debug.Assert(copyOfParameters[i] != null); + Debug.Assert(((RuntimeType)copyOfParameters[i]!.GetType()).IsNullableOfT); + parameters[i] = RuntimeMethodHandle.ReboxFromNullable(copyOfParameters[i]); + } } } } @@ -272,8 +294,8 @@ public override object Invoke(BindingFlags invokeAttr, Binder? binder, object?[] { Debug.Assert(parameters != null); StackAllocedArguments argStorage = default; - Span copyOfParameters = new Span(ref argStorage._arg0, argCount); - Span shouldCopyBackParameters = new Span(ref argStorage._copyBack0, argCount); + Span copyOfParameters = new(ref argStorage._arg0, argCount); + Span shouldCopyBackParameters = new(ref argStorage._copyBack0, argCount); StackAllocatedByRefs byrefStorage = default; IntPtr* pByRefStorage = (IntPtr*)&byrefStorage; @@ -297,9 +319,20 @@ public override object Invoke(BindingFlags invokeAttr, Binder? binder, object?[] // Copy modified values out. This should be done only with ByRef or Type.Missing parameters. for (int i = 0; i < argCount; i++) { - if (shouldCopyBackParameters[i]) + ParameterCopyBackAction action = shouldCopyBackParameters[i]; + if (action != ParameterCopyBackAction.None) { - parameters[i] = RuntimeMethodHandle.ReboxFromNullable(copyOfParameters[i]); + if (action == ParameterCopyBackAction.Copy) + { + parameters[i] = copyOfParameters[i]; + } + else + { + Debug.Assert(action == ParameterCopyBackAction.CopyNullable); + Debug.Assert(copyOfParameters[i] != null); + Debug.Assert(((RuntimeType)copyOfParameters[i]!.GetType()).IsNullableOfT); + parameters[i] = RuntimeMethodHandle.ReboxFromNullable(copyOfParameters[i]); + } } } } @@ -324,15 +357,15 @@ public override object Invoke(BindingFlags invokeAttr, Binder? binder, object?[] Debug.Assert(parameters != null); object[] objHolder = new object[argCount]; - Span copyOfParameters = new Span(objHolder, 0, argCount); + Span copyOfParameters = new(objHolder, 0, argCount); // We don't check a max stack size since we are invoking a method which // naturally requires a stack size that is dependent on the arg count\size. IntPtr* pByRefStorage = stackalloc IntPtr[argCount]; Buffer.ZeroMemory((byte*)pByRefStorage, (uint)(argCount * sizeof(IntPtr))); - bool* boolHolder = stackalloc bool[argCount]; - Span shouldCopyBackParameters = new Span(boolHolder, argCount); + ParameterCopyBackAction* copyBackActions = stackalloc ParameterCopyBackAction[argCount]; + Span shouldCopyBackParameters = new(copyBackActions, argCount); GCFrameRegistration reg = new(pByRefStorage, (uint)argCount, areByRefs: true); @@ -364,9 +397,20 @@ public override object Invoke(BindingFlags invokeAttr, Binder? binder, object?[] // Copy modified values out. This should be done only with ByRef or Type.Missing parameters. for (int i = 0; i < argCount; i++) { - if (shouldCopyBackParameters[i]) + ParameterCopyBackAction action = shouldCopyBackParameters[i]; + if (action != ParameterCopyBackAction.None) { - parameters[i] = RuntimeMethodHandle.ReboxFromNullable(copyOfParameters[i]); + if (action == ParameterCopyBackAction.Copy) + { + parameters[i] = copyOfParameters[i]; + } + else + { + Debug.Assert(action == ParameterCopyBackAction.CopyNullable); + Debug.Assert(copyOfParameters[i] != null); + Debug.Assert(((RuntimeType)copyOfParameters[i]!.GetType()).IsNullableOfT); + parameters[i] = RuntimeMethodHandle.ReboxFromNullable(copyOfParameters[i]); + } } } diff --git a/src/libraries/System.Private.CoreLib/src/System/Reflection/RuntimeMethodInfo.cs b/src/libraries/System.Private.CoreLib/src/System/Reflection/RuntimeMethodInfo.cs index 72d22089d7e08..14b7372605d7d 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Reflection/RuntimeMethodInfo.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Reflection/RuntimeMethodInfo.cs @@ -137,8 +137,8 @@ private void ThrowNoInvokeException() { Debug.Assert(parameters != null); StackAllocedArguments argStorage = default; - Span copyOfParameters = new Span(ref argStorage._arg0, argCount); - Span shouldCopyBackParameters = new Span(ref argStorage._copyBack0, argCount); + Span copyOfParameters = new(ref argStorage._arg0, argCount); + Span shouldCopyBackParameters = new(ref argStorage._copyBack0, argCount); StackAllocatedByRefs byrefStorage = default; IntPtr* pByRefStorage = (IntPtr*)&byrefStorage; @@ -162,9 +162,20 @@ private void ThrowNoInvokeException() // Copy modified values out. This should be done only with ByRef or Type.Missing parameters. for (int i = 0; i < argCount; i++) { - if (shouldCopyBackParameters[i]) + ParameterCopyBackAction action = shouldCopyBackParameters[i]; + if (action != ParameterCopyBackAction.None) { - parameters[i] = RuntimeMethodHandle.ReboxFromNullable(copyOfParameters[i]); + if (action == ParameterCopyBackAction.Copy) + { + parameters[i] = copyOfParameters[i]; + } + else + { + Debug.Assert(action == ParameterCopyBackAction.CopyNullable); + Debug.Assert(copyOfParameters[i] != null); + Debug.Assert(((RuntimeType)copyOfParameters[i]!.GetType()).IsNullableOfT); + parameters[i] = RuntimeMethodHandle.ReboxFromNullable(copyOfParameters[i]); + } } } } @@ -187,15 +198,15 @@ private void ThrowNoInvokeException() CultureInfo? culture) { object[] objHolder = new object[argCount]; - Span copyOfParameters = new Span(objHolder, 0, argCount); + Span copyOfParameters = new(objHolder, 0, argCount); // We don't check a max stack size since we are invoking a method which // naturally requires a stack size that is dependent on the arg count\size. IntPtr* pByRefStorage = stackalloc IntPtr[argCount]; Buffer.ZeroMemory((byte*)pByRefStorage, (uint)(argCount * sizeof(IntPtr))); - bool* boolHolder = stackalloc bool[argCount]; - Span shouldCopyBackParameters = new Span(boolHolder, argCount); + ParameterCopyBackAction* copyBackActions = stackalloc ParameterCopyBackAction[argCount]; + Span shouldCopyBackParameters = new(copyBackActions, argCount); GCFrameRegistration reg = new(pByRefStorage, (uint)argCount, areByRefs: true); @@ -227,9 +238,20 @@ private void ThrowNoInvokeException() // Copy modified values out. This should be done only with ByRef or Type.Missing parameters. for (int i = 0; i < argCount; i++) { - if (shouldCopyBackParameters[i]) + ParameterCopyBackAction action = shouldCopyBackParameters[i]; + if (action != ParameterCopyBackAction.None) { - parameters[i] = RuntimeMethodHandle.ReboxFromNullable(copyOfParameters[i]); + if (action == ParameterCopyBackAction.Copy) + { + parameters[i] = copyOfParameters[i]; + } + else + { + Debug.Assert(action == ParameterCopyBackAction.CopyNullable); + Debug.Assert(copyOfParameters[i] != null); + Debug.Assert(((RuntimeType)copyOfParameters[i]!.GetType()).IsNullableOfT); + parameters[i] = RuntimeMethodHandle.ReboxFromNullable(copyOfParameters[i]); + } } } From c7bee670f7e3fa5aa5994b0ef8694d1089c9e846 Mon Sep 17 00:00:00 2001 From: Steve Harter Date: Fri, 6 May 2022 14:50:32 -0500 Subject: [PATCH 21/21] For perf, avoid calling ReboxFromNullable when not necessary part 2 --- .../src/System/RuntimeType.CoreCLR.cs | 12 +++++++++++- .../src/System/Reflection/RuntimeFieldInfo.cs | 2 +- .../src/System/RuntimeType.Mono.cs | 4 ++-- 3 files changed, 14 insertions(+), 4 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/RuntimeType.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/RuntimeType.CoreCLR.cs index 2ca4dd8f3744b..374cdcf662b84 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/RuntimeType.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/RuntimeType.CoreCLR.cs @@ -3555,7 +3555,17 @@ internal bool CheckValue( value = binder.ChangeType(value, this, culture); if (IsInstanceOfType(value)) { - copyBack = ParameterCopyBackAction.Copy; + if (IsNullableOfT) + { + // Pass as a true boxed Nullable, not as a T or null. + value = RuntimeMethodHandle.ReboxToNullable(value, this); + copyBack = ParameterCopyBackAction.CopyNullable; + } + else + { + copyBack = ParameterCopyBackAction.Copy; + } + return IsValueType; // Note the call to IsValueType, not the variable. } diff --git a/src/mono/System.Private.CoreLib/src/System/Reflection/RuntimeFieldInfo.cs b/src/mono/System.Private.CoreLib/src/System/Reflection/RuntimeFieldInfo.cs index 8d6c46f153ca2..73aa4744e8f56 100644 --- a/src/mono/System.Private.CoreLib/src/System/Reflection/RuntimeFieldInfo.cs +++ b/src/mono/System.Private.CoreLib/src/System/Reflection/RuntimeFieldInfo.cs @@ -278,7 +278,7 @@ public override void SetValue(object? obj, object? val, BindingFlags invokeAttr, if (val != null) { RuntimeType fieldType = (RuntimeType)FieldType; - bool _ = false; + ParameterCopyBackAction _ = default; if (!ReferenceEquals(val.GetType(), fieldType)) { diff --git a/src/mono/System.Private.CoreLib/src/System/RuntimeType.Mono.cs b/src/mono/System.Private.CoreLib/src/System/RuntimeType.Mono.cs index 3d601e552f174..91ba5db495c05 100644 --- a/src/mono/System.Private.CoreLib/src/System/RuntimeType.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/RuntimeType.Mono.cs @@ -1648,7 +1648,7 @@ internal override FieldInfo GetField(FieldInfo fromNoninstanciated) /// Not yet implemented in Mono: True if the value should be considered a value type, False otherwise internal bool CheckValue( ref object? value, - ref bool copyBack, + ref ParameterCopyBackAction copyBack, Binder? binder, CultureInfo? culture, BindingFlags invokeAttr) @@ -1656,7 +1656,7 @@ internal bool CheckValue( // Already fast-pathed by the caller. Debug.Assert(!ReferenceEquals(value?.GetType(), this)); - copyBack = true; + copyBack = ParameterCopyBackAction.Copy; CheckValueStatus status = TryConvertToType(ref value); if (status == CheckValueStatus.Success)