Fix gdb printers for std::string

  Sometimes substitution of basic_string by one of the std::string 
typeedef fails. Here is the fix.

     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.

     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.
             * libstdc++-v3/testsuite/libstdc++-prettyprinters/cxx17.cc: 
Remove obsolete
             \#define _GLIBCXX_USE_CXX11_ABI 0.
             * 
libstdc++-v3/testsuite/libstdc++-prettyprinters/simple.cc: Likewise.
             * 
libstdc++-v3/testsuite/libstdc++-prettyprinters/simple11.cc: Likewise.
             * 
libstdc++-v3/testsuite/libstdc++-prettyprinters/whatis.cc: Likewise.
             * libstdc++-v3/testsuite/libstdc++-prettyprinters/80276.cc: 
Likewise and remove
             xfail for c++20 and debug mode.
             * 
libstdc++-v3/testsuite/libstdc++-prettyprinters/libfundts.cc: Likewise.

Tested under x86_64 linux w/o _GLIBCXX_INLINE_VERSION and w/o 
_GLIBCXX_DEBUG.

I also tested it with my patch to use cxx11 abi in 
_GLIBCXX_INLINE_VERSION mode.

Ok to commit ?

François
  

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 ?

     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.

     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.
             * libstdc++-v3/testsuite/libstdc++-prettyprinters/cxx17.cc: 
Remove obsolete
             \#define _GLIBCXX_USE_CXX11_ABI 0.
             * 
libstdc++-v3/testsuite/libstdc++-prettyprinters/simple.cc: Likewise. 
Adapt test
             to accept std::__cxx11::list.
             * 
libstdc++-v3/testsuite/libstdc++-prettyprinters/simple11.cc: Likewise.
             * 
libstdc++-v3/testsuite/libstdc++-prettyprinters/whatis.cc: Likewise.
             * libstdc++-v3/testsuite/libstdc++-prettyprinters/80276.cc: 
Likewise and remove
             xfail for c++20 and debug mode.
             * 
libstdc++-v3/testsuite/libstdc++-prettyprinters/libfundts.cc: Likewise.

Ok to commit ?

François

On 28/09/22 22:42, François Dumont wrote:
> Sometimes substitution of basic_string by one of the std::string 
> typeedef fails. Here is the fix.
>
>     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.
>
>     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.
>             * 
> libstdc++-v3/testsuite/libstdc++-prettyprinters/cxx17.cc: Remove obsolete
>             \#define _GLIBCXX_USE_CXX11_ABI 0.
>             * 
> libstdc++-v3/testsuite/libstdc++-prettyprinters/simple.cc: Likewise.
>             * 
> libstdc++-v3/testsuite/libstdc++-prettyprinters/simple11.cc: Likewise.
>             * 
> libstdc++-v3/testsuite/libstdc++-prettyprinters/whatis.cc: Likewise.
>             * 
> libstdc++-v3/testsuite/libstdc++-prettyprinters/80276.cc: Likewise and 
> remove
>             xfail for c++20 and debug mode.
>             * 
> libstdc++-v3/testsuite/libstdc++-prettyprinters/libfundts.cc: Likewise.
>
> Tested under x86_64 linux w/o _GLIBCXX_INLINE_VERSION and w/o 
> _GLIBCXX_DEBUG.
>
> I also tested it with my patch to use cxx11 abi in 
> _GLIBCXX_INLINE_VERSION mode.
>
> Ok to commit ?
>
> François
  

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:

@@ -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.
diff --git a/libstdc++-v3/python/libstdcxx/v6/printers.py b/libstdc++-v3/python/libstdcxx/v6/printers.py
index 245b6e3dbcd..6a0b8a22f1d 100644
--- a/libstdc++-v3/python/libstdcxx/v6/printers.py
+++ b/libstdc++-v3/python/libstdcxx/v6/printers.py
@@ -2045,25 +2045,35 @@ class FilteringTypePrinter(object):
     Args:
         match (str): The class template to recognize.
         name (str): The typedef-name that will be used instead.
+        targ1 (str, optional): The first template argument. Defaults to None.
+            If arg1 is provided, only match specializations with this type
+            as the first template argument, e.g. if match='basic_string'
 
     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
+    e.g. for match='basic_istream', name='istream', if any specialization of
+    std::basic_istream<C, T> is the same type as std::istream then print it as
+    std::istream.
+
+    e.g. for match='basic_istream', name='istream', targ1='char', if any
+    specialization of std::basic_istream<char, T> is the same type as
     std::istream then print it as std::istream.
     """
 
-    def __init__(self, match, name):
+    def __init__(self, match, name, targ1 = None):
         self.match = match
         self.name = name
+        self.targ1 = targ1
         self.enabled = True
 
     class _recognizer(object):
         "The recognizer class for FilteringTypePrinter."
 
-        def __init__(self, match, name):
+        def __init__(self, match, name, targ1):
             self.match = match
             self.name = name
+            self.targ1 = targ1
             self.type_obj = None
 
         def recognize(self, type_obj):
@@ -2075,7 +2085,11 @@ class FilteringTypePrinter(object):
                 return None
 
             if self.type_obj is None:
-                if not type_obj.tag.startswith(self.match):
+                if self.tag1 is not None:
+                    if not type_obj.tag.startswith('{}<{}'.format(self.match, self.targ1):
+                        # Filter didn't match.
+                        return None
+                elif not type_obj.tag.startswith(self.match):
                     # Filter didn't match.
                     return None
                 try:
@@ -2084,18 +2098,23 @@ class FilteringTypePrinter(object):
                     pass
             if self.type_obj == type_obj:
                 return strip_inline_namespaces(self.name)
+
+            if self.match.split('::')[-1] is 'basic_string':
+                if self.type_obj.tag.replace('__cxx11::', '') == type_obj.tag.replace('__cxx11::', ''):
++                    return strip_inline_namespaces(self.name)
+
             return None
 
     def instantiate(self):
         "Return a recognizer object for this type printer."
         return self._recognizer(self.match, self.name)
 
-def add_one_type_printer(obj, match, name):
-    printer = FilteringTypePrinter('std::' + match, 'std::' + name)
+def add_one_type_printer(obj, match, name, targ1 = None):
+    printer = FilteringTypePrinter('std::' + match, 'std::' + name, targ1)
     gdb.types.register_type_printer(obj, printer)
     if _versioned_namespace:
         ns = 'std::' + _versioned_namespace
-        printer = FilteringTypePrinter(ns + match, ns + name)
+        printer = FilteringTypePrinter(ns + match, ns + name, targ1)
         gdb.types.register_type_printer(obj, printer)
 
 def register_type_printers(obj):
  

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.
  

On Sat, 1 Oct 2022 at 11:43, François Dumont <frs.dumont@gmail.com> wrote:
>
> 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.

Most of my patch is an alternative way to make the filter match on
"basic_string<char", but there's also an alternative way to check for
the ambiguous string typedefs, by using match.split('::')[-1] to get
the class template name without namespaces and then compare that to
"basic_string".

> 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.

Yeah it looks like it's just an optimization to fail faster without
having to do gdb.lookup_type.

Please revert the changes to register_type_printers then, and we can
consider that part later if we want to revisit it. I'm not opposed to
making that fail-fast optimization, as long as we keep the property
that FilteringTypePrinter.match is the class template name. Maybe it
should be renamed to something other than "match" to make that clear.

The rest of the patch is OK for trunk, thanks.

> 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.

I've not noticed any performance problems with the printers, but I
have wondered how many printers is too many. That's an interesting
observation about the order they're checked. I'll talk to some of the
GDB devs to find out if they think it's something we should worry
about. Let's not try make premature optimizations until we know if it
matters.

>
>
> >
> > @@ -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.
>
>
  

On 01/10/22 17:30, Jonathan Wakely wrote:
> On Sat, 1 Oct 2022 at 11:43, François Dumont <frs.dumont@gmail.com> wrote:
>> 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.
> Most of my patch is an alternative way to make the filter match on
> "basic_string<char", but there's also an alternative way to check for
> the ambiguous string typedefs, by using match.split('::')[-1] to get
> the class template name without namespaces and then compare that to
> "basic_string".
>
>> 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.
> Yeah it looks like it's just an optimization to fail faster without
> having to do gdb.lookup_type.
>
> Please revert the changes to register_type_printers then, and we can
> consider that part later if we want to revisit it. I'm not opposed to
> making that fail-fast optimization, as long as we keep the property
> that FilteringTypePrinter.match is the class template name. Maybe it
> should be renamed to something other than "match" to make that clear.

Or change the doc ? For my info, why is it so important to comply to the 
current doc ? Is it extracted from some gdb doc ?

Now that the problem is fixed it is less important but I never managed 
to find any doc about this gdb feature that we are relying on.


>
> The rest of the patch is OK for trunk, thanks.
>
>> 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.
> I've not noticed any performance problems with the printers, but I
> have wondered how many printers is too many. That's an interesting
> observation about the order they're checked. I'll talk to some of the
> GDB devs to find out if they think it's something we should worry
> about. Let's not try make premature optimizations until we know if it
> matters.

Yes, but with the 1st registration and so the last evaluation being 
'std::string' it sounds more like a premature lowering ;-)
  

On Mon, 3 Oct 2022 at 17:51, François Dumont <frs.dumont@gmail.com> wrote:
>
> On 01/10/22 17:30, Jonathan Wakely wrote:
> > On Sat, 1 Oct 2022 at 11:43, François Dumont <frs.dumont@gmail.com> wrote:
> >> 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.
> > Most of my patch is an alternative way to make the filter match on
> > "basic_string<char", but there's also an alternative way to check for
> > the ambiguous string typedefs, by using match.split('::')[-1] to get
> > the class template name without namespaces and then compare that to
> > "basic_string".
> >
> >> 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.
> > Yeah it looks like it's just an optimization to fail faster without
> > having to do gdb.lookup_type.
> >
> > Please revert the changes to register_type_printers then, and we can
> > consider that part later if we want to revisit it. I'm not opposed to
> > making that fail-fast optimization, as long as we keep the property
> > that FilteringTypePrinter.match is the class template name. Maybe it
> > should be renamed to something other than "match" to make that clear.
>
> Or change the doc ? For my info, why is it so important to comply to the
> current doc ? Is it extracted from some gdb doc ?

The 'match' argument is the name of a class template to match. If we
want to match a class template with specific template arguments, I
would prefer to pass in the name of the class template and the names
of the template argument types, not just a string. We can do more with
individually named types, rather than just munging it all into a
single string and only being able to do string comparisons.


>
> Now that the problem is fixed it is less important but I never managed
> to find any doc about this gdb feature that we are relying on.

It's this one:
https://sourceware.org/gdb/onlinedocs/gdb/Type-Printing-API.html

>
>
> >
> > The rest of the patch is OK for trunk, thanks.
> >
> >> 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.
> > I've not noticed any performance problems with the printers, but I
> > have wondered how many printers is too many. That's an interesting
> > observation about the order they're checked. I'll talk to some of the
> > GDB devs to find out if they think it's something we should worry
> > about. Let's not try make premature optimizations until we know if it
> > matters.
>
> Yes, but with the 1st registration and so the last evaluation being
> 'std::string' it sounds more like a premature lowering ;-)

Ha, yes, maybe.