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

["Heckel, Bernhard" <bernhard.heckel@intel.com>]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8"; format="flowed", Size: 8829 bytes --]

On 04/04/2016 15:30, Yao Qi wrote:
> 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?
Actually, when resolving dynamic types I don't use the valaddr. From 
what I understood
I could even return if valaddr is not Null as the TYPE should already be 
resolved at that time.
But I was unsure about it and kept the code.
>
>>         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?
Imagine we have a dynamic allocatable array as a member of a structure. 
The size of the dynamic array can vary over time.
When we have resolved this allocatable array we don't want to add it's 
length to the structure length it belongs to.
Dynamic members are not embedded in the structure itself. Only the 
description of the dynamic type is embedded
and the size of the description (address, bounds,...) won't change.
If we add the length of the resolved type we would run in problems when 
we resolve arrays of structs
with dynamic types (usually allocatable types) as we would use the wrong 
location (offset) for the boundary, size, location,.. attributes -> see 
patch 2/3.

>
>> +  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));
>>   }
>>   
>> -\f
>>   /* 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?
An internal variable has it's own valaddr to where the copy is located.
If we keep the dynamic location from the origin then this dyn. location 
will be used to set
the value component location.
As the internal variable is a copy of an value at a certain point in 
time I prefer to get rid off the
dynamic location attribute then to do some "if else if else" constructs 
when setting the component location.
There are tests:
gdb.fortran/vla-value.exp: print $myvar set to vla1
gdb.fortran/vla-value.exp: print $myvar(3,6,9)
>
>> +
>>         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 */

Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
\x16º&Öéj×!zÊÞ¶êç×}:ó™b²Ö«r\x18\x1dn–­r\x17¬