c++/8218: Destructors w/arguments.

Message ID

1489010611-28451-1-git-send-email-keiths@redhat.com

State

New, archived

Headers

Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm
Precedence: bulk
Sender: gdb-patches-owner@sourceware.org
From: Keith Seitz <keiths@redhat.com>
To: gdb-patches@sourceware.org
Subject: [PATCH] c++/8218: Destructors w/arguments.
Date: Wed,  8 Mar 2017 14:03:31 -0800
Message-Id: <1489010611-28451-1-git-send-email-keiths@redhat.com>

Commit Message

Keith Seitz March 8, 2017, 10:03 p.m. UTC

  For a long time now, c++/8218 has noted that GDB is printing argument types
for destructors:

(gdb) ptype A
type = class A {
  public:
    ~A(int);
}

This happens because cp_type_print_method_args doesn't ignore artificial
arguments.  [It ignores the first `this' pointer because it simply skips
the first argument for any non-static function.]

This patch fixes this:

(gdb) ptype  A
type = class A {
  public:
    ~A();
}

I've adjusted gdb.cp/templates.exp to account for this and added a new
passing regexp.

gdb/ChangeLog

	PR c++/8218
	* c-typeprint.c (cp_type_print_method_args): Start printing arguments
	at index 0, ignoring STATCIP.
	Skip artificial arguments.

gdb/testsuite/ChangeLog

	PR c++/8218
	* c-typeprint.c (cp_type_print_method_args): Start printing arguments
	at index 0, ignoring STATCIP.
	Skip artificial arguments.
---
 gdb/ChangeLog                      |  7 +++++++
 gdb/c-typeprint.c                  | 11 ++++++++---
 gdb/testsuite/ChangeLog            |  8 ++++++++
 gdb/testsuite/gdb.cp/templates.exp | 24 +++++++++++++++---------
 4 files changed, 38 insertions(+), 12 deletions(-)

Comments

Pedro Alves March 8, 2017, 10:53 p.m. UTC | #1

Hi Keith,

In principle, I like the change, as it's obviously a desirable
behavior change, but I have a few concerns.  See below.

> gdb/ChangeLog
> 
> 	PR c++/8218
> 	* c-typeprint.c (cp_type_print_method_args): Start printing arguments
> 	at index 0, ignoring STATCIP.
> 	Skip artificial arguments.
> 
> gdb/testsuite/ChangeLog
> 
> 	PR c++/8218
> 	* c-typeprint.c (cp_type_print_method_args): Start printing arguments
> 	at index 0, ignoring STATCIP.
> 	Skip artificial arguments.

Wrong entry for testsuite.

> ---
>  gdb/ChangeLog                      |  7 +++++++
>  gdb/c-typeprint.c                  | 11 ++++++++---
>  gdb/testsuite/ChangeLog            |  8 ++++++++
>  gdb/testsuite/gdb.cp/templates.exp | 24 +++++++++++++++---------
>  4 files changed, 38 insertions(+), 12 deletions(-)
> 
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 3ac5170..66cdfbd 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,10 @@
> +2017-03-08  Keith Seitz  <keiths@redhat.com>
> +
> +	PR c++/8218
> +	* c-typeprint.c (cp_type_print_method_args): Start printing arguments
> +	at index 0, ignoring STATCIP.
> +	Skip artificial arguments.
> +
>  2017-02-21  Jan Kratochvil  <jan.kratochvil@redhat.com>
>  
>  	* dwarf2read.c (dwarf2_record_block_ranges): Add forgotten BASEADDR.
> diff --git a/gdb/c-typeprint.c b/gdb/c-typeprint.c
> index 75f6d61..e504618 100644
> --- a/gdb/c-typeprint.c
> +++ b/gdb/c-typeprint.c
> @@ -226,13 +226,18 @@ cp_type_print_method_args (struct type *mtype, const char *prefix,
>  			   language_cplus, DMGL_ANSI);
>    fputs_filtered ("(", stream);
>  
> -  /* Skip the class variable.  */
> -  i = staticp ? 0 : 1;
> +  i = 0;
>    if (nargs > i)
>      {
>        while (i < nargs)
>  	{
> -	  c_print_type (args[i++].type, "", stream, 0, 0, flags);
> +	  struct field arg = args[i++];

The manipulation of 'i' looks a bit obscure.  This is crying
for a "for", IMO:

  if (nargs > 0)
    {
      for (int i = 0; i < nargs; i++)
	{
	  struct field arg = args[i];


> +
> +	  /* Skip any artificial arguments.  */
> +	  if (FIELD_ARTIFICIAL (arg))
> +	    continue;

Can we trust that FIELD_ARTIFICIAL is always set on the implicit this
argument on all debug formats?  I.e., does it work with stabs?

Also, even for DWARF, does it work with debug info produced
by older GCCs?  And older dwarf revisions?  -gdwarf-2, etc.
Can you poke a bit please?  I wouldn't be surprised if we still
needed to treat the first argument specially.

Can you comment on the treatment c_type_print_args gives to
artificial arguments that Tom pointed out in the PR?

Also that function's intro comment talks about C++ "this".
Is that stale?

Thanks,
Pedro Alves

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 3ac5170..66cdfbd 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,10 @@ 
+2017-03-08  Keith Seitz  <keiths@redhat.com>
+
+	PR c++/8218
+	* c-typeprint.c (cp_type_print_method_args): Start printing arguments
+	at index 0, ignoring STATCIP.
+	Skip artificial arguments.
+
 2017-02-21  Jan Kratochvil  <jan.kratochvil@redhat.com>
 
 	* dwarf2read.c (dwarf2_record_block_ranges): Add forgotten BASEADDR.
diff --git a/gdb/c-typeprint.c b/gdb/c-typeprint.c
index 75f6d61..e504618 100644
--- a/gdb/c-typeprint.c
+++ b/gdb/c-typeprint.c
@@ -226,13 +226,18 @@  cp_type_print_method_args (struct type *mtype, const char *prefix,
 			   language_cplus, DMGL_ANSI);
   fputs_filtered ("(", stream);
 
-  /* Skip the class variable.  */
-  i = staticp ? 0 : 1;
+  i = 0;
   if (nargs > i)
     {
       while (i < nargs)
 	{
-	  c_print_type (args[i++].type, "", stream, 0, 0, flags);
+	  struct field arg = args[i++];
+
+	  /* Skip any artificial arguments.  */
+	  if (FIELD_ARTIFICIAL (arg))
+	    continue;
+
+	  c_print_type (arg.type, "", stream, 0, 0, flags);
 
 	  if (i == nargs && varargs)
 	    fprintf_filtered (stream, ", ...");
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 420bfc4..b26d800 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,11 @@ 
+2017-03-08  Keith Seitz  <keiths@redhat.com>
+
+	PR c++/8128
+	* gdb.cp/templates.exp (test_ptype_of_templates): Remove argument
+	type from destructor regexps.
+	Add a branch which actually passes the test.
+	Adjust "ptype t5i" test names.
+
 2017-02-21  Edjunior Barbosa Machado  <emachado@linux.vnet.ibm.com>
 
 	* gdb.arch/ppc64-isa207-atomic-inst.exp: New testcase based on
diff --git a/gdb/testsuite/gdb.cp/templates.exp b/gdb/testsuite/gdb.cp/templates.exp
index 6f9131f..85f86ee 100644
--- a/gdb/testsuite/gdb.cp/templates.exp
+++ b/gdb/testsuite/gdb.cp/templates.exp
@@ -46,16 +46,16 @@  proc test_ptype_of_templates {} {
 	-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\\(unsigned( int| long)?\\);${ws}static void operator delete\\(void ?\\*\\);${ws}int value\\((void|)\\);${ws}T5<int> & operator=\\(T5<int> const ?&\\);${ws}\}\r\n$gdb_prompt $" {
 	    xfail "ptype T5<int> -- new without size_t"
 	}
-	-re "type = class T5<int> \\{${ws}public:${ws}static int X;${ws}int x;${ws}int val;${ws}${ws}T5 \\(int\\);${ws}T5 \\(const class T5<int> &\\);${ws}void ~T5 \\(int\\);${ws}static void \\* new \\(unsigned int\\);${ws}static void delete \\(void ?\\*\\);${ws}int value \\((void|)\\);${ws}\\}${ws}$gdb_prompt $" {
+	-re "type = class T5<int> \\{${ws}public:${ws}static int X;${ws}int x;${ws}int val;${ws}${ws}T5 \\(int\\);${ws}T5 \\(const class T5<int> &\\);${ws}void ~T5 \\(()\\);${ws}static void \\* new \\(unsigned int\\);${ws}static void delete \\(void ?\\*\\);${ws}int value \\((void|)\\);${ws}\\}${ws}$gdb_prompt $" {
 	    xfail "ptype T5<int> -- new with unsigned int"
 	}
-	-re "type = class T5<int> \\{.*public:.*static int X;.*int x;.*int val;.*T5 \\(int\\);.*T5 \\(const class T5<int> &\\);.*void ~T5 \\(int\\);.*static void \\* new \\(unsigned long\\);.*static void delete \\(void ?\\*\\);.*int value \\((void|)\\);.*\\}\r\n$gdb_prompt $" {
+	-re "type = class T5<int> \\{.*public:.*static int X;.*int x;.*int val;.*T5 \\(int\\);.*T5 \\(const class T5<int> &\\);.*void ~T5 \\(\\);.*static void \\* new \\(unsigned long\\);.*static void delete \\(void ?\\*\\);.*int value \\((void|)\\);.*\\}\r\n$gdb_prompt $" {
 	    xfail "ptype T5<int> -- new with unsigned long"
 	}
 	-re "type = class T5<int> \{${ws}public:${ws}static int X;${ws}int x;${ws}int val;((${ws}T5<int> & operator=\\(T5<int> const ?&\\);)|(${ws}T5\\(int\\);)|(${ws}T5\\((T5<int> const|const T5<int>) ?&\\);)|(${ws}~T5\\((void|)\\);)|(${ws}static void \\* operator new\\(unsigned( int| long)?\\);)|(${ws}static void operator delete\\(void ?\\*\\);)|(${ws}int value\\((void|)\\);))*${ws}\}\r\n$gdb_prompt $" {
 	    xfail "ptype T5<int> (obsolescent gcc or gdb)"
 	}
-	-re "type = class T5<int> \{${ws}public:${ws}static int X;${ws}int x;${ws}int val;${ws}void T5\\(int\\);${ws}void T5\\((T5<int> const|const T5<int>) ?&\\);${ws}~T5\\(int\\);${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}void T5\\(int\\);${ws}void 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 $" {
 	    # This also triggers gdb/1113...
 	    kfail "gdb/1111" "ptype T5<int>"
 	    # Add here a PASS case when PR gdb/1111 gets fixed.
@@ -68,19 +68,22 @@  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 $" {
+	    pass "ptype T5<int>"
+	}
     }
 
     gdb_test_multiple "ptype/r t5i" "ptype t5i" {
         -re "type = class T5<int> \\{${ws}public:${ws}static int X;${ws}int x;${ws}int val;\r\n${ws}T5\\(int\\);${ws}T5\\(T5<int> const ?&\\);${ws}~T5\\((void|)\\);${ws}static void \\* operator new\\(unsigned( int| long)?\\);${ws}static void operator delete\\(void ?\\*\\);${ws}int value\\((void|)\\);${ws}\\}\r\n$gdb_prompt $" {
 	    xfail "ptype T5<int> -- with several fixes from 4.17 -- without size_t"
 	}
-        -re "type = class T5<int> \\{${ws}public:${ws}static int X;${ws}int x;${ws}int val;\r\n${ws}T5 \\(int\\);${ws}T5 \\(const class T5<int> &\\);${ws}void ~T5 \\(int\\);${ws}static void \\* new \\(unsigned int\\);${ws}static void 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;\r\n${ws}T5 \\(int\\);${ws}T5 \\(const class T5<int> &\\);${ws}void ~T5 \\(\\);${ws}static void \\* new \\(unsigned int\\);${ws}static void delete \\(void ?\\*\\);${ws}int value \\((void|)\\);${ws}\\}\r\n$gdb_prompt $" {
 	    xfail "ptype t5i<int> -- new with unsigned int -- without size_t"
 	}
-        -re "type = class T5<int> \\{${ws}public:${ws}static int X;${ws}int x;${ws}int val;\r\n${ws}T5 \\(int\\);${ws}T5 \\(const class T5<int> &\\);${ws}void ~T5 \\(int\\);${ws}static void \\* new \\(unsigned long\\);${ws}static void 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;\r\n${ws}T5 \\(int\\);${ws}T5 \\(const class T5<int> &\\);${ws}void ~T5 \\(\\);${ws}static void \\* new \\(unsigned long\\);${ws}static void delete \\(void ?\\*\\);${ws}int value \\((void|)\\);${ws}\\}\r\n$gdb_prompt $" {
 	    xfail "ptype t5i<int> -- new with unsigned long -- without size_t"
 	}
-        -re "type = class T5<int> \{.*public:.*static int X;.*int x;.*int val;.*.*T5 \\(int\\);.*.*void ~T5 \\(int\\).*.*.*int value \\((void|)\\);.*\}.*$gdb_prompt $" { 
+        -re "type = class T5<int> \{.*public:.*static int X;.*int x;.*int val;.*.*T5 \\(int\\);.*.*void ~T5 \\(\\).*.*.*int value \\((void|)\\);.*\}.*$gdb_prompt $" {
             xfail "ptype t5i -- without size_t"
         }
 	-re "type = class T5<int> \{${ws}public:${ws}static int X;${ws}int x;${ws}int val;${ws}T5<int> & operator=\\(T5<int> const ?&\\);${ws}T5\\(int\\);${ws}T5\\((T5<int> const|const T5<int>) ?&\\);${ws}~T5\\((void|)\\);${ws}static void \\* operator new\\(unsigned( int| long)?\\);${ws}static void operator delete\\(void ?\\*\\);${ws}int value\\((void|)\\);${ws}\}\r\n$gdb_prompt $" {
@@ -92,9 +95,9 @@  proc test_ptype_of_templates {} {
 	-re "type = class T5<int> \{${ws}public:${ws}static int X;${ws}int x;${ws}int val;((${ws}T5<int> & operator=\\(T5<int> const ?&\\);)|(${ws}T5\\(int\\);)|(${ws}T5\\(T5<int> const ?&\\);)|(${ws}~T5\\((void|)\\);)|(${ws}static void \\* operator new\\(unsigned( int| long)?\\);)|(${ws}static void operator delete\\(void ?\\*\\);)|(${ws}int value\\((void|)\\);))*${ws}\}\r\n$gdb_prompt $" {
 	    xfail "ptype t5i (obsolescent gcc or gdb) -- without size_t"
 	}
-	-re "type = class T5<int> \{${ws}public:${ws}static int X;${ws}int x;${ws}int val;${ws}void T5\\(int\\);${ws}void T5\\((T5<int> const|const T5<int>) ?&\\);${ws}~T5\\(int\\);${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}void T5\\(int\\);${ws}void 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 $" {
 	    # This also triggers gdb/1113...
-	    kfail "gdb/1111" "ptype T5<int>"
+	    kfail "gdb/1111" "ptype t5i"
 	    # Add here a PASS case when PR gdb/1111 gets fixed.
 	    # These are really:
 	    # http://sourceware.org/bugzilla/show_bug.cgi?id=8216
@@ -103,7 +106,10 @@  proc test_ptype_of_templates {} {
 	-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\\(int\\);${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 $" {
 	    # http://sourceware.org/bugzilla/show_bug.cgi?id=8218
 	    # The destructor has an argument type.
-	    kfail "gdb/8218" "ptype T5<int>"
+	    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 $" {
+	    pass "ptype t5i"
 	}
     }
 }