libbacktrace integration for _GLIBCXX_DEBUG mode
Jonathan Wakely
jwakely@redhat.com
Wed May 29 19:47:00 GMT 2019
On 23/05/19 07:39 +0200, François Dumont wrote:
>Hi
>
>Â Â Â So here what I come up with.
>
>Â Â Â _GLIBCXX_DEBUG_BACKTRACE controls the feature. If the user define
>it and there is a detectable issue with libbacktrace then I generate a
>compilation error. I want to avoid users defining it but having no
>backtrace in the end in the debug assertion.
>
>Â Â Â With this new setup I manage to run testsuite with it like that:
>
>export LD_LIBRARY_PATH=/home/fdt/dev/gcc/install/lib/
>make CXXFLAGS='-D_GLIBCXX_DEBUG_BACKTRACE
>-I/home/fdt/dev/gcc/install/include -lbacktrace' check-debug
>
>Â Â Â An example of result:
>
>/home/fdt/dev/gcc/build/x86_64-pc-linux-gnu/libstdc++-v3/include/debug/vector:606:
>In function:
>Â Â Â std::__debug::vector<_Tp, _Allocator>::iterator
>Â Â Â std::__debug::vector<_Tp,
>_Allocator>::insert(std::__debug::vector<_Tp,
>Â Â Â _Allocator>::const_iterator, _InputIterator, _InputIterator) [with
>Â Â Â _InputIterator = int*; <template-parameter-2-2> = void; _Tp = int;
>Â Â Â _Allocator = std::allocator<int>; std::__debug::vector<_Tp,
>Â Â Â _Allocator>::iterator =
>__gnu_debug::_Safe_iterator<__gnu_cxx::__normal_iterator<int*, std::
>Â Â Â vector<int> >, std::__debug::vector<int>,
>Â Â Â std::random_access_iterator_tag>; typename
>std::iterator_traits<typename
>Â Â Â std::vector<_Tp, _Alloc>::iterator>::iterator_category =
>Â Â Â std::random_access_iterator_tag; typename std::vector<_Tp,
>Â Â Â _Alloc>::iterator = __gnu_cxx::__normal_iterator<int*,
>std::vector<int>
>Â Â Â >; std::__debug::vector<_Tp, _Allocator>::const_iterator =
>__gnu_debug::_Safe_iterator<__gnu_cxx::__normal_iterator<const int*,
>Â Â Â std::vector<int> >, std::__debug::vector<int>,
>Â Â Â std::random_access_iterator_tag>; typename
>std::iterator_traits<typename
>Â Â Â std::vector<_Tp, _Alloc>::const_iterator>::iterator_category =
>Â Â Â std::random_access_iterator_tag; typename std::vector<_Tp,
>Â Â Â _Alloc>::const_iterator = __gnu_cxx::__normal_iterator<const int*,
>std::
>Â Â Â vector<int> >]
>
>Backtrace:
>Â Â Â 0x402718
>__gnu_debug::_Safe_iterator<__gnu_cxx::__normal_iterator<int*,
>std::vector<int> >> std::__debug::vector<int>::insert<int*,
>void>(__gnu_debug::_Safe_iterator<__gnu_cxx::__normal_iterator<int
>const*, std::vector<int> >>, int*, int*)
>/home/fdt/dev/gcc/build/x86_64-pc-linux-gnu/libstdc++-v3/include/debug/vector:606
>Â Â Â 0x402718 test01()
>/home/fdt/dev/gcc/git/libstdc++-v3/testsuite/23_containers/vector/debug/57779_neg.cc:29
>Â Â Â 0x401428 main
>/home/fdt/dev/gcc/git/libstdc++-v3/testsuite/23_containers/vector/debug/57779_neg.cc:34
>
>Error: attempt to insert with an iterator range [__first, __last) from this
>container.
>
>Objects involved in the operation:
>Â Â Â iterator "__first" @ 0x0x7fff730b96b0 {
>Â Â Â Â Â type = int* (mutable iterator);
>Â Â Â }
>Â Â Â iterator "__last" @ 0x0x7fff730b96b8 {
>Â Â Â Â Â type = int* (mutable iterator);
>Â Â Â }
>Â Â Â sequence "this" @ 0x0x7fff730b9720 {
>Â Â Â Â Â type = std::__debug::vector<int>;
>Â Â Â }
>XFAIL: 23_containers/vector/debug/57779_neg.cc execution test
>
>
>Â Â Â * include/debug/formatter.h [_GLIBCXX_DEBUG_BACKTRACE]: Include
>Â Â Â <backtrace-supported.h> and <backtrace.h>.
>Â Â Â [!_GLIBCXX_DEBUG_BACKTRACE]: Include <stdint.h>.
>Â Â Â [!_GLIBCXX_DEBUG_BACKTRACE](backtrace_error_callback): New.
>Â Â Â [!_GLIBCXX_DEBUG_BACKTRACE](backtrace_full_callback): New.
>Â Â Â [!_GLIBCXX_DEBUG_BACKTRACE](struct backtrace_state): New declaration.
>Â Â Â (_Error_formatter::_Bt_full_t): New function pointer type.
>Â Â Â (_Error_formatter::_M_print_backtrace): New.
>Â Â Â (_Error_formatter::_M_backtrace_state): New.
>Â Â Â (_Error_formatter::_M_backtrace_full_func): New.
>Â Â Â * src/c++11/debug.cc: Include <cstring> and <string>.
>Â Â Â (PrintContext::_M_demangle_name): New.
>Â Â Â (_Print_func_t): New.
>Â Â Â (print_word(PrintContext&, const char*)): New.
>Â Â Â (print_raw(PrintContext&, const char*)): New.
>Â Â Â (print_function(PrintContext&, const char*, _Print_func_t)): New.
>Â Â Â (print_type): Use latter.
>Â Â Â (print_string(PrintContext&, const char*)): New.
>Â Â Â (print_backtrace(void*, uintptr_t, const char*, int, const char*)):
>Â Â Â New.
>Â Â Â (_Error_formatter::_M_error()): Adapt.
>Â Â Â * doc/xml/manual/debug_mode.xml: Document _GLIBCXX_DEBUG_BACKTRACE.
>
>Tested under Linux x86_64.
>
>Ok to commit ?
>
>François
>
>
>On 12/21/18 10:03 PM, Jonathan Wakely wrote:
>>On 21/12/18 22:47 +0200, Ville Voutilainen wrote:
>>>On Fri, 21 Dec 2018 at 22:35, Jonathan Wakely <jwakely@redhat.com>
>>>wrote:
>>>>>Â Â Â I also explcitely define BACKTRACE_SUPPORTED to 0 to make sure
>>>>>libstdc++ has no libbacktrace dependency after usual build.
>>>
>>>>I'm concerned about the requirement to link to libbacktrace
>>>>explicitly (which will break existing makefiles and build systems that
>>>>currently use debug mode in testing).
>>>
>>>But see what Francois wrote, "I also explcitely define
>>>BACKTRACE_SUPPORTED to 0 to make sure
>>>libstdc++ has no libbacktrace dependency after usual build."
>>
>>Yes, but if you happen to install libbacktrace headers, the behaviour
>>for users building their own code changes. I agree that if you install
>>those headers, it's probably for a reason, but it might be a different
>>reason to "so that libstdc++ prints better backtraces".
>>
>>>>Also, some of the glibc team pointed out to me that running *any*
>>>>extra code after undefined behaviour has been detected is a potential
>>>>risk. The less that you do between detecting UB and calling abort(),
>>>>the better. Giving the users more information is helpful, but comes
>>>>with some additional risk.
>>>
>>>Ditto. Having said those things, I think we need to figure out a good
>>>way to provide this sensibly
>>>as an opt-in. The backtrace support is bloody useful, and dovetails
>>>into a possible Contracts-aware
>>>implementation of our library, but I think we need to do some more
>>>thought-work on this, thus I agree
>>>that it's not stage3 material. I do think it's something that we need
>>>to keep in mind, thanks
>>>for working on it, Francois!
>>
>>Yes, I agree that making it available via a more explicit opt-in would
>>be good. Maybe require users to define _GLIBCXX_DEBUG_BACKTRACE as well
>>as _GLIBCXX_DEBUG, or something like that.
>>
>>
>>
>
>diff --git a/libstdc++-v3/doc/xml/manual/debug_mode.xml b/libstdc++-v3/doc/xml/manual/debug_mode.xml
>index 570c17ba28a..27873151dae 100644
>--- a/libstdc++-v3/doc/xml/manual/debug_mode.xml
>+++ b/libstdc++-v3/doc/xml/manual/debug_mode.xml
>@@ -104,9 +104,11 @@
> <para>The following library components provide extra debugging
> capabilities in debug mode:</para>
> <itemizedlist>
>+ <listitem><para><code>std::array</code> (no safe iterators)</para></listitem>
> <listitem><para><code>std::basic_string</code> (no safe iterators and see note below)</para></listitem>
> <listitem><para><code>std::bitset</code></para></listitem>
> <listitem><para><code>std::deque</code></para></listitem>
>+ <listitem><para><code>std::forward_list</code></para></listitem>
> <listitem><para><code>std::list</code></para></listitem>
> <listitem><para><code>std::map</code></para></listitem>
> <listitem><para><code>std::multimap</code></para></listitem>
>@@ -160,6 +162,13 @@ which always works correctly.
> <code>GLIBCXX_DEBUG_MESSAGE_LENGTH</code> can be used to request a
> different length.</para>
>
>+<para>Starting with GCC 10 libstdc++ has integrated
>+ <link xmlns:xlink="http://www.w3.org/1999/xlink"
>+ xlink:href="https://github.com/ianlancetaylor/libbacktrace">libbacktrace</link>
>+ to produce backtrace on error. Use <code>-D_GLIBCXX_DEBUG_BACKTRACE</code> to
>+ activate it. Note that if not properly installed or if libbacktrace is not
>+ supported compilation will fail. You'll also have to use the
>+ <code>-lbacktrace</code> 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/include/debug/formatter.h b/libstdc++-v3/include/debug/formatter.h
>index 220379994c0..690750f42be 100644
>--- a/libstdc++-v3/include/debug/formatter.h
>+++ b/libstdc++-v3/include/debug/formatter.h
>@@ -31,6 +31,29 @@
>
> #include <bits/c++config.h>
>
>+#if defined(_GLIBCXX_DEBUG_BACKTRACE)
>+# if !defined(BACKTRACE_SUPPORTED)
>+# if defined(__has_include) && !__has_include(<backtrace-supported.h>)
>+# error No libbacktrace backtrace-supported.h file found.
>+# endif
>+# include <backtrace-supported.h>
>+# endif
>+# if !BACKTRACE_SUPPORTED
>+# error libbacktrace not supported.
>+# endif
>+# include <backtrace.h>
>+#else
>+# include <stdint.h> // For uintptr_t.
>+
>+// Extracted from libbacktrace.
>+typedef void (*backtrace_error_callback) (void*, const char*, int);
>+
>+typedef int (*backtrace_full_callback) (void*, uintptr_t, const char*, int,
>+ const char*);
>+
>+struct backtrace_state;
>+#endif
>+
> #if __cpp_rtti
> # include <typeinfo>
> # define _GLIBCXX_TYPEID(_Type) &typeid(_Type)
>@@ -156,6 +179,9 @@ namespace __gnu_debug
>
> class _Error_formatter
> {
>+ typedef int (*_Bt_full_t) (backtrace_state*, int, backtrace_full_callback,
>+ backtrace_error_callback, void*);
>+
> // Tags denoting the type of parameter for construction
> struct _Is_iterator { };
> struct _Is_iterator_value_type { };
>@@ -558,11 +584,21 @@ namespace __gnu_debug
> _M_print_string(const char* __string) const _GLIBCXX_DEPRECATED;
> #endif
>
>+ void
>+ _M_print_backtrace(backtrace_full_callback __cb, void* __data) const
>+ { _M_backtrace_full_func(_M_backtrace_state, 1, __cb, 0, __data); }
>+
> 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)
>+#if BACKTRACE_SUPPORTED
>+ , _M_backtrace_state(backtrace_create_state(NULL, 0, NULL, NULL))
>+ , _M_backtrace_full_func(&backtrace_full)
>+#else
>+ , _M_backtrace_state()
>+#endif
> { }
>
> #if !_GLIBCXX_INLINE_VERSION
>@@ -578,6 +614,8 @@ namespace __gnu_debug
> unsigned int _M_num_parameters;
> const char* _M_text;
> const char* _M_function;
>+ backtrace_state* _M_backtrace_state;
>+ _Bt_full_t _M_backtrace_full_func;
>
> public:
> static _Error_formatter&
>diff --git a/libstdc++-v3/src/c++11/debug.cc b/libstdc++-v3/src/c++11/debug.cc
>index f5a49992efa..aa7c17248fe 100644
>--- a/libstdc++-v3/src/c++11/debug.cc
>+++ b/libstdc++-v3/src/c++11/debug.cc
>@@ -34,16 +34,13 @@
>
> #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 <string>
>
>-#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"
>
>@@ -570,7 +567,8 @@ 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)
>+ , _M_demangle_name(false)
> { get_max_length(_M_max_length); }
>
> std::size_t _M_max_length;
>@@ -578,16 +576,18 @@ namespace
> std::size_t _M_column;
> bool _M_first_line;
> bool _M_wordwrap;
>+ bool _M_demangle_name;
> };
>
>+ typedef void (*_Print_func_t) (PrintContext&, const char*);
>+
> 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, std::ptrdiff_t count)
> {
> size_t length = count >= 0 ? count : __builtin_strlen(word);
> if (length == 0)
>@@ -623,7 +623,7 @@ namespace
> ctx._M_column += ctx._M_indent;
> }
>
>- int written = fprintf(stderr, "%s", word);
>+ int written = fprintf(stderr, word);
I don't think this is safe. The words being printed come from the
user's code, so could be controlled by an attacker. If a word contains
a printf format specifier like %s then your change will make it
dereference garbage. Please do not make this change.
> if (word[length - 1] == '\n')
> {
>@@ -640,6 +640,133 @@ namespace
> }
> }
>
>+ void
>+ print_word(PrintContext& ctx, const char* word)
>+ { print_word(ctx, word, -1); }
>+
>+ void
>+ print_raw(PrintContext&, const char* word)
>+ { fprintf(stderr, word); }
This is unsafe too, this should be fprintf(stderr, "%s", word).
>+
>+ void
>+ print_function(PrintContext& ctx, const char* function,
>+ _Print_func_t print_func)
>+ {
>+ const string cxx1998 = "__cxx1998::";
>+ const string allocator = ", std::allocator<";
>+ const string safe_iterator = "__gnu_debug::_Safe_iterator<";
These should be string_view objects, not strings that allocate memory.
This code runs when the program has encountered undefined behaviour,
we should be running as little code as possible, and avoiding memory
allocations if possible (and string_view is just a better option here
anyway). If you need to compile this file as C++14 to use
std::experimental::string_view or C++17 to use std::string_view, then
do that. Do't repeatedly allocate strings every time print_function
gets called. Especially as that allocation could fail, which would
then leak the demangled_name string in the caller.
>+ string func(function);
>+ string::size_type index = 0;
>+ for (;;)
>+ {
>+ auto idx1 = func.find(cxx1998, index);
>+ auto idx2 = func.find(allocator, index);
>+ auto idx3 = ctx._M_demangle_name
>+ ? func.find(safe_iterator, index) : string::npos;
>+ if (idx1 != string::npos || idx2 != string::npos ||
>+ idx3 != string::npos)
>+ {
>+ if (idx1 != string::npos
>+ && (idx2 == string::npos || idx1 < idx2)
>+ && (idx3 == string::npos || idx1 < idx3))
>+ {
>+ func[idx1] = '\0';
>+ print_func(ctx, func.c_str() + index);
>+ index = idx1 + cxx1998.size();
>+ }
>+ else if (idx2 != string::npos
>+ && (idx3 == string::npos || idx2 < idx3))
>+ {
>+ func[idx2] = '\0';
>+ print_func(ctx, func.c_str() + index);
>+
>+ // We need to look for the closing '>'
>+ idx2 += allocator.size();
>+ int open_bracket = 0;
>+ for (; idx2 != func.size(); ++idx2)
>+ {
>+ if (func[idx2] == '>')
>+ {
>+ if (open_bracket)
>+ --open_bracket;
>+ else
>+ break;
>+ }
>+ else if (func[idx2] == '<')
>+ ++open_bracket;
>+ }
Will this work if a template argument is an expression like (sizeof(T) < 4) ?
>+
>+ index = idx2 + 1;
>+ idx2 = func.find(" const&", index);
>+ if (idx2 == index)
>+ // Allocator parameter qualification, skipped too.
>+ index += sizeof(" const&") - 1;
>+ }
>+ else if (idx3 != string::npos)
>+ {
>+ // Just keep the 1st template parameter.
>+ // Look for 1st comma outside any additional brackets.
>+ idx3 += safe_iterator.size();
>+ char c = func[idx3];
>+ func[idx3] = '\0';
>+ print_func(ctx, func.c_str() + index);
>+ func[idx3] = c;
>+ index = idx3;
>+ int nb_open_bracket = 0;
>+ for (; nb_open_bracket != -1;)
>+ {
>+ auto n = func.find_first_of("<,>", idx3);
>+ switch (func[n])
>+ {
>+ case '<':
>+ ++nb_open_bracket;
>+ break;
>+ case '>':
>+ --nb_open_bracket;
>+ break;
>+ case ',':
>+ if (nb_open_bracket == 0)
>+ {
>+ func[n] = '\0';
>+ print_function(ctx, func.c_str() + index, print_func);
>+ nb_open_bracket = -1;
>+ }
>+
>+ break;
>+ }
>+
>+ idx3 = n + 1;
>+ }
>+
>+ // Now look for the closing '>'.
>+ nb_open_bracket = 0;
>+ for (; idx3 != func.size(); ++idx3)
>+ {
>+ if (func[idx3] == '>')
>+ {
>+ if (nb_open_bracket)
>+ --nb_open_bracket;
>+ else
>+ break;
>+ }
>+ else if (func[idx3] == '<')
>+ ++nb_open_bracket;
>+ }
>+
>+ index = idx3;
>+ }
>+
>+ while (isspace(func[index]))
>+ ++index;
This is a lot of code with no error checking that is going to run when
the program has encountered undefined behaviour. That's worrying.
>+ }
>+ else
>+ {
>+ print_func(ctx, func.c_str() + index);
>+ break;
>+ }
>+ }
>+ }
>+
> template<size_t Length>
> void
> print_type(PrintContext& ctx,
>@@ -653,7 +780,13 @@ 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)
>+ {
>+ ctx._M_demangle_name = true;
>+ print_function(ctx, demangled_name, &print_word);
>+ }
>+ else
>+ print_word(ctx, info->name());
> free(demangled_name);
> }
> }
>@@ -921,10 +1054,10 @@ namespace
> }
>
> void
>- print_string(PrintContext& ctx, const char* string,
>+ print_string(PrintContext& ctx, const char* str,
> const _Parameter* parameters, std::size_t num_parameters)
> {
>- const char* start = string;
>+ const char* start = str;
> const int bufsize = 128;
> char buf[bufsize];
> int bufindex = 0;
>@@ -1013,6 +1146,63 @@ namespace
> print_word(ctx, buf, bufindex);
> }
> }
>+
>+ void
>+ print_string(PrintContext& ctx, const char* str)
>+ { print_string(ctx, str, nullptr, 0); }
>+
>+ 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)
>+ {
>+ int status;
>+ char* demangled_name =
>+ __cxxabiv1::__cxa_demangle(function, NULL, NULL, &status);
>+ if (status == 0)
>+ {
>+ ctx._M_demangle_name = true;
>+ 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;
>+ }
> }
>
> namespace __gnu_debug
>@@ -1054,35 +1244,19 @@ 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");
>- }
>- }
>-#endif
>+ if (_M_backtrace_state)
>+ {
>+ print_literal(ctx, "Backtrace:\n");
>+ _M_print_backtrace(print_backtrace, &ctx);
>+ ctx._M_first_line = true;
>+ print_literal(ctx, "\n");
>+ }
>
> print_literal(ctx, "Error: ");
>
More information about the Libstdc++
mailing list