Skip to content

Use similar types for self-referential generics instead of the exact canonical type #83995

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
davidwrighton merged 8 commits into from
Apr 4, 2023
Merged

Use similar types for self-referential generics instead of the exact canonical type #83995

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
4c1df34
Potential fix for #6924
davidwrighton Mar 27, 2023
713786a
Address failures
davidwrighton Mar 28, 2023
b6b8f77
Fix Unix X64 build
davidwrighton Mar 28, 2023
332bdab
Fix code review comments
davidwrighton Mar 30, 2023
231c25f
Actually fix it so it works on Linux X64
davidwrighton Mar 31, 2023
5a1df4c
Merge branch 'main' of github.com:dotnet/runtime into fix6924
davidwrighton Mar 31, 2023
6149511
Code Review Feedback
davidwrighton Mar 31, 2023
47a5515
More code review comments
davidwrighton Apr 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/coreclr/debug/daccess/dacdbiimpl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4519,7 +4519,9 @@ void DacDbiInterfaceImpl::EnumerateModulesInAssembly(
// Debugger isn't notified of Resource / Inspection-only modules.
if (pDomainAssembly->GetModule()->IsVisibleToDebugger())
{
_ASSERTE(pDomainAssembly->IsLoaded());
// If domain assembly isn't yet loaded, just return
if (!pDomainAssembly->IsLoaded())
return;

VMPTR_DomainAssembly vmDomainAssembly = VMPTR_DomainAssembly::NullPtr();
vmDomainAssembly.SetHostPtr(pDomainAssembly);
Expand Down
2 changes: 2 additions & 0 deletions src/coreclr/dlls/mscorrc/mscorrc.rc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,6 +314,8 @@ BEGIN
IDS_CLASSLOAD_INLINE_ARRAY_LENGTH "InlineArrayAttribute requires that the length argument is greater than 0. Type: '%1'. Assembly: '%2'."
IDS_CLASSLOAD_INLINE_ARRAY_EXPLICIT "InlineArrayAttribute cannot be applied to a type with explicit layout. Type: '%1'. Assembly: '%2'."

IDS_INVALID_RECURSIVE_GENERIC_FIELD_LOAD "Could not load type '%1' from assembly '%2' because of an invalid self-referential generic field."

#if FEATURE_COMINTEROP
IDS_EE_CANNOTCAST_NOMARSHAL "The Windows Runtime Object can only be used in the threading context where it was created, because it implements INoMarshal or has MarshalingBehaviorAttribute(MarshalingType.None) set."
#endif
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/dlls/mscorrc/resource.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,7 @@
#define IDS_CLASSLOAD_MI_BADRETURNTYPE 0x17a8
#define IDS_CLASSLOAD_STATICVIRTUAL_NOTIMPL 0x17a9

#define IDS_INVALID_RECURSIVE_GENERIC_FIELD_LOAD 0x17aa
#define IDS_CLASSLOAD_TOOMANYGENERICARGS 0x17ab

#define IDS_CLASSLOAD_INLINE_ARRAY_FIELD_COUNT 0x17ac
Expand Down
5 changes: 5 additions & 0 deletions src/coreclr/vm/class.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2230,6 +2230,11 @@ class ApproxFieldDescIterator
SUPPORTS_DAC;
return m_totalFields - m_currField - 1;
}
int GetValueClassCacheIndex()
{
LIMITED_METHOD_CONTRACT;
return m_currField;
}
};

//
Expand Down
37 changes: 32 additions & 5 deletions src/coreclr/vm/classlayoutinfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -461,6 +461,7 @@ namespace
IMDInternalImport* pInternalImport,
HENUMInternal* phEnumField,
Module* pModule,
mdTypeDef cl,
ParseNativeTypeFlags nativeTypeFlags,
const SigTypeContext* pTypeContext,
BOOL* fDisqualifyFromManagedSequential,
Expand Down Expand Up @@ -533,20 +534,45 @@ namespace
}
#endif
MetaSig fsig(pCOMSignature, cbCOMSignature, pModule, pTypeContext, MetaSig::sigField);
CorElementType corElemType = fsig.NextArgNormalized();
CorElementType corElemType = fsig.NextArg();

TypeHandle typeHandleMaybe;
if (corElemType == ELEMENT_TYPE_VALUETYPE) // Only look up the next element in the signature if it is a value type to avoid causing recursive type loads in valid scenarios.
{
typeHandleMaybe = fsig.GetLastTypeHandleThrowing(ClassLoader::LoadTypes,
CLASS_LOAD_APPROXPARENTS,
TRUE);
SigPointer::HandleRecursiveGenericsForFieldLayoutLoad recursiveControl;
recursiveControl.pModuleWithTokenToAvoidIfPossible = pModule;
recursiveControl.tkTypeDefToAvoidIfPossible = cl;
typeHandleMaybe = fsig.GetArgProps().GetTypeHandleThrowing(pModule,
pTypeContext,
ClassLoader::LoadTypes,
CLASS_LOAD_APPROXPARENTS,
TRUE, NULL, NULL, NULL,
&recursiveControl);

if (typeHandleMaybe.IsNull())
{
// Everett C++ compiler can generate a TypeRef with RS=0
// without respective TypeDef for unmanaged valuetypes,
// referenced only by pointers to them.
// In such case, GetTypeHandleThrowing returns null handle,
// and we return E_T_VOID
Comment on lines +556 to +560
Copy link
Member

@AaronRobinsonMSFT AaronRobinsonMSFT Mar 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should update this.

Copy link
Member Author

@davidwrighton davidwrighton Mar 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably, but this change is confusing enough as it is without adding a known potential behavior change. I'd like to see that done as a separate PR.

Copy link
Member Author

@davidwrighton davidwrighton Mar 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you do that, please get rid of the weirdo ThrowButNullV11McppWorkaround logic too.

typeHandleMaybe = TypeHandle(CoreLibBinder::GetElementType(ELEMENT_TYPE_VOID));
}
corElemType = typeHandleMaybe.AsMethodTable()->GetInternalCorElementType();
if (corElemType != ELEMENT_TYPE_VALUETYPE)
typeHandleMaybe = TypeHandle();
}
else if (corElemType == ELEMENT_TYPE_TYPEDBYREF)
{
typeHandleMaybe = TypeHandle(g_TypedReferenceMT);
}

pFieldInfoArrayOut->m_placement = GetFieldPlacementInfo(corElemType, typeHandleMaybe);
*fDisqualifyFromManagedSequential |= TypeHasGCPointers(corElemType, typeHandleMaybe);
*fHasAutoLayoutField |= TypeHasAutoLayoutField(corElemType, typeHandleMaybe);
*fHasInt128Field |= TypeHasInt128Field(corElemType, typeHandleMaybe);

if (!IsFieldBlittable(pModule, fd, fsig.GetArgProps(), pTypeContext, nativeTypeFlags))
if (!IsFieldBlittable(pModule, fd, corElemType, typeHandleMaybe, nativeTypeFlags))
*pIsBlittableOut = FALSE;

(*cInstanceFields)++;
Expand Down Expand Up @@ -705,6 +731,7 @@ VOID EEClassLayoutInfo::CollectLayoutFieldMetadataThrowing(
pInternalImport,
phEnumField,
pModule,
cl,
nativeTypeFlags,
pTypeContext,
&fDisqualifyFromManagedSequential,
Expand Down
28 changes: 28 additions & 0 deletions src/coreclr/vm/field.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -672,6 +672,34 @@ UINT FieldDesc::LoadSize()
return size;
}

UINT FieldDesc::GetSize(MethodTable *pMTOfValueTypeField)
{
CONTRACTL
{
INSTANCE_CHECK;
NOTHROW;
GC_NOTRIGGER;
MODE_ANY;
FORBID_FAULT;
}
CONTRACTL_END

CorElementType type = GetFieldType();
UINT size = GetSizeForCorElementType(type);
if (size == (UINT) -1)
{
LOG((LF_CLASSLOADER, LL_INFO10000, "FieldDesc::GetSize %s::%s\n", GetApproxEnclosingMethodTable()->GetDebugClassName(), m_debugName));
CONSISTENCY_CHECK(GetFieldType() == ELEMENT_TYPE_VALUETYPE);
TypeHandle t = (pMTOfValueTypeField != NULL) ? TypeHandle(pMTOfValueTypeField) : LookupApproxFieldTypeHandle();
if (!t.IsNull())
{
size = t.GetMethodTable()->GetNumInstanceFieldBytes();
}
}

return size;
}

UINT FieldDesc::GetSize()
{
CONTRACTL
Expand Down
3 changes: 3 additions & 0 deletions src/coreclr/vm/field.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,6 +324,9 @@ class FieldDesc
// Return -1 if the type isn't loaded yet (i.e. if LookupFieldTypeHandle() would return null)
UINT GetSize();

// Return -1 if the type is a structure and pMTOfValueTypeField is NULL and LookupFieldTypeHandle() returns null
Copy link
Member

@jkotas jkotas Apr 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we actually depend on this returning -1 anywhere? It seems we always assume that it is going to succeed. Should we make this more restrictive and assert instead of returning -1?

UINT GetSize(MethodTable *pMTOfValueTypeField);

// These routines encapsulate the operation of getting and setting
// fields.
void GetInstanceField(OBJECTREF o, VOID * pOutVal);
Expand Down
128 changes: 62 additions & 66 deletions src/coreclr/vm/fieldmarshaler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,11 +201,19 @@ VOID ParseNativeType(Module* pModule,
bool IsFieldBlittable(
Module* pModule,
mdFieldDef fd,
SigPointer fieldSig,
const SigTypeContext* pTypeContext,
CorElementType corElemType,
TypeHandle valueTypeHandle,
ParseNativeTypeFlags flags
)
{
CONTRACTL
{
THROWS;
GC_TRIGGERS;
MODE_ANY;
}
CONTRACTL_END;
Copy link
Member

@jkotas jkotas Apr 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
CONTRACTL
{
THROWS;
GC_TRIGGERS;
MODE_ANY;
}
CONTRACTL_END;
STANDARD_VM_CONTRACT;


PCCOR_SIGNATURE marshalInfoSig;
ULONG marshalInfoSigLength;

Expand All @@ -218,75 +226,63 @@ bool IsFieldBlittable(

bool isBlittable = false;

EX_TRY
switch (corElemType)
{
TypeHandle valueTypeHandle;
CorElementType corElemType = fieldSig.PeekElemTypeNormalized(pModule, pTypeContext, &valueTypeHandle);

switch (corElemType)
case ELEMENT_TYPE_CHAR:
isBlittable = (nativeType == NATIVE_TYPE_DEFAULT && flags != ParseNativeTypeFlags::IsAnsi) || (nativeType == NATIVE_TYPE_I2) || (nativeType == NATIVE_TYPE_U2);
break;
case ELEMENT_TYPE_I1:
case ELEMENT_TYPE_U1:
isBlittable = (nativeType == NATIVE_TYPE_DEFAULT) || (nativeType == NATIVE_TYPE_I1) || (nativeType == NATIVE_TYPE_U1);
break;
case ELEMENT_TYPE_I2:
case ELEMENT_TYPE_U2:
isBlittable = (nativeType == NATIVE_TYPE_DEFAULT) || (nativeType == NATIVE_TYPE_I2) || (nativeType == NATIVE_TYPE_U2);
break;
case ELEMENT_TYPE_I4:
case ELEMENT_TYPE_U4:
isBlittable = (nativeType == NATIVE_TYPE_DEFAULT) || (nativeType == NATIVE_TYPE_I4) || (nativeType == NATIVE_TYPE_U4) || (nativeType == NATIVE_TYPE_ERROR);
break;
case ELEMENT_TYPE_I8:
case ELEMENT_TYPE_U8:
isBlittable = (nativeType == NATIVE_TYPE_DEFAULT) || (nativeType == NATIVE_TYPE_I8) || (nativeType == NATIVE_TYPE_U8);
break;
case ELEMENT_TYPE_R4:
isBlittable = (nativeType == NATIVE_TYPE_DEFAULT) || (nativeType == NATIVE_TYPE_R4);
break;
case ELEMENT_TYPE_R8:
isBlittable = (nativeType == NATIVE_TYPE_DEFAULT) || (nativeType == NATIVE_TYPE_R8);
break;
case ELEMENT_TYPE_I:
case ELEMENT_TYPE_U:
isBlittable = (nativeType == NATIVE_TYPE_DEFAULT) || (nativeType == NATIVE_TYPE_INT) || (nativeType == NATIVE_TYPE_UINT);
break;
case ELEMENT_TYPE_PTR:
isBlittable = nativeType == NATIVE_TYPE_DEFAULT;
break;
case ELEMENT_TYPE_FNPTR:
isBlittable = nativeType == NATIVE_TYPE_DEFAULT || nativeType == NATIVE_TYPE_FUNC;
break;
case ELEMENT_TYPE_VALUETYPE:
if (nativeType != NATIVE_TYPE_DEFAULT && nativeType != NATIVE_TYPE_STRUCT)
{
case ELEMENT_TYPE_CHAR:
isBlittable = (nativeType == NATIVE_TYPE_DEFAULT && flags != ParseNativeTypeFlags::IsAnsi) || (nativeType == NATIVE_TYPE_I2) || (nativeType == NATIVE_TYPE_U2);
break;
case ELEMENT_TYPE_I1:
case ELEMENT_TYPE_U1:
isBlittable = (nativeType == NATIVE_TYPE_DEFAULT) || (nativeType == NATIVE_TYPE_I1) || (nativeType == NATIVE_TYPE_U1);
break;
case ELEMENT_TYPE_I2:
case ELEMENT_TYPE_U2:
isBlittable = (nativeType == NATIVE_TYPE_DEFAULT) || (nativeType == NATIVE_TYPE_I2) || (nativeType == NATIVE_TYPE_U2);
break;
case ELEMENT_TYPE_I4:
case ELEMENT_TYPE_U4:
isBlittable = (nativeType == NATIVE_TYPE_DEFAULT) || (nativeType == NATIVE_TYPE_I4) || (nativeType == NATIVE_TYPE_U4) || (nativeType == NATIVE_TYPE_ERROR);
break;
case ELEMENT_TYPE_I8:
case ELEMENT_TYPE_U8:
isBlittable = (nativeType == NATIVE_TYPE_DEFAULT) || (nativeType == NATIVE_TYPE_I8) || (nativeType == NATIVE_TYPE_U8);
break;
case ELEMENT_TYPE_R4:
isBlittable = (nativeType == NATIVE_TYPE_DEFAULT) || (nativeType == NATIVE_TYPE_R4);
break;
case ELEMENT_TYPE_R8:
isBlittable = (nativeType == NATIVE_TYPE_DEFAULT) || (nativeType == NATIVE_TYPE_R8);
break;
case ELEMENT_TYPE_I:
case ELEMENT_TYPE_U:
isBlittable = (nativeType == NATIVE_TYPE_DEFAULT) || (nativeType == NATIVE_TYPE_INT) || (nativeType == NATIVE_TYPE_UINT);
break;
case ELEMENT_TYPE_PTR:
isBlittable = nativeType == NATIVE_TYPE_DEFAULT;
break;
case ELEMENT_TYPE_FNPTR:
isBlittable = nativeType == NATIVE_TYPE_DEFAULT || nativeType == NATIVE_TYPE_FUNC;
break;
case ELEMENT_TYPE_VALUETYPE:
if (nativeType != NATIVE_TYPE_DEFAULT && nativeType != NATIVE_TYPE_STRUCT)
{
isBlittable = false;
}
else if (valueTypeHandle.GetMethodTable() == CoreLibBinder::GetClass(CLASS__DECIMAL))
{
// The alignment requirements of the managed System.Decimal type do not match the native DECIMAL type.
// As a result, a field of type System.Decimal can't be blittable.
isBlittable = false;
}
else
{
isBlittable = valueTypeHandle.GetMethodTable()->IsBlittable();
}
break;
default:
isBlittable = false;
break;
}
else if (valueTypeHandle.GetMethodTable() == CoreLibBinder::GetClass(CLASS__DECIMAL))
{
// The alignment requirements of the managed System.Decimal type do not match the native DECIMAL type.
// As a result, a field of type System.Decimal can't be blittable.
isBlittable = false;
}
else
{
isBlittable = valueTypeHandle.GetMethodTable()->IsBlittable();
}
break;
default:
isBlittable = false;
break;
}
EX_CATCH
{
// We were unable to determine the native type, likely because there is a mutually recursive type reference
// in this field's type. A mutually recursive object would never be blittable, so we don't need to do anything.
}
EX_END_CATCH(RethrowTerminalExceptions);
return isBlittable;
}

Expand Down
4 changes: 2 additions & 2 deletions src/coreclr/vm/fieldmarshaler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,8 @@ BOOL IsStructMarshalable(TypeHandle th);
bool IsFieldBlittable(
Module* pModule,
mdFieldDef fd,
SigPointer fieldSig,
const SigTypeContext* pTypeContext,
CorElementType corElemType,
TypeHandle valueTypeHandle,
ParseNativeTypeFlags flags
);

Expand Down
Loading