Merge pull request #1217 from mikem8361/fixassert

Fix recursive asserts in coreclr.

5 files changed, 64 insertions, 46 deletions

diff --git a/src/inc/debugmacros.h b/src/inc/debugmacros.h
index f805c134f8..970b1a5f65 100644
--- a/src/inc/debugmacros.h
+++ b/src/inc/debugmacros.h
@@ -28,7 +28,7 @@ class SString;
 bool GetStackTraceAtContext(SString & s, struct _CONTEXT * pContext);
 
 void _cdecl DbgWriteEx(LPCTSTR szFmt, ...);
-int _DbgBreakCheck(LPCSTR szFile, int iLine, LPCSTR szExpr, BOOL fConstrained = FALSE);
+bool _DbgBreakCheck(LPCSTR szFile, int iLine, LPCSTR szExpr, BOOL fConstrained = FALSE);
 
 extern VOID DbgAssertDialog(const char *szFile, int iLine, const char *szExpr);
 
diff --git a/src/inc/winwrap.h b/src/inc/winwrap.h
index 3c0b5751c0..65a8e6c2d0 100644
--- a/src/inc/winwrap.h
+++ b/src/inc/winwrap.h
@@ -943,7 +943,9 @@ inline void DbgWPrintf(const LPCWSTR wszFormat, ...)
     va_end(args);
 
     if (IsDebuggerPresent())
+    {
         OutputDebugStringW(wszBuffer);
+    }
     else
     {
         fwprintf(stdout, W("%s"), wszBuffer);
diff --git a/src/pal/src/exception/signal.cpp b/src/pal/src/exception/signal.cpp
index 9d8faec24e..a2e276284c 100644
--- a/src/pal/src/exception/signal.cpp
+++ b/src/pal/src/exception/signal.cpp
@@ -415,8 +415,9 @@ static void sigtrap_handler(int code, siginfo_t *siginfo, void *context)
     }
     else
     {
-        // Restore the original or default handler and restart h/w exception
-        restore_signal(code, &g_previous_sigtrap);
+        // We abort instead of restore the original or default handler and returning
+        // because returning from a SIGTRAP handler continues execution past the trap.
+        abort();
     }
 }
 
diff --git a/src/utilcode/debug.cpp b/src/utilcode/debug.cpp
index 8139712a53..d5be6cbc18 100644
--- a/src/utilcode/debug.cpp
+++ b/src/utilcode/debug.cpp
@@ -339,7 +339,7 @@ static const char * szLowMemoryAssertMessage = "Assert failure (unable to format
 //*****************************************************************************
 // This function will handle ignore codes and tell the user what is happening.
 //*****************************************************************************
-int _DbgBreakCheck(
+bool _DbgBreakCheck(
     LPCSTR      szFile,
     int         iLine,
     LPCSTR      szExpr, 
@@ -358,14 +358,16 @@ int _DbgBreakCheck(
     }
 
     DBGIGNORE* pDBGIFNORE = GetDBGIGNORE();
-    _DBGIGNOREDATA *psData = NULL;
-    int        i;
+    _DBGIGNOREDATA *psData;
+    int i;
+
     // Check for ignore all.
-    for (i=0, psData = pDBGIFNORE->Ptr();  i<pDBGIFNORE->Count();  i++, psData++)
+    for (i = 0, psData = pDBGIFNORE->Ptr();  i < pDBGIFNORE->Count();  i++, psData++)
     {
-        if (psData->iLine == iLine && SString::_stricmp(psData->rcFile, szFile) == 0 &&
-            psData->bIgnore == true)
-            return (false);
+        if (psData->iLine == iLine && SString::_stricmp(psData->rcFile, szFile) == 0 && psData->bIgnore == true)
+        {
+            return false;
+        }
     }
 
     CONTRACT_VIOLATION(FaultNotFatal | GCViolation | TakesLockViolation);
@@ -384,26 +386,29 @@ int _DbgBreakCheck(
         EX_TRY
         {
             ClrGetModuleFileName(0, modulePath);
-            debugOutput.Printf(W("Assert failure(PID %d [0x%08x], Thread: %d [0x%x]): %hs\n")
-                W("    File: %hs, Line: %d Image:\n"),
+            debugOutput.Printf(
+                W("\nAssert failure(PID %d [0x%08x], Thread: %d [0x%04x]): %hs\n")
+                W("    File: %hs Line: %d\n")
+                W("    Image: "),
                 GetCurrentProcessId(), GetCurrentProcessId(),
                 GetCurrentThreadId(), GetCurrentThreadId(),
                 szExpr, szFile, iLine);
             debugOutput.Append(modulePath);
-            debugOutput.Append(W("\n"));
-            
+            debugOutput.Append(W("\n\n"));
+         
             // Change format for message box.  The extra spaces in the title
             // are there to get around format truncation.
-            dialogOutput.Printf(W("%hs\n\n%hs, Line: %d\n\nAbort - Kill program\nRetry - Debug\nIgnore - Keep running\n")
+            dialogOutput.Printf(
+                W("%hs\n\n%hs, Line: %d\n\nAbort - Kill program\nRetry - Debug\nIgnore - Keep running\n")
                 W("\n\nImage:\n"), szExpr, szFile, iLine);
             dialogOutput.Append(modulePath);
             dialogOutput.Append(W("\n"));
-            dialogTitle.Printf(W("Assert Failure (PID %d, Thread %d/%x)        "),
+            dialogTitle.Printf(W("Assert Failure (PID %d, Thread %d/0x%04x)"),
                 GetCurrentProcessId(), GetCurrentThreadId(), GetCurrentThreadId());
             
             dialogIgnoreMessage.Printf(W("Ignore the assert for the rest of this run?\nYes - Assert will never fire again.\nNo - Assert will continue to fire.\n\n%hs\nLine: %d\n"),
                 szFile, iLine);
-            
+
             formattedMessages = TRUE;
         }
         EX_CATCH
@@ -418,7 +423,8 @@ int _DbgBreakCheck(
         WszOutputDebugString(debugOutput);
         fwprintf(stderr, W("%s"), (const WCHAR*)debugOutput);    
     }
-    else {
+    else 
+    {
         // Note: we cannot convert to unicode or concatenate in this situation.
         OutputDebugStringA(szLowMemoryAssertMessage);        
         OutputDebugStringA("\n");
@@ -440,7 +446,7 @@ int _DbgBreakCheck(
 
     if (ContinueOnAssert())
     {
-        return false; // don't stop debugger. No gui.
+        return false;       // don't stop debugger. No gui.
     }
 
     if (NoGuiOnAssert())
@@ -450,7 +456,7 @@ int _DbgBreakCheck(
 
     if (DebugBreakOnAssert())
     {
-        return(true);       // like a retry
+        return true;       // like a retry
     }
 
     if (IsDisplayingAssertDlg())
@@ -462,12 +468,14 @@ int _DbgBreakCheck(
         // user. So we just continue.
         return false;
     }
+
     SetDisplayingAssertDlg(TRUE);
 
     // Tell user there was an error.
     _DbgBreakCount++;
     int ret;
-    if (formattedMessages) {
+    if (formattedMessages)
+    {
         ret = UtilMessageBoxCatastrophicNonLocalized(
             W("%s"), dialogTitle, MB_ABORTRETRYIGNORE | MB_ICONEXCLAMATION, TRUE, (const WCHAR*)dialogOutput);
     }
@@ -497,7 +505,7 @@ int _DbgBreakCheck(
         {
             LaunchJITDebugger();
         }
-        return (true);
+        return true;
 
     // If we want to ignore the assert, find out if this is forever.
     case IDIGNORE:
@@ -519,11 +527,13 @@ int _DbgBreakCheck(
                                    "Ignore Assert Forever?",
                                    MB_ICONQUESTION | MB_YESNO) != IDYES) 
             {
-            break;
+                break;
             }                                  
         }
         if ((psData = pDBGIFNORE->Append()) == 0)
-            return (false);
+        {
+            return false;
+        }
         psData->bIgnore = true;
         psData->iLine = iLine;
         strcpy(psData->rcFile, szFile);
@@ -534,10 +544,10 @@ int _DbgBreakCheck(
         return true;
     }
 
-    return (false);
+    return false;
 }
 
-int _DbgBreakCheckNoThrow(
+bool _DbgBreakCheckNoThrow(
     LPCSTR      szFile,
     int         iLine,
     LPCSTR      szExpr, 
@@ -555,8 +565,8 @@ int _DbgBreakCheckNoThrow(
         DebugBreak();
     }
 
-    int failed = false;
-    int result = false;
+    bool failed = false;
+    bool result = false;
     EX_TRY
     {
         result = _DbgBreakCheck(szFile, iLine, szExpr, fConstrained);
@@ -567,7 +577,7 @@ int _DbgBreakCheckNoThrow(
     }
     EX_END_CATCH(SwallowAllExceptions);
     
-    if (failed == TRUE)
+    if (failed)
     {
         return true;
     }
@@ -683,7 +693,9 @@ VOID DebBreakHr(HRESULT hr)
   #endif
 }
 
+#ifndef FEATURE_PAL
 CHAR g_szExprWithStack2[10480];
+#endif
 void *dbgForceToMemory;     // dummy pointer that pessimises enregistration
 
 #ifdef MDA_SUPPORTED
@@ -771,14 +783,21 @@ VOID DbgAssertDialog(const char *szFile, int iLine, const char *szExpr)
 #endif
     
     LONG lAlreadyOwned = InterlockedExchange((LPLONG)&g_BufferLock, 1);
-    if (fConstrained || dwAssertStacktrace == 0 || lAlreadyOwned == 1) {
-        if (1 == _DbgBreakCheckNoThrow(szFile, iLine, szExpr, fConstrained))
+    if (fConstrained || dwAssertStacktrace == 0 || lAlreadyOwned == 1)
+    {
+        if (_DbgBreakCheckNoThrow(szFile, iLine, szExpr, fConstrained))
+        {
             _DbgBreak();
-    } else {
+        }
+    } 
+    else
+    {
+        char *szExprToDisplay = (char*)szExpr;
+#ifdef FEATURE_PAL
+        BOOL fGotStackTrace = TRUE;
+#else
         BOOL fGotStackTrace = FALSE;
-        char *szExprToDisplay = &g_szExprWithStack2[0];
-
-#if !defined(DACCESS_COMPILE) && !defined(FEATURE_PAL)
+#ifndef DACCESS_COMPILE
         EX_TRY
         {
             FAULT_NOT_FATAL();
@@ -786,20 +805,20 @@ VOID DbgAssertDialog(const char *szFile, int iLine, const char *szExpr)
             _ASSERTE(szExprToDisplay == g_szExprWithStack2);
             strcat_s(szExprToDisplay, _countof(g_szExprWithStack2), "\n\n");
             GetStringFromStackLevels(1, 10, szExprToDisplay + strlen(szExprToDisplay));
+            szExprToDisplay = &g_szExprWithStack2[0];
             fGotStackTrace = TRUE;               
         }
         EX_CATCH
         {            
         }
         EX_END_CATCH(SwallowAllExceptions);
-#endif 
-
-        // If we failed to get the stack trace, simply display the assert without one.
-        if (!fGotStackTrace)
-            szExprToDisplay = (char*)szExpr;
+#endif  // DACCESS_COMPILE
+#endif  // FEATURE_PAL
         
-        if (1 == _DbgBreakCheckNoThrow(szFile, iLine, szExprToDisplay, !fGotStackTrace))
-            _DbgBreak();        
+        if (_DbgBreakCheckNoThrow(szFile, iLine, szExprToDisplay, !fGotStackTrace))
+        {
+            _DbgBreak();
+        }
         
         g_BufferLock = 0;
     }
diff --git a/src/vm/exceptionhandling.cpp b/src/vm/exceptionhandling.cpp
index 37f1369c5d..c22b40c0d2 100644
--- a/src/vm/exceptionhandling.cpp
+++ b/src/vm/exceptionhandling.cpp
@@ -5061,10 +5061,6 @@ VOID PALAPI HandleHardwareException(PAL_SEHException* ex)
             {
                 RtlRestoreContext(&ex->ContextRecord, &ex->ExceptionRecord);
             }
-            else
-            {
-                _ASSERTE(!"Looks like a random breakpoint/trap that was not prepared by the EE debugger");
-            }
         }
     }
 }