Re: [PATCH 1/3] fort_dyn_array: Enable dynamic member types inside a structure.

[Yao Qi <qiyaoltc at gmail dot com>]

Bernhard Heckel <bernhard.heckel@intel.com> writes:

> fort_dyn_array: Enable dynamic member types inside a structure.
>
> 2016-02-24  Bernhard Heckel  <bernhard.heckel@intel.com>
> 2015-03-20  Keven Boell  <keven.boell@intel.com>
>
> Before:
> (gdb) print threev%ivla(1)
> Cannot access memory at address 0x3
> (gdb) print threev%ivla(5)
> no such vector element
>
> After:
> (gdb) print threev%ivla(1)
> $9 = 1
> (gdb) print threev%ivla(5)
> $10 = 42

It would be helpful to describe your change, at least people don't know
much about fortran, like me, can understand the change.

>
> gdb/Changelog:
>
> 	* gdbtypes.c (remove_dyn_prop): New.
> 	* gdbtypes.h: Forward declaration of new function.
> 	* value.c (value_address): Return dynamic resolved location of a value.
> 	(set_value_component_location): Adjust the value address
> 	for single value prints.
> 	(value_primitive_field): Support value types with a dynamic location.
> 	(set_internalvar): Remove dynamic location property of
> 	internal variables.
>
> gdb/testsuite/Changelog:
>
> 	* gdb.fortran/vla-type.f90: New file.
> 	* gdb.fortran/vla-type.exp: New file.
>
> ---
>  gdb/gdbtypes.c                         | 43 +++++++++++++--
>  gdb/gdbtypes.h                         |  3 ++
>  gdb/testsuite/gdb.fortran/vla-type.exp | 98 ++++++++++++++++++++++++++++++++++
>  gdb/testsuite/gdb.fortran/vla-type.f90 | 88 ++++++++++++++++++++++++++++++
>  gdb/value.c                            | 35 ++++++++++--
>  5 files changed, 261 insertions(+), 6 deletions(-)
>  create mode 100755 gdb/testsuite/gdb.fortran/vla-type.exp
>  create mode 100755 gdb/testsuite/gdb.fortran/vla-type.f90
>
> diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
> index f129b0e..066fe88 100644
> --- a/gdb/gdbtypes.c
> +++ b/gdb/gdbtypes.c
> @@ -2064,7 +2064,8 @@ resolve_dynamic_struct (struct type *type,
>  

First of all, the change in resolve_dynamic_struct isn't mentioned in
the CL entry.

>        pinfo.type = check_typedef (TYPE_FIELD_TYPE (type, i));
>        pinfo.valaddr = addr_stack->valaddr;
> -      pinfo.addr = addr_stack->addr;
> +      pinfo.addr = addr_stack->addr
> +              + (TYPE_FIELD_BITPOS (resolved_type, i) / TARGET_CHAR_BIT);

AFAICS, addr_stack->valaddr and addr_stack->addr can't be used
together.  Either of them is used, so why do we set both of them?

>        pinfo.next = addr_stack;
>  
>        TYPE_FIELD_TYPE (resolved_type, i)
> @@ -2090,8 +2091,13 @@ resolve_dynamic_struct (struct type *type,
>  	resolved_type_bit_length = new_bit_length;
>      }
>  
> -  TYPE_LENGTH (resolved_type)
> -    = (resolved_type_bit_length + TARGET_CHAR_BIT - 1) / TARGET_CHAR_BIT;
> +  /* Type length won't change for fortran. Keep what we got from DWARF.

Two spaces after ".".  Multiple instances of this problem.

> +     Dynamic fields might change over time but not the struct definition.
> +     If we would adapt it we run into problems when
> +     calculating the element offset for arrays of structs.  */

What is the problem we run into?

> +  if (current_language->la_language != language_fortran)
> +    TYPE_LENGTH (resolved_type)
> +      = (resolved_type_bit_length + TARGET_CHAR_BIT - 1) / TARGET_CHAR_BIT;
>  
>    /* The Ada language uses this field as a cache for static fixed types: reset
>       it as RESOLVED_TYPE must have its own static fixed type.  */
> @@ -2224,6 +2230,37 @@ add_dyn_prop (enum dynamic_prop_node_kind prop_kind, struct dynamic_prop prop,
>    TYPE_DYN_PROP_LIST (type) = temp;
>  }
>  
> +/* Remove dynamic property from a type in case it exist.  */
> +

The comment can be

   /* Remove dynamic property from TYPE in case it exists.  */

> +void
> +remove_dyn_prop (enum dynamic_prop_node_kind prop_kind,
> +                 struct type *type)
> +{
> +  struct dynamic_prop_list *prev_node, *curr_node;
> +
> +  curr_node = TYPE_DYN_PROP_LIST (type);
> +  prev_node = NULL;
> +
> +  while (NULL != curr_node)
> +    {
> +      if (curr_node->prop_kind == prop_kind)
> +	{
> +	  /* Upadate the linked list but don't free anything.
> +	     The property was allocated on objstack and it is not known
> +	     if we are on top of it. Nevertheless, everything is released
> +	     when the complete objstack is freed.  */
> +	  if (NULL == prev_node)
> +	    TYPE_DYN_PROP_LIST (type) = curr_node->next;
> +	  else
> +	    prev_node->next = curr_node->next;
> +
> +	  return;
> +	}
> +
> +      prev_node = curr_node;
> +      curr_node = curr_node->next;
> +    }
> +}

>  CORE_ADDR
> @@ -1846,6 +1851,8 @@ void
>  set_value_component_location (struct value *component,
>  			      const struct value *whole)
>  {
> +  struct type *type;
> +
>    gdb_assert (whole->lval != lval_xcallable);
>  
>    if (whole->lval == lval_internalvar)
> @@ -1861,9 +1868,14 @@ set_value_component_location (struct value *component,
>        if (funcs->copy_closure)
>          component->location.computed.closure = funcs->copy_closure (whole);
>      }
> +
> +  /* If type has a dynamic resolved location property update it's value address.  */
> +  type = value_type (whole);
> +  if (TYPE_DATA_LOCATION (type)

 TYPE_DATA_LOCATION (type) != NULL

> +      && TYPE_DATA_LOCATION_KIND (type) == PROP_CONST)
> +    set_value_address (component, TYPE_DATA_LOCATION_ADDR (type));
>  }
>  
> -
>  /* Access to the value history.  */
>  
>  /* Record a new value in the value history.
> @@ -2416,6 +2428,12 @@ set_internalvar (struct internalvar *var, struct value *val)
>  	 call error () until new_data is installed into the var->u to avoid
>  	 leaking memory.  */
>        release_value (new_data.value);
> +
> +      /* Internal variables which are created from values with a dynamic location
> +         don't need the location property of the origin anymore.
> +         Remove the location property in case it exist.  */
> +      remove_dyn_prop(DYN_PROP_DATA_LOCATION, value_type(new_data.value));

Space is needed before "(".  What is wrong if we don't do so?  Do you
have a test case for this?

> +
>        break;
>      }
>  
> @@ -3157,6 +3175,17 @@ value_primitive_field (struct value *arg1, int offset,
>        v->offset = value_offset (arg1);
>        v->embedded_offset = offset + value_embedded_offset (arg1) + boffset;
>      }
> +  else if (TYPE_DATA_LOCATION (type))
> +    {
> +      /* Field is a dynamic data member.  */
> +
> +      gdb_assert (0 == offset);
> +      /* We expect an already resolved data location.  */
> +      gdb_assert (PROP_CONST == TYPE_DATA_LOCATION_KIND (type));
> +      /* For dynamic data types defer memory allocation
> +         until we actual access the value.  */
> +      v = allocate_value_lazy (type);

Do we need to call set_value_lazy (v, 1)?

> +    }
>    else
>      {
>        /* Plain old data member */

-- 
Yao (éå)