-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Replace some uses of MethodDescCallSite with UnmanagedCallersOnly methods
#123832
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
- Add Value property to ObjectHandleOnStack and ByteRef - Add UnmanagedCallersOnly overloads for ConvertContentsToNative, ConvertContentsToManaged, ClearNative, and ClearManaged - Add corresponding method entries in corelib.h with _UCO suffix - Add SM_IntPtr_IntPtr_PtrIntPtr_IntPtr_RetVoid signature in metasig.h This enables calling MngdRefCustomMarshaler methods via reverse P/Invoke instead of the more expensive MethodDescCallSite/CallDescrWorker path.
…amCustomMarshaler - Add UnmanagedCallersOnlyCaller template class to callhelpers.h for calling managed methods marked with [UnmanagedCallersOnly] - Add CallerArgConverter templates for automatic argument conversion (OBJECTREF* -> QCall::ObjectHandleOnStack*) - Update DispParamCustomMarshaler to use the new pattern for all four MngdRefCustomMarshaler method calls: - MarshalNativeToManaged (ConvertContentsToManaged) - MarshalManagedToNative (ConvertContentsToNative) - MarshalManagedToNativeRef (ConvertContentsToNative) - CleanUpManaged (ClearManaged) - Add ObjectHandleOnStack constructor with GC frame validation in qcall.h This replaces the expensive CallDescrWorker infrastructure with more efficient reverse P/Invoke calls via UnmanagedCallersOnly methods.
- Update AppContext.Setup to be [UnmanagedCallersOnly] with exception parameter - Update corelib.h APPCONTEXT SETUP signature to include IntPtr for exception - Update corhost.cpp to use UnmanagedCallersOnlyCaller for Setup call - Add SM_PtrPtrChar_PtrPtrChar_Int_IntPtr_RetVoid metasig Since Setup is only called from native code, there's no need to maintain a separate non-UCO overload.
|
Tagging subscribers to this area: @agocke |
- Define OBJECT_HANDLE_ON_STACK class in corelib.h for metasig use - Add SM_PtrObjectHandleOnStack_... metasig with typed pointer syntax - Update MngdRefCustomMarshaler UCO methods to use ObjectHandleOnStack* instead of IntPtr in C# signatures - Update AppContext.Setup to use ObjectHandleOnStack* for exception param - Update corelib.h method entries to use the typed signatures This provides better type safety and self-documenting signatures for methods that accept object handles from native code.
5fd601b to
53b5d65
Compare
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 pull request modernizes the calling convention for invoking managed code from native code by replacing MethodDescCallSite machinery with UnmanagedCallersOnly methods, which use the more efficient reverse P/Invoke infrastructure.
Changes:
- Introduces
UnmanagedCallersOnlyCallertemplate class in C++ for calling managed methods marked with[UnmanagedCallersOnly] - Adds
[UnmanagedCallersOnly]wrapper methods inAppContextandStubHelpersthat handle exceptions through an explicit exception parameter - Refactors
ObjectHandleOnStackandByteRefto use property-based access instead of method-based access - Updates method signatures and metadata to support the new calling convention
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/vm/callhelpers.h | Adds UnmanagedCallersOnlyCaller template class and CallerArgConverter for type-safe invocation of UnmanagedCallersOnly methods |
| src/coreclr/vm/dispparammarshaler.cpp | Replaces MethodDescCallSite with UnmanagedCallersOnlyCaller for custom marshaler method invocations |
| src/coreclr/vm/corhost.cpp | Replaces MethodDescCallSite with UnmanagedCallersOnlyCaller for AppContext.Setup |
| src/coreclr/vm/qcall.h | Adds constructor declaration for ObjectHandleOnStack |
| src/coreclr/vm/qcall.cpp | Implements ObjectHandleOnStack constructor with GC frame validation |
| src/coreclr/vm/metasig.h | Adds method signature metadata for new UnmanagedCallersOnly method signatures |
| src/coreclr/vm/corelib.h | Updates method definitions to reference new signatures and adds new UCO method definitions |
| src/libraries/System.Private.CoreLib/src/System/AppContext.cs | Adds [UnmanagedCallersOnly] version of Setup method with exception parameter |
| src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/QCallHandles.cs | Changes ObjectHandleOnStack to use typed pointer and adds Value property; changes ByteRef.Get() to ByteRef.Value property |
| src/coreclr/System.Private.CoreLib/src/System/StubHelpers.cs | Adds [UnmanagedCallersOnly] wrapper methods for custom marshaler operations with exception handling; removes pragma warning in favor of discard parameter |
| src/coreclr/System.Private.CoreLib/src/System/RuntimeHandles.cs | Updates calls from ByteRef.Get() to ByteRef.Value |
| src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/StaticsHelpers.cs | Updates calls from ByteRef.Get() to ByteRef.Value |
src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/QCallHandles.cs
Outdated
Show resolved
Hide resolved
|
Just curious, why didn't we have "reverse-FCall" that declares managed method in FCall signature and call it directly in C++ code? Why do the arguments have to go through the argument structure and get filled with asm helper? |
It is not just a simple call. It needs GC and EH transitions. For example, look for CallDescrWorkerInternalReturnAddress. The idea is to replace this internal transition with UCO, similar how we have replace internal HelperMethodFrames with PInvokes (QCalls). |
Yes I'm aware of these transitions. I'm just unsure why it can't be the form of |
…andleOnStack - Updated dispparammarshaler.cpp to pass OBJECTREF* directly - Replaced ObjectHandleOnStack-based metasig entries with typed pointer signatures - Removed OBJECT_HANDLE_ON_STACK class definition from corelib.h - Fixed GetMultiCallableAddrOfCode call in UnmanagedCallersOnlyCaller
src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/QCallHandles.cs
Outdated
Show resolved
Hide resolved
The managed calling convention is not exact match of the unmanaged calling convention. A simple |
src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/QCallHandles.cs
Outdated
Show resolved
Hide resolved
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
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
|
Copilot uncovered a bug in the existing implementation - #123832 (comment). This PR fixes it, but it does indicate there is a test hole. I'll add tests on Monday. |
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
Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.
…Attribute for clarity.
…allersOnly for clarity.
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
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
Instead of calling through the
MethodDescCallSitemachinery, retrieve a pointer to aUnmanagedCallersOnlyfunction and call it like a normal reverse P/Invoke.