From c6d8dee93f778825010283f52f8b86e338d73342 Mon Sep 17 00:00:00 2001 From: Dan Moseley Date: Sun, 2 Jun 2019 06:53:55 -0700 Subject: [PATCH] Improve fatal err msg (dotnet/coreclr#24390) * Improve fatal err msg * Match SO format * a * Remove dead define * More adjustments to ex msg * And PAL * Remove special case for SOE * Remove excess Debug.Assert newline * Remove excess newline * typo * New format * Remove DebugProvider redundancy * Adjustments * Remove preceding newline * Make other SOE and OOM consistent * Tidy up assertion msg * Fix missing newline after inner exception divider * CR when no inner exception * ToString never CR terminated * disable corefx tests temporarily Commit migrated from https://github.com/dotnet/coreclr/commit/b0381d7bf05cc4a342eb1f7fa33d17885f47d999 --- src/coreclr/src/dlls/mscorrc/mscorrc.rc | 2 +- src/coreclr/src/pal/src/include/pal/palinternal.h | 2 +- src/coreclr/src/vm/comutilnative.cpp | 10 ---------- src/coreclr/src/vm/eepolicy.cpp | 19 +++++++++---------- src/coreclr/src/vm/eepolicy.h | 3 --- src/coreclr/src/vm/eventreporter.cpp | 6 +++--- src/coreclr/src/vm/excep.cpp | 16 +++------------- src/coreclr/tests/CoreFX/CoreFX.issues.rsp | 5 +++++ .../src/System/Diagnostics/DebugProvider.Unix.cs | 14 +------------- .../src/System/Diagnostics/DebugProvider.Windows.cs | 14 +------------- .../src/System/Diagnostics/DebugProvider.cs | 20 +++++++++++--------- 11 files changed, 35 insertions(+), 76 deletions(-) diff --git a/src/coreclr/src/dlls/mscorrc/mscorrc.rc b/src/coreclr/src/dlls/mscorrc/mscorrc.rc index f11d48a..149e096 100644 --- a/src/coreclr/src/dlls/mscorrc/mscorrc.rc +++ b/src/coreclr/src/dlls/mscorrc/mscorrc.rc @@ -837,7 +837,7 @@ BEGIN IDS_EE_STRUCTARRAYTOOLARGE "Array size exceeds addressing limitations." IDS_EE_TOOMANYFIELDS "Internal limitation: too many fields." - IDS_EE_UNHANDLED_EXCEPTION "Unhandled Exception:" + IDS_EE_UNHANDLED_EXCEPTION "Unhandled exception." IDS_EE_EXCEPTION_TOSTRING_FAILED "Cannot print exception string because Exception.ToString() failed." IDS_EE_THREAD_ABORT "Thread was being aborted." diff --git a/src/coreclr/src/pal/src/include/pal/palinternal.h b/src/coreclr/src/pal/src/include/pal/palinternal.h index 7517e91..77978f3 100644 --- a/src/coreclr/src/pal/src/include/pal/palinternal.h +++ b/src/coreclr/src/pal/src/include/pal/palinternal.h @@ -760,7 +760,7 @@ inline T* InterlockedCompareExchangePointerT( #include "volatile.h" -const char StackOverflowMessage[] = "Process is terminating due to StackOverflowException.\n"; +const char StackOverflowMessage[] = "Stack overflow.\n"; #endif // __cplusplus diff --git a/src/coreclr/src/vm/comutilnative.cpp b/src/coreclr/src/vm/comutilnative.cpp index 3305b8a..b912ace 100644 --- a/src/coreclr/src/vm/comutilnative.cpp +++ b/src/coreclr/src/vm/comutilnative.cpp @@ -43,8 +43,6 @@ #include "arraynative.inl" -#define STACK_OVERFLOW_MESSAGE W("StackOverflowException") - /*===================================IsDigit==================================== **Returns a bool indicating whether the character passed in represents a ** **digit. @@ -536,14 +534,6 @@ void ExceptionNative::GetExceptionData(OBJECTREF objException, ExceptionData *pE ZeroMemory(pED, sizeof(ExceptionData)); - if (objException->GetMethodTable() == g_pStackOverflowExceptionClass) { - // In a low stack situation, most everything else in here will fail. - // @TODO: We're not turning the guard page back on here, yet. - pED->hr = COR_E_STACKOVERFLOW; - pED->bstrDescription = SysAllocString(STACK_OVERFLOW_MESSAGE); - return; - } - GCPROTECT_BEGIN(objException); pED->hr = GetExceptionHResult(objException); pED->bstrDescription = GetExceptionDescription(objException); diff --git a/src/coreclr/src/vm/eepolicy.cpp b/src/coreclr/src/vm/eepolicy.cpp index 66ec5c6..b4d6e1b 100644 --- a/src/coreclr/src/vm/eepolicy.cpp +++ b/src/coreclr/src/vm/eepolicy.cpp @@ -804,8 +804,8 @@ inline void LogCallstackForLogWorker() // Arguments: // exitCode - code of the fatal error // pszMessage - error message (can be NULL) -// errorSource - details on the source of the error -// argExceptionString - exception details +// errorSource - details on the source of the error (can be NULL) +// argExceptionString - exception details (can be NULL) // // Return Value: // None @@ -817,9 +817,13 @@ void LogInfoForFatalError(UINT exitCode, LPCWSTR pszMessage, LPCWSTR errorSource Thread *pThread = GetThread(); EX_TRY { - if ((exitCode == (UINT)COR_E_FAILFAST) && (errorSource == NULL)) + if (exitCode == (UINT)COR_E_FAILFAST) { - PrintToStdErrA("FailFast:\n"); + PrintToStdErrA("Process terminated. "); + } + else + { + PrintToStdErrA("Fatal error. "); } if (errorSource != NULL) @@ -847,11 +851,7 @@ void LogInfoForFatalError(UINT exitCode, LPCWSTR pszMessage, LPCWSTR errorSource LogCallstackForLogWorker(); if (argExceptionString != NULL) { - PrintToStdErrA("\n"); - PrintToStdErrA("Exception details:"); - PrintToStdErrA("\n"); PrintToStdErrW(argExceptionString); - PrintToStdErrA("\n"); } } } @@ -1042,9 +1042,8 @@ void EEPolicy::LogFatalError(UINT exitCode, UINT_PTR address, LPCWSTR pszMessage void DisplayStackOverflowException() { LIMITED_METHOD_CONTRACT; - PrintToStdErrA("\n"); - PrintToStdErrA("Process is terminating due to StackOverflowException.\n"); + PrintToStdErrA("Stack overflow.\n"); } void DECLSPEC_NORETURN EEPolicy::HandleFatalStackOverflow(EXCEPTION_POINTERS *pExceptionInfo, BOOL fSkipDebugger) diff --git a/src/coreclr/src/vm/eepolicy.h b/src/coreclr/src/vm/eepolicy.h index a87527c..0142ac5 100644 --- a/src/coreclr/src/vm/eepolicy.h +++ b/src/coreclr/src/vm/eepolicy.h @@ -181,7 +181,4 @@ inline EEPolicy* GetEEPolicy() // FailFast with specific error code and exception details #define EEPOLICY_HANDLE_FATAL_ERROR_USING_EXCEPTION_INFO(_exitcode, _pExceptionInfo) EEPolicy::HandleFatalError(_exitcode, GetCurrentIP(), NULL, _pExceptionInfo); -// Failfast with specific error code, exception details, and debug info -#define EEPOLICY_HANDLE_FATAL_ERROR_USING_EXCEPTION_AND_DEBUG_INFO(_exitcode, _pExceptionInfo, _isDebug) EEPolicy::HandleFatalError(_exitcode, GetCurrentIP(), NULL, _pExceptionInfo, _isDebug); - #endif // EEPOLICY_H_ diff --git a/src/coreclr/src/vm/eventreporter.cpp b/src/coreclr/src/vm/eventreporter.cpp index 53556b2..74be2ef 100644 --- a/src/coreclr/src/vm/eventreporter.cpp +++ b/src/coreclr/src/vm/eventreporter.cpp @@ -135,7 +135,7 @@ EventReporter::EventReporter(EventReporterType type) case ERT_ManagedFailFast: if(!ssMessage.LoadResource(CCompRC::Optional, IDS_ER_MANAGEDFAILFAST)) - m_Description.Append(W("Description: The application requested process termination through System.Environment.FailFast(string message).")); + m_Description.Append(W("Description: The application requested process termination through Environment.FailFast.")); else { m_Description.Append(ssMessage); @@ -145,7 +145,7 @@ EventReporter::EventReporter(EventReporterType type) case ERT_UnmanagedFailFast: if(!ssMessage.LoadResource(CCompRC::Optional, IDS_ER_UNMANAGEDFAILFAST)) - m_Description.Append(W("Description: The process was terminated due to an internal error in the .NET Runtime ")); + m_Description.Append(W("Description: The process was terminated due to an internal error in the .NET Runtime.")); else { m_Description.Append(ssMessage); @@ -155,7 +155,7 @@ EventReporter::EventReporter(EventReporterType type) case ERT_StackOverflow: // Fetch the localized Stack Overflow Error text or fall back on a hardcoded variant if things get dire. if(!ssMessage.LoadResource(CCompRC::Optional, IDS_ER_STACK_OVERFLOW)) - m_Description.Append(W("Description: The process was terminated due to stack overflow.")); + m_Description.Append(W("Description: The process was terminated due to a stack overflow.")); else { m_Description.Append(ssMessage); diff --git a/src/coreclr/src/vm/excep.cpp b/src/coreclr/src/vm/excep.cpp index 15598af..7a949f4 100644 --- a/src/coreclr/src/vm/excep.cpp +++ b/src/coreclr/src/vm/excep.cpp @@ -105,7 +105,7 @@ BOOL IsExceptionFromManagedCode(const EXCEPTION_RECORD * pExceptionRecord) #ifndef DACCESS_COMPILE -#define SZ_UNHANDLED_EXCEPTION W("Unhandled Exception:") +#define SZ_UNHANDLED_EXCEPTION W("Unhandled exception.") #define SZ_UNHANDLED_EXCEPTION_CHARLEN ((sizeof(SZ_UNHANDLED_EXCEPTION) / sizeof(WCHAR))) @@ -5243,8 +5243,6 @@ DefaultCatchHandlerExceptionMessageWorker(Thread* pThread, GCPROTECT_BEGIN(throwable); if (throwable != NULL) { - PrintToStdErrA("\n"); - if (FAILED(UtilLoadResourceString(CCompRC::Error, IDS_EE_UNHANDLED_EXCEPTION, buf, buf_size))) { wcsncpy_s(buf, buf_size, SZ_UNHANDLED_EXCEPTION, SZ_UNHANDLED_EXCEPTION_CHARLEN); @@ -5440,22 +5438,14 @@ DefaultCatchHandler(PEXCEPTION_POINTERS pExceptionPointers, // die. e.g. IsAsyncThreadException() and Exception.ToString both consume too much stack -- and can't // be called here. dump = FALSE; - PrintToStdErrA("\n"); - - if (FAILED(UtilLoadStringRC(IDS_EE_UNHANDLED_EXCEPTION, buf, buf_size))) - { - wcsncpy_s(buf, COUNTOF(buf), SZ_UNHANDLED_EXCEPTION, SZ_UNHANDLED_EXCEPTION_CHARLEN); - } - - PrintToStdErrW(buf); if (IsOutOfMemory) { - PrintToStdErrA(" OutOfMemoryException.\n"); + PrintToStdErrA("Out of memory.\n"); } else { - PrintToStdErrA(" StackOverflowException.\n"); + PrintToStdErrA("Stack overflow.\n"); } } else if (SentEvent || IsAsyncThreadException(&throwable)) diff --git a/src/coreclr/tests/CoreFX/CoreFX.issues.rsp b/src/coreclr/tests/CoreFX/CoreFX.issues.rsp index 475d06f..bdc931a 100644 --- a/src/coreclr/tests/CoreFX/CoreFX.issues.rsp +++ b/src/coreclr/tests/CoreFX/CoreFX.issues.rsp @@ -51,3 +51,8 @@ -nomethod System.Tests.AppDomainTests.MonitoringTotalAllocatedMemorySize -nomethod System.Tests.AppDomainTests.MonitoringTotalProcessorTime -nomethod System.Text.Tests.StringBuilderTests.AppendFormat + +# requires corefx test updates +-nomethod System.Tests.EnvironmentTests.FailFast_ExceptionStackTrace_StackOverflowException +-nomethod System.Tests.EnvironmentTests.FailFast_ExceptionStackTrace_InnerException +-nomethod System.Tests.EnvironmentTests.FailFast_ExceptionStackTrace_ArgumentException diff --git a/src/libraries/System.Private.CoreLib/src/System/Diagnostics/DebugProvider.Unix.cs b/src/libraries/System.Private.CoreLib/src/System/Diagnostics/DebugProvider.Unix.cs index 0799d09..8588764 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Diagnostics/DebugProvider.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Diagnostics/DebugProvider.Unix.cs @@ -27,19 +27,7 @@ namespace System.Diagnostics // In Core, we do not show a dialog. // Fail in order to avoid anyone catching an exception and masking // an assert failure. - DebugAssertException ex; - if (message == string.Empty) - { - ex = new DebugAssertException(stackTrace); - } - else if (detailMessage == string.Empty) - { - ex = new DebugAssertException(message, stackTrace); - } - else - { - ex = new DebugAssertException(message, detailMessage, stackTrace); - } + DebugAssertException ex = new DebugAssertException(message, detailMessage, stackTrace); Environment.FailFast(ex.Message, ex, errorSource); } } diff --git a/src/libraries/System.Private.CoreLib/src/System/Diagnostics/DebugProvider.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/Diagnostics/DebugProvider.Windows.cs index e382e81..6714746 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Diagnostics/DebugProvider.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Diagnostics/DebugProvider.Windows.cs @@ -23,19 +23,7 @@ namespace System.Diagnostics // In Core, we do not show a dialog. // Fail in order to avoid anyone catching an exception and masking // an assert failure. - DebugAssertException ex; - if (message == string.Empty) - { - ex = new DebugAssertException(stackTrace); - } - else if (detailMessage == string.Empty) - { - ex = new DebugAssertException(message, stackTrace); - } - else - { - ex = new DebugAssertException(message, detailMessage, stackTrace); - } + DebugAssertException ex = new DebugAssertException(message, detailMessage, stackTrace); Environment.FailFast(ex.Message, ex, errorSource); } } diff --git a/src/libraries/System.Private.CoreLib/src/System/Diagnostics/DebugProvider.cs b/src/libraries/System.Private.CoreLib/src/System/Diagnostics/DebugProvider.cs index 939f371..3037277 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Diagnostics/DebugProvider.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Diagnostics/DebugProvider.cs @@ -24,7 +24,7 @@ namespace System.Diagnostics stackTrace = ""; } WriteAssert(stackTrace, message, detailMessage); - FailCore(stackTrace, message, detailMessage, "Assertion Failed"); + FailCore(stackTrace, message, detailMessage, "Assertion failed."); } internal void WriteAssert(string stackTrace, string? message, string? detailMessage) @@ -72,19 +72,21 @@ namespace System.Diagnostics private sealed class DebugAssertException : Exception { - internal DebugAssertException(string? stackTrace) : - base(Environment.NewLine + stackTrace) + internal DebugAssertException(string? message, string? detailMessage, string? stackTrace) : + base(Terminate(message) + Terminate(detailMessage) + stackTrace) { } - internal DebugAssertException(string? message, string? stackTrace) : - base(message + Environment.NewLine + Environment.NewLine + stackTrace) + private static string? Terminate(string? s) { - } + if (s == null) + return s; - internal DebugAssertException(string? message, string? detailMessage, string? stackTrace) : - base(message + Environment.NewLine + detailMessage + Environment.NewLine + Environment.NewLine + stackTrace) - { + s = s.Trim(); + if (s.Length > 0) + s += Environment.NewLine; + + return s; } } -- 2.7.4