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

[Jonathan Wakely <jwakely@redhat.com>]

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.