Re: [PATCH] Fix printing destructors with void in parameters for clang programs

[Tom de Vries <tdevries@suse.de>]

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