On Mon, 3 Oct 2022 at 17:51, François Dumont <frs.dum...@gmail.com> wrote:
>
> On 01/10/22 17:30, Jonathan Wakely wrote:
> > On Sat, 1 Oct 2022 at 11:43, François Dumont <frs.dum...@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++
> >>> <libstd...@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.
