From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 3C0EE3858C20 for ; Sat, 1 Oct 2022 15:31:03 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 3C0EE3858C20 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1664638262; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=ja+svccG2jVQ/vxfSM1q0ICQi3oZU9QAeVZzkX5ZLmg=; b=M6uYj5K20Fm6pGR8ioxzrtPy+1HrES04hAD78dArbtmfFJt2BfzDeRcD4p2biQy3aCk5nW yF1XYcgx8e/6ZB1zpkV3FQPLQq2CmS+l9gu0qu8oijEu3VxTR9iQz0aoOhclyJ5n+oncNQ cZK5/FaWLjtOqZ0q4TgEsAYn1SRBYDk= Received: from mail-qk1-f200.google.com (mail-qk1-f200.google.com [209.85.222.200]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-240-ivckoFZdPgywNhFeOnV7jg-1; Sat, 01 Oct 2022 11:30:59 -0400 X-MC-Unique: ivckoFZdPgywNhFeOnV7jg-1 Received: by mail-qk1-f200.google.com with SMTP id bl17-20020a05620a1a9100b006cdf19243acso5905427qkb.4 for ; Sat, 01 Oct 2022 08:30:59 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date; bh=ja+svccG2jVQ/vxfSM1q0ICQi3oZU9QAeVZzkX5ZLmg=; b=zMwUKvlOhNMmmoNc6+TtEOhwKSuRyLAVMmke8q86jtG6nXftPaOp4VGIbkc/GB2X9a Zrt3Hv19jNzRQmpMqvJlO9pSvszjzTb+Y19sZBXy4LMZLFJMbxhOQDP2Qt6tm5EQp7iD 90sWBfSphHdxmhCa/JACBsv7T+H9lXRAO4KLhE49WwrtPKys9ZQNCLi8Ci/3YVNxOXep uA2Kn+r8CILQsZQfAjoPzGkkt1wgnSCgUAKkUYmZy1pKQV1zNhRm6XfHsBHgds3NFY8C yyVefN58UqEBbbxZ4tvgSza/PLQtGJEiWr2e/29Q4/nvAe+tKKWt+h0MwmGgA+6rcDXu TRWQ== X-Gm-Message-State: ACrzQf0hF95O/jAl4nXV7xnWxG6xT8EmdDnj437u7XlaeVZaV2rQC9b+ AZmi1aNpJydLSjKtsB9fkxklziBK4pjC4VtPNc4mGMuaZRquiUvvxpPbBpjtuF7OP0sE/VIXPQt tG1tPFJRRIPh3U+UOY95bOTG95nHPonf9Qg== X-Received: by 2002:ae9:e816:0:b0:6cd:d849:859c with SMTP id a22-20020ae9e816000000b006cdd849859cmr9679953qkg.287.1664638258725; Sat, 01 Oct 2022 08:30:58 -0700 (PDT) X-Google-Smtp-Source: AMsMyM5g3eVut8Xlnhm3xljuITcvu4RPvd3CBty6dii9R/LC9UheVNGD+epa7IqlMTKST5FtOQgeX8Rt0gsSjbjNIKE= X-Received: by 2002:ae9:e816:0:b0:6cd:d849:859c with SMTP id a22-20020ae9e816000000b006cdd849859cmr9679945qkg.287.1664638258502; Sat, 01 Oct 2022 08:30:58 -0700 (PDT) MIME-Version: 1.0 References: <1d7b853e-0180-718c-1947-2517272f44e8@gmail.com> <2eb944c4-131e-001d-a3a6-cf1b7aea5614@gmail.com> In-Reply-To: <2eb944c4-131e-001d-a3a6-cf1b7aea5614@gmail.com> From: Jonathan Wakely Date: Sat, 1 Oct 2022 16:30:47 +0100 Message-ID: Subject: Re: [PATCH] Fix gdb printers for std::string To: =?UTF-8?Q?Fran=C3=A7ois_Dumont?= Cc: "libstdc++@gcc.gnu.org" , gcc-patches X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-6.8 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Sat, 1 Oct 2022 at 11:43, Fran=C3=A7ois Dumont wr= ote: > > On 01/10/22 12:06, Jonathan Wakely wrote: > > On Sat, 1 Oct 2022 at 08:20, Fran=C3=A7ois Dumont via Libstdc++ > > 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 real= ly > >> 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::str= ing > >> and other > >> similar typedef are ambiguous from a gdb point of view because i= t > >> matches both > >> std::basic_string and std::__cxx11::basic_string > >> symbols. For those > >> typedef add a workaround to accept the substitution as long as t= he > >> 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 li= mit > >> false positive in > >> FilteringTypePrinter._recognize.recognize requiring to l= ook > >> 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 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. 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. > > > > > > @@ -2105,29 +2120,29 @@ def register_type_printers(obj): > > return > > > > # Add type printers for typedefs std::string, std::wstring etc. > > - for ch in ('', 'w', 'u8', 'u16', 'u32'): > > - add_one_type_printer(obj, 'basic_string', ch + 'string') > > - add_one_type_printer(obj, '__cxx11::basic_string', ch + 'strin= g') > > + for ch in (('char', ''), ('wchar_t', 'w'), ('char8_t', 'u8'), > > ('char16_t', 'u16'), ('char32_t', 'u32')): > > + add_one_type_printer(obj, 'basic_string<' + ch[0], ch[1] + 'st= ring') > > + add_one_type_printer(obj, '__cxx11::basic_string<' + ch[0], > > ch[1] + 'string') > > > > > > As the docs for FilteringTypePrinter says, the first argument is > > supposed to be the class template name: > > > > class FilteringTypePrinter(object): > > r""" > > A type printer that uses typedef names for common template special= izations. > > > > Args: > > match (str): The class template to recognize. > > name (str): The typedef-name that will be used instead. > > > > Checks if a specialization of the class template 'match' is the sa= me type > > as the typedef 'name', and prints it as 'name' instead. > > > > e.g. if an instantiation of std::basic_istream is the same t= ype as > > std::istream then print it as std::istream. > > """ > > > > With this change, the "class template" is sometimes just a string > > prefix of a particular specialization, e.g. "basic_string > The class template is "basic_string", and that's how the > > FilteringTypePrinter was intended to work. > > > > How about something like the attached (untested) change instead. which > > keeps the 'match' argument as the class template name, but optionally > > supports passing the first template argument? Then you can use > > match.split('::')[-1] is 'basic_string' to check if we are dealing > > with one of the possibly ambiguous typedefs. > >