[wasm] Fix interpreter crash with MethodImpl .override on PortableEntryPoints

[radekdoulik]

Fix interpreter crash with .override on PortableEntryPoints.

On WASM, when a .override directive remaps a virtual method's vtable slot, the overriding method's PortableEntryPoint occupies the slot. This causes SetNativeCodeInterlocked CAS to fail for the overridden method, so SetInterpreterCode is never reached and GetInterpreterCode returns NULL, leading to a crash.

Changes:

• method.cpp: Fix ShouldCallPrestub to use the method's own PortableEntryPoint instead of the remapped vtable slot, which belongs to a different method and returns incorrect compilation state.
• interpexec.cpp: In PrepareInterpreterCode, when GetInterpreterCode returns NULL and the vtable slot points to a different method due to .override, follow the redirect to the overriding method's interpreter code.
• self_override5.il: Add delegate test coverage using Delegate.CreateDelegate.

[Copilot]

Pull request overview

Fixes a CoreCLR interpreter crash on WASM when a MethodImpl .override causes a virtual method’s vtable slot to be remapped to a different method’s PortableEntryPoint, which can prevent interpreter code from being established during prestub execution.

Changes:

• Adjusts MethodDesc::ShouldCallPrestub() (PortableEntryPoints builds) to consult the method’s own PortableEntryPoint when the vtable slot has been remapped to a different method’s PE.
• Enhances PrepareInterpreterCode() to detect vtable-slot PE redirection to a different MethodDesc (MethodImpl scenario), prepare interpreter code for the redirected method, and cache it on the original target method.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
src/coreclr/vm/method.cpp	Updates prestub decision logic to handle MethodImpl-induced vtable PE remapping correctly under PortableEntryPoints.
src/coreclr/vm/interpexec.cpp	Adds MethodImpl redirect detection when interpreter code isn’t established, preparing interpreter code via the redirected slot method instead.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

[jkotas]

It does not look right to try to compile the wrong method, and then retry with the right method.

Most places in the runtime deal with this situation by calling MapMethodDeclToMethodImpl. We should follow that plan for the interpreter. I think you earlier commit was closer to the right fix.

[radekdoulik]

I considered it before and it seemed costly for such corner case. We would make all direct calls slower, while we will reach this case very rarely. So it is very rare unnecessary compilation against more expensive way to avoid it on every direct call.

This is what I measured back then on Loader merged runner:

Profiling data from the full Loader test suite (387 tests, browser-wasm Debug) on PR #126124.

Summary

	Previous change	ShouldCallPrestub
Hot-path calls	32,405	~1,782
Cost per call	method table walk	bit-flag check
Actual resolutions	1	1

The current ShouldCallPrestub approach is dramatically cheaper — fewer calls, each much lighter weight, and fixes the root cause for all callers (reflection, delegates, class init), not just the compiler path.

Previous change — MapMethodDeclToMethodImpl in getCallInfo (jitinterface.cpp)

Resolves .override at compile time for every direct non-virtual call.

Metric	Count
directCall invocations	32,405
Actual .override resolution (target changed)	1
Hit rate	0.003%

MapMethodDeclToMethodImpl is a real function call that walks method tables — called on every direct call.

Current solution — ShouldCallPrestub fix (method.cpp)

Adds an IsVtableSlot() check in ShouldCallPrestub to detect when a .override directive has remapped the vtable slot.

Metric	Count
Total ShouldCallPrestub calls	~9,000
IsVtableSlot() = true (bit-flag check)	~1,782
Actual .override redirect	1

IsVtableSlot() is a simple bit-flag check; GetPortableEntryPointIfExists() is a flag + pointer read — both essentially free.

[radekdoulik]

It does not look right to try to compile the wrong method, and then retry with the right method.

Most places in the runtime deal with this situation by calling MapMethodDeclToMethodImpl. We should follow that plan for the interpreter. I think you earlier commit was closer to the right fix.

I will look into whether we can detect it in the interpexec and avoid the unnecessary compilation.

[radekdoulik]

With the latest change we don't compile the unwanted body. The cost is still much better than resolving it in compiler. 1,730 calls vs 32,405 of MapMethodDeclToMethodImpl

Metric	Count
PrepareInterpreterCode total calls	8,500
IsVtableSlot() = true → MapMethodDeclToMethodImpl called	1,730 (20%)
Actual .override resolution	1

The IsVtableSlot() guard skips MapMethodDeclToMethodImpl for 80% of calls (6,770 out of 8,500).

[jkotas]

Similar issue exists for some of the existing callers of ShouldCallPrestub on non-portable entrypoints targets.

For example, if you run the following on a regular x64 runtime, PrepareMethod won't actually compile the B method body.

using System;
using System.Runtime.CompilerServices;

class Program
{
    public virtual void A() => Console.WriteLine("A");

     //  Add `.override Program::A` to .il
    public virtual void B() => Console.WriteLine("B");

    static void Main() 
    {          
        var h = typeof(Program).GetMethod("A").MethodHandle;
        for (;;) RuntimeHelpers.PrepareMethod(h);
    }
}

You may want to consider and fix these cases too when devising the fix.

[jkotas]

We should decide whether it is ok to call ShouldCallPrestub and DoPrestub with the decl method and then fix either ShouldCallPrestub/DoPrestub or its callers.

[radekdoulik]

You may want to consider and fix these cases too when devising the fix.

I have added test for that case and it is already handled with the current change.

[radekdoulik]

We should decide whether it is ok to call ShouldCallPrestub and DoPrestub with the decl method and then fix either ShouldCallPrestub/DoPrestub or its callers.

Is the updated change fine or do you prefer to explore this path as well? I can continue with it tomorrow.

[jkotas]

I have added test for that case and it is already handled with the current change.

I do not think the test is testing what I tried to highlight. PrepareMethod(methodHandle) is expected to JIT the code that is going to be executed when you call that method. I do think it is happening currently.

Your test is only verifying that we will end up executing the correct method once you actually try to call it. It is not verifying that the method was JITed upfront.

It is a bit involved to write tests for PrepareMethod. You would have to listed to JITed method event via EventSource (that we do not have enabled on wasm yet). I am not asking that you write a test for the PrepareMethod as part of this PR.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

[github-actions]

🤖 Copilot Code Review — PR #126124

Note

This review was generated by Copilot.

Holistic Assessment

Motivation: The PR addresses a real interpreter crash on WASM (PortableEntryPoints) when a .override (MethodImpl) directive remaps a virtual method's vtable slot. The SetNativeCodeInterlocked CAS fails because the slot contains the overriding method's PortableEntryPoint, preventing SetInterpreterCode from being called. This is a genuine bug that was reproduced and iterated on through multiple review rounds with @jkotas and @AaronRobinsonMSFT.

Approach: After extensive review iteration, the current approach takes @jkotas's recommended pattern: (1) resolve .override upfront in PrepareInterpreterCode via MapMethodDeclToMethodImpl before compilation — following the established runtime convention, and (2) fix ShouldCallPrestub to check the method's own PortableEntryPoint when the vtable slot is remapped. This avoids compiling the wrong method body entirely.

Summary: ⚠️ Needs Human Review. The code is logically correct and follows the MapMethodDeclToMethodImpl pattern @jkotas recommended. Two areas need human judgment: (1) @jkotas raised an open design question about whether ShouldCallPrestub/DoPrestub should handle decl methods or whether callers should always resolve first — this hasn't been explicitly resolved; (2) the ShouldCallPrestub fix in method.cpp is a complementary change that affects all FEATURE_PORTABLE_ENTRYPOINTS platforms, and a human should confirm the interaction between both fixes is the intended long-term design.

---

Detailed Findings

✅ Correctness — PrepareInterpreterCode upfront resolution

The upfront MapMethodDeclToMethodImpl call at interpexec.cpp:1160 before DoPrestub/GetInterpreterCode follows the established runtime pattern used elsewhere (e.g., fptrstubs.cpp:108). This ensures:

• The correct method body is compiled (not the overridden decl's body)
• The result is cached on the original MethodDesc via SetInterpreterCode (line 1188) / PoisonInterpreterCode (line 1182)
• No recursive PrepareInterpreterCode call is needed — simpler than the previous iteration

The IsVtableSlot() guard (line 1158) is an appropriate optimization that skips the function call for ~80% of PrepareInterpreterCode invocations (per the author's profiling: 1,730 out of 8,500 calls pass the guard).

✅ Thread Safety — Adequate for the concurrency model

Multiple threads calling PrepareInterpreterCode for the same overridden method would compute the same result via MapMethodDeclToMethodImpl (deterministic). SetInterpreterCode uses VolatileStore providing release semantics — last-write-wins is safe since all writers write the same value. PoisonInterpreterCode is similarly idempotent. This matches the existing interpreter concurrency model.

✅ Contract Compliance — MapMethodDeclToMethodImpl in cooperative mode

MapMethodDeclToMethodImpl has STATIC_CONTRACT_THROWS; STATIC_CONTRACT_GC_TRIGGERS (methodtable.cpp:6096-6097). It's called before the GCX_PREEMP() block (line 1164), so it executes in cooperative mode. GC triggers are permitted in cooperative mode (the thread is at a safe point). For generics, FindOrCreateAssociatedMethodDesc (called when slots differ, methodtable.cpp:6124) can also trigger GC/loading, but the types are already loaded at this point since we're in PrepareInterpreterCode.

✅ Caching Symmetry — Both paths handled correctly

• Success (targetIp != NULL, pOriginalMethod != targetMethod): pOriginalMethod->SetInterpreterCode(targetIp) caches the result so IsInterpreterCodeInitialized at call sites (lines 3186, 3286) hits the fast path on subsequent calls.
• Failure (targetIp == NULL): both targetMethod and pOriginalMethod get poisoned, preventing repeated futile compilation attempts.
• No override (pOriginalMethod == targetMethod): the #ifdef blocks are no-ops, preserving the existing behavior.

✅ Test Quality — Good coverage per reviewer requests

self_override_generic.il (added per @jkotas's request) covers generic .override with both reference type (string) and value type (int32) instantiations, for both virtual (callvirt) and non-virtual (call) dispatch. The self_override5.il delegate tests exercise the Delegate.CreateDelegate path. CLRTestPriority is correctly set to 1. The test structure matches existing patterns in the MethodImpl test directory (.il in Desktop/, .ilproj at the parent level).

⚠️ Open Design Question — ShouldCallPrestub fix scope (advisory, not merge-blocking)

@jkotas raised (comment): "We should decide whether it is ok to call ShouldCallPrestub and DoPrestub with the decl method and then fix either ShouldCallPrestub/DoPrestub or its callers." The current PR does both — fixes ShouldCallPrestub (to handle decl methods correctly) AND fixes the caller (PrepareInterpreterCode, to resolve upfront). This belt-and-suspenders approach is defensively sound, but the design intent should be explicitly confirmed. If the long-term answer is "callers should always resolve first," the ShouldCallPrestub fix becomes dead code for the interpreter path (though it still protects other callers like PrepareMethod). A human reviewer should confirm whether both fixes are desired or whether the ShouldCallPrestub change should be deferred to a separate PR addressing the broader PrepareMethod issue @jkotas highlighted.

💡 Minor — IsVtableSlot() guard is redundant but beneficial

MapMethodDeclToMethodImpl already returns pMDDecl unchanged when !pMDDecl->IsVirtual() (methodtable.cpp:6106). Since IsVtableSlot() is a strict subset of IsVirtual(), the guard is functionally redundant. However, it saves a function call for non-vtable methods, and @jkotas suggested simplifying the inner code (removing the explicit pResolved != targetMethod check), which was done. No change needed.

Generated by Code Review for issue #126124 · ◷