public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
From: Bruno Larsen <blarsen@redhat.com>
To: Tom de Vries <tdevries@suse.de>, gdb-patches@sourceware.org
Subject: Re: [PATCH] Fix printing destructors with void in parameters for clang programs
Date: Mon, 26 Sep 2022 10:29:35 +0200	[thread overview]
Message-ID: <856a18c7-657c-1f4f-892f-8d992f60e16b@redhat.com> (raw)
In-Reply-To: <4baea1af-e671-239c-e3c3-d91e222d9929@suse.de>

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
>


  reply	other threads:[~2022-09-26  8:29 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-09-23 15:50 Bruno Larsen
2022-09-23 23:00 ` Tom de Vries
2022-09-26  8:29   ` Bruno Larsen [this message]
2022-09-27 10:51     ` Tom de Vries

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=856a18c7-657c-1f4f-892f-8d992f60e16b@redhat.com \
    --to=blarsen@redhat.com \
    --cc=gdb-patches@sourceware.org \
    --cc=tdevries@suse.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).