Message ID | 20220923155013.124413-1-blarsen@redhat.com |
---|---|
State | Superseded |
Headers |
Return-Path: <gdb-patches-bounces+patchwork=sourceware.org@sourceware.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 08A0B3857000 for <patchwork@sourceware.org>; Fri, 23 Sep 2022 15:54:36 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 08A0B3857000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1663948476; bh=lfGdwVqNoAA8Mlf0pjh1UPg5gJ/D5iSpDWT6PkThxZM=; h=To:Subject:Date:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:From; b=lvuRa+6MfSvAE80wXCvjYn3n92v6vR68SDfK2SvciKG73ogu30jf5sEdNf+42Vqxx mtub5narKuyBb1jJvTxVgx16ZiIAqMrUnH7u95nxVvv1ISCgcCv7yTPNsizTEzZt38 87lf/VG+5Dreao5m22l0x0tuWFA5IL59+OLwMbnI= X-Original-To: gdb-patches@sourceware.org Delivered-To: gdb-patches@sourceware.org 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 216A93857B8D for <gdb-patches@sourceware.org>; Fri, 23 Sep 2022 15:54:13 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 216A93857B8D Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-517-WOliSVMiO0GOFj6hyLwD1A-1; Fri, 23 Sep 2022 11:54:11 -0400 X-MC-Unique: WOliSVMiO0GOFj6hyLwD1A-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.rdu2.redhat.com [10.11.54.8]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 1F867185A79C for <gdb-patches@sourceware.org>; Fri, 23 Sep 2022 15:54:11 +0000 (UTC) Received: from fedora.redhat.com (unknown [10.40.192.31]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 1CB44C15BA4; Fri, 23 Sep 2022 15:54:09 +0000 (UTC) To: gdb-patches@sourceware.org Subject: [PATCH] Fix printing destructors with void in parameters for clang programs Date: Fri, 23 Sep 2022 17:50:14 +0200 Message-Id: <20220923155013.124413-1-blarsen@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.1 on 10.11.54.8 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Transfer-Encoding: 8bit Content-Type: text/plain; charset="US-ASCII"; x-default=true X-Spam-Status: No, score=-12.4 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, 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 <gdb-patches.sourceware.org> List-Unsubscribe: <https://sourceware.org/mailman/options/gdb-patches>, <mailto:gdb-patches-request@sourceware.org?subject=unsubscribe> List-Archive: <https://sourceware.org/pipermail/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-request@sourceware.org?subject=help> List-Subscribe: <https://sourceware.org/mailman/listinfo/gdb-patches>, <mailto:gdb-patches-request@sourceware.org?subject=subscribe> From: Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org> Reply-To: Bruno Larsen <blarsen@redhat.com> Errors-To: gdb-patches-bounces+patchwork=sourceware.org@sourceware.org Sender: "Gdb-patches" <gdb-patches-bounces+patchwork=sourceware.org@sourceware.org> |
Series |
Fix printing destructors with void in parameters for clang programs
|
|
Commit Message
Guinevere Larsen
Sept. 23, 2022, 3:50 p.m. UTC
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(). Fix this behavior by explicitly testing if there were 0 arguments. --- There is another possibility for a fix, which is to stop ignoring the first parameter, but there is a comment at that part of the code that says "some compilers may not support artificial parameters". I'm not sure how true that still is, and I would certainly like a solution like that more, since (as keiths points out in here: https://sourceware.org/bugzilla/show_bug.cgi?id=8218) there is no actual rule saying that compilers must use an artificial 'this' as the first parameter. If anyone knows, please share with the class :) --- gdb/c-typeprint.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Comments
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? [ 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 <A::A()> $2 = {void (A * const)} 0x40050a <A::~A()> ... So, I wonder if the expression parsing could be improved to handle the "A (void) == A ()" equivalence. But that aside. ] 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. > - 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. 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 ? Thanks, - Tom
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. > > [ 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 <A::A()> > $2 = {void (A * const)} 0x40050a <A::~A()> > ... > > 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 However, I don't think this distinction is all that important for the debugger, so I'm fine with removing the language check. > > 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. Cheers, Bruno > > Thanks, > - Tom >
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 <A::A()> >> $2 = {void (A * const)} 0x40050a <A::~A()> >> ... >> >> 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
diff --git a/gdb/c-typeprint.c b/gdb/c-typeprint.c index 3a611cdac5d..8e05bdc81fe 100644 --- a/gdb/c-typeprint.c +++ b/gdb/c-typeprint.c @@ -300,7 +300,7 @@ cp_type_print_method_args (struct type *mtype, const char *prefix, } else if (varargs) gdb_printf (stream, "..."); - else if (language == language_cplus) + else if (language == language_cplus && nargs == 0) gdb_printf (stream, "void"); gdb_printf (stream, ")");