[PATCH][gdb/c++] Print destructor the same for gcc and clang

[PATCH][gdb/c++] Print destructor the same for gcc and clang

[Tom de Vries]

Hi,

Consider the test-case contained in this patch.

With g++ (7.5.0) we have for "ptype A":
...
type = class A {
  public:
    int a;

    A(void);
    ~A();
}
...
and with clang++ (13.0.1):
...
type = class A {
  public:
    int a;

    A(void);
    ~A(void);
}
...
and we observe that the destructor is printed differently.

There's a difference in debug info between the two cases: in the clang case,
there's one artificial parameter, but in the g++ case, there are two, and
these similar cases are handled differently in cp_type_print_method_args.

This is due to this slightly convoluted bit of code:
...
  i = staticp ? 0 : 1;
  if (nargs > i)
    {
      while (i < nargs)
        ...
    }
  else if (varargs)
    gdb_printf (stream, "...");
  else if (language == language_cplus)
    gdb_printf (stream, "void");
...

The purpose of "i = staticp ? 0 : 1" is to skip the printing of the implicit
this parameter.

In commit 5f4d1085085 ("c++/8218: Destructors w/arguments"), skipping of other
artificial parameters was added, but using a different method: rather than
adjusting the potential loop start, it skips the parameter in the loop.

The observed difference in printing is explained by whether we enter the loop:
- in the clang case, the loop is not entered and we print "void".
- in the gcc case, the loop is entered, and nothing is printed.

Fix this by rewriting the code to:
- always enter the loop
- handle whether arguments need printing in the loop
- keep track of how many arguments are printed, and
  use that after the loop to print void etc.
such that we have the same for both gcc and clang:
...
    A(void);
    ~A(void);
...

Note that I consider the discussion of whether we want to print:
- A(void) / ~A(void), or
- A() / ~A()
out-of-scope for this patch.

Tested on x86_64-linux.

Any comments?

Thanks,
- Tom

[gdb/c++] Print destructor the same for gcc and clang

---
 gdb/c-typeprint.c                          | 55 +++++++++++++++++-------------
 gdb/testsuite/gdb.base/ptype-offsets.exp   |  2 +-
 gdb/testsuite/gdb.cp/print-method-args.cc  | 38 +++++++++++++++++++++
 gdb/testsuite/gdb.cp/print-method-args.exp | 36 +++++++++++++++++++
 gdb/testsuite/gdb.cp/templates.exp         |  4 +--
 5 files changed, 109 insertions(+), 26 deletions(-)

diff --git a/gdb/c-typeprint.c b/gdb/c-typeprint.c
index 3a611cdac5d..23e8a5a4d6f 100644
--- a/gdb/c-typeprint.c
+++ b/gdb/c-typeprint.c
@@ -273,35 +273,44 @@ 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
+	gdb_printf (stream, ", ...");
+    }
+  else if (printed_args == 0)
+    {
+      if (language == language_cplus)
+	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.base/ptype-offsets.exp b/gdb/testsuite/gdb.base/ptype-offsets.exp
index e80d876fa32..eb41bafb3e6 100644
--- a/gdb/testsuite/gdb.base/ptype-offsets.exp
+++ b/gdb/testsuite/gdb.base/ptype-offsets.exp
@@ -104,7 +104,7 @@ gdb_test "ptype /oTM struct abc" \
 "/*     48      |       2 */    my_int_type field9;" \
 "" \
 "                               abc(void);" \
-"                               ~abc();" \
+"                               ~abc(void);" \
 "" \
 "                               typedef short my_int_type;" \
 "/* XXX  6-byte padding   */" \
diff --git a/gdb/testsuite/gdb.cp/print-method-args.cc b/gdb/testsuite/gdb.cp/print-method-args.cc
new file mode 100644
index 00000000000..98be8286aeb
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/print-method-args.cc
@@ -0,0 +1,38 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright (C) 2022 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+class A {
+public:
+  int a;
+
+  A() {
+    this->a = 3;
+  }
+  ~A() {
+    a = 0;
+  }
+};
+
+int
+main (void)
+{
+  A a;
+
+  return a.a;
+}
diff --git a/gdb/testsuite/gdb.cp/print-method-args.exp b/gdb/testsuite/gdb.cp/print-method-args.exp
new file mode 100644
index 00000000000..7df044711b0
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/print-method-args.exp
@@ -0,0 +1,36 @@
+# Copyright (C) 2022 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# This file is part of the gdb testsuite.
+
+if { [skip_cplus_tests] } { continue }
+
+standard_testfile .cc
+
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile {debug c++}]} {
+    return -1
+}
+
+set re \
+    [multi_line \
+	 "type = class A {" \
+	 "  public:" \
+	 "    int a;" \
+	 "" \
+	 "    A\\(void\\);" \
+	 "    ~A\\(void\\);" \
+	 "}"]
+
+gdb_test "ptype A" $re
diff --git a/gdb/testsuite/gdb.cp/templates.exp b/gdb/testsuite/gdb.cp/templates.exp
index 27c9bbc1c7e..d3ede5a5fb6 100644
--- a/gdb/testsuite/gdb.cp/templates.exp
+++ b/gdb/testsuite/gdb.cp/templates.exp
@@ -68,7 +68,7 @@ proc test_ptype_of_templates {} {
 	    # The destructor has an argument type.
 	    kfail "gdb/8218" "ptype T5<int>"
 	}
-	-re "type = class T5<int> \{${ws}public:${ws}static int X;${ws}int x;${ws}int val;${ws}T5\\(int\\);${ws}T5\\((T5<int> const|const T5<int>) ?&\\);${ws}~T5\\(\\);${ws}static void \\* operator new\\((size_t|unsigned( int| long|))\\);${ws}static void operator delete\\(void ?\\*\\);${ws}int value\\((void|)\\);${ws}\}\r\n$gdb_prompt $" {
+	-re "type = class T5<int> \{${ws}public:${ws}static int X;${ws}int x;${ws}int val;${ws}T5\\(int\\);${ws}T5\\((T5<int> const|const T5<int>) ?&\\);${ws}~T5\\(void\\);${ws}static void \\* operator new\\((size_t|unsigned( int| long|))\\);${ws}static void operator delete\\(void ?\\*\\);${ws}int value\\((void|)\\);${ws}\}\r\n$gdb_prompt $" {
 	    pass "ptype T5<int>"
 	}
     }
@@ -108,7 +108,7 @@ proc test_ptype_of_templates {} {
 	    # The destructor has an argument type.
 	    kfail "gdb/8218" "ptype t5i"
 	}
-	-re "type = class T5<int> \{${ws}public:${ws}static int X;${ws}int x;${ws}int val;${ws}T5\\(int\\);${ws}T5\\((T5<int> const|const T5<int>) ?&\\);${ws}~T5\\(\\);${ws}static void \\* operator new\\((size_t|unsigned( int| long|))\\);${ws}static void operator delete\\(void ?\\*\\);${ws}int value\\((void|)\\);${ws}\}\r\n$gdb_prompt $" {
+	-re "type = class T5<int> \{${ws}public:${ws}static int X;${ws}int x;${ws}int val;${ws}T5\\(int\\);${ws}T5\\((T5<int> const|const T5<int>) ?&\\);${ws}~T5\\(void\\);${ws}static void \\* operator new\\((size_t|unsigned( int| long|))\\);${ws}static void operator delete\\(void ?\\*\\);${ws}int value\\((void|)\\);${ws}\}\r\n$gdb_prompt $" {
 	    pass "ptype t5i"
 	}
     }

Re: [PATCH][gdb/c++] Print destructor the same for gcc and clang

[Bruno Larsen]

On 27/09/2022 12:35, Tom de Vries wrote:
> Hi,
>
> Consider the test-case contained in this patch.
>
> With g++ (7.5.0) we have for "ptype A":
> ...
> type = class A {
>    public:
>      int a;
>
>      A(void);
>      ~A();
> }
> ...
> and with clang++ (13.0.1):
> ...
> type = class A {
>    public:
>      int a;
>
>      A(void);
>      ~A(void);
> }
> ...
> and we observe that the destructor is printed differently.
>
> There's a difference in debug info between the two cases: in the clang case,
> there's one artificial parameter, but in the g++ case, there are two, and
> these similar cases are handled differently in cp_type_print_method_args.
>
> This is due to this slightly convoluted bit of code:
> ...
>    i = staticp ? 0 : 1;
>    if (nargs > i)
>      {
>        while (i < nargs)
>          ...
>      }
>    else if (varargs)
>      gdb_printf (stream, "...");
>    else if (language == language_cplus)
>      gdb_printf (stream, "void");
> ...
>
> The purpose of "i = staticp ? 0 : 1" is to skip the printing of the implicit
> this parameter.
>
> In commit 5f4d1085085 ("c++/8218: Destructors w/arguments"), skipping of other
> artificial parameters was added, but using a different method: rather than
> adjusting the potential loop start, it skips the parameter in the loop.
>
> The observed difference in printing is explained by whether we enter the loop:
> - in the clang case, the loop is not entered and we print "void".
> - in the gcc case, the loop is entered, and nothing is printed.
>
> Fix this by rewriting the code to:
> - always enter the loop
> - handle whether arguments need printing in the loop
> - keep track of how many arguments are printed, and
>    use that after the loop to print void etc.
> such that we have the same for both gcc and clang:
> ...
>      A(void);
>      ~A(void);
> ...
>
> Note that I consider the discussion of whether we want to print:
> - A(void) / ~A(void), or
> - A() / ~A()
> out-of-scope for this patch.
>
> Tested on x86_64-linux.
>
> Any comments?
>
> Thanks,
> - Tom
>
> [gdb/c++] Print destructor the same for gcc and clang

Hi Tom,

I am ok with these changes, they make the code easier to read and easier 
to change, based on the output of the A()/A(void) discussion.

Cheers,
Bruno

> ---
>   gdb/c-typeprint.c                          | 55 +++++++++++++++++-------------
>   gdb/testsuite/gdb.base/ptype-offsets.exp   |  2 +-
>   gdb/testsuite/gdb.cp/print-method-args.cc  | 38 +++++++++++++++++++++
>   gdb/testsuite/gdb.cp/print-method-args.exp | 36 +++++++++++++++++++
>   gdb/testsuite/gdb.cp/templates.exp         |  4 +--
>   5 files changed, 109 insertions(+), 26 deletions(-)
>
> diff --git a/gdb/c-typeprint.c b/gdb/c-typeprint.c
> index 3a611cdac5d..23e8a5a4d6f 100644
> --- a/gdb/c-typeprint.c
> +++ b/gdb/c-typeprint.c
> @@ -273,35 +273,44 @@ 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
> +	gdb_printf (stream, ", ...");
> +    }
> +  else if (printed_args == 0)
> +    {
> +      if (language == language_cplus)
> +	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.base/ptype-offsets.exp b/gdb/testsuite/gdb.base/ptype-offsets.exp
> index e80d876fa32..eb41bafb3e6 100644
> --- a/gdb/testsuite/gdb.base/ptype-offsets.exp
> +++ b/gdb/testsuite/gdb.base/ptype-offsets.exp
> @@ -104,7 +104,7 @@ gdb_test "ptype /oTM struct abc" \
>   "/*     48      |       2 */    my_int_type field9;" \
>   "" \
>   "                               abc(void);" \
> -"                               ~abc();" \
> +"                               ~abc(void);" \
>   "" \
>   "                               typedef short my_int_type;" \
>   "/* XXX  6-byte padding   */" \
> diff --git a/gdb/testsuite/gdb.cp/print-method-args.cc b/gdb/testsuite/gdb.cp/print-method-args.cc
> new file mode 100644
> index 00000000000..98be8286aeb
> --- /dev/null
> +++ b/gdb/testsuite/gdb.cp/print-method-args.cc
> @@ -0,0 +1,38 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright (C) 2022 Free Software Foundation, Inc.
> +
> +   This file is part of GDB.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +class A {
> +public:
> +  int a;
> +
> +  A() {
> +    this->a = 3;
> +  }
> +  ~A() {
> +    a = 0;
> +  }
> +};
> +
> +int
> +main (void)
> +{
> +  A a;
> +
> +  return a.a;
> +}
> diff --git a/gdb/testsuite/gdb.cp/print-method-args.exp b/gdb/testsuite/gdb.cp/print-method-args.exp
> new file mode 100644
> index 00000000000..7df044711b0
> --- /dev/null
> +++ b/gdb/testsuite/gdb.cp/print-method-args.exp
> @@ -0,0 +1,36 @@
> +# Copyright (C) 2022 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +# This file is part of the gdb testsuite.
> +
> +if { [skip_cplus_tests] } { continue }
> +
> +standard_testfile .cc
> +
> +if {[prepare_for_testing "failed to prepare" $testfile $srcfile {debug c++}]} {
> +    return -1
> +}
> +
> +set re \
> +    [multi_line \
> +	 "type = class A {" \
> +	 "  public:" \
> +	 "    int a;" \
> +	 "" \
> +	 "    A\\(void\\);" \
> +	 "    ~A\\(void\\);" \
> +	 "}"]
> +
> +gdb_test "ptype A" $re
> diff --git a/gdb/testsuite/gdb.cp/templates.exp b/gdb/testsuite/gdb.cp/templates.exp
> index 27c9bbc1c7e..d3ede5a5fb6 100644
> --- a/gdb/testsuite/gdb.cp/templates.exp
> +++ b/gdb/testsuite/gdb.cp/templates.exp
> @@ -68,7 +68,7 @@ proc test_ptype_of_templates {} {
>   	    # The destructor has an argument type.
>   	    kfail "gdb/8218" "ptype T5<int>"
>   	}
> -	-re "type = class T5<int> \{${ws}public:${ws}static int X;${ws}int x;${ws}int val;${ws}T5\\(int\\);${ws}T5\\((T5<int> const|const T5<int>) ?&\\);${ws}~T5\\(\\);${ws}static void \\* operator new\\((size_t|unsigned( int| long|))\\);${ws}static void operator delete\\(void ?\\*\\);${ws}int value\\((void|)\\);${ws}\}\r\n$gdb_prompt $" {
> +	-re "type = class T5<int> \{${ws}public:${ws}static int X;${ws}int x;${ws}int val;${ws}T5\\(int\\);${ws}T5\\((T5<int> const|const T5<int>) ?&\\);${ws}~T5\\(void\\);${ws}static void \\* operator new\\((size_t|unsigned( int| long|))\\);${ws}static void operator delete\\(void ?\\*\\);${ws}int value\\((void|)\\);${ws}\}\r\n$gdb_prompt $" {
>   	    pass "ptype T5<int>"
>   	}
>       }
> @@ -108,7 +108,7 @@ proc test_ptype_of_templates {} {
>   	    # The destructor has an argument type.
>   	    kfail "gdb/8218" "ptype t5i"
>   	}
> -	-re "type = class T5<int> \{${ws}public:${ws}static int X;${ws}int x;${ws}int val;${ws}T5\\(int\\);${ws}T5\\((T5<int> const|const T5<int>) ?&\\);${ws}~T5\\(\\);${ws}static void \\* operator new\\((size_t|unsigned( int| long|))\\);${ws}static void operator delete\\(void ?\\*\\);${ws}int value\\((void|)\\);${ws}\}\r\n$gdb_prompt $" {
> +	-re "type = class T5<int> \{${ws}public:${ws}static int X;${ws}int x;${ws}int val;${ws}T5\\(int\\);${ws}T5\\((T5<int> const|const T5<int>) ?&\\);${ws}~T5\\(void\\);${ws}static void \\* operator new\\((size_t|unsigned( int| long|))\\);${ws}static void operator delete\\(void ?\\*\\);${ws}int value\\((void|)\\);${ws}\}\r\n$gdb_prompt $" {
>   	    pass "ptype t5i"
>   	}
>       }
>

Re: [PATCH][gdb/c++] Print destructor the same for gcc and clang

[Keith Seitz]

On 9/27/22 03:35, Tom de Vries wrote:
> 
> Fix this by rewriting the code to:
> - always enter the loop
> - handle whether arguments need printing in the loop
> - keep track of how many arguments are printed, and
>    use that after the loop to print void etc.
> such that we have the same for both gcc and clang:
> ...
>      A(void);
>      ~A(void);
> ...
> 
> Note that I consider the discussion of whether we want to print:
> - A(void) / ~A(void), or
> - A() / ~A()
> out-of-scope for this patch.
> 
> Tested on x86_64-linux.
> 
> Any comments?

This all looks okay; thank you for that cleanup. Consistency is welcome.

On the [off-]topic of "void" vs ""... The standard (IIUC) permits the use of
"void" in destructors. If I create a class with a private dtor, e.g., to elicit
a compilation failure, clang++ never outputs the symbol name. GCC, on the other
hand, does, and it outputs "" (no "void", even if I explicitly use "void" in the
dtor).

While I would prefer "", I don't have a compelling argument either way. [It's
trivial to implement with your patch.]

I recommend you approve your patch. :-)

Keith

Re: [PATCH][gdb/c++] Print destructor the same for gcc and clang

[Pedro Alves]

On 2022-09-27 11:35 a.m., Tom de Vries wrote:

> 
> Note that I consider the discussion of whether we want to print:
> - A(void) / ~A(void), or
> - A() / ~A()
> out-of-scope for this patch.
> 

I'd vote to remove the void.  I can't think of a good reason to keep it (for C++).
It just looks odd to C++ developers, IMHO.

I'm fine with that being a separate decision from this patch, though.

> diff --git a/gdb/c-typeprint.c b/gdb/c-typeprint.c
> index 3a611cdac5d..23e8a5a4d6f 100644
> --- a/gdb/c-typeprint.c
> +++ b/gdb/c-typeprint.c
> @@ -273,35 +273,44 @@ 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;
> +	}


A remark on this preexisting "Skip the class variable." logic -- I strongly suspect that we'll
need to revisit it for the C++23 "deducing this" feature:

  https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2021/p0847r6.html

With that feature, some methods we'll have explicit, named "this" parameters, and
thus, I'd think, be non-artificial.  E.g., from the proposal:

 struct X {
     void foo(this X const& self, int i);
 
     template <typename Self>
     void bar(this Self&& self);
 };

Plainly skipping printing the "self" parameter in these methods just because it is a non-static
method, and "self" is the first parameter, will be incorrect.

According to:

  https://gcc.gnu.org/projects/cxx-status.html
  https://clang.llvm.org/cxx_status.html

neither GCC not Clang support that yet though.

We don't have tests for many of the C++14, C++17 and C++20 features, so I'm not holding my
breath waiting for tests for this, though.  :-P

> +++ b/gdb/testsuite/gdb.cp/print-method-args.exp
> @@ -0,0 +1,36 @@
> +# Copyright (C) 2022 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +# This file is part of the gdb testsuite.
> +

Can you add an intro comment?  LGTM with that fixed.

Re: [PATCH][gdb/c++] Print destructor the same for gcc and clang

[Pedro Alves]

On 2022-09-29 6:52 p.m., Keith Seitz via Gdb-patches wrote:

> This all looks okay; thank you for that cleanup. Consistency is welcome.
> 

+1

> On the [off-]topic of "void" vs ""... The standard (IIUC) permits the use of
> "void" in destructors. If I create a class with a private dtor, e.g., to elicit
> a compilation failure, clang++ never outputs the symbol name. GCC, on the other
> hand, does, and it outputs "" (no "void", even if I explicitly use "void" in the
> dtor).
> 
> While I would prefer "", I don't have a compelling argument either way. [It's
> trivial to implement with your patch.]

I would prefer "" too.  It just looks weird/redundant for C++, IMHO.

> 
> I recommend you approve your patch. :-)

Thanks Keith.  That certainly helps!