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

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

[-- 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();" }