From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out1.suse.de (smtp-out1.suse.de [195.135.220.28]) by sourceware.org (Postfix) with ESMTPS id 020433858C2D for ; Tue, 27 Sep 2022 10:51:02 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 020433858C2D Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by smtp-out1.suse.de (Postfix) with ESMTPS id 06D6421E55; Tue, 27 Sep 2022 10:51:01 +0000 (UTC) Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by imap2.suse-dmz.suse.de (Postfix) with ESMTPS id E4A2B139B3; Tue, 27 Sep 2022 10:51:00 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap2.suse-dmz.suse.de with ESMTPSA id q7WlNpTVMmMgWQAAMHmgww (envelope-from ); Tue, 27 Sep 2022 10:51:00 +0000 Message-ID: Date: Tue, 27 Sep 2022 12:51:00 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.12.0 Subject: Re: [PATCH] Fix printing destructors with void in parameters for clang programs Content-Language: en-US To: Bruno Larsen , gdb-patches@sourceware.org References: <20220923155013.124413-1-blarsen@redhat.com> <4baea1af-e671-239c-e3c3-d91e222d9929@suse.de> <856a18c7-657c-1f4f-892f-8d992f60e16b@redhat.com> From: Tom de Vries In-Reply-To: <856a18c7-657c-1f4f-892f-8d992f60e16b@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-6.3 required=5.0 tests=BAYES_00, BODY_8BITS, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, NICE_REPLY_A, SPF_HELO_NONE, SPF_PASS, 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 10:51:05 -0000 On 9/26/22 10:29, Bruno Larsen wrote: > On 24/09/2022 01:00, Tom de Vries wrote: >> On 9/23/22 17:50, Bruno Larsen via Gdb-patches wrote: >>> When GDB prints the methods of a C++ class, it will always skip the >>> first >>> parameter, assuming it to be a 'this' pointer. However, when deciding if >>> it should print "void" for the parameters, it disregards the fact that >>> the first parameter was skipped, so if the method  only had that >>> parameter, for instance how clang compiles destructors, we'd see >>> ~foo(void) instead of ~foo(). >>> >> >> Hi, >> >> I wrote the following test-case to investigate the problem: >> ... >> $ cat test.cc >> class A { >> public: >>   int a; >> >>   A() { >>     this->a = 3; >>   } >>   ~A() { >>     a = 0; >>   } >> }; >> >> int >> main (void) >> { >>   A a; >> >>   return a.a; >> } >> $ g++ test.cc -g >> $ ./a.out; echo $? >> 3 >> ... >> >> With g++ (7.5.0) I see: >> ... >> $ gdb -q -batch a.out -ex "ptype A" >> type = class A { >>   public: >>     int a; >> >>     A(void); >>     ~A(); >> } >> ... >> and with clang++ (13.0.1): >> ... >> $ gdb -q -batch a.out -ex "ptype A" >> type = class A { >>   public: >>     int a; >> >>     A(void); >>     ~A(void); >> } >> ... >> >> So, ok, I see how the patch makes the output for clang++ and g++ the >> same, getting us "A()" and "~A()".  I don't have a preference of say >> ~A::A() vs A::~A(void), but I suppose the former is the newer, more >> c++-like style, and the latter leaves less room for confusion.  Do you >> have a preference, or were you merely trying to make the output for >> the destructor the same for both cases? > > Hi! > > Thanks for the testcase. Much more concise than the one I was using and > forgot to mention (gdb.cp/templates.exp). > > As for a reasoning for the change, my personal preference is > consistency, but while looking over this bug, I found this PR c++/8218 > (https://sourceware.org/bugzilla/show_bug.cgi?id=8218), and seeing > Keith's history with this area, I asked him, and he stated that he > believes ~A(void) is definitely a bug. > Since you're after consistency, I've submitted a patch ( https://sourceware.org/pipermail/gdb-patches/2022-September/192155.html ) that achieves that, by printing ~A(void). I'm not sure in what sense that's a bug, but we'll have that discussion there. >> >> [ But now look at the expression parsing side.  This does not get us >> anything, with both g++ and clang++: >> ... >> $ gdb -q -batch a.out -ex "p A::A()" -ex "p A::~A()" >> Cannot resolve method A::A to any overloaded instance >> Cannot resolve method A::~A to any overloaded instance >> ... >> but again with both g++ and clang++ (and with and without your patch): >> ... >> $ gdb -q -batch a.out -ex "p A::A(void)" -ex "p A::~A(void)" >> $1 = {void (A * const)} 0x4004f4 >> $2 = {void (A * const)} 0x40050a >> ... >> >> So, I wonder if the expression parsing could be improved to handle the >> "A (void) == A ()" equivalence.  But that aside. ] > [ That's a good point. I think this is worth fixing, especially if we > start printing constructors and destructors without (void) consistently. ] >> >> Anyway, an interesting detail is that the g++-generated destructor was >> already printed without void.  Investigation shows that it is due to >> having two artificial parameters. I think that shows that we're making >> decisions about how to handle similar things in different places in >> the code, which means it needs a bit of restructuring. > Yeah, the commit that creates this behavior is mentioned in the bug I > linked above. It was specifically fixing GDB printing A::A(int) when the > int parameter was just artificial. >> >>> -  else if (language == language_cplus) >>> +  else if (language == language_cplus && nargs == 0) >>>       gdb_printf (stream, "void"); >> >> I remain confused about why we're doing things differently based on >> language. > > There is a difference in semantics between C and C++ in function > declarations with an empty parameter list, as I found in this stack > overflow post: > https://softwareengineering.stackexchange.com/questions/286490/what-is-the-difference-between-function-and-functionvoid > There is indeed. All the more reason to print void when language is C. > > However, I don't think this distinction is all that important for the > debugger, so I'm fine with removing the language check. > After pondering it a bit more, I realized it could be to avoid void for languages that do not contain the keyword. >> >> Anyway, the nargs == 0 implies staticp, so I wonder if we should not >> handle a static function with implicit first argument the same (not >> sure if that can happen, but even so, maybe we don't want to use that >> assumption into the code), in which case the nargs == 0 doesn't >> address that scenario. >> >> I tried rewriting the code in a more regular for loop structure, in >> attached patch, not fully tested yet but passes at least gdb.cp/*.exp >> for me.  WDYT ? > I also haven't tested this, but it looks like a better solution. Feel > free to make this into a proper patch, or let me know and I'll add the > finishing touches. So, I'm hoping that I get aforementioned patch committed, which should hopefully address your consistency concern. Thanks, - Tom