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