Remove shared IL stubs for P/Invokes. #117901

AaronRobinsonMSFT · 2025-07-21T22:15:54Z

This removes usage of the secret argument in P/Invokes. It removes sharing for all P/Invoke IL stubs except CALLI.

Depends on #118462

AaronRobinsonMSFT · 2025-07-21T22:18:43Z

@kg @janvorli This is a simple change that removes the shared IL stubs and use of the secret argument for P/Invokes. This isn't something that would go in until .NET 11, but might be sufficient to prototype P/Invokes via the interpreter. A follow-up approach would be to remove the use of IL Stubs entirely and rely on the transient code pattern. Using the transient code pattern proved to be a bit more work thant I'd anticipated so I reduced it to this approach.

/cc @jkotas

jkotas · 2025-07-21T23:52:10Z

There is more cleanup that should be done as part of this:

MethodDesc::RequiresMethodDescCallingConvention should drop IsNDirect condition
CORJIT_FLAG_PUBLISH_SECRET_PARAM should not be set for PInvoke stubs
HasMDContextArg() should drop IsNDirect condition
There are probably some changes needed around GetActualInteropMethodDesc

src/coreclr/vm/dllimport.h

AaronRobinsonMSFT · 2025-07-22T20:51:27Z

There is more cleanup that should be done as part of this:

MethodDesc::RequiresMethodDescCallingConvention should drop IsNDirect condition

CORJIT_FLAG_PUBLISH_SECRET_PARAM should not be set for PInvoke stubs

HasMDContextArg() should drop IsNDirect condition

There are probably some changes needed around GetActualInteropMethodDesc

Yep. This was a quick and dirty solution I could pass off to the interpreter team.

AaronRobinsonMSFT · 2025-07-28T22:41:46Z

MethodDesc::RequiresMethodDescCallingConvention should drop IsNDirect condition

Done.

CORJIT_FLAG_PUBLISH_SECRET_PARAM should not be set for PInvoke stubs

Inserted a SF_IsForwardStub(m_dwStubFlags) check before the flag is set in stubs.

HasMDContextArg() should drop IsNDirect condition

Removed the PInvoke check.

There are probably some changes needed around GetActualInteropMethodDesc

Usage is blocked by HasMDContextArg() so updates flow to the single usage. Suggestions on what I could assert?

jkotas · 2025-07-29T01:54:02Z

Usage is blocked by HasMDContextArg() so updates flow to the single usage.

If I am reading the code correctly,

runtime/src/coreclr/vm/stackwalk.cpp

Line 3030 in 2db9fdf

m_crawl.pFunc = m_crawl.pFrame->GetFunction();

will cause stackwalker to report the IL stub method desc instead of the actual method desc. It may have undesirable fallout - I am thinking about problems like:

Are IL stubs going to show up in stacktraces, e.g. when P/Invoke throws an exception that gets unhandled?
Can it confuse debuggers?

This removes usage of the secret argument in P/Invokes. It also removes all usage of shared IL stubs.

Clean-up dead code in RequiresMethodDescCallingConvention and RequiresStableEntryPoint.

AaronRobinsonMSFT · 2025-07-29T20:35:16Z

will cause stackwalker to report the IL stub method desc instead of the actual method desc. It may have undesirable fallout - I am thinking about problems like:

You're right. I don't know the best way to do this. I suppose I could get the target from the DynamicMethodDesc and pass that back?

jkotas · 2025-07-29T20:59:14Z

I think it would best to minimize stackwalker dependencies if possible.

Can we get InlinedCallFrame::m_Datum initialized to point to the actual target method desc? It is done here:

runtime/src/coreclr/jit/lower.cpp

Lines 6595 to 6612 in 97d9422

    
           assert(callType == CT_USER_FUNC); 
        
           void*                 pEmbedMethodHandle = nullptr; 
        
           CORINFO_METHOD_HANDLE embedMethodHandle = 
        
               comp->info.compCompHnd->embedMethodHandle(call->gtCallMethHnd, &pEmbedMethodHandle); 
        
           noway_assert((!embedMethodHandle) != (!pEmbedMethodHandle)); 
        
           if (embedMethodHandle != nullptr) 
        
           { 
        
               // InlinedCallFrame.callSiteTarget = methodHandle 
        
               src = AddrGen(embedMethodHandle); 
        
           } 
        
           else 
        
           { 
        
               // InlinedCallFrame.callSiteTarget = *pEmbedMethodHandle 
        
               src = Ind(AddrGen(pEmbedMethodHandle)); 
        
           }

.

It is also going to fix a type mismatch: InlinedCallFrame::m_Datum is expected to be NDirectMethodDesc (modulo some bit twiddling and special cases), but it is DynamicMethodDesc with this change.

AaronRobinsonMSFT · 2025-07-29T22:39:56Z

It is also going to fix a type mismatch: InlinedCallFrame::m_Datum is expected to be NDirectMethodDesc (modulo some bit twiddling and special cases), but it is DynamicMethodDesc with this change.

Sure, I can give that a shot. This doesn't ever appear to be set for coreclr JIT, the embedMethodHandle is a nop.

runtime/src/coreclr/vm/jitinterface.cpp

Lines 10455 to 10473 in 97d9422

    
           CORINFO_METHOD_HANDLE CEEInfo::embedMethodHandle(CORINFO_METHOD_HANDLE handle, 
        
                                                            void **ppIndirection) 
        
           { 
        
               CONTRACTL { 
        
                   NOTHROW; 
        
                   GC_NOTRIGGER; 
        
                   MODE_PREEMPTIVE; 
        
               } 
        
               CONTRACTL_END; 
        
               if (ppIndirection != NULL) 
        
                   *ppIndirection = NULL; 
        
               JIT_TO_EE_TRANSITION_LEAF(); 
        
               EE_TO_JIT_TRANSITION_LEAF(); 
        
               return handle; 
        
           }

Side note, I've really got no idea what is expected here. All of this is a huge confusion about various states of fields and very loosely typed in practice. I'm really just randomly guessing what things are expected at this point, sorry.

AaronRobinsonMSFT · 2025-07-29T23:02:07Z

My confusion here is about m_Datum and the various other terms for things. It seems the m_Datum is also called callSiteTarget based on JIT comments, yay. I get that now. The CT_USER_FUNC seems to represent the actual MethodDesc, where as the CT_INDIRECT represents a calli, I think. I also believe I understand why setting m_Datum will make this work, but that is still a bit of a mystery to me in some cases about when some of these code paths are taken.

instead of the IL Stub's DynamicMethodDesc.

src/coreclr/vm/prestub.cpp

src/coreclr/vm/method.hpp

Fix "Forward PInvoke" and CALLI confusion.

src/coreclr/vm/dllimport.h

src/coreclr/vm/frames.h

src/coreclr/vm/ceeload.cpp

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

src/coreclr/tools/Common/JitInterface/ThunkGenerator/ThunkInput.txt

src/coreclr/vm/jitinterface.cpp

Copilot

Pull Request Overview

This PR removes shared IL stubs for P/Invokes except for CALLI instructions. The primary objective is to eliminate the use of secret arguments in P/Invoke stubs while maintaining sharing only for CALLI scenarios.

Key changes:

Removes secret argument handling from P/Invoke IL stubs
Updates VarargPInvokeStubWorker to take fewer parameters
Modifies stub sharing logic to exclude forward P/Invokes (except CALLI)

Reviewed Changes

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

Show a summary per file

File	Description
src/coreclr/vm/stubmgr.h	Adds new GetFirstArg method and refactors GetThisPtr to use it
src/coreclr/vm/stubmgr.cpp	Updates logging format specifiers and removes P/Invoke handling from IL stub tracing
src/coreclr/vm/stackwalk.cpp	Moves variable declaration closer to usage
src/coreclr/vm/prestub.cpp	Removes precompiled IL stub sharing for P/Invokes and updates assertion logic
src/coreclr/vm/method.hpp	Renames RequiresMethodDescCallingConvention to RequiresMDContextArg and updates related methods
src/coreclr/vm/method.cpp	Implements renamed methods and removes P/Invoke context argument requirements
src/coreclr/vm/jitinterface.cpp	Adds P/Invoke method descriptor to VASigCookie creation
src/coreclr/vm/i386/asmhelpers.asm	Reduces parameter count for VarargPInvokeStubWorker call
src/coreclr/vm/i386/asmhelpers.S	Removes unused parameter from assembly stub
src/coreclr/vm/frames.h	Updates comments to clarify PInvokeMethodDesc usage
src/coreclr/vm/dynamicmethod.cpp	Updates method name calls to use new convention
src/coreclr/vm/dllimport.h	Adds shared stub flag and reorganizes stub checking logic
src/coreclr/vm/dllimport.cpp	Major refactoring of P/Invoke stub generation to remove secret argument usage
src/coreclr/vm/comdelegate.cpp	Removes redundant MethodDesc qualification
src/coreclr/vm/codeman.h	Converts CodeHeapRequestInfo to proper class with encapsulated members
src/coreclr/vm/codeman.cpp	Updates to use new CodeHeapRequestInfo interface and improves method descriptor handling
src/coreclr/vm/cgensys.h	Updates VarargPInvokeStubWorker signature
src/coreclr/vm/ceeload.h	Adds MethodDesc field to VASigCookie and improves encapsulation
src/coreclr/vm/ceeload.cpp	Refactors VASigCookie creation to embed MethodDesc and use loader allocator
src/coreclr/vm/amd64/pinvokestubs.S	Removes unused parameter from stub worker call
src/coreclr/vm/amd64/PInvokeStubs.asm	Removes unused parameter from stub worker call

Comments suppressed due to low confidence (1)

src/coreclr/vm/method.cpp:1

The removal of the fEstimateForChunk parameter and related logic eliminates the interface check optimization that was previously guarded by the parameter. This may impact performance for interface method processing during chunk estimation.

// Licensed to the .NET Foundation under one or more agreements.

src/coreclr/vm/stubmgr.cpp

src/coreclr/vm/dllimport.cpp

src/coreclr/vm/ceeload.cpp

GCStress is not able to find the original version of the code for PInvoke stubs after dotnet#117901. We need to compensate for the MethodDesc adjustment done for PInvoke stubs when storing the original version of the code during GC stress.

Interop marshalling of varargs needs MethodDesc calling convention to support ldftn <PInvoke method with varargs>. It is not possible to smugle the target MethodDesc* via vararg cookie in this case.

GCStress is not able to find the original version of the code for PInvoke stubs after dotnet#117901. We need to compensate for the MethodDesc adjustment done for PInvoke stubs when storing the original version of the code during GC stress.

Interop marshalling of varargs needs MethodDesc calling convention to support ldftn <PInvoke method with varargs>. It is not possible to smugle the target MethodDesc* via vararg cookie in this case.

GCStress is not able to find the original version of the code for PInvoke stubs after dotnet#117901. We need to compensate for the MethodDesc adjustment done for PInvoke stubs when storing the original version of the code during GC stress.

Interop marshalling of varargs needs MethodDesc calling convention to support ldftn <PInvoke method with varargs>. It is not possible to smugle the target MethodDesc* via vararg cookie in this case.

* Fix GCStress regression GCStress is not able to find the original version of the code for PInvoke stubs after #117901. We need to compensate for the MethodDesc adjustment done for PInvoke stubs when storing the original version of the code during GC stress. * Partial revert of #117901 for vararg stubs Interop marshalling of varargs needs MethodDesc calling convention to support ldftn <PInvoke method with varargs>. It is not possible to smuggle the target MethodDesc* via vararg cookie in this case. Co-authored-by: Aaron Robinson <arobins@microsoft.com>

AaronRobinsonMSFT added this to the 11.0.0 milestone Jul 21, 2025

AaronRobinsonMSFT added NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) area-Interop-coreclr labels Jul 21, 2025

dotnet-policy-service bot assigned AaronRobinsonMSFT Jul 21, 2025

jkotas reviewed Jul 21, 2025

View reviewed changes

src/coreclr/vm/dllimport.h Show resolved Hide resolved

This was referenced Jul 22, 2025

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

Open

Test failure: Microsoft.Extensions.Configuration.EnvironmentVariables.Test.EnvironmentVariablesTest.BindingDoesNotThrowIfReloadedDuringBinding #109904

Closed

AaronRobinsonMSFT added 3 commits July 29, 2025 10:58

Remove shared IL stubs for P/Invokes.

753c75b

This removes usage of the secret argument in P/Invokes. It also removes all usage of shared IL stubs.

Feedback

ff7349e

Clean-up dead code in RequiresMethodDescCallingConvention and RequiresStableEntryPoint.

Bring back sharing for COM stubs.

2b6f869

AaronRobinsonMSFT force-pushed the remove_shared_ilstubs branch from 3c29ece to 2b6f869 Compare July 29, 2025 17:59

AaronRobinsonMSFT added 2 commits July 30, 2025 17:11

Set the CodeHeader to contain the NDirectMethodDesc

739ba97

instead of the IL Stub's DynamicMethodDesc.

Clean up the CodeHeapRequestInfo type.

d91fc12

This was referenced Jul 31, 2025

Test failure: baseservices/exceptions/stackoverflow/stackoverflowtester/stackoverflowtester.cmd #110173

Open

Android tests fail with INSTALL_FAILED_INSUFFICIENT_STORAGE #118090

Closed

jkotas reviewed Jul 31, 2025

View reviewed changes

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

jkotas reviewed Jul 31, 2025

View reviewed changes

src/coreclr/vm/method.hpp Outdated Show resolved Hide resolved

AaronRobinsonMSFT added 2 commits July 31, 2025 10:47

Feedback.

97ed27a

Fix "Forward PInvoke" and CALLI confusion.

Merge remote-tracking branch 'upstream/main' into remove_shared_ilstubs

a329add

jkotas reviewed Jul 31, 2025

View reviewed changes

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

Make order checks consistent for stub types.

1eca412

AaronRobinsonMSFT commented Aug 6, 2025

View reviewed changes

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

Apply suggestions from code review

b56bd92