From 756ac603db6d6f706faded71209c17fda361341e Mon Sep 17 00:00:00 2001 From: Nik Bougalis Date: Sun, 16 Nov 2014 23:17:42 -0800 Subject: [PATCH] Simplify the Beast fatal error reporting framework: * Reduce interface to a single function which reports error details * Remove unused functions --- beast/module/core/diagnostic/FatalError.cpp | 135 ++++++------------- beast/module/core/diagnostic/FatalError.h | 125 +---------------- beast/module/core/system/SystemStats.cpp | 50 ++++--- beast/module/core/system/SystemStats.h | 3 +- beast/utility/Debug.h | 9 -- beast/utility/impl/Debug.cpp | 142 -------------------- 6 files changed, 75 insertions(+), 389 deletions(-) diff --git a/beast/module/core/diagnostic/FatalError.cpp b/beast/module/core/diagnostic/FatalError.cpp index 3add7cbe76..6806a20652 100644 --- a/beast/module/core/diagnostic/FatalError.cpp +++ b/beast/module/core/diagnostic/FatalError.cpp @@ -18,111 +18,58 @@ //============================================================================== #include -#include + +#include +#include +#include +#include namespace beast { -// -// FatalError::Reporter -// -void FatalError::Reporter::onFatalError ( - char const* message, - char const* backtrace, - char const* filePath, - int lineNumber) -{ - reportMessage (formatMessage (message, backtrace, filePath, lineNumber)); -} - -void FatalError::Reporter::reportMessage (std::string const& message) -{ - std::cerr << message << std::endl; -} - -std::string FatalError::Reporter::formatMessage ( - char const* message, - char const* backtrace, - char const* filePath, - int lineNumber) -{ - std::string output; - output.reserve (16 * 1024); - - output.append (message); - - if (filePath != nullptr && filePath [0] != 0) - { - output.append (", in "); - output.append (formatFilePath (filePath)); - output.append (" line "); - output.append (std::to_string (lineNumber)); - } - - output.append ("\n"); - - if (backtrace != nullptr && backtrace[0] != 0) - { - output.append ("Stack:\n"); - output.append (backtrace); - } - - return output; -} - -std::string FatalError::Reporter::formatFilePath (char const* filePath) -{ - return filePath; -} - //------------------------------------------------------------------------------ - -FatalError::Reporter *FatalError::s_reporter; - -/** Returns the current fatal error reporter. */ -FatalError::Reporter* FatalError::getReporter () +void +FatalError (char const* message, char const* file, int line) { - return s_reporter; -} + static std::atomic error_count (0); + static std::recursive_mutex gate; -FatalError::Reporter* FatalError::setReporter (Reporter* reporter) -{ - Reporter* const previous (s_reporter); - s_reporter = reporter; - return previous; -} + // We only allow one thread to report a fatal error. Other threads that + // encounter fatal errors while we are reporting get blocked here. + std::lock_guard lock(gate); -FatalError::FatalError (char const* message, char const* fileName, int lineNumber) -{ - typedef CriticalSection LockType; + // If we encounter a recursive fatal error, then we want to terminate + // unconditionally. + if (error_count++ != 0) + return std::terminate (); - static LockType s_mutex; - - std::lock_guard lock (s_mutex); - - auto const backtrace = SystemStats::getStackBacktrace (); - - Reporter* const reporter = s_reporter; - - if (reporter != nullptr) + // We protect this entire block of code since writing to cerr might trigger + // exceptions. + try { - reporter->onFatalError (message, backtrace.c_str (), fileName, lineNumber); + std::cerr << "An error has occurred. The application will terminate.\n"; + + if (message != nullptr && message [0] != 0) + std::cerr << "Message: " << message << '\n'; + + if (file != nullptr && file [0] != 0) + std::cerr << " File: " << file << ":" << line << '\n'; + + auto const backtrace = SystemStats::getStackBacktrace (); + + if (!backtrace.empty ()) + { + std::cerr << " Stack:" << std::endl; + + for (auto const& frame : backtrace) + std::cerr << " " << frame << '\n'; + } + } + catch (...) + { + // nothing we can do - just fall through and terminate } - Process::terminate (); + return std::terminate (); } -//------------------------------------------------------------------------------ - -class FatalError_test : public unit_test::suite -{ -public: - void run () - { - int shouldBeZero (1); - check_invariant (shouldBeZero == 0); - } -}; - -BEAST_DEFINE_TESTSUITE_MANUAL(FatalError,beast_core,beast); - } // beast diff --git a/beast/module/core/diagnostic/FatalError.h b/beast/module/core/diagnostic/FatalError.h index d91c8bb4dd..9b11b27423 100644 --- a/beast/module/core/diagnostic/FatalError.h +++ b/beast/module/core/diagnostic/FatalError.h @@ -32,128 +32,11 @@ namespace beast would be to protect data integrity, prevent valuable resources from being wasted, or to ensure that the user does not experience undefined behavior. - This function will end the process with exit code EXIT_FAILURE. Before - the process is terminated, a listener object gets notified so that the - client application can perform logging or emit further diagnostics. + If multiple threads raise an error, only one will succeed while the others + will be blocked before the process terminates. */ -class FatalError -{ -public: - struct Reporter - { - virtual ~Reporter() = default; - - /** Called when a fatal error is raised. - - Because the program is likely in an inconsistent state, it is a - good idea to do as little as possible from within this function. - It will be called from the thread that raised the fatal error. - - The default implementation of this function first calls - formatMessage to produce the string, then calls reportMessage - to report the results. - - You can override this to perform custom formatting. - - @note filePath may be a zero length string if identifying - information was stripped from the executable for security. - - @note stackBacktrace will be a string with zero characters for - platforms for which which don't support stack crawls, or - when symbolic information is missing from the executable. - - @param message The error message. - @param stackBackTrace The stack of the thread that raised the error. - @param filePath A full or partial path to the source file that raised the error. - @param lineNumber The line number in the source file. - */ - virtual void onFatalError (char const* message, - char const* backtrace, - char const* filePath, - int lineNumber); - - /** Called to report the message. - - The default implementation simply writes this to standard error. - You can override this to perform additional things like logging - to a file or sending the message to another process. - - @param formattedMessage The message to report. - */ - virtual void reportMessage (std::string const& formattedMessage); - - protected: - /** Called to format the message. - - The default implementation calls formatFilePath to produce - a formatted file name, and then creates a suitable string - containing all of the information. - - You can override this function to format your own messages. - - @param message The message from the report. - @param stackBacktrace The stack backtrace from the report. - @param filePath The file path from the report. - @param lineNumber The line number from the report - */ - virtual std::string formatMessage ( - char const* message, - char const* backtrace, - char const* filePath, - int lineNumber); - - /** Call to reformat the file path. - - Usually the file is a full path, which we really don't care - to see and can also be a security hole. - - The default implementation removes most of the useless - directory components from the front. - - You can override this to do a custom format on the file path. - */ - virtual std::string formatFilePath (char const* filePath); - }; - - /** Returns the current fatal error reporter. */ - static Reporter* getReporter (); - - /** Set the fatal error reporter. - - Note that if a fatal error is raised during the construction of - objects with static storage duration, it might not be possible to - set the reporter before the error is raised. The solution is not - to use objects with static storage duration that have non-trivial - constructors, use SharedSingleton instead. - - The default behavior when no reporter is set is to invoke - the base class version of Reporter::onFatalError. - - If a reporter was previously set, this routine will do nothing. - - @return The previous Reporter (Which may be null). - - @see SharedSingleton, Reporter - */ - static Reporter* setReporter (Reporter* reporter); - - /** Raise a fatal error. - - If multiple threads raise an error, only one will succeed. The - other threads will be blocked before the process terminates. - - @param message A null terminated string, which should come from a constant. - @param filePath Pass __FILE__ here. - @param lineNumber Pass __LINE__ here. - */ - FatalError (char const* message, char const* filePath, int lineNumber); - - FatalError(FatalError const&) = delete; - FatalError& operator= (FatalError const&) = delete; - -private: - static Reporter* s_reporter; -}; +void +FatalError (char const* message, char const* file = nullptr, int line = 0); } // beast diff --git a/beast/module/core/system/SystemStats.cpp b/beast/module/core/system/SystemStats.cpp index 556d3f781c..0a121402e4 100644 --- a/beast/module/core/system/SystemStats.cpp +++ b/beast/module/core/system/SystemStats.cpp @@ -21,6 +21,10 @@ */ //============================================================================== +#include +#include +#include + // Some basic tests, to keep an eye on things and make sure these types work ok // on all platforms. @@ -48,20 +52,22 @@ SystemStats::getBeastVersion() } //============================================================================== -std::string +std::vector SystemStats::getStackBacktrace() { - std::string result; + std::vector result; - #if BEAST_ANDROID || BEAST_MINGW || BEAST_BSD +#if BEAST_ANDROID || BEAST_MINGW || BEAST_BSD bassertfalse; // sorry, not implemented yet! - #elif BEAST_WINDOWS +#elif BEAST_WINDOWS HANDLE process = GetCurrentProcess(); SymInitialize (process, nullptr, TRUE); void* stack[128]; - int frames = (int) CaptureStackBackTrace (0, numElementsInArray (stack), stack, nullptr); + int frames = (int) CaptureStackBackTrace (0, + std::distance(std::begin(stack), std::end(stack)), + stack, nullptr); HeapBlock symbol; symbol.calloc (sizeof (SYMBOL_INFO) + 256, 1); @@ -74,7 +80,9 @@ SystemStats::getStackBacktrace() if (SymFromAddr (process, (DWORD64) stack[i], &displacement, symbol)) { - result.append (std::to_string (i) + ": "); + std::string frame; + + frame.append (std::to_string (i) + ": "); IMAGEHLP_MODULE64 moduleInfo; zerostruct (moduleInfo); @@ -82,35 +90,33 @@ SystemStats::getStackBacktrace() if (::SymGetModuleInfo64 (process, symbol->ModBase, &moduleInfo)) { - result.append (moduleInfo.ModuleName); - result.append (": "); + frame.append (moduleInfo.ModuleName); + frame.append (": "); } - result.append (symbol->Name); + frame.append (symbol->Name); if (displacement) { - result.append ("+"); - result.append (std::to_string (displacement)); + frame.append ("+"); + frame.append (std::to_string (displacement)); } - result.append ("\r\n"); + result.push_back (frame); } } - #else +#else void* stack[128]; - int frames = backtrace (stack, numElementsInArray (stack)); - char** frameStrings = backtrace_symbols (stack, frames); + int frames = backtrace (stack, + std::distance(std::begin(stack), std::end(stack))); + + std::unique_ptr frame ( + backtrace_symbols (stack, frames), std::free); for (int i = 0; i < frames; ++i) - { - result.append (frameStrings[i]); - result.append ("\n"); - } - - ::free (frameStrings); - #endif + result.push_back (frame[i]); +#endif return result; } diff --git a/beast/module/core/system/SystemStats.h b/beast/module/core/system/SystemStats.h index f760567931..30806942e3 100644 --- a/beast/module/core/system/SystemStats.h +++ b/beast/module/core/system/SystemStats.h @@ -48,7 +48,8 @@ namespace SystemStats The usefulness of the result will depend on the level of debug symbols that are available in the executable. */ - std::string getStackBacktrace(); + std::vector + getStackBacktrace(); /** A void() function type, used by setApplicationCrashHandler(). */ typedef void (*CrashHandlerFunction)(); diff --git a/beast/utility/Debug.h b/beast/utility/Debug.h index c1b0616a08..f43b3ae2e8 100644 --- a/beast/utility/Debug.h +++ b/beast/utility/Debug.h @@ -46,15 +46,6 @@ String getSourceLocation (char const* fileName, int lineNumber, */ String getFileNameFromPath (const char* sourceFileName, int numberOfParents = 0); -// Convert a String that may contain double quotes and newlines -// into a String with double quotes escaped as \" and each -// line as a separate quoted command line argument. -String stringToCommandLine (const String& s); - -// Convert a quoted and escaped command line back into a String -// that can contain newlines and double quotes. -String commandLineToString (const String& commandLine); - // // These control the MSVC C Runtime Debug heap. // diff --git a/beast/utility/impl/Debug.cpp b/beast/utility/impl/Debug.cpp index c66bcb8da8..e533df7602 100644 --- a/beast/utility/impl/Debug.cpp +++ b/beast/utility/impl/Debug.cpp @@ -155,148 +155,6 @@ String getFileNameFromPath (const char* sourceFileName, int numberOfParents) return path; } -// Returns a String with double quotes escaped -static const String withEscapedQuotes (String const& string) -{ - String escaped; - - int i0 = 0; - int i; - - do - { - i = string.indexOfChar (i0, '"'); - - if (i == -1) - { - escaped << string.substring (i0, string.length ()); - } - else - { - escaped << string.substring (i0, i) << "\\\""; - i0 = i + 1; - } - } - while (i != -1); - - return escaped; -} - -// Converts escaped quotes back into regular quotes -static const String withUnescapedQuotes (String const& string) -{ - String unescaped; - - int i0 = 0; - int i; - - do - { - i = string.indexOfChar (i0, '\\'); - - if (i == -1) - { - unescaped << string.substring (i0, string.length ()); - } - else - { - // peek - if (string.length () > i && string[i + 1] == '\"') - { - unescaped << string.substring (i0, i) << '"'; - i0 = i + 2; - } - else - { - unescaped << string.substring (i0, i + 1); - i0 = i + 1; - } - } - } - while (i != -1); - - return unescaped; -} - -// Converts a String that may contain newlines, into a -// command line where each line is delimited with quotes. -// Any quotes in the actual string will be escaped via \". -String stringToCommandLine (String const& string) -{ - String commandLine; - - int i0 = 0; - int i; - - for (i = 0; i < string.length (); i++) - { - beast_wchar c = string[i]; - - if (c == '\n') - { - if (i0 != 0) - commandLine << ' '; - - commandLine << '"' << withEscapedQuotes (string.substring (i0, i)) << '"'; - i0 = i + 1; - } - } - - if (i0 < i) - { - if (i0 != 0) - commandLine << ' '; - - commandLine << '"' << withEscapedQuotes (string.substring (i0, i)) << '"'; - } - - return commandLine; -} - -// Converts a command line consisting of multiple quoted strings -// back into a single string with newlines delimiting each quoted -// string. Escaped quotes \" are turned into real quotes. -String commandLineToString (const String& commandLine) -{ - String string; - - bool quoting = false; - int i0 = 0; - int i; - - for (i = 0; i < commandLine.length (); i++) - { - beast_wchar c = commandLine[i]; - - if (c == '\\') - { - // peek - if (commandLine.length () > i && commandLine[i + 1] == '\"') - { - i++; - } - } - else if (c == '"') - { - if (!quoting) - { - i0 = i + 1; - quoting = true; - } - else - { - if (!string.isEmpty ()) - string << '\n'; - - string << withUnescapedQuotes (commandLine.substring (i0, i)); - quoting = false; - } - } - } - - return string; -} - } //------------------------------------------------------------------------------