From 9ef5ee736212e95d456cc13f3e1f7ff96db51bef Mon Sep 17 00:00:00 2001 From: Mike McLaughlin Date: Tue, 10 Nov 2015 14:28:10 -0800 Subject: [PATCH] Fix Thread.Start while debugging bug on OSX. The OSX exception logic is running on a separate thread from the one that the exception happened. The CatchHardwareExceptionHolder::IsEnabled used to check for h/w exception holders assumed it was running on the thread the exception/holders happened not the exception notification thread. Moved the h/w exception holder count to the CPalThread object instead of a TLS thread variable so the OSX exception code can check it given a CPalThread instance. The main problem was that the stubmgr when creating a thread (for the start notification) put a breakpoint in ThePreStubPatch which is in the coreclr text section and because the h/w exception holder check was broken, it thought the breakpoint wasn't the debugger's and aborted the coreclr process. The other part of this fix is to put a h/w exception holder around the called to ThePreStubPatch in prestub.cpp. The stubmgr's delegate invoke subclass used the wrong registers for Linux/OSX to read the "this" and parameter registers. Added the proper ifdefs and registers (ecx -> rdi, rdx -> rsi) for the Unix platforms. On both Linux and OSX, the h/w exception holder check in the exception routing logic needed to check if the int3/trap is in DBG_DebugBreak so asserts always abort/core dump. Move DBG_DebugBreak to an assembly worker so the start/end address can be used for this check. --- src/inc/palclr.h | 2 + src/pal/src/CMakeLists.txt | 3 ++ src/pal/src/arch/arm/debugbreak.S | 15 +++++++ src/pal/src/arch/arm64/debugbreak.S | 12 ++++++ src/pal/src/arch/i386/debugbreak.S | 13 ++++++ src/pal/src/debug/debug.cpp | 15 +++++++ src/pal/src/exception/machexception.cpp | 74 ++++++++++++++++++++++++++------- src/pal/src/exception/seh.cpp | 37 ++++++++--------- src/pal/src/include/pal/debug.h | 13 +++++- src/pal/src/include/pal/thread.hpp | 13 ++++++ src/pal/src/thread/context.cpp | 18 -------- src/vm/prestub.cpp | 8 +++- src/vm/stubmgr.cpp | 6 +-- 13 files changed, 171 insertions(+), 58 deletions(-) create mode 100644 src/pal/src/arch/arm/debugbreak.S create mode 100644 src/pal/src/arch/arm64/debugbreak.S create mode 100644 src/pal/src/arch/i386/debugbreak.S diff --git a/src/inc/palclr.h b/src/inc/palclr.h index e005c29..9b06a13 100644 --- a/src/inc/palclr.h +++ b/src/inc/palclr.h @@ -299,6 +299,8 @@ #include "staticcontract.h" +#define HardwareExceptionHolder + // Note: PAL_SEH_RESTORE_GUARD_PAGE is only ever defined in clrex.h, so we only restore guard pages automatically // when these macros are used from within the VM. #define PAL_SEH_RESTORE_GUARD_PAGE diff --git a/src/pal/src/CMakeLists.txt b/src/pal/src/CMakeLists.txt index 0191a99..b1f11bd 100644 --- a/src/pal/src/CMakeLists.txt +++ b/src/pal/src/CMakeLists.txt @@ -73,16 +73,19 @@ add_compile_options(-fPIC) if(PAL_CMAKE_PLATFORM_ARCH_AMD64) set(ARCH_SOURCES arch/i386/context2.S + arch/i386/debugbreak.S arch/i386/processor.cpp ) elseif(PAL_CMAKE_PLATFORM_ARCH_ARM) set(ARCH_SOURCES arch/arm/context2.S + arch/arm/debugbreak.S arch/arm/processor.cpp ) elseif(PAL_CMAKE_PLATFORM_ARCH_ARM64) set(ARCH_SOURCES arch/arm64/context2.S + arch/arm64/debugbreak.S arch/arm64/processor.cpp ) endif() diff --git a/src/pal/src/arch/arm/debugbreak.S b/src/pal/src/arch/arm/debugbreak.S new file mode 100644 index 0000000..d5d6ba8 --- /dev/null +++ b/src/pal/src/arch/arm/debugbreak.S @@ -0,0 +1,15 @@ +// +// Copyright (c) Microsoft. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. +// + +#include "unixasmmacros.inc" + +.syntax unified +.thumb + +LEAF_ENTRY DBG_DebugBreak, _TEXT + EMIT_BREAKPOINT + bx lr +LEAF_END DBG_DebugBreak, _TEXT + diff --git a/src/pal/src/arch/arm64/debugbreak.S b/src/pal/src/arch/arm64/debugbreak.S new file mode 100644 index 0000000..4637a2e --- /dev/null +++ b/src/pal/src/arch/arm64/debugbreak.S @@ -0,0 +1,12 @@ +// +// Copyright (c) Microsoft. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. +// + +#include "unixasmmacros.inc" + +LEAF_ENTRY DBG_DebugBreak, _TEXT + EMIT_BREAKPOINT + ret +LEAF_END DBG_DebugBreak, _TEXT + diff --git a/src/pal/src/arch/i386/debugbreak.S b/src/pal/src/arch/i386/debugbreak.S new file mode 100644 index 0000000..e61bb21 --- /dev/null +++ b/src/pal/src/arch/i386/debugbreak.S @@ -0,0 +1,13 @@ +// +// Copyright (c) Microsoft. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. +// + +.intel_syntax noprefix +#include "unixasmmacros.inc" + +LEAF_ENTRY DBG_DebugBreak, _TEXT + int3 + ret +LEAF_END DBG_DebugBreak, _TEXT + diff --git a/src/pal/src/debug/debug.cpp b/src/pal/src/debug/debug.cpp index 6a09e84..fa8aa38 100644 --- a/src/pal/src/debug/debug.cpp +++ b/src/pal/src/debug/debug.cpp @@ -69,6 +69,8 @@ using namespace CorUnix; SET_DEFAULT_DEBUG_CHANNEL(DEBUG); +extern "C" void DBG_DebugBreak_End(); + #if HAVE_PROCFS_CTL #define CTL_ATTACH "attach" #define CTL_DETACH "detach" @@ -412,6 +414,19 @@ DebugBreak( /*++ Function: + IsInDebugBreak(addr) + + Returns true if the address is in DBG_DebugBreak. + +--*/ +BOOL +IsInDebugBreak(void *addr) +{ + return (addr >= (void *)DBG_DebugBreak) && (addr <= (void *)DBG_DebugBreak_End); +} + +/*++ +Function: GetThreadContext See MSDN doc. diff --git a/src/pal/src/exception/machexception.cpp b/src/pal/src/exception/machexception.cpp index d853e9e..bf6d83c 100644 --- a/src/pal/src/exception/machexception.cpp +++ b/src/pal/src/exception/machexception.cpp @@ -688,6 +688,43 @@ static DWORD exception_from_trap_code( return EXCEPTION_ILLEGAL_INSTRUCTION; } +#ifdef _DEBUG +const char * +GetExceptionString( + exception_type_t exception +) +{ + switch(exception) + { + case EXC_BAD_ACCESS: + return "EXC_BAD_ACCESS"; + + case EXC_BAD_INSTRUCTION: + return "EXC_BAD_INSTRUCTION"; + + case EXC_ARITHMETIC: + return "EXC_ARITHMETIC"; + + case EXC_SOFTWARE: + return "EXC_SOFTWARE"; + + case EXC_BREAKPOINT: + return "EXC_BREAKPOINT"; + + case EXC_SYSCALL: + return "EXC_SYSCALL"; + + case EXC_MACH_SYSCALL: + return "EXC_MACH_SYSCALL"; + + default: + ASSERT("Got unknown trap code %d\n", exception); + break; + } + return "INVALID CODE"; +} +#endif // DEBUG + /*++ Function : catch_exception_raise @@ -1084,6 +1121,8 @@ bool IsWithinCoreCLR(void *pAddr) // we calculated in the previous step is relative to the base address of the image, i.e. the Mach-O // header). s_pUpperBound = (unsigned char*)pHeader + addrMaxAddress + cbMaxSegment; + + NONPAL_TRACE("coreclr s_pLowerBound %p s_pUpperBound %p\n", s_pLowerBound, s_pUpperBound); } // Perform the range check. @@ -1168,7 +1207,7 @@ static bool malloc_zone_owns_addr(malloc_zone_t *zone, void * const addr) // generated by this instance of the CoreCLR. If true is returned the CoreCLR "owns" the faulting code and we // should handle the exception. Otherwise the exception should be forwarded to another handler (if there is // one) or the process terminated. -bool IsHandledException(MachMessage *pNotification) +bool IsHandledException(MachMessage *pNotification, CorUnix::CPalThread *pThread) { // Retrieve the state of faulting thread from the message (or directly, if the message doesn't contain // this information). @@ -1197,7 +1236,11 @@ bool IsHandledException(MachMessage *pNotification) if (IsWithinCoreCLR(ip)) { NONPAL_TRACE(" IP (%p) is in CoreCLR.\n", ip); - return CatchHardwareExceptionHolder::IsEnabled(); + if (IsInDebugBreak(ip)) + { + return false; + } + return pThread->IsHardwareExceptionsEnabled(); } // Check inside our executable heap. @@ -1342,12 +1385,23 @@ void *SEHExceptionThread(void *args) // decide not to handle the exception ourselves. bool fTopException = sMessage.GetLocalPort() == s_TopExceptionPort; - NONPAL_TRACE(" Notification is for exception %u on thread %08X (sent to the %s exception handler)\n", - sMessage.GetException(), hThread, fTopException ? "top" : "bottom"); + // Note that the call to PROCThreadFromMachPort() requires taking the PAL process critical + // section, which is dangerous. The assumption we make is that code holding this critical + // section (of which there is little) never generates an exception while the lock is still + // held. + CorUnix::CPalThread *pTargetThread = PROCThreadFromMachPort(hThread); + if (pTargetThread == NULL) + NONPAL_RETAIL_ASSERT("Failed to translate mach thread to a CPalThread."); + + NONPAL_TRACE(" Notification is for exception %u (%s) on thread id 0x%04lX (sent to the %s exception handler)\n", + sMessage.GetException(), + GetExceptionString(sMessage.GetException()), + pTargetThread->GetThreadId(), + fTopException ? "top" : "bottom"); // Determine if we should handle this exception ourselves or forward it to the previous handler // registered for this exception type (if there is one). - if (IsHandledException(&sMessage)) + if (IsHandledException(&sMessage, pTargetThread)) { // This is one of ours, pass the relevant exception data to our handler. MACH_EH_TYPE(exception_data_type_t) rgCodes[2]; @@ -1376,15 +1430,7 @@ void *SEHExceptionThread(void *args) // any handler previously registered for this kind of exception. // Locate the record of previously installed handlers that the target thread keeps. - // Note that the call to PROCThreadFromMachPort() requires taking the PAL process critical - // section, which is dangerous. The assumption we make is that code holding this critical - // section (of which there is little) never generates an exception while the lock is still - // held. - CorUnix::CPalThread *pTargetThread = PROCThreadFromMachPort(hThread); - if (pTargetThread == NULL) - NONPAL_RETAIL_ASSERT("Failed to translate mach thread to a CPalThread."); - CorUnix::CThreadMachExceptionHandlers *pHandlers = - pTargetThread->GetSavedMachHandlers(); + CorUnix::CThreadMachExceptionHandlers *pHandlers = pTargetThread->GetSavedMachHandlers(); // Check whether there's even a handler for the particular exception we've been handed. CorUnix::MachExceptionHandler sHandler; diff --git a/src/pal/src/exception/seh.cpp b/src/pal/src/exception/seh.cpp index 7d9ff08..22f8aef 100644 --- a/src/pal/src/exception/seh.cpp +++ b/src/pal/src/exception/seh.cpp @@ -133,9 +133,7 @@ VOID PALAPI PAL_SetHardwareExceptionHandler( IN PHARDWARE_EXCEPTION_HANDLER exceptionHandler) - { - //TRACE("Hardware exception installed: %p\n", exceptionHandler); g_hardwareExceptionHandler = exceptionHandler; } @@ -154,16 +152,19 @@ Return value: VOID SEHProcessException(PEXCEPTION_POINTERS pointers) { - PAL_SEHException exception(pointers->ExceptionRecord, pointers->ContextRecord); - - if (g_hardwareExceptionHandler != NULL) + if (!IsInDebugBreak(pointers->ExceptionRecord->ExceptionAddress)) { - g_hardwareExceptionHandler(&exception); - } + PAL_SEHException exception(pointers->ExceptionRecord, pointers->ContextRecord); - if (CatchHardwareExceptionHolder::IsEnabled()) - { - throw exception; + if (g_hardwareExceptionHandler != NULL) + { + g_hardwareExceptionHandler(&exception); + } + + if (CatchHardwareExceptionHolder::IsEnabled()) + { + throw exception; + } } TRACE("Unhandled hardware exception %08x at %p\n", @@ -230,26 +231,22 @@ PAL_ERROR SEHDisable(CPalThread *pthrCurrent) --*/ -#ifdef __llvm__ -__thread -#else // __llvm__ -__declspec(thread) -#endif // !__llvm__ -int t_holderCount = 0; - CatchHardwareExceptionHolder::CatchHardwareExceptionHolder() { - ++t_holderCount; + CPalThread *pThread = InternalGetCurrentThread(); + ++pThread->m_hardwareExceptionHolderCount; } CatchHardwareExceptionHolder::~CatchHardwareExceptionHolder() { - --t_holderCount; + CPalThread *pThread = InternalGetCurrentThread(); + --pThread->m_hardwareExceptionHolderCount; } bool CatchHardwareExceptionHolder::IsEnabled() { - return t_holderCount > 0; + CPalThread *pThread = InternalGetCurrentThread(); + return pThread->IsHardwareExceptionsEnabled(); } /*++ diff --git a/src/pal/src/include/pal/debug.h b/src/pal/src/include/pal/debug.h index d950773..f8198de 100644 --- a/src/pal/src/include/pal/debug.h +++ b/src/pal/src/include/pal/debug.h @@ -35,7 +35,18 @@ Function : (no parameters, no return value) --*/ -VOID DBG_DebugBreak(); +extern "C" VOID +DBG_DebugBreak(); + +/*++ +Function: + IsInDebugBreak(addr) + + Returns true if the address is in DBG_DebugBreak. + +--*/ +BOOL +IsInDebugBreak(void *addr); /*++ Function : diff --git a/src/pal/src/include/pal/thread.hpp b/src/pal/src/include/pal/thread.hpp index 15d4f43..928651d 100644 --- a/src/pal/src/include/pal/thread.hpp +++ b/src/pal/src/include/pal/thread.hpp @@ -275,6 +275,8 @@ namespace CorUnix CPalThread *pNewThread, HANDLE *phThread ); + + friend CatchHardwareExceptionHolder; private: @@ -318,6 +320,10 @@ namespace CorUnix mach_port_t m_machPortSelf; #endif + // > 0 when there is an exception holder which causes h/w + // exceptions to be sent down the C++ exception chain. + int m_hardwareExceptionHolderCount; + // // Start info // @@ -406,6 +412,7 @@ namespace CorUnix #if HAVE_MACH_THREADS m_machPortSelf(0), #endif + m_hardwareExceptionHolderCount(0), m_lpStartAddress(NULL), m_lpStartParameter(NULL), m_bCreateSuspended(FALSE), @@ -572,6 +579,12 @@ namespace CorUnix }; #endif + bool + IsHardwareExceptionsEnabled() + { + return m_hardwareExceptionHolderCount > 0; + } + LPTHREAD_START_ROUTINE GetStartAddress( void diff --git a/src/pal/src/thread/context.cpp b/src/pal/src/thread/context.cpp index cdabb4b..f945c42 100644 --- a/src/pal/src/thread/context.cpp +++ b/src/pal/src/thread/context.cpp @@ -1275,24 +1275,6 @@ EXIT: /*++ Function: - DBG_DebugBreak: same as DebugBreak - -See MSDN doc. ---*/ -VOID -DBG_DebugBreak() -{ -#if defined(_AMD64_) || defined(_X86_) - __asm__ __volatile__("int $3"); -#elif defined(_ARM_) - // This assumes thumb - __asm__ __volatile__(".inst.w 0xde01"); -#endif -} - - -/*++ -Function: DBG_FlushInstructionCache: processor-specific portion of FlushInstructionCache diff --git a/src/vm/prestub.cpp b/src/vm/prestub.cpp index 06d1967..948d925 100644 --- a/src/vm/prestub.cpp +++ b/src/vm/prestub.cpp @@ -1039,8 +1039,12 @@ extern "C" PCODE STDCALL PreStubWorker(TransitionBlock * pTransitionBlock, Metho UNINSTALL_UNWIND_AND_CONTINUE_HANDLER; UNINSTALL_MANAGED_EXCEPTION_DISPATCHER; - // Give debugger opportunity to stop here - ThePreStubPatch(); + { + HardwareExceptionHolder + + // Give debugger opportunity to stop here + ThePreStubPatch(); + } pPFrame->Pop(CURRENT_THREAD); diff --git a/src/vm/stubmgr.cpp b/src/vm/stubmgr.cpp index f1b07b2..f217216 100644 --- a/src/vm/stubmgr.cpp +++ b/src/vm/stubmgr.cpp @@ -2269,7 +2269,7 @@ BOOL DelegateInvokeStubManager::TraceManager(Thread *thread, TraceDestination *t { LOG((LF_CORDB, LL_INFO10000, "DISM::TraceManager: isSingle\n")); - orDelegate = (DELEGATEREF)ObjectToOBJECTREF((Object*)(size_t)pContext->Rcx); // The "this" pointer is rcx + orDelegate = (DELEGATEREF)ObjectToOBJECTREF(StubManagerHelpers::GetThisPtr(pContext)); // _methodPtr is where we are going to next. However, in ngen cases, we may have a shuffle thunk // burned into the ngen image, in which case the shuffle thunk is not added to the range list of @@ -2293,11 +2293,11 @@ BOOL DelegateInvokeStubManager::TraceManager(Thread *thread, TraceDestination *t if (pStub->GetPatchOffset() != 0) { // This stub has a hidden return buffer argument. - orDelegate = (DELEGATEREF)ObjectToOBJECTREF((Object*)(size_t)(pContext->Rdx)); + orDelegate = (DELEGATEREF)ObjectToOBJECTREF(StubManagerHelpers::GetSecondArg(pContext)); } else { - orDelegate = (DELEGATEREF)ObjectToOBJECTREF((Object*)(size_t)(pContext->Rcx)); + orDelegate = (DELEGATEREF)ObjectToOBJECTREF(StubManagerHelpers::GetThisPtr(pContext)); } } -- 2.7.4