-
Notifications
You must be signed in to change notification settings - Fork 5k
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.
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds support for string constructors in the interpreter and updates the associated call stub generation.
- Added a new test case (TestStringCtor) to verify string constructor functionality
- Updated interpreter execution logic to correctly handle fcalls for string constructors
- Adjusted call stub generation to account for special string constructors
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
src/tests/JIT/interpreter/Interpreter.cs | Added test method for string constructor support |
src/coreclr/vm/interpexec.cpp | Updated interpreter method call to support special string constructors |
src/coreclr/vm/callstubgenerator.cpp | Modified call stub generation for special constructor handling |
src/coreclr/vm/interpexec.cpp
Outdated
@@ -1186,15 +1186,46 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr | |||
callArgsOffset = ip[2]; | |||
methodSlot = ip[3]; | |||
|
|||
OBJECTREF objRef = AllocateObject((MethodTable*)pMethod->pDataItems[ip[4]]); | |||
MethodTable *pClass = (MethodTable*)pMethod->pDataItems[ip[4]]; | |||
// FIXME: Duplicated code from CALL_INTERP_SLOT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider refactoring the duplicated code block for handling string constructor invocations to improve maintainability.
// FIXME: Duplicated code from CALL_INTERP_SLOT |
Copilot uses AI. Check for mistakes.
Anyone know what's up with this crossdac failure on CI?
EDIT: Looks like https://developercommunity.visualstudio.com/t/C1090-PDB-API-call-failed-error-code-2/48897 |
Also tracked here: #48070 . Build analysis should flag it for you. |
src/coreclr/vm/interpexec.cpp
Outdated
// fcall that is basically a static method that returns the new instance. | ||
if (pMD && pClass->HasComponentSize()) | ||
{ | ||
// The compiler didn't know about this so it reserved space for a this-reference. We need to skip |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sounds like a temporary workaround. The compiler can know about this (by checking CORINFO_FLG_VAROBJSIZE
flag). What needs to happen to move this logic to the compiler?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m_compHnd didn't appear to expose the things I needed to determine this. I can take another look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's the flag CORINFO_FLG_VAROBJSIZE
that @jkotas mentioned offline yesterday:
runtime/src/coreclr/jit/importercalls.cpp
Lines 1010 to 1026 in fc85a87
if (opcode == CEE_NEWOBJ) | |
{ | |
if (clsFlags & CORINFO_FLG_VAROBJSIZE) | |
{ | |
assert(!(clsFlags & CORINFO_FLG_ARRAY)); // arrays handled separately | |
// This is a 'new' of a variable sized object, wher | |
// the constructor is to return the object. In this case | |
// the constructor claims to return VOID but we know it | |
// actually returns the new object | |
assert(callRetTyp == TYP_VOID); | |
callRetTyp = TYP_REF; | |
call->gtType = TYP_REF; | |
impSpillSpecialSideEff(); | |
impPushOnStack(call, typeInfo(clsHnd)); | |
} | |
else |
You can get it by
getClassAttribs
or by the getCallInfo
in the CORINFO_CALL_INFO::classFlags
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixing this on the compiler side complicates it a lot. Could we just keep the extra slot allocation we ignore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated. 0298d2e
Not sure how I feel about it.
src/coreclr/vm/interpexec.cpp
Outdated
callArgsOffset = ip[2]; | ||
methodSlot = ip[3]; | ||
|
||
// FIXME: Duplicated code from CALL_INTERP_SLOT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really understand why this opcode is not a normal call like the others. Could we avoid having this code duplication here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
generating the call and getting everything to work right given the way we do the tagged method pointer and then cache the call target looked like a pain.
right now we rely on being able to cache the call target and then use CodeInfo to figure out whether it is interp code or jit code. the helpers for this are a third category so we would need another different tag for them or would need to generate a generic helper opcode.
if we're not ok with a special opcode for arrays and strings i can figure something out, but i don't know how long it will take
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
md arrays make it worse because you have to do a weird thing to adapt the array of dimensions to its actual call signature, so that one would need additional setup opcodes before a regular call. you can see that in the mdarray draft, look for an array called dims
src/coreclr/vm/interpexec.cpp
Outdated
// FIXME: Duplicated code from CALL_INTERP_SLOT | ||
size_t targetMethod = (size_t)pMethod->pDataItems[methodSlot]; | ||
MethodDesc *pMD = nullptr; | ||
if (targetMethod & INTERP_METHOD_HANDLE_TAG) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be assert instead? If this is not set, the pMD
is null and the TryGetMultiCallableAddrOfCode
below will crash.
Update src/coreclr/vm/callstubgenerator.cpp Co-authored-by: Aaron Robinson <[email protected]> Update isSpecialConstructor to match other parts of the runtime Migrate some string/array ctor smarts from interpexec to compiler Separate newobj opcode for string and mdarray
@@ -1213,6 +1213,28 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr | |||
ip += 5; | |||
goto CALL_INTERP_SLOT; | |||
} | |||
case INTOP_NEWOBJ_VAROBJSIZE: |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Uh oh!
There was an error while loading. Please reload this page.