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

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

[Bruno Larsen]

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(-)

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, ")");
-- 
2.37.3

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

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 3033 bytes --]

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


[-- Attachment #2: tmp.patch --]
[-- Type: text/x-patch, Size: 3282 bytes --]

diff --git a/gdb/c-typeprint.c b/gdb/c-typeprint.c
index 3a611cdac5d..e17a4ccfa1e 100644
--- a/gdb/c-typeprint.c
+++ b/gdb/c-typeprint.c
@@ -273,35 +273,46 @@ cp_type_print_method_args (struct type *mtype, const char *prefix,
 		  language_cplus, DMGL_ANSI);
   gdb_puts ("(", stream);
 
-  /* Skip the class variable.  We keep this here to accommodate older
-     compilers and debug formats which may not support artificial
-     parameters.  */
-  i = staticp ? 0 : 1;
-  if (nargs > i)
+  int printed_args = 0;
+  for (i = 0; i < nargs; ++i)
     {
-      while (i < nargs)
+      if (i == 0 && !staticp)
 	{
-	  struct field arg = args[i++];
-
-	  /* Skip any artificial arguments.  */
-	  if (FIELD_ARTIFICIAL (arg))
-	    continue;
+	  /* Skip the class variable.  We keep this here to accommodate older
+	     compilers and debug formats which may not support artificial
+	     parameters.  */
+	  continue;
+	}
 
-	  c_print_type (arg.type (), "", stream, 0, 0, language, flags);
+      struct field arg = args[i];
+      /* Skip any artificial arguments.  */
+      if (FIELD_ARTIFICIAL (arg))
+	continue;
 
-	  if (i == nargs && varargs)
-	    gdb_printf (stream, ", ...");
-	  else if (i < nargs)
-	    {
-	      gdb_printf (stream, ", ");
-	      stream->wrap_here (8);
-	    }
+      if (printed_args > 0)
+	{
+	  gdb_printf (stream, ", ");
+	  stream->wrap_here (8);
 	}
+
+      c_print_type (arg.type (), "", stream, 0, 0, language, flags);
+      printed_args++;
+    }
+
+  if (varargs)
+    {
+      if (printed_args == 0)
+	gdb_printf (stream, "...");
+      else
+	/* Note: no wrapping done here.  Is that intended?  */
+	gdb_printf (stream, ", ...");
+    }
+  else if (printed_args == 0)
+    {
+      /* Why do we do this differently for non-c++?  */
+      if (language == language_cplus && staticp)
+	gdb_printf (stream, "void");
     }
-  else if (varargs)
-    gdb_printf (stream, "...");
-  else if (language == language_cplus)
-    gdb_printf (stream, "void");
 
   gdb_printf (stream, ")");
 
diff --git a/gdb/testsuite/gdb.cp/classes.exp b/gdb/testsuite/gdb.cp/classes.exp
index 7b8b315ac1f..7c214a57100 100644
--- a/gdb/testsuite/gdb.cp/classes.exp
+++ b/gdb/testsuite/gdb.cp/classes.exp
@@ -328,7 +328,7 @@ proc test_ptype_class_objects {} {
 		  "class_with_typedefs::protected_int protected_int_;" }
 	    { field protected \
 		  "class_with_typedefs::private_int private_int_;" }
-	    { method public "class_with_typedefs(void);" }
+	    { method public "class_with_typedefs();" }
 	    { method public "class_with_typedefs::public_int add_public(class_with_typedefs::public_int);" }
 	    { method public \
 		  "class_with_typedefs::public_int add_all(int);" }
diff --git a/gdb/testsuite/gdb.cp/rvalue-ref-overload.exp b/gdb/testsuite/gdb.cp/rvalue-ref-overload.exp
index 36342673aad..b0c555a8fa9 100644
--- a/gdb/testsuite/gdb.cp/rvalue-ref-overload.exp
+++ b/gdb/testsuite/gdb.cp/rvalue-ref-overload.exp
@@ -43,7 +43,7 @@ gdb_test "up" ".*main.*" "up from marker1"
 # Print the monster class type.
 cp_test_ptype_class "foo_rr_instance1" "" "class" "foo" \
     {
-	{ method public "foo(void);" }
+	{ method public "foo();" }
 	{ method public "foo(foo_lval_ref);" }
 	{ method public "foo(foo_rval_ref);" }
 	{ method public "~foo();" }

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

[Bruno Larsen]

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
>

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

[Tom de Vries]

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