From ab69128e1ded73c8da0113958a61991b3e711229 Mon Sep 17 00:00:00 2001 From: Nikita Lapkov Date: Tue, 14 May 2019 22:15:23 +0000 Subject: [PATCH] introduce Backtrace class --- dbms/src/Common/QueryProfiler.h | 7 +- dbms/src/Functions/FunctionsIntrospection.h | 2 +- dbms/src/Interpreters/TraceCollector.cpp | 16 +-- libs/libcommon/CMakeLists.txt | 4 +- libs/libcommon/include/common/Backtrace.h | 25 ++++- libs/libcommon/src/Backtrace.cpp | 78 +++++--------- libs/libdaemon/CMakeLists.txt | 8 -- libs/libdaemon/src/BaseDaemon.cpp | 109 ++------------------ 8 files changed, 77 insertions(+), 172 deletions(-) diff --git a/dbms/src/Common/QueryProfiler.h b/dbms/src/Common/QueryProfiler.h index 3828bdc49e0..7c5e7b1aa61 100644 --- a/dbms/src/Common/QueryProfiler.h +++ b/dbms/src/Common/QueryProfiler.h @@ -1,5 +1,6 @@ #pragma once +#include #include #include #include @@ -9,6 +10,7 @@ #include #include + namespace Poco { class Logger; @@ -36,7 +38,10 @@ namespace const std::string & query_id = CurrentThread::getQueryId(); - DB::writePODBinary(*reinterpret_cast(context), out); + const auto signal_context = *reinterpret_cast(context); + const Backtrace backtrace(signal_context); + + DB::writePODBinary(backtrace, out); DB::writeStringBinary(query_id, out); DB::writeIntBinary(timer_type, out); out.next(); diff --git a/dbms/src/Functions/FunctionsIntrospection.h b/dbms/src/Functions/FunctionsIntrospection.h index fd2c53b5cbd..00bff12bfe5 100644 --- a/dbms/src/Functions/FunctionsIntrospection.h +++ b/dbms/src/Functions/FunctionsIntrospection.h @@ -92,7 +92,7 @@ public: { frames.push_back(reinterpret_cast(data[pos])); } - std::string backtrace = backtraceFramesToString(frames, "\n"); + std::string backtrace = Backtrace(frames).toString("\n"); result_column->insertDataWithTerminatingZero(backtrace.c_str(), backtrace.length() + 1); } diff --git a/dbms/src/Interpreters/TraceCollector.cpp b/dbms/src/Interpreters/TraceCollector.cpp index 5682b820834..915246384cc 100644 --- a/dbms/src/Interpreters/TraceCollector.cpp +++ b/dbms/src/Interpreters/TraceCollector.cpp @@ -26,16 +26,16 @@ namespace DB while (stop_future.wait_for(std::chrono::milliseconds(1)) == std::future_status::timeout) { - ucontext_t context; + Backtrace backtrace; std::string query_id; TimerType timer_type; try { - DB::readPODBinary(context, in); + DB::readPODBinary(backtrace, in); DB::readStringBinary(query_id, in); DB::readIntBinary(timer_type, in); } - catch (Exception&) + catch (...) { /// Pipe was closed - looks like server is about to shutdown /// Let us wait for stop_future @@ -44,11 +44,13 @@ namespace DB if (trace_log != nullptr) { - std::vector frames = getBacktraceFrames(context); + const auto size = backtrace.getSize(); + const auto& frames = backtrace.getFrames(); + std::vector trace; - trace.reserve(frames.size()); - for (void * frame : frames) - trace.push_back(reinterpret_cast(frame)); + trace.reserve(size); + for (size_t i = 0; i < size; i++) + trace.push_back(reinterpret_cast(frames[i])); TraceLogElement element{std::time(nullptr), timer_type, query_id, trace}; diff --git a/libs/libcommon/CMakeLists.txt b/libs/libcommon/CMakeLists.txt index f1a8fc2e153..a3fcd247bf6 100644 --- a/libs/libcommon/CMakeLists.txt +++ b/libs/libcommon/CMakeLists.txt @@ -64,7 +64,9 @@ add_library (common if (USE_UNWIND) target_compile_definitions (common PRIVATE USE_UNWIND=1) target_include_directories (common BEFORE PRIVATE ${UNWIND_INCLUDE_DIR}) - target_link_libraries (common PRIVATE ${UNWIND_LIBRARY}) + if (NOT USE_INTERNAL_UNWIND_LIBRARY_FOR_EXCEPTION_HANDLING) + target_link_libraries (common PRIVATE ${UNWIND_LIBRARY}) + endif () endif () # When testing for memory leaks with Valgrind, dont link tcmalloc or jemalloc. diff --git a/libs/libcommon/include/common/Backtrace.h b/libs/libcommon/include/common/Backtrace.h index afee870c860..6f19133dcd9 100644 --- a/libs/libcommon/include/common/Backtrace.h +++ b/libs/libcommon/include/common/Backtrace.h @@ -2,6 +2,7 @@ #include #include +#include #include #ifdef __APPLE__ @@ -11,10 +12,26 @@ #include -std::string signalToErrorMessage(int sig, siginfo_t & info, ucontext_t & context); +class Backtrace +{ +public: + static constexpr size_t capacity = 50; + using Frames = std::array; -void * getCallerAddress(ucontext_t & context); + Backtrace() = default; + Backtrace(const std::vector& sourceFrames); + Backtrace(const ucontext_t & signal_context); -std::vector getBacktraceFrames(ucontext_t & context); + size_t getSize() const; + const Frames& getFrames() const; + std::string toString(const std::string & delimiter = "") const; + +protected: + size_t size; + Frames frames; +}; + +std::string signalToErrorMessage(int sig, const siginfo_t & info, const ucontext_t & context); + +void * getCallerAddress(const ucontext_t & context); -std::string backtraceFramesToString(const std::vector & frames, const std::string & delimiter = ""); diff --git a/libs/libcommon/src/Backtrace.cpp b/libs/libcommon/src/Backtrace.cpp index 68a63c2063d..c72d75c4cbc 100644 --- a/libs/libcommon/src/Backtrace.cpp +++ b/libs/libcommon/src/Backtrace.cpp @@ -16,47 +16,9 @@ #if USE_UNWIND #define UNW_LOCAL_ONLY #include - -/** We suppress the following ASan report. Also shown by Valgrind. -==124==ERROR: AddressSanitizer: stack-use-after-scope on address 0x7f054be57000 at pc 0x0000068b0649 bp 0x7f060eeac590 sp 0x7f060eeabd40 -READ of size 1 at 0x7f054be57000 thread T3 - #0 0x68b0648 in write (/usr/bin/clickhouse+0x68b0648) - #1 0x717da02 in write_validate /build/obj-x86_64-linux-gnu/../contrib/libunwind/src/x86_64/Ginit.c:110:13 - #2 0x717da02 in mincore_validate /build/obj-x86_64-linux-gnu/../contrib/libunwind/src/x86_64/Ginit.c:146 - #3 0x717dec1 in validate_mem /build/obj-x86_64-linux-gnu/../contrib/libunwind/src/x86_64/Ginit.c:206:7 - #4 0x717dec1 in access_mem /build/obj-x86_64-linux-gnu/../contrib/libunwind/src/x86_64/Ginit.c:240 - #5 0x71881a9 in dwarf_get /build/obj-x86_64-linux-gnu/../contrib/libunwind/include/tdep-x86_64/libunwind_i.h:168:12 - #6 0x71881a9 in apply_reg_state /build/obj-x86_64-linux-gnu/../contrib/libunwind/src/dwarf/Gparser.c:872 - #7 0x718705c in _ULx86_64_dwarf_step /build/obj-x86_64-linux-gnu/../contrib/libunwind/src/dwarf/Gparser.c:953:10 - #8 0x718f155 in _ULx86_64_step /build/obj-x86_64-linux-gnu/../contrib/libunwind/src/x86_64/Gstep.c:71:9 - #9 0x7162671 in backtraceLibUnwind(void**, unsigned long, ucontext_t&) /build/obj-x86_64-linux-gnu/../libs/libdaemon/src/BaseDaemon.cpp:202:14 - */ -std::vector NO_SANITIZE_ADDRESS backtraceLibUnwind(size_t max_frames, ucontext_t & context) -{ - std::vector out_frames; - out_frames.reserve(max_frames); - unw_cursor_t cursor; - - if (unw_init_local2(&cursor, &context, UNW_INIT_SIGNAL_FRAME) >= 0) - { - for (size_t i = 0; i < max_frames; ++i) - { - unw_word_t ip; - unw_get_reg(&cursor, UNW_REG_IP, &ip); - out_frames.push_back(reinterpret_cast(ip)); - - /// NOTE This triggers "AddressSanitizer: stack-buffer-overflow". Looks like false positive. - /// It's Ok, because we use this method if the program is crashed nevertheless. - if (!unw_step(&cursor)) - break; - } - } - - return out_frames; -} #endif -std::string signalToErrorMessage(int sig, siginfo_t & info, ucontext_t & context) +std::string signalToErrorMessage(int sig, const siginfo_t & info, const ucontext_t & context) { std::stringstream error; switch (sig) @@ -199,7 +161,7 @@ std::string signalToErrorMessage(int sig, siginfo_t & info, ucontext_t & context return error.str(); } -void * getCallerAddress(ucontext_t & context) +void * getCallerAddress(const ucontext_t & context) { #if defined(__x86_64__) /// Get the address at the time the signal was raised from the RIP (x86-64) @@ -217,37 +179,49 @@ void * getCallerAddress(ucontext_t & context) return nullptr; } -std::vector getBacktraceFrames(ucontext_t & context) + +Backtrace::Backtrace(const ucontext_t & signal_context) { - std::vector frames; + size = 0; #if USE_UNWIND - static size_t max_frames = 50; - frames = backtraceLibUnwind(max_frames, context); + size = unw_backtrace(frames.data(), capacity); #else /// No libunwind means no backtrace, because we are in a different thread from the one where the signal happened. /// So at least print the function where the signal happened. - void * caller_address = getCallerAddress(context); + void * caller_address = getCallerAddress(signal_context); if (caller_address) - frames.push_back(caller_address); + frames[size++] = reinterpret_cast(caller_address); #endif +} +Backtrace::Backtrace(const std::vector& sourceFrames) { + for (size = 0; size < std::min(sourceFrames.size(), capacity); size++) + frames[size] = sourceFrames[size]; +} + +size_t Backtrace::getSize() const { + return size; +} + +const Backtrace::Frames& Backtrace::getFrames() const { return frames; } -std::string backtraceFramesToString(const std::vector & frames, const std::string & delimiter) -{ +std::string Backtrace::toString(const std::string & delimiter) const { + if (size == 0) + return ""; + std::stringstream backtrace; - char ** symbols = backtrace_symbols(frames.data(), frames.size()); + char ** symbols = backtrace_symbols(frames.data(), size); if (!symbols) { - if (frames.size() > 0) - backtrace << "No symbols could be found for backtrace starting at " << frames[0]; + backtrace << "No symbols could be found for backtrace starting at " << frames[0]; } else { - for (size_t i = 0; i < frames.size(); ++i) + for (size_t i = 0; i < size; ++i) { /// Perform demangling of names. Name is in parentheses, before '+' character. diff --git a/libs/libdaemon/CMakeLists.txt b/libs/libdaemon/CMakeLists.txt index 7c4bf767f9e..547c570313c 100644 --- a/libs/libdaemon/CMakeLists.txt +++ b/libs/libdaemon/CMakeLists.txt @@ -14,14 +14,6 @@ add_library (daemon include/daemon/OwnSplitChannel.h ) -if (USE_UNWIND) - target_compile_definitions (daemon PRIVATE USE_UNWIND=1) - target_include_directories (daemon BEFORE PRIVATE ${UNWIND_INCLUDE_DIR}) - if (NOT USE_INTERNAL_UNWIND_LIBRARY_FOR_EXCEPTION_HANDLING) - target_link_libraries (daemon PRIVATE ${UNWIND_LIBRARY}) - endif () -endif () - target_include_directories (daemon PUBLIC include) target_link_libraries (daemon PRIVATE clickhouse_common_io clickhouse_common_config common ${Poco_Net_LIBRARY} ${Poco_Util_LIBRARY} ${EXECINFO_LIBRARIES}) diff --git a/libs/libdaemon/src/BaseDaemon.cpp b/libs/libdaemon/src/BaseDaemon.cpp index af9190cf780..a14fe84e6a6 100644 --- a/libs/libdaemon/src/BaseDaemon.cpp +++ b/libs/libdaemon/src/BaseDaemon.cpp @@ -19,6 +19,7 @@ #include #include #include +#include #include #include #include @@ -56,13 +57,6 @@ #include #include -#if USE_UNWIND - #define UNW_LOCAL_ONLY - #include -#else - using unw_context_t = int; -#endif - #ifdef __APPLE__ // ucontext is not available without _XOPEN_SOURCE #define _XOPEN_SOURCE @@ -70,54 +64,6 @@ #include -/** For transferring information from signal handler to a separate thread. - * If you need to do something serious in case of a signal (example: write a message to the log), - * then sending information to a separate thread through pipe and doing all the stuff asynchronously - * - is probably the only safe method for doing it. - * (Because it's only safe to use reentrant functions in signal handlers.) - */ -struct Pipe -{ - union - { - int fds[2]; - struct - { - int read_fd; - int write_fd; - }; - }; - - Pipe() - { - read_fd = -1; - write_fd = -1; - - if (0 != pipe(fds)) - DB::throwFromErrno("Cannot create pipe", 0); - } - - void close() - { - if (-1 != read_fd) - { - ::close(read_fd); - read_fd = -1; - } - - if (-1 != write_fd) - { - ::close(write_fd); - write_fd = -1; - } - } - - ~Pipe() - { - close(); - } -}; - Pipe signal_pipe; @@ -132,7 +78,7 @@ static void call_default_signal_handler(int sig) using ThreadNumber = decltype(getThreadNumber()); -static const size_t buf_size = sizeof(int) + sizeof(siginfo_t) + sizeof(ucontext_t) + sizeof(ThreadNumber); +static const size_t buf_size = sizeof(int) + sizeof(siginfo_t) + sizeof(ucontext_t) + sizeof(Backtrace) + sizeof(ThreadNumber); using signal_function = void(int, siginfo_t*, void*); @@ -169,18 +115,13 @@ static void faultSignalHandler(int sig, siginfo_t * info, void * context) char buf[buf_size]; DB::WriteBufferFromFileDescriptor out(signal_pipe.fds_rw[1], buf_size, buf); - unw_context_t unw_context; - -#if USE_UNWIND - unw_getcontext(&unw_context); -#else - unw_context = 0; -#endif + const ucontext_t signal_context = *reinterpret_cast(context); + const Backtrace backtrace(signal_context); DB::writeBinary(sig, out); DB::writePODBinary(*info, out); - DB::writePODBinary(*reinterpret_cast(context), out); - DB::writePODBinary(unw_context, out); + DB::writePODBinary(signal_context, out); + DB::writePODBinary(backtrace, out); DB::writeBinary(getThreadNumber(), out); out.next(); @@ -192,30 +133,6 @@ static void faultSignalHandler(int sig, siginfo_t * info, void * context) } -#if USE_UNWIND -size_t backtraceLibUnwind(void ** out_frames, size_t max_frames, unw_context_t & context) -{ - unw_cursor_t cursor; - - if (unw_init_local(&cursor, &context) < 0) - return 0; - - size_t i = 0; - for (; i < max_frames; ++i) - { - unw_word_t ip; - unw_get_reg(&cursor, UNW_REG_IP, &ip); - out_frames[i] = reinterpret_cast(ip); - - if (!unw_step(&cursor)) - break; - } - - return i; -} -#endif - - /** The thread that read info about signal or std::terminate from pipe. * On HUP / USR1, close log files (for new files to be opened later). * On information about std::terminate, write it to log. @@ -277,15 +194,15 @@ public: { siginfo_t info; ucontext_t context; - unw_context_t unw_context; + Backtrace backtrace; ThreadNumber thread_num; DB::readPODBinary(info, in); DB::readPODBinary(context, in); - DB::readPODBinary(unw_context, in); + DB::readPODBinary(backtrace, in); DB::readBinary(thread_num, in); - onFault(sig, info, context, unw_context, thread_num); + onFault(sig, info, context, backtrace, thread_num); } } } @@ -300,18 +217,14 @@ private: LOG_ERROR(log, "(version " << VERSION_STRING << VERSION_OFFICIAL << ") (from thread " << thread_num << ") " << message); } - void onFault(int sig, siginfo_t & info, ucontext_t & context, unw_context_t & unw_context, ThreadNumber thread_num) const + void onFault(int sig, const siginfo_t & info, const ucontext_t & context, const Backtrace & backtrace, ThreadNumber thread_num) const { LOG_ERROR(log, "########################################"); LOG_ERROR(log, "(version " << VERSION_STRING << VERSION_OFFICIAL << ") (from thread " << thread_num << ") " << "Received signal " << strsignal(sig) << " (" << sig << ")" << "."); LOG_ERROR(log, signalToErrorMessage(sig, info, context)); - - std::vector frames = getBacktraceFrames(context); - std::string backtrace = backtraceFramesToString(frames); - - LOG_ERROR(log, backtrace); + LOG_ERROR(log, backtrace.toString("\n")); } };