Skip to content

Change unhandled exception detection #117620

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Change unhandled exception detection #117620

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/coreclr/vm/FrameTypes.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ FRAME_TYPE_NAME(DebuggerClassInitMarkFrame)
FRAME_TYPE_NAME(DebuggerExitFrame)
FRAME_TYPE_NAME(DebuggerU2MCatchHandlerFrame)
FRAME_TYPE_NAME(ExceptionFilterFrame)
FRAME_TYPE_NAME(UnhandledExceptionMarkerFrame)
Copy link
Member

Choose a reason for hiding this comment

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

Does the new Frame type need to be hooked up into cDac?

cc @max-charlamb

Copy link
Member

@jkotas jkotas Jul 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Alternatively, I am wondering whether it would be better to have a managed code at the root of these threads so that the unhandled exception handling for them works the same way as exception handling for external threads. Would it allow us to get rid of this special casing?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure I understand how that would work and how it would work the same way as for external threads. For external threads, we assume that the thread will either catch it or the exception will be reported as c++ unhandled. So we let it flow into the native code. But we cannot do that for the unhandled exceptions that we want to report as such, because at the point we would reach some top level catch, the stack frames of the throwing location would be gone. So when we would break into the debugger or have a dump caused by the unhandled exception, there would be no evidence on why it happened.

Copy link
Member

@jkotas jkotas Jul 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

If there is an unhandled external exception thrown on external thread (and the external thread does not have any catch in native code), we will end up reporting it like a regular unhandled managed exception as far as I can tell.

I have tried this: https://gist.github.com/jkotas/2d4f15056768c1b221a78684b4245fec

Console output:

Thread 24600 started.
Unhandled exception. System.Exception: My exception
   at Program.ThreadFunction(IntPtr lpParameter) in C:\repro\Program.cs:line 74

Stacktrace in the crash dump is - notice that it includes ThreadFunction where the exception was thrown originally:

kernel32!WerpReportFault+0xc5 [onecore\windows\feedback\core\faultrep\lib\faultrep.cpp @ 1384] 
KERNELBASE!UnhandledExceptionFilter+0x34c [minkernel\kernelbase\xcpt.c @ 701] 
ntdll!RtlpThreadExceptionFilter+0x2e [minkernel\ldr\rtlstrt.c @ 958] 
ntdll!RtlUserThreadStart$filt$0+0x3f [minkernel\ldr\rtlstrt.c @ 1185] 
ntdll!__C_specific_handler+0x93 [minkernel\crts\crtw32\misc\riscchandler.c @ 446] 
ntdll!RtlpExecuteHandlerForException+0xf [minkernel\ntos\rtl\amd64\xcptmisc.asm @ 132] 
ntdll!RtlDispatchException+0x437 [minkernel\ntos\rtl\amd64\exdsptch.c @ 680] 
ntdll!RtlRaiseException+0x206 [minkernel\ntos\rtl\amd64\raise.c @ 240] 
KERNELBASE!RaiseException+0x8a [minkernel\kernelbase\xcpt.c @ 954] 
coreclr!SfiNext+0x1e5 [D:\a\_work\1\s\src\runtime\src\coreclr\vm\exceptionhandling.cpp @ 4048] 
System_Private_CoreLib!System.Runtime.EH.DispatchEx+0x48e [_/src/runtime/src/coreclr/nativeaot/Runtime.Base/src/System/Runtime/ExceptionHandling.cs @ 900] 
System_Private_CoreLib!System.Runtime.EH.RhThrowEx+0x2d [_/src/runtime/src/coreclr/nativeaot/Runtime.Base/src/System/Runtime/ExceptionHandling.cs @ 673] 
coreclr!CallDescrWorkerInternal+0x83 [D:\a\_work\1\s\src\runtime\src\coreclr\vm\amd64\CallDescrWorkerAMD64.asm @ 74] 
coreclr!DispatchCallSimple+0x66 [D:\a\_work\1\s\src\runtime\src\coreclr\vm\callhelpers.cpp @ 238] 
coreclr!DispatchManagedException+0x1f9 [D:\a\_work\1\s\src\runtime\src\coreclr\vm\exceptionhandling.cpp @ 1665] 
coreclr!IL_Throw+0x10e [D:\a\_work\1\s\src\runtime\src\coreclr\vm\jithelpers.cpp @ 852] 
repro!Program.ThreadFunction+0x6b
repro!ILStubClass.IL_STUB_ReversePInvoke(Int64)+0x87
kernel32!BaseThreadInitThunk+0x17 [clientcore\base\win32\client\thread.c @ 77] 
ntdll!RtlUserThreadStart+0x2c [minkernel\ldr\rtlstrt.c @ 1184]

I do not see what we are getting by special casing finalizer and other threads created by the runtime.

Copy link
Member

Choose a reason for hiding this comment

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

I am trying to understand why this is so complicated. If it is too complicated for no good reason and it is too risky to simplify for .NET 10, it is fine to postpone the simplification to .NET 11.

Copy link
Member

Choose a reason for hiding this comment

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

We would not let the exception flow to the native caller on Unix. The idea is that the finalizer and other threads would start with a regular reverse P/Invoke transition (same way as on Native AOT). Unhandled exceptions stop at reverse P/Invoke transition on non-Windows. I believe that we preserve the stacktrace of the original fault when exception goes unhandled at reverse P/Invoke transition on Unix.

In other words, the main reason for this special casing seems to be that these few places do not enter the runtime via a regular reverse P/Invoke transition. They use a custom scheme that we need to compensate for.

I can believe that the diagnostic tests depend on implementation details and changing anything is going to break them.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure how it would work for the Main method entry. We call the Main via CallDescrWorker. Where would we fit in the reverse pinvoke transition?

Copy link
Member

Choose a reason for hiding this comment

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

Runtime C/C++ code would call the reverse PInvoke directly, without CallDescrWorker. The reverse PInvoke would take function pointer that points to Main method, some information about Main method signature (there are a few possible shapes - returning int vs. returning void, etc.), and the arguments to pass to main method.

The extra reverse PInvoke may impact diagnostic as discussed above. It would be show up in stacktraces, etc.

Copy link
Member

Choose a reason for hiding this comment

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

Related #117844 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

Your example works because we report the exception as unhandled in the EH and then re-raise it to flow into the native code to give the external native code a chance to catch it. If we didn't treat it as unhandled, the EH would currently unwind stack upto the native code frame and raise the exception from there. That's the way we use for CallDescrWorker. But that way looses the managed frames that triggered the exception.

If we used the same mechanism as we currently use for re-raising the exception when we consider it unhandled, but wanted to do it for any reverse pinvoke transition without checking if there are managed frames on the stack on top of that, then we would have a problem. This exception is re-raised when there are all the managed frames upto the one that has thrown the exception still on the stack. So re-raising from that point requires us to make sure that when the exception calls ProcessCLRException for all managed frames below the native frame, we don't do anything. That's currently done by marking the thread by TSNC_ProcessedUnhandledException. But if the exception would end up flowing through the native frames uncaught and then reach managed code again, we want the ProcessCLRException to start doing its job again past that point.
I am not trying to say we cannot do that, but it complicates things and will require some extra state to store the address of the boundary frame that would decide whether the ProcessCLRException should ignore the exception for a given frame or not. And in case the exception will be caught in the native code between those blocks of managed code, the state will become stale and we would need to clean it up at a right time later.

#ifdef FEATURE_INTERPRETER
FRAME_TYPE_NAME(InterpreterFrame)
#endif // FEATURE_INTERPRETER
Expand Down
13 changes: 12 additions & 1 deletion src/coreclr/vm/corhost.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,6 +295,8 @@ HRESULT CorHost2::ExecuteAssembly(DWORD dwAppDomainId,

AppDomain *pCurDomain = SystemDomain::GetCurrentDomain();

UnhandledExceptionMarkerFrame unhandledExceptionMarkerFrame;

Thread *pThread = GetThreadNULLOk();
if (pThread == NULL)
{
Expand All @@ -305,6 +307,11 @@ HRESULT CorHost2::ExecuteAssembly(DWORD dwAppDomainId,
}
}

{
GCX_COOP();
unhandledExceptionMarkerFrame.Push(pThread);
}

INSTALL_UNHANDLED_MANAGED_EXCEPTION_TRAP;
Copy link
Member

Choose a reason for hiding this comment

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

Should the unhandledExceptionMarkerFrame be coupled with INSTALL_UNHANDLED_MANAGED_EXCEPTION_TRAP? They seem to be handling same scenario.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for the suggestion, it looks like it would make sense to put the frame creation / destruction into the INSTALL_UNHANDLED_MANAGED_EXCEPTION_TRAP / UNINSTALL_UNHANDLED_MANAGED_EXCEPTION_TRAP

INSTALL_UNWIND_AND_CONTINUE_HANDLER;

Expand All @@ -327,7 +334,6 @@ HRESULT CorHost2::ExecuteAssembly(DWORD dwAppDomainId,

{
GCX_COOP();

PTRARRAYREF arguments = NULL;
GCPROTECT_BEGIN(arguments);

Expand Down Expand Up @@ -359,6 +365,11 @@ HRESULT CorHost2::ExecuteAssembly(DWORD dwAppDomainId,
UNINSTALL_UNWIND_AND_CONTINUE_HANDLER;
UNINSTALL_UNHANDLED_MANAGED_EXCEPTION_TRAP;

{
GCX_COOP();
unhandledExceptionMarkerFrame.Pop(pThread);
}

#ifdef LOG_EXECUTABLE_ALLOCATOR_STATISTICS
ExecutableAllocator::DumpHolderUsage();
ExecutionManager::DumpExecutionManagerUsage();
Copy link
Member

Choose a reason for hiding this comment

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

Does ExecuteInDefaultAppDomain below need the same change?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I've missed that one.

Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/vm/dispatchinfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2168,7 +2168,7 @@ HRESULT DispatchInfo::InvokeMember(SimpleComCallWrapper *pSimpleWrap, DISPID id,
// The sole purpose of having this frame is to tell the debugger that we have a catch handler here
// which may swallow managed exceptions. The debugger needs this in order to send a
// CatchHandlerFound (CHF) notification.
DebuggerU2MCatchHandlerFrame catchFrame(true /* catchesAllExceptions */);
DebuggerU2MCatchHandlerFrame catchFrame;

EX_TRY
{
Expand Down
9 changes: 5 additions & 4 deletions src/coreclr/vm/exceptionhandling.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4013,16 +4013,17 @@ extern "C" CLR_BOOL QCALLTYPE SfiNext(StackFrameIterator* pThis, uint* uExCollid
// Check if there are any further managed frames on the stack or a catch for all exceptions in native code (marked by
// DebuggerU2MCatchHandlerFrame with CatchesAllExceptions() returning true).
// If not, the exception is unhandled.
bool isNotHandledByRuntime =
(pFrame == FRAME_TOP) ||
(IsTopmostDebuggerU2MCatchHandlerFrame(pFrame) && !((DebuggerU2MCatchHandlerFrame*)pFrame)->CatchesAllExceptions())
bool reportUnhandledException =
Copy link
Member

@jkotas jkotas Jul 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The comment about DebuggerU2MCatchHandlerFrame above needs updating

((pFrame != FRAME_TOP) &&
(pFrame->GetFrameIdentifier() == FrameIdentifier::UnhandledExceptionMarkerFrame) &&
IsExceptionFromManagedCode(pTopExInfo->m_ptrs.ExceptionRecord))
#ifdef HOST_UNIX
// Don't allow propagating exceptions from managed to non-runtime native code
|| isPropagatingToExternalNativeCode
#endif
;

if (isNotHandledByRuntime && IsExceptionFromManagedCode(pTopExInfo->m_ptrs.ExceptionRecord))
if (reportUnhandledException)
{
EH_LOG((LL_INFO100, "SfiNext (pass %d): no more managed frames on the stack, the exception is unhandled", pTopExInfo->m_passNumber));
if (pTopExInfo->m_passNumber == 1)
Expand Down
26 changes: 15 additions & 11 deletions src/coreclr/vm/frames.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,9 @@
// +-DebuggerU2MCatchHandlerFrame - marker frame to indicate that native code is going to catch and
// | swallow a managed exception
// |
// +- UnhandledExceptionMarkerFrame - When exception handling passes through this frame,
// | the exception is reported as unhandled.
// |
#ifdef DEBUGGING_SUPPORTED
// +-FuncEvalFrame - frame for debugger function evaluation
#endif // DEBUGGING_SUPPORTED
Expand Down Expand Up @@ -2077,15 +2080,13 @@ class DebuggerU2MCatchHandlerFrame : public Frame
{
public:
#ifndef DACCESS_COMPILE
DebuggerU2MCatchHandlerFrame(bool catchesAllExceptions) : Frame(FrameIdentifier::DebuggerU2MCatchHandlerFrame),
m_catchesAllExceptions(catchesAllExceptions)
DebuggerU2MCatchHandlerFrame() : Frame(FrameIdentifier::DebuggerU2MCatchHandlerFrame)
{
WRAPPER_NO_CONTRACT;
Frame::Push();
}

DebuggerU2MCatchHandlerFrame(Thread * pThread, bool catchesAllExceptions) : Frame(FrameIdentifier::DebuggerU2MCatchHandlerFrame),
m_catchesAllExceptions(catchesAllExceptions)
DebuggerU2MCatchHandlerFrame(Thread * pThread) : Frame(FrameIdentifier::DebuggerU2MCatchHandlerFrame)
{
WRAPPER_NO_CONTRACT;
Frame::Push(pThread);
Expand All @@ -2097,16 +2098,19 @@ class DebuggerU2MCatchHandlerFrame : public Frame
LIMITED_METHOD_DAC_CONTRACT;
return TT_U2M;
}
};

typedef DPTR(class UnhandledExceptionMarkerFrame) PTR_UnhandledExceptionMarkerFrame;

bool CatchesAllExceptions()
// When exception handling passes through this frame, the exception is reported as unhandled.
class UnhandledExceptionMarkerFrame : public Frame
{
public:
#ifndef DACCESS_COMPILE
UnhandledExceptionMarkerFrame() : Frame(FrameIdentifier::UnhandledExceptionMarkerFrame)
{
LIMITED_METHOD_DAC_CONTRACT;
return m_catchesAllExceptions;
}
Copy link
Preview

Copilot AI Jul 14, 2025

Choose a reason for hiding this comment

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

The UnhandledExceptionMarkerFrame class is missing the GetTransitionType() method override that is present in DebuggerU2MCatchHandlerFrame. This inconsistency may cause issues with frame type identification during stack walks.

Copilot uses AI. Check for mistakes.


private:
// The catch handled marked by the DebuggerU2MCatchHandlerFrame catches all exceptions.
bool m_catchesAllExceptions;
#endif
};

// Frame for the Reverse PInvoke (i.e. UnmanagedCallersOnlyAttribute).
Expand Down
15 changes: 5 additions & 10 deletions src/coreclr/vm/interoplibinterface_comwrappers.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -355,18 +355,15 @@ namespace InteropLibImports
return TryInvokeICustomQueryInterfaceResult::FailedToInvoke;
}

// Switch to Cooperative mode since object references
// are being manipulated and the catchFrame needs that so that it can push
// itself to the explicit frame stack.
GCX_COOP();
// Indicate to the debugger and exception handling that managed exceptions are being caught
// here.
DebuggerU2MCatchHandlerFrame catchFrame(true /* catchesAllExceptions */);

HRESULT hr;
auto result = TryInvokeICustomQueryInterfaceResult::FailedToInvoke;
EX_TRY_THREAD(CURRENT_THREAD)
{
// Switch to Cooperative mode since object references
// are being manipulated and the catchFrame needs that so that it can push
// itself to the explicit frame stack.
GCX_COOP();

struct
{
OBJECTREF objRef;
Expand All @@ -384,8 +381,6 @@ namespace InteropLibImports
}
EX_CATCH_HRESULT(hr);

catchFrame.Pop();

// Assert valid value.
_ASSERTE(TryInvokeICustomQueryInterfaceResult::Min <= result
&& result <= TryInvokeICustomQueryInterfaceResult::Max);
Expand Down
6 changes: 0 additions & 6 deletions src/coreclr/vm/jitinterface.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10512,12 +10512,8 @@ bool CEEInfo::runWithErrorTrap(void (*function)(void*), void* param)

bool success = true;

GCX_COOP();
DebuggerU2MCatchHandlerFrame catchFrame(true /* catchesAllExceptions */);

EX_TRY
{
GCX_PREEMP();
function(param);
}
EX_CATCH
Expand All @@ -10527,8 +10523,6 @@ bool CEEInfo::runWithErrorTrap(void (*function)(void*), void* param)
}
EX_END_CATCH

catchFrame.Pop();

return success;
}

Expand Down
8 changes: 6 additions & 2 deletions src/coreclr/vm/threads.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7069,7 +7069,10 @@ static void ManagedThreadBase_DispatchOuter(ManagedThreadCallState *pCallState)
// The sole purpose of having this frame is to tell the debugger that we have a catch handler here
// which may swallow managed exceptions. The debugger needs this in order to send a
// CatchHandlerFound (CHF) notification.
DebuggerU2MCatchHandlerFrame catchFrame(false /* catchesAllExceptions */);
DebuggerU2MCatchHandlerFrame catchFrame(pThread);

UnhandledExceptionMarkerFrame unhandledExceptionMarkerFrame;
unhandledExceptionMarkerFrame.Push(pThread);

TryParam param(pCallState);
param.pFrame = &catchFrame;
Expand Down Expand Up @@ -7124,7 +7127,8 @@ static void ManagedThreadBase_DispatchOuter(ManagedThreadCallState *pCallState)
}
PAL_FINALLY
{
catchFrame.Pop();
unhandledExceptionMarkerFrame.Pop(pThread);
catchFrame.Pop(pThread);
}
PAL_ENDTRY;
}
Expand Down
Loading