From f8b7a1511bb14e295381241c2c8786db20a76e40 Mon Sep 17 00:00:00 2001 From: Julian Brost Date: Wed, 14 Oct 2020 15:21:25 +0200 Subject: [PATCH 01/19] Run termination handler for uncaught C++ exceptions on Windows On Windows, the termination handler is executed for uncaught C++ exceptions unless a SEH unhandled exception filter is also set. In this case, this filter has to explicitly chain the default filter to keep this behavior. --- lib/base/application.cpp | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/lib/base/application.cpp b/lib/base/application.cpp index 2339250ec..a560e888e 100644 --- a/lib/base/application.cpp +++ b/lib/base/application.cpp @@ -34,6 +34,14 @@ using namespace icinga; +#ifdef _WIN32 +/* MSVC throws unhandled C++ exceptions as SEH exceptions with this specific error code. + * There seems to be no system header that actually defines this constant. + * See also https://devblogs.microsoft.com/oldnewthing/20160915-00/?p=94316 + */ +#define EXCEPTION_CODE_CXX_EXCEPTION 0xe06d7363 +#endif /* _WIN32 */ + REGISTER_TYPE(Application); boost::signals2::signal Application::OnReopenLogs; @@ -55,6 +63,10 @@ double Application::m_StartTime; bool Application::m_ScriptDebuggerEnabled = false; double Application::m_LastReloadFailed; +#ifdef _WIN32 +static LPTOP_LEVEL_EXCEPTION_FILTER l_DefaultUnhandledExceptionFilter = nullptr; +#endif /* _WIN32 */ + /** * Constructor for the Application class. */ @@ -885,6 +897,15 @@ void Application::ExceptionHandler() #ifdef _WIN32 LONG CALLBACK Application::SEHUnhandledExceptionFilter(PEXCEPTION_POINTERS exi) { + /* If an unhandled C++ exception occurs with both a termination handler (std::set_terminate()) and an unhandled + * SEH filter (SetUnhandledExceptionFilter()) set, the latter one is called. However, our termination handler is + * better suited for dealing with C++ exceptions. In this case, the SEH exception will have a specific code and + * we can just call the default filter function which will take care of calling the termination handler. + */ + if (exi->ExceptionRecord->ExceptionCode == EXCEPTION_CODE_CXX_EXCEPTION) { + return l_DefaultUnhandledExceptionFilter(exi); + } + if (l_InExceptionHandler) return EXCEPTION_CONTINUE_SEARCH; @@ -938,7 +959,7 @@ void Application::InstallExceptionHandlers() sa.sa_handler = &Application::SigAbrtHandler; sigaction(SIGABRT, &sa, nullptr); #else /* _WIN32 */ - SetUnhandledExceptionFilter(&Application::SEHUnhandledExceptionFilter); + l_DefaultUnhandledExceptionFilter = SetUnhandledExceptionFilter(&Application::SEHUnhandledExceptionFilter); #endif /* _WIN32 */ } From 0db59bad83a1a59cf927f7bb6c7845bc9d65f199 Mon Sep 17 00:00:00 2001 From: Julian Brost Date: Wed, 14 Oct 2020 16:25:56 +0200 Subject: [PATCH 02/19] Use boost::stacktrace instead of custom implementation in Windows SEH filter --- lib/base/application.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/base/application.cpp b/lib/base/application.cpp index a560e888e..7a5728c0d 100644 --- a/lib/base/application.cpp +++ b/lib/base/application.cpp @@ -19,6 +19,7 @@ #include #include #include +#include #include #include #include @@ -936,9 +937,9 @@ LONG CALLBACK Application::SEHUnhandledExceptionFilter(PEXCEPTION_POINTERS exi) << "Current time: " << Utility::FormatDateTime("%Y-%m-%d %H:%M:%S %z", Utility::GetTime()) << "\n" << "\n"; - StackTrace trace(exi); - ofs << "Stacktrace:" << "\n"; - trace.Print(ofs, 1); + ofs << "Stacktrace:\n" + << boost::stacktrace::stacktrace() + << "\n"; DisplayBugMessage(ofs); From 68e4b807d1948f7d344903cabfc579c608457675 Mon Sep 17 00:00:00 2001 From: Julian Brost Date: Wed, 14 Oct 2020 17:57:17 +0200 Subject: [PATCH 03/19] Move error message and time to the beginning of the SEH crash log This is more similar to the normal exception crashlog which also states the problem and time at the beginning of the file. --- lib/base/application.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/base/application.cpp b/lib/base/application.cpp index 7a5728c0d..7b92fdbf4 100644 --- a/lib/base/application.cpp +++ b/lib/base/application.cpp @@ -931,12 +931,12 @@ LONG CALLBACK Application::SEHUnhandledExceptionFilter(PEXCEPTION_POINTERS exi) Log(LogCritical, "Application") << "Icinga 2 has terminated unexpectedly. Additional information can be found in '" << fname << "'"; - DisplayInfoMessage(ofs); - ofs << "Caught unhandled SEH exception." << "\n" << "Current time: " << Utility::FormatDateTime("%Y-%m-%d %H:%M:%S %z", Utility::GetTime()) << "\n" << "\n"; + DisplayInfoMessage(ofs); + ofs << "Stacktrace:\n" << boost::stacktrace::stacktrace() << "\n"; From e11b4b7b55e22c81b4f1768902c2312c087b7c3b Mon Sep 17 00:00:00 2001 From: Julian Brost Date: Wed, 14 Oct 2020 18:05:49 +0200 Subject: [PATCH 04/19] Print details in uncaught SEH exception handler --- lib/base/application.cpp | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/lib/base/application.cpp b/lib/base/application.cpp index 7b92fdbf4..9d0d0475a 100644 --- a/lib/base/application.cpp +++ b/lib/base/application.cpp @@ -937,9 +937,15 @@ LONG CALLBACK Application::SEHUnhandledExceptionFilter(PEXCEPTION_POINTERS exi) DisplayInfoMessage(ofs); - ofs << "Stacktrace:\n" - << boost::stacktrace::stacktrace() - << "\n"; + std::ios::fmtflags savedflags(ofs.flags()); + ofs << std::showbase << std::hex + << "\nSEH exception:\n" + << " Code: " << exi->ExceptionRecord->ExceptionCode << "\n" + << " Address: " << exi->ExceptionRecord->ExceptionAddress << "\n" + << " Flags: " << exi->ExceptionRecord->ExceptionFlags << "\n"; + ofs.flags(savedflags); + + ofs << "\nStacktrace:\n" << boost::stacktrace::stacktrace() << "\n"; DisplayBugMessage(ofs); From 5cb7c7cc095b62f2dd4aab2b838f5cdc0f7d19f8 Mon Sep 17 00:00:00 2001 From: Julian Brost Date: Tue, 20 Oct 2020 10:38:55 +0200 Subject: [PATCH 05/19] Replace icinga::StackTrace with boost::stacktrace::stacktrace Provides roughly the same functionality but works better on certain platforms (especially Windows) and is less code to maintain. --- lib/base/CMakeLists.txt | 1 - lib/base/application.cpp | 5 +- lib/base/exception.cpp | 18 ++--- lib/base/exception.hpp | 12 ++-- lib/base/stacktrace.cpp | 139 --------------------------------------- lib/base/stacktrace.hpp | 38 ----------- test/CMakeLists.txt | 2 - test/base-stacktrace.cpp | 18 ----- 8 files changed, 17 insertions(+), 216 deletions(-) delete mode 100644 lib/base/stacktrace.cpp delete mode 100644 lib/base/stacktrace.hpp delete mode 100644 test/base-stacktrace.cpp diff --git a/lib/base/CMakeLists.txt b/lib/base/CMakeLists.txt index 108ca27c1..ed8576b56 100644 --- a/lib/base/CMakeLists.txt +++ b/lib/base/CMakeLists.txt @@ -64,7 +64,6 @@ set(base_SOURCES shared-object.hpp singleton.hpp socket.cpp socket.hpp - stacktrace.cpp stacktrace.hpp statsfunction.hpp stdiostream.cpp stdiostream.hpp stream.cpp stream.hpp diff --git a/lib/base/application.cpp b/lib/base/application.cpp index 9d0d0475a..78af235cb 100644 --- a/lib/base/application.cpp +++ b/lib/base/application.cpp @@ -2,7 +2,6 @@ #include "base/application.hpp" #include "base/application-ti.cpp" -#include "base/stacktrace.hpp" #include "base/timer.hpp" #include "base/logger.hpp" #include "base/exception.hpp" @@ -761,9 +760,7 @@ void Application::SigAbrtHandler(int) DisplayInfoMessage(ofs); - StackTrace trace; - ofs << "Stacktrace:" << "\n"; - trace.Print(ofs, 1); + ofs << "\nStacktrace:\n" << boost::stacktrace::stacktrace() << "\n"; DisplayBugMessage(ofs); diff --git a/lib/base/exception.cpp b/lib/base/exception.cpp index cb505b04b..a080fdcba 100644 --- a/lib/base/exception.cpp +++ b/lib/base/exception.cpp @@ -14,7 +14,7 @@ using namespace icinga; -static boost::thread_specific_ptr l_LastExceptionStack; +static boost::thread_specific_ptr l_LastExceptionStack; static boost::thread_specific_ptr l_LastExceptionContext; #ifdef HAVE_CXXABI_H @@ -113,7 +113,7 @@ void __cxa_throw(void *obj, TYPEINFO_TYPE *pvtinfo, void (*dest)(void *)) if (!uex) { #endif /* NO_CAST_EXCEPTION */ - StackTrace stack; + boost::stacktrace::stacktrace stack; SetLastExceptionStack(stack); #ifndef NO_CAST_EXCEPTION @@ -134,14 +134,14 @@ void __cxa_throw(void *obj, TYPEINFO_TYPE *pvtinfo, void (*dest)(void *)) } #endif /* HAVE_CXXABI_H */ -StackTrace *icinga::GetLastExceptionStack() +boost::stacktrace::stacktrace *icinga::GetLastExceptionStack() { return l_LastExceptionStack.get(); } -void icinga::SetLastExceptionStack(const StackTrace& trace) +void icinga::SetLastExceptionStack(const boost::stacktrace::stacktrace& trace) { - l_LastExceptionStack.reset(new StackTrace(trace)); + l_LastExceptionStack.reset(new boost::stacktrace::stacktrace(trace)); } ContextTrace *icinga::GetLastExceptionContext() @@ -154,7 +154,7 @@ void icinga::SetLastExceptionContext(const ContextTrace& context) l_LastExceptionContext.reset(new ContextTrace(context)); } -String icinga::DiagnosticInformation(const std::exception& ex, bool verbose, StackTrace *stack, ContextTrace *context) +String icinga::DiagnosticInformation(const std::exception& ex, bool verbose, boost::stacktrace::stacktrace *stack, ContextTrace *context) { std::ostringstream result; @@ -216,7 +216,7 @@ String icinga::DiagnosticInformation(const std::exception& ex, bool verbose, Sta const auto *pex = dynamic_cast(&ex); if (!uex && !pex && verbose) { - const StackTrace *st = boost::get_error_info(ex); + const boost::stacktrace::stacktrace *st = boost::get_error_info(ex); if (st) { result << *st; @@ -251,8 +251,8 @@ String icinga::DiagnosticInformation(const std::exception& ex, bool verbose, Sta String icinga::DiagnosticInformation(const boost::exception_ptr& eptr, bool verbose) { - StackTrace *pt = GetLastExceptionStack(); - StackTrace stack; + boost::stacktrace::stacktrace *pt = GetLastExceptionStack(); + boost::stacktrace::stacktrace stack; ContextTrace *pc = GetLastExceptionContext(); ContextTrace context; diff --git a/lib/base/exception.hpp b/lib/base/exception.hpp index 7cc0ce7c7..1a4b71fcf 100644 --- a/lib/base/exception.hpp +++ b/lib/base/exception.hpp @@ -5,7 +5,6 @@ #include "base/i2-base.hpp" #include "base/string.hpp" -#include "base/stacktrace.hpp" #include "base/context.hpp" #include "base/debuginfo.hpp" #include "base/dictionary.hpp" @@ -15,6 +14,7 @@ #include #include #include +#include #ifdef _WIN32 # include @@ -76,15 +76,16 @@ private: Dictionary::Ptr m_DebugHint; }; -StackTrace *GetLastExceptionStack(); -void SetLastExceptionStack(const StackTrace& trace); +boost::stacktrace::stacktrace *GetLastExceptionStack(); +void SetLastExceptionStack(const boost::stacktrace::stacktrace& trace); ContextTrace *GetLastExceptionContext(); void SetLastExceptionContext(const ContextTrace& context); void RethrowUncaughtException(); -typedef boost::error_info StackTraceErrorInfo; +struct errinfo_stacktrace_; +typedef boost::error_info StackTraceErrorInfo; std::string to_string(const StackTraceErrorInfo&); @@ -92,7 +93,8 @@ typedef boost::error_info ContextTraceErrorInfo; std::string to_string(const ContextTraceErrorInfo& e); -String DiagnosticInformation(const std::exception& ex, bool verbose = true, StackTrace *stack = nullptr, ContextTrace *context = nullptr); +String DiagnosticInformation(const std::exception& ex, bool verbose = true, + boost::stacktrace::stacktrace *stack = nullptr, ContextTrace *context = nullptr); String DiagnosticInformation(const boost::exception_ptr& eptr, bool verbose = true); class posix_error : virtual public std::exception, virtual public boost::exception { diff --git a/lib/base/stacktrace.cpp b/lib/base/stacktrace.cpp deleted file mode 100644 index 81fdd3380..000000000 --- a/lib/base/stacktrace.cpp +++ /dev/null @@ -1,139 +0,0 @@ -/* Icinga 2 | (c) 2012 Icinga GmbH | GPLv2+ */ - -#include "base/stacktrace.hpp" -#include "base/utility.hpp" -#include "base/initialize.hpp" - -#ifdef HAVE_BACKTRACE_SYMBOLS -# include -#endif /* HAVE_BACKTRACE_SYMBOLS */ - -using namespace icinga; - -#ifdef _MSC_VER -# pragma optimize("", off) -#endif /* _MSC_VER */ - -StackTrace::StackTrace() -{ -#ifdef HAVE_BACKTRACE_SYMBOLS - m_Count = backtrace(m_Frames, sizeof(m_Frames) / sizeof(m_Frames[0])); -#else /* HAVE_BACKTRACE_SYMBOLS */ -# ifdef _WIN32 - m_Count = CaptureStackBackTrace(0, sizeof(m_Frames) / sizeof(m_Frames), m_Frames, nullptr); -# else /* _WIN32 */ - m_Count = 0; -# endif /* _WIN32 */ -#endif /* HAVE_BACKTRACE_SYMBOLS */ -} - -#ifdef _MSC_VER -# pragma optimize("", on) -#endif /* _MSC_VER */ - -#ifdef _WIN32 -StackTrace::StackTrace(PEXCEPTION_POINTERS exi) -{ - STACKFRAME64 frame; - int architecture; - -#ifdef _WIN64 - architecture = IMAGE_FILE_MACHINE_AMD64; - - frame.AddrPC.Offset = exi->ContextRecord->Rip; - frame.AddrFrame.Offset = exi->ContextRecord->Rbp; - frame.AddrStack.Offset = exi->ContextRecord->Rsp; -#else /* _WIN64 */ - architecture = IMAGE_FILE_MACHINE_I386; - - frame.AddrPC.Offset = exi->ContextRecord->Eip; - frame.AddrFrame.Offset = exi->ContextRecord->Ebp; - frame.AddrStack.Offset = exi->ContextRecord->Esp; -#endif /* _WIN64 */ - - frame.AddrPC.Mode = AddrModeFlat; - frame.AddrFrame.Mode = AddrModeFlat; - frame.AddrStack.Mode = AddrModeFlat; - - m_Count = 0; - - while (StackWalk64(architecture, GetCurrentProcess(), GetCurrentThread(), - &frame, exi->ContextRecord, nullptr, &SymFunctionTableAccess64, - &SymGetModuleBase64, nullptr) && m_Count < sizeof(m_Frames) / sizeof(m_Frames[0])) { - m_Frames[m_Count] = reinterpret_cast(frame.AddrPC.Offset); - m_Count++; - } -} -#endif /* _WIN32 */ - -#ifdef _WIN32 -INITIALIZE_ONCE([]() { - (void) SymSetOptions(SYMOPT_UNDNAME | SYMOPT_LOAD_LINES); - (void) SymInitialize(GetCurrentProcess(), nullptr, TRUE); -}); -#endif /* _WIN32 */ - -/** - * Prints a stacktrace to the specified stream. - * - * @param fp The stream. - * @param ignoreFrames The number of stackframes to ignore (in addition to - * the one this function is executing in). - * @returns true if the stacktrace was printed, false otherwise. - */ -void StackTrace::Print(std::ostream& fp, int ignoreFrames) const -{ - fp << std::endl; - -#ifndef _WIN32 -# ifdef HAVE_BACKTRACE_SYMBOLS - char **messages = backtrace_symbols(m_Frames, m_Count); - - for (int i = ignoreFrames + 1; i < m_Count && messages; ++i) { - String message = messages[i]; - - char *sym_begin = strchr(messages[i], '('); - - if (sym_begin) { - char *sym_end = strchr(sym_begin, '+'); - - if (sym_end) { - String sym = String(sym_begin + 1, sym_end); - String sym_demangled = Utility::DemangleSymbolName(sym); - - if (sym_demangled.IsEmpty()) - sym_demangled = ""; - - String path = String(messages[i], sym_begin); - - size_t slashp = path.RFind("/"); - - if (slashp != String::NPos) - path = path.SubStr(slashp + 1); - - message = path + ": " + sym_demangled + " (" + String(sym_end); - } - } - - fp << "\t(" << i - ignoreFrames - 1 << ") " << message << std::endl; - } - - std::free(messages); - - fp << std::endl; -# else /* HAVE_BACKTRACE_SYMBOLS */ - fp << "(not available)" << std::endl; -# endif /* HAVE_BACKTRACE_SYMBOLS */ -#else /* _WIN32 */ - for (int i = ignoreFrames + 1; i < m_Count; i++) { - fp << "\t(" << i - ignoreFrames - 1 << "): " << Utility::GetSymbolName(m_Frames[i]) << std::endl; - } -#endif /* _WIN32 */ -} - -std::ostream& icinga::operator<<(std::ostream& stream, const StackTrace& trace) -{ - trace.Print(stream, 1); - - return stream; -} diff --git a/lib/base/stacktrace.hpp b/lib/base/stacktrace.hpp deleted file mode 100644 index 1416d6558..000000000 --- a/lib/base/stacktrace.hpp +++ /dev/null @@ -1,38 +0,0 @@ -/* Icinga 2 | (c) 2012 Icinga GmbH | GPLv2+ */ - -#ifndef STACKTRACE_H -#define STACKTRACE_H - -#include "base/i2-base.hpp" -#include - -namespace icinga -{ - -/** - * A stacktrace. - * - * @ingroup base - */ -class StackTrace -{ -public: - StackTrace(); -#ifdef _WIN32 - explicit StackTrace(PEXCEPTION_POINTERS exi); -#endif /* _WIN32 */ - - void Print(std::ostream& fp, int ignoreFrames = 0) const; - - static void StaticInitialize(); - -private: - void *m_Frames[64]; - int m_Count; -}; - -std::ostream& operator<<(std::ostream& stream, const StackTrace& trace); - -} - -#endif /* UTILITY_H */ diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 3bcf04319..274456d1c 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -16,7 +16,6 @@ set(base_test_SOURCES base-object-packer.cpp base-serialize.cpp base-shellescape.cpp - base-stacktrace.cpp base-stream.cpp base-string.cpp base-timer.cpp @@ -90,7 +89,6 @@ add_boost_test(base base_serialize/object base_shellescape/escape_basic base_shellescape/escape_quoted - base_stacktrace/stacktrace base_stream/readline_stdio base_string/construct base_string/equal diff --git a/test/base-stacktrace.cpp b/test/base-stacktrace.cpp deleted file mode 100644 index 349c2364a..000000000 --- a/test/base-stacktrace.cpp +++ /dev/null @@ -1,18 +0,0 @@ -/* Icinga 2 | (c) 2012 Icinga GmbH | GPLv2+ */ - -#include "base/stacktrace.hpp" -#include - -using namespace icinga; - -BOOST_AUTO_TEST_SUITE(base_stacktrace) - -BOOST_AUTO_TEST_CASE(stacktrace) -{ - StackTrace st; - std::ostringstream obuf; - obuf << st; - BOOST_CHECK(obuf.str().size() > 0); -} - -BOOST_AUTO_TEST_SUITE_END() From 15a16b23d9b6bb630bd3c79b64ec023426d1f1de Mon Sep 17 00:00:00 2001 From: Julian Brost Date: Tue, 20 Oct 2020 11:18:34 +0200 Subject: [PATCH 06/19] Pass fallback stacktrace to DiagnosticInformation in terminate handler By default, DiagnosticInformation uses the stack trace saved when the exception was thrown, but this mechanism is not in use on Windows. Gathering a stacktrace in the terminate handler serves as a fallback. --- lib/base/application.cpp | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/lib/base/application.cpp b/lib/base/application.cpp index 78af235cb..85f0b5e81 100644 --- a/lib/base/application.cpp +++ b/lib/base/application.cpp @@ -877,8 +877,19 @@ void Application::ExceptionHandler() << "\n" << "Additional information is available in '" << fname << "'" << "\n"; + /* On platforms where HAVE_CXXABI_H is defined, we prefer to print the stack trace that was saved + * when the last exception was thrown. Everywhere else, we do not have this information so we + * collect a stack trace here, which might lack some information, for example when an exception + * is rethrown, but this is still better than nothing. + */ + boost::stacktrace::stacktrace *stack = nullptr; +#ifndef HAVE_CXXABI_H + boost::stacktrace::stacktrace local_stack; + stack = &local_stack; +#endif /* HAVE_CXXABI_H */ + ofs << "\n" - << DiagnosticInformation(ex) + << DiagnosticInformation(ex, true, stack) << "\n"; } From 7f2868ab36df257b396847ada100dd466b571069 Mon Sep 17 00:00:00 2001 From: Julian Brost Date: Tue, 20 Oct 2020 11:21:30 +0200 Subject: [PATCH 07/19] Add documentation for cast_exception function --- lib/base/exception.cpp | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/lib/base/exception.cpp b/lib/base/exception.cpp index a080fdcba..01ae07369 100644 --- a/lib/base/exception.cpp +++ b/lib/base/exception.cpp @@ -33,6 +33,14 @@ public: #if defined(__GLIBCXX__) || defined(_LIBCPPABI_VERSION) +/** + * Attempts to cast an exception to a destination type + * + * @param obj Exception to be casted + * @param src Type information of obj + * @param dst Information of which type to cast to + * @return Pointer to the exception if the cast is possible, nullptr otherwise + */ inline void *cast_exception(void *obj, const std::type_info *src, const std::type_info *dst) { #ifdef __GLIBCXX__ From e60dd5128473aeb1a503c47b547c51d648d638f3 Mon Sep 17 00:00:00 2001 From: Julian Brost Date: Tue, 20 Oct 2020 11:22:20 +0200 Subject: [PATCH 08/19] Restructure stack and context trace selection in DiagnosticInformation and document behavior The logic for selecting the traces to print stays the same, but there are fewer nested ifs now. This changes the format of the returned string a bit by adding a heading for both traces. --- lib/base/exception.cpp | 45 ++++++++++++++++++++++-------------------- lib/base/exception.hpp | 19 ++++++++++++++++++ 2 files changed, 43 insertions(+), 21 deletions(-) diff --git a/lib/base/exception.cpp b/lib/base/exception.cpp index 01ae07369..fa301fe4f 100644 --- a/lib/base/exception.cpp +++ b/lib/base/exception.cpp @@ -224,34 +224,37 @@ String icinga::DiagnosticInformation(const std::exception& ex, bool verbose, boo const auto *pex = dynamic_cast(&ex); if (!uex && !pex && verbose) { + // Print the first of the following stack traces (if any exists) + // 1. stack trace from boost exception error information const boost::stacktrace::stacktrace *st = boost::get_error_info(ex); + // 2. stack trace explicitly passed as a parameter + if (!st) { + st = stack; + } + // 3. stack trace saved when the last exception was thrown + if (!st) { + st = GetLastExceptionStack(); + } - if (st) { - result << *st; - } else { - result << std::endl; - - if (!stack) - stack = GetLastExceptionStack(); - - if (stack) - result << *stack; - + if (st && !st->empty()) { + result << "\nStacktrace:\n" << *st; } } + // Print the first of the following context traces (if any exists) + // 1. context trace from boost exception error information const ContextTrace *ct = boost::get_error_info(ex); + // 2. context trace explicitly passed as a parameter + if (!ct) { + ct = context; + } + // 3. context trace saved when the last exception was thrown + if (!ct) { + ct = GetLastExceptionContext(); + } - if (ct) { - result << *ct; - } else { - result << std::endl; - - if (!context) - context = GetLastExceptionContext(); - - if (context) - result << *context; + if (ct && ct->GetLength() > 0) { + result << "\nContext:\n" << *ct; } return result.str(); diff --git a/lib/base/exception.hpp b/lib/base/exception.hpp index 1a4b71fcf..c18bd7abe 100644 --- a/lib/base/exception.hpp +++ b/lib/base/exception.hpp @@ -93,6 +93,25 @@ typedef boost::error_info ContextTraceErrorInfo; std::string to_string(const ContextTraceErrorInfo& e); +/** + * Generate diagnostic information about an exception + * + * The following information is gathered in the result: + * - Exception error message + * - Debug information about the Icinga config if the exception is a ValidationError + * - Stack trace + * - Context trace + * + * Each, stack trace and the context trace, are printed if the they were saved in the boost exception error + * information, are explicitly passed as a parameter, or were stored when the last exception was thrown. If multiple + * of these exist, the first one is used. + * + * @param ex exception to print diagnostic information about + * @param verbose if verbose is set, a stack trace is added + * @param stack optionally supply a stack trace + * @param context optionally supply a context trace + * @return string containing the aforementioned information + */ String DiagnosticInformation(const std::exception& ex, bool verbose = true, boost::stacktrace::stacktrace *stack = nullptr, ContextTrace *context = nullptr); String DiagnosticInformation(const boost::exception_ptr& eptr, bool verbose = true); From 1ad233da8680c7f974d1119c1a59768ed8dcc2bc Mon Sep 17 00:00:00 2001 From: Julian Brost Date: Tue, 20 Oct 2020 14:02:42 +0200 Subject: [PATCH 09/19] Add some comments to __cxa_throw Maybe this will save the next person who has to look at this code some time. Please don't blame me for the implementation, I'm just trying to reconstruct what it does. --- lib/base/exception.cpp | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/lib/base/exception.cpp b/lib/base/exception.cpp index fa301fe4f..17921a2f6 100644 --- a/lib/base/exception.cpp +++ b/lib/base/exception.cpp @@ -101,6 +101,13 @@ void icinga::RethrowUncaughtException() extern "C" void __cxa_throw(void *obj, TYPEINFO_TYPE *pvtinfo, void (*dest)(void *)) { + /* This function overrides an internal function of libstdc++ that is called when a C++ exception is thrown in order + * to capture as much information as possible at that time and then call the original implementation. This + * information includes: + * - stack trace (for later use in DiagnosticInformation) + * - context trace (for later use in DiagnosticInformation) + */ + auto *tinfo = static_cast(pvtinfo); typedef void (*cxa_throw_fn)(void *, std::type_info *, void (*)(void *)) __attribute__((noreturn)); @@ -112,6 +119,7 @@ void __cxa_throw(void *obj, TYPEINFO_TYPE *pvtinfo, void (*dest)(void *)) l_LastExceptionDest.reset(new DestCallback(dest)); #endif /* !defined(__GLIBCXX__) && !defined(_WIN32) */ + // resolve symbol to original implementation of __cxa_throw for the call at the end of this function if (real_cxa_throw == nullptr) real_cxa_throw = (cxa_throw_fn)dlsym(RTLD_NEXT, "__cxa_throw"); @@ -121,10 +129,12 @@ void __cxa_throw(void *obj, TYPEINFO_TYPE *pvtinfo, void (*dest)(void *)) if (!uex) { #endif /* NO_CAST_EXCEPTION */ + // save the current stack trace in a thread-local variable boost::stacktrace::stacktrace stack; SetLastExceptionStack(stack); #ifndef NO_CAST_EXCEPTION + // save the current stack trace in the boost exception error info if the exception is a boost::exception if (ex && !boost::get_error_info(*ex)) *ex << StackTraceErrorInfo(stack); } @@ -134,6 +144,7 @@ void __cxa_throw(void *obj, TYPEINFO_TYPE *pvtinfo, void (*dest)(void *)) SetLastExceptionContext(context); #ifndef NO_CAST_EXCEPTION + // save the current context trace in the boost exception error info if the exception is a boost::exception if (ex && !boost::get_error_info(*ex)) *ex << ContextTraceErrorInfo(context); #endif /* NO_CAST_EXCEPTION */ From 532d37624fa19ddddd343f03ae9145de7b8b6275 Mon Sep 17 00:00:00 2001 From: Julian Brost Date: Wed, 21 Oct 2020 12:22:18 +0200 Subject: [PATCH 10/19] docs: mention use of boost::stacktrace --- doc/21-development.md | 1 + 1 file changed, 1 insertion(+) diff --git a/doc/21-development.md b/doc/21-development.md index d89947f61..8fd877c9e 100644 --- a/doc/21-development.md +++ b/doc/21-development.md @@ -1153,6 +1153,7 @@ General: - [function_types](https://www.boost.org/doc/libs/1_66_0/libs/function_types/doc/html/index.html) (header only) - [circular_buffer](https://www.boost.org/doc/libs/1_66_0/doc/html/circular_buffer.html) (header only) - [math](https://www.boost.org/doc/libs/1_66_0/libs/math/doc/html/index.html) (header only) +- [stacktrace](https://www.boost.org/doc/libs/1_66_0/doc/html/stacktrace.html) (header only) Events and Runtime: From f084cdecbeb3f033aecb28b969c831ba4b6e7688 Mon Sep 17 00:00:00 2001 From: Julian Brost Date: Wed, 21 Oct 2020 12:49:50 +0200 Subject: [PATCH 11/19] Windows: require at least MSVC 19.20 to build Older versions of MSVC fail to rethrow an unhandled C++ exception (using `throw;`) in the termination handler (`std::set_terminate`), however Icinga relies on this behavior in its crash handler (`Application::ExceptionHandler`). --- CMakeLists.txt | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CMakeLists.txt b/CMakeLists.txt index 42102a99a..3d0dc7881 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -378,6 +378,12 @@ if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU") endif() endif() +if(MSVC) + if("${CMAKE_CXX_COMPILER_VERSION}" VERSION_LESS "19.20") + message(FATAL_ERROR "Your version of MSVC (${CMAKE_CXX_COMPILER_VERSION}) is too old for building Icinga 2 (MSVC >= 19.20 from Visual Studio 2019 is required).") + endif() +endif() + if(NOT MSVC) set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++0x") From 710cd7287f2ce3a9ee62c0c0a7df5df8e40497c5 Mon Sep 17 00:00:00 2001 From: Julian Brost Date: Wed, 21 Oct 2020 16:21:14 +0200 Subject: [PATCH 12/19] Begin crash log for SIGABRT with error message and timestamp This makes the format more similar to what the uncaught C++ and SEH exception handlers write. Previously there was no indication in the crash log that a SIGABRT happened. --- lib/base/application.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lib/base/application.cpp b/lib/base/application.cpp index 85f0b5e81..aa2bc1b2a 100644 --- a/lib/base/application.cpp +++ b/lib/base/application.cpp @@ -758,6 +758,10 @@ void Application::SigAbrtHandler(int) Log(LogCritical, "Application") << "Icinga 2 has terminated unexpectedly. Additional information can be found in '" << fname << "'" << "\n"; + ofs << "Caught SIGABRT." << "\n" + << "Current time: " << Utility::FormatDateTime("%Y-%m-%d %H:%M:%S %z", Utility::GetTime()) << "\n" + << "\n"; + DisplayInfoMessage(ofs); ofs << "\nStacktrace:\n" << boost::stacktrace::stacktrace() << "\n"; From 7d3885d05c50fdd5f3af946bcfc9c681e4f32389 Mon Sep 17 00:00:00 2001 From: Julian Brost Date: Thu, 22 Oct 2020 10:35:09 +0200 Subject: [PATCH 13/19] Crash handlers: use more compact string representation --- lib/base/application.cpp | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/lib/base/application.cpp b/lib/base/application.cpp index aa2bc1b2a..06265ec0e 100644 --- a/lib/base/application.cpp +++ b/lib/base/application.cpp @@ -758,9 +758,8 @@ void Application::SigAbrtHandler(int) Log(LogCritical, "Application") << "Icinga 2 has terminated unexpectedly. Additional information can be found in '" << fname << "'" << "\n"; - ofs << "Caught SIGABRT." << "\n" - << "Current time: " << Utility::FormatDateTime("%Y-%m-%d %H:%M:%S %z", Utility::GetTime()) << "\n" - << "\n"; + ofs << "Caught SIGABRT.\n" + << "Current time: " << Utility::FormatDateTime("%Y-%m-%d %H:%M:%S %z", Utility::GetTime()) << "\n\n"; DisplayInfoMessage(ofs); @@ -867,9 +866,8 @@ void Application::ExceptionHandler() std::ofstream ofs; ofs.open(fname.CStr()); - ofs << "Caught unhandled exception." << "\n" - << "Current time: " << Utility::FormatDateTime("%Y-%m-%d %H:%M:%S %z", Utility::GetTime()) << "\n" - << "\n"; + ofs << "Caught unhandled exception.\n" + << "Current time: " << Utility::FormatDateTime("%Y-%m-%d %H:%M:%S %z", Utility::GetTime()) << "\n\n"; DisplayInfoMessage(ofs); @@ -943,9 +941,8 @@ LONG CALLBACK Application::SEHUnhandledExceptionFilter(PEXCEPTION_POINTERS exi) Log(LogCritical, "Application") << "Icinga 2 has terminated unexpectedly. Additional information can be found in '" << fname << "'"; - ofs << "Caught unhandled SEH exception." << "\n" - << "Current time: " << Utility::FormatDateTime("%Y-%m-%d %H:%M:%S %z", Utility::GetTime()) << "\n" - << "\n"; + ofs << "Caught unhandled SEH exception.\n" + << "Current time: " << Utility::FormatDateTime("%Y-%m-%d %H:%M:%S %z", Utility::GetTime()) << "\n\n"; DisplayInfoMessage(ofs); From b931194f59b431c804eb0e0c3acf4960d123d301 Mon Sep 17 00:00:00 2001 From: Julian Brost Date: Fri, 23 Oct 2020 13:08:43 +0200 Subject: [PATCH 14/19] Use backtrace_symbols() when printing stack traces on FreeBSD Unfortunately, the symbol resolution of boost::stacktrace is broken on FreeBSD, therefore fall back to using backtrace_symbols() to print the stack trace saved by Boost. Additionally, -D_GNU_SOURCE is required on FreeBSD for the _Unwind_Backtrace function used by boost::stacktrace. --- CMakeLists.txt | 9 +++++++++ config.h.cmake | 1 + lib/base/CMakeLists.txt | 1 + lib/base/application.cpp | 5 +++-- lib/base/exception.cpp | 3 ++- lib/base/stacktrace.cpp | 36 ++++++++++++++++++++++++++++++++++++ lib/base/stacktrace.hpp | 25 +++++++++++++++++++++++++ 7 files changed, 77 insertions(+), 3 deletions(-) create mode 100644 lib/base/stacktrace.cpp create mode 100644 lib/base/stacktrace.hpp diff --git a/CMakeLists.txt b/CMakeLists.txt index 3d0dc7881..9229656df 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -367,6 +367,15 @@ if(HAVE_LIBEXECINFO) set(HAVE_BACKTRACE_SYMBOLS TRUE) endif() +if(CMAKE_SYSTEM_NAME STREQUAL "FreeBSD") + set(ICINGA2_STACKTRACE_USE_BACKTRACE_SYMBOLS TRUE) + add_definitions(-D_GNU_SOURCE) +endif() + +if(ICINGA2_STACKTRACE_USE_BACKTRACE_SYMBOLS AND NOT HAVE_BACKTRACE_SYMBOLS) + message(FATAL_ERROR "ICINGA2_STACKTRACE_USE_BACKTRACE_SYMBOLS is set but backtrace_symbols() was not found") +endif() + if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU") exec_program(${CMAKE_CXX_COMPILER} ARGS -dumpversion diff --git a/config.h.cmake b/config.h.cmake index 16fa190f1..3ed2ae46d 100644 --- a/config.h.cmake +++ b/config.h.cmake @@ -12,6 +12,7 @@ #cmakedefine HAVE_SYSTEMD #cmakedefine ICINGA2_UNITY_BUILD +#cmakedefine ICINGA2_STACKTRACE_USE_BACKTRACE_SYMBOLS #define ICINGA_CONFIGDIR "${ICINGA2_FULL_CONFIGDIR}" #define ICINGA_DATADIR "${ICINGA2_FULL_DATADIR}" diff --git a/lib/base/CMakeLists.txt b/lib/base/CMakeLists.txt index ed8576b56..108ca27c1 100644 --- a/lib/base/CMakeLists.txt +++ b/lib/base/CMakeLists.txt @@ -64,6 +64,7 @@ set(base_SOURCES shared-object.hpp singleton.hpp socket.cpp socket.hpp + stacktrace.cpp stacktrace.hpp statsfunction.hpp stdiostream.cpp stdiostream.hpp stream.cpp stream.hpp diff --git a/lib/base/application.cpp b/lib/base/application.cpp index 06265ec0e..c70926c6d 100644 --- a/lib/base/application.cpp +++ b/lib/base/application.cpp @@ -2,6 +2,7 @@ #include "base/application.hpp" #include "base/application-ti.cpp" +#include "base/stacktrace.hpp" #include "base/timer.hpp" #include "base/logger.hpp" #include "base/exception.hpp" @@ -763,7 +764,7 @@ void Application::SigAbrtHandler(int) DisplayInfoMessage(ofs); - ofs << "\nStacktrace:\n" << boost::stacktrace::stacktrace() << "\n"; + ofs << "\nStacktrace:\n" << StackTraceFormatter(boost::stacktrace::stacktrace()) << "\n"; DisplayBugMessage(ofs); @@ -954,7 +955,7 @@ LONG CALLBACK Application::SEHUnhandledExceptionFilter(PEXCEPTION_POINTERS exi) << " Flags: " << exi->ExceptionRecord->ExceptionFlags << "\n"; ofs.flags(savedflags); - ofs << "\nStacktrace:\n" << boost::stacktrace::stacktrace() << "\n"; + ofs << "\nStacktrace:\n" << StackTraceFormatter(boost::stacktrace::stacktrace()) << "\n"; DisplayBugMessage(ofs); diff --git a/lib/base/exception.cpp b/lib/base/exception.cpp index 17921a2f6..a194dafd5 100644 --- a/lib/base/exception.cpp +++ b/lib/base/exception.cpp @@ -1,6 +1,7 @@ /* Icinga 2 | (c) 2012 Icinga GmbH | GPLv2+ */ #include "base/exception.hpp" +#include "base/stacktrace.hpp" #include #include @@ -248,7 +249,7 @@ String icinga::DiagnosticInformation(const std::exception& ex, bool verbose, boo } if (st && !st->empty()) { - result << "\nStacktrace:\n" << *st; + result << "\nStacktrace:\n" << StackTraceFormatter(*st); } } diff --git a/lib/base/stacktrace.cpp b/lib/base/stacktrace.cpp new file mode 100644 index 000000000..01e3988bb --- /dev/null +++ b/lib/base/stacktrace.cpp @@ -0,0 +1,36 @@ +/* Icinga 2 | (c) 2020 Icinga GmbH | GPLv2+ */ + +#include +#include "base/stacktrace.hpp" +#include +#include +#include + +#ifdef HAVE_BACKTRACE_SYMBOLS +# include +#endif /* HAVE_BACKTRACE_SYMBOLS */ + +using namespace icinga; + +std::ostream &icinga::operator<<(std::ostream &os, const StackTraceFormatter &f) +{ + const boost::stacktrace::stacktrace &stack = f.m_Stack; + +#ifdef ICINGA2_STACKTRACE_USE_BACKTRACE_SYMBOLS + std::vector addrs; + addrs.reserve(stack.size()); + std::transform(stack.begin(), stack.end(), std::back_inserter(addrs), [](const boost::stacktrace::frame &f) { + return const_cast(f.address()); + }); + + char **symbols = backtrace_symbols(addrs.data(), addrs.size()); + for (size_t i = 0; i < addrs.size(); i++) { + os << std::setw(2) << i << "# " << symbols[i] << std::endl; + } + std::free(symbols); +#else /* ICINGA2_STACKTRACE_USE_BACKTRACE_SYMBOLS */ + os << stack; +#endif /* ICINGA2_STACKTRACE_USE_BACKTRACE_SYMBOLS */ + + return os; +} diff --git a/lib/base/stacktrace.hpp b/lib/base/stacktrace.hpp new file mode 100644 index 000000000..53a9b898b --- /dev/null +++ b/lib/base/stacktrace.hpp @@ -0,0 +1,25 @@ +/* Icinga 2 | (c) 2020 Icinga GmbH | GPLv2+ */ + +#ifndef STACKTRACE_H +#define STACKTRACE_H + +#include + +namespace icinga +{ + +class StackTraceFormatter { +public: + StackTraceFormatter(const boost::stacktrace::stacktrace &stack) : m_Stack(stack) {} + +private: + const boost::stacktrace::stacktrace &m_Stack; + + friend std::ostream &operator<<(std::ostream &os, const StackTraceFormatter &f); +}; + +std::ostream& operator<<(std::ostream& os, const StackTraceFormatter &f); + +} + +#endif /* STACKTRACE_H */ From 24f6283362486eaa74cb654942897011498701fb Mon Sep 17 00:00:00 2001 From: Julian Brost Date: Fri, 23 Oct 2020 15:04:28 +0200 Subject: [PATCH 15/19] Add a test case for the stack trace formatter --- test/CMakeLists.txt | 2 ++ test/base-stacktrace.cpp | 44 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+) create mode 100644 test/base-stacktrace.cpp diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 274456d1c..3bcf04319 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -16,6 +16,7 @@ set(base_test_SOURCES base-object-packer.cpp base-serialize.cpp base-shellescape.cpp + base-stacktrace.cpp base-stream.cpp base-string.cpp base-timer.cpp @@ -89,6 +90,7 @@ add_boost_test(base base_serialize/object base_shellescape/escape_basic base_shellescape/escape_quoted + base_stacktrace/stacktrace base_stream/readline_stdio base_string/construct base_string/equal diff --git a/test/base-stacktrace.cpp b/test/base-stacktrace.cpp new file mode 100644 index 000000000..0a8a04adf --- /dev/null +++ b/test/base-stacktrace.cpp @@ -0,0 +1,44 @@ +/* Icinga 2 | (c) 2020 Icinga GmbH | GPLv2+ */ + +#include "base/stacktrace.hpp" +#include + +using namespace icinga; + + +#pragma GCC push_options +#pragma GCC optimize ("O0") +#pragma clang optimize off + +BOOST_AUTO_TEST_SUITE(base_stacktrace) + +[[gnu::noinline]] +void stack_test_func_b() +{ + boost::stacktrace::stacktrace stack; + std::ostringstream obuf; + obuf << StackTraceFormatter(stack); + std::string result = obuf.str(); + BOOST_CHECK_MESSAGE(!result.empty(), "stack trace must not be empty"); + size_t pos_a = result.find("stack_test_func_a"); + size_t pos_b = result.find("stack_test_func_b"); + BOOST_CHECK_MESSAGE(pos_a != std::string::npos, "'stack_test_func_a' not found\n\n" << result); + BOOST_CHECK_MESSAGE(pos_b != std::string::npos, "'stack_test_func_b' not found\n\n" << result); + BOOST_CHECK_MESSAGE(pos_a > pos_b, "'stack_test_func_a' must appear after 'stack_test_func_b'\n\n" << result); +} + +[[gnu::noinline]] +void stack_test_func_a() +{ + stack_test_func_b(); +} + +BOOST_AUTO_TEST_CASE(stacktrace) +{ + stack_test_func_a(); +} + +BOOST_AUTO_TEST_SUITE_END() + +#pragma GCC pop_options +#pragma clang optimize on From cdfcef4d634ece78a4829e2b3a1cfee180353f56 Mon Sep 17 00:00:00 2001 From: Julian Brost Date: Tue, 27 Oct 2020 15:31:39 +0100 Subject: [PATCH 16/19] CMakeLists: use HAVE_LIBEXECINFO only after actually checking for it So far, the check that actually sets HAVE_LIBEXECINFO was executed after it was already used to add dependencies. --- CMakeLists.txt | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 9229656df..1f66e58db 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -203,10 +203,6 @@ include_directories( ${CMAKE_CURRENT_BINARY_DIR} ${CMAKE_CURRENT_BINARY_DIR}/lib ) -if(HAVE_LIBEXECINFO) - list(APPEND base_DEPS execinfo) -endif() - if(UNIX OR CYGWIN) list(APPEND base_OBJS $) endif() @@ -365,6 +361,7 @@ check_include_file_cxx(cxxabi.h HAVE_CXXABI_H) if(HAVE_LIBEXECINFO) set(HAVE_BACKTRACE_SYMBOLS TRUE) + list(APPEND base_DEPS execinfo) endif() if(CMAKE_SYSTEM_NAME STREQUAL "FreeBSD") From 3ad9d3316cdcbe3e4d7a2c8648a738661ea10639 Mon Sep 17 00:00:00 2001 From: Julian Brost Date: Wed, 28 Oct 2020 10:26:39 +0100 Subject: [PATCH 17/19] Compile with -D_GNU_SOURCE Needed by `boost::stacktrace` for `_Unwind_Backtrace()`. --- CMakeLists.txt | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 1f66e58db..1bc456006 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -364,9 +364,13 @@ if(HAVE_LIBEXECINFO) list(APPEND base_DEPS execinfo) endif() +if(NOT WIN32) + # boost::stacktrace uses _Unwind_Backtrace which is only exposed if _GNU_SOURCE is defined on most systems + add_definitions(-D_GNU_SOURCE) +endif() + if(CMAKE_SYSTEM_NAME STREQUAL "FreeBSD") set(ICINGA2_STACKTRACE_USE_BACKTRACE_SYMBOLS TRUE) - add_definitions(-D_GNU_SOURCE) endif() if(ICINGA2_STACKTRACE_USE_BACKTRACE_SYMBOLS AND NOT HAVE_BACKTRACE_SYMBOLS) From 1742e3122549410620696b7ec49395d1a12a7b6e Mon Sep 17 00:00:00 2001 From: Julian Brost Date: Wed, 28 Oct 2020 10:46:18 +0100 Subject: [PATCH 18/19] Add comments to stack trace formatter and test case --- lib/base/stacktrace.cpp | 7 +++++++ lib/base/stacktrace.hpp | 6 ++++++ test/base-stacktrace.cpp | 14 ++++++++++++++ 3 files changed, 27 insertions(+) diff --git a/lib/base/stacktrace.cpp b/lib/base/stacktrace.cpp index 01e3988bb..e3f15ceb7 100644 --- a/lib/base/stacktrace.cpp +++ b/lib/base/stacktrace.cpp @@ -14,6 +14,13 @@ using namespace icinga; std::ostream &icinga::operator<<(std::ostream &os, const StackTraceFormatter &f) { + /* In most cases, this operator<< just relies on the operator<< for the `boost::stacktrace::stacktrace` wrapped in + * the `StackTraceFormatter`. But as this operator turned out to not work properly on some platforms, there is a + * fallback implementation that can be enabled using the `-DICINGA2_STACKTRACE_USE_BACKTRACE_SYMBOLS` flag at + * compile time. This will then switch to `backtrace_symbols()` from `` instead of the implementation + * provided by Boost. + */ + const boost::stacktrace::stacktrace &stack = f.m_Stack; #ifdef ICINGA2_STACKTRACE_USE_BACKTRACE_SYMBOLS diff --git a/lib/base/stacktrace.hpp b/lib/base/stacktrace.hpp index 53a9b898b..b4a9765f9 100644 --- a/lib/base/stacktrace.hpp +++ b/lib/base/stacktrace.hpp @@ -8,6 +8,12 @@ namespace icinga { +/** + * Formatter for `boost::stacktrace::stacktrace` objects + * + * This class wraps `boost::stacktrace::stacktrace` objects and provides an operator<< + * for printing them to an `std::ostream` in a custom format. + */ class StackTraceFormatter { public: StackTraceFormatter(const boost::stacktrace::stacktrace &stack) : m_Stack(stack) {} diff --git a/test/base-stacktrace.cpp b/test/base-stacktrace.cpp index 0a8a04adf..34400a2a1 100644 --- a/test/base-stacktrace.cpp +++ b/test/base-stacktrace.cpp @@ -6,6 +6,20 @@ using namespace icinga; +/* If you are reading this, you are probably doing so because this test case just failed. This might happen as it + * heavily depends on platform and compiler behavior. There are two likely causes why this could break: + * + * - Your compiler found new ways to optimize the functions that are called to create a stack, even though we tried + * to disable optimizations using #pragmas for some compilers. If you know a way to disable (more) optimizations for + * your compiler, you can try if this helps. + * + * - Boost fails to resolve symbol names as we've already seen on some platforms. In this case, you can try again + * passing the additional flag `-DICINGA2_STACKTRACE_USE_BACKTRACE_SYMBOLS=ON` to CMake and see if this helps. + * + * In any case, please report a bug. If you run `make CTEST_OUTPUT_ON_FAILURE=1 test`, the stack trace in question + * should be printed. If it looks somewhat meaningful, you can probably ignore a failure of this test case. + */ + #pragma GCC push_options #pragma GCC optimize ("O0") #pragma clang optimize off From c5626cb79340be3aba80a63cdf341879ee09eb09 Mon Sep 17 00:00:00 2001 From: Julian Brost Date: Mon, 1 Mar 2021 09:00:40 +0100 Subject: [PATCH 19/19] Stacktrace test: try to prevent inlining even harder - Explicitly disable optimizations for MSVC - Make stack_test_func_a bigger --- test/base-stacktrace.cpp | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/test/base-stacktrace.cpp b/test/base-stacktrace.cpp index 34400a2a1..f0b87e2cf 100644 --- a/test/base-stacktrace.cpp +++ b/test/base-stacktrace.cpp @@ -23,6 +23,9 @@ using namespace icinga; #pragma GCC push_options #pragma GCC optimize ("O0") #pragma clang optimize off +#ifdef _MSVC_VER +#pragma optimize("", off) +#endif /* _MSVC_VER */ BOOST_AUTO_TEST_SUITE(base_stacktrace) @@ -44,6 +47,14 @@ void stack_test_func_b() [[gnu::noinline]] void stack_test_func_a() { + boost::stacktrace::stacktrace stack; + std::ostringstream obuf; + obuf << StackTraceFormatter(stack); + std::string result = obuf.str(); + BOOST_CHECK_MESSAGE(!result.empty(), "stack trace must not be empty"); + size_t pos_a = result.find("stack_test_func_a"); + BOOST_CHECK_MESSAGE(pos_a != std::string::npos, "'stack_test_func_a' not found\n\n" << result); + stack_test_func_b(); } @@ -56,3 +67,6 @@ BOOST_AUTO_TEST_SUITE_END() #pragma GCC pop_options #pragma clang optimize on +#ifdef _MSVC_VER +#pragma optimize("", on) +#endif /* _MSVC_VER */