From c05ff26c86c77a0488a2acfbb5b68e02eab5a336 Mon Sep 17 00:00:00 2001 From: Benjamin Winger Date: Fri, 10 May 2024 15:23:42 -0400 Subject: [PATCH] Backtraces (#3456) * Print backtraces when exceptions are thrown Currently does so by printing them in the exception constructor, as otherwise we wouldn't be able to check for expected query result messages. Also adds a std::terminate handler to handle some unexpected exits which would otherwise not print traces * Add support for SIGSEGV backtraces (and other signals) * Ignore false positive GCC warning in relwithdebinfo mode (cherry picked from commit 4e1a9fd0fcf8c03f2062c3577525896133e95c70) --- CMakeLists.txt | 22 +++++++++++++ Makefile | 31 ++++++++++++++---- src/CMakeLists.txt | 3 ++ src/common/CMakeLists.txt | 1 + src/common/exception/CMakeLists.txt | 5 +++ src/common/exception/exception.cpp | 17 ++++++++++ src/common/signal/CMakeLists.txt | 4 +++ src/common/signal/register.cpp | 32 +++++++++++++++++++ src/include/common/exception/exception.h | 2 +- .../spdlog/spdlog/fmt/bundled/format.h | 8 +++++ tools/shell/CMakeLists.txt | 3 ++ 11 files changed, 120 insertions(+), 8 deletions(-) create mode 100644 src/common/exception/exception.cpp create mode 100644 src/common/signal/CMakeLists.txt create mode 100644 src/common/signal/register.cpp diff --git a/CMakeLists.txt b/CMakeLists.txt index 2e313d53805..38bf6c0b833 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -168,16 +168,35 @@ option(BUILD_SINGLE_FILE_HEADER "Build single file header. Requires Python >= 3. option(BUILD_TESTS "Build C++ tests." FALSE) option(BUILD_EXTENSION_TESTS "Build C++ extension tests." FALSE) option(BUILD_KUZU "Build Kuzu." TRUE) +option(ENABLE_BACKTRACES "Enable backtrace printing for exceptions and segfaults" FALSE) option(BUILD_LCOV "Build coverage report." FALSE) if(${BUILD_LCOV}) set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fprofile-arcs -ftest-coverage") endif() +if (ENABLE_BACKTRACES) + find_package(cpptrace) + if (NOT cpptrace_FOUND) + include(FetchContent) + FetchContent_Declare( + cpptrace + GIT_REPOSITORY https://github.com/jeremy-rifkin/cpptrace.git + GIT_TAG v0.5.4 + GIT_SHALLOW TRUE + ) + FetchContent_MakeAvailable(cpptrace) + endif() + add_compile_definitions(KUZU_BACKTRACE) +endif() + function(add_kuzu_test TEST_NAME) set(SRCS ${ARGN}) add_executable(${TEST_NAME} ${SRCS}) target_link_libraries(${TEST_NAME} PRIVATE test_helper test_runner graph_test) + if (ENABLE_BACKTRACES) + target_link_libraries(${TEST_NAME} PRIVATE register_backtrace_signal_handler) + endif() target_include_directories(${TEST_NAME} PRIVATE ${PROJECT_SOURCE_DIR}/test/include) include(GoogleTest) gtest_discover_tests(${TEST_NAME} DISCOVERY_TIMEOUT 600 DISCOVERY_MODE PRE_TEST) @@ -187,6 +206,9 @@ function(add_kuzu_api_test TEST_NAME) set(SRCS ${ARGN}) add_executable(${TEST_NAME} ${SRCS}) target_link_libraries(${TEST_NAME} PRIVATE api_graph_test api_test_helper) + if (ENABLE_BACKTRACES) + target_link_libraries(${TEST_NAME} PRIVATE register_backtrace_signal_handler) + endif() target_include_directories(${TEST_NAME} PRIVATE ${PROJECT_SOURCE_DIR}/test/include) include(GoogleTest) gtest_discover_tests(${TEST_NAME}) diff --git a/Makefile b/Makefile index b04cff76550..95fbac6b016 100644 --- a/Makefile +++ b/Makefile @@ -7,7 +7,7 @@ clangd tidy clangd-diagnostics \ install \ clean-extension clean-python-api clean-java clean \ - extension-test shell-test + extension-test shell-test .ONESHELL: .SHELLFLAGS = -ec @@ -57,6 +57,9 @@ endif release: $(call run-cmake-release,) +relwithdebinfo: + $(call run-cmake-relwithdebinfo,) + debug: $(call run-cmake-debug,) @@ -90,8 +93,8 @@ alldebug: # Main tests test: - $(call run-cmake-release, -DBUILD_TESTS=TRUE) - ctest --test-dir build/release/test --output-on-failure -j ${TEST_JOBS} + $(call run-cmake-relwithdebinfo, -DBUILD_TESTS=TRUE -DENABLE_BACKTRACES=TRUE) + ctest --test-dir build/relwithdebinfo/test --output-on-failure -j ${TEST_JOBS} lcov: $(call run-cmake-release, -DBUILD_TESTS=TRUE -DBUILD_LCOV=TRUE) @@ -167,12 +170,13 @@ example: $(call run-cmake-release, -DBUILD_EXAMPLES=TRUE) extension-test: - $(call run-cmake-release, \ + $(call run-cmake-relwithdebinfo, \ -DBUILD_EXTENSIONS="httpfs;duckdb;postgres" \ -DBUILD_EXTENSION_TESTS=TRUE \ -DENABLE_ADDRESS_SANITIZER=TRUE \ + -DENABLE_BACKTRACES=TRUE \ ) - ctest --test-dir build/release/extension --output-on-failure -j ${TEST_JOBS} + ctest --test-dir build/relwithdebinfo/extension --output-on-failure -j ${TEST_JOBS} aws s3 rm s3://kuzu-dataset-us/${RUN_ID}/ --recursive extension-debug: @@ -188,10 +192,10 @@ extension-release: ) shell-test: - $(call run-cmake-release, \ + $(call run-cmake-relwithdebinfo, \ -DBUILD_SHELL=TRUE \ ) - cd tools/shell/test && python3 -m pytest -v + cd tools/shell/test && python3 -m pytest -v # Clang-related tools and checks @@ -253,11 +257,24 @@ define build-cmake-release $(call build-cmake,release,Release,$1) endef +define build-cmake-relwithdebinfo + $(call build-cmake,relwithdebinfo,RelWithDebInfo,$1) +endef + define config-cmake-release $(call config-cmake,release,Release,$1) endef +define config-cmake-relwithdebinfo + $(call config-cmake,relwithdebinfo,RelWithDebInfo,$1) +endef + define run-cmake-release $(call config-cmake-release,$1) $(call build-cmake-release,$1) endef + +define run-cmake-relwithdebinfo + $(call config-cmake-relwithdebinfo,$1) + $(call build-cmake-relwithdebinfo,$1) +endef diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index ab227e5ddbe..8810225a4fd 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -25,6 +25,9 @@ set(KUZU_LIBRARIES antlr4_cypher antlr4_runtime brotlidec brotlicommon fast_floa if(NOT WIN32) set(KUZU_LIBRARIES dl ${KUZU_LIBRARIES}) endif() +if (ENABLE_BACKTRACES) + set(KUZU_LIBRARIES ${KUZU_LIBRARIES} cpptrace::cpptrace) +endif() target_link_libraries(kuzu PUBLIC ${KUZU_LIBRARIES}) target_link_libraries(kuzu_shared PUBLIC ${KUZU_LIBRARIES}) unset(KUZU_LIBRARIES) diff --git a/src/common/CMakeLists.txt b/src/common/CMakeLists.txt index 4635043a3de..b71e434bca1 100644 --- a/src/common/CMakeLists.txt +++ b/src/common/CMakeLists.txt @@ -4,6 +4,7 @@ add_subdirectory(data_chunk) add_subdirectory(enums) add_subdirectory(exception) add_subdirectory(serializer) +add_subdirectory(signal) add_subdirectory(task_system) add_subdirectory(types) add_subdirectory(vector) diff --git a/src/common/exception/CMakeLists.txt b/src/common/exception/CMakeLists.txt index 5302b85e56f..5ecbd2c42d1 100644 --- a/src/common/exception/CMakeLists.txt +++ b/src/common/exception/CMakeLists.txt @@ -1,7 +1,12 @@ add_library(kuzu_common_exception OBJECT + exception.cpp message.cpp) +if (ENABLE_BACKTRACES) + target_link_libraries(kuzu_common_exception PRIVATE cpptrace::cpptrace) +endif() + set(ALL_OBJECT_FILES ${ALL_OBJECT_FILES} $ PARENT_SCOPE) diff --git a/src/common/exception/exception.cpp b/src/common/exception/exception.cpp new file mode 100644 index 00000000000..df0ea853334 --- /dev/null +++ b/src/common/exception/exception.cpp @@ -0,0 +1,17 @@ +#include "common/exception/exception.h" + +#ifdef KUZU_BACKTRACE +#include +#endif + +namespace kuzu { +namespace common { + +Exception::Exception(std::string msg) : exception(), exception_message_(std::move(msg)) { +#ifdef KUZU_BACKTRACE + cpptrace::generate_trace(1 /*skip this function's frame*/).print(); +#endif +} + +} // namespace common +} // namespace kuzu diff --git a/src/common/signal/CMakeLists.txt b/src/common/signal/CMakeLists.txt new file mode 100644 index 00000000000..af256cc72ed --- /dev/null +++ b/src/common/signal/CMakeLists.txt @@ -0,0 +1,4 @@ +if (ENABLE_BACKTRACES) + add_library(register_backtrace_signal_handler OBJECT register.cpp) + target_link_libraries(register_backtrace_signal_handler cpptrace::cpptrace) +endif() diff --git a/src/common/signal/register.cpp b/src/common/signal/register.cpp new file mode 100644 index 00000000000..52604ed3523 --- /dev/null +++ b/src/common/signal/register.cpp @@ -0,0 +1,32 @@ +#ifdef KUZU_BACKTRACE +#include +#include +#include +#include + +#include + +namespace { + +void handler(int signo) { + // Not safe. Safe method would be writing directly to stderr with a pre-defined string + // But since the below isn't safe either... + std::cerr << "Fatal signal " << signo << std::endl; + // This is not safe, however the safe version, described at the link below, + // was causing hangs when the tracer program can't be found. + // Since this is only used in CI, the occasional failure/hang is probably acceptable. + // https://github.com/jeremy-rifkin/cpptrace/blob/main/docs/signal-safe-tracing.md + cpptrace::generate_trace(1 /*skip this function's frame*/).print(); + std::_Exit(1); +} + +int register_signal_handlers() noexcept { + std::signal(SIGSEGV, handler); + std::signal(SIGFPE, handler); + cpptrace::register_terminate_handler(); + return 0; +} + +static int ignore = register_signal_handlers(); +}; // namespace +#endif diff --git a/src/include/common/exception/exception.h b/src/include/common/exception/exception.h index cfaf4e19620..27436d73dcf 100644 --- a/src/include/common/exception/exception.h +++ b/src/include/common/exception/exception.h @@ -10,7 +10,7 @@ namespace common { class KUZU_API Exception : public std::exception { public: - explicit Exception(std::string msg) : exception(), exception_message_(std::move(msg)){}; + explicit Exception(std::string msg); public: const char* what() const noexcept override { return exception_message_.c_str(); } diff --git a/third_party/spdlog/spdlog/fmt/bundled/format.h b/third_party/spdlog/spdlog/fmt/bundled/format.h index 7c607dbd304..0b8a4bea1b5 100644 --- a/third_party/spdlog/spdlog/fmt/bundled/format.h +++ b/third_party/spdlog/spdlog/fmt/bundled/format.h @@ -922,8 +922,16 @@ FMT_CONSTEXPR20 void basic_memory_buffer::grow( T* new_data = std::allocator_traits::allocate(alloc_, new_capacity); // The following code doesn't throw, so the raw pointer above doesn't leak. + +// False positive warning when built in RelWithDebInfo mode +// see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109717 +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Warray-bounds" +#pragma clang diagnostic ignored "-Wunknown-warning-option" +#pragma GCC diagnostic ignored "-Wstringop-overflow" std::uninitialized_copy(old_data, old_data + this->size(), detail::make_checked(new_data, new_capacity)); +#pragma GCC diagnostic pop this->set(new_data, new_capacity); // deallocate must not throw according to the standard, but even if it does, // the buffer already uses the new storage and will deallocate it in diff --git a/tools/shell/CMakeLists.txt b/tools/shell/CMakeLists.txt index 5e37ac4c62c..6792fff781b 100644 --- a/tools/shell/CMakeLists.txt +++ b/tools/shell/CMakeLists.txt @@ -6,6 +6,9 @@ add_executable(kuzu_shell shell_runner.cpp) target_link_libraries(kuzu_shell kuzu) +if (ENABLE_BACKTRACES) + target_link_libraries(kuzu_shell register_backtrace_signal_handler) +endif() if (MSVC) target_link_libraries(kuzu_shell ws2_32) endif()