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

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

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 ;-)