Skip to content

Remove unneeded DebuggerU2MCatchHandleFrames - #132082

Merged
jkoritzinsky merged 3 commits into
dotnet:mainfrom
jkoritzinsky:debuggeru2m-cleanup
Aug 21, 2026
Merged

Remove unneeded DebuggerU2MCatchHandleFrames#132082
jkoritzinsky merged 3 commits into
dotnet:mainfrom
jkoritzinsky:debuggeru2m-cleanup

Conversation

@jkoritzinsky

Copy link
Copy Markdown
Member

Now that we call into managed code from unmanaged with UCO methods with try-catch blocks around the body, the DebuggerU2MCatchHandleFrames that represent "there's a catch-all in manually managed runtime code" no longer provide benefit.

This PR removes the unneeded frames. It also cleans up a bit of the usage in ComWrappers so we aren't rethrowing an exception just to immediately catch it and extract the HResult to a local (that we don't even return out anywhere).

Now that we call into managed code from unmanaged with UCO methods with try-catch blocks around the body, the DebuggerU2MCatchHandleFrames that represent "there's a catch-all in manually managed runtime code" no longer provide benefit.
This PR removes the unneeded frames. It also cleans up a bit of the usage in ComWrappers so we aren't rethrowing an exception just to immediately catch it and extract the HResult to a local (that we don't even return out anywhere).
@jkoritzinsky
jkoritzinsky marked this pull request as ready for review August 10, 2026 16:24
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

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 removes several uses of DebuggerU2MCatchHandlerFrame (and related “catch-all” metadata) across CoreCLR unmanaged-to-managed transition points, and simplifies some COM/dispatch interop paths that previously used dedicated debugger/EH wrapper scaffolding.

Changes:

  • Simplifies DebuggerU2MCatchHandlerFrame by removing the “catches all exceptions” flag and updating call sites accordingly.
  • Removes DebuggerU2MCatchHandlerFrame usage from JIT interface error-trap and COM IDispatch invocation paths, including deleting the InvokeMemberDebuggerWrapper helper.
  • Refactors ComWrappers interop invocation to avoid throw/rethrow patterns by using direct UCO invocation with an exception out-parameter.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
FileDescription
src/coreclr/vm/threads.cppUpdates DebuggerU2MCatchHandlerFrame construction to new signature while preserving push/pop lifetime.
src/coreclr/vm/jitinterface.cppRemoves debugger catch-handler frame from CEEInfo::runWithErrorTrap.
src/coreclr/vm/interoplibinterface_comwrappers.cppSwitches CallICustomQueryInterface invocation to InvokeDirect_Ret with an exception out-param.
src/coreclr/vm/frames.hRemoves catchesAllExceptions state from DebuggerU2MCatchHandlerFrame and updates constructors.
src/coreclr/vm/exceptionhandling.cppSimplifies the “unhandled by runtime” check involving topmost DebuggerU2MCatchHandlerFrame.
src/coreclr/vm/dispatchinfo.hRemoves the InvokeMemberDebuggerWrapper declaration.
src/coreclr/vm/dispatchinfo.cppRemoves InvokeMemberDebuggerWrapper implementation and calls InvokeMemberWorker directly.
Suppressed comments (1)

src/coreclr/vm/frames.h:1836

  • The "Notes" section a few lines above still says this frame is only used in DispatchInfo::InvokeMember, but that usage was removed and the frame is still used elsewhere (e.g., ManagedThreadBase_DispatchOuter in threads.cpp). Please update/remove that note so it remains accurate.
 static constexpr size_t NumObjRefs = offsetof(GCFrame, m_numObjRefs);
static constexpr size_t GCFlags = offsetof(GCFrame, m_gcFlags);
};
//-----------------------------------------------------------------------------

Comment threadsrc/coreclr/vm/interoplibinterface_comwrappers.cpp
CopilotAI review requested due to automatic review settings August 10, 2026 16:30

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

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

Suppressed comments (2)

src/coreclr/vm/interoplibinterface_comwrappers.cpp:346

  • gc.exceptionRef is GC-protected but never initialized before GCPROTECT_BEGIN(gc). Leaving an EXCEPTIONREF uninitialized can make the GC treat arbitrary stack data as an object reference, leading to memory corruption or crashes.
 struct
{
OBJECTREF objRef;
EXCEPTIONREF exceptionRef;
} gc;
gc.objRef = NULL;

src/coreclr/vm/frames.h:2017

  • The class comment’s note says this frame is only used by DispatchInfo.InvokeMember, but this PR removes that usage and the remaining usage is in ManagedThreadBase_DispatchOuter (threads.cpp). Please update the note to avoid misleading future readers.
 DebuggerU2MCatchHandlerFrame() : Frame(FrameIdentifier::DebuggerU2MCatchHandlerFrame)
{
WRAPPER_NO_CONTRACT;
Frame::Push();
}

CopilotAI review requested due to automatic review settings August 19, 2026 22:32

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

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

Suppressed comments (1)

src/coreclr/vm/interoplibinterface_comwrappers.cpp:356

  • InvokeDirect_Ret does not throw on managed exceptions; it reports them via the trailing Exception* parameter. In this path, ComWrappers.CoreCLR.CallICustomQueryInterface sets *pException and returns default (0 / Handled). Assigning result unconditionally can therefore report Handled with *obj == NULL, causing QueryInterface to return S_OK incorrectly. Only update result when exceptionRef is null; otherwise preserve FailedToInvoke (or map to Failed) so the caller doesn't treat the interface as found.
 UnmanagedCallersOnlyCaller callICustomQueryInterface(METHOD__COMWRAPPERS__CALL_ICUSTOMQUERYINTERFACE);
result = (TryInvokeICustomQueryInterfaceResult)callICustomQueryInterface.InvokeDirect_Ret<INT32>(&gc.objRef, &iid, obj, &gc.exceptionRef);
hr = gc.exceptionRef == NULL ? S_OK : gc.exceptionRef->GetHResult();

@janvorlijanvorli left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you!

@jkoritzinsky

Copy link
Copy Markdown
MemberAuthor

/ba-g TypeMapApp failure fixed in main before KBE issue was opened

@jkoritzinsky
jkoritzinsky enabled auto-merge (squash) August 21, 2026 17:48
@jkoritzinsky
jkoritzinsky merged commit ce930f9 into dotnet:mainAug 21, 2026
90 of 101 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants

@jkoritzinsky@janvorli@AaronRobinsonMSFT