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 8ac8a44be9448..7f34d5438c6be 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 @@ -212,17 +212,17 @@ private void InvokeClassConstructor() } [MethodImpl(MethodImplOptions.AggressiveInlining)] - private object? InvokeWorker(object? obj, BindingFlags invokeAttr, in Span arguments) + private object? InvokeWorker(object? obj, BindingFlags invokeAttr, Span arguments) { bool wrapExceptions = (invokeAttr & BindingFlags.DoNotWrapExceptions) == 0; - return RuntimeMethodHandle.InvokeMethod(obj, arguments, Signature, false, wrapExceptions); + return RuntimeMethodHandle.InvokeMethod(obj, in arguments, Signature, false, wrapExceptions); } [MethodImpl(MethodImplOptions.AggressiveInlining)] - private object InvokeCtorWorker(BindingFlags invokeAttr, in Span arguments) + private object InvokeCtorWorker(BindingFlags invokeAttr, Span arguments) { bool wrapExceptions = (invokeAttr & BindingFlags.DoNotWrapExceptions) == 0; - return RuntimeMethodHandle.InvokeMethod(null, arguments, Signature, true, wrapExceptions)!; + return RuntimeMethodHandle.InvokeMethod(null, in arguments, Signature, true, wrapExceptions)!; } [RequiresUnreferencedCode("Trimming may change method bodies. For example it can change some instructions, remove branches or local variables.")] 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 4710e6d3b7e6e..68c09394e9275 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 @@ -316,10 +316,10 @@ public override MethodImplAttributes GetMethodImplementationFlags() #region Invocation Logic(On MemberBase) [MethodImpl(MethodImplOptions.AggressiveInlining)] - private object? InvokeWorker(object? obj, BindingFlags invokeAttr, in Span arguments) + private object? InvokeWorker(object? obj, BindingFlags invokeAttr, Span arguments) { bool wrapExceptions = (invokeAttr & BindingFlags.DoNotWrapExceptions) == 0; - return RuntimeMethodHandle.InvokeMethod(obj, arguments, Signature, false, wrapExceptions); + return RuntimeMethodHandle.InvokeMethod(obj, in arguments, Signature, false, wrapExceptions); } [DebuggerStepThroughAttribute] diff --git a/src/coreclr/vm/jitinterface.cpp b/src/coreclr/vm/jitinterface.cpp index 43c4fc17a597f..1dc6e15da1e30 100644 --- a/src/coreclr/vm/jitinterface.cpp +++ b/src/coreclr/vm/jitinterface.cpp @@ -3627,9 +3627,9 @@ bool CEEInfo::isStructRequiringStackAllocRetBuf(CORINFO_CLASS_HANDLE clsHnd) JIT_TO_EE_TRANSITION_LEAF(); - TypeHandle VMClsHnd(clsHnd); - MethodTable * pMT = VMClsHnd.GetMethodTable(); - ret = (pMT != NULL && pMT->IsStructRequiringStackAllocRetBuf()); + // Disable this optimization. It has limited value (only kicks in on x86, and only for less common structs), + // causes bugs and introduces odd ABI differences not compatible with ReadyToRun. + ret = false; EE_TO_JIT_TRANSITION_LEAF(); diff --git a/src/coreclr/vm/methodtable.cpp b/src/coreclr/vm/methodtable.cpp index a685151f9838d..1d63103cb36f1 100644 --- a/src/coreclr/vm/methodtable.cpp +++ b/src/coreclr/vm/methodtable.cpp @@ -6002,20 +6002,6 @@ BOOL MethodTable::SanityCheck() return (pCanonMT == this) || IsArray(); } -//========================================================================================== - -// Structs containing GC pointers whose size is at most this are always stack-allocated. -const unsigned MaxStructBytesForLocalVarRetBuffBytes = 2 * sizeof(void*); // 4 pointer-widths. - -BOOL MethodTable::IsStructRequiringStackAllocRetBuf() -{ - LIMITED_METHOD_DAC_CONTRACT; - - // Disable this optimization. It has limited value (only kicks in on x86, and only for less common structs), - // causes bugs and introduces odd ABI differences not compatible with ReadyToRun. - return FALSE; -} - //========================================================================================== unsigned MethodTable::GetTypeDefRid() { diff --git a/src/coreclr/vm/methodtable.h b/src/coreclr/vm/methodtable.h index 1fa0d1be43452..6c3c89e7710af 100644 --- a/src/coreclr/vm/methodtable.h +++ b/src/coreclr/vm/methodtable.h @@ -2511,14 +2511,6 @@ class MethodTable // Is this value type? Returns false for System.ValueType and System.Enum. inline BOOL IsValueType(); - // Returns "TRUE" iff "this" is a struct type such that return buffers used for returning a value - // of this type must be stack-allocated. This will generally be true only if the struct - // contains GC pointers, and does not exceed some size limit. Maintaining this as an invariant allows - // an optimization: the JIT may assume that return buffer pointers for return types for which this predicate - // returns TRUE are always stack allocated, and thus, that stores to the GC-pointer fields of such return - // buffers do not require GC write barriers. - BOOL IsStructRequiringStackAllocRetBuf(); - // Is this enum? Returns false for System.Enum. inline BOOL IsEnum(); diff --git a/src/coreclr/vm/reflectioninvocation.cpp b/src/coreclr/vm/reflectioninvocation.cpp index ebf13351d4c06..68268942e562f 100644 --- a/src/coreclr/vm/reflectioninvocation.cpp +++ b/src/coreclr/vm/reflectioninvocation.cpp @@ -451,7 +451,7 @@ struct ByRefToNullable { } }; -void CallDescrWorkerReflectionWrapper(CallDescrData * pCallDescrData, Frame * pFrame) +static void CallDescrWorkerReflectionWrapper(CallDescrData * pCallDescrData, Frame * pFrame) { // Use static contracts b/c we have SEH. STATIC_CONTRACT_THROWS; @@ -478,7 +478,7 @@ void CallDescrWorkerReflectionWrapper(CallDescrData * pCallDescrData, Frame * pF PAL_ENDTRY } // CallDescrWorkerReflectionWrapper -OBJECTREF InvokeArrayConstructor(TypeHandle th, MethodDesc* pMeth, Span* objs, int argCnt) +static OBJECTREF InvokeArrayConstructor(TypeHandle th, Span* objs, int argCnt) { CONTRACTL { THROWS; @@ -510,7 +510,9 @@ OBJECTREF InvokeArrayConstructor(TypeHandle th, MethodDesc* pMeth, SpanGetAt(i)->UnBox(),pMT->GetNumInstanceFieldBytes()); + ARG_SLOT value; + InvokeUtil::CreatePrimitiveValue(ELEMENT_TYPE_I4, oType, objs->GetAt(i), &value); + memcpyNoGCRefs(indexes + i, ArgSlotEndianessFixup(&value, sizeof(INT32)), sizeof(INT32)); } return AllocateArrayEx(th, indexes, argCnt); @@ -769,8 +771,6 @@ FCIMPL5(Object*, RuntimeMethodHandle::InvokeMethod, HELPER_METHOD_FRAME_BEGIN_RET_PROTECT(gc); - Assembly *pAssem = pMeth->GetAssembly(); - if (ownerType.IsSharedByGenericInstantiations()) COMPlusThrow(kNotSupportedException, W("NotSupported_Type")); @@ -786,23 +786,20 @@ FCIMPL5(Object*, RuntimeMethodHandle::InvokeMethod, if (fConstructor) { // If we are invoking a constructor on an array then we must - // handle this specially. String objects allocate themselves - // so they are a special case. + // handle this specially. if (ownerType.IsArray()) { gc.retVal = InvokeArrayConstructor(ownerType, - pMeth, objs, gc.pSig->NumFixedArgs()); goto Done; } + // Variable sized objects, like String instances, allocate themselves + // so they are a special case. MethodTable * pMT = ownerType.AsMethodTable(); - - { - fCtorOfVariableSizedObject = pMT->HasComponentSize(); - if (!fCtorOfVariableSizedObject) - gc.retVal = pMT->Allocate(); - } + fCtorOfVariableSizedObject = pMT->HasComponentSize(); + if (!fCtorOfVariableSizedObject) + gc.retVal = pMT->Allocate(); } { @@ -936,8 +933,6 @@ FCIMPL5(Object*, RuntimeMethodHandle::InvokeMethod, // If an exception occurs a gc may happen but we are going to dump the stack anyway and we do // not need to protect anything. - PVOID pRetBufStackCopy = NULL; - { BEGINFORBIDGC(); #ifdef _DEBUG @@ -947,24 +942,8 @@ FCIMPL5(Object*, RuntimeMethodHandle::InvokeMethod, // Take care of any return arguments if (fHasRetBuffArg) { - // We stack-allocate this ret buff, to preserve the invariant that ret-buffs are always in the - // caller's stack frame. We'll copy into gc.retVal later. - TypeHandle retTH = gc.pSig->GetReturnTypeHandle(); - MethodTable* pMT = retTH.GetMethodTable(); - if (pMT->IsStructRequiringStackAllocRetBuf()) - { - SIZE_T sz = pMT->GetNumInstanceFieldBytes(); - pRetBufStackCopy = _alloca(sz); - memset(pRetBufStackCopy, 0, sz); - - pValueClasses = new (_alloca(sizeof(ValueClassInfo))) ValueClassInfo(pRetBufStackCopy, pMT, pValueClasses); - *((LPVOID*) (pTransitionBlock + argit.GetRetBuffArgOffset())) = pRetBufStackCopy; - } - else - { - PVOID pRetBuff = gc.retVal->GetData(); - *((LPVOID*) (pTransitionBlock + argit.GetRetBuffArgOffset())) = pRetBuff; - } + PVOID pRetBuff = gc.retVal->GetData(); + *((LPVOID*) (pTransitionBlock + argit.GetRetBuffArgOffset())) = pRetBuff; } // copy args @@ -1128,10 +1107,6 @@ FCIMPL5(Object*, RuntimeMethodHandle::InvokeMethod, { CopyValueClass(gc.retVal->GetData(), &callDescrData.returnValue, gc.retVal->GetMethodTable()); } - else if (pRetBufStackCopy) - { - CopyValueClass(gc.retVal->GetData(), pRetBufStackCopy, gc.retVal->GetMethodTable()); - } // From here on out, it is OK to have GCs since the return object (which may have had // GC pointers has been put into a GC object and thus protected. 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 e3502601407cf..8ee3b14a658cc 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Reflection/RuntimeConstructorInfo.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Reflection/RuntimeConstructorInfo.cs @@ -105,6 +105,13 @@ internal void ThrowNoInvokeException() ValidateInvokeTarget(obj); + // Correct number of arguments supplied + int actualCount = (parameters is null) ? 0 : parameters.Length; + if (ArgumentTypes.Length != actualCount) + { + throw new TargetParameterCountException(SR.Arg_ParmCnt); + } + if ((InvocationFlags & InvocationFlags.RunClassConstructor) != 0) { // Run the class constructor through the class constructor mechanism instead of the Invoke path. @@ -113,13 +120,6 @@ internal void ThrowNoInvokeException() return null; } - // Correct number of arguments supplied - int actualCount = (parameters is null) ? 0 : parameters.Length; - if (ArgumentTypes.Length != actualCount) - { - throw new TargetParameterCountException(SR.Arg_ParmCnt); - } - StackAllocedArguments stackArgs = default; Span arguments = default; if (actualCount != 0) diff --git a/src/libraries/System.Reflection/tests/Common.cs b/src/libraries/System.Reflection/tests/Common.cs index 2c9e65368ec21..e70336e7e330c 100644 --- a/src/libraries/System.Reflection/tests/Common.cs +++ b/src/libraries/System.Reflection/tests/Common.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. +using System.Globalization; + namespace System.Reflection.Tests { public enum PublicEnum @@ -105,4 +107,25 @@ public class Helpers { public static Assembly ExecutingAssembly => typeof(Helpers).GetTypeInfo().Assembly; } + + public class ConvertStringToIntBinder : Binder + { + public override FieldInfo BindToField(BindingFlags bindingAttr, FieldInfo[] match, object value, CultureInfo? culture) + => throw new NotImplementedException(); + + public override MethodBase BindToMethod(BindingFlags bindingAttr, MethodBase[] match, ref object?[] args, ParameterModifier[]? modifiers, CultureInfo? culture, string[]? names, out object? state) + => throw new NotImplementedException(); + + public override object ChangeType(object value, Type type, CultureInfo? culture) + => int.Parse((string)value); + + public override void ReorderArgumentArray(ref object?[] args, object state) + => throw new NotImplementedException(); + + public override MethodBase? SelectMethod(BindingFlags bindingAttr, MethodBase[] match, Type[] types, ParameterModifier[]? modifiers) + => throw new NotImplementedException(); + + public override PropertyInfo? SelectProperty(BindingFlags bindingAttr, PropertyInfo[] match, Type? returnType, Type[]? indexes, ParameterModifier[]? modifiers) + => throw new NotImplementedException(); + } } diff --git a/src/libraries/System.Reflection/tests/ConstructorInfoTests.cs b/src/libraries/System.Reflection/tests/ConstructorInfoTests.cs index a104245281074..ea861aa1dcdeb 100644 --- a/src/libraries/System.Reflection/tests/ConstructorInfoTests.cs +++ b/src/libraries/System.Reflection/tests/ConstructorInfoTests.cs @@ -126,6 +126,17 @@ public void Invoke_OneDimensionalArray_NegativeLengths_ThrowsOverflowException() } } + [Fact] + public void Invoke_TwoDimensionalArray_CustomBinder_IncorrectTypeArguments() + { + var ctor = typeof(int[,]).GetConstructor(new[] { typeof(int), typeof(int) }); + var args = new object[] { "1", "2" }; + var arr = (int[,])ctor.Invoke(BindingFlags.Default, new ConvertStringToIntBinder(), args, null); + Assert.Equal(2, arr.Length); + Assert.True(args[0] is int); + Assert.True(args[1] is int); + } + [Fact] public void Invoke_OneParameter() { @@ -143,6 +154,19 @@ public void Invoke_TwoParameters() Assert.Equal("hello", obj.stringValue); } + [Fact] + public void Invoke_TwoParameters_CustomBinder_IncorrectTypeArgument() + { + ConstructorInfo[] constructors = GetConstructors(typeof(ClassWith3Constructors)); + + var args = new object[] { "101", "hello" }; + ClassWith3Constructors obj = (ClassWith3Constructors)constructors[2].Invoke(BindingFlags.Default, new ConvertStringToIntBinder(), args, null); + Assert.Equal(101, obj.intValue); + Assert.Equal("hello", obj.stringValue); + Assert.True(args[0] is int); + Assert.True(args[1] is string); + } + [Fact] public void Invoke_NoParameters_ThowsTargetParameterCountException() { @@ -248,8 +272,6 @@ public ClassWith3Constructors(int intValue, string stringValue) this.intValue = intValue; this.stringValue = stringValue; } - - public string Method1(DateTime dt) => ""; } public static class ClassWithStaticConstructor diff --git a/src/libraries/System.Reflection/tests/MethodInfoTests.cs b/src/libraries/System.Reflection/tests/MethodInfoTests.cs index b8abc6e9118eb..a538fc9f3a9c9 100644 --- a/src/libraries/System.Reflection/tests/MethodInfoTests.cs +++ b/src/libraries/System.Reflection/tests/MethodInfoTests.cs @@ -381,6 +381,16 @@ public static void Invoke_OptionalParameterUnassingableFromMissing_WithMissingVa AssertExtensions.Throws(null, () => GetMethod(typeof(MethodInfoDefaultParameters), "OptionalStringParameter").Invoke(new MethodInfoDefaultParameters(), new object[] { Type.Missing })); } + [Fact] + public void Invoke_TwoParameters_CustomBinder_IncorrectTypeArguments() + { + MethodInfo method = GetMethod(typeof(MI_SubClass), nameof(MI_SubClass.StaticIntIntMethodReturningInt)); + var args = new object[] { "10", "100" }; + Assert.Equal(110, method.Invoke(null, BindingFlags.Default, new ConvertStringToIntBinder(), args, null)); + Assert.True(args[0] is int); + Assert.True(args[1] is int); + } + [Theory] [InlineData(typeof(MI_SubClass), nameof(MI_SubClass.GenericMethod1), new Type[] { typeof(int) })] [InlineData(typeof(MI_SubClass), nameof(MI_SubClass.GenericMethod2), new Type[] { typeof(string), typeof(int) })] 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 1c6e10f1db233..b715e4e386962 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 @@ -374,7 +374,7 @@ private RuntimeType[] ArgumentTypes internal extern object? InternalInvoke(object? obj, in Span parameters, out Exception? exc); [MethodImpl(MethodImplOptions.AggressiveInlining)] - private object? InvokeWorker(object? obj, BindingFlags invokeAttr, in Span parameters) + private object? InvokeWorker(object? obj, BindingFlags invokeAttr, Span parameters) { Exception? exc; object? o = null; @@ -862,7 +862,7 @@ private void InvokeClassConstructor() } [MethodImpl(MethodImplOptions.AggressiveInlining)] - internal object? InvokeWorker(object? obj, BindingFlags invokeAttr, in Span arguments) + internal object? InvokeWorker(object? obj, BindingFlags invokeAttr, Span arguments) { bool wrapExceptions = (invokeAttr & BindingFlags.DoNotWrapExceptions) == 0; return InternalInvoke(obj, arguments, wrapExceptions); diff --git a/src/mono/mono/metadata/icall.c b/src/mono/mono/metadata/icall.c index 43d826dd67c6f..536c6cf617b20 100644 --- a/src/mono/mono/metadata/icall.c +++ b/src/mono/mono/metadata/icall.c @@ -3373,9 +3373,6 @@ ves_icall_InternalInvoke (MonoReflectionMethodHandle method_handle, MonoObjectHa MonoMethodSignature* const sig = mono_method_signature_internal (m); int pcount = 0; void *obj = this_arg; - char *this_name = NULL; - char *target_name = NULL; - char *msg = NULL; MonoObject *result = NULL; MonoArray *arr = NULL; MonoException *exception = NULL; @@ -3403,6 +3400,7 @@ ves_icall_InternalInvoke (MonoReflectionMethodHandle method_handle, MonoObjectHa } } + /* Array constructor */ if (m_class_get_rank (m->klass) && !strcmp (m->name, ".ctor")) { int i; pcount = mono_span_length (params_span); @@ -3436,12 +3434,12 @@ ves_icall_InternalInvoke (MonoReflectionMethodHandle method_handle, MonoObjectHa g_assert (pcount == (m_class_get_rank (m->klass) * 2)); /* The arguments are lower-bound-length pairs */ intptr_t * const lower_bounds = (intptr_t *)g_alloca (sizeof (intptr_t) * pcount); - + for (i = 0; i < pcount / 2; ++i) { lower_bounds [i] = *(int32_t*) ((char*)mono_span_get (params_span, MonoObject*, (i * 2)) + sizeof (MonoObject)); lengths [i] = *(int32_t*) ((char*)mono_span_get (params_span, MonoObject*, (i * 2) + 1) + sizeof (MonoObject)); } - + arr = mono_array_new_full_checked (m->klass, lengths, lower_bounds, error); goto_if_nok (error, return_null); goto exit; @@ -3457,9 +3455,6 @@ ves_icall_InternalInvoke (MonoReflectionMethodHandle method_handle, MonoObjectHa MONO_HANDLE_NEW (MonoException, exception); // FIXME? overkill? mono_gc_wbarrier_generic_store_internal (MONO_HANDLE_REF (exception_out), (MonoObject*)exception); } - g_free (target_name); - g_free (this_name); - g_free (msg); g_assert (!result || !arr); // only one, or neither, should be set return result ? MONO_HANDLE_NEW (MonoObject, result) : arr ? MONO_HANDLE_NEW (MonoObject, (MonoObject*)arr) : NULL_HANDLE; } diff --git a/src/mono/mono/metadata/object-internals.h b/src/mono/mono/metadata/object-internals.h index 361a6dbcc01dc..4a47bd81ddfb6 100644 --- a/src/mono/mono/metadata/object-internals.h +++ b/src/mono/mono/metadata/object-internals.h @@ -176,10 +176,13 @@ struct _MonoArray { }; /* match the layout of the managed definition of Span */ -typedef struct { - MonoObject** _pointer; - int32_t _length; -} MonoSpanOfObjects; +#define MONO_DEFINE_SPAN_OF_T(name, type) \ + typedef struct { \ + type* _pointer; \ + uint32_t _length; \ + } name; + +MONO_DEFINE_SPAN_OF_T (MonoSpanOfObjects, MonoObject*) #define MONO_SIZEOF_MONO_ARRAY (MONO_STRUCT_OFFSET_CONSTANT (MonoArray, vector)) @@ -284,7 +287,7 @@ mono_handle_array_get_bounds_dim (MonoArrayHandle arr, gint32 dim, MonoArrayBoun #define mono_span_length(span) (span->_length) -#define mono_span_get(span,type,idx) (type)(!span->_pointer ? NULL : span->_pointer[idx]) +#define mono_span_get(span,type,idx) (type)(!span->_pointer ? (type)0 : span->_pointer[idx]) #define mono_span_addr(span,type,idx) (type*)(span->_pointer + idx)