From: "François Dumont" <frs.dumont@gmail.com>
To: Jonathan Wakely <jwakely@redhat.com>
Cc: "libstdc++@gcc.gnu.org" <libstdc++@gcc.gnu.org>,
gcc-patches <gcc-patches@gcc.gnu.org>
Subject: Re: [PATCH] Fix gdb printers for std::string
Date: Mon, 3 Oct 2022 18:51:25 +0200 [thread overview]
Message-ID: <36ff419f-ff22-f954-b62b-8d7e62920fb0@gmail.com> (raw)
In-Reply-To: <CACb0b4nMDFxPsRZ=B5610Ffiq46Lmq5mtv_O2xSXgnfvN+shSQ@mail.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 ;-)
next prev parent reply other threads:[~2022-10-03 16:51 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-09-28 20:42 François Dumont
2022-10-01 7:20 ` François Dumont
2022-10-01 10:06 ` Jonathan Wakely
2022-10-01 10:43 ` François Dumont
2022-10-01 15:30 ` Jonathan Wakely
2022-10-03 16:51 ` François Dumont [this message]
2022-10-03 17:14 ` Jonathan Wakely
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=36ff419f-ff22-f954-b62b-8d7e62920fb0@gmail.com \
--to=frs.dumont@gmail.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=jwakely@redhat.com \
--cc=libstdc++@gcc.gnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).