Ldvirtftn implementation for interpreter #116661

davidwrighton · 2025-06-14T00:10:13Z

Implement more complex call scenarios

ldvirtftn based (generic virtual calls)
code pointer based (certain static virtual calls)
ldvirtftn path to delegate creation

And as a bonus, to keep the tests working...

Rework where we attach intepreted code to a method, so that we never put the interpreter code bytes into the vtable (with this change, the vtable, etc, always have the InterpreterPrecode in it.

Add scheme for enabling logging of the CallStubGenerator.

Fix calling convention handling for jit calling convention to interpreter calling convention for Unix X64 and Windows Arm64.

- ldvirtftn based (generic virtual calls) - code pointer based (certain static virtual calls) - ldvirtftn path to delegate creation

…put the interpreter code bytes into the vtable (with this change, the vtable, etc, always have the InterpreterPrecode in it.

Copilot

Pull Request Overview

This PR implements improvements for handling generic virtual calls in the interpreter by introducing an ldvirtftn-based pathway, supporting code pointer based static virtual calls, and updating delegate creation support.

Adds tests in the interpreter for GenericVirtualMethod and new delegate methods.
Updates JIT and interpreter helper functions to propagate interpreter code flags and allocate InterpreterPrecode correctly.
Modifies the interpreter compiler to support a new ldvirtftn path and a corresponding calli opcode.

Reviewed Changes

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

Show a summary per file

File	Description
src/tests/JIT/interpreter/Interpreter.cs	Adds tests for generic virtual methods and new call paths.
src/coreclr/vm/prestub.cpp	Removes inline InterpreterPrecode allocation and migrates it to JitCompileCodeLocked.
src/coreclr/vm/precode.h	Introduces FromEntryPoint helper in InterpreterPrecode.
src/coreclr/vm/jitinterface.h & jitinterface.cpp	Updates UnsafeJitFunction signature and handling for interpreter flags.
src/coreclr/interpreter/intops.def	Fixes the opcode name for calli.
src/coreclr/interpreter/compiler.{h,cpp}	Adds EmitPushLdvirtftn and updates ldvirtftn handling in call emission.

Comments suppressed due to low confidence (1)

src/coreclr/interpreter/compiler.cpp:2664

It would help to document or clarify the expected initialization and invariant of extraParamArgLocation to ensure its value is correctly maintained across different code paths.

assert(extraParamArgLocation == INT_MAX);

Copilot · 2025-06-14T00:10:48Z

src/coreclr/interpreter/compiler.cpp

+void DECLSPEC_NORETURN Interp_CompilationFail(const char *msg)
+{
+    // This is a fatal error, so we don't return.
+    throw std::exception();


Consider throwing a more descriptive exception (or a custom exception type) in Interp_CompilationFail instead of using std::exception, to improve clarity when a fatal compilation error occurs.

Suggested change

void DECLSPEC_NORETURN Interp_CompilationFail(const char *msg)

{

// This is a fatal error, so we don't return.

throw std::exception();

// Custom exception for fatal compilation errors

class InterpCompilationException : public std::exception

{

private:

const char* m_msg;

public:

explicit InterpCompilationException(const char* msg) : m_msg(msg) {}

virtual const char* what() const noexcept override

{

return m_msg;

}

};

void DECLSPEC_NORETURN Interp_CompilationFail(const char *msg)

{

// This is a fatal error, so we don't return.

throw InterpCompilationException(msg);

src/coreclr/interpreter/compiler.cpp

BrzVlad · 2025-06-14T10:51:48Z

src/coreclr/interpreter/compiler.cpp

+    CORINFO_GENERICHANDLE_RESULT embedInfo;
+    m_compHnd->embedGenericHandle(pResolvedToken, true, m_methodInfo->ftn, &embedInfo);
+    assert(embedInfo.compileTimeHandle != NULL);
+    int typeVar = EmitGenericHandleAsVar(embedInfo);


Just for my clarity, GenericHandle has nothing to do with generics ? The generic part just means that it can be any handle, for example a non-generic method handle ?

janvorli · 2025-06-16T11:21:49Z

src/coreclr/interpreter/compiler.cpp

+                        // This check really only applies to intrinsic Array.Address methods
+                        if (callInfo.sig.callConv & CORINFO_CALLCONV_PARAMTYPE)
+                        {
+                            Interp_CompilationFail("Currently do not support LDFTN of Parameterized functions");


The preexisting way of handling errors like this is too set m_hasInvalidCode to true and go to exit_bad_code. It would be nice to keep that concept or change to the new one everywhere.
Btw, I've found on Friday that we are not handling the CORJIT_BADCODE that we return at exit_bad_code at the caller, so it would need to be fixed.

I'm now replacing all the m_hasInvalidCode stuff with a new exception based flow. It's simple/straightforward.

janvorli · 2025-06-16T11:25:49Z

src/coreclr/vm/prestub.cpp

@@ -429,17 +429,6 @@ PCODE MethodDesc::PrepareILBasedCode(PrepareCodeConfig* pConfig)
        LOG((LF_CLASSLOADER, LL_INFO1000000,
            "    In PrepareILBasedCode, calling JitCompileCode\n"));
        pCode = JitCompileCode(pConfig);
-#ifdef FEATURE_INTERPRETER
-        if (pConfig->IsInterpreterCode())


With your change, the IsInterpreterCode method and the underlying field is no longer needed. Can you please remove it?

kg · 2025-06-16T15:50:29Z

src/coreclr/interpreter/compiler.cpp

+    callArgs[0] = thisVar;
+    callArgs[1] = typeVar;
+    callArgs[2] = methodVar;
+    callArgs[3] = -1;


Should we have a named constant for this -1? I think it's the call args end of list sentinel?

We already have one: CALL_ARGS_TERMINATOR, it used just at few places.

kg · 2025-06-16T15:52:42Z

src/coreclr/interpreter/compiler.cpp

            {
-                // Resolve a virtual call using the helper function to a function pointer, and then call through that
-                assert(!"Need to support ldvirtftn path");
+                assert(extraParamArgLocation == INT_MAX);


Should we have a comment explaining what it means if this assert fails? I assume it's for methods that have a hidden extra parameter as part of their calling convention, and we don't support those yet

kg · 2025-06-16T16:00:43Z

LGTM aside from existing comments

…inor code feedback/build errors.

janvorli · 2025-06-16T21:29:38Z

src/coreclr/interpreter/compiler.cpp

-                        assert(!"Unsupported element type for STELEM");
-                        m_hasInvalidCode = true;
-                        goto exit_bad_code;
+                        BADCODE("Unsupported element type for STELEM");


Is this unsupported in a sense that the other element types than the one we currently handle are invalid or in the sense that we don't support it yet?

I don't know. I'm just replacing the m_hasInvalidCode logic, not changing behavior.

I was asking because the new mechanism would now let the interpreter compilation return error code and we would fall back to JIT. So during the bringup, there may be cases when we think things are working interpreted, but they'd be running jitted instead. When we generate the error as a result of bad IL, it is ok, because the JIT would fail too.
So it seems that for cases when we don't support something yet, we should assert.

janvorli

LGTM, thank you!

jkotas · 2025-06-16T22:28:01Z

src/coreclr/interpreter/eeinterp.cpp

+        // FIXME this shouldn't be here
+        compHnd->setMethodAttribs(methodInfo->ftn, CORINFO_FLG_INTERPRETER);
+


Suggested change

// FIXME this shouldn't be here

compHnd->setMethodAttribs(methodInfo->ftn, CORINFO_FLG_INTERPRETER);

Can this and CORINFO_FLG_INTERPRETER be deleted too?

jkotas · 2025-06-16T22:33:41Z

src/coreclr/interpreter/compiler.cpp

@@ -47,9 +47,10 @@ thread_local ICorJitInfo* t_InterpJitInfoTls = nullptr;
 static const char *g_stackTypeString[] = { "I4", "I8", "R4", "R8", "O ", "VT", "MP", "F " };

 /*****************************************************************************/
-void DECLSPEC_NORETURN Interp_NOMEM()
+void DECLSPEC_NORETURN Interp_CompilationFail(const char *msg)


Suggested change

void DECLSPEC_NORETURN Interp_CompilationFail(const char *msg)

This look unused. Delete it?

janvorli · 2025-06-17T15:16:26Z

@davidwrighton I have tried to run this PR with coreclr tests and it crashes because of the JitCompileCodeLocked returning the interpreter precode in the ETW event logging below. It ends up calling EECodeInfo::GetMethodRegionInfo, but we've created the EECodeInfo from the interpreter precode address. It needs to be initialized from the interpreter IR bytecode address instead.
In other words, the ETW::MethodLog::MethodJitted needs to be passed the interpreter IR as pCode.

runtime/src/coreclr/vm/prestub.cpp

Lines 837 to 845 in 504fb7d

    
                   pCode = JitCompileCodeLocked(pConfig, pilHeader, pEntry, &sizeOfCode); 
        
           #ifdef FEATURE_EVENT_TRACE 
        
                   ETW::MethodLog::MethodJitted(this, 
        
                       &namespaceOrClassName, 
        
                       &methodName, 
        
                       &methodSignature, 
        
                       pCode, 
        
                       pConfig); 
        
           #endif //FEATURE_EVENT_TRACE

src/coreclr/vm/prestub.cpp

jkotas

Nit

src/coreclr/vm/jitinterface.cpp

…Unix X64

…n the correct values in thisTransform

dotnet-policy-service · 2025-06-17T23:11:19Z

Tagging subscribers to this area: @BrzVlad, @janvorli, @kg
See info in area-owners.md if you want to be subscribed.

dotnet-policy-service · 2025-06-17T23:11:19Z

Tagging subscribers to this area: @BrzVlad, @janvorli, @kg
See info in area-owners.md if you want to be subscribed.

janvorli · 2025-06-18T18:57:17Z

@davidwrighton the interpreter test is failing on arm64 Windows in bc.GenericVirtualMethod with error code C0000602

davidwrighton · 2025-06-18T23:25:42Z

@janvorli Yeah, I've spent my spare time today to setup an Windows Arm64 test machine for that. I suspect its another subtle bug in the calling convention work.

…irtftn

davidwrighton added 2 commits June 13, 2025 17:06

Implement more complex call scenarios

2a4e2d5

- ldvirtftn based (generic virtual calls) - code pointer based (certain static virtual calls) - ldvirtftn path to delegate creation

Rework where we attach intepreted code to a method, so that we never …

52703f1

…put the interpreter code bytes into the vtable (with this change, the vtable, etc, always have the InterpreterPrecode in it.

Copilot AI review requested due to automatic review settings June 14, 2025 00:10

davidwrighton requested review from BrzVlad, janvorli and kg as code owners June 14, 2025 00:10

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 14, 2025

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

Copilot AI reviewed Jun 14, 2025

View reviewed changes

jkotas reviewed Jun 14, 2025

View reviewed changes

src/coreclr/interpreter/compiler.cpp Outdated Show resolved Hide resolved

jkotas reviewed Jun 14, 2025

View reviewed changes

src/coreclr/interpreter/compiler.cpp Outdated Show resolved Hide resolved

BrzVlad reviewed Jun 14, 2025

View reviewed changes

janvorli reviewed Jun 16, 2025

View reviewed changes

kg mentioned this pull request Jun 16, 2025

[WIP] Interpreter unsafe accessor tests #116110

Draft

kg reviewed Jun 16, 2025

View reviewed changes

Convert error paths in the jit to use exceptions, and address other m…

0735d4f

…inor code feedback/build errors.

janvorli reviewed Jun 16, 2025

View reviewed changes

janvorli approved these changes Jun 16, 2025

View reviewed changes

jkotas reviewed Jun 16, 2025

View reviewed changes

Fix code review issues

ed4b94d

janvorli reviewed Jun 17, 2025

View reviewed changes

src/coreclr/vm/prestub.cpp Show resolved Hide resolved

jkotas reviewed Jun 17, 2025

View reviewed changes

src/coreclr/vm/jitinterface.cpp Outdated Show resolved Hide resolved

src/coreclr/vm/jitinterface.cpp Outdated Show resolved Hide resolved

jkotas and others added 4 commits June 17, 2025 10:50

Apply suggestions from code review

68f1f6e

Fix call stub generator for reverse invoke of interpreted methods on …

b33a8b7

…Unix X64

Merge remote-tracking branch 'origin/ldvirtftn' into ldvirtftn

3914b5e

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

b8dd62e

davidwrighton added 2 commits June 17, 2025 15:56

Fix the combination of static virtual methods and valuetypes to retur…

3283372

…n the correct values in thisTransform

Remove unnecessary local

68fff2d

am11 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 17, 2025

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

browser-wasm Windows build error #116746

Open

davidwrighton added 4 commits June 19, 2025 13:19

Remove unneeded var

21a5cac

Merge branch 'ldvirtftn' of github.com:davidwrighton/runtime into ldv…

151fc2b

…irtftn

Fix issues causing runtime failures on Windows arm64

2651f1d

Revert change I didn't intend to merge at this time

9a0debb

This was referenced Jun 19, 2025

browser-wasm windows Debug AllSubsets_CoreCLR builds failing in emcc seemingly unrelated to any code issues #116647

Open

Occasional failure in "browser-wasm windows Release LibraryTests: Build Product" #116671

Open

davidwrighton merged commit a13232a into dotnet:main Jun 19, 2025
102 of 104 checks passed

-void DECLSPEC_NORETURN Interp_CompilationFail(const char *msg)
-{
-    // This is a fatal error, so we don't return.
-    throw std::exception();
+// Custom exception for fatal compilation errors
+class InterpCompilationException : public std::exception
+{
+private:
+    const char* m_msg;
+public:
+    explicit InterpCompilationException(const char* msg) : m_msg(msg) {}
+    virtual const char* what() const noexcept override
+    {
+        return m_msg;
+    }
+};
+void DECLSPEC_NORETURN Interp_CompilationFail(const char *msg)
+{
+    // This is a fatal error, so we don't return.
+    throw InterpCompilationException(msg);

		// FIXME this shouldn't be here
		compHnd->setMethodAttribs(methodInfo->ftn, CORINFO_FLG_INTERPRETER);

Ldvirtftn implementation for interpreter #116661

Ldvirtftn implementation for interpreter #116661

Uh oh!

Conversation

davidwrighton commented Jun 14, 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!

Copilot AI Jun 14, 2025

Choose a reason for hiding this comment

Uh oh!

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!

kg commented Jun 16, 2025

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!

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!

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

Uh oh!

Uh oh!

Uh oh!

jkotas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dotnet-policy-service bot commented Jun 17, 2025

Uh oh!

dotnet-policy-service bot commented Jun 17, 2025

Uh oh!

janvorli commented Jun 18, 2025

Uh oh!

davidwrighton commented Jun 18, 2025

Uh oh!

Uh oh!

Uh oh!

davidwrighton commented Jun 14, 2025 •

edited

Loading

janvorli commented Jun 17, 2025 •

edited

Loading