* [PATCH][_GLIBCXX_DEBUG] libbacktrace integration @ 2021-04-24 13:46 François Dumont 2021-05-03 20:17 ` François Dumont 2021-05-05 11:33 ` Jonathan Wakely 0 siblings, 2 replies; 8+ messages in thread From: François Dumont @ 2021-04-24 13:46 UTC (permalink / raw) To: libstdc++; +Cc: gcc-patches [-- Attachment #1: Type: text/plain, Size: 3535 bytes --] Hi Here is the patch to add backtrace generation on _GLIBCXX_DEBUG assertions thanks to libbacktrace. In addition to this integration I am also improving the generation of the assertion message thanks to the "%.*s" printf format, it avoids an intermediate buffer most of the time. I am also removing the "__" used for uglification to get a nicer output. I can propose it in a dedicated patch if you prefer. I am adding GLIBCXX_3.4.30 abi version to properly export the 2 new weak symbols. Let me know if it isn't necessary. libstdc++: [_GLIBCXX_DEBUG] Add backtrace generation thanks to libbacktrace Add _GLIBCXX_DEBUG_BACKTRACE macro to activate backtrace generation on _GLIBCXX_DEBUG assertions using libbacktrace. * config/abi/pre/gnu.ver: Add GLIBCXX_3.4.30 version and new exports. * include/debug/formatter.h [_GLIBCXX_DEBUG_BACKTRACE]: Include <backtrace-supported.h>. [_GLIBCXX_DEBUG_BACKTRACE && BACKTRACE_SUPPORTED]: Include <backtrace.h>. [(!_GLIBCXX_DEBUG_BACKTRACE || !BACKTRACE_SUPPORTED) && _GLIBCXX_USE_C99_STDINT_TR1]: Include <stdint.h>. [_GLIBCXX_DEBUG_USE_LIBBACKTRACE] (__gnu_debug::__create_backtrace_state): New. [_GLIBCXX_DEBUG_USE_LIBBACKTRACE] (__gnu_debug::__render_backtrace): New. [_GLIBCXX_DEBUG_USE_LIBBACKTRACE](_Error_formatter::_M_print_backtrace): New. [_GLIBCXX_DEBUG_USE_LIBBACKTRACE](_Error_formatter::_M_backtrace_state): New. (_Error_formatter::_Error_formatter): Outline definition. * src/c++11/debug.cc: Include <cstring>. (_Print_func_t): New. (print_word): Use '%.*s' format in fprintf to render only expected number of chars. (print_raw(PrintContext&, const char*, ptrdiff_t)): New. (print_function(PrintContext&, const char*, _Print_func_t)): New. (print_type): Use latter. (print_string(PrintContext&, const char*, const _Parameter*, size_t)): Change signature to... (print_string(PrintContext&, const char*, ptrdiff_t, const _Parameter*, size_t)): ...this and adapt. Remove intermediate buffer to render input string. (print_string(PrintContext&, const char*, ptrdiff_t)): New. [_GLIBCXX_DEBUG_USE_LIBBACKTRACE] (print_backtrace(void*, uintptr_t, const char*, int, const char*)): New. (_Error_formatter::_M_error()): Adapt. [_GLIBCXX_DEBUG_USE_LIBBACKTRACE] (__gnu_debug::__create_backtrace_state): New, weak symbol. [_GLIBCXX_DEBUG_USE_LIBBACKTRACE] (__gnu_debug::__render_backtrace): New, weak symbol. * testsuite/util/testsuite_abi.cc: Add new symbol version. * doc/xml/manual/debug_mode.xml: Document _GLIBCXX_DEBUG_BACKTRACE. * doc/xml/manual/using.xml: Likewise. Tested under Linux x86_64. Ok to commit ? François [-- Attachment #2: libbacktrace.patch --] [-- Type: text/x-patch, Size: 17131 bytes --] diff --git a/libstdc++-v3/config/abi/pre/gnu.ver b/libstdc++-v3/config/abi/pre/gnu.ver index 5323c7f0604..2606d67d8a9 100644 --- a/libstdc++-v3/config/abi/pre/gnu.ver +++ b/libstdc++-v3/config/abi/pre/gnu.ver @@ -2397,6 +2397,15 @@ GLIBCXX_3.4.29 { } GLIBCXX_3.4.28; +GLIBCXX_3.4.30 { + + # __gnu_debug::__create_backtrace + _ZN11__gnu_debug24__create_backtrace_stateEv; + _ZN11__gnu_debug18__render_backtraceEPvPFiS0_mPKciS2_ES0_; + +} GLIBCXX_3.4.29; + + # Symbols in the support library (libsupc++) have their own tag. CXXABI_1.3 { diff --git a/libstdc++-v3/doc/xml/manual/debug_mode.xml b/libstdc++-v3/doc/xml/manual/debug_mode.xml index 883e8cb4f03..931b09710f3 100644 --- a/libstdc++-v3/doc/xml/manual/debug_mode.xml +++ b/libstdc++-v3/doc/xml/manual/debug_mode.xml @@ -160,6 +160,12 @@ which always works correctly. <code>GLIBCXX_DEBUG_MESSAGE_LENGTH</code> can be used to request a different length.</para> +<para>Note that libstdc++ is able to use + <link xmlns:xlink="http://www.w3.org/1999/xlink" + xlink:href="https://github.com/ianlancetaylor/libbacktrace">libbacktrace</link> + to produce backtraces on error. Use <code>-D_GLIBCXX_DEBUG_BACKTRACE</code> to + activate it. You'll also have to link with libbacktrace + (<option>-lbacktrace</option>) to build your application.</para> </section> <section xml:id="debug_mode.using.specific" xreflabel="Using Specific"><info><title>Using a Specific Debug Container</title></info> diff --git a/libstdc++-v3/doc/xml/manual/using.xml b/libstdc++-v3/doc/xml/manual/using.xml index 24543e9526e..9bd0da8c1c5 100644 --- a/libstdc++-v3/doc/xml/manual/using.xml +++ b/libstdc++-v3/doc/xml/manual/using.xml @@ -1128,6 +1128,16 @@ g++ -Winvalid-pch -I. -include stdc++.h -H -g -O2 hello.cc -o test.exe extensions and libstdc++-specific behavior into errors. </para> </listitem></varlistentry> + <varlistentry><term><code>_GLIBCXX_DEBUG_BACKTRACE</code></term> + <listitem> + <para> + Undefined by default. Considered only if <code>_GLIBCXX_DEBUG</code> + is defined. When defined, checks for <link xmlns:xlink="http://www.w3.org/1999/xlink" + xlink:href="https://github.com/ianlancetaylor/libbacktrace">libbacktrace</link> + support and use it to display backtraces on + <link linkend="manual.ext.debug_mode">debug mode</link> assertions. + </para> + </listitem></varlistentry> <varlistentry><term><code>_GLIBCXX_PARALLEL</code></term> <listitem> <para>Undefined by default. When defined, compiles user code @@ -1634,6 +1644,17 @@ A quick read of the relevant part of the GCC header will remain compatible between different GCC releases. </para> </section> + + <section xml:id="manual.intro.using.linkage.externals" xreflabel="External Libraries"><info><title>External Libraries</title></info> + + <para> + libstdc++ <link linkend="manual.ext.debug_mode">debug mode</link> is able to + produce backtraces thanks to <link xmlns:xlink="http://www.w3.org/1999/xlink" + xlink:href="https://github.com/ianlancetaylor/libbacktrace">libbacktrace</link>. + To use the library you should define <code>_GLIBCXX_DEBUG_BACKTRACE</code> along + with <code>_GLIBCXX_DEBUG</code> and link with <option>-lbacktrace</option>. + </para> + </section> </section> <section xml:id="manual.intro.using.concurrency" xreflabel="Concurrency"><info><title>Concurrency</title></info> diff --git a/libstdc++-v3/include/debug/formatter.h b/libstdc++-v3/include/debug/formatter.h index 7140fed5e83..ec0984e438f 100644 --- a/libstdc++-v3/include/debug/formatter.h +++ b/libstdc++-v3/include/debug/formatter.h @@ -31,6 +31,31 @@ #include <bits/c++config.h> +#if defined(_GLIBCXX_DEBUG_BACKTRACE) +# if !defined(BACKTRACE_SUPPORTED) +# include <backtrace-supported.h> +# endif +#endif + +#if defined(_GLIBCXX_DEBUG_BACKTRACE) && BACKTRACE_SUPPORTED +# define _GLIBCXX_DEBUG_USE_LIBBACKTRACE 1 +# include <backtrace.h> + +namespace __gnu_debug +{ + typedef backtrace_full_callback __backtrace_full_cb; +} +#elif defined (_GLIBCXX_USE_C99_STDINT_TR1) +# define _GLIBCXX_DEBUG_USE_LIBBACKTRACE 0 +# include <stdint.h> // For uintptr_t. + +namespace __gnu_debug +{ + typedef int (*__backtrace_full_cb) (void*, uintptr_t, const char*, + int, const char*); +} +#endif + #if __cpp_rtti # include <typeinfo> # define _GLIBCXX_TYPEID(_Type) &typeid(_Type) @@ -155,6 +180,16 @@ namespace __gnu_debug __msg_irreflexive_ordering }; +#ifdef _GLIBCXX_DEBUG_USE_LIBBACKTRACE + + void* + __create_backtrace_state(); + + void + __render_backtrace(void*, __backtrace_full_cb, void*); + +#endif + class _Error_formatter { // Tags denoting the type of parameter for construction @@ -558,12 +593,14 @@ namespace __gnu_debug _M_print_string(const char* __string) const _GLIBCXX_DEPRECATED; #endif +#ifdef _GLIBCXX_DEBUG_USE_LIBBACKTRACE + void + _M_print_backtrace(__backtrace_full_cb __cb, void* __data) const; +#endif + private: _Error_formatter(const char* __file, unsigned int __line, - const char* __function) - : _M_file(__file), _M_line(__line), _M_num_parameters(0), _M_text(0) - , _M_function(__function) - { } + const char* __function); #if !_GLIBCXX_INLINE_VERSION void @@ -578,6 +615,9 @@ namespace __gnu_debug unsigned int _M_num_parameters; const char* _M_text; const char* _M_function; +#ifdef _GLIBCXX_DEBUG_USE_LIBBACKTRACE + void* _M_backtrace_state; +#endif public: static _Error_formatter& @@ -587,6 +627,36 @@ namespace __gnu_debug return __formatter; } }; + +#if _GLIBCXX_DEBUG_USE_LIBBACKTRACE + + void* + __create_backtrace_state() + { return backtrace_create_state(NULL, 0, NULL, NULL); } + + void + __render_backtrace(void* __bt, __backtrace_full_cb __cb, void* __data) + { backtrace_full((backtrace_state*)__bt, 1, __cb, 0, __data); } + +#endif + + _Error_formatter:: + _Error_formatter(const char* __file, unsigned int __line, + const char* __function) + : _M_file(__file), _M_line(__line), _M_num_parameters(0), _M_text(0) + , _M_function(__function) +#ifdef _GLIBCXX_DEBUG_USE_LIBBACKTRACE + , _M_backtrace_state(__create_backtrace_state()) +#endif + { } + +#ifdef _GLIBCXX_DEBUG_USE_LIBBACKTRACE + void + _Error_formatter:: + _M_print_backtrace(__backtrace_full_cb __cb, void* __data) const + { __render_backtrace(_M_backtrace_state, __cb, __data); } +#endif + } // namespace __gnu_debug #undef _GLIBCXX_TYPEID diff --git a/libstdc++-v3/src/c++11/debug.cc b/libstdc++-v3/src/c++11/debug.cc index 5a642097d17..790810fdab4 100644 --- a/libstdc++-v3/src/c++11/debug.cc +++ b/libstdc++-v3/src/c++11/debug.cc @@ -34,16 +34,12 @@ #include <cassert> #include <cstdio> -#include <cctype> // for std::isspace +#include <cctype> // for std::isspace. +#include <cstring> // for std::strstr. -#include <algorithm> // for std::min +#include <algorithm> // for std::min. -#include <cxxabi.h> // for __cxa_demangle - -// libstdc++/85768 -#if 0 // defined _GLIBCXX_HAVE_EXECINFO_H -# include <execinfo.h> // for backtrace -#endif +#include <cxxabi.h> // for __cxa_demangle. #include "mutex_pool.h" @@ -574,7 +570,7 @@ namespace struct PrintContext { PrintContext() - : _M_max_length(78), _M_column(1), _M_first_line(true), _M_wordwrap(false) + : _M_max_length(78), _M_column(1), _M_first_line(true), _M_wordwrap(false) { get_max_length(_M_max_length); } std::size_t _M_max_length; @@ -584,16 +580,17 @@ namespace bool _M_wordwrap; }; + typedef void (*_Print_func_t) (PrintContext&, const char*, ptrdiff_t); + template<size_t Length> void print_literal(PrintContext& ctx, const char(&word)[Length]) { print_word(ctx, word, Length - 1); } void - print_word(PrintContext& ctx, const char* word, - std::ptrdiff_t count = -1) + print_word(PrintContext& ctx, const char* word, ptrdiff_t nbc = -1) { - size_t length = count >= 0 ? count : __builtin_strlen(word); + size_t length = nbc >= 0 ? nbc : __builtin_strlen(word); if (length == 0) return; @@ -619,15 +616,12 @@ namespace // If this isn't the first line, indent if (ctx._M_column == 1 && !ctx._M_first_line) { - char spacing[ctx._M_indent + 1]; - for (int i = 0; i < ctx._M_indent; ++i) - spacing[i] = ' '; - spacing[ctx._M_indent] = '\0'; + const char spacing[ctx._M_indent + 1] = " "; fprintf(stderr, "%s", spacing); ctx._M_column += ctx._M_indent; } - int written = fprintf(stderr, "%s", word); + int written = fprintf(stderr, "%.*s", (int)length, word); if (word[length - 1] == '\n') { @@ -640,7 +634,57 @@ namespace else { print_literal(ctx, "\n"); - print_word(ctx, word, count); + print_word(ctx, word, nbc); + } + } + + void + print_raw(PrintContext&, const char* str, ptrdiff_t nbc) + { + if (nbc >= 0) + fprintf(stderr, "%.*s", (int)nbc, str); + else + fprintf(stderr, "%s", str); + } + + void + print_function(PrintContext& ctx, const char* func, + _Print_func_t print_func) + { + const char cxx1998[] = "__cxx1998::"; + const char uglification[] = "__"; + for (;;) + { + auto idx1 = strstr(func, cxx1998); + auto idx2 = strstr(func, uglification); + if (idx1 || idx2) + { + if (idx1 && (!idx2 || idx1 <= idx2)) + { + if (idx1 != func) + print_func(ctx, func, idx1 - func); + + func = idx1 + sizeof(cxx1998) - 1; + } + else if (idx2) + { + if (idx2 != func) + print_func(ctx, func, idx2 - func); + + func = idx2 + sizeof(uglification) - 1; + } + + while (*func && isspace(*func)) + ++func; + + if (!*func) + break; + } + else + { + print_func(ctx, func, -1); + break; + } } } @@ -657,7 +701,10 @@ namespace int status; char* demangled_name = __cxxabiv1::__cxa_demangle(info->name(), NULL, NULL, &status); - print_word(ctx, status == 0 ? demangled_name : info->name()); + if (status == 0) + print_function(ctx, demangled_name, &print_word); + else + print_word(ctx, info->name(), -1); free(demangled_name); } } @@ -925,61 +972,62 @@ namespace } void - print_string(PrintContext& ctx, const char* string, + print_string(PrintContext& ctx, const char* str, ptrdiff_t nbc, const _Parameter* parameters, std::size_t num_parameters) { - const char* start = string; - const int bufsize = 128; - char buf[bufsize]; - int bufindex = 0; + const char* start = str; + const char* end = nbc >= 0 ? start + nbc : nullptr; + char buf[64]; - while (*start) + while ((end && str != end) || (!end && *str)) { - if (isspace(*start)) + if (isspace(*str)) { - buf[bufindex++] = *start++; - buf[bufindex] = '\0'; - print_word(ctx, buf, bufindex); - bufindex = 0; + ++str; + print_word(ctx, start, str - start); + start = str; continue; } - if (!num_parameters || *start != '%') + if (!parameters || *str != '%') { // Normal char or no parameter to look for. - buf[bufindex++] = *start++; + ++str; continue; } - if (*++start == '%') + if (*++str == '%') { // Escaped '%' - buf[bufindex++] = *start++; + print_word(ctx, start, str - start); + ++str; + start = str; continue; } // We are on a parameter property reference, we need to flush buffer // first. - if (bufindex != 0) + if (str != start) { - buf[bufindex] = '\0'; - print_word(ctx, buf, bufindex); - bufindex = 0; + // Avoid to print the '%'. + if (str - start > 1) + print_word(ctx, start, str - start - 1); + start = str; } // Get the parameter number - assert(*start >= '1' && *start <= '9'); - size_t param_index = *start - '0' - 1; + assert(*str >= '1' && *str <= '9'); + size_t param_index = *str - '0' - 1; assert(param_index < num_parameters); const auto& param = parameters[param_index]; // '.' separates the parameter number from the field // name, if there is one. - ++start; - if (*start != '.') + ++str; + if (*str != '.') { - assert(*start == ';'); - ++start; + assert(*str == ';'); + ++str; if (param._M_kind == _Parameter::__integer) { int written @@ -988,8 +1036,9 @@ namespace print_word(ctx, buf, written); } else if (param._M_kind == _Parameter::__string) - print_string(ctx, param._M_variant._M_string._M_value, + print_string(ctx, param._M_variant._M_string._M_value, -1, parameters, num_parameters); + start = str; continue; } @@ -997,26 +1046,80 @@ namespace const int max_field_len = 16; char field[max_field_len]; int field_idx = 0; - ++start; - while (*start != ';') + ++str; + while (*str != ';') { - assert(*start); + assert(*str); assert(field_idx < max_field_len - 1); - field[field_idx++] = *start++; + field[field_idx++] = *str++; } - ++start; + ++str; field[field_idx] = '\0'; print_field(ctx, param, field); + start = str; } // Might need to flush. - if (bufindex) + if (str != start) + print_word(ctx, start, str - start); + } + + void + print_string(PrintContext& ctx, const char* str, ptrdiff_t nbc) + { print_string(ctx, str, nbc, nullptr, 0); } + +#ifdef _GLIBCXX_DEBUG_USE_LIBBACKTRACE + int + print_backtrace(void* data, uintptr_t pc, const char* filename, + int lineno, const char* function) + { + const int bufsize = 64; + char buf[bufsize]; + + PrintContext& ctx = *static_cast<PrintContext*>(data); + + int written = __builtin_sprintf(buf, "%p ", (void*)pc); + print_word(ctx, buf, written); + + int ret = 0; + if (function) { - buf[bufindex] = '\0'; - print_word(ctx, buf, bufindex); + int status; + char* demangled_name = + __cxxabiv1::__cxa_demangle(function, NULL, NULL, &status); + if (status == 0) + print_function(ctx, demangled_name, &print_raw); + else + print_word(ctx, function); + + free(demangled_name); + ret = strstr(function, "main") ? 1 : 0; } + + print_literal(ctx, "\n"); + + if (filename) + { + bool wordwrap = false; + swap(wordwrap, ctx._M_wordwrap); + print_word(ctx, filename); + + if (lineno) + { + written = __builtin_sprintf(buf, ":%u\n", lineno); + print_word(ctx, buf, written); + } + else + print_literal(ctx, "\n"); + swap(wordwrap, ctx._M_wordwrap); + } + else + print_literal(ctx, "???:0\n"); + + return ret; } +#endif } namespace __gnu_debug @@ -1058,41 +1161,27 @@ namespace __gnu_debug if (_M_function) { print_literal(ctx, "In function:\n"); - print_string(ctx, _M_function, nullptr, 0); + print_function(ctx, _M_function, &print_string); print_literal(ctx, "\n"); ctx._M_first_line = true; print_literal(ctx, "\n"); } -// libstdc++/85768 -#if 0 //defined _GLIBCXX_HAVE_EXECINFO_H - { - void* stack[32]; - int nb = backtrace(stack, 32); - - // Note that we skip current method symbol. - if (nb > 1) - { - print_literal(ctx, "Backtrace:\n"); - auto symbols = backtrace_symbols(stack, nb); - for (int i = 1; i < nb; ++i) - { - print_word(ctx, symbols[i]); - print_literal(ctx, "\n"); - } - - free(symbols); - ctx._M_first_line = true; - print_literal(ctx, "\n"); - } - } +#ifdef _GLIBCXX_DEBUG_USE_LIBBACKTRACE + if (_M_backtrace_state) + { + print_literal(ctx, "Backtrace:\n"); + _M_print_backtrace(print_backtrace, &ctx); + ctx._M_first_line = true; + print_literal(ctx, "\n"); + } #endif print_literal(ctx, "Error: "); // Print the error message assert(_M_text); - print_string(ctx, _M_text, _M_parameters, _M_num_parameters); + print_string(ctx, _M_text, -1, _M_parameters, _M_num_parameters); print_literal(ctx, ".\n"); // Emit descriptions of the objects involved in the operation @@ -1123,6 +1212,18 @@ namespace __gnu_debug abort(); } +#ifdef _GLIBCXX_DEBUG_USE_LIBBACKTRACE + + _GLIBCXX_WEAK_DEFINITION void* + __create_backtrace_state() + { return nullptr; } + + _GLIBCXX_WEAK_DEFINITION void + __render_backtrace(void*, __backtrace_full_cb, void*) + { } + +#endif + #if !_GLIBCXX_INLINE_VERSION // Deprecated methods kept for backward compatibility. void diff --git a/libstdc++-v3/testsuite/util/testsuite_abi.cc b/libstdc++-v3/testsuite/util/testsuite_abi.cc index 3af5dc593c2..2c3204e57a5 100644 --- a/libstdc++-v3/testsuite/util/testsuite_abi.cc +++ b/libstdc++-v3/testsuite/util/testsuite_abi.cc @@ -210,6 +210,7 @@ check_version(symbol& test, bool added) known_versions.push_back("GLIBCXX_3.4.27"); known_versions.push_back("GLIBCXX_3.4.28"); known_versions.push_back("GLIBCXX_3.4.29"); + known_versions.push_back("GLIBCXX_3.4.30"); known_versions.push_back("GLIBCXX_LDBL_3.4.29"); known_versions.push_back("GLIBCXX_IEEE128_3.4.29"); known_versions.push_back("CXXABI_1.3"); ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH][_GLIBCXX_DEBUG] libbacktrace integration 2021-04-24 13:46 [PATCH][_GLIBCXX_DEBUG] libbacktrace integration François Dumont @ 2021-05-03 20:17 ` François Dumont 2021-05-03 21:06 ` Jonathan Wakely 2021-05-05 11:33 ` Jonathan Wakely 1 sibling, 1 reply; 8+ messages in thread From: François Dumont @ 2021-05-03 20:17 UTC (permalink / raw) To: libstdc++; +Cc: gcc-patches Is it too early to consider this patch ? Or just lack of time ? Considering the patch I would really appreciate that, if validated, it gets in as early as possible in next release. Thanks, François On 24/04/21 3:46 pm, François Dumont wrote: > Hi > > Here is the patch to add backtrace generation on _GLIBCXX_DEBUG > assertions thanks to libbacktrace. > > In addition to this integration I am also improving the generation > of the assertion message thanks to the "%.*s" printf format, it avoids > an intermediate buffer most of the time. I am also removing the "__" > used for uglification to get a nicer output. I can propose it in a > dedicated patch if you prefer. > > I am adding GLIBCXX_3.4.30 abi version to properly export the 2 > new weak symbols. Let me know if it isn't necessary. > > libstdc++: [_GLIBCXX_DEBUG] Add backtrace generation thanks to > libbacktrace > > Add _GLIBCXX_DEBUG_BACKTRACE macro to activate backtrace > generation on > _GLIBCXX_DEBUG assertions using libbacktrace. > > * config/abi/pre/gnu.ver: Add GLIBCXX_3.4.30 version and > new exports. > * include/debug/formatter.h [_GLIBCXX_DEBUG_BACKTRACE]: > Include <backtrace-supported.h>. > [_GLIBCXX_DEBUG_BACKTRACE && BACKTRACE_SUPPORTED]: > Include <backtrace.h>. > [(!_GLIBCXX_DEBUG_BACKTRACE || !BACKTRACE_SUPPORTED) && > _GLIBCXX_USE_C99_STDINT_TR1]: Include <stdint.h>. > [_GLIBCXX_DEBUG_USE_LIBBACKTRACE] > (__gnu_debug::__create_backtrace_state): New. > [_GLIBCXX_DEBUG_USE_LIBBACKTRACE] > (__gnu_debug::__render_backtrace): New. > [_GLIBCXX_DEBUG_USE_LIBBACKTRACE](_Error_formatter::_M_print_backtrace): > New. > [_GLIBCXX_DEBUG_USE_LIBBACKTRACE](_Error_formatter::_M_backtrace_state): > New. > (_Error_formatter::_Error_formatter): Outline definition. > * src/c++11/debug.cc: Include <cstring>. > (_Print_func_t): New. > (print_word): Use '%.*s' format in fprintf to render only > expected > number of chars. > (print_raw(PrintContext&, const char*, ptrdiff_t)): New. > (print_function(PrintContext&, const char*, > _Print_func_t)): New. > (print_type): Use latter. > (print_string(PrintContext&, const char*, const > _Parameter*, size_t)): > Change signature to... > (print_string(PrintContext&, const char*, ptrdiff_t, const > _Parameter*, > size_t)): ...this and adapt. Remove intermediate buffer to > render input > string. > (print_string(PrintContext&, const char*, ptrdiff_t)): New. > [_GLIBCXX_DEBUG_USE_LIBBACKTRACE] > (print_backtrace(void*, uintptr_t, const char*, int, const > char*)): New. > (_Error_formatter::_M_error()): Adapt. > [_GLIBCXX_DEBUG_USE_LIBBACKTRACE] > (__gnu_debug::__create_backtrace_state): New, weak symbol. > [_GLIBCXX_DEBUG_USE_LIBBACKTRACE] > (__gnu_debug::__render_backtrace): New, weak symbol. > * testsuite/util/testsuite_abi.cc: Add new symbol version. > * doc/xml/manual/debug_mode.xml: Document > _GLIBCXX_DEBUG_BACKTRACE. > * doc/xml/manual/using.xml: Likewise. > > Tested under Linux x86_64. > > Ok to commit ? > > François > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH][_GLIBCXX_DEBUG] libbacktrace integration 2021-05-03 20:17 ` François Dumont @ 2021-05-03 21:06 ` Jonathan Wakely 2021-05-04 6:03 ` François Dumont 0 siblings, 1 reply; 8+ messages in thread From: Jonathan Wakely @ 2021-05-03 21:06 UTC (permalink / raw) To: François Dumont; +Cc: libstdc++, gcc-patches On 03/05/21 22:17 +0200, François Dumont via Libstdc++ wrote: >Is it too early to consider this patch ? Or just lack of time ? I haven't had time to review it yet, but my general feeling hasn't changed. I still don't like the idea of executing additional code after undefined behaviour is detected. I've been convinced by glibc folk that every bit of code run when the program state is corrupt increases the risk that it can be exploited by an attacker. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH][_GLIBCXX_DEBUG] libbacktrace integration 2021-05-03 21:06 ` Jonathan Wakely @ 2021-05-04 6:03 ` François Dumont 2021-05-05 10:01 ` Jonathan Wakely 0 siblings, 1 reply; 8+ messages in thread From: François Dumont @ 2021-05-04 6:03 UTC (permalink / raw) To: Jonathan Wakely; +Cc: libstdc++, gcc-patches On 03/05/21 11:06 pm, Jonathan Wakely wrote: > On 03/05/21 22:17 +0200, François Dumont via Libstdc++ wrote: >> Is it too early to consider this patch ? Or just lack of time ? > > I haven't had time to review it yet, but my general feeling hasn't > changed. I still don't like the idea of executing additional code > after undefined behaviour is detected. I've been convinced by glibc > folk that every bit of code run when the program state is corrupt > increases the risk that it can be exploited by an attacker. > > > Ok, I must have miss (or forgot) this feedback. Well, isn't it the current situation of the whole _GLIBCXX_DEBUG mode ? For me _GLIBCXX_DEBUG mode purpose is to detect UB situation and to assert _before_ any UB code is run. Moreover it is optional. This is a feature to use when _GLIBCXX_DEBUG is telling you that you have a problem in your code but you just cannot find where it is called. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH][_GLIBCXX_DEBUG] libbacktrace integration 2021-05-04 6:03 ` François Dumont @ 2021-05-05 10:01 ` Jonathan Wakely 0 siblings, 0 replies; 8+ messages in thread From: Jonathan Wakely @ 2021-05-05 10:01 UTC (permalink / raw) To: François Dumont; +Cc: libstdc++, gcc-patches On 04/05/21 08:03 +0200, François Dumont wrote: >On 03/05/21 11:06 pm, Jonathan Wakely wrote: >>On 03/05/21 22:17 +0200, François Dumont via Libstdc++ wrote: >>>Is it too early to consider this patch ? Or just lack of time ? >> >>I haven't had time to review it yet, but my general feeling hasn't >>changed. I still don't like the idea of executing additional code >>after undefined behaviour is detected. I've been convinced by glibc >>folk that every bit of code run when the program state is corrupt >>increases the risk that it can be exploited by an attacker. >> >> >> >Ok, I must have miss (or forgot) this feedback. See https://gcc.gnu.org/pipermail/libstdc++/2018-December/048061.html >Well, isn't it the current situation of the whole _GLIBCXX_DEBUG mode ? Yes, but adding more code makes it worse. >For me _GLIBCXX_DEBUG mode purpose is to detect UB situation and to >assert _before_ any UB code is run. Yes, it stops running the user code, but then runs its own code to format the message to show to the user. The more code that runs when the program is in an inconsistent/undefined state, the more likely it is that some of that code can be exploited to do something bad. >Moreover it is optional. This is a feature to use when _GLIBCXX_DEBUG >is telling you that you have a problem in your code but you just >cannot find where it is called. Which you can do with a debugger. When debug mode calls abort() it will stop the program in a debugger, or produce a core file that can be examined in a debugger. The stacktrace is a convenience, not providing anything that couldn't be done already. Anyway, I'll review the patch... ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH][_GLIBCXX_DEBUG] libbacktrace integration 2021-04-24 13:46 [PATCH][_GLIBCXX_DEBUG] libbacktrace integration François Dumont 2021-05-03 20:17 ` François Dumont @ 2021-05-05 11:33 ` Jonathan Wakely 2021-05-07 14:26 ` Jonathan Wakely 1 sibling, 1 reply; 8+ messages in thread From: Jonathan Wakely @ 2021-05-05 11:33 UTC (permalink / raw) To: François Dumont; +Cc: libstdc++, gcc-patches On 24/04/21 15:46 +0200, François Dumont via Libstdc++ wrote: >Hi > > Here is the patch to add backtrace generation on _GLIBCXX_DEBUG >assertions thanks to libbacktrace. > > In addition to this integration I am also improving the generation >of the assertion message thanks to the "%.*s" printf format, it avoids >an intermediate buffer most of the time. I am also removing the "__" >used for uglification to get a nicer output. I can propose it in a >dedicated patch if you prefer. > > I am adding GLIBCXX_3.4.30 abi version to properly export the 2 >new weak symbols. Let me know if it isn't necessary. > > libstdc++: [_GLIBCXX_DEBUG] Add backtrace generation thanks to >libbacktrace > > Add _GLIBCXX_DEBUG_BACKTRACE macro to activate backtrace >generation on > _GLIBCXX_DEBUG assertions using libbacktrace. > > * config/abi/pre/gnu.ver: Add GLIBCXX_3.4.30 version and >new exports. > * include/debug/formatter.h [_GLIBCXX_DEBUG_BACKTRACE]: > Include <backtrace-supported.h>. > [_GLIBCXX_DEBUG_BACKTRACE && BACKTRACE_SUPPORTED]: > Include <backtrace.h>. > [(!_GLIBCXX_DEBUG_BACKTRACE || !BACKTRACE_SUPPORTED) && > _GLIBCXX_USE_C99_STDINT_TR1]: Include <stdint.h>. > [_GLIBCXX_DEBUG_USE_LIBBACKTRACE] > (__gnu_debug::__create_backtrace_state): New. > [_GLIBCXX_DEBUG_USE_LIBBACKTRACE] > (__gnu_debug::__render_backtrace): New. >[_GLIBCXX_DEBUG_USE_LIBBACKTRACE](_Error_formatter::_M_print_backtrace): > New. >[_GLIBCXX_DEBUG_USE_LIBBACKTRACE](_Error_formatter::_M_backtrace_state): > New. > (_Error_formatter::_Error_formatter): Outline definition. > * src/c++11/debug.cc: Include <cstring>. > (_Print_func_t): New. > (print_word): Use '%.*s' format in fprintf to render only >expected > number of chars. > (print_raw(PrintContext&, const char*, ptrdiff_t)): New. > (print_function(PrintContext&, const char*, >_Print_func_t)): New. > (print_type): Use latter. > (print_string(PrintContext&, const char*, const >_Parameter*, size_t)): > Change signature to... > (print_string(PrintContext&, const char*, ptrdiff_t, const >_Parameter*, > size_t)): ...this and adapt. Remove intermediate buffer to >render input > string. > (print_string(PrintContext&, const char*, ptrdiff_t)): New. > [_GLIBCXX_DEBUG_USE_LIBBACKTRACE] > (print_backtrace(void*, uintptr_t, const char*, int, const >char*)): New. > (_Error_formatter::_M_error()): Adapt. > [_GLIBCXX_DEBUG_USE_LIBBACKTRACE] > (__gnu_debug::__create_backtrace_state): New, weak symbol. > [_GLIBCXX_DEBUG_USE_LIBBACKTRACE] > (__gnu_debug::__render_backtrace): New, weak symbol. > * testsuite/util/testsuite_abi.cc: Add new symbol version. > * doc/xml/manual/debug_mode.xml: Document >_GLIBCXX_DEBUG_BACKTRACE. > * doc/xml/manual/using.xml: Likewise. > >Tested under Linux x86_64. > >Ok to commit ? > >François > >diff --git a/libstdc++-v3/config/abi/pre/gnu.ver b/libstdc++-v3/config/abi/pre/gnu.ver >index 5323c7f0604..2606d67d8a9 100644 >--- a/libstdc++-v3/config/abi/pre/gnu.ver >+++ b/libstdc++-v3/config/abi/pre/gnu.ver >@@ -2397,6 +2397,15 @@ GLIBCXX_3.4.29 { > > } GLIBCXX_3.4.28; > >+GLIBCXX_3.4.30 { >+ >+ # __gnu_debug::__create_backtrace >+ _ZN11__gnu_debug24__create_backtrace_stateEv; >+ _ZN11__gnu_debug18__render_backtraceEPvPFiS0_mPKciS2_ES0_; >+ >+} GLIBCXX_3.4.29; >+ >+ > # Symbols in the support library (libsupc++) have their own tag. > CXXABI_1.3 { > >diff --git a/libstdc++-v3/doc/xml/manual/debug_mode.xml b/libstdc++-v3/doc/xml/manual/debug_mode.xml >index 883e8cb4f03..931b09710f3 100644 >--- a/libstdc++-v3/doc/xml/manual/debug_mode.xml >+++ b/libstdc++-v3/doc/xml/manual/debug_mode.xml >@@ -160,6 +160,12 @@ which always works correctly. > <code>GLIBCXX_DEBUG_MESSAGE_LENGTH</code> can be used to request a > different length.</para> > >+<para>Note that libstdc++ is able to use >+ <link xmlns:xlink="http://www.w3.org/1999/xlink" >+ xlink:href="https://github.com/ianlancetaylor/libbacktrace">libbacktrace</link> >+ to produce backtraces on error. Use <code>-D_GLIBCXX_DEBUG_BACKTRACE</code> to >+ activate it. You'll also have to link with libbacktrace >+ (<option>-lbacktrace</option>) to build your application.</para> > </section> > > <section xml:id="debug_mode.using.specific" xreflabel="Using Specific"><info><title>Using a Specific Debug Container</title></info> >diff --git a/libstdc++-v3/doc/xml/manual/using.xml b/libstdc++-v3/doc/xml/manual/using.xml >index 24543e9526e..9bd0da8c1c5 100644 >--- a/libstdc++-v3/doc/xml/manual/using.xml >+++ b/libstdc++-v3/doc/xml/manual/using.xml >@@ -1128,6 +1128,16 @@ g++ -Winvalid-pch -I. -include stdc++.h -H -g -O2 hello.cc -o test.exe > extensions and libstdc++-specific behavior into errors. > </para> > </listitem></varlistentry> >+ <varlistentry><term><code>_GLIBCXX_DEBUG_BACKTRACE</code></term> >+ <listitem> >+ <para> >+ Undefined by default. Considered only if <code>_GLIBCXX_DEBUG</code> >+ is defined. When defined, checks for <link xmlns:xlink="http://www.w3.org/1999/xlink" >+ xlink:href="https://github.com/ianlancetaylor/libbacktrace">libbacktrace</link> >+ support and use it to display backtraces on s/and use it/and uses it/ >+ <link linkend="manual.ext.debug_mode">debug mode</link> assertions. >+ </para> >+ </listitem></varlistentry> > <varlistentry><term><code>_GLIBCXX_PARALLEL</code></term> > <listitem> > <para>Undefined by default. When defined, compiles user code >@@ -1634,6 +1644,17 @@ A quick read of the relevant part of the GCC > header will remain compatible between different GCC releases. > </para> > </section> >+ >+ <section xml:id="manual.intro.using.linkage.externals" xreflabel="External Libraries"><info><title>External Libraries</title></info> >+ >+ <para> >+ libstdc++ <link linkend="manual.ext.debug_mode">debug mode</link> is able to >+ produce backtraces thanks to <link xmlns:xlink="http://www.w3.org/1999/xlink" >+ xlink:href="https://github.com/ianlancetaylor/libbacktrace">libbacktrace</link>. >+ To use the library you should define <code>_GLIBCXX_DEBUG_BACKTRACE</code> along >+ with <code>_GLIBCXX_DEBUG</code> and link with <option>-lbacktrace</option>. >+ </para> >+ </section> > </section> > > <section xml:id="manual.intro.using.concurrency" xreflabel="Concurrency"><info><title>Concurrency</title></info> >diff --git a/libstdc++-v3/include/debug/formatter.h b/libstdc++-v3/include/debug/formatter.h >index 7140fed5e83..ec0984e438f 100644 >--- a/libstdc++-v3/include/debug/formatter.h >+++ b/libstdc++-v3/include/debug/formatter.h >@@ -31,6 +31,31 @@ > > #include <bits/c++config.h> > >+#if defined(_GLIBCXX_DEBUG_BACKTRACE) >+# if !defined(BACKTRACE_SUPPORTED) I think this should also depend on __has_include(<backtrace-supported.h>) #if defined(_GLIBCXX_DEBUG_BACKTRACE) # if !defined(BACKTRACE_SUPPORTED) && __has_include(<backtrace-supported.h>) # include <backtrace-supported.h> # endif #endif >+# include <backtrace-supported.h> >+# endif >+#endif >+ >+#if defined(_GLIBCXX_DEBUG_BACKTRACE) && BACKTRACE_SUPPORTED >+# define _GLIBCXX_DEBUG_USE_LIBBACKTRACE 1 >+# include <backtrace.h> >+ >+namespace __gnu_debug >+{ >+ typedef backtrace_full_callback __backtrace_full_cb; >+} >+#elif defined (_GLIBCXX_USE_C99_STDINT_TR1) >+# define _GLIBCXX_DEBUG_USE_LIBBACKTRACE 0 The interaction between _GLIBCXX_DEBUG_USE_LIBBACKTRACE==1 and _GLIBCXX_DEBUG_USE_LIBBACKTRACE==0 is a bit subtle and should be documented in a comment. If I'm reading this correctly, we will always define it (at least when USE_C99_STDINT_TR1 is defined ... which is always), but we don't always use libbacktrace. >+# include <stdint.h> // For uintptr_t. >+ >+namespace __gnu_debug >+{ >+ typedef int (*__backtrace_full_cb) (void*, uintptr_t, const char*, >+ int, const char*); I don't think we need this type. You can just use void(*)() as the callback argument. >+} >+#endif >+ > #if __cpp_rtti > # include <typeinfo> > # define _GLIBCXX_TYPEID(_Type) &typeid(_Type) >@@ -155,6 +180,16 @@ namespace __gnu_debug > __msg_irreflexive_ordering > }; > >+#ifdef _GLIBCXX_DEBUG_USE_LIBBACKTRACE >+ >+ void* >+ __create_backtrace_state(); >+ >+ void >+ __render_backtrace(void*, __backtrace_full_cb, void*); >+ >+#endif >+ > class _Error_formatter > { > // Tags denoting the type of parameter for construction >@@ -558,12 +593,14 @@ namespace __gnu_debug > _M_print_string(const char* __string) const _GLIBCXX_DEPRECATED; > #endif > >+#ifdef _GLIBCXX_DEBUG_USE_LIBBACKTRACE >+ void >+ _M_print_backtrace(__backtrace_full_cb __cb, void* __data) const; >+#endif >+ > private: > _Error_formatter(const char* __file, unsigned int __line, >- const char* __function) >- : _M_file(__file), _M_line(__line), _M_num_parameters(0), _M_text(0) >- , _M_function(__function) >- { } >+ const char* __function); > > #if !_GLIBCXX_INLINE_VERSION > void >@@ -578,6 +615,9 @@ namespace __gnu_debug > unsigned int _M_num_parameters; > const char* _M_text; > const char* _M_function; >+#ifdef _GLIBCXX_DEBUG_USE_LIBBACKTRACE >+ void* _M_backtrace_state; >+#endif This member could be defined unconditionally, and just initialize it to 0 in the constructor if not producing backtraces. > > public: > static _Error_formatter& >@@ -587,6 +627,36 @@ namespace __gnu_debug > return __formatter; > } > }; >+ >+#if _GLIBCXX_DEBUG_USE_LIBBACKTRACE >+ >+ void* >+ __create_backtrace_state() >+ { return backtrace_create_state(NULL, 0, NULL, NULL); } >+ >+ void >+ __render_backtrace(void* __bt, __backtrace_full_cb __cb, void* __data) >+ { backtrace_full((backtrace_state*)__bt, 1, __cb, 0, __data); } These two functions need to be 'inline' don't they? And if you pass void(*)() as the callback you can just cast it back to the right type here: void __render_backtrace(void* __bt, void (*__cb)(), void* __data) { backtrace_full((backtrace_state*)__bt, 1, (backtrace_full_callback)__cb, 0, __data); } That way you don't need the __backtrace_full_cb typedef defined anywhere. Pass around void(*)() and cast to the right type only when you actually use it, where we know that libbacktrace is supported. >+#endif >+ >+ _Error_formatter:: >+ _Error_formatter(const char* __file, unsigned int __line, >+ const char* __function) >+ : _M_file(__file), _M_line(__line), _M_num_parameters(0), _M_text(0) >+ , _M_function(__function) >+#ifdef _GLIBCXX_DEBUG_USE_LIBBACKTRACE >+ , _M_backtrace_state(__create_backtrace_state()) >+#endif >+ { } >+ >+#ifdef _GLIBCXX_DEBUG_USE_LIBBACKTRACE >+ void >+ _Error_formatter:: >+ _M_print_backtrace(__backtrace_full_cb __cb, void* __data) const >+ { __render_backtrace(_M_backtrace_state, __cb, __data); } >+#endif And these need to be inline too. Maybe just define them in the class body again? > } // namespace __gnu_debug > > #undef _GLIBCXX_TYPEID >diff --git a/libstdc++-v3/src/c++11/debug.cc b/libstdc++-v3/src/c++11/debug.cc >index 5a642097d17..790810fdab4 100644 >--- a/libstdc++-v3/src/c++11/debug.cc >+++ b/libstdc++-v3/src/c++11/debug.cc >@@ -34,16 +34,12 @@ > > #include <cassert> > #include <cstdio> >-#include <cctype> // for std::isspace >+#include <cctype> // for std::isspace. >+#include <cstring> // for std::strstr. > >-#include <algorithm> // for std::min >+#include <algorithm> // for std::min. > >-#include <cxxabi.h> // for __cxa_demangle >- >-// libstdc++/85768 >-#if 0 // defined _GLIBCXX_HAVE_EXECINFO_H >-# include <execinfo.h> // for backtrace >-#endif >+#include <cxxabi.h> // for __cxa_demangle. > > #include "mutex_pool.h" > >@@ -574,7 +570,7 @@ namespace > struct PrintContext > { > PrintContext() >- : _M_max_length(78), _M_column(1), _M_first_line(true), _M_wordwrap(false) >+ : _M_max_length(78), _M_column(1), _M_first_line(true), _M_wordwrap(false) > { get_max_length(_M_max_length); } > > std::size_t _M_max_length; >@@ -584,16 +580,17 @@ namespace > bool _M_wordwrap; > }; > >+ typedef void (*_Print_func_t) (PrintContext&, const char*, ptrdiff_t); This can use an alias-declaration to be more readable: using _Print_func_t = void (PrintContext&, const char*, ptrdiff_t); >+ void >+ print_function(PrintContext& ctx, const char* func, >+ _Print_func_t print_func) >+ { >+ const char cxx1998[] = "__cxx1998::"; >+ const char uglification[] = "__"; >+ for (;;) >+ { >+ auto idx1 = strstr(func, cxx1998); >+ auto idx2 = strstr(func, uglification); >+ if (idx1 || idx2) Checking idx1 is redundant, because if the func name contains "__cxx1998" then it also contains "__" >+ { >+ if (idx1 && (!idx2 || idx1 <= idx2)) It's impossible for idx1 && !idx2 to be true. It's also impossible for idx1 < idx2 to be true. I think this can be: if (auto idx = strstr(func, cxx1998)) { ... } else if (idx = strstr(func, uglification)) { ... } else >+ { >+ if (idx1 != func) >+ print_func(ctx, func, idx1 - func); >+ >+ func = idx1 + sizeof(cxx1998) - 1; >+ } >+ else if (idx2) >+ { >+ if (idx2 != func) >+ print_func(ctx, func, idx2 - func); >+ >+ func = idx2 + sizeof(uglification) - 1; >+ } >+ >+ while (*func && isspace(*func)) This needs to be isspace((unsigned char)*func), see https://en.cppreference.com/w/cpp/string/byte/isspace#Notes It's probably unlikely we'll get negative char values in these backtraces, but let's not risk it. >+ ++func; >+ >+ if (!*func) >+ break; >+ } >+ else >+ { >+ print_func(ctx, func, -1); >+ break; >+ } > } > } > >@@ -657,7 +701,10 @@ namespace > int status; > char* demangled_name = > __cxxabiv1::__cxa_demangle(info->name(), NULL, NULL, &status); >- print_word(ctx, status == 0 ? demangled_name : info->name()); >+ if (status == 0) >+ print_function(ctx, demangled_name, &print_word); >+ else >+ print_word(ctx, info->name(), -1); > free(demangled_name); > } > } >@@ -925,61 +972,62 @@ namespace > } > > void >- print_string(PrintContext& ctx, const char* string, >+ print_string(PrintContext& ctx, const char* str, ptrdiff_t nbc, > const _Parameter* parameters, std::size_t num_parameters) > { >- const char* start = string; >- const int bufsize = 128; >- char buf[bufsize]; >- int bufindex = 0; >+ const char* start = str; >+ const char* end = nbc >= 0 ? start + nbc : nullptr; >+ char buf[64]; > >- while (*start) >+ while ((end && str != end) || (!end && *str)) > { >- if (isspace(*start)) >+ if (isspace(*str)) Again, this needs to be isspace((unsigned char)*str). > { >- buf[bufindex++] = *start++; >- buf[bufindex] = '\0'; >- print_word(ctx, buf, bufindex); >- bufindex = 0; >+ ++str; >+ print_word(ctx, start, str - start); >+ start = str; > continue; > } > >- if (!num_parameters || *start != '%') >+ if (!parameters || *str != '%') > { > // Normal char or no parameter to look for. >- buf[bufindex++] = *start++; >+ ++str; > continue; > } > >- if (*++start == '%') >+ if (*++str == '%') > { > // Escaped '%' >- buf[bufindex++] = *start++; >+ print_word(ctx, start, str - start); >+ ++str; >+ start = str; > continue; > } All this string manipulation code scares me, this is exactly the kind of thing I'm talking about when I say that running extra code between detecting UB and terminating the process is risky. I know we already have this code, but it's exactly the kind of code where subtle bugs creep in and nobody notices :-( > // We are on a parameter property reference, we need to flush buffer > // first. >- if (bufindex != 0) >+ if (str != start) > { >- buf[bufindex] = '\0'; >- print_word(ctx, buf, bufindex); >- bufindex = 0; >+ // Avoid to print the '%'. s/Avoid to print/Avoid printing/ >+ if (str - start > 1) >+ print_word(ctx, start, str - start - 1); >+ start = str; > } > > // Get the parameter number >- assert(*start >= '1' && *start <= '9'); >- size_t param_index = *start - '0' - 1; >+ assert(*str >= '1' && *str <= '9'); >+ size_t param_index = *str - '0' - 1; > assert(param_index < num_parameters); > const auto& param = parameters[param_index]; > > // '.' separates the parameter number from the field > // name, if there is one. >- ++start; >- if (*start != '.') >+ ++str; >+ if (*str != '.') > { >- assert(*start == ';'); >- ++start; >+ assert(*str == ';'); >+ ++str; > if (param._M_kind == _Parameter::__integer) > { > int written >@@ -988,8 +1036,9 @@ namespace > print_word(ctx, buf, written); > } > else if (param._M_kind == _Parameter::__string) >- print_string(ctx, param._M_variant._M_string._M_value, >+ print_string(ctx, param._M_variant._M_string._M_value, -1, > parameters, num_parameters); >+ start = str; > continue; > } > >@@ -997,26 +1046,80 @@ namespace > const int max_field_len = 16; > char field[max_field_len]; > int field_idx = 0; >- ++start; >- while (*start != ';') >+ ++str; >+ while (*str != ';') > { >- assert(*start); >+ assert(*str); > assert(field_idx < max_field_len - 1); >- field[field_idx++] = *start++; >+ field[field_idx++] = *str++; > } >- ++start; >+ ++str; > field[field_idx] = '\0'; > > print_field(ctx, param, field); >+ start = str; > } > > // Might need to flush. >- if (bufindex) >+ if (str != start) >+ print_word(ctx, start, str - start); >+ } >+ >+ void >+ print_string(PrintContext& ctx, const char* str, ptrdiff_t nbc) >+ { print_string(ctx, str, nbc, nullptr, 0); } >+ >+#ifdef _GLIBCXX_DEBUG_USE_LIBBACKTRACE >+ int >+ print_backtrace(void* data, uintptr_t pc, const char* filename, >+ int lineno, const char* function) >+ { >+ const int bufsize = 64; >+ char buf[bufsize]; >+ >+ PrintContext& ctx = *static_cast<PrintContext*>(data); >+ >+ int written = __builtin_sprintf(buf, "%p ", (void*)pc); >+ print_word(ctx, buf, written); >+ >+ int ret = 0; >+ if (function) > { >- buf[bufindex] = '\0'; >- print_word(ctx, buf, bufindex); >+ int status; >+ char* demangled_name = >+ __cxxabiv1::__cxa_demangle(function, NULL, NULL, &status); >+ if (status == 0) >+ print_function(ctx, demangled_name, &print_raw); >+ else >+ print_word(ctx, function); >+ >+ free(demangled_name); >+ ret = strstr(function, "main") ? 1 : 0; > } >+ >+ print_literal(ctx, "\n"); >+ >+ if (filename) >+ { >+ bool wordwrap = false; >+ swap(wordwrap, ctx._M_wordwrap); >+ print_word(ctx, filename); >+ >+ if (lineno) >+ { >+ written = __builtin_sprintf(buf, ":%u\n", lineno); >+ print_word(ctx, buf, written); >+ } >+ else >+ print_literal(ctx, "\n"); >+ swap(wordwrap, ctx._M_wordwrap); >+ } >+ else >+ print_literal(ctx, "???:0\n"); >+ >+ return ret; > } >+#endif > } > > namespace __gnu_debug >@@ -1058,41 +1161,27 @@ namespace __gnu_debug > if (_M_function) > { > print_literal(ctx, "In function:\n"); >- print_string(ctx, _M_function, nullptr, 0); >+ print_function(ctx, _M_function, &print_string); > print_literal(ctx, "\n"); > ctx._M_first_line = true; > print_literal(ctx, "\n"); > } > >-// libstdc++/85768 >-#if 0 //defined _GLIBCXX_HAVE_EXECINFO_H >- { >- void* stack[32]; >- int nb = backtrace(stack, 32); >- >- // Note that we skip current method symbol. >- if (nb > 1) >- { >- print_literal(ctx, "Backtrace:\n"); >- auto symbols = backtrace_symbols(stack, nb); >- for (int i = 1; i < nb; ++i) >- { >- print_word(ctx, symbols[i]); >- print_literal(ctx, "\n"); >- } >- >- free(symbols); >- ctx._M_first_line = true; >- print_literal(ctx, "\n"); >- } >- } >+#ifdef _GLIBCXX_DEBUG_USE_LIBBACKTRACE >+ if (_M_backtrace_state) >+ { >+ print_literal(ctx, "Backtrace:\n"); >+ _M_print_backtrace(print_backtrace, &ctx); If libstdc++.so gets compiled on a machine without libacktrace installed, then _GLIBCXX_DEBUG_USE_LIBBACKTRACE will be defined to 0 here. How do you prevent _M_print_backtrace from being inlined, using the no-op definition of __render_backtrace defined in this file? I think you might want to defined _M_print_backtrace as: __attribute__((__noinline__)) inline void _Error_formatter:: _M_print_backtrace(void (*__cb)(), void* __data) const { __render_backtrace(_M_backtrace_state, (backtrace_full_callback)__cb, __data); } The __noinline__ attribute will ensure it isn't inlined into src/c++11/debug.cc and that a definition in the application can be interposed in place of the one in libstdc++.so I think. >+ ctx._M_first_line = true; >+ print_literal(ctx, "\n"); >+ } > #endif > > print_literal(ctx, "Error: "); > > // Print the error message > assert(_M_text); >- print_string(ctx, _M_text, _M_parameters, _M_num_parameters); >+ print_string(ctx, _M_text, -1, _M_parameters, _M_num_parameters); > print_literal(ctx, ".\n"); > > // Emit descriptions of the objects involved in the operation >@@ -1123,6 +1212,18 @@ namespace __gnu_debug > abort(); > } > >+#ifdef _GLIBCXX_DEBUG_USE_LIBBACKTRACE >+ >+ _GLIBCXX_WEAK_DEFINITION void* >+ __create_backtrace_state() >+ { return nullptr; } >+ >+ _GLIBCXX_WEAK_DEFINITION void >+ __render_backtrace(void*, __backtrace_full_cb, void*) >+ { } >+ What happens if the target doesn't support weak symbols? Should the use of libbacktrace be conditional on whether weak symbols are supported? Or do we even need these functions at all? Would this work? __attribute__((__noinline__)) inline void _Error_formatter:: _M_print_backtrace(void (*__cb)(), void* __data) const { #if _GLIBCXX_DEBUG_USE_LIBBACKTRACE backtrace_full((backtrace_state*)_M_backtrace_state, 1, (backtrace_full_callback)__cb, 0, __data); #endif } And for the constructor: __attribute__((__noinline__)) inline void _Error_formatter:: _Error_formatter(const char* __file, unsigned int __line, const char* __function) : _M_file(__file), _M_line(__line), _M_num_parameters(0), _M_text(0) , _M_function(__function) , _M_backtrace_state(0) { #if _GLIBCXX_DEBUG_USE_LIBBACKTRACE _M_backtrace_state = backtrace_create_state(NULL, 0, NULL, NULL); #endif } I think this will mean that the application code will use backtrace_create_state when constructing an _Error_formatter and then the library code will call _M_print_backtrace, which will use the first definition that is found by the linker. >+#endif >+ > #if !_GLIBCXX_INLINE_VERSION > // Deprecated methods kept for backward compatibility. > void >diff --git a/libstdc++-v3/testsuite/util/testsuite_abi.cc b/libstdc++-v3/testsuite/util/testsuite_abi.cc >index 3af5dc593c2..2c3204e57a5 100644 >--- a/libstdc++-v3/testsuite/util/testsuite_abi.cc >+++ b/libstdc++-v3/testsuite/util/testsuite_abi.cc >@@ -210,6 +210,7 @@ check_version(symbol& test, bool added) > known_versions.push_back("GLIBCXX_3.4.27"); > known_versions.push_back("GLIBCXX_3.4.28"); > known_versions.push_back("GLIBCXX_3.4.29"); >+ known_versions.push_back("GLIBCXX_3.4.30"); > known_versions.push_back("GLIBCXX_LDBL_3.4.29"); > known_versions.push_back("GLIBCXX_IEEE128_3.4.29"); > known_versions.push_back("CXXABI_1.3"); You also need to update the latestp variable: // Check that added symbols are added in the latest pre-release version. bool latestp = (test.version_name == "GLIBCXX_3.4.29" And if you add a new symbol version then you need to update libtool_VERSION in acinclude.m4 and regenerate the autoconf templates, see https://gcc.gnu.org/onlinedocs/libstdc++/manual/appendix_porting.html#build_hacking.configure.version ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH][_GLIBCXX_DEBUG] libbacktrace integration 2021-05-05 11:33 ` Jonathan Wakely @ 2021-05-07 14:26 ` Jonathan Wakely 2021-05-09 15:03 ` François Dumont 0 siblings, 1 reply; 8+ messages in thread From: Jonathan Wakely @ 2021-05-07 14:26 UTC (permalink / raw) To: François Dumont; +Cc: libstdc++, gcc-patches On 05/05/21 12:33 +0100, Jonathan Wakely wrote: >On 24/04/21 15:46 +0200, François Dumont via Libstdc++ wrote: >>Hi >> >> Here is the patch to add backtrace generation on _GLIBCXX_DEBUG >>assertions thanks to libbacktrace. Ville pointed out that we'll need to use libbacktrace for std::stacktrace <https://wg21.link/P0881R7> anyway, and it would be useful if/when we add support for C++ Contracts to the lirbary. So let's integrate libbacktrace into libstdc++ properly. Jakub suggested doing it how libsanitizer does it, which is to rebuild the libbacktrace sources as part of the libsanitizer build, using the preprocessor to rename the symbols so that they use reserved names. e.g. rename backtrace_full to __glibcxx_backtrace_full or something like that. I'll work on getting it building as part of libstdc++ (or maybe as a separate static library for now, as we do for libstdc++fs.a) and then you can rework your Debug Mode patch to depend on the private version of libbacktrace included with libstdc++ (instead of expecting users to provide it themselves). ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH][_GLIBCXX_DEBUG] libbacktrace integration 2021-05-07 14:26 ` Jonathan Wakely @ 2021-05-09 15:03 ` François Dumont 0 siblings, 0 replies; 8+ messages in thread From: François Dumont @ 2021-05-09 15:03 UTC (permalink / raw) To: Jonathan Wakely; +Cc: libstdc++, gcc-patches On 07/05/21 4:26 pm, Jonathan Wakely wrote: > On 05/05/21 12:33 +0100, Jonathan Wakely wrote: >> On 24/04/21 15:46 +0200, François Dumont via Libstdc++ wrote: >>> Hi >>> >>> Here is the patch to add backtrace generation on _GLIBCXX_DEBUG >>> assertions thanks to libbacktrace. > > Ville pointed out that we'll need to use libbacktrace for > std::stacktrace <https://wg21.link/P0881R7> anyway, and it would be > useful if/when we add support for C++ Contracts to the lirbary. > > So let's integrate libbacktrace into libstdc++ properly. Jakub > suggested doing it how libsanitizer does it, which is to rebuild the > libbacktrace sources as part of the libsanitizer build, using the > preprocessor to rename the symbols so that they use reserved names. > e.g. rename backtrace_full to __glibcxx_backtrace_full or something > like that. > > I'll work on getting it building as part of libstdc++ (or maybe as a > separate static library for now, as we do for libstdc++fs.a) and then > you can rework your Debug Mode patch to depend on the private version > of libbacktrace included with libstdc++ (instead of expecting users to > provide it themselves). > Ok, thanks for the info. I will perhaps extract the non-backtrace related stuff from this patch then and I'll rework the backtrace part later. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-05-09 15:03 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-04-24 13:46 [PATCH][_GLIBCXX_DEBUG] libbacktrace integration François Dumont 2021-05-03 20:17 ` François Dumont 2021-05-03 21:06 ` Jonathan Wakely 2021-05-04 6:03 ` François Dumont 2021-05-05 10:01 ` Jonathan Wakely 2021-05-05 11:33 ` Jonathan Wakely 2021-05-07 14:26 ` Jonathan Wakely 2021-05-09 15:03 ` François Dumont
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).