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 7A6B83858C83 for ; Mon, 26 Sep 2022 08:29:40 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 7A6B83858C83 Received: from mail-wr1-f69.google.com (mail-wr1-f69.google.com [209.85.221.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-574-uj0FQLsiMRyBOE86tnbfdg-1; Mon, 26 Sep 2022 04:29:38 -0400 X-MC-Unique: uj0FQLsiMRyBOE86tnbfdg-1 Received: by mail-wr1-f69.google.com with SMTP id l5-20020adfa385000000b0022a482f8285so924409wrb.5 for ; Mon, 26 Sep 2022 01:29:38 -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:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date; bh=uwiea/9uFaR3vT6fOeP8YDijl30ynqqvctF2lEutYVU=; b=vVBmFMmIcP4agxkQE6QtJrW3kZaIcYErPBhB2mEDLD4in1nYLgdieObOkLycc0R9QB +bQNYU+dFhI54cGzeozCczChCcmm4fFv1B5wvBPuuNIRzBtqSUQw8xy6Unp3GIEXXtnm naf1TwcHFmwRtMz10yVE4b/y4RRicK6zKO6PD7VA/ERbEExapyekgJm4HTXul02/OfcZ kNfbokZeFVl2pfwaCKpMKN7B6i1CoTVF/NyYCL8c1Cq2GItbcBUIq7U5ZLIz2sPp8Ctn GM7hj1OZFpq98gpUXHsLGSPoHzqOviGBLbI0ATVGPy+MMqRW8DVlFvIYr7I/YkJhJZy0 Gg4A== X-Gm-Message-State: ACrzQf2+GScxl87In1QAy07qgqjvRfs8nBP1NnNPFcEyxZ8ZYfkfLzTr ww1gAFUgRe/D/irRJc9FU43SPunO5+W9C8JasnhW54Yo551P+8Bv8bJsS9cQm4Rl2QlnR1jnG0l hXPwEhpnsKKmei3DuMF7cdA== X-Received: by 2002:a05:6000:811:b0:22a:2856:9418 with SMTP id bt17-20020a056000081100b0022a28569418mr12332692wrb.46.1664180977205; Mon, 26 Sep 2022 01:29:37 -0700 (PDT) X-Google-Smtp-Source: AMsMyM7BtcxiIMS56TgTahegYqkRfKUw0xLN60FI+452/1fIdAK+9YuKhPSD5Wg147io/3LBll2USw== X-Received: by 2002:a05:6000:811:b0:22a:2856:9418 with SMTP id bt17-20020a056000081100b0022a28569418mr12332680wrb.46.1664180976974; Mon, 26 Sep 2022 01:29:36 -0700 (PDT) Received: from [192.168.0.45] (ip-213-220-232-121.bb.vodafone.cz. [213.220.232.121]) by smtp.gmail.com with ESMTPSA id n21-20020a05600c4f9500b003b4764442f0sm10773727wmq.11.2022.09.26.01.29.36 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 26 Sep 2022 01:29:36 -0700 (PDT) Message-ID: <856a18c7-657c-1f4f-892f-8d992f60e16b@redhat.com> Date: Mon, 26 Sep 2022 10:29:35 +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] Fix printing destructors with void in parameters for clang programs To: Tom de Vries , gdb-patches@sourceware.org References: <20220923155013.124413-1-blarsen@redhat.com> <4baea1af-e671-239c-e3c3-d91e222d9929@suse.de> From: Bruno Larsen In-Reply-To: <4baea1af-e671-239c-e3c3-d91e222d9929@suse.de> 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: 8bit X-Spam-Status: No, score=-6.0 required=5.0 tests=BAYES_00, BODY_8BITS, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, 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: Mon, 26 Sep 2022 08:29:46 -0000 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 > $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 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 >