* [_GLIBCXX_DEBUG] Enhance rendering of assert message @ 2021-05-22 20:08 François Dumont 2021-05-25 9:58 ` Jonathan Wakely 0 siblings, 1 reply; 4+ messages in thread From: François Dumont @ 2021-05-22 20:08 UTC (permalink / raw) To: libstdc++; +Cc: gcc-patches [-- Attachment #1: Type: text/plain, Size: 2499 bytes --] Here is the part of the libbacktrace patch with the enhancement to the rendering of assert message. It only contains one real fix, the rendering of address. In 2 places it was done with "0x%p", so resulting in something like: 0x0x012345678 Otherwise it is just enhancements, mostly avoiding intermediate buffering. I am adding the _Parameter::_Named type to help on the rendering. I hesitated in doing the same for the _M_iterator type but eventually managed it with a template function. libstdc++: [_GLIBCXX_DEBUG] Enhance rendering of assert message Avoid building an intermediate buffer to print to stderr, push directly to stderr. libstdc++-v3/ChangeLog: * include/debug/formatter.h (_Error_formatter::_Parameter::_Named): New. (_Error_formatter::_Parameter::_Type): Inherit latter. (_Error_formatter::_Parameter::_M_integer): Likewise. (_Error_formatter::_Parameter::_M_string): Likewise. * src/c++11/debug.cc: Include <cstring>. (_Print_func_t): New. (print_raw(PrintContext&, const char*, ptrdiff_t)): New. (print_word): Use '%.*s' format in fprintf to render only expected number of chars. (pretty_print(PrintContext&, const char*, _Print_func_t)): New. (print_type): Rename in... (print_type_info): ...this. Use pretty_print. (print_address, print_integer): New. (print_named_name, print_iterator_constness, print_iterator_state): New. (print_iterator_seq_type): New. (print_named_field, print_type_field, print_instance_field, print_iterator_field): New. (print_field): Use latters. (print_quoted_named_name, print_type_type, print_type, print_instance): New. (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. Ok to commit ? François [-- Attachment #2: debug_mode.patch --] [-- Type: text/x-patch, Size: 21116 bytes --] diff --git a/libstdc++-v3/include/debug/formatter.h b/libstdc++-v3/include/debug/formatter.h index 7140fed5e83..a83d33a4cb8 100644 --- a/libstdc++-v3/include/debug/formatter.h +++ b/libstdc++-v3/include/debug/formatter.h @@ -202,9 +202,13 @@ namespace __gnu_debug __iterator_value_type } _M_kind; - struct _Type + struct _Named { const char* _M_name; + }; + + struct _Type : _Named + { const type_info* _M_type; }; @@ -228,16 +232,14 @@ namespace __gnu_debug _Instance _M_sequence; // When _M_kind == __integer - struct + struct : _Named { - const char* _M_name; long _M_value; } _M_integer; // When _M_kind == __string - struct + struct : _Named { - const char* _M_name; const char* _M_value; } _M_string; diff --git a/libstdc++-v3/src/c++11/debug.cc b/libstdc++-v3/src/c++11/debug.cc index 5a642097d17..8b6ad73f950 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,26 @@ namespace bool _M_wordwrap; }; + using _Print_func_t = void (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_raw(PrintContext& ctx, const char* str, ptrdiff_t nbc = -1) { - size_t length = count >= 0 ? count : __builtin_strlen(word); + if (nbc >= 0) + ctx._M_column += fprintf(stderr, "%.*s", (int)nbc, str); + else + ctx._M_column += fprintf(stderr, "%s", str); + } + + void + print_word(PrintContext& ctx, const char* word, ptrdiff_t nbc = -1) + { + size_t length = nbc >= 0 ? nbc : __builtin_strlen(word); if (length == 0) return; @@ -610,7 +616,7 @@ namespace } size_t visual_length - = isspace(word[length - 1]) ? length - 1 : length; + = isspace((unsigned char)word[length - 1]) ? length - 1 : length; if (visual_length == 0 || !ctx._M_wordwrap || (ctx._M_column + visual_length < ctx._M_max_length) @@ -619,15 +625,11 @@ 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'; - fprintf(stderr, "%s", spacing); - ctx._M_column += ctx._M_indent; + const char spacing[ctx._M_indent + 1] = " "; + print_raw(ctx, spacing, ctx._M_indent); } - int written = fprintf(stderr, "%s", word); + int written = fprintf(stderr, "%.*s", (int)length, word); if (word[length - 1] == '\n') { @@ -640,15 +642,48 @@ namespace else { print_literal(ctx, "\n"); - print_word(ctx, word, count); + print_word(ctx, word, nbc); + } + } + + void + pretty_print(PrintContext& ctx, const char* str, _Print_func_t print_func) + { + const char cxx1998[] = "__cxx1998::"; + const char uglification[] = "__"; + for (;;) + { + auto idx = strstr(str, uglification); + if (idx) + { + size_t offset = + (idx == strstr(str, cxx1998) + ? sizeof(cxx1998) : sizeof(uglification)) - 1; + + if (idx != str) + print_func(ctx, str, idx - str); + + str = idx + offset; + + while (*str && isspace((unsigned char)*str)) + ++str; + + if (!*str) + break; + } + else + { + print_func(ctx, str, -1); + break; + } } } template<size_t Length> void - print_type(PrintContext& ctx, - const type_info* info, - const char(&unknown_name)[Length]) + print_type_info(PrintContext& ctx, + const type_info* info, + const char(&unknown_name)[Length]) { if (!info) print_literal(ctx, unknown_name); @@ -657,22 +692,86 @@ 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) + pretty_print(ctx, demangled_name, &print_word); + else + print_word(ctx, info->name()); free(demangled_name); } } + void + print_address(PrintContext& ctx, const char* fmt, const void* address) + { + char buf[128]; + int written = __builtin_sprintf(buf, fmt, address); + print_word(ctx, buf, written); + } + + void + print_address(PrintContext& ctx, const void* address) + { print_address(ctx, "%p", address); } + + void + print_integer(PrintContext& ctx, long integer) + { + char buf[64]; + int written = __builtin_sprintf(buf, "%ld", integer); + print_word(ctx, buf, written); + } + + void + print_named_name(PrintContext& ctx, const _Parameter::_Named& named) + { + assert(named._M_name); + pretty_print(ctx, named._M_name, print_word); + } + + template<typename _Iterator> + void + print_iterator_constness(PrintContext& ctx, const _Iterator& iterator) + { + static const char* + constness_names[_Error_formatter::__last_constness] = + { + "<unknown constness>", + "constant", + "mutable" + }; + print_word(ctx, constness_names[iterator._M_constness]); + } + + template<typename _Iterator> + void + print_iterator_state(PrintContext& ctx, const _Iterator& iterator) + { + static const char* + state_names[_Error_formatter::__last_state] = + { + "<unknown state>", + "singular", + "dereferenceable (start-of-sequence)", + "dereferenceable", + "past-the-end", + "before-begin", + "dereferenceable (start-of-reverse-sequence)", + "dereferenceable (reverse)", + "past-the-reverse-end" + }; + print_word(ctx, state_names[iterator._M_state]); + } + + template<typename _Iterator> + void + print_iterator_seq_type(PrintContext& ctx, const _Iterator& iterator) + { print_type_info(ctx, iterator._M_seq_type, "<unknown seq_type>"); } + bool - print_field(PrintContext& ctx, - const char* name, const _Parameter::_Type& type) + print_named_field(PrintContext& ctx, + const char* fname, const _Parameter::_Named& named) { - if (__builtin_strcmp(name, "name") == 0) - { - assert(type._M_name); - print_word(ctx, type._M_name); - } - else if (__builtin_strcmp(name, "type") == 0) - print_type(ctx, type._M_type, "<unknown type>"); + if (__builtin_strcmp(fname, "name") == 0) + print_named_name(ctx, named); else return false; @@ -680,112 +779,92 @@ namespace } bool - print_field(PrintContext& ctx, - const char* name, const _Parameter::_Instance& inst) + print_type_field(PrintContext& ctx, + const char* fname, const _Parameter::_Type& type) { - const _Parameter::_Type& type = inst; - if (print_field(ctx, name, type)) + if (print_named_field(ctx, fname, type)) { } - else if (__builtin_strcmp(name, "address") == 0) - { - char buf[64]; - int ret = __builtin_sprintf(buf, "%p", inst._M_address); - print_word(ctx, buf, ret); - } + else if (__builtin_strcmp(fname, "type") == 0) + print_type_info(ctx, type._M_type, "<unknown type>"); else return false; return true; } + bool + print_instance_field(PrintContext& ctx, + const char* fname, const _Parameter::_Instance& inst) + { + if (print_type_field(ctx, fname, inst)) + { } + else if (__builtin_strcmp(fname, "address") == 0) + print_address(ctx, inst._M_address); + else + return false; + + return true; + } + + template<typename _Iterator> + bool + print_iterator_field(PrintContext& ctx, + const char* fname, const _Iterator& iterator) + { + if (print_instance_field(ctx, fname, iterator)) + { } + else if (__builtin_strcmp(fname, "constness") == 0) + print_iterator_constness(ctx, iterator); + else if (__builtin_strcmp(fname, "state") == 0) + print_iterator_state(ctx, iterator); + else if (__builtin_strcmp(fname, "sequence") == 0) + { + assert(iterator._M_sequence); + print_address(ctx, iterator._M_sequence); + } + else if (__builtin_strcmp(fname, "seq_type") == 0) + print_iterator_seq_type(ctx, iterator); + else + return false; + + return true; + } + void - print_field(PrintContext& ctx, const _Parameter& param, const char* name) + print_field(PrintContext& ctx, const _Parameter& param, const char* fname) { assert(param._M_kind != _Parameter::__unused_param); - const int bufsize = 64; - char buf[bufsize]; const auto& variant = param._M_variant; switch (param._M_kind) { case _Parameter::__iterator: - { - const auto& iterator = variant._M_iterator; - if (print_field(ctx, name, iterator)) - { } - else if (__builtin_strcmp(name, "constness") == 0) - { - static const char* - constness_names[_Error_formatter::__last_constness] = - { - "<unknown constness>", - "constant", - "mutable" - }; - print_word(ctx, constness_names[iterator._M_constness]); - } - else if (__builtin_strcmp(name, "state") == 0) - { - static const char* - state_names[_Error_formatter::__last_state] = - { - "<unknown state>", - "singular", - "dereferenceable (start-of-sequence)", - "dereferenceable", - "past-the-end", - "before-begin", - "dereferenceable (start-of-reverse-sequence)", - "dereferenceable (reverse)", - "past-the-reverse-end" - }; - print_word(ctx, state_names[iterator._M_state]); - } - else if (__builtin_strcmp(name, "sequence") == 0) - { - assert(iterator._M_sequence); - int written = __builtin_sprintf(buf, "%p", iterator._M_sequence); - print_word(ctx, buf, written); - } - else if (__builtin_strcmp(name, "seq_type") == 0) - print_type(ctx, iterator._M_seq_type, "<unknown seq_type>"); - else - assert(false); - } + if (!print_iterator_field(ctx, fname, variant._M_iterator)) + assert(false); break; case _Parameter::__sequence: - if (!print_field(ctx, name, variant._M_sequence)) + if (!print_instance_field(ctx, fname, variant._M_sequence)) assert(false); break; case _Parameter::__integer: - if (__builtin_strcmp(name, "name") == 0) - { - assert(variant._M_integer._M_name); - print_word(ctx, variant._M_integer._M_name); - } - else + if (!print_named_field(ctx, fname, variant._M_integer)) assert(false); break; case _Parameter::__string: - if (__builtin_strcmp(name, "name") == 0) - { - assert(variant._M_string._M_name); - print_word(ctx, variant._M_string._M_name); - } - else + if (!print_named_field(ctx, fname, variant._M_string)) assert(false); break; case _Parameter::__instance: - if (!print_field(ctx, name, variant._M_instance)) + if (!print_instance_field(ctx, fname, variant._M_instance)) assert(false); break; case _Parameter::__iterator_value_type: - if (!print_field(ctx, name, variant._M_iterator_value_type)) + if (!print_type_field(ctx, fname, variant._M_iterator_value_type)) assert(false); break; @@ -796,55 +875,53 @@ namespace } void - print_description(PrintContext& ctx, const _Parameter::_Type& type) + print_quoted_named_name(PrintContext& ctx, const _Parameter::_Named& named) { - if (type._M_name) + if (named._M_name) { print_literal(ctx, "\""); - print_word(ctx, type._M_name); - print_literal(ctx, "\""); + print_named_name(ctx, named); + print_literal(ctx, "\" "); } + } - print_literal(ctx, " {\n"); - + void + print_type_type(PrintContext& ctx, const _Parameter::_Type& type, + bool close_desc = true) + { if (type._M_type) { print_literal(ctx, " type = "); - print_type(ctx, type._M_type, "<unknown type>"); - print_literal(ctx, ";\n"); + print_type_info(ctx, type._M_type, "<unknown type>"); + if (close_desc) + print_literal(ctx, ";\n"); } } void - print_description(PrintContext& ctx, const _Parameter::_Instance& inst) + print_type(PrintContext& ctx, const _Parameter::_Type& type) { - const int bufsize = 64; - char buf[bufsize]; - - if (inst._M_name) - { - print_literal(ctx, "\""); - print_word(ctx, inst._M_name); - print_literal(ctx, "\" "); - } + print_quoted_named_name(ctx, type); + print_literal(ctx, " {\n"); + print_type_type(ctx, type); + print_literal(ctx, "}\n"); + } - int written - = __builtin_sprintf(buf, "@ 0x%p {\n", inst._M_address); - print_word(ctx, buf, written); + void + print_instance(PrintContext& ctx, const _Parameter::_Instance& inst, + bool close_desc = true) + { + print_quoted_named_name(ctx, inst); + print_address(ctx, "@ %p {\n", inst._M_address); + print_type_type(ctx, inst, close_desc); - if (inst._M_type) - { - print_literal(ctx, " type = "); - print_type(ctx, inst._M_type, "<unknown type>"); - } + if (close_desc) + print_literal(ctx, "}\n"); } void print_description(PrintContext& ctx, const _Parameter& param) { - const int bufsize = 128; - char buf[bufsize]; - const auto& variant = param._M_variant; switch (param._M_kind) { @@ -853,14 +930,14 @@ namespace const auto& ite = variant._M_iterator; print_literal(ctx, "iterator "); - print_description(ctx, ite); + print_instance(ctx, ite, false); if (ite._M_type) { if (ite._M_constness != _Error_formatter::__unknown_constness) { print_literal(ctx, " ("); - print_field(ctx, param, "constness"); + print_iterator_constness(ctx, ite); print_literal(ctx, " iterator)"); } @@ -870,7 +947,7 @@ namespace if (ite._M_state != _Error_formatter::__unknown_state) { print_literal(ctx, " state = "); - print_field(ctx, param, "state"); + print_iterator_state(ctx, ite); print_literal(ctx, ";\n"); } @@ -880,13 +957,11 @@ namespace if (ite._M_seq_type) { print_literal(ctx, "with type '"); - print_field(ctx, param, "seq_type"); + print_iterator_seq_type(ctx, ite); print_literal(ctx, "' "); } - int written - = __builtin_sprintf(buf, "@ 0x%p\n", ite._M_sequence); - print_word(ctx, buf, written); + print_address(ctx, "@ %p\n", ite._M_sequence); } print_literal(ctx, "}\n"); @@ -895,28 +970,17 @@ namespace case _Parameter::__sequence: print_literal(ctx, "sequence "); - print_description(ctx, variant._M_sequence); - - if (variant._M_sequence._M_type) - print_literal(ctx, ";\n"); - - print_literal(ctx, "}\n"); + print_instance(ctx, variant._M_sequence); break; case _Parameter::__instance: print_literal(ctx, "instance "); - print_description(ctx, variant._M_instance); - - if (variant._M_instance._M_type) - print_literal(ctx, ";\n"); - - print_literal(ctx, "}\n"); + print_instance(ctx, variant._M_instance); break; case _Parameter::__iterator_value_type: print_literal(ctx, "iterator::value_type "); - print_description(ctx, variant._M_iterator_value_type); - print_literal(ctx, "}\n"); + print_type(ctx, variant._M_iterator_value_type); break; default: @@ -925,71 +989,67 @@ 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; - while (*start) + while ((end && str != end) || (!end && *str)) { - if (isspace(*start)) + if (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; } // 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 printing 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 - = __builtin_sprintf(buf, "%ld", - param._M_variant._M_integer._M_value); - print_word(ctx, buf, written); - } + print_integer(ctx, param._M_variant._M_integer._M_value); 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 +1057,28 @@ 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) - { - buf[bufindex] = '\0'; - print_word(ctx, buf, 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); } } namespace __gnu_debug @@ -1036,16 +1098,15 @@ namespace __gnu_debug PrintContext ctx; if (_M_file) { - print_word(ctx, _M_file); + print_raw(ctx, _M_file); print_literal(ctx, ":"); go_to_next_line = true; } if (_M_line > 0) { - char buf[64]; - int written = __builtin_sprintf(buf, "%u:", _M_line); - print_word(ctx, buf, written); + ctx._M_column += fprintf(stderr, "%u", _M_line); + print_literal(ctx, ":"); go_to_next_line = true; } @@ -1058,41 +1119,17 @@ namespace __gnu_debug if (_M_function) { print_literal(ctx, "In function:\n"); - print_string(ctx, _M_function, nullptr, 0); + pretty_print(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 - 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [_GLIBCXX_DEBUG] Enhance rendering of assert message 2021-05-22 20:08 [_GLIBCXX_DEBUG] Enhance rendering of assert message François Dumont @ 2021-05-25 9:58 ` Jonathan Wakely 2021-05-25 21:01 ` François Dumont 0 siblings, 1 reply; 4+ messages in thread From: Jonathan Wakely @ 2021-05-25 9:58 UTC (permalink / raw) To: François Dumont; +Cc: libstdc++, gcc-patches On 22/05/21 22:08 +0200, François Dumont via Libstdc++ wrote: >Here is the part of the libbacktrace patch with the enhancement to the >rendering of assert message. > >It only contains one real fix, the rendering of address. In 2 places >it was done with "0x%p", so resulting in something like: 0x0x012345678 > >Otherwise it is just enhancements, mostly avoiding intermediate buffering. > >I am adding the _Parameter::_Named type to help on the rendering. I >hesitated in doing the same for the _M_iterator type but eventually >managed it with a template function. > > libstdc++: [_GLIBCXX_DEBUG] Enhance rendering of assert message > > Avoid building an intermediate buffer to print to stderr, push >directly to stderr. > > libstdc++-v3/ChangeLog: > > * include/debug/formatter.h > (_Error_formatter::_Parameter::_Named): New. > (_Error_formatter::_Parameter::_Type): Inherit latter. > (_Error_formatter::_Parameter::_M_integer): Likewise. > (_Error_formatter::_Parameter::_M_string): Likewise. > * src/c++11/debug.cc: Include <cstring>. > (_Print_func_t): New. > (print_raw(PrintContext&, const char*, ptrdiff_t)): New. > (print_word): Use '%.*s' format in fprintf to render only >expected number of chars. > (pretty_print(PrintContext&, const char*, _Print_func_t)): New. > (print_type): Rename in... > (print_type_info): ...this. Use pretty_print. > (print_address, print_integer): New. > (print_named_name, print_iterator_constness, >print_iterator_state): New. > (print_iterator_seq_type): New. > (print_named_field, print_type_field, >print_instance_field, print_iterator_field): New. > (print_field): Use latters. > (print_quoted_named_name, print_type_type, print_type, >print_instance): New. > (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. > >Ok to commit ? > >François > >+ void >+ pretty_print(PrintContext& ctx, const char* str, _Print_func_t print_func) >+ { >+ const char cxx1998[] = "__cxx1998::"; >+ const char uglification[] = "__"; >+ for (;;) >+ { >+ auto idx = strstr(str, uglification); This is confusing. strstr returns a pointer, not an index into the string. >+ if (idx) >+ { >+ size_t offset = >+ (idx == strstr(str, cxx1998) >+ ? sizeof(cxx1998) : sizeof(uglification)) - 1; This is a bit inefficient. Consider "int __foo(__cxx1998::bar)". The first strstr returns a pointer to "__foo" and then the second one searches the entire string from the beginning looking for "__cxx1998::", and checks if it is the same position as "__foo". The second strstr doesn't need to search from the beginning, and it doesn't need to look all the way to the end. It should be memcmp. if (auto pos = strstr(str, uglification)) { if (pos != str) print_func(ctx, str, pos - str); if (memcmp(pos, cxx1998, sizeof(cxx1998)-1) == 0) str = pos + (sizeof(cxx1998) - 1); else str = pos + (sizeof(uglification) - 1); while (*str && isspace((unsigned char)*str)) ++str; if (!*str) break; } else It doesn't even need to search from the position found by the first strstr, because we already know it starts with "__", so: for (;;) { if (auto pos = strstr(str, "__")) { if (pos != str) print_func(ctx, str, pos - str); pos += 2; // advance past "__" if (memcmp(pos, "cxx1998::", 9) == 0) str = pos + 9; // advance past "cxx1998::" while (*str && isspace((unsigned char)*str)) ++str; if (!*str) break; } else But either is OK. Just not doing a second strstr through the entire string again to look for "__cxx1998::". >+ >+ if (idx != str) >+ print_func(ctx, str, idx - str); >+ >+ str = idx + offset; >+ >+ while (*str && isspace((unsigned char)*str)) >+ ++str; Is this really needed? Why would there be whitespace following "__" or "__cxx1998::" and why would we want to skip it? I know it doesn't follow our usual naming scheme, but a symbol like "__foo__ bar()" would get printed as "foobar()" wouldn't it? The rest of the patch looks fine, I'm just unsure about pretty_print. Maybe I've misunderstood the possible strings it gets used with? ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [_GLIBCXX_DEBUG] Enhance rendering of assert message 2021-05-25 9:58 ` Jonathan Wakely @ 2021-05-25 21:01 ` François Dumont 2021-05-25 21:10 ` Jonathan Wakely 0 siblings, 1 reply; 4+ messages in thread From: François Dumont @ 2021-05-25 21:01 UTC (permalink / raw) To: Jonathan Wakely; +Cc: libstdc++, gcc-patches [-- Attachment #1: Type: text/plain, Size: 6302 bytes --] On 25/05/21 11:58 am, Jonathan Wakely wrote: > On 22/05/21 22:08 +0200, François Dumont via Libstdc++ wrote: >> Here is the part of the libbacktrace patch with the enhancement to >> the rendering of assert message. >> >> It only contains one real fix, the rendering of address. In 2 places >> it was done with "0x%p", so resulting in something like: 0x0x012345678 >> >> Otherwise it is just enhancements, mostly avoiding intermediate >> buffering. >> >> I am adding the _Parameter::_Named type to help on the rendering. I >> hesitated in doing the same for the _M_iterator type but eventually >> managed it with a template function. >> >> libstdc++: [_GLIBCXX_DEBUG] Enhance rendering of assert message >> >> Avoid building an intermediate buffer to print to stderr, push >> directly to stderr. >> >> libstdc++-v3/ChangeLog: >> >> * include/debug/formatter.h >> (_Error_formatter::_Parameter::_Named): New. >> (_Error_formatter::_Parameter::_Type): Inherit latter. >> (_Error_formatter::_Parameter::_M_integer): Likewise. >> (_Error_formatter::_Parameter::_M_string): Likewise. >> * src/c++11/debug.cc: Include <cstring>. >> (_Print_func_t): New. >> (print_raw(PrintContext&, const char*, ptrdiff_t)): New. >> (print_word): Use '%.*s' format in fprintf to render only >> expected number of chars. >> (pretty_print(PrintContext&, const char*, >> _Print_func_t)): New. >> (print_type): Rename in... >> (print_type_info): ...this. Use pretty_print. >> (print_address, print_integer): New. >> (print_named_name, print_iterator_constness, >> print_iterator_state): New. >> (print_iterator_seq_type): New. >> (print_named_field, print_type_field, >> print_instance_field, print_iterator_field): New. >> (print_field): Use latters. >> (print_quoted_named_name, print_type_type, print_type, >> print_instance): New. >> (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. >> >> Ok to commit ? >> >> François >> > >> + void >> + pretty_print(PrintContext& ctx, const char* str, _Print_func_t >> print_func) >> + { >> + const char cxx1998[] = "__cxx1998::"; >> + const char uglification[] = "__"; >> + for (;;) >> + { >> + auto idx = strstr(str, uglification); > > This is confusing. strstr returns a pointer, not an index into the > string. > >> + if (idx) >> + { >> + size_t offset = >> + (idx == strstr(str, cxx1998) >> + ? sizeof(cxx1998) : sizeof(uglification)) - 1; > > This is a bit inefficient. Consider "int __foo(__cxx1998::bar)". The > first strstr returns a pointer to "__foo" and then the second one > searches the entire string from the beginning looking for > "__cxx1998::", and checks if it is the same position as "__foo". > > The second strstr doesn't need to search from the beginning, and it > doesn't need to look all the way to the end. It should be memcmp. > > if (auto pos = strstr(str, uglification)) > { > if (pos != str) > print_func(ctx, str, pos - str); > > if (memcmp(pos, cxx1998, sizeof(cxx1998)-1) == 0) > str = pos + (sizeof(cxx1998) - 1); > else > str = pos + (sizeof(uglification) - 1); > while (*str && isspace((unsigned char)*str)) > ++str; > > if (!*str) > break; > } > else > > It doesn't even need to search from the position found by the first > strstr, because we already know it starts with "__", so: > > for (;;) > { > if (auto pos = strstr(str, "__")) > { > if (pos != str) > print_func(ctx, str, pos - str); > > pos += 2; // advance past "__" > > if (memcmp(pos, "cxx1998::", 9) == 0) > str = pos + 9; // advance past "cxx1998::" > while (*str && isspace((unsigned char)*str)) > ++str; > > if (!*str) > break; > } > else > > But either is OK. Just not doing a second strstr through the entire > string again to look for "__cxx1998::". > > > >> + >> + if (idx != str) >> + print_func(ctx, str, idx - str); >> + >> + str = idx + offset; >> + >> + while (*str && isspace((unsigned char)*str)) >> + ++str; > > Is this really needed? > > Why would there be whitespace following "__" or "__cxx1998::" and why > would we want to skip it? Yes, I cannot remember why I added it in the first place. So removed in this new proposal with your other changes. > > I know it doesn't follow our usual naming scheme, but a symbol like > "__foo__ bar()" would get printed as "foobar()" wouldn't it? > Yes, it would. > The rest of the patch looks fine, I'm just unsure about pretty_print. > Maybe I've misunderstood the possible strings it gets used with? > pretty_print is used for demangle type names, variable names and function names given by the __PRETTY_FUNCTION__ macro. So in theory yes, __foo__bar would be replaced by foobar but I considered that we will never face this situation for the moment. When considering backtraces we might have to review this. Ok to commit ? François [-- Attachment #2: debug_mode.patch --] [-- Type: text/x-patch, Size: 20957 bytes --] diff --git a/libstdc++-v3/include/debug/formatter.h b/libstdc++-v3/include/debug/formatter.h index 7140fed5e83..a83d33a4cb8 100644 --- a/libstdc++-v3/include/debug/formatter.h +++ b/libstdc++-v3/include/debug/formatter.h @@ -202,9 +202,13 @@ namespace __gnu_debug __iterator_value_type } _M_kind; - struct _Type + struct _Named { const char* _M_name; + }; + + struct _Type : _Named + { const type_info* _M_type; }; @@ -228,16 +232,14 @@ namespace __gnu_debug _Instance _M_sequence; // When _M_kind == __integer - struct + struct : _Named { - const char* _M_name; long _M_value; } _M_integer; // When _M_kind == __string - struct + struct : _Named { - const char* _M_name; const char* _M_value; } _M_string; diff --git a/libstdc++-v3/src/c++11/debug.cc b/libstdc++-v3/src/c++11/debug.cc index 5a642097d17..33d76bfcaab 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,26 @@ namespace bool _M_wordwrap; }; + using _Print_func_t = void (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_raw(PrintContext& ctx, const char* str, ptrdiff_t nbc = -1) { - size_t length = count >= 0 ? count : __builtin_strlen(word); + if (nbc >= 0) + ctx._M_column += fprintf(stderr, "%.*s", (int)nbc, str); + else + ctx._M_column += fprintf(stderr, "%s", str); + } + + void + print_word(PrintContext& ctx, const char* word, ptrdiff_t nbc = -1) + { + size_t length = nbc >= 0 ? nbc : __builtin_strlen(word); if (length == 0) return; @@ -610,7 +616,7 @@ namespace } size_t visual_length - = isspace(word[length - 1]) ? length - 1 : length; + = isspace((unsigned char)word[length - 1]) ? length - 1 : length; if (visual_length == 0 || !ctx._M_wordwrap || (ctx._M_column + visual_length < ctx._M_max_length) @@ -619,15 +625,11 @@ 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'; - fprintf(stderr, "%s", spacing); - ctx._M_column += ctx._M_indent; + const char spacing[ctx._M_indent + 1] = " "; + print_raw(ctx, spacing, ctx._M_indent); } - int written = fprintf(stderr, "%s", word); + int written = fprintf(stderr, "%.*s", (int)length, word); if (word[length - 1] == '\n') { @@ -640,15 +642,40 @@ namespace else { print_literal(ctx, "\n"); - print_word(ctx, word, count); + print_word(ctx, word, nbc); + } + } + + void + pretty_print(PrintContext& ctx, const char* str, _Print_func_t print_func) + { + const char cxx1998[] = "cxx1998::"; + for (;;) + { + if (auto pos = strstr(str, "__")) + { + if (pos != str) + print_func(ctx, str, pos - str); + + pos += 2; // advance past "__" + if (memcmp(pos, cxx1998, 9) == 0) + pos += 9; // advance part "cxx1998::" + + str = pos; + } + else + { + print_func(ctx, str, -1); + break; + } } } template<size_t Length> void - print_type(PrintContext& ctx, - const type_info* info, - const char(&unknown_name)[Length]) + print_type_info(PrintContext& ctx, + const type_info* info, + const char(&unknown_name)[Length]) { if (!info) print_literal(ctx, unknown_name); @@ -657,22 +684,86 @@ 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) + pretty_print(ctx, demangled_name, &print_word); + else + print_word(ctx, info->name()); free(demangled_name); } } + void + print_address(PrintContext& ctx, const char* fmt, const void* address) + { + char buf[128]; + int written = __builtin_sprintf(buf, fmt, address); + print_word(ctx, buf, written); + } + + void + print_address(PrintContext& ctx, const void* address) + { print_address(ctx, "%p", address); } + + void + print_integer(PrintContext& ctx, long integer) + { + char buf[64]; + int written = __builtin_sprintf(buf, "%ld", integer); + print_word(ctx, buf, written); + } + + void + print_named_name(PrintContext& ctx, const _Parameter::_Named& named) + { + assert(named._M_name); + pretty_print(ctx, named._M_name, print_word); + } + + template<typename _Iterator> + void + print_iterator_constness(PrintContext& ctx, const _Iterator& iterator) + { + static const char* + constness_names[_Error_formatter::__last_constness] = + { + "<unknown constness>", + "constant", + "mutable" + }; + print_word(ctx, constness_names[iterator._M_constness]); + } + + template<typename _Iterator> + void + print_iterator_state(PrintContext& ctx, const _Iterator& iterator) + { + static const char* + state_names[_Error_formatter::__last_state] = + { + "<unknown state>", + "singular", + "dereferenceable (start-of-sequence)", + "dereferenceable", + "past-the-end", + "before-begin", + "dereferenceable (start-of-reverse-sequence)", + "dereferenceable (reverse)", + "past-the-reverse-end" + }; + print_word(ctx, state_names[iterator._M_state]); + } + + template<typename _Iterator> + void + print_iterator_seq_type(PrintContext& ctx, const _Iterator& iterator) + { print_type_info(ctx, iterator._M_seq_type, "<unknown seq_type>"); } + bool - print_field(PrintContext& ctx, - const char* name, const _Parameter::_Type& type) + print_named_field(PrintContext& ctx, + const char* fname, const _Parameter::_Named& named) { - if (__builtin_strcmp(name, "name") == 0) - { - assert(type._M_name); - print_word(ctx, type._M_name); - } - else if (__builtin_strcmp(name, "type") == 0) - print_type(ctx, type._M_type, "<unknown type>"); + if (__builtin_strcmp(fname, "name") == 0) + print_named_name(ctx, named); else return false; @@ -680,112 +771,92 @@ namespace } bool - print_field(PrintContext& ctx, - const char* name, const _Parameter::_Instance& inst) + print_type_field(PrintContext& ctx, + const char* fname, const _Parameter::_Type& type) { - const _Parameter::_Type& type = inst; - if (print_field(ctx, name, type)) + if (print_named_field(ctx, fname, type)) { } - else if (__builtin_strcmp(name, "address") == 0) - { - char buf[64]; - int ret = __builtin_sprintf(buf, "%p", inst._M_address); - print_word(ctx, buf, ret); - } + else if (__builtin_strcmp(fname, "type") == 0) + print_type_info(ctx, type._M_type, "<unknown type>"); + else + return false; + + return true; + } + + bool + print_instance_field(PrintContext& ctx, + const char* fname, const _Parameter::_Instance& inst) + { + if (print_type_field(ctx, fname, inst)) + { } + else if (__builtin_strcmp(fname, "address") == 0) + print_address(ctx, inst._M_address); else return false; return true; } + template<typename _Iterator> + bool + print_iterator_field(PrintContext& ctx, + const char* fname, const _Iterator& iterator) + { + if (print_instance_field(ctx, fname, iterator)) + { } + else if (__builtin_strcmp(fname, "constness") == 0) + print_iterator_constness(ctx, iterator); + else if (__builtin_strcmp(fname, "state") == 0) + print_iterator_state(ctx, iterator); + else if (__builtin_strcmp(fname, "sequence") == 0) + { + assert(iterator._M_sequence); + print_address(ctx, iterator._M_sequence); + } + else if (__builtin_strcmp(fname, "seq_type") == 0) + print_iterator_seq_type(ctx, iterator); + else + return false; + + return true; + } + void - print_field(PrintContext& ctx, const _Parameter& param, const char* name) + print_field(PrintContext& ctx, const _Parameter& param, const char* fname) { assert(param._M_kind != _Parameter::__unused_param); - const int bufsize = 64; - char buf[bufsize]; const auto& variant = param._M_variant; switch (param._M_kind) { case _Parameter::__iterator: - { - const auto& iterator = variant._M_iterator; - if (print_field(ctx, name, iterator)) - { } - else if (__builtin_strcmp(name, "constness") == 0) - { - static const char* - constness_names[_Error_formatter::__last_constness] = - { - "<unknown constness>", - "constant", - "mutable" - }; - print_word(ctx, constness_names[iterator._M_constness]); - } - else if (__builtin_strcmp(name, "state") == 0) - { - static const char* - state_names[_Error_formatter::__last_state] = - { - "<unknown state>", - "singular", - "dereferenceable (start-of-sequence)", - "dereferenceable", - "past-the-end", - "before-begin", - "dereferenceable (start-of-reverse-sequence)", - "dereferenceable (reverse)", - "past-the-reverse-end" - }; - print_word(ctx, state_names[iterator._M_state]); - } - else if (__builtin_strcmp(name, "sequence") == 0) - { - assert(iterator._M_sequence); - int written = __builtin_sprintf(buf, "%p", iterator._M_sequence); - print_word(ctx, buf, written); - } - else if (__builtin_strcmp(name, "seq_type") == 0) - print_type(ctx, iterator._M_seq_type, "<unknown seq_type>"); - else - assert(false); - } + if (!print_iterator_field(ctx, fname, variant._M_iterator)) + assert(false); break; case _Parameter::__sequence: - if (!print_field(ctx, name, variant._M_sequence)) + if (!print_instance_field(ctx, fname, variant._M_sequence)) assert(false); break; case _Parameter::__integer: - if (__builtin_strcmp(name, "name") == 0) - { - assert(variant._M_integer._M_name); - print_word(ctx, variant._M_integer._M_name); - } - else + if (!print_named_field(ctx, fname, variant._M_integer)) assert(false); break; case _Parameter::__string: - if (__builtin_strcmp(name, "name") == 0) - { - assert(variant._M_string._M_name); - print_word(ctx, variant._M_string._M_name); - } - else + if (!print_named_field(ctx, fname, variant._M_string)) assert(false); break; case _Parameter::__instance: - if (!print_field(ctx, name, variant._M_instance)) + if (!print_instance_field(ctx, fname, variant._M_instance)) assert(false); break; case _Parameter::__iterator_value_type: - if (!print_field(ctx, name, variant._M_iterator_value_type)) + if (!print_type_field(ctx, fname, variant._M_iterator_value_type)) assert(false); break; @@ -796,55 +867,53 @@ namespace } void - print_description(PrintContext& ctx, const _Parameter::_Type& type) + print_quoted_named_name(PrintContext& ctx, const _Parameter::_Named& named) { - if (type._M_name) + if (named._M_name) { print_literal(ctx, "\""); - print_word(ctx, type._M_name); - print_literal(ctx, "\""); + print_named_name(ctx, named); + print_literal(ctx, "\" "); } + } - print_literal(ctx, " {\n"); - + void + print_type_type(PrintContext& ctx, const _Parameter::_Type& type, + bool close_desc = true) + { if (type._M_type) { print_literal(ctx, " type = "); - print_type(ctx, type._M_type, "<unknown type>"); - print_literal(ctx, ";\n"); + print_type_info(ctx, type._M_type, "<unknown type>"); + if (close_desc) + print_literal(ctx, ";\n"); } } void - print_description(PrintContext& ctx, const _Parameter::_Instance& inst) + print_type(PrintContext& ctx, const _Parameter::_Type& type) { - const int bufsize = 64; - char buf[bufsize]; - - if (inst._M_name) - { - print_literal(ctx, "\""); - print_word(ctx, inst._M_name); - print_literal(ctx, "\" "); - } + print_quoted_named_name(ctx, type); + print_literal(ctx, " {\n"); + print_type_type(ctx, type); + print_literal(ctx, "}\n"); + } - int written - = __builtin_sprintf(buf, "@ 0x%p {\n", inst._M_address); - print_word(ctx, buf, written); + void + print_instance(PrintContext& ctx, const _Parameter::_Instance& inst, + bool close_desc = true) + { + print_quoted_named_name(ctx, inst); + print_address(ctx, "@ %p {\n", inst._M_address); + print_type_type(ctx, inst, close_desc); - if (inst._M_type) - { - print_literal(ctx, " type = "); - print_type(ctx, inst._M_type, "<unknown type>"); - } + if (close_desc) + print_literal(ctx, "}\n"); } void print_description(PrintContext& ctx, const _Parameter& param) { - const int bufsize = 128; - char buf[bufsize]; - const auto& variant = param._M_variant; switch (param._M_kind) { @@ -853,14 +922,14 @@ namespace const auto& ite = variant._M_iterator; print_literal(ctx, "iterator "); - print_description(ctx, ite); + print_instance(ctx, ite, false); if (ite._M_type) { if (ite._M_constness != _Error_formatter::__unknown_constness) { print_literal(ctx, " ("); - print_field(ctx, param, "constness"); + print_iterator_constness(ctx, ite); print_literal(ctx, " iterator)"); } @@ -870,7 +939,7 @@ namespace if (ite._M_state != _Error_formatter::__unknown_state) { print_literal(ctx, " state = "); - print_field(ctx, param, "state"); + print_iterator_state(ctx, ite); print_literal(ctx, ";\n"); } @@ -880,13 +949,11 @@ namespace if (ite._M_seq_type) { print_literal(ctx, "with type '"); - print_field(ctx, param, "seq_type"); + print_iterator_seq_type(ctx, ite); print_literal(ctx, "' "); } - int written - = __builtin_sprintf(buf, "@ 0x%p\n", ite._M_sequence); - print_word(ctx, buf, written); + print_address(ctx, "@ %p\n", ite._M_sequence); } print_literal(ctx, "}\n"); @@ -895,28 +962,17 @@ namespace case _Parameter::__sequence: print_literal(ctx, "sequence "); - print_description(ctx, variant._M_sequence); - - if (variant._M_sequence._M_type) - print_literal(ctx, ";\n"); - - print_literal(ctx, "}\n"); + print_instance(ctx, variant._M_sequence); break; case _Parameter::__instance: print_literal(ctx, "instance "); - print_description(ctx, variant._M_instance); - - if (variant._M_instance._M_type) - print_literal(ctx, ";\n"); - - print_literal(ctx, "}\n"); + print_instance(ctx, variant._M_instance); break; case _Parameter::__iterator_value_type: print_literal(ctx, "iterator::value_type "); - print_description(ctx, variant._M_iterator_value_type); - print_literal(ctx, "}\n"); + print_type(ctx, variant._M_iterator_value_type); break; default: @@ -925,71 +981,67 @@ 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; - while (*start) + while ((end && str != end) || (!end && *str)) { - if (isspace(*start)) + if (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; } // 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 printing 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 - = __builtin_sprintf(buf, "%ld", - param._M_variant._M_integer._M_value); - print_word(ctx, buf, written); - } + print_integer(ctx, param._M_variant._M_integer._M_value); 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 +1049,28 @@ 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) - { - buf[bufindex] = '\0'; - print_word(ctx, buf, 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); } } namespace __gnu_debug @@ -1036,16 +1090,15 @@ namespace __gnu_debug PrintContext ctx; if (_M_file) { - print_word(ctx, _M_file); + print_raw(ctx, _M_file); print_literal(ctx, ":"); go_to_next_line = true; } if (_M_line > 0) { - char buf[64]; - int written = __builtin_sprintf(buf, "%u:", _M_line); - print_word(ctx, buf, written); + ctx._M_column += fprintf(stderr, "%u", _M_line); + print_literal(ctx, ":"); go_to_next_line = true; } @@ -1058,41 +1111,17 @@ namespace __gnu_debug if (_M_function) { print_literal(ctx, "In function:\n"); - print_string(ctx, _M_function, nullptr, 0); + pretty_print(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 - 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [_GLIBCXX_DEBUG] Enhance rendering of assert message 2021-05-25 21:01 ` François Dumont @ 2021-05-25 21:10 ` Jonathan Wakely 0 siblings, 0 replies; 4+ messages in thread From: Jonathan Wakely @ 2021-05-25 21:10 UTC (permalink / raw) To: François Dumont; +Cc: libstdc++, gcc-patches On 25/05/21 23:01 +0200, François Dumont wrote: >On 25/05/21 11:58 am, Jonathan Wakely wrote: >>On 22/05/21 22:08 +0200, François Dumont via Libstdc++ wrote: >>>Here is the part of the libbacktrace patch with the enhancement to >>>the rendering of assert message. >>> >>>It only contains one real fix, the rendering of address. In 2 >>>places it was done with "0x%p", so resulting in something like: >>>0x0x012345678 >>> >>>Otherwise it is just enhancements, mostly avoiding intermediate >>>buffering. >>> >>>I am adding the _Parameter::_Named type to help on the rendering. >>>I hesitated in doing the same for the _M_iterator type but >>>eventually managed it with a template function. >>> >>> libstdc++: [_GLIBCXX_DEBUG] Enhance rendering of assert message >>> >>> Avoid building an intermediate buffer to print to stderr, push >>>directly to stderr. >>> >>> libstdc++-v3/ChangeLog: >>> >>> * include/debug/formatter.h >>> (_Error_formatter::_Parameter::_Named): New. >>> (_Error_formatter::_Parameter::_Type): Inherit latter. >>> (_Error_formatter::_Parameter::_M_integer): Likewise. >>> (_Error_formatter::_Parameter::_M_string): Likewise. >>> * src/c++11/debug.cc: Include <cstring>. >>> (_Print_func_t): New. >>> (print_raw(PrintContext&, const char*, ptrdiff_t)): New. >>> (print_word): Use '%.*s' format in fprintf to render >>>only expected number of chars. >>> (pretty_print(PrintContext&, const char*, >>>_Print_func_t)): New. >>> (print_type): Rename in... >>> (print_type_info): ...this. Use pretty_print. >>> (print_address, print_integer): New. >>> (print_named_name, print_iterator_constness, >>>print_iterator_state): New. >>> (print_iterator_seq_type): New. >>> (print_named_field, print_type_field, >>>print_instance_field, print_iterator_field): New. >>> (print_field): Use latters. >>> (print_quoted_named_name, print_type_type, print_type, >>>print_instance): New. >>> (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. >>> >>>Ok to commit ? >>> >>>François >>> >> >>>+ void >>>+ pretty_print(PrintContext& ctx, const char* str, _Print_func_t >>>print_func) >>>+ { >>>+ const char cxx1998[] = "__cxx1998::"; >>>+ const char uglification[] = "__"; >>>+ for (;;) >>>+ { >>>+ auto idx = strstr(str, uglification); >> >>This is confusing. strstr returns a pointer, not an index into the >>string. >> >>>+ if (idx) >>>+ { >>>+ size_t offset = >>>+ (idx == strstr(str, cxx1998) >>>+ ? sizeof(cxx1998) : sizeof(uglification)) - 1; >> >>This is a bit inefficient. Consider "int __foo(__cxx1998::bar)". The >>first strstr returns a pointer to "__foo" and then the second one >>searches the entire string from the beginning looking for >>"__cxx1998::", and checks if it is the same position as "__foo". >> >>The second strstr doesn't need to search from the beginning, and it >>doesn't need to look all the way to the end. It should be memcmp. >> >> if (auto pos = strstr(str, uglification)) >> { >> if (pos != str) >> print_func(ctx, str, pos - str); >> >> if (memcmp(pos, cxx1998, sizeof(cxx1998)-1) == 0) >> str = pos + (sizeof(cxx1998) - 1); >> else >> str = pos + (sizeof(uglification) - 1); >> while (*str && isspace((unsigned char)*str)) >> ++str; >> >> if (!*str) >> break; >> } >> else >> >>It doesn't even need to search from the position found by the first >>strstr, because we already know it starts with "__", so: >> >> for (;;) >> { >> if (auto pos = strstr(str, "__")) >> { >> if (pos != str) >> print_func(ctx, str, pos - str); >> >> pos += 2; // advance past "__" >> >> if (memcmp(pos, "cxx1998::", 9) == 0) >> str = pos + 9; // advance past "cxx1998::" >> while (*str && isspace((unsigned char)*str)) >> ++str; >> >> if (!*str) >> break; >> } >> else >> >>But either is OK. Just not doing a second strstr through the entire >>string again to look for "__cxx1998::". >> >> >> >>>+ >>>+ if (idx != str) >>>+ print_func(ctx, str, idx - str); >>>+ >>>+ str = idx + offset; >>>+ >>>+ while (*str && isspace((unsigned char)*str)) >>>+ ++str; >> >>Is this really needed? >> >>Why would there be whitespace following "__" or "__cxx1998::" and why >>would we want to skip it? > >Yes, I cannot remember why I added it in the first place. So removed >in this new proposal with your other changes. > >> >>I know it doesn't follow our usual naming scheme, but a symbol like >>"__foo__ bar()" would get printed as "foobar()" wouldn't it? >> >Yes, it would. >>The rest of the patch looks fine, I'm just unsure about pretty_print. >>Maybe I've misunderstood the possible strings it gets used with? >> >pretty_print is used for demangle type names, variable names and >function names given by the __PRETTY_FUNCTION__ macro. > >So in theory yes, __foo__bar would be replaced by foobar but I >considered that we will never face this situation for the moment. > >When considering backtraces we might have to review this. > >Ok to commit ? OK for trunk, thanks. ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-05-25 21:10 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-05-22 20:08 [_GLIBCXX_DEBUG] Enhance rendering of assert message François Dumont 2021-05-25 9:58 ` Jonathan Wakely 2021-05-25 21:01 ` François Dumont 2021-05-25 21:10 ` Jonathan Wakely
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).