Implementation of ldftn and calli #116449

davidwrighton · 2025-06-09T22:24:01Z

This implements ldftn by matching the implementation in the JIT. Integration with the delegate construction path is not done, so we always go down slow path delegate creation, but it does work.
This implementation leverages our CallStubGenerator to create stubs for calli instructions. To get the appropriate call stub to the right location, we use a new jit interface api called GetCookieForInterpreterCalliSig to pass the needed cookie around.
As a bonus it is now possible to call delegates by calling the Invoke method on the delegate. We will likely want to translate that to using an INTOP_CALLI in the future, but it does work for now.

NOTE: This logic results in going through an interpreter->native->interpreter calling thunks to make interpreter to interpreter calls. We will likely want to build an optimized path which uses of NonVirtualEntry2MethodDesc or an interpreter specific form to avoid bouncing through the calling convention trampolines, but I'd like to have a fully functional system before diving into doing that sort of thing as an optimization.

- This implements ldftn by matching the implementation in the JIT. It appears to work correctly with shared generics, but the Interpreter stub handling of the generic param argument is missing, so that doesn't work yet - This implementation leverages our CallStubGenerator to create stubs for calli instructions. To get the appropriate call stub to the right location, we misuse the GetCookieForPInvokeCalliSig to pass the needed cookie around. - As a bonus its now possible to call delegates which take no parameters.

Copilot

Pull Request Overview

This PR implements the ldftn and calli instruction support in the interpreter and JIT, along with associated test cases and improvements in call stub generation. Key changes include adding tests for delegate and calli execution in Interpreter.cs, modifications to call stub generation in CallStubGenerator for both method and signature-based stubs, and updates to interpreter opcode definitions and compilation to support calli functionality.

Reviewed Changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/tests/JIT/interpreter/Interpreter.cs	Added tests for delegate and calli, and updated catch block structures
src/coreclr/vm/typehashingalgorithms.h	Introduced an AddPointer method for pointer hashing
src/coreclr/vm/siginfo.hpp	Added const qualifiers to NumFixedArgs and HasThis methods
src/coreclr/vm/jitinterface.{h,cpp}	Added GetCookieForPInvokeCalliSig to support calli cookie propagation
src/coreclr/vm/interpexec.cpp	Extended interpreter execution to handle INTOP_CALLI and ldptr.deref
src/coreclr/vm/ceemain.cpp	Initialized CallStubGenerator for interpreter support
src/coreclr/vm/callstubgenerator.{h,cpp}	Updated call stub generation logic and caching for calli support
src/coreclr/interpreter/{intops.def,compiler.{h,cpp}}	Added new opcodes and modified EmitCall to handle calli cases
src/coreclr/inc/{crsttypes_generated.h,CrstTypes.def}	Updated CRST enumerations to include the CallStubCache

Comments suppressed due to low confidence (1)

src/coreclr/interpreter/compiler.cpp:2309

[nitpick] The variable name 'callIFunctionPointerVar' could be renamed to 'calliFunctionPointerVar' to better reflect its purpose and maintain consistency with the 'isCalli' flag.

int callIFunctionPointerVar = m_pStackPointer[-1].var;

src/coreclr/vm/typehashingalgorithms.h

…erface. It isn't used, but it should be there.

…eter_calli_support

…ng pr

Co-authored-by: Copilot <[email protected]>

…using the pinvoke one.

…righton/runtime into interpreter_calli_support

janvorli · 2025-06-10T19:47:58Z

src/coreclr/interpreter/compiler.cpp

@@ -2239,6 +2239,32 @@ InterpCompiler::InterpEmbedGenericResult InterpCompiler::EmitGenericHandle(CORIN
    return result;
 }

+void InterpCompiler::EmitCORINFO_LOOKUPToStack(const CORINFO_LOOKUP& lookup)


Just a question - what does the "ToStack" mean in the function name?

Uh, nothing good. In this case its stuffing its result onto the interpreter stack.

@janvorli How about renaming it to EmitPushCORINFO_LOOKUP, does that sound good?

Sounds good

janvorli · 2025-06-10T20:13:46Z

src/coreclr/vm/callstubgenerator.cpp

+    // plus one slot for the target pointer and reallocated to the real size at the end.
+    PCODE *pRoutines = (PCODE*)alloca(ComputeTempStorageSize(sig));
+
+    ComputeCallStub(sig, pRoutines);


Couldn't we use the signature data to generate the hash instead of building the call stub and then throwing it away when we find it in the cache? It seems it defeats the purpose of caching the stub - we could then just generate it every time again and the perf would be the same.

The store exists to handle the storage problem not the cost of calculating the callstub problem. Computing the callstub is actually very fast, but managing the storage is what we're doing here. Really I shouldn't call this a cache, but callings things like this caches is extremely prevalent in our codebase.

jkotas · 2025-06-10T21:01:55Z

src/coreclr/inc/corinfo.h

+    // Generate a cookie based on the signature that would needs to be passed
+    // to INTOP_CALLI in the interpreter


Suggested change

// Generate a cookie based on the signature that would needs to be passed

// to INTOP_CALLI in the interpreter

// Generate a cookie from the signature to pass to INTOP_CALLI in the interpreter.

(Also, change fix the comment on GetCookieForPInvokeCalliSig.)

src/coreclr/tools/Common/JitInterface/CorInfoTypes.cs

janvorli

LGTM, thank you!

…ubGenerator to be initialized to some well known value (and by explicitly setting m_interpreterToNative to true in GenerateCallStubForSig (which is what actually fixed the problem)

BrzVlad · 2025-06-11T14:38:57Z

src/coreclr/vm/interpexec.cpp

+    CONTRACTL_END
+
+    // CallStubHeaders encode their destination addresses in the Routines array, so they need to be
+    // copied to a local buffer before we can actually set their target address.


In my opinion, call stubs shouldn't include the target pointer to call. They could be tied to the signature only and we could cache them for interp->native calls as well, instead of creating a new stub for every method.

Not having the target in the routines list would mean that we would have an unnecessary extra return and call, because we would need to put a routine with just ret to the end of the routines list and then call the target from one of the CallJittedMethodRetXXX functions. Also, having it per method allows us to not to have an extra storage for the call stubs pointers and a lookup based on the signature. I am not sure if the benefit of having less of the call stubs has enough weight, especially when these stubs are created only for the cases when the interpreter code calls native code.

This works ok for Apple platforms.

Once we get to wasm, these call stubs will need to pre-compiled and we will want to share them per signature to minimize binary size. I believe it is how Mono works today. @BrzVlad Is that correct? (An alternative is to make all managed methods in wasm to have a uniform signature - but that comes with a different set of problems, like less than ideal perf.)

I thought we only had a variety of different stubs in Mono on wasm to support calling native functions, and the managed to managed function interface was more of the uniform signature model. Honestly, I'd be tempted to use the interpreter calling convention entirely, including the parallel stack.

From https://github.com/dotnet/runtimelab/blob/feature/CoreclrInterpreter/docs/design/interpreter/compiled-code-interop.md#interpreter-exit-1 :

When the application is AOT compiled, we will include a compiled wrapper for every signature of a compiled method as well as for every pinvoke signature. The wrapper will receive the target pointer to call and the address of the interpreter stack where the arguments are present. On mono these wrappers are written in C with dynamically generated code during app compilation time.

Honestly, I'd be tempted to use the interpreter calling convention entirely, including the parallel stack.

This should be data driven decision. If we were to do this, w should try to quantify the perf loss before we build the whole system around it.

Yes, on mono wasm we share transition wrappers per signature and we would probably have to do the same on CoreCLR. We already have to spill refs on the stack (in mono wasm aot), so the GC can detect references and pin the objects. Maybe having this spilling also as part of the call convention is not that bad.

Transitions on mono wasm are kind of messy, hoping I'm not making any mistakes but a rough overview of how they work is the following:

interp -> compiled. When compiling aot image we should include compiled IL wrappers for all signatures of compiled methods. This transitions will invoke this compiled wrapper, passing the target IP as an argument.

interp -> pinvoke. When building app, we traverse over all pinvoke/icall signatures and generate a C thunk that does the transition. We invoke this passing the target IP as an argument.

compiled -> interp. When compiling aot image, when compiling a call, we keep track of possible signatures that might serve as interpreter entry point. We then include per signature wrapper for interpreter entry. Compiled code will call another method through a special ftndesc structure. In the case of interpreter code, this will be a pair of the InterpMethod to execute and some wrapper to be used.

pinvoke -> interp. When building app, we traverse over all unmanaged callers only. We generate a C thunk per method to be invoked. There is no signature sharing here.

BrzVlad · 2025-06-11T16:01:25Z

src/coreclr/interpreter/compiler.cpp

-    if (EmitCallIntrinsics(callInfo.hMethod, callInfo.sig))
+        callIFunctionPointerVar = m_pStackPointer[-1].var;
+        m_pStackPointer--;
+        calliCookie = m_compHnd->GetCookieForInterpreterCalliSig(&callInfo.sig);


It seems like adding a new method to the jit interface API is quite invasive. I'm wondering if it is even needed. Long term, I would imagine the result of ldftn is either a compiled ftnptr or a tagged pointer to some interp information. In the case of interpreter invocation, the call stub wouldn't even be used.

I'm wondering whether it might be easier to not compute anything during compile time and when doing the CALLI, in the call to compiled code case, from the code pointer we would obtain the EECodeInfo then the MethodDesc and we would then create the CallStub and store it in some data slot for the CALLI opcode instead. Thus the call stub would be lazily generated during first invocation. Not entirely sure whether this EECodeInfo stuff works with the interpreter precodes though

It is really annoying to add things to the jit interface, but it's not an unreasonable thing to do. One detail you probably didn't know is that most of the changes are auto-generated after editing the ThunkInput.txt file, and then running the gen script which is next to it. The only actually tricky thing to do is to handle the subset of SPMI changes which are not auto-generated, which for interpreter only changes is technically optional (although I have hopes that once we get around to having a stable implementation of the interpreter, we can use the SPMI infrastructure as we implement optimizations in the interpreter.) In general, I'm not convinced that lazy updating bits as the interpreter executes is a good plan for anything other than dynamic optimization opportunities (such as a monomorphic cache of MethodDesc to execution strategy or something.)

jakobbotsch · 2025-06-12T09:59:45Z

src/coreclr/tools/superpmi/superpmi-shared/methodcontext.cpp

+    key.token      = (DWORD)szMetaSig->token;
+
+    DLDL value;
+    value.A = CastPointer(result);


If you are not using the second field of the DLDL then the value can just be a DWORDLONG.

dotnet-policy-service · 2025-06-12T20:40:21Z

Tagging @dotnet/jit-contrib for JIT-EE GUID update

Copilot AI review requested due to automatic review settings June 9, 2025 22:24

davidwrighton requested review from BrzVlad, janvorli and kg as code owners June 9, 2025 22:24

github-actions bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jun 9, 2025

davidwrighton added area-CodeGen-Interpreter-coreclr and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Jun 9, 2025

dotnet-policy-service bot assigned davidwrighton Jun 9, 2025

Copilot AI reviewed Jun 9, 2025

View reviewed changes

src/coreclr/vm/typehashingalgorithms.h Outdated Show resolved Hide resolved

davidwrighton added 2 commits June 9, 2025 15:29

Add CORINFO_CALLINFO_DISALLOW_STUB to the managed side of the JIT int…

0a4c830

…erface. It isn't used, but it should be there.

Merge branch 'main' of https://github.com/dotnet/runtime into interpr…

341febd

…eter_calli_support

davidwrighton requested a review from MichalStrehovsky as a code owner June 9, 2025 22:34

davidwrighton and others added 6 commits June 9, 2025 15:52

Merge branch 'main' of https://github.com/dotnet/runtime into interpr…

d2f9d09

…eter_calli_support

Fixup changes to merge with the native to interpreter argument handli…

fbb653a

…ng pr

Update src/coreclr/vm/typehashingalgorithms.h

0e19a22

Co-authored-by: Copilot <[email protected]>

Fix unix build break

d5cdc19

Use new jit interface api for interpreter calli cookie instead of mis…

7f1fc4e

…using the pinvoke one.

Merge branch 'interpreter_calli_support' of https://github.com/davidw…

12bef79

…righton/runtime into interpreter_calli_support

janvorli reviewed Jun 10, 2025

View reviewed changes

jkotas reviewed Jun 10, 2025

View reviewed changes

src/coreclr/tools/Common/JitInterface/CorInfoTypes.cs Show resolved Hide resolved

Code review commentary

ffa9ba3

build-analysis bot mentioned this pull request Jun 10, 2025

[Mono/linux-arm64] int32_t CryptoNative_EvpPKeyBits(EVP_PKEY *): Assertion `pkey != NULL' failed. #110952

Open

janvorli approved these changes Jun 10, 2025

View reviewed changes

Fix Linux Arm64 copy of this code by forcing all the fields of CallSt…

436efb6

…ubGenerator to be initialized to some well known value (and by explicitly setting m_interpreterToNative to true in GenerateCallStubForSig (which is what actually fixed the problem)

build-analysis bot mentioned this pull request Jun 11, 2025

[9.0] unable to locate xcodebuild, please make sure the path to the Xcode folder is set correctly dotnet/dnceng#5850

Open

3 tasks

BrzVlad reviewed Jun 11, 2025

View reviewed changes

Merge branch 'main' into interpreter_calli_support

108554a

build-analysis bot mentioned this pull request Jun 11, 2025

RestApiException`1: The response contained an invalid status code 500 Internal Server Error dotnet/dnceng#2298

Open

3 tasks

We're supposed to use CORINFO_TOKENKIND_Method, not Ldtoken for this

21f03aa

This was referenced Jun 12, 2025

The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#3008

Open

Timeout in HostFactoryResolverTests.NoSpecialEntryPointPatternCanRunInParallel #114704

Open

jakobbotsch reviewed Jun 12, 2025

View reviewed changes

davidwrighton enabled auto-merge (squash) June 12, 2025 20:39

davidwrighton merged commit e713f13 into dotnet:main Jun 12, 2025
105 checks passed

jakobbotsch mentioned this pull request Jun 12, 2025

getHelperFtn support for getting CORINFO_METHOD_HANDLE #116603

Merged

janvorli mentioned this pull request Jun 19, 2025

CoreCLR Interpreter #112158

Open

61 tasks

		// Generate a cookie based on the signature that would needs to be passed
		// to INTOP_CALLI in the interpreter

	// Generate a cookie based on the signature that would needs to be passed
	// to INTOP_CALLI in the interpreter
	// Generate a cookie from the signature to pass to INTOP_CALLI in the interpreter.

Implementation of ldftn and calli #116449

Implementation of ldftn and calli #116449

Uh oh!

Conversation

davidwrighton commented Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

janvorli left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BrzVlad Jun 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dotnet-policy-service bot commented Jun 12, 2025

Uh oh!

Uh oh!

Uh oh!

davidwrighton commented Jun 9, 2025 •

edited

Loading

BrzVlad Jun 11, 2025 •

edited

Loading