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 <jwak...@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: ");
