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.129.124]) by sourceware.org (Postfix) with ESMTPS id 770323858C50 for ; Tue, 27 Sep 2022 14:16:38 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 770323858C50 Received: from mail-ej1-f72.google.com (mail-ej1-f72.google.com [209.85.218.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-643-hm4A7lsNNyirZOm2hY1WVQ-1; Tue, 27 Sep 2022 10:16:34 -0400 X-MC-Unique: hm4A7lsNNyirZOm2hY1WVQ-1 Received: by mail-ej1-f72.google.com with SMTP id qb16-20020a1709077e9000b007827e5f3e2dso3931855ejc.3 for ; Tue, 27 Sep 2022 07:16:28 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date; bh=ucCn852RUrKURqJvwqgIi751Zf4j8QiczhMJTECEQlo=; b=ZZhwYcxSoBgT3+vHnbwSyxkeE4Tkl6I6lZ12m/ZYzBSm6+SLimSGUAnowXOCDM+uXU ly2o4FRjQMpwrcjp/5krVDa4wCeMh3fvDustLxuHGf1pU7hk4Zwgom+f8D28QbYs0Rq8 9ZTt82iBewBhO4aQCWaelhGWOYXdIpf1EkA/637zUqNbmpDFyvXugEsD9Xc/wK8ZiWlx 5cqyYOPhPblOn3upNACyIFSos3b5JWj2xDOPVPDSfORFnAavi/gfC+oYWdNTbmZ89GOj S1Mbbw9iuMCWnqbxKQXWnhCZwma2aU0gfPdjsPy5u6AIczNATyia+tnDkYyDw61O9LDp 2xCg== X-Gm-Message-State: ACrzQf0rHSXFOuZWcCfBa73IV3wmS7MgnKtlAhSB14/nXeYtsoxlVU+F fVHT2cJLL6xxyb3KMl6gfR359jz7G7eJmAo4YbejA5i2AgVR9yWjsDoDionAGQJXeNSnVu9kj7T Y3IwKOqhP0FIoEuDSVp8Xjw== X-Received: by 2002:a05:6402:b29:b0:456:f2dc:826d with SMTP id bo9-20020a0564020b2900b00456f2dc826dmr17512095edb.94.1664288186776; Tue, 27 Sep 2022 07:16:26 -0700 (PDT) X-Google-Smtp-Source: AMsMyM7gkHdNZeP01ZYixuv6b2gidjIL15/NFnAh8SfW1gnQl6J6C/M4JdNnydyXXzdjhmQzZ5Se1g== X-Received: by 2002:a05:6402:b29:b0:456:f2dc:826d with SMTP id bo9-20020a0564020b2900b00456f2dc826dmr17512074edb.94.1664288186461; Tue, 27 Sep 2022 07:16:26 -0700 (PDT) Received: from [192.168.0.45] (ip-62-245-66-121.bb.vodafone.cz. [62.245.66.121]) by smtp.gmail.com with ESMTPSA id 17-20020a170906211100b00741383c1c5bsm855468ejt.196.2022.09.27.07.16.25 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 27 Sep 2022 07:16:25 -0700 (PDT) Message-ID: <995b24ae-d801-144f-eb3c-c9632e48e626@redhat.com> Date: Tue, 27 Sep 2022 16:16:24 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.2.1 Subject: Re: [PATCH][gdb/c++] Print destructor the same for gcc and clang To: Tom de Vries , gdb-patches@sourceware.org Cc: Pedro Alves , Keith Seitz References: <20220927103546.GA27362@delia.home> From: Bruno Larsen In-Reply-To: <20220927103546.GA27362@delia.home> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-11.7 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, NICE_REPLY_A, RCVD_IN_BARRACUDACENTRAL, RCVD_IN_DNSWL_LOW, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 27 Sep 2022 14:16:40 -0000 On 27/09/2022 12:35, Tom de Vries wrote: > Hi, > > Consider the test-case contained in this patch. > > With g++ (7.5.0) we have for "ptype A": > ... > type = class A { > public: > int a; > > A(void); > ~A(); > } > ... > and with clang++ (13.0.1): > ... > type = class A { > public: > int a; > > A(void); > ~A(void); > } > ... > and we observe that the destructor is printed differently. > > There's a difference in debug info between the two cases: in the clang case, > there's one artificial parameter, but in the g++ case, there are two, and > these similar cases are handled differently in cp_type_print_method_args. > > This is due to this slightly convoluted bit of code: > ... > i = staticp ? 0 : 1; > if (nargs > i) > { > while (i < nargs) > ... > } > else if (varargs) > gdb_printf (stream, "..."); > else if (language == language_cplus) > gdb_printf (stream, "void"); > ... > > The purpose of "i = staticp ? 0 : 1" is to skip the printing of the implicit > this parameter. > > In commit 5f4d1085085 ("c++/8218: Destructors w/arguments"), skipping of other > artificial parameters was added, but using a different method: rather than > adjusting the potential loop start, it skips the parameter in the loop. > > The observed difference in printing is explained by whether we enter the loop: > - in the clang case, the loop is not entered and we print "void". > - in the gcc case, the loop is entered, and nothing is printed. > > Fix this by rewriting the code to: > - always enter the loop > - handle whether arguments need printing in the loop > - keep track of how many arguments are printed, and > use that after the loop to print void etc. > such that we have the same for both gcc and clang: > ... > A(void); > ~A(void); > ... > > Note that I consider the discussion of whether we want to print: > - A(void) / ~A(void), or > - A() / ~A() > out-of-scope for this patch. > > Tested on x86_64-linux. > > Any comments? > > Thanks, > - Tom > > [gdb/c++] Print destructor the same for gcc and clang Hi Tom, I am ok with these changes, they make the code easier to read and easier to change, based on the output of the A()/A(void) discussion. Cheers, Bruno > --- > gdb/c-typeprint.c | 55 +++++++++++++++++------------- > gdb/testsuite/gdb.base/ptype-offsets.exp | 2 +- > gdb/testsuite/gdb.cp/print-method-args.cc | 38 +++++++++++++++++++++ > gdb/testsuite/gdb.cp/print-method-args.exp | 36 +++++++++++++++++++ > gdb/testsuite/gdb.cp/templates.exp | 4 +-- > 5 files changed, 109 insertions(+), 26 deletions(-) > > diff --git a/gdb/c-typeprint.c b/gdb/c-typeprint.c > index 3a611cdac5d..23e8a5a4d6f 100644 > --- a/gdb/c-typeprint.c > +++ b/gdb/c-typeprint.c > @@ -273,35 +273,44 @@ cp_type_print_method_args (struct type *mtype, const char *prefix, > language_cplus, DMGL_ANSI); > gdb_puts ("(", stream); > > - /* Skip the class variable. We keep this here to accommodate older > - compilers and debug formats which may not support artificial > - parameters. */ > - i = staticp ? 0 : 1; > - if (nargs > i) > + int printed_args = 0; > + for (i = 0; i < nargs; ++i) > { > - while (i < nargs) > + if (i == 0 && !staticp) > { > - struct field arg = args[i++]; > - > - /* Skip any artificial arguments. */ > - if (FIELD_ARTIFICIAL (arg)) > - continue; > + /* Skip the class variable. We keep this here to accommodate older > + compilers and debug formats which may not support artificial > + parameters. */ > + continue; > + } > > - c_print_type (arg.type (), "", stream, 0, 0, language, flags); > + struct field arg = args[i]; > + /* Skip any artificial arguments. */ > + if (FIELD_ARTIFICIAL (arg)) > + continue; > > - if (i == nargs && varargs) > - gdb_printf (stream, ", ..."); > - else if (i < nargs) > - { > - gdb_printf (stream, ", "); > - stream->wrap_here (8); > - } > + if (printed_args > 0) > + { > + gdb_printf (stream, ", "); > + stream->wrap_here (8); > } > + > + c_print_type (arg.type (), "", stream, 0, 0, language, flags); > + printed_args++; > + } > + > + if (varargs) > + { > + if (printed_args == 0) > + gdb_printf (stream, "..."); > + else > + gdb_printf (stream, ", ..."); > + } > + else if (printed_args == 0) > + { > + if (language == language_cplus) > + gdb_printf (stream, "void"); > } > - else if (varargs) > - gdb_printf (stream, "..."); > - else if (language == language_cplus) > - gdb_printf (stream, "void"); > > gdb_printf (stream, ")"); > > diff --git a/gdb/testsuite/gdb.base/ptype-offsets.exp b/gdb/testsuite/gdb.base/ptype-offsets.exp > index e80d876fa32..eb41bafb3e6 100644 > --- a/gdb/testsuite/gdb.base/ptype-offsets.exp > +++ b/gdb/testsuite/gdb.base/ptype-offsets.exp > @@ -104,7 +104,7 @@ gdb_test "ptype /oTM struct abc" \ > "/* 48 | 2 */ my_int_type field9;" \ > "" \ > " abc(void);" \ > -" ~abc();" \ > +" ~abc(void);" \ > "" \ > " typedef short my_int_type;" \ > "/* XXX 6-byte padding */" \ > diff --git a/gdb/testsuite/gdb.cp/print-method-args.cc b/gdb/testsuite/gdb.cp/print-method-args.cc > new file mode 100644 > index 00000000000..98be8286aeb > --- /dev/null > +++ b/gdb/testsuite/gdb.cp/print-method-args.cc > @@ -0,0 +1,38 @@ > +/* This testcase is part of GDB, the GNU debugger. > + > + Copyright (C) 2022 Free Software Foundation, Inc. > + > + This file is part of GDB. > + > + This program is free software; you can redistribute it and/or modify > + it under the terms of the GNU General Public License as published by > + the Free Software Foundation; either version 3 of the License, or > + (at your option) any later version. > + > + This program is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + GNU General Public License for more details. > + > + You should have received a copy of the GNU General Public License > + along with this program. If not, see . */ > + > +class A { > +public: > + int a; > + > + A() { > + this->a = 3; > + } > + ~A() { > + a = 0; > + } > +}; > + > +int > +main (void) > +{ > + A a; > + > + return a.a; > +} > diff --git a/gdb/testsuite/gdb.cp/print-method-args.exp b/gdb/testsuite/gdb.cp/print-method-args.exp > new file mode 100644 > index 00000000000..7df044711b0 > --- /dev/null > +++ b/gdb/testsuite/gdb.cp/print-method-args.exp > @@ -0,0 +1,36 @@ > +# Copyright (C) 2022 Free Software Foundation, Inc. > + > +# This program is free software; you can redistribute it and/or modify > +# it under the terms of the GNU General Public License as published by > +# the Free Software Foundation; either version 3 of the License, or > +# (at your option) any later version. > +# > +# This program is distributed in the hope that it will be useful, > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > +# GNU General Public License for more details. > +# > +# You should have received a copy of the GNU General Public License > +# along with this program. If not, see . > + > +# This file is part of the gdb testsuite. > + > +if { [skip_cplus_tests] } { continue } > + > +standard_testfile .cc > + > +if {[prepare_for_testing "failed to prepare" $testfile $srcfile {debug c++}]} { > + return -1 > +} > + > +set re \ > + [multi_line \ > + "type = class A {" \ > + " public:" \ > + " int a;" \ > + "" \ > + " A\\(void\\);" \ > + " ~A\\(void\\);" \ > + "}"] > + > +gdb_test "ptype A" $re > diff --git a/gdb/testsuite/gdb.cp/templates.exp b/gdb/testsuite/gdb.cp/templates.exp > index 27c9bbc1c7e..d3ede5a5fb6 100644 > --- a/gdb/testsuite/gdb.cp/templates.exp > +++ b/gdb/testsuite/gdb.cp/templates.exp > @@ -68,7 +68,7 @@ proc test_ptype_of_templates {} { > # The destructor has an argument type. > kfail "gdb/8218" "ptype T5" > } > - -re "type = class T5 \{${ws}public:${ws}static int X;${ws}int x;${ws}int val;${ws}T5\\(int\\);${ws}T5\\((T5 const|const T5) ?&\\);${ws}~T5\\(\\);${ws}static void \\* operator new\\((size_t|unsigned( int| long|))\\);${ws}static void operator delete\\(void ?\\*\\);${ws}int value\\((void|)\\);${ws}\}\r\n$gdb_prompt $" { > + -re "type = class T5 \{${ws}public:${ws}static int X;${ws}int x;${ws}int val;${ws}T5\\(int\\);${ws}T5\\((T5 const|const T5) ?&\\);${ws}~T5\\(void\\);${ws}static void \\* operator new\\((size_t|unsigned( int| long|))\\);${ws}static void operator delete\\(void ?\\*\\);${ws}int value\\((void|)\\);${ws}\}\r\n$gdb_prompt $" { > pass "ptype T5" > } > } > @@ -108,7 +108,7 @@ proc test_ptype_of_templates {} { > # The destructor has an argument type. > kfail "gdb/8218" "ptype t5i" > } > - -re "type = class T5 \{${ws}public:${ws}static int X;${ws}int x;${ws}int val;${ws}T5\\(int\\);${ws}T5\\((T5 const|const T5) ?&\\);${ws}~T5\\(\\);${ws}static void \\* operator new\\((size_t|unsigned( int| long|))\\);${ws}static void operator delete\\(void ?\\*\\);${ws}int value\\((void|)\\);${ws}\}\r\n$gdb_prompt $" { > + -re "type = class T5 \{${ws}public:${ws}static int X;${ws}int x;${ws}int val;${ws}T5\\(int\\);${ws}T5\\((T5 const|const T5) ?&\\);${ws}~T5\\(void\\);${ws}static void \\* operator new\\((size_t|unsigned( int| long|))\\);${ws}static void operator delete\\(void ?\\*\\);${ws}int value\\((void|)\\);${ws}\}\r\n$gdb_prompt $" { > pass "ptype t5i" > } > } >