From b5017b78d79e67a1ead09d8d37d36236b456b34f 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 9ebd812da5d6c7228f25474cc2850a9d0ade8e70 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 8b67e4a637d31e43532ec6e018138ee252df43a5 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 4a29c39ebac77188402f3bd821a444a7efc40bd5 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 8b2f4636db81b4a6093b9e7718461f72bc5fb78a 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 c694b6bf2..025da3315 100644 --- a/lib/base/exception.cpp +++ b/lib/base/exception.cpp @@ -13,7 +13,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 @@ -112,7 +112,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 @@ -133,14 +133,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() @@ -153,7 +153,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; @@ -215,7 +215,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; @@ -250,8 +250,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 246c28a78..10e2b4d0b 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 7da1a0c54..658ff447c 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 996f280bfccf65432b39f5192cae70b8dbdd2b70 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 df59aa0087b819fada3686d2ef24ffed43bdfe46 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 025da3315..5ab24cec0 100644 --- a/lib/base/exception.cpp +++ b/lib/base/exception.cpp @@ -32,6 +32,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 9fcc7811726f19c5d4368d208ed605d0ee51a3f0 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 5ab24cec0..c6e485483 100644 --- a/lib/base/exception.cpp +++ b/lib/base/exception.cpp @@ -223,34 +223,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 10e2b4d0b..279b1d036 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 2330ab59f82ea57eab6dea2a18f0b75cb5397a0c 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 c6e485483..ebeba420d 100644 --- a/lib/base/exception.cpp +++ b/lib/base/exception.cpp @@ -100,6 +100,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)); @@ -111,6 +118,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"); @@ -120,10 +128,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); } @@ -133,6 +143,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 51bb751f23ad573861d8500fbb76abf54cb5b899 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 504543652..034f054ff 100644 --- a/doc/21-development.md +++ b/doc/21-development.md @@ -1151,6 +1151,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 cb3febfd2ad519b065feffdde1404fd99ad47582 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 46e765415..3bb6e0536 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 2310cdb4fa4cc84bcc7fd3ec691ae1ed922286e8 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 a2e5cfd34f76975de5b0e6c1bb623df299cae3fb 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 f5873a8e75b7f2ff1997bb6f4d16089f23c2326b 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 3bb6e0536..23108ce6b 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 ebeba420d..2d5331f01 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 #ifdef _WIN32 @@ -247,7 +248,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 14987274dcea8516ba930c0dfbd671454250d8a0 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 658ff447c..7da1a0c54 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 1a82a9fa87fe2eec102d807f33aa6f281561b46c 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 23108ce6b..4cb04d491 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 c3f77be473b066c0b171b22f23f34902790d292e 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 4cb04d491..ad0f05204 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 d8ab328c33657d4ce3d08e8dc75e0da65360a85c 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 7bfe896efbe83902d1151e88d202753388d6121d 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 */