Re: [PATCH] fort_dyn_array: enable dynamic array of types

Joel Brobecker
2015-12-06T13:42:54Z

> I've narrowed down the patch to the bare minimum and added a more
> descriptive commit message, which explains what the patch is supposed
> to do. I thought you were still familiar with the Fortran VLA patch
> series, were this patch was one part of it. All hunks get tested in
> the attached test case.

Thanks very much for doing this. Again, my deepest apologies for
being so slow in reviewing these. I wish days where 26 hours long!

> ---
> This patch enables dynamic arrays nested in types in Fortran by
> adjusting the address of inner elements of a struct and using
> the data_location attribute to fetch values of nested VLA's
> in Fortran types.
>
> Example:
> type :: mytype
> REAL, ALLOCATABLE :: ivla1 (:, :)
> REAL, ALLOCATABLE :: ivla2 (:)
> END TYPE mytype
> [...]
> type(mytype) :: mytypev
> ALLOCATE (mytypev%ivla1 (2,2))
> ALLOCATE (mytypev%ivla2 (2))
>
> mytypev%ivla1(:, :) = 1
> mytypev%ivla2(:) = 1
>
> Old:
> (gdb) p mytypev%ivla1
> $2 = (( 8.84481654e-39, 0) ( -nan(0x7ffffd), -nan(0x7fffff)) )
>
> (gdb) p mytypev%ivla1(1,2)
> $3 = -nan(0x7ffffd)
> (gdb)
>
> New:
> (gdb) p mytypev%ivla1
> $2 = (( 1, 321) ( 123, 1) )
>
> (gdb) p mytypev%ivla1(1,2)
> $3 = 1
>
> 2015-03-20 Keven Boell <keven.boell@intel.com>
>
> * gdbtypes (resolve_dynamic_struct): Adjust address of inner types
> of a struct to reflect the offset of the inner's type data_location
> of a VLA.
> * value.c (set_value_component_location): Adjust the value address
> for single value prints. For dynamic types compute the address of
> the component value location in sub range types based on the
> location of the sub range type. This enables accessing single
> elements of a nested array.
> (value_primitive_field): Use lazy fetch to re-evaluate a field to
> get the actual value.

The "why" (Eg. "This enabls accessing [etc]") should almost always
go into the code, rather than being in the ChangeLog. The ChangeLog
should only mention the "what".

>
> testsuite/gdb.fortran:
>
> * vla-type.exp: New file.
> * vla-type.f90: New file.
> ---
> gdb/gdbtypes.c | 6 +-
> gdb/testsuite/gdb.fortran/vla-type.exp | 94 ++++++++++++++++++++++++++++++++
> gdb/testsuite/gdb.fortran/vla-type.f90 | 89 ++++++++++++++++++++++++++++++
> gdb/value.c | 39 +++++++++++--
> 4 files changed, 221 insertions(+), 7 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 b9850cf..e8987c1 100644
> --- a/gdb/gdbtypes.c
> +++ b/gdb/gdbtypes.c
> @@ -2064,7 +2064,11 @@ resolve_dynamic_struct (struct type *type,
>
> pinfo.type = check_typedef (TYPE_FIELD_TYPE (type, i));
> pinfo.valaddr = addr_stack->valaddr;
> - pinfo.addr = addr_stack->addr;
> + if (TYPE_CODE (TYPE_FIELD_TYPE (resolved_type, i)) != TYPE_CODE_RANGE)
> + pinfo.addr = addr_stack->addr
> + + (TYPE_FIELD_BITPOS (resolved_type, i) / 8);
> + else
> + pinfo.addr = addr_stack->addr;

I don't quite understand the entire logic behind this change.
Why checking for TYPE_CODE_RANGE? Also, if you adjust pinfo.addr,
you'll need to adjust pinfo.valaddr correspondingly (but watchout
for valaddr being NULL, in which case it needs to stay that way).

[divide by TARGET_CHAR_BIT instead of dividing by 8].

> diff --git a/gdb/testsuite/gdb.fortran/vla-type.exp b/gdb/testsuite/gdb.fortran/vla-type.exp
> new file mode 100755
> index 0000000..ffd0ab4
> --- /dev/null
> +++ b/gdb/testsuite/gdb.fortran/vla-type.exp
> @@ -0,0 +1,94 @@
> +# Copyright 2015 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/>.
> +
> +standard_testfile ".f90"
> +
> +if { [prepare_for_testing ${testfile}.exp ${testfile} ${srcfile} \
> + {debug f90 quiet}] } {
> + return -1
> +}
> +
> +if ![runto MAIN__] then {
> + perror "couldn't run to breakpoint MAIN__"
> + continue
> +}

See the testcase cookbook, where it shows an example of using runto
and what to do when runto fails:
https://sourceware.org/gdb/wiki/GDBTestcaseCookbook

if ![runto_main] {
untested "could not run to main"
return -1
}

> +
> +# Check if not allocated VLA in type does not break
> +# the debugger when accessing it.
> +gdb_breakpoint [gdb_get_line_number "before-allocated"]
> +gdb_continue_to_breakpoint "before-allocated"
> +gdb_test "print twov" "\\$\\d+ = \\\( <not allocated>, <not allocated> \\\)" \

For "print <expression>" tests, you don't need to match the $<number>
part, so change the expected to: " = \\\([etc]".

By the way, there are a number of predefined regexps you can use,
which is also documented in that cookbook above:
https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Convenient_variables_defined_for_use_in_output_matching

In particular, you could have used $decimal instead of \d+.

> + "print twov before allocated"
> +gdb_test "print twov%ivla1" "\\$\\d+ = <not allocated>" \
> + "print twov%ivla1 before allocated"
> +
> +# Check type with one VLA's inside
> +gdb_breakpoint [gdb_get_line_number "onev-filled"]
> +gdb_continue_to_breakpoint "onev-filled"
> +gdb_test "print onev%ivla(5, 11, 23)" "\\$\\d+ = 1" "print onev%ivla(5, 11, 23)"
> +gdb_test "print onev%ivla(1, 2, 3)" "\\$\\d+ = 123" "print onev%ivla(1, 2, 3)"
> +gdb_test "print onev%ivla(3, 2, 1)" "\\$\\d+ = 321" "print onev%ivla(3, 2, 1)"
> +gdb_test "ptype onev" \
> + "type = Type one\r\n\\s+real\\\(kind=4\\\) :: ivla\\\(11,22,33\\\)\r\nEnd Type one" \
> + "ptype onev"

For multi-line output, you can use multi_line. You might fine that
the test becomes a lot more readable when you do. This might also
help keep each line under 80 chars.

For the last argument of gdb_test, if it's identical to the command
being sent, I would simply omit it entirely. Otherwise, if you modify
the test, then you run the risk of having the label and the command
no longer match.

[this applies to the rest of the testcase file]

> diff --git a/gdb/testsuite/gdb.fortran/vla-type.f90 b/gdb/testsuite/gdb.fortran/vla-type.f90
> new file mode 100755
> index 0000000..cf3f24b
> --- /dev/null
> +++ b/gdb/testsuite/gdb.fortran/vla-type.f90
> @@ -0,0 +1,89 @@
> +! Copyright 2015 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 2 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, write to the Free Software
> +! Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.

Wrong license version and address. Make sure you use the up-to-date
copyright header used in the GDB project.

> diff --git a/gdb/value.c b/gdb/value.c
> index 91bf49e..8d8ffde 100644
> --- a/gdb/value.c
> +++ b/gdb/value.c
> @@ -1788,6 +1788,25 @@ set_value_component_location (struct value *component,
> if (funcs->copy_closure)
> component->location.computed.closure = funcs->copy_closure (whole);
> }
> +
> + /* For dynamic types compute the address of the component value location in
> + sub range types based on the location of the sub range type, if not being
> + an internal GDB variable or parts of it. */
> + if (VALUE_LVAL (component) != lval_internalvar
> + && VALUE_LVAL (component) != lval_internalvar_component)
> + {
> + CORE_ADDR addr;
> + struct type *type = value_type (whole);
> +
> + addr = value_raw_address (component);
> +
> + if (TYPE_DATA_LOCATION (type)
> + && TYPE_DATA_LOCATION_KIND (type) == PROP_CONST)
> + {
> + addr = TYPE_DATA_LOCATION_ADDR (type);
> + set_value_address (component, addr);
> + }
> + }

Can you explain that? Most likely, a dump of the DWARF you are trying
to handle would help. The comment talks about "sub range types", but
I don't see any reference to RANGE_TYPEs in the code. And why are
internal vars excluded? You get a sense that internal vars are probably
static regardless, but I fear that the condition might be giving you
the correct result for the wrong reason. I also don't understand why
the address of the address of the component should be the data-location
of the container...

> @@ -3095,13 +3114,21 @@ value_primitive_field (struct value *arg1, int offset,
> v = allocate_value_lazy (type);
> else
> {
> - v = allocate_value (type);
> - value_contents_copy_raw (v, value_embedded_offset (v),
> - arg1, value_embedded_offset (arg1) + offset,
> - type_length_units (type));
> + if (TYPE_DATA_LOCATION (type)
> + && TYPE_DATA_LOCATION_KIND (type) == PROP_CONST)
> + v = value_at_lazy (type, value_address (arg1) + offset);
> + else
> + {
> + v = allocate_value (type);
> + value_contents_copy_raw (v, value_embedded_offset (v),
> + arg1, value_embedded_offset (arg1) + offset,
> + type_length_units (type));
> + }
> }
> - v->offset = (value_offset (arg1) + offset
> - + value_embedded_offset (arg1));
> + if (!TYPE_DATA_LOCATION (type)
> + || !TYPE_DATA_LOCATION_KIND (type) == PROP_CONST)
> + v->offset = (value_offset (arg1) + offset
> + + value_embedded_offset (arg1));

I'm sorry if I am being dense, but I don't undestand this hunk
either. I'm looking at the first "if" you added, and if the field
has a TYPE_DATA_LOCATION, and the data-location is either constant
or has been resolved, then you create a value, except that instead
of using the field's data location, you use arg1's address, which
is the container?!? And then you condition the setting of v->offset
by the opposite of that condititon - why?

Taking a step back, one thing that strikes me is the fact that
perhaps we could avoid all that special handling of struct/union
component data location if we had resolved the type first. Then
we know that the field's location should be correct. If not, what's
preventing us from making it so, so that the rest of the code can
forget about dynamic types?

--
Joel