diff mbox

c++/8218: Destructors w/arguments.

Message ID 58C0B6B6.9040008@redhat.com
State New
Headers show

Commit Message

Keith Seitz March 9, 2017, 1:58 a.m. UTC
On 03/08/2017 02:53 PM, Pedro Alves wrote:
>> 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.

Cut-n-paste-o. Fixed/updated.

>> 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:

Yeah, that could have been done a little better, but given your concerns
below, I've reverted this bit. Better safe than sorry.

> 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?

No, only DWARF, I think. I've tested gdb.cp/*.exp for -gdwarf-2 and
-gstabs+ for both 64-bit and -m32. In all cases, with or without my v1
proposed patch, logs show no differences whatsoever, except in the
-gdwarf-2 cases where the modified templates.exp tests now pass.

The STABS cases never pass the two (templates.exp) tests.

Nonetheless, I've simplified my changes, maintaining the "if non-static,
always skip the first argument". I simply also skip any subsequent
artificial arguments in the loop. [I've also added a comment about why
we always skip the first argument for non-static functions.]

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

As Tom mentions, c_type_print_args will output the types of function
arguments /unless/ it is being called from the dwarf reader (to
construct physnames).  This function is called when someone does "ptype
A::~A". It is /not/ used when one types "ptype A". The dtor in that
instance is handled by cp_type_print_method_args.

Detailed explanation of why we don't see the artificial "int" parameter
in both cases (before my patch):

Because of how the compiler is outputting dtor debug info, we see
multiple DIEs describing the destructor.

When one types "ptype A", this uses one of the DIEs. This particular one
mentions the two artificial parameters. That's why we see (before this
patch) the "int" parameter.

When one types "ptype A::~A", this uses a different DIE, which has valid
PC bounds, but only mentions the first formal parameter. This is why
when we see only the "const A *" in the description of the parameters.

I've no explanation as to why we don't hide these implementation details
other than to offer a truly pedantic view of the code.

As best I can describe it, then, "ptype A::~A" (aka c_type_print_args)
attempts to be as tools-developer-oriented as possible while "ptype
CLASS" attempts to be more user-centric, hiding internal details like
artificial constructs.

Did any of that actually answer your question?

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

You mean c_type_print_args? I believe that comment is accurate.

Keith

\\((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"
 	}
     }
 }
diff mbox

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 3ac5170..ca7e5e2 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@ 
+2017-03-08  Keith Seitz  <keiths@redhat.com>
+
+	PR c++/8218
+	* c-typeprint.c (cp_type_print_method_args): 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..b97216e 100644
--- a/gdb/c-typeprint.c
+++ b/gdb/c-typeprint.c
@@ -226,13 +226,21 @@  cp_type_print_method_args (struct type *mtype,
const char *prefix,
 			   language_cplus, DMGL_ANSI);
   fputs_filtered ("(", stream);

-  /* Skip the class variable.  */
+  /* 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)
     {
       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