Skip to content

Add support for string constructors to the interpreter #115914

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

Closed
wants to merge 3 commits into from
Closed

Add support for string constructors to the interpreter #115914

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
adf3c0b
Add support for string constructors to the interpreter
kg May 22, 2025
4cc83dc
Address pr feedback
kg May 29, 2025
c450924
Revert unnecessary changes
kg May 29, 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
47 changes: 33 additions & 14 deletions src/coreclr/interpreter/compiler.cpp
View file
Original file line number Diff line number Diff line change
@@ -3026,36 +3026,53 @@ int InterpCompiler::GenerateCode(CORINFO_METHOD_INFO* methodInfo)
m_compHnd->getMethodSig(ctorMethod, &ctorSignature);
ctorClass = m_compHnd->getMethodClass(ctorMethod);
int32_t numArgs = ctorSignature.numArgs;
bool isStringOrArray = m_compHnd->getClassAttribs(ctorClass) & CORINFO_FLG_VAROBJSIZE;
int32_t numExtraArgs = isStringOrArray ? 1 : 2;
int32_t callArgOffset = isStringOrArray ? 0 : 1;

// TODO Special case array ctor / string ctor
m_pStackPointer -= numArgs;

// Allocate callArgs for the call, this + numArgs + terminator
int32_t *callArgs = (int32_t*) AllocMemPool((numArgs + 2) * sizeof(int32_t));
int32_t *callArgs = (int32_t*) AllocMemPool((numArgs + numExtraArgs) * sizeof(int32_t));
for (int i = 0; i < numArgs; i++)
callArgs[i + 1] = m_pStackPointer[i].var;
callArgs[numArgs + 1] = -1;
callArgs[i + callArgOffset] = m_pStackPointer[i].var;
callArgs[numArgs + callArgOffset] = -1;

// Push the return value and `this` argument to the ctor
InterpType retType = GetInterpType(m_compHnd->asCorInfoType(ctorClass));
int32_t vtsize = 0;
if (retType == InterpTypeVT)
int32_t vtsize = 0, dVar, thisVar;
if (isStringOrArray)
{
// result
PushInterpType(retType, ctorClass);
dVar = m_pStackPointer[-1].var;
thisVar = -1;
}
else if (retType == InterpTypeVT)
{
vtsize = m_compHnd->getClassSize(ctorClass);
PushTypeVT(ctorClass, vtsize);
PushInterpType(InterpTypeByRef, NULL);
dVar = m_pStackPointer[-2].var;
thisVar = m_pStackPointer[-1].var;
}
else
{
// result
PushInterpType(retType, ctorClass);
// this-ref
PushInterpType(retType, ctorClass);
dVar = m_pStackPointer[-2].var;
thisVar = m_pStackPointer[-1].var;
}

if (!isStringOrArray)
{
// Consider this arg as being defined, although newobj defines it
AddIns(INTOP_DEF);
m_pLastNewIns->SetDVar(thisVar);
callArgs[0] = thisVar;
}
int32_t dVar = m_pStackPointer[-2].var;
int32_t thisVar = m_pStackPointer[-1].var;
// Consider this arg as being defined, although newobj defines it
AddIns(INTOP_DEF);
m_pLastNewIns->SetDVar(thisVar);
callArgs[0] = thisVar;

if (retType == InterpTypeVT)
{
@@ -3064,9 +3081,10 @@ int InterpCompiler::GenerateCode(CORINFO_METHOD_INFO* methodInfo)
}
else
{
AddIns(INTOP_NEWOBJ);
AddIns(isStringOrArray ? INTOP_NEWOBJ_VAROBJSIZE : INTOP_NEWOBJ);
m_pLastNewIns->data[1] = GetDataItemIndex(ctorClass);
}

m_pLastNewIns->data[0] = GetMethodDataItemIndex(ctorMethod);
m_pLastNewIns->SetSVar(CALL_ARGS_SVAR);
m_pLastNewIns->SetDVar(dVar);
@@ -3076,7 +3094,8 @@ int InterpCompiler::GenerateCode(CORINFO_METHOD_INFO* methodInfo)
m_pLastNewIns->info.pCallInfo->pCallArgs = callArgs;

// Pop this, the result of the newobj still remains on the stack
m_pStackPointer--;
if (!isStringOrArray)
m_pStackPointer--;
break;
}
case CEE_DUP:
1 change: 1 addition & 0 deletions src/coreclr/interpreter/intops.def
View file
Original file line number Diff line number Diff line change
@@ -281,6 +281,7 @@ OPDEF(INTOP_CALL, "call", 4, 1, 1, InterpOpMethodHandle)
OPDEF(INTOP_CALLVIRT, "callvirt", 4, 1, 1, InterpOpMethodHandle)
OPDEF(INTOP_NEWOBJ, "newobj", 5, 1, 1, InterpOpMethodHandle)
OPDEF(INTOP_NEWOBJ_VT, "newobj.vt", 5, 1, 1, InterpOpMethodHandle)
OPDEF(INTOP_NEWOBJ_VAROBJSIZE, "newobj.varobjsize", 5, 1, 1, InterpOpMethodHandle)

OPDEF(INTOP_CALL_HELPER_PP, "call.helper.pp", 5, 1, 0, InterpOpThreeInts)

14 changes: 14 additions & 0 deletions src/coreclr/vm/callstubgenerator.cpp
View file
Original file line number Diff line number Diff line change
@@ -548,6 +548,14 @@ CallStubHeader *CallStubGenerator::GenerateCallStub(MethodDesc *pMD, AllocMemTra
_ASSERTE(pMD != NULL);

MetaSig sig(pMD);

// Classes like System.String have special constructors that are fcalls. When invoking these, we need to override
// HasThis (there's no thisref) and the return type (the return value is the new instance, not void).
bool isSpecialConstructor = sig.HasThis() && (pMD->GetMethodTable() == g_pStringClass) && pMD->IsCtor();

if (isSpecialConstructor)
sig.ClearHasThis();

ArgIterator argIt(&sig);

m_r1 = NoRange; // indicates that there is no active range of general purpose registers
@@ -679,6 +687,12 @@ CallStubHeader *CallStubGenerator::GenerateCallStub(MethodDesc *pMD, AllocMemTra
{
TypeHandle thReturnValueType;
CorElementType thReturnType = sig.GetReturnTypeNormalized(&thReturnValueType);
if (isSpecialConstructor)
{
assert(thReturnType == ELEMENT_TYPE_VOID);
// FIXME: String?
thReturnType = ELEMENT_TYPE_CLASS;
}

switch (thReturnType)
{
28 changes: 25 additions & 3 deletions src/coreclr/vm/interpexec.cpp
View file
Original file line number Diff line number Diff line change
@@ -8,7 +8,7 @@
#include "interpexec.h"
#include "callstubgenerator.h"

void InvokeCompiledMethod(MethodDesc *pMD, int8_t *pArgs, int8_t *pRet)
void InvokeCompiledMethod(MethodDesc *pMD, int8_t *pArgs, int8_t *pRet, PCODE pCode)
{
CONTRACTL
{
@@ -41,7 +41,7 @@ void InvokeCompiledMethod(MethodDesc *pMD, int8_t *pArgs, int8_t *pRet)
}
}

pHeader->SetTarget(pMD->GetNativeCode()); // The method to call
pHeader->SetTarget(pCode); // The method to call

pHeader->Invoke(pHeader->Routines, pArgs, pRet, pHeader->TotalStackSize);
}
@@ -1151,7 +1151,7 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr
else if (codeInfo.GetCodeManager() != ExecutionManager::GetInterpreterCodeManager())
{
MethodDesc *pMD = codeInfo.GetMethodDesc();
InvokeCompiledMethod(pMD, stack + callArgsOffset, stack + returnOffset);
InvokeCompiledMethod(pMD, stack + callArgsOffset, stack + returnOffset, pMD->GetNativeCode());
break;
}

@@ -1213,6 +1213,28 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr
ip += 5;
goto CALL_INTERP_SLOT;
}
case INTOP_NEWOBJ_VAROBJSIZE:
Copy link
Member

Choose a reason for hiding this comment

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

What's the difference between this and INTOP_CALL?

In other words - if the JIT produced a regular INTOP_CALL targetMethod instead of this special opcode, where would it break?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right now, nowhere, but mdarrays are going to use this opcode and have special behavior. I'm open to generating call for this and reserving the opcode only for mdarray.

Copy link
Member

Choose a reason for hiding this comment

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

generating call for this and reserving the opcode only for mdarray.

I think it would make more sense.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll test generating CALL and see if anything breaks.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can't use INTOP_CALL because the string ctors aren't IL, they're icalls:

runtime/src/libraries/System.Private.CoreLib/src/System/String.cs

Line 287 in 60e9340

public extern String(char c, int count);

Right now we can only call IL methods with INTOP_CALL because of how it implements invoking native code.

I wanted to originally pierce through and find the managed method that implements the ctors and call that, but it sounded like there's not a way to do that from inside the JIT (and adding a new method to the JIT to do it would have been a pain anyway, and was opposed when I suggested it).

The new opcode I added happens to be constructed in a way that works for icalls. And then the mdarrays PR will expand it to also handle the unique calling convention for mdarray ctors. IMO it makes sense to have a dedicated opcode for the two variable-size objects we have in the type system.

Copy link
Member

Choose a reason for hiding this comment

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

The interpreter will need a way to call icalls. I am actually surprised that we are not hitting problem with calling icalls in more places. What would it take to make icalls work in the interpreter instead of adding new special opcode?

unique calling convention for mdarray ctors

I agree that mdarray ctors have unique calling convention and it makes sense to have a special opcode for those. I do not think the string ctors are special like that.

Copy link
Member Author

Choose a reason for hiding this comment

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

We would need to detect whether a method will successfully PrepareInitialCode or not, either at compile time (preferable) or at execution time. I'm not sure how to do it at compile time, maybe getCallInfo or getMethodInfo. I can look into that. At execution time we can check all the relevant flags, though it looks like there are a lot of them.

I think what I would do if I had to fix this now is only attempt calls through GetNativeCode for methods with IL, and do everything via TryGetMultiCallableAddrOfCode for anything else. We know that if IsIL() then PrepareInitialCode should work.

This complicates the existing call opcodes at execution time though because we don't have anywhere to store this flag. We would need to add an additional data item to store it, or add new opcode(s) for 'non-IL calls'. This is because we use a tag bit to put the MethodDesc and native code ptr in the same data item instead of storing both. If we were to start storing both separately we could do MethodDesc->IsIL() before every call to decide what to do. Or we add a new INTOP_CALL_NATIVE opcode that is designed for targets which are not IL - would we also need a INTOP_CALLVIRT_NATIVE or anything? I can't think of a case where we would.

Copy link
Member

Choose a reason for hiding this comment

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

We would need to detect whether a method will successfully PrepareInitialCode or not, either at compile time

We have a temporary hack for that https://github.com/dotnet/runtime/blob/main/src/coreclr/vm/interpexec.cpp#L1144-L1150 . You can update the hack to invoke the native code instead of EEPOLICY_HANDLE_FATAL_ERROR_WITH_MESSAGE. I think changing the condition to if (!codeInfo.IsValid() || codeInfo.GetCodeManager() != ExecutionManager::GetInterpreterCodeManager()) should do it.

Copy link
Member

Choose a reason for hiding this comment

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

I can look into that.

There is a discussion about how this should work in one of the Teams chats. @janvorli started a design doc to describe how this should work.

{
returnOffset = ip[1];
callArgsOffset = ip[2];
methodSlot = ip[3];

size_t targetMethod = (size_t)pMethod->pDataItems[methodSlot];
assert(targetMethod & INTERP_METHOD_HANDLE_TAG);
MethodDesc *pMD = (MethodDesc*)(targetMethod & ~INTERP_METHOD_HANDLE_TAG);

// If we are constructing a type with a component size (i.e. a string) its constructor is a special
// fcall that is basically a static method that returns the new instance.
// Get the address of the fcall that implements the ctor
PCODE code = pMD->TryGetMultiCallableAddrOfCode(CORINFO_ACCESS_ANY);
assert(code);

// callArgsOffset points to the ctor arguments, which are what the fcall expects.
// returnOffset points to where the new instance goes, and the fcall will write it there.
InvokeCompiledMethod(pMD, stack + callArgsOffset, stack + returnOffset, code);
ip += 5;
break;
}
case INTOP_ZEROBLK_IMM:
memset(LOCAL_VAR(ip[1], void*), 0, ip[2]);
ip += 3;
15 changes: 15 additions & 0 deletions src/tests/JIT/interpreter/Interpreter.cs
View file
Original file line number Diff line number Diff line change
@@ -413,6 +413,9 @@ public static void RunInterpreterTests()
if (!TestBoxing())
Environment.FailFast(null);

if (!TestStringCtor())
Environment.FailFast(null);

if (!TestArray())
Environment.FailFast(null);

@@ -708,6 +711,18 @@ public static bool TestBoxing()
return result == 3;
}

public static bool TestStringCtor()
{
string s = new string('a', 4);
if (s.Length != 4)
return false;
if (s[0] != 'a')
return false;
if (s != "aaaa")
return false;
return true;
}

[MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.NoOptimization)]
static object BoxedSubtraction (object lhs, object rhs) {
return (int)lhs - (int)rhs;
Loading
Oops, something went wrong.