Skip to content
New issue

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

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

Already on GitHub? # to your account

[cdac] Make GetObjectData for arrays use canonical method table when getting the element type #106718

Closed
wants to merge 1 commit into from

Conversation

elinor-fung
Copy link
Member

When getting the element type for an array, cDAC was just getting the CorElementType of the element type handle. However, there are cases where that does not match the element type on the class for the array's canonical method table - for example, arrays of multi-dimensional or non-zero-based arrays. This changes the cDAC to use the canonical method table when getting the element type - matching the DAC (which uses MethodTable::GetArrayElementType).

Copy link
Contributor

Tagging subscribers to this area: @tommcdon
See info in area-owners.md if you want to be subscribed.

TypeHandle element = runtimeTypeSystemContract.GetTypeParam(handle);
data->ElementTypeHandle = element.Address;
data->ElementType = (uint)runtimeTypeSystemContract.GetSignatureCorElementType(element);

// Get the element type from the canonical method table - see MethodTable::GetArrayElementType for coreclr equivalent
Copy link
Member Author

@elinor-fung elinor-fung Aug 20, 2024

Choose a reason for hiding this comment

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

@davidwrighton It came as a surprise to me that MethodTable::GetArrayElementType would return something different from MethodTable::GetArrayElementTypeHandle().GetSignatureCorElementType(). I don't know the implications (if any) of those two not matching - I'm not sure if this just expected or if it could be problematic in some way?

I see how we get there by using the object[] method table:

// Arrays of reference types all share the same EEClass.
//
// We can't share nested SZARRAYs because they have different
// numbers of constructors.
//
// Unfortunately, we cannot share more because of it would affect user visible System.RuntimeMethodHandle behavior
if (CorTypeInfo::IsObjRef(elemType) && elemType != ELEMENT_TYPE_SZARRAY && pElemMT != g_pObjectClass)
{
// This is loading the canonical version of the array so we can override
OVERRIDE_TYPE_LOAD_LEVEL_LIMIT(CLASS_LOADED);
pCanonMT = ClassLoader::LoadArrayTypeThrowing(TypeHandle(g_pObjectClass), arrayKind, Rank).AsMethodTable();
}

The only case I could figure out where this would happen is an array of a multi-dimensional or non-zero-based array - for example, ushort[][,]. The CorElementType of the element type handle (ushort[,]) is ELEMENT_TYPE_ARRAY in that case, which doesn't match ELEMENT_TYPE_CLASS of object[] .

Copy link
Member

Choose a reason for hiding this comment

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

This is historic artifact of how arrays were represented in the past and fragile NGen.

It may be best to delete ArrayClass::m_ElementType field, GetArrayElementType methods, and replace their calls with GetArrayElementTypeHandle().GetSignatureCorElementType/GetVerifierCorElementType as appropriate.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, thanks. I'll look at doing that.

# for free to subscribe to this conversation on GitHub. Already have an account? #.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants