Convert priority 3 MethodDescCallSite calls to UnmanagedCallersOnly#124854
Convert priority 3 MethodDescCallSite calls to UnmanagedCallersOnly#124854steveisok wants to merge 4 commits intodotnet:mainfrom
Conversation
Replace MethodDescCallSite/CallDescrWorker calls with more efficient UnmanagedCallersOnly reverse P/Invoke calls for priority 3 items from dotnet#123864. Converted call sites: - AppDomain::RaiseExitProcessEvent (ON_PROCESS_EXIT) - AppDomain::OnUnhandledException (ON_UNHANDLED_EXCEPTION) - RunManagedStartup (MANAGED_STARTUP) - RunMainInternal async helpers (HANDLE_ASYNC_ENTRYPOINT, TARGET_BROWSER) - InvokeUtil::CreateClassLoadExcept (ReflectionTypeLoadException ctor) - InvokeUtil::CreateTargetExcept (TargetInvocationException ctor) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Tagging subscribers to this area: @agocke |
...libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/AsyncHelpers.Browser.cs
Outdated
Show resolved
Hide resolved
src/coreclr/System.Private.CoreLib/src/System/AppContext.CoreCLR.cs
Outdated
Show resolved
Hide resolved
…-param and managed GetDefinedTypes() throws ReflectionTypeLoadException. Deleted reateClassLoadExcept + UCO wrapper + metasig as a result. Adjusted OnUnhandledException in AppContext.CoreCLR.cs to use a bare catch based on feedback.
There was a problem hiding this comment.
Pull request overview
This PR converts priority 3 MethodDescCallSite/CallDescrWorker calls to more efficient UnmanagedCallersOnly reverse P/Invoke calls, following the established pattern from PR #124834. The conversions target application entry points and process lifecycle events in CoreCLR.
Changes:
- Adds
[UnmanagedCallersOnly]wrappers in managed code withException*out-parameters for exception propagation - Replaces native
MethodDescCallSitecalls withUnmanagedCallersOnlyCaller::InvokeThrowingtemplate class - Refactors
ReflectionTypeLoadExceptioncreation from native to managed code via QCall pattern
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/AsyncHelpers.Browser.cs | Adds UCO wrapper for HandleAsyncEntryPoint to handle async Main entry points on Browser target |
| src/coreclr/System.Private.CoreLib/src/System/StartupHookProvider.CoreCLR.cs | Adds UCO wrapper for ManagedStartup with exception handling |
| src/coreclr/System.Private.CoreLib/src/System/Exception.CoreCLR.cs | Adds UCO wrapper for CreateTargetInvocationException |
| src/coreclr/System.Private.CoreLib/src/System/AppContext.CoreCLR.cs | New file with UCO wrappers for OnProcessExit and OnUnhandledException |
| src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeModule.cs | Updates GetTypes QCall to return exceptions array for managed-side exception construction |
| src/coreclr/System.Private.CoreLib/System.Private.CoreLib.csproj | Adds new AppContext.CoreCLR.cs file to build |
| src/coreclr/vm/metasig.h | Adds new metasig definitions for UCO method signatures |
| src/coreclr/vm/corelib.h | Updates DEFINE_METHOD entries to reference new UCO signatures |
| src/coreclr/vm/appdomain.cpp | Replaces MethodDescCallSite with UnmanagedCallersOnlyCaller for process lifecycle events |
| src/coreclr/vm/assembly.cpp | Replaces MethodDescCallSite with UnmanagedCallersOnlyCaller for startup hooks and async entry points |
| src/coreclr/vm/invokeutil.h | Removes obsolete CreateClassLoadExcept declaration |
| src/coreclr/vm/invokeutil.cpp | Removes CreateClassLoadExcept implementation; converts CreateTargetExcept to use UCO pattern |
| src/coreclr/vm/commodule.h | Adds retExceptions parameter to RuntimeModule_GetTypes QCall signature |
| src/coreclr/vm/commodule.cpp | Returns exceptions array via out-parameter instead of throwing from native code |
| src/coreclr/vm/exceptionhandlingqcalls.h | Formatting fix - adds blank line at end of file |
src/coreclr/System.Private.CoreLib/src/System/AppContext.CoreCLR.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/coreclr/vm/commodule.cpp:766
RuntimeModule_GetTypesno longer throws inside the QCall when type loading fails, but the loop still doesn’t advancecurPoson failure. WithretExceptions.Set(...)replacing the throw, execution now continues and will hit_ASSERTE(curPos == dwNumTypeDefs)when any type load fails. Adjust the loop to keepcurPosin sync withdwNumTypeDefs(e.g., advance and leave a null slot for failed loads, or remove/replace the assertion and ensure the returnedTypesarray matches the intended semantics).
// Return exceptions to managed side for throwing
if (cXcept > 0) {
gc.xceptRet = (PTRARRAYREF) AllocateObjectArray(cXcept,g_pExceptionClass);
for (DWORD i=0;i<cXcept;i++) {
gc.xceptRet->SetAt(i, gc.xcept->GetAt(i));
}
retExceptions.Set(gc.xceptRet);
}
// We should have filled the array exactly.
_ASSERTE(curPos == dwNumTypeDefs);
|
|
||
| namespace System | ||
| { | ||
| public partial class AppContext |
There was a problem hiding this comment.
AppContext is defined elsewhere in CoreLib as public static partial class AppContext (e.g. src/libraries/System.Private.CoreLib/src/System/AppContext.cs). This new CoreCLR partial declaration is missing the static modifier, which will cause a compile error due to mismatched partial type modifiers. Update this to public static partial class AppContext to match the existing declaration.
| public partial class AppContext | |
| public static partial class AppContext |
| } | ||
|
|
||
| [UnmanagedCallersOnly] | ||
| private static unsafe void OnUnhandledException(object* pUnhandledException, Exception* pOutException) |
There was a problem hiding this comment.
| private static unsafe void OnUnhandledException(object* pUnhandledException, Exception* pOutException) | |
| private static unsafe void OnUnhandledException(object* pUnhandledException, Exception* _) |
|
|
||
| ctor.Call(args); | ||
| OBJECTREF innerEx = (except && IsException((*except)->GetMethodTable())) ? *except : NULL; | ||
| GCPROTECT_BEGIN(innerEx); |
There was a problem hiding this comment.
There is no need for two GCPROTECT_BEGIN regions. We can pass in a struct with two fields.
struct
{
OBJECTREF oRet;
OBJECTREF innerEx;
} gc;
gc.oRet = NULL;
gc.innerEx = NULL;
GCPROTECT_BEGIN(gc);
...
GCPROTECT_END();
return gc.oRet;| ResMgrGetString(W("ReflectionTypeLoad_LoadFailed"), &gc.str); | ||
|
|
||
| MethodDesc* pMD = MemberLoader::FindMethod(gc.o->GetMethodTable(), | ||
| COR_CTOR_METHOD_NAME, &gsig_IM_ArrType_ArrException_Str_RetVoid); |
There was a problem hiding this comment.
We should be able to remove ArrType_ArrException_Str_RetVoid from metasig.h.
First set of priority 3 conversions from #123864 — replace
MethodDescCallSite/CallDescrWorkercalls withUnmanagedCallersOnlyreverse P/Invoke calls.Converted call sites
appdomain.cppAppDomain::RaiseExitProcessEventON_PROCESS_EXITappdomain.cppAppDomain::OnUnhandledExceptionON_UNHANDLED_EXCEPTIONassembly.cppRunManagedStartupMANAGED_STARTUPassembly.cppRunMainInternal(TARGET_BROWSER)HANDLE_ASYNC_ENTRYPOINTinvokeutil.cppInvokeUtil::CreateClassLoadExceptReflectionTypeLoadExceptionctorinvokeutil.cppInvokeUtil::CreateTargetExceptTargetInvocationExceptionctorRemaining priority 3 items (follow ups)
RunMainInternal(dynamic MethodDesc)CustomAttribute_CreateCustomAttributeInstance(dynamic ctor, already a QCall)CorHost2::ExecuteAssembly(dynamic method lookup)FuncEvalWrapper/DoNormalFuncEval(debugger func-eval)Pattern
Follows the same conversion pattern as #124834 (priority 2). Each conversion adds an
[UnmanagedCallersOnly]managed wrapper with anException*out-parameter and replaces the nativeMethodDescCallSite+ARG_SLOTwithUnmanagedCallersOnlyCaller::InvokeThrowing.