Skip to content

Convert constructor invocations from the runtime to use UnmanagedCallersOnly#124920

Open
jkoritzinsky wants to merge 11 commits intodotnet:mainfrom
jkoritzinsky:chf-uco
Open

Convert constructor invocations from the runtime to use UnmanagedCallersOnly#124920
jkoritzinsky wants to merge 11 commits intodotnet:mainfrom
jkoritzinsky:chf-uco

Conversation

@jkoritzinsky
Copy link
Member

This PR moves the class constructor and instance constructor invocations from within the runtime to use the UnmanagedCallersOnly pattern established with #123864 to invoke the constructors.

Contributes to #123864.

As this PR removes all usages of CATCH_HANDLER_FOUND_NOTIFICATION_CALLSITE and PREPARE_NONVIRTUAL_CALLSITE_USING_METHODDESC , these pieces of infrastructure are removed as well.

…RE_NONVIRTUAL_CALLSITE_USING_METHODDESC to use UnmanagedCallersOnly
@dotnet-policy-service
Copy link
Contributor

Tagging subscribers to this area: @agocke
See info in area-owners.md if you want to be subscribed.

Copy link
Contributor

Copilot AI left a 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 PR continues the migration away from MethodDescCallSite / CallDescrWorker-style VM-to-managed invocation, switching class constructor and default instance constructor calls over to the UnmanagedCallersOnly reverse P/Invoke pattern (per #123864). It also removes the now-unused CATCH_HANDLER_FOUND_NOTIFICATION_CALLSITE / PREPARE_NONVIRTUAL_CALLSITE_USING_METHODDESC infrastructure.

Changes:

  • Route MethodTable::RunClassInitEx (.cctor invocation) through a new InitHelpers.CallClassConstructor [UnmanagedCallersOnly] entrypoint and UnmanagedCallersOnlyCaller.
  • Route default constructor invocations through a new RuntimeHelpers.CallDefaultConstructor [UnmanagedCallersOnly] entrypoint and UnmanagedCallersOnlyCaller.
  • Remove debugger notification dispatch flag and wrapper logic from DispatchCallSimple/call helper macros.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
src/coreclr/vm/methodtable.cpp Switches class constructor invocation to UnmanagedCallersOnlyCaller.
src/coreclr/vm/corelib.h Adds CoreLib binder entries for the new UCO-managed helpers.
src/coreclr/vm/cominterfacemarshaler.cpp Switches extensible RCW default ctor invocation to UCO path.
src/coreclr/vm/callhelpers.h Removes catch-handler-found notification flag/macro; retains UCO caller helper.
src/coreclr/vm/callhelpers.cpp Removes debugger wrapper dispatch path; switches default ctor helper to UCO path.
src/coreclr/System.Private.CoreLib/.../RuntimeHelpers.CoreCLR.cs Adds [UnmanagedCallersOnly] method for default ctor invocation.
src/coreclr/System.Private.CoreLib/.../InitHelpers.cs Adds [UnmanagedCallersOnly] method for class constructor invocation.

@jkotas
Copy link
Member

jkotas commented Feb 27, 2026

I think the right solution here is to do the allocation in managed code as well.

We would have to use reflection (Activator.CreateInstance) for that. We may find surprising cycles between reflection and EEException::CreateThrowable where these constructors are called from in the VM. EEException::CreateThrowable is several layers lower than reflection. I think it is ok to leave it as is for this PR.

@AaronRobinsonMSFT
Copy link
Member

I think the right solution here is to do the allocation in managed code as well.

We would have to use reflection (Activator.CreateInstance) for that. We may find surprising cycles between reflection and EEException::CreateThrowable where these constructors are called from in the VM. EEException::CreateThrowable is several layers lower than reflection. I think it is ok to leave it as is for this PR.

Okay.

Copilot AI review requested due to automatic review settings February 27, 2026 22:29
Copy link
Contributor

Copilot AI left a 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 12 out of 12 changed files in this pull request and generated 1 comment.

Copilot AI review requested due to automatic review settings February 28, 2026 01:00
Copy link
Contributor

Copilot AI left a 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 12 out of 12 changed files in this pull request and generated 1 comment.

Copilot AI review requested due to automatic review settings February 28, 2026 01:35
Copy link
Contributor

Copilot AI left a 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 12 out of 12 changed files in this pull request and generated no new comments.

Copilot AI review requested due to automatic review settings March 2, 2026 18:32
@jkoritzinsky jkoritzinsky enabled auto-merge (squash) March 2, 2026 18:32
Copy link
Contributor

Copilot AI left a 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 12 out of 12 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

src/coreclr/vm/excep.cpp:8286

  • The comment at line 8285-8286, "since we are here because of a cross AD reraise of the original exception," is now stale. With the removal of AppDomainTransitionExceptionFilter and the updated assertion at line 8281 (_ASSERTE((fIsThreadAbortException || fIsPreallocatedOOMExceptionForTA) && pUEWatsonBucketTracker->CapturedForThreadAbort())), the if (!fIsThreadAbortException) branch can only be reached in the fIsPreallocatedOOMExceptionForTA case—not an AD transition. The comment should be updated to reflect the remaining scenario (preallocated OOM exception during thread abort).
            if (!fIsThreadAbortException)
            {
                // The watson bucket tracker for the exceptiong being raised should be empty at this point
                // since we are here because of a cross AD reraise of the original exception.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings March 2, 2026 18:59
Copy link
Contributor

Copilot AI left a 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 12 out of 12 changed files in this pull request and generated no new comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

5 participants