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.

10 files changed, 160 insertions, 53 deletions

diff --git a/src/pal/src/CMakeLists.txt b/src/pal/src/CMakeLists.txt
index 0191a992d3..b1f11bd9e3 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 0000000000..d5d6ba8b81
--- /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 0000000000..4637a2e198
--- /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 0000000000..e61bb21282
--- /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 6a09e84fb1..fa8aa3848f 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 d853e9e947..bf6d83c1c6 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 7d9ff08bc0..22f8aef499 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 d9507733a7..f8198deaea 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 15d4f43ca1..928651dee5 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 cdabb4bad6..f945c427e4 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