Re: [PATCH] Fix gdb printers for std::string

["François Dumont" <frs.dumont@gmail.com>]

On 01/10/22 12:06, Jonathan Wakely wrote:
> On Sat, 1 Oct 2022 at 08:20, François Dumont via Libstdc++
> <libstdc++@gcc.gnu.org> wrote:
>> I had forgotten to re-run tests after I removed the #define
>> _GLIBCXX_USE_CXX11_ABI 0.
>>
>> The comment was misleading, it could also impact output of std::list.
>>
>> I am also restoring the correct std::string alias for
>> std::__cxx11::basic_string, even if with my workaround it doesn't really
>> matter as the one for std::basic_string will be used.
>>
>> I also restored the printer for std::__cxx11::string typedef. Is it
>> intentional to keep this ?
> Yes, I kept that intentionally. There can be programs where some
> objects still use that typedef, if those objects were compiled with
> GCC 8.x or older.
>
>>       libstdc++: Fix gdb pretty printers when dealing with std::string
>>
>>       Since revision 33b43b0d8cd2de722d177ef823930500948a7487 std::string
>> and other
>>       similar typedef are ambiguous from a gdb point of view because it
>> matches both
>>       std::basic_string<char> and std::__cxx11::basic_string<char>
>> symbols. For those
>>       typedef add a workaround to accept the substitution as long as the
>> same regardless
>>       of __cxx11 namespace.
> Thanks for figuring out what was going wrong here, and how to fix it.
>
>
>>       Also avoid to register printers for types in std::__cxx11::__8::
>> namespace, there is
>>       no such symbols.
>>
>>       libstdc++-v3/ChangeLog:
>>
>>               * libstdc++-v3/python/libstdcxx/v6/printers.py
>> (Printer.add_version): Do not add
>>               version namespace for __cxx11 symbols.
>>               (add_one_template_type_printer): Likewise.
>>               (add_one_type_printer): Likewise.
>>               (FilteringTypePrinter._recognizer.recognize): Add a
>> workaround for std::string & al
>>               ambiguous typedef matching both std:: and std::__cxx11::
>> symbols.
>>               (register_type_printers): Refine type registration to limit
>> false positive in
>>               FilteringTypePrinter._recognize.recognize requiring to look
>> for the type in gdb.
> I don't really like this part of the change though:

I'll check what you are proposing but I don't think it is necessary to 
fix the problem.

I did this on my path to find out what was going wrong. Once I 
understood it I consider that it was just a good change to keep. If you 
think otherwise I can revert this part.

I also noted that gdb consider the filters as a filo list, not fifo. And 
I think that the 1st filters registered are the most extensively used. I 
can propose to invert all the registration if you think it worth it.


>
> @@ -2105,29 +2120,29 @@ def register_type_printers(obj):
>           return
>
>       # Add type printers for typedefs std::string, std::wstring etc.
> -    for ch in ('', 'w', 'u8', 'u16', 'u32'):
> -        add_one_type_printer(obj, 'basic_string', ch + 'string')
> -        add_one_type_printer(obj, '__cxx11::basic_string', ch + 'string')
> +    for ch in (('char', ''), ('wchar_t', 'w'), ('char8_t', 'u8'),
> ('char16_t', 'u16'), ('char32_t', 'u32')):
> +        add_one_type_printer(obj, 'basic_string<' + ch[0], ch[1] + 'string')
> +        add_one_type_printer(obj, '__cxx11::basic_string<' + ch[0],
> ch[1] + 'string')
>
>
> As the docs for FilteringTypePrinter says, the first argument is
> supposed to be the class template name:
>
> class FilteringTypePrinter(object):
>      r"""
>      A type printer that uses typedef names for common template specializations.
>
>      Args:
>          match (str): The class template to recognize.
>          name (str): The typedef-name that will be used instead.
>
>      Checks if a specialization of the class template 'match' is the same type
>      as the typedef 'name', and prints it as 'name' instead.
>
>      e.g. if an instantiation of std::basic_istream<C, T> is the same type as
>      std::istream then print it as std::istream.
>      """
>
> With this change, the "class template" is sometimes just a string
> prefix of a particular specialization, e.g. "basic_string<char"
> The class template is "basic_string", and that's how the
> FilteringTypePrinter was intended to work.
>
> How about something like the attached (untested) change instead. which
> keeps the 'match' argument as the class template name, but optionally
> supports passing the first template argument? Then you can use
> match.split('::')[-1] is 'basic_string' to check if we are dealing
> with one of the possibly ambiguous typedefs.