* [_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).