public inbox for
 help / color / mirror / Atom feed
From: "François Dumont" <>
To: Jonathan Wakely <>
Cc: "" <>,
	gcc-patches <>
Subject: Re: [PATCH] Fix gdb printers for std::string
Date: Mon, 3 Oct 2022 18:51:25 +0200	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

On 01/10/22 17:30, Jonathan Wakely wrote:
> On Sat, 1 Oct 2022 at 11:43, François Dumont <> wrote:
>> On 01/10/22 12:06, Jonathan Wakely wrote:
>>> On Sat, 1 Oct 2022 at 08:20, François Dumont via Libstdc++
>>> <> wrote:
>>>> I had forgotten to re-run tests after I removed the #define
>>>> 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/
>>>> (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 ;-)

  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:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \

* 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).