Re: [_GLIBCXX_DEBUG] Enhance rendering of assert message

[Jonathan Wakely <jwakely@redhat.com>]

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.