Skip to content

Fix handling generic type in several IL #3172

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.

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

Open
wants to merge 11 commits into
base: develop
Choose a base branch
from
133 changes: 132 additions & 1 deletion src/CLR/Core/CLR_RT_HeapBlock.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -774,6 +774,137 @@ HRESULT CLR_RT_HeapBlock::Reassign(const CLR_RT_HeapBlock &value)
NANOCLR_NOCLEANUP();
}

HRESULT CLR_RT_HeapBlock::Reassign(CLR_RT_HeapBlock &rhs, const CLR_RT_TypeDef_Instance &expectedType)
{
NATIVE_PROFILE_CLR_CORE();
NANOCLR_HEADER();

// Build a TypeDescriptor for the *expected* type (the IL TypeSpec/TypeDef)
CLR_RT_TypeDescriptor descExpected;
NANOCLR_CHECK_HRESULT(descExpected.InitializeFromTypeDef(expectedType));

// Build a TypeDescriptor for the *actual* runtime object in rhs
CLR_RT_TypeDescriptor descActual;
NANOCLR_CHECK_HRESULT(descActual.InitializeFromObject(rhs));

// Compare them (including generics, arrays, value-types, etc.)
if (!TypeDescriptorsMatch(descExpected, descActual))
{
NANOCLR_SET_AND_LEAVE(CLR_E_WRONG_TYPE);
}

// They match: now do the actual copy
// - reference types & arrays: copy the object reference
// - value-types & primitives: copy the raw data
switch (descActual.GetDataType())
{
case DATATYPE_CLASS:
case DATATYPE_SZARRAY:
{
// object reference or single-dim array
this->Assign(rhs);
break;
}

default:
{
// value-type, primitive, struct, etc.
// this->CopyFrom(rhs);
break;
}
}

NANOCLR_NOCLEANUP();
}

bool CLR_RT_HeapBlock::TypeDescriptorsMatch(
const CLR_RT_TypeDescriptor &expectedType,
const CLR_RT_TypeDescriptor &actualType)
{
// Figure out logical DataTypes, promoting ACTUAL CLASS ---> GENERICINST
NanoCLRDataType expectedDataType = expectedType.GetDataType();
NanoCLRDataType actualDataType = actualType.GetDataType();

// If the *actual* object is a closed-generic (even though boxed as CLASS),
// it will have m_handlerGenericType set. Promote it to GENERICINST.
if (actualDataType == DATATYPE_CLASS && actualType.m_handlerGenericType.data != CLR_EmptyToken)
{
actualDataType = DATATYPE_GENERICINST;
}

// If either side is GENERICINST, we do generic-inst matching
if (expectedDataType == DATATYPE_GENERICINST || actualDataType == DATATYPE_GENERICINST)
{
auto &eSpec = expectedType.m_handlerGenericType;
auto &aSpec = actualType.m_handlerGenericType;

return eSpec.Assembly() == aSpec.Assembly() && eSpec.typeDefIndex == aSpec.typeDefIndex;
}

if (actualDataType <= DATATYPE_LAST_PRIMITIVE_TO_PRESERVE)
{
// If they declared a true valuetype, match directly:
if (expectedDataType == DATATYPE_VALUETYPE)
{
const auto &dtl = c_CLR_RT_DataTypeLookup[actualDataType];
if (dtl.m_cls && dtl.m_cls->data == expectedType.m_handlerCls.data)
{
return true;
}
}
// if they declared a boxed struct (CLASS whose TypeDef is a struct),
// need to match that too:
else if (expectedDataType == DATATYPE_CLASS && expectedType.m_handlerGenericType.data == 0)
{
// Look up the TypeDef record flags to see if it's a VALUE-TYPE.
CLR_RT_TypeDef_Index clsIdx = expectedType.m_handlerCls;

// fetch the owning assembly
CLR_RT_Assembly *ownerAsm = g_CLR_RT_TypeSystem.m_assemblies[clsIdx.Assembly() - 1];
const CLR_RECORD_TYPEDEF *rec = ownerAsm->GetTypeDef(clsIdx.Type());

if (rec &&
((rec->flags & CLR_RECORD_TYPEDEF::TD_Semantics_Mask) == CLR_RECORD_TYPEDEF::TD_Semantics_ValueType))
{
const auto &dtl = c_CLR_RT_DataTypeLookup[actualDataType];
if (dtl.m_cls && dtl.m_cls->data == clsIdx.data)
{
return true;
}
}
}
}

// For everything else, DataTypes must line up exactly
if (expectedDataType != actualDataType)
{
return false;
}

// Dispatch on the remaining kinds
switch (expectedDataType)
{
case DATATYPE_CLASS:
case DATATYPE_VALUETYPE:
{
// compare TypeDef indices
auto &eCls = expectedType.m_handlerCls;
auto &aCls = actualType.m_handlerCls;
return eCls.data == aCls.data;
}

case DATATYPE_SZARRAY:
{
// compare outer dims (always 1) then element types
return TypeDescriptorsMatch(expectedType, actualType);
}
Comment on lines +899 to +900
Copy link

@coderabbitai coderabbitai bot May 13, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue

Potential recursive call issue in array element type comparison.

This line appears to be making a recursive call to TypeDescriptorsMatch with the same parameters, which could lead to infinite recursion. For array element type comparison, you typically need to compare the element types, not the array types again.

Consider modifying to compare element types instead:

-            return TypeDescriptorsMatch(expectedType, actualType);
+            CLR_RT_TypeDescriptor expectedElementType, actualElementType;
+            expectedType.GetElementType(expectedElementType);
+            actualType.GetElementType(actualElementType);
+            return TypeDescriptorsMatch(expectedElementType, actualElementType);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
return TypeDescriptorsMatch(expectedType, actualType);
}
CLR_RT_TypeDescriptor expectedElementType, actualElementType;
expectedType.GetElementType(expectedElementType);
actualType.GetElementType(actualElementType);
return TypeDescriptorsMatch(expectedElementType, actualElementType);
}

Copy link
Member Author

Choose a reason for hiding this comment

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

There is no GetElementType() API in CLR_RT_TypeDescriptor .
I believe this is OK as it is... if you don't have a better suggestion let's leave it as it is.

Copy link

Choose a reason for hiding this comment

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

@josesimoes Thank you for clarifying that there's no GetElementType() API available. You're right that my suggestion wouldn't work without that API.

However, I still want to caution about the current implementation, as it calls itself recursively with the same parameters:

return TypeDescriptorsMatch(expectedType, actualType);

This could potentially lead to infinite recursion if both types are arrays. If you're confident this won't be an issue in your implementation (perhaps because of other constraints that prevent this case), then it's reasonable to leave as is.

Alternatively, you might consider adding explicit element type extraction logic in this case block if needed for correctness.


✏️ Learnings added
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3172
File: src/CLR/Core/CLR_RT_HeapBlock.cpp:899-900
Timestamp: 2025-05-14T16:27:02.525Z
Learning: The CLR_RT_TypeDescriptor type in nanoFramework doesn't have a GetElementType() API for extracting array element types.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


// primitives and other leaf types match on the DataType alone
default:
return true;
}
}

void CLR_RT_HeapBlock::AssignAndPinReferencedObject(const CLR_RT_HeapBlock &value)
{
// This is very special case that we have local variable with pinned attribute in metadata.
Expand All @@ -787,7 +918,7 @@ void CLR_RT_HeapBlock::AssignAndPinReferencedObject(const CLR_RT_HeapBlock &valu
m_data.objectReference.ptr->Unpin();
}

// Move the data.
// Move the data
m_data = value.m_data;

// Leave the same logic as in AssignAndPreserveType
Expand Down
81 changes: 80 additions & 1 deletion src/CLR/Core/Execution.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1983,7 +1983,7 @@ HRESULT CLR_RT_ExecutionEngine::InitializeLocals(
parser.Advance(element);

// walk forward to the Nth generic-parameter
for (int i = 0; i < genericParamPosition; i++)
for (int i = 0; i <= genericParamPosition; i++)
{
parser.Advance(element);
}
Expand Down Expand Up @@ -3217,6 +3217,85 @@ bool CLR_RT_ExecutionEngine::IsInstanceOf(
return IsInstanceOf(desc, descTarget, isInstInstruction);
}

/// <summary>
/// Checks whether the heap-object 'obj' satisfies exactly the type encoded by
/// the compressed token 'token' in the IL stream, under the current generic
/// instantiation in 'caller'. Supports DATATYPE_VAR slots and full GENERICINST.
/// </summary>
bool CLR_RT_ExecutionEngine::IsInstanceOfToken(
CLR_UINT32 token,
CLR_RT_HeapBlock &obj,
const CLR_RT_MethodDef_Instance &caller)
{
// Resolve the *expected* signature into a TypeDescriptor
CLR_RT_TypeDescriptor expectedDesc;
HRESULT hr = expectedDesc.InitializeFromSignatureToken(caller.assembly, token, &caller);

if (FAILED(hr))
{
return false;
}

// Extract the *actual* runtime type of the object
CLR_RT_TypeDescriptor actualDesc;
hr = actualDesc.InitializeFromObject(obj);

if (FAILED(hr))
{
return false;
}

// Delegate to the CLR built-in type-compatibility test
return TypeDescriptorsMatch(expectedDesc, actualDesc);
}

bool CLR_RT_ExecutionEngine::TypeDescriptorsMatch(const CLR_RT_TypeDescriptor &exp, const CLR_RT_TypeDescriptor &act)
{
// Quick check on the raw element kind
if (exp.GetDataType() != act.GetDataType())
{
return false;
}

switch (exp.GetDataType())
{
// Closed‐generic instantiation: compare the TypeSpec head
case DATATYPE_GENERICINST:
case DATATYPE_VAR:
{
auto &eSpec = exp.m_handlerGenericType;
auto &aSpec = act.m_handlerGenericType;
return eSpec.Assembly() == aSpec.Assembly() && eSpec.typeDefIndex == aSpec.typeDefIndex;
}

// Plain object or value‐type: compare the TypeDef_Index
case DATATYPE_CLASS:
case DATATYPE_VALUETYPE:
case DATATYPE_SZARRAY:
{
auto &eCls = exp.m_handlerCls;
auto &aCls = act.m_handlerCls;

if (eCls.data != aCls.data)
{
return false;
}

// for array we may need to compare element‐types
if (exp.GetDataType() == DATATYPE_SZARRAY)
{
// recurse into element descriptors
return TypeDescriptorsMatch(exp, act);
}
return true;
}

// All the primitives (I4, I8, R4, etc.) don't carry extra metadata
default:
return true;
}
}

HRESULT CLR_RT_ExecutionEngine::CastToType(
CLR_RT_HeapBlock &ref,
CLR_UINT32 tk,
Expand Down
33 changes: 26 additions & 7 deletions src/CLR/Core/Interpreter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2644,9 +2644,9 @@ HRESULT CLR_RT_Thread::Execute_IL(CLR_RT_StackFrame &stackArg)

switch (dt)
{
case DATATYPE_GENERICINST:
case DATATYPE_CLASS:
case DATATYPE_VALUETYPE:
case DATATYPE_GENERICINST:
obj[fieldInst.CrossReference().offset].AssignAndPreserveType(evalPos[2]);
break;
case DATATYPE_DATETIME: // Special case.
Expand Down Expand Up @@ -2969,7 +2969,7 @@ HRESULT CLR_RT_Thread::Execute_IL(CLR_RT_StackFrame &stackArg)
NANOCLR_CHECK_HRESULT(CLR_RT_TypeDescriptor::ExtractTypeIndexFromObject(evalPos[0], cls));

// Check this is an object of the requested type.
if (type.data != cls.data)
if (!g_CLR_RT_ExecutionEngine.IsInstanceOfToken(arg, evalPos[0], stack->m_call))
{
NANOCLR_SET_AND_LEAVE(CLR_E_WRONG_TYPE);
}
Expand Down Expand Up @@ -3125,18 +3125,37 @@ HRESULT CLR_RT_Thread::Execute_IL(CLR_RT_StackFrame &stackArg)
OPDEF(CEE_STELEM, "stelem", PopRef + PopI + Pop1, Push0, InlineType, IObjModel, 1, 0xFF, 0xA4, NEXT)
// Stack: ... ... <obj> <index> <value> -> ...
{
// Treat STELEM like ldelema + stobj
ip += 2; // Skip type argument, not used...
FETCH_ARG_COMPRESSED_TYPETOKEN(arg, ip);

evalPos -= 3; // "pop" args from evaluation stack
evalPos -= 3;
CHECKSTACK(stack, evalPos);

// Build a by-ref to the array slot at [index]
NANOCLR_CHECK_HRESULT(
evalPos[1].InitializeArrayReference(evalPos[1], evalPos[2].NumericByRef().s4));
evalPos[1].FixArrayReferenceForValueTypes();

// Reassign will make sure these are objects of the same type.
NANOCLR_CHECK_HRESULT(evalPos[1].Reassign(evalPos[3]));
// Resolve the IL's element type in the context of any generics
CLR_RT_TypeDef_Instance expectedType;
if (!expectedType.ResolveToken(arg, assm, &stack->m_call))
{
NANOCLR_SET_AND_LEAVE(CLR_E_WRONG_TYPE);
}

NanoCLRDataType elemDT = (NanoCLRDataType)expectedType.target->dataType;

// Promote the value if it's a reference or boxed struct
evalPos[3].Promote();

// Compute the element‐size: 0 for refs (incl. genericinst), sizeInBytes for primitives
size_t size = 0;
if (elemDT <= DATATYPE_LAST_PRIMITIVE_TO_PRESERVE)
{
size = c_CLR_RT_DataTypeLookup[elemDT].m_sizeInBytes;
}

// Store the value into the actual array buffer
NANOCLR_CHECK_HRESULT(evalPos[3].StoreToReference(evalPos[1], size));

break;
}
Expand Down
Loading
Loading