Message ID | e1355b5b-71cc-6726-c4e2-c1828d7a5850@gmail.com |
---|---|
State | New |
Headers | show |
Series | Fix gdb printers for std::string | expand |
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.
diff --git a/libstdc++-v3/python/libstdcxx/v6/printers.py b/libstdc++-v3/python/libstdcxx/v6/printers.py index 245b6e3dbcd..b4878b93bb2 100644 --- a/libstdc++-v3/python/libstdcxx/v6/printers.py +++ b/libstdc++-v3/python/libstdcxx/v6/printers.py @@ -1857,7 +1857,7 @@ class Printer(object): # Add a name using _GLIBCXX_BEGIN_NAMESPACE_VERSION. def add_version(self, base, name, function): self.add(base + name, function) - if _versioned_namespace: + if _versioned_namespace and not '__cxx11' in base: vbase = re.sub('^(std|__gnu_cxx)::', r'\g<0>%s' % _versioned_namespace, base) self.add(vbase + name, function) @@ -2026,7 +2026,7 @@ def add_one_template_type_printer(obj, name, defargs): printer = TemplateTypePrinter('std::__debug::'+name, defargs) gdb.types.register_type_printer(obj, printer) - if _versioned_namespace: + if _versioned_namespace and not '__cxx11' in name: # Add second type printer for same type in versioned namespace: ns = 'std::' + _versioned_namespace # PR 86112 Cannot use dict comprehension here: @@ -2084,6 +2084,21 @@ class FilteringTypePrinter(object): pass if self.type_obj == type_obj: return strip_inline_namespaces(self.name) + + if self.type_obj is None: + return None + + # Workaround ambiguous typedefs matching both std:: and std::__cxx11:: symbols. + ambiguous = False + for ch in ('', 'w', 'u8', 'u16', 'u32'): + if self.name == 'std::' + ch + 'string': + ambiguous = True + break + + if ambiguous: + if self.type_obj.tag.replace('__cxx11::', '') == type_obj.tag.replace('__cxx11::', ''): + return strip_inline_namespaces(self.name) + return None def instantiate(self): @@ -2093,7 +2108,7 @@ class FilteringTypePrinter(object): def add_one_type_printer(obj, match, name): printer = FilteringTypePrinter('std::' + match, 'std::' + name) gdb.types.register_type_printer(obj, printer) - if _versioned_namespace: + if _versioned_namespace and not '__cxx11' in match: ns = 'std::' + _versioned_namespace printer = FilteringTypePrinter(ns + match, ns + name) gdb.types.register_type_printer(obj, printer) @@ -2105,29 +2120,26 @@ 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') - # Typedefs for __cxx11::basic_string used to be in namespace __cxx11: - add_one_type_printer(obj, '__cxx11::basic_string', - '__cxx11::' + ch + 'string') - add_one_type_printer(obj, 'basic_string_view', ch + 'string_view') + 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], '__cxx11::' + ch[1] + 'string') + add_one_type_printer(obj, 'basic_string_view<' + ch[0], ch[1] + 'string_view') # Add type printers for typedefs std::istream, std::wistream etc. - for ch in ('', 'w'): + for ch in (('char', ''), ('wchar_t', 'w')): for x in ('ios', 'streambuf', 'istream', 'ostream', 'iostream', 'filebuf', 'ifstream', 'ofstream', 'fstream'): - add_one_type_printer(obj, 'basic_' + x, ch + x) + add_one_type_printer(obj, 'basic_' + x + '<' + ch[0], ch[1] + x) for x in ('stringbuf', 'istringstream', 'ostringstream', 'stringstream'): - add_one_type_printer(obj, 'basic_' + x, ch + x) + add_one_type_printer(obj, 'basic_' + x, ch[1] + x) # <sstream> types are in __cxx11 namespace, but typedefs aren't: - add_one_type_printer(obj, '__cxx11::basic_' + x, ch + x) + add_one_type_printer(obj, '__cxx11::basic_' + x + '<' + ch[0], ch[1] + x) # Add type printers for typedefs regex, wregex, cmatch, wcmatch etc. for abi in ('', '__cxx11::'): - for ch in ('', 'w'): - add_one_type_printer(obj, abi + 'basic_regex', abi + ch + 'regex') + for ch in (('char', ''), ('wchar_t', 'w')): + add_one_type_printer(obj, abi + 'basic_regex<' + ch[0], abi + ch[1] + 'regex') for ch in ('c', 's', 'wc', 'ws'): add_one_type_printer(obj, abi + 'match_results', abi + ch + 'match') for x in ('sub_match', 'regex_iterator', 'regex_token_iterator'): diff --git a/libstdc++-v3/testsuite/libstdc++-prettyprinters/80276.cc b/libstdc++-v3/testsuite/libstdc++-prettyprinters/80276.cc index 4abe7d384e8..d1016b58d79 100644 --- a/libstdc++-v3/testsuite/libstdc++-prettyprinters/80276.cc +++ b/libstdc++-v3/testsuite/libstdc++-prettyprinters/80276.cc @@ -18,9 +18,6 @@ // with this library; see the file COPYING3. If not see // <http://www.gnu.org/licenses/>. -// Type printers only recognize the old std::string for now. -#define _GLIBCXX_USE_CXX11_ABI 0 - #include <iostream> #include <list> #include <memory> @@ -46,7 +43,7 @@ main() // { dg-final { whatis-regexp-test p1 "std::unique_ptr<std::(__debug::)?vector<std::unique_ptr<std::(__debug::)?vector<int>\\*>>>" } } // { dg-final { whatis-regexp-test p2 "std::unique_ptr<std::(__debug::)?vector<std::unique_ptr<std::(__debug::)?set<int>\\*>>\\\[\\\]>" } } // { dg-final { whatis-regexp-test p3 "std::unique_ptr<std::(__debug::)?set<std::unique_ptr<std::(__debug::)?vector<int>\\*>>\\\[10\\\]>" } } - // { dg-final { whatis-regexp-test p4 "std::unique_ptr<std::(__debug::)?vector<std::unique_ptr<std::(__debug::)?list<std::string>\\\[\\\]>>\\\[99\\\]>" { xfail { c++20 || debug_mode } } } } + // { dg-final { whatis-regexp-test p4 "std::unique_ptr<std::(__debug::)?vector<std::unique_ptr<std::(__debug::)?list<std::string>\\\[\\\]>>\\\[99\\\]>" } } placeholder(&p1); // Mark SPOT placeholder(&p2); diff --git a/libstdc++-v3/testsuite/libstdc++-prettyprinters/cxx17.cc b/libstdc++-v3/testsuite/libstdc++-prettyprinters/cxx17.cc index e52ffbbcc15..cf699d22e78 100644 --- a/libstdc++-v3/testsuite/libstdc++-prettyprinters/cxx17.cc +++ b/libstdc++-v3/testsuite/libstdc++-prettyprinters/cxx17.cc @@ -18,9 +18,6 @@ // with this library; see the file COPYING3. If not see // <http://www.gnu.org/licenses/>. -// Type printers only recognize the old std::string for now. -#define _GLIBCXX_USE_CXX11_ABI 0 - #include <filesystem> #include <any> #include <optional> diff --git a/libstdc++-v3/testsuite/libstdc++-prettyprinters/libfundts.cc b/libstdc++-v3/testsuite/libstdc++-prettyprinters/libfundts.cc index e81308d4f7e..b2f464d0894 100644 --- a/libstdc++-v3/testsuite/libstdc++-prettyprinters/libfundts.cc +++ b/libstdc++-v3/testsuite/libstdc++-prettyprinters/libfundts.cc @@ -18,9 +18,6 @@ // with this library; see the file COPYING3. If not see // <http://www.gnu.org/licenses/>. -// Type printers only recognize the old std::string for now. -#define _GLIBCXX_USE_CXX11_ABI 0 - #include <experimental/any> #include <experimental/optional> #include <experimental/string_view> @@ -50,7 +47,7 @@ main() om = std::map<int, double>{ {1, 2.}, {3, 4.}, {5, 6.} }; // { dg-final { regexp-test om {std::experimental::optional<std::(__debug::)?map<int, double>> containing std::(__debug::)?map with 3 elements = {\[1\] = 2, \[3\] = 4, \[5\] = 6}} } } optional<std::string> os{ "stringy" }; -// { dg-final { note-test os {std::experimental::optional<std::string> = {[contained value] = "stringy"}} { xfail { c++20 || debug_mode } } } } +// { dg-final { note-test os {std::experimental::optional<std::string> = {[contained value] = "stringy"}} } } any a; // { dg-final { note-test a {std::experimental::any [no contained value]} } } @@ -61,7 +58,7 @@ main() any ap = (void*)nullptr; // { dg-final { note-test ap {std::experimental::any containing void * = {[contained value] = 0x0}} } } any as = *os; -// { dg-final { note-test as {std::experimental::any containing std::string = {[contained value] = "stringy"}} { xfail { c++20 || debug_mode } } } } +// { dg-final { note-test as {std::experimental::any containing std::string = {[contained value] = "stringy"}} } } any as2("stringiest"); // { dg-final { regexp-test as2 {std::experimental::any containing const char \* = {\[contained value\] = 0x[[:xdigit:]]+ "stringiest"}} } } any am = *om; diff --git a/libstdc++-v3/testsuite/libstdc++-prettyprinters/simple.cc b/libstdc++-v3/testsuite/libstdc++-prettyprinters/simple.cc index 1609ae2c8db..3d14120371e 100644 --- a/libstdc++-v3/testsuite/libstdc++-prettyprinters/simple.cc +++ b/libstdc++-v3/testsuite/libstdc++-prettyprinters/simple.cc @@ -20,9 +20,6 @@ // with this library; see the file COPYING3. If not see // <http://www.gnu.org/licenses/>. -// Type printers only recognize the old std::string for now. -#define _GLIBCXX_USE_CXX11_ABI 0 - #include <string> #include <deque> #include <bitset> diff --git a/libstdc++-v3/testsuite/libstdc++-prettyprinters/simple11.cc b/libstdc++-v3/testsuite/libstdc++-prettyprinters/simple11.cc index a4b82e30f9c..367e04579ca 100644 --- a/libstdc++-v3/testsuite/libstdc++-prettyprinters/simple11.cc +++ b/libstdc++-v3/testsuite/libstdc++-prettyprinters/simple11.cc @@ -20,9 +20,6 @@ // with this library; see the file COPYING3. If not see // <http://www.gnu.org/licenses/>. -// Type printers only recognize the old std::string for now. -#define _GLIBCXX_USE_CXX11_ABI 0 - #include <string> #include <deque> #include <bitset> diff --git a/libstdc++-v3/testsuite/libstdc++-prettyprinters/whatis.cc b/libstdc++-v3/testsuite/libstdc++-prettyprinters/whatis.cc index 046d26f0020..23b9947a5de 100644 --- a/libstdc++-v3/testsuite/libstdc++-prettyprinters/whatis.cc +++ b/libstdc++-v3/testsuite/libstdc++-prettyprinters/whatis.cc @@ -18,10 +18,6 @@ // with this library; see the file COPYING3. If not see // <http://www.gnu.org/licenses/>. -// GDB can't find global variables using the abi_tag attribute. -// https://sourceware.org/bugzilla/show_bug.cgi?id=19436 -#define _GLIBCXX_USE_CXX11_ABI 0 - #include <string> #include <iostream> #include <regex>