From 5265735b38a409022568490e1bdfd4c9b38bd4b0 Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Tue, 18 Feb 2025 17:48:39 +0100 Subject: [PATCH 1/3] Fix exception handling in the prestub worker There is a bug in the new exception handling when ThePreStub is called from CallDescrWorkerInternal and the exception is propagated through that. One of such cases is when a managed class is being initialized during JITting and the constructor is in an assembly that's not found. The bug results in skipping all the native frames upto a managed frame that called that native chain that lead to the exception. In the specific case I've mentioned, a lock in the native code is left in locked state. That later leads to a hang. This was case was observed with Roslyn invoking an analyzer where one of the dependencies was missing. The fix is to ensure that when ThePreStub is called by CallDescrWorkerInternal, the exception is not caught in the PreStubWorker. It is left flowing into the native calling code instead. The same treatment is applied to ExternalMethodFixupWorker and VSD_ResolveWorker too. On Windows, we also need to prevent the ProcessCLRException invocation to call into the managed exception handling code. --- src/coreclr/vm/excep.cpp | 17 +-- src/coreclr/vm/exceptionhandling.cpp | 18 ++- src/coreclr/vm/exceptmacros.h | 16 ++- src/coreclr/vm/prestub.cpp | 114 +++++++++++------- src/coreclr/vm/virtualcallstub.cpp | 21 ++-- .../GitHub_76531/dependencytodelete.cs | 14 +++ .../GitHub_76531/dependencytodelete.csproj | 9 ++ .../coreclr/GitHub_76531/tailcallinvoker.il | 17 +++ .../GitHub_76531/tailcallinvoker.ilproj | 10 ++ .../coreclr/GitHub_76531/test76531.cs | 82 +++++++++++++ .../coreclr/GitHub_76531/test76531.csproj | 17 +++ src/tests/issues.targets | 3 + 12 files changed, 270 insertions(+), 68 deletions(-) create mode 100644 src/tests/Regressions/coreclr/GitHub_76531/dependencytodelete.cs create mode 100644 src/tests/Regressions/coreclr/GitHub_76531/dependencytodelete.csproj create mode 100644 src/tests/Regressions/coreclr/GitHub_76531/tailcallinvoker.il create mode 100644 src/tests/Regressions/coreclr/GitHub_76531/tailcallinvoker.ilproj create mode 100644 src/tests/Regressions/coreclr/GitHub_76531/test76531.cs create mode 100644 src/tests/Regressions/coreclr/GitHub_76531/test76531.csproj diff --git a/src/coreclr/vm/excep.cpp b/src/coreclr/vm/excep.cpp index 985310c640a942..5734d184610eee 100644 --- a/src/coreclr/vm/excep.cpp +++ b/src/coreclr/vm/excep.cpp @@ -3053,14 +3053,17 @@ void StackTraceInfo::AppendElement(OBJECTHANDLE hThrowable, UINT_PTR currentIP, // This is a workaround to fix the generation of stack traces from exception objects so that // they point to the line that actually generated the exception instead of the line // following. - if (pCf->IsIPadjusted()) + if (pCf != NULL) { - stackTraceElem.flags |= STEF_IP_ADJUSTED; - } - else if (!pCf->HasFaulted() && stackTraceElem.ip != 0) - { - stackTraceElem.ip -= STACKWALK_CONTROLPC_ADJUST_OFFSET; - stackTraceElem.flags |= STEF_IP_ADJUSTED; + if (pCf->IsIPadjusted()) + { + stackTraceElem.flags |= STEF_IP_ADJUSTED; + } + else if (!pCf->HasFaulted() && stackTraceElem.ip != 0) + { + stackTraceElem.ip -= STACKWALK_CONTROLPC_ADJUST_OFFSET; + stackTraceElem.flags |= STEF_IP_ADJUSTED; + } } #ifndef TARGET_UNIX // Watson is supported on Windows only diff --git a/src/coreclr/vm/exceptionhandling.cpp b/src/coreclr/vm/exceptionhandling.cpp index 54963e0e2995d2..f9e4cc57cb5996 100644 --- a/src/coreclr/vm/exceptionhandling.cpp +++ b/src/coreclr/vm/exceptionhandling.cpp @@ -932,14 +932,13 @@ ProcessCLRExceptionNew(IN PEXCEPTION_RECORD pExceptionRecord, Thread* pThread = GetThread(); - if (pThread->HasThreadStateNC(Thread::TSNC_ProcessedUnhandledException)) + // Skip native frames of asm helpers that have the ProcessCLRException set as their personality routine. + // There is nothing to do for those with the new exception handling. + // Also skip all frames when processing unhandled exceptions. That allows them to reach the host app + // level and let 3rd party the chance to handle them. + if (!ExecutionManager::IsManagedCode((PCODE)pDispatcherContext->ControlPc) || + pThread->HasThreadStateNC(Thread::TSNC_ProcessedUnhandledException)) { - if ((pExceptionRecord->ExceptionFlags & EXCEPTION_UNWINDING)) - { - GCX_COOP(); - PopExplicitFrames(pThread, (void*)pDispatcherContext->EstablisherFrame, (void*)GetSP(pDispatcherContext->ContextRecord)); - ExInfo::PopExInfos(pThread, (void*)pDispatcherContext->EstablisherFrame); - } return ExceptionContinueSearch; } @@ -8514,7 +8513,7 @@ static StackWalkAction MoveToNextNonSkippedFrame(StackFrameIterator* pStackFrame return retVal; } -extern "C" size_t CallDescrWorkerInternalReturnAddressOffset; +bool IsCallDescrWorkerInternalReturnAddress(PCODE pCode); extern "C" bool QCALLTYPE SfiNext(StackFrameIterator* pThis, uint* uExCollideClauseIdx, bool* fUnwoundReversePInvoke, bool* pfIsExceptionIntercepted) { @@ -8569,8 +8568,7 @@ extern "C" bool QCALLTYPE SfiNext(StackFrameIterator* pThis, uint* uExCollideCla } else { - size_t CallDescrWorkerInternalReturnAddress = (size_t)CallDescrWorkerInternal + CallDescrWorkerInternalReturnAddressOffset; - if (GetIP(pThis->m_crawl.GetRegisterSet()->pCallerContext) == CallDescrWorkerInternalReturnAddress) + if (IsCallDescrWorkerInternalReturnAddress(GetIP(pThis->m_crawl.GetRegisterSet()->pCallerContext))) { invalidRevPInvoke = true; } diff --git a/src/coreclr/vm/exceptmacros.h b/src/coreclr/vm/exceptmacros.h index fb21a8be5119b1..2c11c059853ba9 100644 --- a/src/coreclr/vm/exceptmacros.h +++ b/src/coreclr/vm/exceptmacros.h @@ -277,15 +277,22 @@ VOID DECLSPEC_NORETURN UnwindAndContinueRethrowHelperAfterCatch(Frame* pEntryFra #ifdef TARGET_UNIX VOID DECLSPEC_NORETURN DispatchManagedException(PAL_SEHException& ex, bool isHardwareException); -#define INSTALL_MANAGED_EXCEPTION_DISPATCHER \ +#define INSTALL_MANAGED_EXCEPTION_DISPATCHER_EX \ PAL_SEHException exCopy; \ bool hasCaughtException = false; \ try { -#define UNINSTALL_MANAGED_EXCEPTION_DISPATCHER \ +#define INSTALL_MANAGED_EXCEPTION_DISPATCHER \ + INSTALL_MANAGED_EXCEPTION_DISPATCHER_EX + +#define UNINSTALL_MANAGED_EXCEPTION_DISPATCHER_EX(nativeRethrow) \ } \ catch (PAL_SEHException& ex) \ { \ + if (nativeRethrow) \ + { \ + throw; \ + } \ exCopy = std::move(ex); \ hasCaughtException = true; \ } \ @@ -294,6 +301,9 @@ VOID DECLSPEC_NORETURN DispatchManagedException(PAL_SEHException& ex, bool isHar DispatchManagedException(exCopy, false);\ } +#define UNINSTALL_MANAGED_EXCEPTION_DISPATCHER \ + UNINSTALL_MANAGED_EXCEPTION_DISPATCHER_EX(false) + // Install trap that catches unhandled managed exception and dumps its stack #define INSTALL_UNHANDLED_MANAGED_EXCEPTION_TRAP \ try { @@ -315,7 +325,9 @@ VOID DECLSPEC_NORETURN DispatchManagedException(PAL_SEHException& ex, bool isHar #else // TARGET_UNIX #define INSTALL_MANAGED_EXCEPTION_DISPATCHER +#define INSTALL_MANAGED_EXCEPTION_DISPATCHER_EX #define UNINSTALL_MANAGED_EXCEPTION_DISPATCHER +#define UNINSTALL_MANAGED_EXCEPTION_DISPATCHER_EX #define INSTALL_UNHANDLED_MANAGED_EXCEPTION_TRAP #define UNINSTALL_UNHANDLED_MANAGED_EXCEPTION_TRAP diff --git a/src/coreclr/vm/prestub.cpp b/src/coreclr/vm/prestub.cpp index c77922c6220938..b231b5c571441e 100644 --- a/src/coreclr/vm/prestub.cpp +++ b/src/coreclr/vm/prestub.cpp @@ -2538,6 +2538,20 @@ Stub * MakeInstantiatingStubWorker(MethodDesc *pMD) } #endif // defined(FEATURE_SHARE_GENERIC_CODE) +extern "C" size_t CallDescrWorkerInternalReturnAddressOffset; + +bool IsCallDescrWorkerInternalReturnAddress(PCODE pCode) +{ + LIMITED_METHOD_CONTRACT; +#ifdef FEATURE_EH_FUNCLETS + size_t CallDescrWorkerInternalReturnAddress = (size_t)CallDescrWorkerInternal + CallDescrWorkerInternalReturnAddressOffset; + + return pCode == CallDescrWorkerInternalReturnAddress; +#else // FEATURE_EH_FUNCLETS + return false; +#endif // FEATURE_EH_FUNCLETS +} + //============================================================================= // This function generates the real code when from Preemptive mode. // It is specifically designed to work with the UnmanagedCallersOnlyAttribute. @@ -2633,60 +2647,76 @@ extern "C" PCODE STDCALL PreStubWorker(TransitionBlock* pTransitionBlock, Method pPFrame->Push(CURRENT_THREAD); - INSTALL_MANAGED_EXCEPTION_DISPATCHER; - INSTALL_UNWIND_AND_CONTINUE_HANDLER; + EX_TRY + { + bool propagateExceptionToNativeCode = IsCallDescrWorkerInternalReturnAddress(pTransitionBlock->m_ReturnAddress); - // Make sure the method table is restored, and method instantiation if present - pMD->CheckRestore(); - CONSISTENCY_CHECK(GetAppDomain()->CheckCanExecuteManagedCode(pMD)); + INSTALL_MANAGED_EXCEPTION_DISPATCHER_EX; + INSTALL_UNWIND_AND_CONTINUE_HANDLER_EX; - MethodTable* pDispatchingMT = NULL; - if (pMD->IsVtableMethod()) - { - OBJECTREF curobj = pPFrame->GetThis(); + // Make sure the method table is restored, and method instantiation if present + pMD->CheckRestore(); + CONSISTENCY_CHECK(GetAppDomain()->CheckCanExecuteManagedCode(pMD)); - if (curobj != NULL) // Check for virtual function called non-virtually on a NULL object + MethodTable* pDispatchingMT = NULL; + if (pMD->IsVtableMethod()) { - pDispatchingMT = curobj->GetMethodTable(); + OBJECTREF curobj = pPFrame->GetThis(); - if (pDispatchingMT->IsIDynamicInterfaceCastable()) + if (curobj != NULL) // Check for virtual function called non-virtually on a NULL object { - MethodTable* pMDMT = pMD->GetMethodTable(); - TypeHandle objectType(pDispatchingMT); - TypeHandle methodType(pMDMT); + pDispatchingMT = curobj->GetMethodTable(); - GCStress::MaybeTrigger(); - INDEBUG(curobj = NULL); // curobj is unprotected and CanCastTo() can trigger GC - if (!objectType.CanCastTo(methodType)) + if (pDispatchingMT->IsIDynamicInterfaceCastable()) { - // Apparently IDynamicInterfaceCastable magic was involved when we chose this method to be called - // that's why we better stick to the MethodTable it belongs to, otherwise - // DoPrestub() will fail not being able to find implementation for pMD in pDispatchingMT. + MethodTable* pMDMT = pMD->GetMethodTable(); + TypeHandle objectType(pDispatchingMT); + TypeHandle methodType(pMDMT); + + GCStress::MaybeTrigger(); + INDEBUG(curobj = NULL); // curobj is unprotected and CanCastTo() can trigger GC + if (!objectType.CanCastTo(methodType)) + { + // Apparently IDynamicInterfaceCastable magic was involved when we chose this method to be called + // that's why we better stick to the MethodTable it belongs to, otherwise + // DoPrestub() will fail not being able to find implementation for pMD in pDispatchingMT. - pDispatchingMT = pMDMT; + pDispatchingMT = pMDMT; + } } - } - // For value types, the only virtual methods are interface implementations. - // Thus pDispatching == pMT because there - // is no inheritance in value types. Note the BoxedEntryPointStubs are shared - // between all sharable generic instantiations, so the == test is on - // canonical method tables. + // For value types, the only virtual methods are interface implementations. + // Thus pDispatching == pMT because there + // is no inheritance in value types. Note the BoxedEntryPointStubs are shared + // between all sharable generic instantiations, so the == test is on + // canonical method tables. #ifdef _DEBUG - MethodTable* pMDMT = pMD->GetMethodTable(); // put this here to see what the MT is in debug mode - _ASSERTE(!pMD->GetMethodTable()->IsValueType() || - (pMD->IsUnboxingStub() && (pDispatchingMT->GetCanonicalMethodTable() == pMDMT->GetCanonicalMethodTable()))); + MethodTable* pMDMT = pMD->GetMethodTable(); // put this here to see what the MT is in debug mode + _ASSERTE(!pMD->GetMethodTable()->IsValueType() || + (pMD->IsUnboxingStub() && (pDispatchingMT->GetCanonicalMethodTable() == pMDMT->GetCanonicalMethodTable()))); #endif // _DEBUG + } } - } - GCX_PREEMP_THREAD_EXISTS(CURRENT_THREAD); + GCX_PREEMP_THREAD_EXISTS(CURRENT_THREAD); + { + pbRetVal = pMD->DoPrestub(pDispatchingMT, CallerGCMode::Coop); + } + + UNINSTALL_UNWIND_AND_CONTINUE_HANDLER_EX(propagateExceptionToNativeCode); + UNINSTALL_MANAGED_EXCEPTION_DISPATCHER_EX(propagateExceptionToNativeCode); + } + EX_CATCH { - pbRetVal = pMD->DoPrestub(pDispatchingMT, CallerGCMode::Coop); + if (g_isNewExceptionHandlingEnabled) + { + OBJECTHANDLE ohThrowable = CURRENT_THREAD->LastThrownObjectHandle(); + _ASSERTE(ohThrowable); + StackTraceInfo::AppendElement(ohThrowable, 0, (UINT_PTR)pTransitionBlock, pMD, NULL); + } + EX_RETHROW; } - - UNINSTALL_UNWIND_AND_CONTINUE_HANDLER; - UNINSTALL_MANAGED_EXCEPTION_DISPATCHER; + EX_END_CATCH(SwallowAllExceptions) { HardwareExceptionHolder; @@ -3156,8 +3186,10 @@ EXTERN_C PCODE STDCALL ExternalMethodFixupWorker(TransitionBlock * pTransitionBl pEMFrame->Push(CURRENT_THREAD); // Push the new ExternalMethodFrame onto the frame stack - INSTALL_MANAGED_EXCEPTION_DISPATCHER; - INSTALL_UNWIND_AND_CONTINUE_HANDLER; + bool propagateExceptionToNativeCode = IsCallDescrWorkerInternalReturnAddress(pTransitionBlock->m_ReturnAddress); + + INSTALL_MANAGED_EXCEPTION_DISPATCHER_EX; + INSTALL_UNWIND_AND_CONTINUE_HANDLER_EX; bool fVirtual = false; MethodDesc * pMD = NULL; @@ -3399,8 +3431,8 @@ EXTERN_C PCODE STDCALL ExternalMethodFixupWorker(TransitionBlock * pTransitionBl } // Ready to return - UNINSTALL_UNWIND_AND_CONTINUE_HANDLER; - UNINSTALL_MANAGED_EXCEPTION_DISPATCHER; + UNINSTALL_UNWIND_AND_CONTINUE_HANDLER_EX(propagateExceptionToNativeCode); + UNINSTALL_MANAGED_EXCEPTION_DISPATCHER_EX(propagateExceptionToNativeCode); pEMFrame->Pop(CURRENT_THREAD); // Pop the ExternalMethodFrame from the frame stack diff --git a/src/coreclr/vm/virtualcallstub.cpp b/src/coreclr/vm/virtualcallstub.cpp index c22fbaedd7e53d..a2bfb6f2c49604 100644 --- a/src/coreclr/vm/virtualcallstub.cpp +++ b/src/coreclr/vm/virtualcallstub.cpp @@ -1262,6 +1262,8 @@ ResolveCacheElem* __fastcall VirtualCallStubManager::PromoteChainEntry(ResolveCa } #endif // CHAIN_LOOKUP +bool IsCallDescrWorkerInternalReturnAddress(PCODE pCode); + /* Resolve to a method and return its address or NULL if there is none. Our return value is the target address that control should continue to. Our caller will enter the target address as if a direct call with the original stack frame had been made from @@ -1309,14 +1311,16 @@ PCODE VSD_ResolveWorker(TransitionBlock * pTransitionBlock, PCODE target = (PCODE)NULL; + bool propagateExceptionToNativeCode = IsCallDescrWorkerInternalReturnAddress(pTransitionBlock->m_ReturnAddress); + if (pObj == NULL) { pSDFrame->SetForNullReferenceException(); pSDFrame->Push(CURRENT_THREAD); - INSTALL_MANAGED_EXCEPTION_DISPATCHER; - INSTALL_UNWIND_AND_CONTINUE_HANDLER; + INSTALL_MANAGED_EXCEPTION_DISPATCHER_EX; + INSTALL_UNWIND_AND_CONTINUE_HANDLER_EX; COMPlusThrow(kNullReferenceException); - UNINSTALL_UNWIND_AND_CONTINUE_HANDLER; - UNINSTALL_MANAGED_EXCEPTION_DISPATCHER; + UNINSTALL_UNWIND_AND_CONTINUE_HANDLER_EX(propagateExceptionToNativeCode); + UNINSTALL_MANAGED_EXCEPTION_DISPATCHER_EX(propagateExceptionToNativeCode); _ASSERTE(!"Throw returned"); } @@ -1351,8 +1355,9 @@ PCODE VSD_ResolveWorker(TransitionBlock * pTransitionBlock, pSDFrame->SetRepresentativeSlot(pRepresentativeMT, representativeToken.GetSlotNumber()); pSDFrame->Push(CURRENT_THREAD); - INSTALL_MANAGED_EXCEPTION_DISPATCHER; - INSTALL_UNWIND_AND_CONTINUE_HANDLER; + + INSTALL_MANAGED_EXCEPTION_DISPATCHER_EX; + INSTALL_UNWIND_AND_CONTINUE_HANDLER_EX; // For Virtual Delegates the m_siteAddr is a field of a managed object // Thus we have to report it as an interior pointer, @@ -1388,8 +1393,8 @@ PCODE VSD_ResolveWorker(TransitionBlock * pTransitionBlock, GCPROTECT_END(); - UNINSTALL_UNWIND_AND_CONTINUE_HANDLER; - UNINSTALL_MANAGED_EXCEPTION_DISPATCHER; + UNINSTALL_UNWIND_AND_CONTINUE_HANDLER_EX(propagateExceptionToNativeCode); + UNINSTALL_MANAGED_EXCEPTION_DISPATCHER_EX(propagateExceptionToNativeCode); pSDFrame->Pop(CURRENT_THREAD); return target; diff --git a/src/tests/Regressions/coreclr/GitHub_76531/dependencytodelete.cs b/src/tests/Regressions/coreclr/GitHub_76531/dependencytodelete.cs new file mode 100644 index 00000000000000..b00742958432fe --- /dev/null +++ b/src/tests/Regressions/coreclr/GitHub_76531/dependencytodelete.cs @@ -0,0 +1,14 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; + +namespace Dependency +{ + public class DependencyClass + { + public static void Test() + { + } + } +} diff --git a/src/tests/Regressions/coreclr/GitHub_76531/dependencytodelete.csproj b/src/tests/Regressions/coreclr/GitHub_76531/dependencytodelete.csproj new file mode 100644 index 00000000000000..fa1f2d01f80e57 --- /dev/null +++ b/src/tests/Regressions/coreclr/GitHub_76531/dependencytodelete.csproj @@ -0,0 +1,9 @@ + + + BuildOnly + Library + + + + + diff --git a/src/tests/Regressions/coreclr/GitHub_76531/tailcallinvoker.il b/src/tests/Regressions/coreclr/GitHub_76531/tailcallinvoker.il new file mode 100644 index 00000000000000..8cfd723e4be6e2 --- /dev/null +++ b/src/tests/Regressions/coreclr/GitHub_76531/tailcallinvoker.il @@ -0,0 +1,17 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +.assembly extern legacy library mscorlib {} +.assembly extern dependencytodelete {} +.assembly 'tailcallinvoker' {} + +.class public sequential ansi sealed beforefieldinit TailCallInvoker + extends [mscorlib]System.Object +{ + .method public static void Test() cil managed noinlining + { + .maxstack 1 + tail. call void [dependencytodelete]Dependency.DependencyClass::Test() + ret + } // end of method TailCallInvoker.Test +} // end of class TailCallInvoker \ No newline at end of file diff --git a/src/tests/Regressions/coreclr/GitHub_76531/tailcallinvoker.ilproj b/src/tests/Regressions/coreclr/GitHub_76531/tailcallinvoker.ilproj new file mode 100644 index 00000000000000..ae3ae9ac54895f --- /dev/null +++ b/src/tests/Regressions/coreclr/GitHub_76531/tailcallinvoker.ilproj @@ -0,0 +1,10 @@ + + + BuildOnly + Library + + + + + + diff --git a/src/tests/Regressions/coreclr/GitHub_76531/test76531.cs b/src/tests/Regressions/coreclr/GitHub_76531/test76531.cs new file mode 100644 index 00000000000000..4cd1573c309d66 --- /dev/null +++ b/src/tests/Regressions/coreclr/GitHub_76531/test76531.cs @@ -0,0 +1,82 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +using System; +using System.IO; +using System.Reflection; +using System.Runtime.CompilerServices; +using System.Runtime.InteropServices; +using Xunit; + +namespace Test76531 +{ + internal class Test + { + private static Dependency.DependencyClass? value; + + static Test() + { + value = new Dependency.DependencyClass(); + } + } + + public class MyObject : IDynamicInterfaceCastable + { + public RuntimeTypeHandle GetInterfaceImplementation(RuntimeTypeHandle interfaceType) + => throw new Exception("My exception"); + + public bool IsInterfaceImplemented(RuntimeTypeHandle interfaceType, bool throwIfNotImplemented) + => true; + + [MethodImpl(MethodImplOptions.AggressiveOptimization)] + public static void CallMe(MyObject o, Action d) => d((IMyInterface)o); + } + + public interface IMyInterface + { + void M(); + } + + public class Program + { + [Fact] + public static void TestExternalMethodFixupWorker() + { + File.Delete(Path.Combine(Path.GetDirectoryName(Assembly.GetExecutingAssembly().Location), "dependencytodelete.dll")); + Assert.Throws(() => + { + typeof(TailCallInvoker).GetMethod("Test")!.Invoke(null, null); + }); + } + + [Fact] + public static void TestPreStubWorker() + { + File.Delete(Path.Combine(Path.GetDirectoryName(Assembly.GetExecutingAssembly().Location), "dependencytodelete.dll")); + if (TestLibrary.Utilities.IsMonoRuntime) + { + Assert.Throws(() => + { + Test test = new (); + }); + } + else + { + // The exception is of different type with issue #76531 + Assert.Throws(() => + { + Test test = new (); + }); + } + } + + [Fact] + public static void TestVSD_ResolveWorker() + { + Assert.Throws(() => + { + var d = typeof(IMyInterface).GetMethod("M")!.CreateDelegate>(); + typeof(MyObject).GetMethod("CallMe")!.Invoke(null, [new MyObject(), d]); + }); + } + } +} diff --git a/src/tests/Regressions/coreclr/GitHub_76531/test76531.csproj b/src/tests/Regressions/coreclr/GitHub_76531/test76531.csproj new file mode 100644 index 00000000000000..ae1513d8e93546 --- /dev/null +++ b/src/tests/Regressions/coreclr/GitHub_76531/test76531.csproj @@ -0,0 +1,17 @@ + + + + true + + true + true + + + + + + + + + + diff --git a/src/tests/issues.targets b/src/tests/issues.targets index 1e2562569eb4fd..0ab073e0218d10 100644 --- a/src/tests/issues.targets +++ b/src/tests/issues.targets @@ -3084,6 +3084,9 @@ System.Diagnostics.Process is not supported + + Assembly.GetExecutingAssembly().Location returns NULL on WASM + System.Threading.Thread.UnsafeStart not supported From 730114c721591344021f5519bff2e16732f926b9 Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Tue, 18 Feb 2025 22:32:21 +0100 Subject: [PATCH 2/3] Fix missing ContextFlags setting I was hitting intermittent crashes in the unhandled exception test with GC stress C enabled due to this when GetSSP tried to access the SSP in the extended part of the context. --- src/coreclr/vm/exceptionhandling.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/coreclr/vm/exceptionhandling.cpp b/src/coreclr/vm/exceptionhandling.cpp index f9e4cc57cb5996..8748d309d68e69 100644 --- a/src/coreclr/vm/exceptionhandling.cpp +++ b/src/coreclr/vm/exceptionhandling.cpp @@ -6138,6 +6138,7 @@ BOOL IsSafeToUnwindFrameChain(Thread* pThread, LPVOID MemoryStackFpForFrameChain // Otherwise "unwind" to managed method REGDISPLAY rd; CONTEXT ctx; + ctx.ContextFlags = CONTEXT_CONTROL; SetIP(&ctx, 0); SetSP(&ctx, 0); FillRegDisplay(&rd, &ctx); From 7616a0f5b8a26aa6986ca0d9006837d6800cb69c Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Thu, 20 Feb 2025 14:39:43 +0100 Subject: [PATCH 3/3] Fix couple of issues * The previous set of changes removed popping of ExInfos too. That's not correct though. But it should be done in a different place than the ProcessCLRExceptionNew. * There was a problem (even before the fix) that an exception caught in DispatchInfo::InvokeMember was reported (via console log and to the debugger) as unhandled. This change also adds a new flavor of the unhandled exception test that throws an unhandled exception on a secondary thread to exercise the related code path. --- src/coreclr/vm/dispatchinfo.cpp | 2 +- src/coreclr/vm/exceptionhandling.cpp | 19 +++++++++++++++---- src/coreclr/vm/frames.h | 16 ++++++++++++++-- src/coreclr/vm/threads.cpp | 2 +- .../exceptions/unhandled/unhandled.cs | 7 +++++++ .../exceptions/unhandled/unhandledTester.cs | 10 +++++++++- 6 files changed, 47 insertions(+), 9 deletions(-) diff --git a/src/coreclr/vm/dispatchinfo.cpp b/src/coreclr/vm/dispatchinfo.cpp index 652d0927097299..bade7f6ce88636 100644 --- a/src/coreclr/vm/dispatchinfo.cpp +++ b/src/coreclr/vm/dispatchinfo.cpp @@ -2164,7 +2164,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; + DebuggerU2MCatchHandlerFrame catchFrame(true /* catchesAllExceptions */); EX_TRY { InvokeMemberDebuggerWrapper(pDispMemberInfo, diff --git a/src/coreclr/vm/exceptionhandling.cpp b/src/coreclr/vm/exceptionhandling.cpp index 8748d309d68e69..c0f4c6903aabc6 100644 --- a/src/coreclr/vm/exceptionhandling.cpp +++ b/src/coreclr/vm/exceptionhandling.cpp @@ -6184,9 +6184,16 @@ void CleanUpForSecondPass(Thread* pThread, bool fIsSO, LPVOID MemoryStackFpForFr // Instead, we rely on the END_SO_TOLERANT_CODE macro to call ClearExceptionStateAfterSO(). Of course, // we may leak in the UMThunkStubCommon() case where we don't have this macro lower on the stack // (stack grows up). - if (!fIsSO && !g_isNewExceptionHandlingEnabled) + if (!fIsSO) { - ExceptionTracker::PopTrackerIfEscaping(MemoryStackFp); + if (g_isNewExceptionHandlingEnabled) + { + ExInfo::PopExInfos(pThread, MemoryStackFp); + } + else + { + ExceptionTracker::PopTrackerIfEscaping(MemoryStackFp); + } } } @@ -8592,8 +8599,12 @@ extern "C" bool QCALLTYPE SfiNext(StackFrameIterator* pThis, uint* uExCollideCla _ASSERTE(pThis->GetFrameState() != StackFrameIterator::SFITER_SKIPPED_FRAME_FUNCTION); pFrame = pThis->m_crawl.GetFrame(); - // Check if there are any further managed frames on the stack, if not, the exception is unhandled. - if ((pFrame == FRAME_TOP) || IsTopmostDebuggerU2MCatchHandlerFrame(pFrame)) + + // 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. + if ((pFrame == FRAME_TOP) || + (IsTopmostDebuggerU2MCatchHandlerFrame(pFrame) && !((DebuggerU2MCatchHandlerFrame*)pFrame)->CatchesAllExceptions())) { if (pTopExInfo->m_passNumber == 1) { diff --git a/src/coreclr/vm/frames.h b/src/coreclr/vm/frames.h index faf0e660e03541..c18408461b1474 100644 --- a/src/coreclr/vm/frames.h +++ b/src/coreclr/vm/frames.h @@ -2496,13 +2496,15 @@ class DebuggerU2MCatchHandlerFrame : public Frame { public: #ifndef DACCESS_COMPILE - DebuggerU2MCatchHandlerFrame() : Frame(FrameIdentifier::DebuggerU2MCatchHandlerFrame) + DebuggerU2MCatchHandlerFrame(bool catchesAllExceptions) : Frame(FrameIdentifier::DebuggerU2MCatchHandlerFrame), + m_catchesAllExceptions(catchesAllExceptions) { WRAPPER_NO_CONTRACT; Frame::Push(); } - DebuggerU2MCatchHandlerFrame(Thread * pThread) : Frame(FrameIdentifier::DebuggerU2MCatchHandlerFrame) + DebuggerU2MCatchHandlerFrame(Thread * pThread, bool catchesAllExceptions) : Frame(FrameIdentifier::DebuggerU2MCatchHandlerFrame), + m_catchesAllExceptions(catchesAllExceptions) { WRAPPER_NO_CONTRACT; Frame::Push(pThread); @@ -2514,6 +2516,16 @@ class DebuggerU2MCatchHandlerFrame : public Frame LIMITED_METHOD_DAC_CONTRACT; return TT_U2M; } + + bool CatchesAllExceptions() + { + LIMITED_METHOD_DAC_CONTRACT; + return m_catchesAllExceptions; + } + +private: + // The catch handled marked by the DebuggerU2MCatchHandlerFrame catches all exceptions. + bool m_catchesAllExceptions; }; // Frame for the Reverse PInvoke (i.e. UnmanagedCallersOnlyAttribute). diff --git a/src/coreclr/vm/threads.cpp b/src/coreclr/vm/threads.cpp index 51462ddd278a02..cb40472d664620 100644 --- a/src/coreclr/vm/threads.cpp +++ b/src/coreclr/vm/threads.cpp @@ -7206,7 +7206,7 @@ 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; + DebuggerU2MCatchHandlerFrame catchFrame(false /* catchesAllExceptions */); TryParam param(pCallState); param.pFrame = &catchFrame; diff --git a/src/tests/baseservices/exceptions/unhandled/unhandled.cs b/src/tests/baseservices/exceptions/unhandled/unhandled.cs index d897b2671c26e2..a5c91702556442 100644 --- a/src/tests/baseservices/exceptions/unhandled/unhandled.cs +++ b/src/tests/baseservices/exceptions/unhandled/unhandled.cs @@ -5,6 +5,7 @@ using System.Reflection; using System.Runtime.CompilerServices; using System.Runtime.InteropServices; +using System.Threading; namespace TestUnhandledException { @@ -47,6 +48,12 @@ static void Main(string[] args) { InvokeCallbackOnNewThread(&ThrowException); } + else if (args[0] == "secondary") + { + Thread t = new Thread(() => throw new Exception("Test")); + t.Start(); + t.Join(); + } } } } diff --git a/src/tests/baseservices/exceptions/unhandled/unhandledTester.cs b/src/tests/baseservices/exceptions/unhandled/unhandledTester.cs index 6006ccb4ad7a73..0b779a387a778a 100644 --- a/src/tests/baseservices/exceptions/unhandled/unhandledTester.cs +++ b/src/tests/baseservices/exceptions/unhandled/unhandledTester.cs @@ -89,7 +89,7 @@ static void RunExternalProcess(string unhandledType, string assembly) } else { - if (unhandledType == "main") + if (unhandledType == "main" || unhandledType == "secondary") { if (lines[0] != "Unhandled exception. System.Exception: Test") { @@ -113,6 +113,13 @@ static void RunExternalProcess(string unhandledType, string assembly) throw new Exception("Missing exception source frame"); } } + else if (unhandledType == "secondary") + { + if (!lines[exceptionStackFrameLine].TrimStart().StartsWith("at TestUnhandledException.Program.")) + { + throw new Exception("Missing exception source frame"); + } + } Console.WriteLine("Test process exited with expected error code and produced expected output"); } @@ -121,6 +128,7 @@ static void RunExternalProcess(string unhandledType, string assembly) public static void TestEntryPoint() { RunExternalProcess("main", "unhandled.dll"); + RunExternalProcess("secondary", "unhandled.dll"); RunExternalProcess("foreign", "unhandled.dll"); File.Delete(Path.Combine(Path.GetDirectoryName(Assembly.GetExecutingAssembly().Location), "dependencytodelete.dll")); RunExternalProcess("missingdependency", "unhandledmissingdependency.dll");