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

Yao Qi
2016-04-04T13:31:16Z

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));
> }
>
> -\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?

> +
> 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 (齐尧)