Re: [PATCH] Record nested types

[Pedro Alves <palves at redhat dot com>]

Hi Keith,

On 10/20/2017 07:26 PM, Keith Seitz wrote:
> 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."

Nice!

This looks mostly good to me.  A few comments/questions below.

> 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

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

Super.

> @@ -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.


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

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

> +	  TYPE_NESTED_TYPES_ARRAY (type)
> +	    = ((struct decl_field *)
> +	       TYPE_ALLOC (type, sizeof (struct decl_field) * i));
> +	  TYPE_NESTED_TYPES_COUNT (type) = i;
> +
> +	  /* Reverse the list order to keep the debug info elements order.  */
> +	  while (--i >= 0)
> +	    {
> +	      struct decl_field *dest, *src;
> +
> +	      dest = &TYPE_NESTED_TYPES_FIELD (type, i);
> +	      src = &fi.nested_types_list->field;
> +	      fi.nested_types_list = fi.nested_types_list->next;
> +	      *dest = *src;
> +	    }
> +	}
> +
>        do_cleanups (back_to);
>      }
>  
> 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?

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

> +
> +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"

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

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

  # 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 ""

?

> +# A namespace/commands to support a queue.
> +#
> +# To create a queue, call ::Queue::new, recording the returned stack ID

"stack ID" -> "queue ID" ?  (The reuse of stack elements seems like
implementation detail.)

> +# for future calls to manipulate the queue object.
> +#
> +# Example:
> +#
> +# set qid1 [::Queue::new]
> +# set qid2 [::Queue::new]
> +# queue push $qid1 a
> +# queue push $qid1 b
> +# queue empty $qid1;  # returns false
> +# queue empty $qid2;  # returns true
> +# queue pop $qid1;    # returns "a"
> +# queue pop $qid2;    # returns ""
> +# queue delete $qid1
> +# queue delete $qid2

Same comments, really.

> @@ -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.

Thanks,
Pedro Alves