Convert priority 3 MethodDescCallSite calls to UnmanagedCallersOnly#124854
Convert priority 3 MethodDescCallSite calls to UnmanagedCallersOnly#124854steveisok wants to merge 24 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
…-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 |
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);
src/coreclr/System.Private.CoreLib/src/System/AppContext.CoreCLR.cs
Outdated
Show resolved
Hide resolved
src/coreclr/System.Private.CoreLib/src/System/AppContext.CoreCLR.cs
Outdated
Show resolved
Hide resolved
|
Test failure |
|
The wasm temporary callhelpers needs to be regenerated as part of these PRs: https://github.com/dotnet/runtime/blob/main/docs/workflow/wasm-documentation.md#running-coreclr-callhelpers-generator |
The generated output included incorrect removals of System.Net.Primitives networking functions and accumulated drift from main. Will regenerate properly in a follow-up with a complete browser build. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| UnmanagedCallersOnlyCaller managedStartup(METHOD__STARTUP_HOOK_PROVIDER__MANAGED_STARTUP); | ||
| managedStartup.InvokeThrowing(s_wszDiagnosticStartupHookPaths); |
There was a problem hiding this comment.
The PR description's "Converted call sites" table claims that RunMainInternal (TARGET_BROWSER) / HANDLE_ASYNC_ENTRYPOINT in assembly.cpp has been converted, but the actual diff only converts RunManagedStartup. The RunMainInternal function still uses MethodDescCallSite. This appears to be an inaccuracy in the PR description, not the code — the actual "Remaining priority 3 items" section correctly notes that the main entry point in RunMainInternal is left as a follow-up.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.
You can also share your feedback on Copilot code review. Take the survey.
| DEFINE_METHOD(APPCONTEXT, ON_PROCESS_EXIT, OnProcessExit, SM_RetVoid) | ||
| DEFINE_METHOD(APPCONTEXT, ON_UNHANDLED_EXCEPTION, OnUnhandledException, SM_Obj_RetVoid) | ||
| DEFINE_METHOD(APPCONTEXT, ON_PROCESS_EXIT, OnProcessExit, SM_PtrException_RetVoid) | ||
| DEFINE_METHOD(APPCONTEXT, ON_UNHANDLED_EXCEPTION, OnUnhandledException, SM_PtrObj_PtrException_RetVoid) |
There was a problem hiding this comment.
A managed [UnmanagedCallersOnly] UCO wrapper for OnUnhandledException with the signature (object* pObj, Exception* pException) is missing. The corelib.h declares ON_UNHANDLED_EXCEPTION with metasig SM_PtrObj_PtrException_RetVoid, and appdomain.cpp calls raiseEvent.InvokeThrowing(pThrowable) expecting that wrapper to exist, but no such UCO method has been added to either AppContext.CoreCLR.cs or AppContext.cs. The existing AppContext.OnUnhandledException(object e) is not marked [UnmanagedCallersOnly] and does not have the Exception* out-parameter. Without this method, METHOD__APPCONTEXT__ON_UNHANDLED_EXCEPTION binding will fail at runtime when the unhandled exception event fires.
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.