Record nested types

  On 10/27/2017 05:00 AM, Pedro Alves wrote:
>> By default, the code maintains the status quo, that is, it will not print
>> any nested type definitions at all.
> 
> IWBN to add a new flag to ptype that allows overriding this.
> 
>> Testing is carried out via cp_ptype_class which required quite a bit of
>> modificaiton to permit recursive calling (for the nested types).  This
> 
> modificaiton -> modification

Fixed.

>> @@ -7139,7 +7144,8 @@ add_partial_symbol (struct partial_die_info *pdi, struct dwarf2_cu *cu)
>>      {
>>      case DW_TAG_subprogram:
>>        addr = gdbarch_adjust_dwarf2_addr (gdbarch, pdi->lowpc + baseaddr);
>> -      if (pdi->is_external || cu->language == language_ada)
>> +      if (pdi->is_external || cu->language == language_ada
>> +	  || cu->language == language_cplus)
> 
> Why were this ...
> 
>>  	{
>>            /* brobecker/2007-12-26: Normally, only "external" DIEs are part
>>               of the global scope.  But in Ada, we want to be able to access
>> @@ -7392,7 +7398,7 @@ add_partial_subprogram (struct partial_die_info *pdi,
>>    if (! pdi->has_children)
>>      return;
>>  
>> -  if (cu->language == language_ada)
>> +  if (cu->language == language_ada || cu->language == language_cplus)
> 
> ... and this ...
> 
> 
>>      {
>>        pdi = pdi->die_child;
>>        while (pdi != NULL)
>> @@ -12635,7 +12641,7 @@ dwarf2_get_subprogram_pc_bounds (struct die_info *die,
>>  
>>    /* If the language does not allow nested subprograms (either inside
>>       subprograms or lexical blocks), we're done.  */
>> -  if (cu->language != language_ada)
>> +  if (cu->language != language_ada && cu->language != language_cplus)
> 
> ... and this changes necessary?  The new nested-types.exp test
> passes cleanly without them.

Bah. Those are from another patch. I thought I'd removed all those bits, but I apparently did not. Expect to see those at some future date.

>>    /* Allocate a new field list entry and link it in.  */
>> -  new_field = XCNEW (struct typedef_field_list);
>> +  new_field = XCNEW (struct decl_field_list);
>>    make_cleanup (xfree, new_field);
>>  
>> -  gdb_assert (die->tag == DW_TAG_typedef);
>> +  gdb_assert (die->tag == DW_TAG_typedef
>> +	      || die->tag == DW_TAG_class_type
>> +	      || die->tag == DW_TAG_structure_type
>> +	      || die->tag == DW_TAG_union_type
>> +	      || die->tag == DW_TAG_enumeration_type);
>>  
> 
>> -	  else if (child_die->tag == DW_TAG_typedef)
>> -	    dwarf2_add_typedef (&fi, child_die, cu);
>> +	  else if (child_die->tag == DW_TAG_typedef
>> +		   || child_die->tag == DW_TAG_class_type
>> +		   || child_die->tag == DW_TAG_structure_type
>> +		   || child_die->tag == DW_TAG_union_type
>> +		   || child_die->tag == DW_TAG_enumeration_type)
>> +	    dwarf2_add_type_defn (&fi, child_die, cu);
> 
> Feel free to not do this, but, would it make sense to factor these
> out to a function like:
> 
> bool is_something (tag)
> {
>   switch (tag)
>     {
>     case DW_TAG_typedef:
>     case DW_TAG_class_type:
>     case DW_TAG_structure_type:
>     case DW_TAG_union_type:
>     case DW_TAG_enumeration_type:
>        return true;
>     default:
>        return false;
>    }
> }
> 
> and then use is_something in both places?

Sure. I've added type_can_define_types (const struct die_info *) to do this.

>> @@ -13887,6 +13907,30 @@ process_structure_scope (struct die_info *die, struct dwarf2_cu *cu)
>>  	    }
>>  	}
>>  
>> +      /* Copy fi.nested_types_list linked list elements content into the
>> +	 allocated array TYPE_NESTED_TYPES_ARRAY (type).  */
>> +      if (fi.nested_types_list != NULL)
>> +	{
>> +	  int i = fi.nested_types_list_count;
>> +
>> +	  ALLOCATE_CPLUS_STRUCT_TYPE (type);
> 
> Is it OK to call ALLOCATE_CPLUS_STRUCT_TYPE for Ada symbols (for example)?

I would, of course, say, "Probably not," if I could verify it. INIT_CPLUS_SPECIFIC *is* called unconditionally from read_structure_type, but that is a little more innocent than ALLOCATE_CPLUS_TYPE. I see a lot of calls fenceposted by language != language_ada, though, and I see no harm in adding that here. [And I have done so.]

I don't know Ada very well, but I think that the typedef case is not fenceposted only because the language doesn't support the concept (today at least). Of course, AFAICT, Ada doesn't support nested type definitions, either.

>> diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
>> index 5c1aecd211..d01c59a88f 100644
> 
>> @@ -1424,6 +1431,23 @@ extern void set_type_vptr_basetype (struct type *, struct type *);
>>  #define TYPE_TYPEDEF_FIELD_PRIVATE(thistype, n)        \
>>    TYPE_TYPEDEF_FIELD (thistype, n).is_private
>>  
>> +#define TYPE_NESTED_TYPES_ARRAY(thistype)	\
>> +  TYPE_CPLUS_SPECIFIC (thistype)->nested_types
>> +#define TYPE_NESTED_TYPES_FIELD(thistype, n) \
>> +  TYPE_CPLUS_SPECIFIC (thistype)->nested_types[n]
>> +#define TYPE_NESTED_TYPES_FIELD_NAME(thistype, n) \
>> +  TYPE_NESTED_TYPES_FIELD (thistype, n).name
>> +#define TYPE_NESTED_TYPES_FIELD_TYPE(thistype, n) \
>> +  TYPE_NESTED_TYPES_FIELD (thistype, n).type
>> +#define TYPE_NESTED_TYPES_COUNT(thistype) \
>> +  TYPE_CPLUS_SPECIFIC (thistype)->nested_types_count
> 
> These always map to TYPE_CPLUS_SPECIFIC stuff, but their
> names don't imply C++.  Is this OK for other languages?

The same could be said of TYPE_TYPEDEF_*, but at least as far as Java would go -- the only other language we support with which I have any real familiarity -- that could certainly be supported.

>> +++ b/gdb/testsuite/gdb.cp/nested-types.cc
>> @@ -0,0 +1,668 @@
>> +/* This testcase is part of GDB, the GNU debugger.
>> +
>> +   Copyright 2017 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/>.  */
>> +
>> +/* Tests for nested type definitions.  */
>> +
>> +struct S10
>> +{
> 
> W00t, lots of repetition.  OOC, did you consider using the preprocessor
> to build the types?

Preprocessor? I used the logging feature of the test script to generate the struct. [When the log is enabled, it sends the entire struct (up to the nested type limit) to gdb.log.]

If you would like this done some other way, just let me know.

>> +
>> +proc build_node {id} {
>> +    global nodes
>> +
>> +    # For any node, FIELDS is always the types i(N), e(N), u(N)
>> +    # CHILDREN is a list of nodes called [E(N), U(N)] S(N+1)
>> +    #
>> +    # The root (10) also has S(N+11), S(N+21), S(N+31), S(N+41)
>> +
>> +    set nodes($id,fields) [list "int i$id" "E$id e$id" "U$id u$id"]
>> +    set ndoes($id,children) {}
> 
> Typo: "ndoes" -> "nodes"

Bah. Fixed. How did that not generate an error? [Oh, I see. Defensive programming -- "info exists" is used later.]

>> --- a/gdb/testsuite/lib/cp-support.exp
>> +++ b/gdb/testsuite/lib/cp-support.exp
> 
>> -# Test ptype of a class.
>> +# A convenience procedure for outputting debug info for cp_test_ptype_class
>> +# to the log.  Set the global variable "debug_cp_test_ptype_class"
>> +# to enable logging (to help with debugging failures).
>> +
>> +proc cp_ptype_class_verbose {msg} {
>> +    global debug_cp_test_ptype_class
>> +
>> +    if {$debug_cp_test_ptype_class} {
>> +	verbose -log $msg
>> +    }
>> +}
>> +
>> +# A convenience procedure to return the next element of the queue.
>> +
>> +proc next_line {qid} {
> 
> Should we pick a name that is a bit less generic, to avoid
> potentially causing conflict/overrides in other parts of the
> hardness / tests?  ( I know, namespaces... :-) )

Sure. Although Tcl does not support anonymous namespaces, I've wrapped the next_line procedure inside a new cp_support_internal namespace.

>> +# To create a stack, call ::Stack::new, recording the returned object ID
>> +# for future calls to manipulate the stack object.
>> +#
>> +# Example:
>> +#
>> +# set sid1 [::Stack::new]
>> +# set sid2 [::Stack::new]
>> +# stack push $sid1 a
>> +# stack push $sid1 b
>> +# stack empty $sid1;  # returns false
>> +# stack empty $sid2;  # returns true
>> +# stack pop $sid1;    # returns "b"
>> +# stack pop $sid2;    # returns ""
> 
> I'd suggest adding a line for a second pop on sid1 to
> make it dead obvious that this really behaves like a LIFO.
> And then maybe do the last pop that returns "" on the
> same stack:

I've rewritten this whole thing a little more clearly based on your suggestions.

> 
>   # stack pop $sid1;    # returns "b"
>   # stack pop $sid1;    # returns "a"
>   # stack pop $sid1;    # returns ""
> 
> Though off hand I'd assume that poping from an empty
> stack would be an error instead.  But perhaps that's more
> common in TCL?  Or does it not work to push an empty
> element, like:
> 
>   # stack push $sid1 ""
> 
> ?

Yeah, with the submitted patch as-is, empty string elements cannot be pushed onto stacks/queues. [Well, they can, but without using the empty proc, one cannot tell the difference. I've therefore changed both stack and queue to error when an empty stack/queue is popped.

>> @@ -755,6 +792,16 @@ Show printing of typedefs defined in classes."), NULL,
>>  			   set_print_type_typedefs,
>>  			   show_print_type_typedefs,
>>  			   &setprinttypelist, &showprinttypelist);
>> +
>> +  add_setshow_zuinteger_unlimited_cmd ("nested-type-limit", no_class,
>> +				       &print_nested_type_limit,
>> +				       _("\
>> +Set the number of recursive nested type definitions to print \
>> +(-1 to show all)."), _("\
> 
> Please mention the "unlimited" literal.  Something
> like ("unlimited" or -1 to show all), or even just 
> ("unlimited" to show all).  Likewise in the manual.
> See f81d112039fa.

I've followed suit with how other commands handle the "unlimited" keyword, updating documentation/help strings as necessary, e.g.,

(gdb) help set print type nested-type-limit 
Set the number of recursive nested type definitions to print ("unlimited" or -1 to show all).

Revised patch below (including commit log).

Keith

--- Patch v2

GDB currently does not track types defined in classes.  Consider:

class A
{
  public:

  class B
  {
    public:
      class C { };
  };
};

(gdb) ptype A
type = class A {
   <no data fields>
}

This patch changes this behavior so that GDB records these nested types
and displays them to the user when he has set the (new) "print type"
option "nested-type-limit."

Example:

(gdb) set print type nested-type-limit 1
(gdb) ptype A
type = class A {
    <no data fields>
    class A::B {
        <no data fields>
    };
}
(gdb) set print type nested-type-limit 2
type = class A {
    <no data fields>
    class A::B {
        <no data fields>
        class A::B::C {
            <no data fields>
        };
    };
}

By default, the code maintains the status quo, that is, it will not print
any nested type definitions at all.

Testing is carried out via cp_ptype_class which required quite a bit of
modification to permit recursive calling (for the nested types).  This
was most easily facilitated by turning the ptype command output into a
queue.  Upshot: the test suite now has stack and queue data structures that
may be used by test writers.

gdb/ChangeLog

	* NEWS (New commands): Mention set/show print type nested-type-limit.
	* c-typeprint.c (c_type_print_base): Print out nested types.
	* dwarf2read.c (struct typedef_field_list): Rename to ...
	(struct decl_field_list): ... this.  Change all uses.
	(struct field_info) <nested_types_list, nested_types_list_count>:
	New fields.
	(add_partial_symbol): Look for nested type definitions in C++, too.
	(dwarf2_add_typedef): Rename to ...
	(dwarf2_add_type_defn): ... this.
	(type_can_define_types): New function.
	Update assertion to use type_can_define_types.
	Permit NULL for a field's name.
	(process_structure_scope): Handle child DIEs of types that can
	define types.
	Copy the list of nested types into the type struct.
	* gdbtypes.h (struct typedef_field): Rename to ...
	(struct decl_field): ... this.  Change all uses.
	[is_protected, is_private]: New fields.
	(struct cplus_struct_type) <nested_types, nested_types_count>: New
	fields.
	(TYPE_NESTED_TYPES_ARRAY, TYPE_NESTED_TYPES_FIELD)
	(TYPE_NESTED_TYPES_FIELD_NAME, TYPE_NESTED_TYPES_FIELD_TYPE)
	(TYPE_NESTED_TYPES_COUNT, TYPE_NESTED_TYPES_FIELD_PUBLIC)
	(TYPE_NESTED_TYPES_FIELD_PROTECTED)
	(TYPE_NESTED_TYPES_FIELD_PRIVATE): New macros.
	* typeprint.c (type_print_raw_options, default_ptype_flags): Add
	default value for print_nested_type_limit.
	(print_nested_type_limit): New static variable.
	(set_print_type_nested_types, show_print_type_nested_types): New
	functions.
	(_initialize_typeprint): Register new commands for set/show
	`print-nested-type-limit'.
	* typeprint.h (struct type_print_options) [print_nested_type_limit]:
	New field.

gdb/testsuite/ChangeLog

	* gdb.cp/nested-types.cc: New file.
	* gdb.cp/nested-types.exp: New file.
	* lib/cp-support.exp: Load data-structures.exp library.
	(debug_cp_test_ptype_class): New global.
	(cp_ptype_class_verbose, next_line): New procedures.
	(cp_test_ptype_class): Add and document new parameter `recursive_qid'.
	Add and document new return value.
	Switch the list of lines to a queue.
	Add support for new `type' key for nested type definitions.
	Add debugging/troubleshooting messages.
	* lib/data-structures.exp: New file.

gdb/doc/ChangeLog

	* gdb.texinfo (Symbols): Document "set print type nested-type-limit"
	and "show print type nested-type-limit".
---
 gdb/NEWS                              |   4 +
 gdb/c-typeprint.c                     |  22 +-
 gdb/doc/gdb.texinfo                   |  13 +
 gdb/dwarf2read.c                      |  98 +++--
 gdb/gdbtypes.h                        |  28 +-
 gdb/testsuite/gdb.cp/nested-types.cc  | 668 ++++++++++++++++++++++++++++++++++
 gdb/testsuite/gdb.cp/nested-types.exp | 267 ++++++++++++++
 gdb/testsuite/lib/cp-support.exp      | 364 +++++++++++++++---
 gdb/testsuite/lib/data-structures.exp | 164 +++++++++
 gdb/typeprint.c                       |  71 +++-
 gdb/typeprint.h                       |   3 +
 11 files changed, 1616 insertions(+), 86 deletions(-)
 create mode 100644 gdb/testsuite/gdb.cp/nested-types.cc
 create mode 100644 gdb/testsuite/gdb.cp/nested-types.exp
 create mode 100644 gdb/testsuite/lib/data-structures.exp

Record nested types

Commit Message

Comments

Patch