Skip to content

Commit ff82b6d

Browse files
AaronRobinsonMSFTAaronRobinsonMSFT
authored
Fix Array ctor integer widening and add Reflection tests (#61347)
* coreclr/ Make sure integral types respect sign extension during widen operation for Invoke Array ctor. Remove always false IsStructRequiringStackAllocRetBuf(). * mono/ Create macro define for SPAN_T. * libraries/ Add tests for Reflection Binder type conversion support during Invoke.
1 parent fd64e64 commit ff82b6d

File tree

13 files changed

+99
-93
lines changed

13 files changed

+99
-93
lines changed

‎src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeConstructorInfo.CoreCLR.cs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -212,17 +212,17 @@ private void InvokeClassConstructor()
212212
}
213213

214214
[MethodImpl(MethodImplOptions.AggressiveInlining)]
215-
private object? InvokeWorker(object? obj, BindingFlags invokeAttr, in Span<object?> arguments)
215+
private object? InvokeWorker(object? obj, BindingFlags invokeAttr, Span<object?> arguments)
216216
{
217217
bool wrapExceptions = (invokeAttr & BindingFlags.DoNotWrapExceptions) == 0;
218-
return RuntimeMethodHandle.InvokeMethod(obj, arguments, Signature, false, wrapExceptions);
218+
return RuntimeMethodHandle.InvokeMethod(obj, in arguments, Signature, false, wrapExceptions);
219219
}
220220

221221
[MethodImpl(MethodImplOptions.AggressiveInlining)]
222-
private object InvokeCtorWorker(BindingFlags invokeAttr, in Span<object?> arguments)
222+
private object InvokeCtorWorker(BindingFlags invokeAttr, Span<object?> arguments)
223223
{
224224
bool wrapExceptions = (invokeAttr & BindingFlags.DoNotWrapExceptions) == 0;
225-
return RuntimeMethodHandle.InvokeMethod(null, arguments, Signature, true, wrapExceptions)!;
225+
return RuntimeMethodHandle.InvokeMethod(null, in arguments, Signature, true, wrapExceptions)!;
226226
}
227227

228228
[RequiresUnreferencedCode("Trimming may change method bodies. For example it can change some instructions, remove branches or local variables.")]

‎src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeMethodInfo.CoreCLR.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -316,10 +316,10 @@ public override MethodImplAttributes GetMethodImplementationFlags()
316316

317317
#region Invocation Logic(On MemberBase)
318318
[MethodImpl(MethodImplOptions.AggressiveInlining)]
319-
private object? InvokeWorker(object? obj, BindingFlags invokeAttr, in Span<object?> arguments)
319+
private object? InvokeWorker(object? obj, BindingFlags invokeAttr, Span<object?> arguments)
320320
{
321321
bool wrapExceptions = (invokeAttr & BindingFlags.DoNotWrapExceptions) == 0;
322-
return RuntimeMethodHandle.InvokeMethod(obj, arguments, Signature, false, wrapExceptions);
322+
return RuntimeMethodHandle.InvokeMethod(obj, in arguments, Signature, false, wrapExceptions);
323323
}
324324

325325
[DebuggerStepThroughAttribute]

‎src/coreclr/vm/jitinterface.cpp

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3627,9 +3627,9 @@ bool CEEInfo::isStructRequiringStackAllocRetBuf(CORINFO_CLASS_HANDLE clsHnd)
36273627

36283628
JIT_TO_EE_TRANSITION_LEAF();
36293629

3630-
TypeHandle VMClsHnd(clsHnd);
3631-
MethodTable * pMT = VMClsHnd.GetMethodTable();
3632-
ret = (pMT != NULL && pMT->IsStructRequiringStackAllocRetBuf());
3630+
// Disable this optimization. It has limited value (only kicks in on x86, and only for less common structs),
3631+
// causes bugs and introduces odd ABI differences not compatible with ReadyToRun.
3632+
ret = false;
36333633

36343634
EE_TO_JIT_TRANSITION_LEAF();
36353635

‎src/coreclr/vm/methodtable.cpp

Lines changed: 0 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -6002,20 +6002,6 @@ BOOL MethodTable::SanityCheck()
60026002
return (pCanonMT == this) || IsArray();
60036003
}
60046004

6005-
//==========================================================================================
6006-
6007-
// Structs containing GC pointers whose size is at most this are always stack-allocated.
6008-
const unsigned MaxStructBytesForLocalVarRetBuffBytes = 2 * sizeof(void*); // 4 pointer-widths.
6009-
6010-
BOOL MethodTable::IsStructRequiringStackAllocRetBuf()
6011-
{
6012-
LIMITED_METHOD_DAC_CONTRACT;
6013-
6014-
// Disable this optimization. It has limited value (only kicks in on x86, and only for less common structs),
6015-
// causes bugs and introduces odd ABI differences not compatible with ReadyToRun.
6016-
return FALSE;
6017-
}
6018-
60196005
//==========================================================================================
60206006
unsigned MethodTable::GetTypeDefRid()
60216007
{

‎src/coreclr/vm/methodtable.h

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2511,14 +2511,6 @@ class MethodTable
25112511
// Is this value type? Returns false for System.ValueType and System.Enum.
25122512
inline BOOL IsValueType();
25132513

2514-
// Returns "TRUE" iff "this" is a struct type such that return buffers used for returning a value
2515-
// of this type must be stack-allocated. This will generally be true only if the struct
2516-
// contains GC pointers, and does not exceed some size limit. Maintaining this as an invariant allows
2517-
// an optimization: the JIT may assume that return buffer pointers for return types for which this predicate
2518-
// returns TRUE are always stack allocated, and thus, that stores to the GC-pointer fields of such return
2519-
// buffers do not require GC write barriers.
2520-
BOOL IsStructRequiringStackAllocRetBuf();
2521-
25222514
// Is this enum? Returns false for System.Enum.
25232515
inline BOOL IsEnum();
25242516

‎src/coreclr/vm/reflectioninvocation.cpp

Lines changed: 13 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -451,7 +451,7 @@ struct ByRefToNullable {
451451
}
452452
};
453453

454-
void CallDescrWorkerReflectionWrapper(CallDescrData * pCallDescrData, Frame * pFrame)
454+
static void CallDescrWorkerReflectionWrapper(CallDescrData * pCallDescrData, Frame * pFrame)
455455
{
456456
// Use static contracts b/c we have SEH.
457457
STATIC_CONTRACT_THROWS;
@@ -478,7 +478,7 @@ void CallDescrWorkerReflectionWrapper(CallDescrData * pCallDescrData, Frame * pF
478478
PAL_ENDTRY
479479
} // CallDescrWorkerReflectionWrapper
480480

481-
OBJECTREF InvokeArrayConstructor(TypeHandle th, MethodDesc* pMeth, Span<OBJECTREF>* objs, int argCnt)
481+
static OBJECTREF InvokeArrayConstructor(TypeHandle th, Span<OBJECTREF>* objs, int argCnt)
482482
{
483483
CONTRACTL {
484484
THROWS;
@@ -510,7 +510,9 @@ OBJECTREF InvokeArrayConstructor(TypeHandle th, MethodDesc* pMeth, Span<OBJECTRE
510510
if (!InvokeUtil::IsPrimitiveType(oType) || !InvokeUtil::CanPrimitiveWiden(ELEMENT_TYPE_I4,oType))
511511
COMPlusThrow(kArgumentException,W("Arg_PrimWiden"));
512512

513-
memcpy(&indexes[i], objs->GetAt(i)->UnBox(),pMT->GetNumInstanceFieldBytes());
513+
ARG_SLOT value;
514+
InvokeUtil::CreatePrimitiveValue(ELEMENT_TYPE_I4, oType, objs->GetAt(i), &value);
515+
memcpyNoGCRefs(indexes + i, ArgSlotEndianessFixup(&value, sizeof(INT32)), sizeof(INT32));
514516
}
515517

516518
return AllocateArrayEx(th, indexes, argCnt);
@@ -769,8 +771,6 @@ FCIMPL5(Object*, RuntimeMethodHandle::InvokeMethod,
769771

770772
HELPER_METHOD_FRAME_BEGIN_RET_PROTECT(gc);
771773

772-
Assembly *pAssem = pMeth->GetAssembly();
773-
774774
if (ownerType.IsSharedByGenericInstantiations())
775775
COMPlusThrow(kNotSupportedException, W("NotSupported_Type"));
776776

@@ -786,23 +786,20 @@ FCIMPL5(Object*, RuntimeMethodHandle::InvokeMethod,
786786
if (fConstructor)
787787
{
788788
// If we are invoking a constructor on an array then we must
789-
// handle this specially. String objects allocate themselves
790-
// so they are a special case.
789+
// handle this specially.
791790
if (ownerType.IsArray()) {
792791
gc.retVal = InvokeArrayConstructor(ownerType,
793-
pMeth,
794792
objs,
795793
gc.pSig->NumFixedArgs());
796794
goto Done;
797795
}
798796

797+
// Variable sized objects, like String instances, allocate themselves
798+
// so they are a special case.
799799
MethodTable * pMT = ownerType.AsMethodTable();
800-
801-
{
802-
fCtorOfVariableSizedObject = pMT->HasComponentSize();
803-
if (!fCtorOfVariableSizedObject)
804-
gc.retVal = pMT->Allocate();
805-
}
800+
fCtorOfVariableSizedObject = pMT->HasComponentSize();
801+
if (!fCtorOfVariableSizedObject)
802+
gc.retVal = pMT->Allocate();
806803
}
807804

808805
{
@@ -936,8 +933,6 @@ FCIMPL5(Object*, RuntimeMethodHandle::InvokeMethod,
936933
// If an exception occurs a gc may happen but we are going to dump the stack anyway and we do
937934
// not need to protect anything.
938935

939-
PVOID pRetBufStackCopy = NULL;
940-
941936
{
942937
BEGINFORBIDGC();
943938
#ifdef _DEBUG
@@ -947,24 +942,8 @@ FCIMPL5(Object*, RuntimeMethodHandle::InvokeMethod,
947942
// Take care of any return arguments
948943
if (fHasRetBuffArg)
949944
{
950-
// We stack-allocate this ret buff, to preserve the invariant that ret-buffs are always in the
951-
// caller's stack frame. We'll copy into gc.retVal later.
952-
TypeHandle retTH = gc.pSig->GetReturnTypeHandle();
953-
MethodTable* pMT = retTH.GetMethodTable();
954-
if (pMT->IsStructRequiringStackAllocRetBuf())
955-
{
956-
SIZE_T sz = pMT->GetNumInstanceFieldBytes();
957-
pRetBufStackCopy = _alloca(sz);
958-
memset(pRetBufStackCopy, 0, sz);
959-
960-
pValueClasses = new (_alloca(sizeof(ValueClassInfo))) ValueClassInfo(pRetBufStackCopy, pMT, pValueClasses);
961-
*((LPVOID*) (pTransitionBlock + argit.GetRetBuffArgOffset())) = pRetBufStackCopy;
962-
}
963-
else
964-
{
965-
PVOID pRetBuff = gc.retVal->GetData();
966-
*((LPVOID*) (pTransitionBlock + argit.GetRetBuffArgOffset())) = pRetBuff;
967-
}
945+
PVOID pRetBuff = gc.retVal->GetData();
946+
*((LPVOID*) (pTransitionBlock + argit.GetRetBuffArgOffset())) = pRetBuff;
968947
}
969948

970949
// copy args
@@ -1128,10 +1107,6 @@ FCIMPL5(Object*, RuntimeMethodHandle::InvokeMethod,
11281107
{
11291108
CopyValueClass(gc.retVal->GetData(), &callDescrData.returnValue, gc.retVal->GetMethodTable());
11301109
}
1131-
else if (pRetBufStackCopy)
1132-
{
1133-
CopyValueClass(gc.retVal->GetData(), pRetBufStackCopy, gc.retVal->GetMethodTable());
1134-
}
11351110
// From here on out, it is OK to have GCs since the return object (which may have had
11361111
// GC pointers has been put into a GC object and thus protected.
11371112

‎src/libraries/System.Private.CoreLib/src/System/Reflection/RuntimeConstructorInfo.cs

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,13 @@ internal void ThrowNoInvokeException()
105105

106106
ValidateInvokeTarget(obj);
107107

108+
// Correct number of arguments supplied
109+
int actualCount = (parameters is null) ? 0 : parameters.Length;
110+
if (ArgumentTypes.Length != actualCount)
111+
{
112+
throw new TargetParameterCountException(SR.Arg_ParmCnt);
113+
}
114+
108115
if ((InvocationFlags & InvocationFlags.RunClassConstructor) != 0)
109116
{
110117
// Run the class constructor through the class constructor mechanism instead of the Invoke path.
@@ -113,13 +120,6 @@ internal void ThrowNoInvokeException()
113120
return null;
114121
}
115122

116-
// Correct number of arguments supplied
117-
int actualCount = (parameters is null) ? 0 : parameters.Length;
118-
if (ArgumentTypes.Length != actualCount)
119-
{
120-
throw new TargetParameterCountException(SR.Arg_ParmCnt);
121-
}
122-
123123
StackAllocedArguments stackArgs = default;
124124
Span<object?> arguments = default;
125125
if (actualCount != 0)

‎src/libraries/System.Reflection/tests/Common.cs

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
// Licensed to the .NET Foundation under one or more agreements.
22
// The .NET Foundation licenses this file to you under the MIT license.
33

4+
using System.Globalization;
5+
46
namespace System.Reflection.Tests
57
{
68
public enum PublicEnum
@@ -105,4 +107,25 @@ public class Helpers
105107
{
106108
public static Assembly ExecutingAssembly => typeof(Helpers).GetTypeInfo().Assembly;
107109
}
110+
111+
public class ConvertStringToIntBinder : Binder
112+
{
113+
public override FieldInfo BindToField(BindingFlags bindingAttr, FieldInfo[] match, object value, CultureInfo? culture)
114+
=> throw new NotImplementedException();
115+
116+
public override MethodBase BindToMethod(BindingFlags bindingAttr, MethodBase[] match, ref object?[] args, ParameterModifier[]? modifiers, CultureInfo? culture, string[]? names, out object? state)
117+
=> throw new NotImplementedException();
118+
119+
public override object ChangeType(object value, Type type, CultureInfo? culture)
120+
=> int.Parse((string)value);
121+
122+
public override void ReorderArgumentArray(ref object?[] args, object state)
123+
=> throw new NotImplementedException();
124+
125+
public override MethodBase? SelectMethod(BindingFlags bindingAttr, MethodBase[] match, Type[] types, ParameterModifier[]? modifiers)
126+
=> throw new NotImplementedException();
127+
128+
public override PropertyInfo? SelectProperty(BindingFlags bindingAttr, PropertyInfo[] match, Type? returnType, Type[]? indexes, ParameterModifier[]? modifiers)
129+
=> throw new NotImplementedException();
130+
}
108131
}

‎src/libraries/System.Reflection/tests/ConstructorInfoTests.cs

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -126,6 +126,17 @@ public void Invoke_OneDimensionalArray_NegativeLengths_ThrowsOverflowException()
126126
}
127127
}
128128

129+
[Fact]
130+
public void Invoke_TwoDimensionalArray_CustomBinder_IncorrectTypeArguments()
131+
{
132+
var ctor = typeof(int[,]).GetConstructor(new[] { typeof(int), typeof(int) });
133+
var args = new object[] { "1", "2" };
134+
var arr = (int[,])ctor.Invoke(BindingFlags.Default, new ConvertStringToIntBinder(), args, null);
135+
Assert.Equal(2, arr.Length);
136+
Assert.True(args[0] is int);
137+
Assert.True(args[1] is int);
138+
}
139+
129140
[Fact]
130141
public void Invoke_OneParameter()
131142
{
@@ -143,6 +154,19 @@ public void Invoke_TwoParameters()
143154
Assert.Equal("hello", obj.stringValue);
144155
}
145156

157+
[Fact]
158+
public void Invoke_TwoParameters_CustomBinder_IncorrectTypeArgument()
159+
{
160+
ConstructorInfo[] constructors = GetConstructors(typeof(ClassWith3Constructors));
161+
162+
var args = new object[] { "101", "hello" };
163+
ClassWith3Constructors obj = (ClassWith3Constructors)constructors[2].Invoke(BindingFlags.Default, new ConvertStringToIntBinder(), args, null);
164+
Assert.Equal(101, obj.intValue);
165+
Assert.Equal("hello", obj.stringValue);
166+
Assert.True(args[0] is int);
167+
Assert.True(args[1] is string);
168+
}
169+
146170
[Fact]
147171
public void Invoke_NoParameters_ThowsTargetParameterCountException()
148172
{
@@ -248,8 +272,6 @@ public ClassWith3Constructors(int intValue, string stringValue)
248272
this.intValue = intValue;
249273
this.stringValue = stringValue;
250274
}
251-
252-
public string Method1(DateTime dt) => "";
253275
}
254276

255277
public static class ClassWithStaticConstructor

‎src/libraries/System.Reflection/tests/MethodInfoTests.cs

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -381,6 +381,16 @@ public static void Invoke_OptionalParameterUnassingableFromMissing_WithMissingVa
381381
AssertExtensions.Throws<ArgumentException>(null, () => GetMethod(typeof(MethodInfoDefaultParameters), "OptionalStringParameter").Invoke(new MethodInfoDefaultParameters(), new object[] { Type.Missing }));
382382
}
383383

384+
[Fact]
385+
public void Invoke_TwoParameters_CustomBinder_IncorrectTypeArguments()
386+
{
387+
MethodInfo method = GetMethod(typeof(MI_SubClass), nameof(MI_SubClass.StaticIntIntMethodReturningInt));
388+
var args = new object[] { "10", "100" };
389+
Assert.Equal(110, method.Invoke(null, BindingFlags.Default, new ConvertStringToIntBinder(), args, null));
390+
Assert.True(args[0] is int);
391+
Assert.True(args[1] is int);
392+
}
393+
384394
[Theory]
385395
[InlineData(typeof(MI_SubClass), nameof(MI_SubClass.GenericMethod1), new Type[] { typeof(int) })]
386396
[InlineData(typeof(MI_SubClass), nameof(MI_SubClass.GenericMethod2), new Type[] { typeof(string), typeof(int) })]

‎src/mono/System.Private.CoreLib/src/System/Reflection/RuntimeMethodInfo.Mono.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -374,7 +374,7 @@ private RuntimeType[] ArgumentTypes
374374
internal extern object? InternalInvoke(object? obj, in Span<object?> parameters, out Exception? exc);
375375

376376
[MethodImpl(MethodImplOptions.AggressiveInlining)]
377-
private object? InvokeWorker(object? obj, BindingFlags invokeAttr, in Span<object?> parameters)
377+
private object? InvokeWorker(object? obj, BindingFlags invokeAttr, Span<object?> parameters)
378378
{
379379
Exception? exc;
380380
object? o = null;
@@ -862,7 +862,7 @@ private void InvokeClassConstructor()
862862
}
863863

864864
[MethodImpl(MethodImplOptions.AggressiveInlining)]
865-
internal object? InvokeWorker(object? obj, BindingFlags invokeAttr, in Span<object?> arguments)
865+
internal object? InvokeWorker(object? obj, BindingFlags invokeAttr, Span<object?> arguments)
866866
{
867867
bool wrapExceptions = (invokeAttr & BindingFlags.DoNotWrapExceptions) == 0;
868868
return InternalInvoke(obj, arguments, wrapExceptions);

‎src/mono/mono/metadata/icall.c

Lines changed: 3 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -3373,9 +3373,6 @@ ves_icall_InternalInvoke (MonoReflectionMethodHandle method_handle, MonoObjectHa
33733373
MonoMethodSignature* const sig = mono_method_signature_internal (m);
33743374
int pcount = 0;
33753375
void *obj = this_arg;
3376-
char *this_name = NULL;
3377-
char *target_name = NULL;
3378-
char *msg = NULL;
33793376
MonoObject *result = NULL;
33803377
MonoArray *arr = NULL;
33813378
MonoException *exception = NULL;
@@ -3403,6 +3400,7 @@ ves_icall_InternalInvoke (MonoReflectionMethodHandle method_handle, MonoObjectHa
34033400
}
34043401
}
34053402

3403+
/* Array constructor */
34063404
if (m_class_get_rank (m->klass) && !strcmp (m->name, ".ctor")) {
34073405
int i;
34083406
pcount = mono_span_length (params_span);
@@ -3436,12 +3434,12 @@ ves_icall_InternalInvoke (MonoReflectionMethodHandle method_handle, MonoObjectHa
34363434
g_assert (pcount == (m_class_get_rank (m->klass) * 2));
34373435
/* The arguments are lower-bound-length pairs */
34383436
intptr_t * const lower_bounds = (intptr_t *)g_alloca (sizeof (intptr_t) * pcount);
3439-
3437+
34403438
for (i = 0; i < pcount / 2; ++i) {
34413439
lower_bounds [i] = *(int32_t*) ((char*)mono_span_get (params_span, MonoObject*, (i * 2)) + sizeof (MonoObject));
34423440
lengths [i] = *(int32_t*) ((char*)mono_span_get (params_span, MonoObject*, (i * 2) + 1) + sizeof (MonoObject));
34433441
}
3444-
3442+
34453443
arr = mono_array_new_full_checked (m->klass, lengths, lower_bounds, error);
34463444
goto_if_nok (error, return_null);
34473445
goto exit;
@@ -3457,9 +3455,6 @@ ves_icall_InternalInvoke (MonoReflectionMethodHandle method_handle, MonoObjectHa
34573455
MONO_HANDLE_NEW (MonoException, exception); // FIXME? overkill?
34583456
mono_gc_wbarrier_generic_store_internal (MONO_HANDLE_REF (exception_out), (MonoObject*)exception);
34593457
}
3460-
g_free (target_name);
3461-
g_free (this_name);
3462-
g_free (msg);
34633458
g_assert (!result || !arr); // only one, or neither, should be set
34643459
return result ? MONO_HANDLE_NEW (MonoObject, result) : arr ? MONO_HANDLE_NEW (MonoObject, (MonoObject*)arr) : NULL_HANDLE;
34653460
}

0 commit comments

Comments
 (0)