Re: [_GLIBCXX_DEBUG] Enhance rendering of assert message

[Jonathan Wakely <jwakely@redhat.com>]

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?