Move coreclr EH second pass to native code #119863

janvorli · 2025-09-18T20:04:09Z

There were some GC holes discovered caused by the fact that GC can be triggered during 2nd pass of EH in-between calls to finally handlers and catch handler. After considering options, moving the 2nd pass to native code seems the most reasonable solution.

janvorli · 2025-09-18T20:05:08Z

The current state is that it almost works. A catch handler is correctly invoked, but then something crashes.

Copilot

Pull Request Overview

This PR moves the CoreCLR exception handling second pass from managed to native code to fix GC holes that can occur when GC is triggered between finally handlers and catch handlers during the second pass of exception handling.

Splits existing RhThrowEx/RhThrowHwEx methods into separate first-pass handler finding methods and second-pass execution
Moves the second pass execution logic (DispatchExPass2) from managed to native code
Removes QCALL exports for CallCatchFunclet and CallFinallyFunclet as they're now called directly from native code

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
`src/coreclr/vm/qcallentrypoints.cpp`	Removes QCALL exports for CallCatchFunclet and CallFinallyFunclet
`src/coreclr/vm/metasig.h`	Updates method signatures to include handlingFrameSP and pCatchHandler out parameters
`src/coreclr/vm/exinfo.h`	Adds ContainsCodeOffset helper method to RhEHClause
`src/coreclr/vm/exceptionhandlingqcalls.h`	Removes QCALL declarations for catch/finally funclets
`src/coreclr/vm/exceptionhandling.h`	Adds declaration for new native DispatchExPass2 function
`src/coreclr/vm/exceptionhandling.cpp`	Major refactoring: converts QCALL functions to native, adds new DispatchExPass2 implementation
`src/coreclr/vm/excep.cpp`	Updates managed fault handling to use new method signatures
`src/coreclr/vm/corelib.h`	Updates method name mappings for exception handling entry points
`src/coreclr/nativeaot/Runtime.Base/src/System/Runtime/ExceptionHandling.cs`	Splits exception handling into separate first/second pass methods

src/coreclr/vm/exceptionhandling.cpp

src/coreclr/nativeaot/Runtime.Base/src/System/Runtime/ExceptionHandling.cs

janvorli · 2025-09-18T20:37:23Z

Also, please note that the collided unwind detection is not working yet, as I cannot rely on the pinvokes from the managed EH code anymore. I am working on a fix.
With the contract and EH clauses enumeration fixes based on David's feedback, many EH tests now pass until I hit one with collided unwind. I am reusing the interpreter test ran without interpreter for now.

janvorli · 2025-09-18T22:08:19Z

And one more thing not done yet is rethrow.

* Reflect PR feedback * Implement rethrow * Implement new way of collided unwind detection now that the CallCatchFunclet is not called via pinvoke * Remove forced reporting of EH code from stack frame iterator, as we now cannot have that code on the stack during 2nd pass

dotnet-policy-service · 2025-09-19T00:39:25Z

Tagging subscribers to 'arch-wasm': @lewing, @pavelsavara
See info in area-owners.md if you want to be subscribed.

src/coreclr/nativeaot/Runtime.Base/src/System/Runtime/ExceptionHandling.cs

janvorli · 2025-09-19T15:15:13Z

I am considering reducing the number of changes in the exceptionhandling.cs by a slightly different approach - keeping only the methods we had and adding an extra argument "firstPassOnly" to them. Then if that argument is set to true, in DispatchEx store the handlingFrameSP/PC, _pReversePInvokePropagationCallback/Context and the pCatchHandler into the ExInfo and return;
The native side would then take these from the ExInfo. On NativeAOT, this argument would be ignored.

The current state is that, with some additional changes that I am going to commit after I fix the offsets in StackFrameIterator for some targets, it passes all diagnostics tests and coreclr tests (except ForeignThreadExceptionTest where it ends up messing GC mode when calling managed callback from native code catch)

I also need to add proper contracts here and there.

src/coreclr/vm/exceptionhandling.cpp

davidwrighton · 2025-09-19T21:57:42Z

I would add some correlation comments so that we know that C++ function X is supposed to be the same as C# function Y. But otherwise, this actually looks good to me (assuming it works of course)

…nHandling.cs

janvorli · 2025-09-22T15:59:40Z

@jkotas I've added some contracts. I also had to change contract on the Debugger::FirstChanceManagedExceptionCatcherFound from GC_TRIGGERS_FROM_GETJITINFO to GC_NOTRIGGER. I cannot find how the GC can get triggered by the GetJitInfo (and it has GC_NOTRIGGER contract), so I hope I haven't missed anything.

janvorli · 2025-09-22T16:01:30Z

There is one issue left that I am investigating - Linux arm64 failures in several libraries tests.

src/coreclr/vm/exceptionhandling.cpp

janvorli · 2025-09-23T18:05:58Z

/azp run runtime-coreclr outerloop

azure-pipelines · 2025-09-23T18:06:17Z

Azure Pipelines successfully started running 1 pipeline(s).

janvorli · 2025-09-23T18:07:43Z

/azp run runtime-coreclr gcstress0x3-gcstress0xc

azure-pipelines · 2025-09-23T18:07:54Z

Azure Pipelines successfully started running 1 pipeline(s).

This is done to ensure that no GC is allowed between the scanned stack range is extended and a funclet for the current frame is called.

janvorli · 2025-09-24T20:47:43Z

@jkotas, @davidwrighton the current state of this PR is final from my point of view. Can you please review it?

jkotas · 2025-09-24T20:58:51Z

src/coreclr/nativeaot/Runtime.Base/src/System/Runtime/ExceptionHandling.cs

+
+#if TARGET_ARM64
+            [FieldOffset(AsmOffsets.OFFSETOF__ExInfo__m_handlingFramePC)]
+            internal volatile byte* _handlingFramePC;


Should we have a comment somewhere that explains what's special about Arm64 that it needs the handlingFramePC ?

kg · 2025-09-24T21:01:50Z

src/coreclr/vm/exceptionhandling.cpp


-extern "C" CLR_BOOL QCALLTYPE EHEnumNext(EH_CLAUSE_ENUMERATOR* pEHEnum, RhEHClause* pEHClause)
+// The doNotCalculateCatchType option makes the function skip calculation of the catch type. It is used in
+// the 2nd path of EH to avoid possible GC stemming from a call to ResolveEHClause.


nit: path -> pass?

davidwrighton · 2025-09-24T22:16:05Z

src/coreclr/vm/exceptionhandling.cpp

    MethodDesc *pMD = exInfo->m_frameIter.m_crawl.GetFunction();
    // Profiler, debugger and ETW events
    TADDR spForDebugger = GetSpForDiagnosticReporting(pvRegDisplay);
    exInfo->MakeCallbacksRelatedToHandler(true, pThread, pMD, &exInfo->m_CurrentClause, (DWORD_PTR)pHandlerIP, spForDebugger);


I don't understand how your are protecting the callbacks here against triggering GCs. AFAIK BEGINFORBIDGC and ENDFORBIDGC are debug only macros. They have no effect in retail builds.

The callbacks that are called here don't trigger GC, there is a subset of profiler callbacks that are documented here https://github.com/dotnet/runtime/blob/fc1dee66fac8ae088d01d07b4a2faaeef15a234a/docs/design/coreclr/botr/profiling.md#gc-safe-callouts
that runtime calls without switching to preemptive mode.

janvorli added this to the 10.0.0 milestone Sep 18, 2025

janvorli requested review from jkotas and davidwrighton September 18, 2025 20:04

janvorli self-assigned this Sep 18, 2025

janvorli requested a review from MichalStrehovsky as a code owner September 18, 2025 20:04

Copilot AI review requested due to automatic review settings September 18, 2025 20:04

janvorli added NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) area-ExceptionHandling-coreclr labels Sep 18, 2025

Copilot AI reviewed Sep 18, 2025

View reviewed changes

src/coreclr/vm/exceptionhandling.cpp Outdated Show resolved Hide resolved

src/coreclr/vm/exceptionhandling.cpp Show resolved Hide resolved

src/coreclr/vm/exceptionhandling.cpp Show resolved Hide resolved

davidwrighton reviewed Sep 18, 2025

View reviewed changes

src/coreclr/vm/exceptionhandling.cpp Outdated Show resolved Hide resolved

davidwrighton reviewed Sep 18, 2025

View reviewed changes

src/coreclr/vm/exceptionhandling.cpp Outdated Show resolved Hide resolved

SingleAccretion reviewed Sep 18, 2025

View reviewed changes

src/coreclr/nativeaot/Runtime.Base/src/System/Runtime/ExceptionHandling.cs Outdated Show resolved Hide resolved

build-analysis bot mentioned this pull request Sep 18, 2025

Unable to read data from the transport connection: An existing connection was forcibly closed by the remote host. dotnet/dnceng#5922

Open

3 tasks

Several fixes

9a066bf

* Reflect PR feedback * Implement rethrow * Implement new way of collided unwind detection now that the CallCatchFunclet is not called via pinvoke * Remove forced reporting of EH code from stack frame iterator, as we now cannot have that code on the stack during 2nd pass

jkotas added the arch-wasm WebAssembly architecture label Sep 19, 2025

davidwrighton mentioned this pull request Sep 19, 2025

[clr-interp] Add support for a new model of stack handling for funclets in the interpreter #119787

Open

janvorli added 2 commits September 19, 2025 13:58

Fix arm64 build and remove now useless stuff from stackwalk

cf5d23c

Fix build break

0887cff

jkotas reviewed Sep 19, 2025

View reviewed changes

src/coreclr/nativeaot/Runtime.Base/src/System/Runtime/ExceptionHandling.cs Show resolved Hide resolved

This was referenced Sep 19, 2025

Cannot find 'arm64-v8a' device dotnet/dnceng#2284

Open

LibraryImportGenerator.Unit.Tests crashing on linux-x64 mono interpreter #100800

Open

AndyAyersMS mentioned this pull request Sep 19, 2025

Missing GC live range in a catch handler for a variable unused in the handler #119657

Closed

janvorli added 2 commits September 19, 2025 20:35

Fix offsets, exception interception and MUSL build break

cdfae1a

Alternative change with minimalistic managed code differences

dd51b6d

build-analysis bot mentioned this pull request Sep 19, 2025

[linux-x64] [mono-aot] Test Runtime_101731.TestConvertToInt64NativeSingle(3.4028235E+38) returns exit code 22 #112557

Open

Fix 32 bit offsets

da9abb5

davidwrighton reviewed Sep 19, 2025

View reviewed changes

src/coreclr/vm/exceptionhandling.cpp Outdated Show resolved Hide resolved

janvorli and others added 2 commits September 20, 2025 01:17

Fix arm64 issue

521dbb3

Update src/coreclr/nativeaot/Runtime.Base/src/System/Runtime/Exceptio…

43e791b

…nHandling.cs

This was referenced Sep 20, 2025

Infra timeout causing "Unable to obtain kernel buffer" dotnet/dnceng#4547

Open

System.Diagnostics.Process.Tests failure on net10.0-linux-Debug-arm64-Mono_Minijit_Debug #110643

Open

Add/fix some contracts

b2925cd

Fix another arm64 issue - need to use adjusted PC

27424a5

janvorli changed the title ~~[WIP] Move coreclr EH second pass to native code~~ Move coreclr EH second pass to native code Sep 22, 2025

janvorli removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Sep 22, 2025

kg reviewed Sep 22, 2025

View reviewed changes

src/coreclr/vm/exceptionhandling.cpp Outdated Show resolved Hide resolved

janvorli added 2 commits September 23, 2025 00:55

Change the GC prevention to single frame only

608b877

Fix one new contract issue

dde85e2

This was referenced Sep 23, 2025

The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#3008

Open

Unable to pull image from mcr.microsoft.com #117164

Open

janvorli mentioned this pull request Sep 23, 2025

Fix several issues with interpreter and EH #119784

Open

Update GC forbid locations

53bcc9a

This is done to ensure that no GC is allowed between the scanned stack range is extended and a funclet for the current frame is called.

jkotas reviewed Sep 24, 2025

View reviewed changes

kg reviewed Sep 24, 2025

View reviewed changes

davidwrighton reviewed Sep 24, 2025

View reviewed changes

build-analysis bot mentioned this pull request Sep 25, 2025

Test failure: baseservices/exceptions/stackoverflow/stackoverflowtester/stackoverflowtester.cmd #110173

Open

Move coreclr EH second pass to native code #119863

Are you sure you want to change the base?

Move coreclr EH second pass to native code #119863

Conversation

janvorli commented Sep 18, 2025

Uh oh!

janvorli commented Sep 18, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

janvorli commented Sep 18, 2025

Uh oh!

janvorli commented Sep 18, 2025

Uh oh!

dotnet-policy-service bot commented Sep 19, 2025

Uh oh!

Uh oh!

janvorli commented Sep 19, 2025

Uh oh!

Uh oh!

davidwrighton commented Sep 19, 2025

Uh oh!

janvorli commented Sep 22, 2025

Uh oh!

janvorli commented Sep 22, 2025

Uh oh!

Uh oh!

janvorli commented Sep 23, 2025

Uh oh!

azure-pipelines bot commented Sep 23, 2025

Uh oh!

janvorli commented Sep 23, 2025

Uh oh!

azure-pipelines bot commented Sep 23, 2025

Uh oh!

janvorli commented Sep 24, 2025

Uh oh!

jkotas Sep 24, 2025

Choose a reason for hiding this comment

Uh oh!

kg Sep 24, 2025

Choose a reason for hiding this comment

Uh oh!

davidwrighton Sep 24, 2025

Choose a reason for hiding this comment

Uh oh!

janvorli Sep 24, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!