From 87ceb89093fb665a191eebdefd0fcc5af5d3926b Mon Sep 17 00:00:00 2001 From: Jan Kotas Date: Tue, 11 Feb 2025 18:35:25 -0800 Subject: [PATCH] Revert "Fix exception handling in the prestub worker (#111937)" This reverts commit 12fa8646917828c72903eb85827407095ad97f0e. --- src/coreclr/vm/excep.cpp | 17 ++- src/coreclr/vm/exceptionhandling.cpp | 12 +- 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, 62 insertions(+), 270 deletions(-) delete mode 100644 src/tests/Regressions/coreclr/GitHub_76531/dependencytodelete.cs delete mode 100644 src/tests/Regressions/coreclr/GitHub_76531/dependencytodelete.csproj delete mode 100644 src/tests/Regressions/coreclr/GitHub_76531/tailcallinvoker.il delete mode 100644 src/tests/Regressions/coreclr/GitHub_76531/tailcallinvoker.ilproj delete mode 100644 src/tests/Regressions/coreclr/GitHub_76531/test76531.cs delete 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 5734d184610eee..985310c640a942 100644 --- a/src/coreclr/vm/excep.cpp +++ b/src/coreclr/vm/excep.cpp @@ -3053,17 +3053,14 @@ 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 != NULL) + if (pCf->IsIPadjusted()) { - 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; - } + 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 1476bbd19ec9d1..54963e0e2995d2 100644 --- a/src/coreclr/vm/exceptionhandling.cpp +++ b/src/coreclr/vm/exceptionhandling.cpp @@ -932,12 +932,7 @@ ProcessCLRExceptionNew(IN PEXCEPTION_RECORD pExceptionRecord, Thread* pThread = GetThread(); - // 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 (pThread->HasThreadStateNC(Thread::TSNC_ProcessedUnhandledException)) { if ((pExceptionRecord->ExceptionFlags & EXCEPTION_UNWINDING)) { @@ -8519,7 +8514,7 @@ static StackWalkAction MoveToNextNonSkippedFrame(StackFrameIterator* pStackFrame return retVal; } -bool IsCallDescrWorkerInternalReturnAddress(PCODE pCode); +extern "C" size_t CallDescrWorkerInternalReturnAddressOffset; extern "C" bool QCALLTYPE SfiNext(StackFrameIterator* pThis, uint* uExCollideClauseIdx, bool* fUnwoundReversePInvoke, bool* pfIsExceptionIntercepted) { @@ -8574,7 +8569,8 @@ extern "C" bool QCALLTYPE SfiNext(StackFrameIterator* pThis, uint* uExCollideCla } else { - if (IsCallDescrWorkerInternalReturnAddress(GetIP(pThis->m_crawl.GetRegisterSet()->pCallerContext))) + size_t CallDescrWorkerInternalReturnAddress = (size_t)CallDescrWorkerInternal + CallDescrWorkerInternalReturnAddressOffset; + if (GetIP(pThis->m_crawl.GetRegisterSet()->pCallerContext) == CallDescrWorkerInternalReturnAddress) { invalidRevPInvoke = true; } diff --git a/src/coreclr/vm/exceptmacros.h b/src/coreclr/vm/exceptmacros.h index 2c11c059853ba9..fb21a8be5119b1 100644 --- a/src/coreclr/vm/exceptmacros.h +++ b/src/coreclr/vm/exceptmacros.h @@ -277,22 +277,15 @@ VOID DECLSPEC_NORETURN UnwindAndContinueRethrowHelperAfterCatch(Frame* pEntryFra #ifdef TARGET_UNIX VOID DECLSPEC_NORETURN DispatchManagedException(PAL_SEHException& ex, bool isHardwareException); -#define INSTALL_MANAGED_EXCEPTION_DISPATCHER_EX \ +#define INSTALL_MANAGED_EXCEPTION_DISPATCHER \ PAL_SEHException exCopy; \ bool hasCaughtException = false; \ try { -#define INSTALL_MANAGED_EXCEPTION_DISPATCHER \ - INSTALL_MANAGED_EXCEPTION_DISPATCHER_EX - -#define UNINSTALL_MANAGED_EXCEPTION_DISPATCHER_EX(nativeRethrow) \ +#define UNINSTALL_MANAGED_EXCEPTION_DISPATCHER \ } \ catch (PAL_SEHException& ex) \ { \ - if (nativeRethrow) \ - { \ - throw; \ - } \ exCopy = std::move(ex); \ hasCaughtException = true; \ } \ @@ -301,9 +294,6 @@ 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 { @@ -325,9 +315,7 @@ 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 d9a4697abdf6f9..86432765673bc0 100644 --- a/src/coreclr/vm/prestub.cpp +++ b/src/coreclr/vm/prestub.cpp @@ -2536,20 +2536,6 @@ 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. @@ -2645,76 +2631,60 @@ extern "C" PCODE STDCALL PreStubWorker(TransitionBlock* pTransitionBlock, Method pPFrame->Push(CURRENT_THREAD); - EX_TRY - { - bool propagateExceptionToNativeCode = IsCallDescrWorkerInternalReturnAddress(pTransitionBlock->m_ReturnAddress); + INSTALL_MANAGED_EXCEPTION_DISPATCHER; + INSTALL_UNWIND_AND_CONTINUE_HANDLER; - INSTALL_MANAGED_EXCEPTION_DISPATCHER_EX; - INSTALL_UNWIND_AND_CONTINUE_HANDLER_EX; + // Make sure the method table is restored, and method instantiation if present + pMD->CheckRestore(); + CONSISTENCY_CHECK(GetAppDomain()->CheckCanExecuteManagedCode(pMD)); - // Make sure the method table is restored, and method instantiation if present - pMD->CheckRestore(); - CONSISTENCY_CHECK(GetAppDomain()->CheckCanExecuteManagedCode(pMD)); + MethodTable* pDispatchingMT = NULL; + if (pMD->IsVtableMethod()) + { + OBJECTREF curobj = pPFrame->GetThis(); - MethodTable* pDispatchingMT = NULL; - if (pMD->IsVtableMethod()) + if (curobj != NULL) // Check for virtual function called non-virtually on a NULL object { - OBJECTREF curobj = pPFrame->GetThis(); + pDispatchingMT = curobj->GetMethodTable(); - if (curobj != NULL) // Check for virtual function called non-virtually on a NULL object + if (pDispatchingMT->IsIDynamicInterfaceCastable()) { - pDispatchingMT = curobj->GetMethodTable(); + MethodTable* pMDMT = pMD->GetMethodTable(); + TypeHandle objectType(pDispatchingMT); + TypeHandle methodType(pMDMT); - if (pDispatchingMT->IsIDynamicInterfaceCastable()) + GCStress::MaybeTrigger(); + INDEBUG(curobj = NULL); // curobj is unprotected and CanCastTo() can trigger GC + if (!objectType.CanCastTo(methodType)) { - 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. + // 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); - { - pbRetVal = pMD->DoPrestub(pDispatchingMT, CallerGCMode::Coop); } - - UNINSTALL_UNWIND_AND_CONTINUE_HANDLER_EX(propagateExceptionToNativeCode); - UNINSTALL_MANAGED_EXCEPTION_DISPATCHER_EX(propagateExceptionToNativeCode); } - EX_CATCH + + GCX_PREEMP_THREAD_EXISTS(CURRENT_THREAD); { - if (g_isNewExceptionHandlingEnabled) - { - OBJECTHANDLE ohThrowable = CURRENT_THREAD->LastThrownObjectHandle(); - _ASSERTE(ohThrowable); - StackTraceInfo::AppendElement(ohThrowable, 0, (UINT_PTR)pTransitionBlock, pMD, NULL); - } - EX_RETHROW; + pbRetVal = pMD->DoPrestub(pDispatchingMT, CallerGCMode::Coop); } - EX_END_CATCH(SwallowAllExceptions) + + UNINSTALL_UNWIND_AND_CONTINUE_HANDLER; + UNINSTALL_MANAGED_EXCEPTION_DISPATCHER; { HardwareExceptionHolder; @@ -3184,10 +3154,8 @@ EXTERN_C PCODE STDCALL ExternalMethodFixupWorker(TransitionBlock * pTransitionBl pEMFrame->Push(CURRENT_THREAD); // Push the new ExternalMethodFrame onto the frame stack - bool propagateExceptionToNativeCode = IsCallDescrWorkerInternalReturnAddress(pTransitionBlock->m_ReturnAddress); - - INSTALL_MANAGED_EXCEPTION_DISPATCHER_EX; - INSTALL_UNWIND_AND_CONTINUE_HANDLER_EX; + INSTALL_MANAGED_EXCEPTION_DISPATCHER; + INSTALL_UNWIND_AND_CONTINUE_HANDLER; bool fVirtual = false; MethodDesc * pMD = NULL; @@ -3429,8 +3397,8 @@ EXTERN_C PCODE STDCALL ExternalMethodFixupWorker(TransitionBlock * pTransitionBl } // Ready to return - UNINSTALL_UNWIND_AND_CONTINUE_HANDLER_EX(propagateExceptionToNativeCode); - UNINSTALL_MANAGED_EXCEPTION_DISPATCHER_EX(propagateExceptionToNativeCode); + UNINSTALL_UNWIND_AND_CONTINUE_HANDLER; + UNINSTALL_MANAGED_EXCEPTION_DISPATCHER; 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 a2bfb6f2c49604..c22fbaedd7e53d 100644 --- a/src/coreclr/vm/virtualcallstub.cpp +++ b/src/coreclr/vm/virtualcallstub.cpp @@ -1262,8 +1262,6 @@ 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 @@ -1311,16 +1309,14 @@ 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_EX; - INSTALL_UNWIND_AND_CONTINUE_HANDLER_EX; + INSTALL_MANAGED_EXCEPTION_DISPATCHER; + INSTALL_UNWIND_AND_CONTINUE_HANDLER; COMPlusThrow(kNullReferenceException); - UNINSTALL_UNWIND_AND_CONTINUE_HANDLER_EX(propagateExceptionToNativeCode); - UNINSTALL_MANAGED_EXCEPTION_DISPATCHER_EX(propagateExceptionToNativeCode); + UNINSTALL_UNWIND_AND_CONTINUE_HANDLER; + UNINSTALL_MANAGED_EXCEPTION_DISPATCHER; _ASSERTE(!"Throw returned"); } @@ -1355,9 +1351,8 @@ PCODE VSD_ResolveWorker(TransitionBlock * pTransitionBlock, pSDFrame->SetRepresentativeSlot(pRepresentativeMT, representativeToken.GetSlotNumber()); pSDFrame->Push(CURRENT_THREAD); - - INSTALL_MANAGED_EXCEPTION_DISPATCHER_EX; - INSTALL_UNWIND_AND_CONTINUE_HANDLER_EX; + INSTALL_MANAGED_EXCEPTION_DISPATCHER; + INSTALL_UNWIND_AND_CONTINUE_HANDLER; // For Virtual Delegates the m_siteAddr is a field of a managed object // Thus we have to report it as an interior pointer, @@ -1393,8 +1388,8 @@ PCODE VSD_ResolveWorker(TransitionBlock * pTransitionBlock, GCPROTECT_END(); - UNINSTALL_UNWIND_AND_CONTINUE_HANDLER_EX(propagateExceptionToNativeCode); - UNINSTALL_MANAGED_EXCEPTION_DISPATCHER_EX(propagateExceptionToNativeCode); + UNINSTALL_UNWIND_AND_CONTINUE_HANDLER; + UNINSTALL_MANAGED_EXCEPTION_DISPATCHER; 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 deleted file mode 100644 index b00742958432fe..00000000000000 --- a/src/tests/Regressions/coreclr/GitHub_76531/dependencytodelete.cs +++ /dev/null @@ -1,14 +0,0 @@ -// 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 deleted file mode 100644 index fa1f2d01f80e57..00000000000000 --- a/src/tests/Regressions/coreclr/GitHub_76531/dependencytodelete.csproj +++ /dev/null @@ -1,9 +0,0 @@ - - - BuildOnly - Library - - - - - diff --git a/src/tests/Regressions/coreclr/GitHub_76531/tailcallinvoker.il b/src/tests/Regressions/coreclr/GitHub_76531/tailcallinvoker.il deleted file mode 100644 index 8cfd723e4be6e2..00000000000000 --- a/src/tests/Regressions/coreclr/GitHub_76531/tailcallinvoker.il +++ /dev/null @@ -1,17 +0,0 @@ -// 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 deleted file mode 100644 index ae3ae9ac54895f..00000000000000 --- a/src/tests/Regressions/coreclr/GitHub_76531/tailcallinvoker.ilproj +++ /dev/null @@ -1,10 +0,0 @@ - - - BuildOnly - Library - - - - - - diff --git a/src/tests/Regressions/coreclr/GitHub_76531/test76531.cs b/src/tests/Regressions/coreclr/GitHub_76531/test76531.cs deleted file mode 100644 index 4cd1573c309d66..00000000000000 --- a/src/tests/Regressions/coreclr/GitHub_76531/test76531.cs +++ /dev/null @@ -1,82 +0,0 @@ -// 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 deleted file mode 100644 index ae1513d8e93546..00000000000000 --- a/src/tests/Regressions/coreclr/GitHub_76531/test76531.csproj +++ /dev/null @@ -1,17 +0,0 @@ - - - - true - - true - true - - - - - - - - - - diff --git a/src/tests/issues.targets b/src/tests/issues.targets index 8982fc0fbe7a04..73f392cc50a517 100644 --- a/src/tests/issues.targets +++ b/src/tests/issues.targets @@ -3141,9 +3141,6 @@ System.Diagnostics.Process is not supported - - Assembly.GetExecutingAssembly().Location returns NULL on WASM - System.Threading.Thread.UnsafeStart not supported