Skip to content

Fix / disable several coreclr tests with interpreter #121492

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
+92 −6
Open

Fix / disable several coreclr tests with interpreter #121492

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
951dfdd
Fix / disable several coreclr tests with interpreter
janvorli Nov 10, 2025
40e30a4
Change the check for nint array limit to UINT_MAX
janvorli Nov 10, 2025
7ee21e4
PR feedback - fix comments referring to pinvoke marshalling
janvorli Nov 10, 2025
a1adeff
PR feedback - add nullcheck for the array with nint indexing
janvorli Nov 10, 2025
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
54 changes: 50 additions & 4 deletions src/coreclr/interpreter/compiler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1002,6 +1002,10 @@ void ValidateEmittedSequenceTermination(InterpInst *lastIns)

if (InterpOpIsUncondBranch(lastIns->opcode) ||
(lastIns->opcode == INTOP_RET) ||
(lastIns->opcode == INTOP_RET_I1) ||
(lastIns->opcode == INTOP_RET_U1) ||
(lastIns->opcode == INTOP_RET_I2) ||
(lastIns->opcode == INTOP_RET_U2) ||
(lastIns->opcode == INTOP_RET_VOID) ||
(lastIns->opcode == INTOP_RET_VT) ||
(lastIns->opcode == INTOP_THROW) ||
Expand Down Expand Up @@ -2654,7 +2658,7 @@ void InterpCompiler::EmitBinaryArithmeticOp(int32_t opBase)
}
}
}
else if (opBase == INTOP_SUB_I4 && type1 == StackTypeByRef)
else if (opBase == INTOP_SUB_I4 && ((type1 == StackTypeByRef) || (type2 == StackTypeByRef)))
{
if (type2 == StackTypeI4)
{
Expand Down Expand Up @@ -2864,6 +2868,18 @@ static int32_t GetLdindForType(InterpType interpType)
return -1;
}

static int32_t GetRetForType(InterpType interpType)
{
switch (interpType)
{
case InterpTypeI1: return INTOP_RET_I1;
case InterpTypeU1: return INTOP_RET_U1;
case InterpTypeI2: return INTOP_RET_I2;
case InterpTypeU2: return INTOP_RET_U2;
default: return INTOP_RET;
}
}

static bool DoesValueTypeContainGCRefs(COMP_HANDLE compHnd, CORINFO_CLASS_HANDLE clsHnd)
{
unsigned size = compHnd->getClassSize(clsHnd);
Expand Down Expand Up @@ -4573,7 +4589,8 @@ void InterpCompiler::EmitCall(CORINFO_RESOLVED_TOKEN* pConstrainedToken, bool re
if (injectRet)
{
// Jmp to PInvoke was converted to normal pinvoke, so we need to inject a ret after the call
switch (GetInterpType(callInfo.sig.retType))
InterpType retType = GetInterpType(m_methodInfo->args.retType);
switch (retType)
{
case InterpTypeVT:
{
Expand All @@ -4586,7 +4603,7 @@ void InterpCompiler::EmitCall(CORINFO_RESOLVED_TOKEN* pConstrainedToken, bool re
AddIns(INTOP_RET_VOID);
break;
default:
AddIns(INTOP_RET);
AddIns(GetRetForType(retType));
m_pLastNewIns->SetSVar(dVar);
break;
}
Expand Down Expand Up @@ -4710,8 +4727,24 @@ void InterpCompiler::EmitStind(InterpType interpType, CORINFO_CLASS_HANDLE clsHn
m_pStackPointer -= 2;
}

void InterpCompiler::EmitNintIndexCheck(StackInfo *spArray, StackInfo *spIndex)
Copy link
Member

@jkotas jkotas Nov 10, 2025

Choose a reason for hiding this comment

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

newarr has similar problem (it can take nint). Do we need to fix it as well?

Related comment #120981 (comment)

Copy link
Member Author

@janvorli janvorli Nov 10, 2025

Choose a reason for hiding this comment

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

I think we should fix it too, but for that one, I think we can create new versions of the INTOP_NEWARR / INTOP_NEWARR_GENERIC, there is no opcode explosion issue like with the array accesses. I'll create an issue for it.

Copy link
Member Author

@janvorli janvorli Nov 10, 2025

Choose a reason for hiding this comment

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

Created #121507

{
// In the rare case when array is indexed by nint instead of int, we emit an extra check
// to ensure the nint value fits in int32_t
AddIns(INTOP_CONV_NI);
Copy link
Member

@davidwrighton davidwrighton Nov 10, 2025

Choose a reason for hiding this comment

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

If the array is itself NULL, we need to be throwing NullReferenceException instead of IndexOutOfRangeException. This needs both the array and the index to pull that off.

Copy link
Member Author

@janvorli janvorli Nov 10, 2025

Choose a reason for hiding this comment

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

I assumed this would be handled by the actual array access, wouldn't it?

Copy link
Member Author

@janvorli janvorli Nov 10, 2025

Choose a reason for hiding this comment

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

Ah, got it, the NullReferenceException would need to have precedence.

m_pLastNewIns->SetSVars2(spArray->var, spIndex->var);
int32_t var = CreateVarExplicit(g_interpTypeFromStackType[StackTypeI4], NULL, INTERP_STACK_SLOT_SIZE);
new (spIndex) StackInfo(StackTypeI4, NULL, var);
m_pLastNewIns->SetDVar(var);
}

void InterpCompiler::EmitLdelem(int32_t opcode, InterpType interpType)
{
// handle nint index case
if (m_pStackPointer[1].GetStackType() == StackTypeI8)
{
EmitNintIndexCheck(m_pStackPointer - 2, m_pStackPointer - 1);
}
m_pStackPointer -= 2;
AddIns(opcode);
m_pLastNewIns->SetSVars2(m_pStackPointer[0].var, m_pStackPointer[1].var);
Expand All @@ -4721,6 +4754,12 @@ void InterpCompiler::EmitLdelem(int32_t opcode, InterpType interpType)

void InterpCompiler::EmitStelem(InterpType interpType)
{
// handle nint index case
if (m_pStackPointer[-2].GetStackType() == StackTypeI8)
{
EmitNintIndexCheck(m_pStackPointer - 3, m_pStackPointer - 2);
}

m_pStackPointer[-1].BashStackTypeToI_ForLocalVariableAddress();
#ifdef TARGET_64BIT
// nint and int32 can be used interchangeably. Add implicit conversions.
Expand Down Expand Up @@ -6121,7 +6160,7 @@ void InterpCompiler::GenerateCode(CORINFO_METHOD_INFO* methodInfo)
}
}

AddIns(INTOP_RET);
AddIns(GetRetForType(retType));
m_pStackPointer--;
m_pLastNewIns->SetSVar(m_pStackPointer[0].var);
}
Expand Down Expand Up @@ -8346,6 +8385,13 @@ void InterpCompiler::GenerateCode(CORINFO_METHOD_INFO* methodInfo)
CorInfoType elemCorType = m_compHnd->asCorInfoType(elemClsHnd);

m_pStackPointer -= 2;

// handle nint index
if (m_pStackPointer[1].GetStackType() == StackTypeI8)
{
EmitNintIndexCheck(m_pStackPointer, m_pStackPointer + 1);
}

if ((elemCorType == CORINFO_TYPE_CLASS) && !readonly)
{
CORINFO_GENERICHANDLE_RESULT embedInfo;
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/interpreter/compiler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -827,6 +827,7 @@ class InterpCompiler
bool EmitNamedIntrinsicCall(NamedIntrinsic ni, bool nonVirtualCall, CORINFO_CLASS_HANDLE clsHnd, CORINFO_METHOD_HANDLE method, CORINFO_SIG_INFO sig);
void EmitLdind(InterpType type, CORINFO_CLASS_HANDLE clsHnd, int32_t offset);
void EmitStind(InterpType type, CORINFO_CLASS_HANDLE clsHnd, int32_t offset, bool reverseSVarOrder, bool enableImplicitArgConversionRules);
void EmitNintIndexCheck(StackInfo *spArray, StackInfo *spIndex);
void EmitLdelem(int32_t opcode, InterpType type);
void EmitStelem(InterpType type);
void EmitStaticFieldAddress(CORINFO_FIELD_INFO *pFieldInfo, CORINFO_RESOLVED_TOKEN *pResolvedToken);
Expand Down
6 changes: 6 additions & 0 deletions src/coreclr/interpreter/inc/intops.def
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@
// we should add compact opcodes where all data is in uint16_t slots.

OPDEF(INTOP_RET, "ret", 2, 0, 1, InterpOpNoArgs)
OPDEF(INTOP_RET_I1, "ret.i1", 2, 0, 1, InterpOpNoArgs)
OPDEF(INTOP_RET_U1, "ret.u1", 2, 0, 1, InterpOpNoArgs)
OPDEF(INTOP_RET_I2, "ret.i2", 2, 0, 1, InterpOpNoArgs)
OPDEF(INTOP_RET_U2, "ret.u2", 2, 0, 1, InterpOpNoArgs)
OPDEF(INTOP_RET_VT, "ret.vt", 3, 0, 1, InterpOpInt)
OPDEF(INTOP_RET_VOID, "ret.void", 1, 0, 0, InterpOpNoArgs)

Expand Down Expand Up @@ -56,6 +60,8 @@ OPDEF(INTOP_LDELEMA, "ldelema", 5, 1, 2, InterpOpInt)
OPDEF(INTOP_LDELEMA_REF, "ldelema.ref", 5, 1, 2, InterpOpClassHandle)
OPDEF(INTOP_LDELEMA_REF_GENERIC, "ldelema.ref.generic", 6, 1, 3, InterpOpGenericLookup)

OPDEF(INTOP_CONV_NI, "conv.ni", 4, 1, 2, InterpOpNoArgs)

OPDEF(INTOP_MOV_I4_I1, "mov.i4.i1", 3, 1, 1, InterpOpNoArgs)
OPDEF(INTOP_MOV_I4_U1, "mov.i4.u1", 3, 1, 1, InterpOpNoArgs)
OPDEF(INTOP_MOV_I4_I2, "mov.i4.i2", 3, 1, 1, InterpOpNoArgs)
Expand Down
31 changes: 31 additions & 0 deletions src/coreclr/vm/interpexec.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -862,6 +862,22 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr
// Return stack slot sized value
*(int64_t*)pFrame->pRetVal = LOCAL_VAR(ip[1], int64_t);
goto EXIT_FRAME;
case INTOP_RET_I1:
// Return int8 value
*(int64_t*)pFrame->pRetVal = (int8_t)LOCAL_VAR(ip[1], int32_t);
goto EXIT_FRAME;
case INTOP_RET_U1:
// Return uint8 value
*(int64_t*)pFrame->pRetVal = (uint8_t)LOCAL_VAR(ip[1], int32_t);
goto EXIT_FRAME;
case INTOP_RET_I2:
// Return int16 value
*(int64_t*)pFrame->pRetVal = (int16_t)LOCAL_VAR(ip[1], int32_t);
goto EXIT_FRAME;
case INTOP_RET_U2:
// Return uint16 value
*(int64_t*)pFrame->pRetVal = (uint16_t)LOCAL_VAR(ip[1], int32_t);
goto EXIT_FRAME;
case INTOP_RET_VT:
memmove(pFrame->pRetVal, LOCAL_VAR_ADDR(ip[1], void), ip[2]);
goto EXIT_FRAME;
Expand Down Expand Up @@ -2817,6 +2833,8 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr
int32_t vtSize = ip[4];
void *vtThis = LOCAL_VAR_ADDR(returnOffset, void);

memset(vtThis, 0, vtSize);

// pass the address of the valuetype
LOCAL_VAR(callArgsOffset, void*) = vtThis;

Expand Down Expand Up @@ -3325,6 +3343,19 @@ do { \
break;
}

case INTOP_CONV_NI:
{
NULL_CHECK(LOCAL_VAR(ip[2], void*));
int64_t index = LOCAL_VAR(ip[3], int64_t);
if ((index < 0) || (index > UINT_MAX))
{
COMPlusThrow(kIndexOutOfRangeException);
}
LOCAL_VAR(ip[1], int64_t) = index;
ip += 4;
break;
}

case INTOP_GENERICLOOKUP:
{
int dreg = ip[1];
Expand Down
2 changes: 1 addition & 1 deletion src/tests/JIT/Directed/aliasing_retbuf/aliasing_retbuf.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ public static int TestEntryPoint()
failures |= 16;
}

// This requires pinvoke marshalling which is not currently supported by the interpreter. See https://github.com/dotnet/runtime/issues/118965
// This requires pinvoke marshalling with calli which is not currently supported by the interpreter. See https://github.com/dotnet/runtime/issues/118965
if (!TestLibrary.Utilities.IsCoreClrInterpreter)
{
f = new Foo { A = 3, B = 2, C = 1 };
Expand Down
2 changes: 2 additions & 0 deletions src/tests/JIT/Regression/JitBlue/Runtime_58259/Runtime_58259.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ public unsafe class Runtime_58259
{
[OuterLoop]
[Fact]
// This test uses pinvoke marshalling with calli which is not currently supported by the interpreter.
[ActiveIssue("https://github.com/dotnet/runtime/issues/120904", typeof(TestLibrary.Utilities), nameof(TestLibrary.Utilities.IsCoreClrInterpreter))]
public static void TestEntryPoint()
{
M(out _);
Expand Down
2 changes: 1 addition & 1 deletion src/tests/baseservices/callconvs/TestCallingConventions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ public static int TestEntryPoint()
try
{
BlittableFunctionPointers();
// This requires pinvoke marshalling which is not currently supported by the interpreter. See https://github.com/dotnet/runtime/issues/118965
// This requires pinvoke marshalling with calli which is not currently supported by the interpreter. See https://github.com/dotnet/runtime/issues/118965
if (!TestLibrary.Utilities.IsCoreClrInterpreter)
{
NonblittableFunctionPointers();
Expand Down
Loading