Skip to content

Commit

Permalink
Address nullable issues and other clean up
Browse files Browse the repository at this point in the history
  • Loading branch information
@steveharter
steveharter committed Mar 27, 2022
1 parent 3334686 commit 7891c3a
Show file tree
Hide file tree
Showing 17 changed files with 392 additions and 533 deletions.
26 changes: 16 additions & 10 deletions src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/DynamicMethod.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -433,13 +433,21 @@ private DynamicMethodInvoker Invoker

internal Signature Signature
{
[MethodImpl(MethodImplOptions.AggressiveInlining)]
get
{
Debug.Assert(m_methodHandle != null);
Debug.Assert(m_parameterTypes != null);
[MethodImpl(MethodImplOptions.NoInlining)] // move lazy sig generation out of the hot path
Signature LazyCreateSignature()
{
Debug.Assert(m_methodHandle != null);
Debug.Assert(m_parameterTypes != null);

Signature newSig = new Signature(m_methodHandle, m_parameterTypes, m_returnType, CallingConvention);
Volatile.Write(ref _signature, newSig);
return newSig;
}

_signature ??= new Signature(m_methodHandle, m_parameterTypes, m_returnType, CallingConvention);
return _signature;
return _signature ?? LazyCreateSignature();
}
}

Expand Down Expand Up @@ -475,7 +483,6 @@ internal Signature Signature
Debug.Assert(parameters != null);
Span<object?> copyOfParameters;
Span<bool> shouldCopyBackParameters;
Span<IntPtr> byrefParameters;

if (argCount <= MaxStackAllocArgCount)
{
Expand All @@ -484,11 +491,10 @@ internal Signature Signature
shouldCopyBackParameters = new Span<bool>(ref argStorage._copyBack0, argCount);

StackAllocatedByRefs byrefStorage = default;
byrefParameters = new Span<IntPtr>(&byrefStorage, argCount);

CheckArguments(
copyOfParameters,
byrefParameters,
(IntPtr*)&byrefStorage,
shouldCopyBackParameters,
parameters,
Signature.Arguments,
Expand All @@ -504,9 +510,9 @@ internal Signature Signature
copyOfParameters = new Span<object?>(objHolder, 0, argCount);

// We don't check a max stack size since we are invoking a method which
// natually requires a stack size that is dependent on the arg count\size.
// naturally requires a stack size that is dependent on the arg count\size.
IntPtr* byrefStorage = stackalloc IntPtr[argCount];
byrefParameters = new Span<IntPtr>(byrefStorage, argCount);
Buffer.ZeroMemory((byte*)byrefStorage, (uint)(argCount * sizeof(IntPtr)));

bool* boolHolder = stackalloc bool[argCount];
shouldCopyBackParameters = new Span<bool>(boolHolder, argCount);
Expand All @@ -518,7 +524,7 @@ internal Signature Signature
RegisterForGCReporting(&reg);
CheckArguments(
copyOfParameters,
byrefParameters,
byrefStorage,
shouldCopyBackParameters,
parameters,
Signature.Arguments,
Expand Down
3 changes: 1 addition & 2 deletions src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeMethodInfo.CoreCLR.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -350,11 +350,10 @@ public override MethodImplAttributes GetMethodImplementationFlags()
Span<bool> shouldCopyBackParameters = new(ref argStorage._copyBack0, 1);

StackAllocatedByRefs byrefStorage = default;
Span<IntPtr> unsafeByrefParameters = new Span<IntPtr>(&byrefStorage, 1);

CheckArguments(
copyOfParameters,
unsafeByrefParameters,
(IntPtr*)&byrefStorage,
shouldCopyBackParameters,
parameters,
ArgumentTypes,
Expand Down
57 changes: 43 additions & 14 deletions src/coreclr/System.Private.CoreLib/src/System/RuntimeType.CoreCLR.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3443,6 +3443,8 @@ public override Type[] GetGenericParameterConstraints()
#endregion

#region Misc
internal bool IsNullableOfT => Nullable.GetUnderlyingType(this) != null;

public sealed override bool HasSameMetadataDefinitionAs(MemberInfo other) => HasSameMetadataDefinitionAsCore<RuntimeType>(other);

public override Type MakePointerType() => new RuntimeTypeHandle(this).MakePointer();
Expand Down Expand Up @@ -3494,7 +3496,7 @@ internal bool CheckValue(
{
Debug.Assert(!ReferenceEquals(value?.GetType(), this));

// Fast path to whethern a value can be assigned to type.
// Fast path to whether a value can be assigned to type.
if (IsInstanceOfType(value))
{
// Since this cannot be a generic parameter, we use RuntimeTypeHandle.IsValueType here
Expand All @@ -3503,6 +3505,14 @@ internal bool CheckValue(

if (RuntimeTypeHandle.IsValueType(this))
{
// If a nullable, pass the object even though it's a value type.
if (IsNullableOfT)
{
// Treat as a reference type.
copyBack = false;
return false;
}

// Must be an equivalent type, re-box to the target type
object newValue = AllocateValueType(this, value, fForceTypeChange: true);
value = newValue;
Expand Down Expand Up @@ -3567,6 +3577,16 @@ private CheckValueStatus TryChangeType(
if (isByRef)
{
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;
}

if (sigElementType.IsInstanceOfType(value))
{
// Need to create an instance of the ByRef if null was provided, but only if primitive, enum or value type
Expand Down Expand Up @@ -3604,12 +3624,24 @@ private CheckValueStatus TryChangeType(

if (value == null)
{
if (allowNull)
if (!allowNull)
{
if (RuntimeTypeHandle.IsValueType(this))
isValueType = copyBack = default;
return IsByRefLike ? CheckValueStatus.NotSupported_ByRefLike : CheckValueStatus.ArgumentException;
}

if (RuntimeTypeHandle.IsValueType(this))
{
// If a nullable, pass the null as an object even though it's a value type.
if (IsNullableOfT)
{
// Treat as a reference type.
isValueType = false;
copyBack = false;
}
else
{
isValueType = true;

if (RuntimeTypeHandle.IsByRefLike(this))
{
// For a byref-like parameter pass null to the runtime which will create a default value.
Expand All @@ -3622,17 +3654,14 @@ private CheckValueStatus TryChangeType(
copyBack = false;
}
}
else
{
isValueType = false;
copyBack = false;
}

return CheckValueStatus.Success;
}
else
{
isValueType = false;
copyBack = false;
}

isValueType = copyBack = default;
return IsByRefLike ? CheckValueStatus.NotSupported_ByRefLike : CheckValueStatus.ArgumentException;
return CheckValueStatus.Success;
}

if (this == s_typedRef)
Expand Down Expand Up @@ -3674,7 +3703,7 @@ static CheckValueStatus SpecialCast(RuntimeType type, ref object value)

if (pointer != null)
{
value = pointer.GetPointerValue();
value = pointer.GetPointerValue(); // Convert source pointer to IntPtr
}
else
{
Expand Down
111 changes: 34 additions & 77 deletions src/coreclr/vm/invokeutil.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ void *InvokeUtil::GetIntPtrValue(OBJECTREF pObj) {
RETURN *(void **)((pObj)->UnBox());
}

void InvokeUtil::CopyArg(TypeHandle th, PVOID *pArgUnsafe, ArgDestination *argDest) {
void InvokeUtil::CopyArg(TypeHandle th, PVOID **pArgRef, ArgDestination *argDest) {
CONTRACTL {
THROWS;
GC_NOTRIGGER; // Caller does not protect object references
Expand All @@ -135,35 +135,24 @@ void InvokeUtil::CopyArg(TypeHandle th, PVOID *pArgUnsafe, ArgDestination *argDe
CONTRACTL_END;

void *pArgDst = argDest->GetDestinationAddress();
PVOID *rArg = *pArgRef;
CorElementType type = th.GetVerifierCorElementType();

MethodTable* pMT = th.GetMethodTable();
CorElementType oType;
CorElementType type;

oType = pMT->GetInternalCorElementType();
type = th.GetVerifierCorElementType();

// This basically maps the Signature type our type and calls the CreatePrimitiveValue
// method. We can omit this if we get alignment on these types.
switch (type) {
case ELEMENT_TYPE_I4:
case ELEMENT_TYPE_BOOLEAN:
case ELEMENT_TYPE_I1:
case ELEMENT_TYPE_U1:
case ELEMENT_TYPE_I1:
case ELEMENT_TYPE_I2:
case ELEMENT_TYPE_U2:
case ELEMENT_TYPE_CHAR:
case ELEMENT_TYPE_I4:
case ELEMENT_TYPE_U4:
case ELEMENT_TYPE_R4:
IN_TARGET_32BIT(case ELEMENT_TYPE_I:)
IN_TARGET_32BIT(case ELEMENT_TYPE_U:)
{
{
_ASSERTE(pArgUnsafe != NULL);
ARG_SLOT slot;
CreatePrimitiveValue(type, oType, pArgUnsafe, pMT, &slot);
*(PVOID *)pArgDst = (PVOID)slot;
}
_ASSERTE(rArg != NULL);
*(PVOID *)pArgDst = (PVOID)*rArg;
break;
}

Expand All @@ -173,36 +162,45 @@ void InvokeUtil::CopyArg(TypeHandle th, PVOID *pArgUnsafe, ArgDestination *argDe
IN_TARGET_64BIT(case ELEMENT_TYPE_I:)
IN_TARGET_64BIT(case ELEMENT_TYPE_U:)
{
{
_ASSERTE(pArgUnsafe != NULL);
ARG_SLOT slot;
CreatePrimitiveValue(type, oType, pArgUnsafe, pMT, &slot);
*(INT64 *)pArgDst = (INT64)slot;
}
_ASSERTE(rArg != NULL);
*(INT64 *)pArgDst = (INT64)*rArg;
break;
}

case ELEMENT_TYPE_VALUETYPE:
{
// pArgUnsafe can be NULL but only for Nullable types; UnBoxIntoArg verifies that.
{
if (!th.AsMethodTable()->UnBoxIntoArg(argDest, pArgUnsafe))
COMPlusThrow(kArgumentException, W("Arg_ObjObj"));
if (Nullable::IsNullableType(th))
{
// ASSUMPTION: we only receive T or NULL values, not Nullable<T> values
// and the values are boxed, unlike other value types.
OBJECTREF src = (OBJECTREF)(Object*)*rArg;
if (!th.AsMethodTable()->UnBoxIntoArg(argDest, src))
COMPlusThrow(kArgumentException, W("Arg_ObjObj"));
}
else
{
if (rArg == NULL)
COMPlusThrow(kArgumentException, W("Arg_ObjObj"));

MethodTable* pMT = th.GetMethodTable();
CopyValueClassArg(argDest, rArg, pMT, 0);
}
}
break;
}

case ELEMENT_TYPE_STRING: // System.String
case ELEMENT_TYPE_CLASS: // Class
case ELEMENT_TYPE_OBJECT: // System.Object
case ELEMENT_TYPE_SZARRAY: // Single Dim
case ELEMENT_TYPE_ARRAY: // General Array
case ELEMENT_TYPE_CLASS: // Class
case ELEMENT_TYPE_OBJECT:
case ELEMENT_TYPE_STRING: // System.String
case ELEMENT_TYPE_VAR:
{
if (pArgUnsafe == NULL)
if (rArg == NULL)
*(PVOID *)pArgDst = 0;
else
*(PVOID *)pArgDst = OBJECTREFToObject((OBJECTREF)(Object*)*pArgUnsafe);
*(PVOID *)pArgDst = OBJECTREFToObject((OBJECTREF)(Object*)*rArg);
break;
}

Expand All @@ -212,57 +210,16 @@ void InvokeUtil::CopyArg(TypeHandle th, PVOID *pArgUnsafe, ArgDestination *argDe
// heads these off and morphs the type handle to not be byref anymore
_ASSERTE(!Nullable::IsNullableType(th.AsTypeDesc()->GetTypeParam()));

*(PVOID *)pArgDst = pArgUnsafe;
*(PVOID *)pArgDst = rArg;
break;
}

//case ELEMENT_TYPE_TYPEDBYREF:
//{
// TypedByRef* ptr = (TypedByRef*) pArgDst;
// TypeHandle srcTH;
// BOOL bIsZero = FALSE;

// // If we got the univeral zero...Then assign it and exit.
// if (rObj== 0) {
// bIsZero = TRUE;
// ptr->data = 0;
// ptr->type = TypeHandle();
// }
// else {
// bIsZero = FALSE;
// srcTH = rObj->GetTypeHandle();
// ptr->type = rObj->GetTypeHandle();
// }

// if (!bIsZero)
// {
// //CreateByRef only triggers GC in throw path
// ptr->data = CreateByRef(srcTH, oType, srcTH, rObj, pObjUNSAFE);
// }

// break;
//}

case ELEMENT_TYPE_PTR:
case ELEMENT_TYPE_FNPTR:
{
// If we got the univeral zero...Then assign it and exit.
if (pArgUnsafe == NULL) {
*(PVOID *)pArgDst = 0;
}
else {
OBJECTREF rObj = (OBJECTREF)(Object*)*pArgUnsafe;
if (rObj->GetMethodTable() == CoreLibBinder::GetClassIfExist(CLASS__POINTER) && type == ELEMENT_TYPE_PTR)
*(PVOID *)pArgDst = GetPointerValue(rObj);
else if (rObj->GetTypeHandle().AsMethodTable() == CoreLibBinder::GetElementType(ELEMENT_TYPE_I))
{
ARG_SLOT slot;
CreatePrimitiveValue(oType, oType, rObj, &slot);
*(PVOID *)pArgDst = (PVOID)slot;
}
else
COMPlusThrow(kArgumentException,W("Arg_ObjObj"));
}
_ASSERTE(rArg != NULL);
MethodTable* pMT = th.GetMethodTable();
CopyValueClassArg(argDest, rArg, pMT, 0);
break;
}

Expand Down
5 changes: 1 addition & 4 deletions src/coreclr/vm/invokeutil.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ class InvokeUtil
{

public:
static void CopyArg(TypeHandle th, PVOID *pArgRef, ArgDestination *argDest);
static void CopyArg(TypeHandle th, PVOID **pArgRef, ArgDestination *argDest);

// Given a type, this routine will convert an return value representing that
// type into an ObjectReference. If the type is a primitive, the
Expand Down Expand Up @@ -154,9 +154,6 @@ class InvokeUtil
static void* GetPointerValue(OBJECTREF pObj);
static void* GetIntPtrValue(OBJECTREF pObj);

private:
static void* CreateByRef(TypeHandle dstTh,CorElementType srcType, TypeHandle srcTH,OBJECTREF srcObj, OBJECTREF *pIncomingObj);

private:
// The Attributes Table
// This constructs a table of legal widening operations
Expand Down
1 change: 0 additions & 1 deletion src/coreclr/vm/methodtable.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2387,7 +2387,6 @@ class MethodTable
#ifndef DACCESS_COMPILE
BOOL UnBoxInto(void *dest, OBJECTREF src);
BOOL UnBoxIntoArg(ArgDestination *argDest, OBJECTREF src);
BOOL UnBoxIntoArg(ArgDestination *argDest, PVOID pSrcUnsafe);
void UnBoxIntoUnchecked(void *dest, OBJECTREF src);
#endif

Expand Down
Loading

0 comments on commit 7891c3a

Please sign in to comment.