Re: [PATCH] Record nested types

[Pedro Alves <palves at redhat dot com>]

Hi Keith,

Sorry for the delay...

This version is OK with super minor nits below.

On 10/31/2017 07:03 PM, Keith Seitz wrote:
> On 10/27/2017 05:00 AM, Pedro Alves wrote:

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

OK.  The think that made me question it is that main_type.type_specific is
a union, and thus unconditionally setting it to cplus_specific looks
wrong if other languages expect to put/find a different active union
member there.  E.g., for set_die_type has:

static struct type *
set_die_type (struct die_info *die, struct type *type, struct dwarf2_cu *cu)
{
  struct dwarf2_per_cu_offset_and_type **slot, ofs;
  struct objfile *objfile = cu->objfile;
  struct attribute *attr;
  struct dynamic_prop prop;

  /* For Ada types, make sure that the gnat-specific data is always
     initialized (if not already set).  There are a few types where
     we should not be doing so, because the type-specific area is
     already used to hold some other piece of info (eg: TYPE_CODE_FLT
     where the type-specific area is used to store the floatformat).
     But this is not a problem, because the gnat-specific information
     is actually not needed for these types.  */
  if (need_gnat_info (cu)
      && TYPE_CODE (type) != TYPE_CODE_FUNC
      && TYPE_CODE (type) != TYPE_CODE_FLT
      && TYPE_CODE (type) != TYPE_CODE_METHODPTR
      && TYPE_CODE (type) != TYPE_CODE_MEMBERPTR
      && TYPE_CODE (type) != TYPE_CODE_METHOD
      && !HAVE_GNAT_AUX_INFO (type))
    INIT_GNAT_SPECIFIC (type);


Looking at:

enum type_specific_kind
{
  TYPE_SPECIFIC_NONE,
  TYPE_SPECIFIC_CPLUS_STUFF,
  TYPE_SPECIFIC_GNAT_STUFF,
  TYPE_SPECIFIC_FLOATFORMAT,
  /* Note: This is used by TYPE_CODE_FUNC and TYPE_CODE_METHOD.  */
  TYPE_SPECIFIC_FUNC,
  TYPE_SPECIFIC_SELF_TYPE
};


I guess Ada uses TYPE_SPECIFIC_GNAT_STUFF, and every other
language uses TYPE_SPECIFIC_CPLUS_STUFF.

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

Alright.  Given that and the above, this sounds OK.

>>> +++ b/gdb/testsuite/gdb.cp/nested-types.cc
>>> @@ -0,0 +1,668 @@
>>> +/* This testcase is part of GDB, the GNU debugger.

...

>>> +/* 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.]

Ah, that's even better.  Could you add a comment at the top of
the .cc file, so that the next person that needs to change/extend
the file doesn't try to do it manually?

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

Nope, that's fine; I had assumed you had typed the whole thing!  :-P

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

Thanks.

One thing that crossed my mind is that it would be really nice
to have unit tests for these tcl data structures we come up with.
E.g., add a new gdb.whatever/testsuite-selftests.exp or something
like that, that runs just as any other GDB testcase, but that instead
of spawning GDB, it exercises the new APIs, and calls pass/fail
accordingly.  That'd serve both to unsure test coverage of the whole API,
and also as kind of API documentation, as folks that want to know how
to use the API would find easy/isolated examples there.

Anyway, that's a IWBN; it can be done separately and I don't want
to delay this patch further.

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

Thanks.

>  
> -/* Add a typedef defined in the scope of the FIP's class.  */
> +/* Can the type given by DIE define another type?  */
> +
> +static bool
> +type_can_define_types (const struct die_info *die)
> +{
> +  switch (die->tag)
> +    {
> +    case DW_TAG_typedef:
> +    case DW_TAG_class_type:
> +    case  DW_TAG_structure_type:

Spurious double space above.

> +    case DW_TAG_union_type:
> +    case DW_TAG_enumeration_type:
> +      return true;
> +
> +    default:
> +      return false;
> +    }
> +}

...

> +# A namespace/commands to support a queue.
> +#
> +# To create a queue, call ::Queue::new, recording the returned queue ID
> +# for future calls to manipulate the queue object.
> +#
> +# Example:
> +#
> +# set qid [::Queue::new]
> +# queue push $qid a
> +# queue push $qid b
> +# queue empty $qid;  # returns false
> +# queue pop $qid;    # returns "b"
> +# queue pop $qid;    # returns "a"

I assume the first pop returns "a" instead.  :-P

> +# queue pop $qid;    # errors with "queue is empty"
> +# queue delete $qid
> +

Thanks,
Pedro Alves