Re: [PATCH 1/2] fort_dyn_array: add basic fortran dyn array support

[Joel Brobecker <brobecker at adacore dot com>]

> 2015-03-13  Keven Boell  <keven.boell@intel.com>
> 
> 	* dwarf2loc.c (dwarf2_address_data_valid): New
> 	function.
> 	* dwarf2loc.h (dwarf2_address_data_valid): New
> 	function.
> 	* dwarf2read.c (set_die_type): Add read of
> 	DW_AT_allocated and DW_AT_associated.
> 	* f-typeprint.c (f_print_type): Add check for
> 	allocated/associated status of type.
> 	(f_type_print_varspec_suffix): Add check for
> 	allocated/associated status of type.
> 	* gdbtypes.c (create_array_type_with_stride):
> 	Add check for valid data location of type in
> 	case allocated or associated attributes are set.
> 	Length of an array should be only calculated if
> 	allocated or associated is resolved as true.
> 	(is_dynamic_type_internal): Add check for allocated/
> 	associated.
> 	(resolve_dynamic_array): Evaluate allocated/associated
> 	properties.  Since at the end of the function a new
> 	array type will be created where the length is
> 	calculated the properties need to be resolved before.
> 	* gdbtypes.h (enum dynamic_prop_node_kind): Add
> 	allocated/associated.
> 	Add convenient macros to handle allocated/associated.
> 	* valarith.c (value_subscripted_rvalue): Add check for
> 	allocated/associated.
> 	* valprint.c (valprint_check_validity): Add check for
> 	allocated/associated.
> 	(val_print_not_allocated): New function.
> 	(val_print_not_associated): New function.
> 	(value_check_printable): Add check for allocated/
> 	associated.
> 	* valprint.h (val_print_not_allocated): New function.
> 	(val_print_not_associated): New function.

The ChangeLog needs to be reviewed. In particular,
dwarf2_address_data_valid has been removed...

You need to also mention the new #include-s (I missed that in
the previous review).

Also, explicitly mentioning that the patch was tested using
the testsuite and citing the platform on which it was tested
would be helpful.

> 
> testsuite/gdb.fortran:
> 
> 	* vla-alloc-assoc.exp: New file.
> 	* vla-datatypes.exp: New file.
> 	* vla-datatypes.f90: New file.
> 	* vla-history.exp: New file.
> 	* vla-ptype-sub.exp: New file.
> 	* vla-ptype.exp: New file.
> 	* vla-sizeof.exp: New file.
> 	* vla-sub.f90: New file.
> 	* vla-value-sub-arbitrary.exp: New file.
> 	* vla-value-sub-finish.exp: New file.
> 	* vla-value-sub.exp: New file.
> 	* vla-value.exp: New file.
> 	* vla-ptr-info.exp: New file.
> 	* vla.f90: New file.
> 
> testsuite/gdb.mi:
> 
> 	* mi-vla-fortran.exp: New file.
> 	* vla.f90: New file.


Comments below.

> 
> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
> index b5ffd04..df15a76 100644
> --- a/gdb/dwarf2read.c
> +++ b/gdb/dwarf2read.c
> @@ -22300,6 +22300,32 @@ set_die_type (struct die_info *die, struct type *type, struct dwarf2_cu *cu)
>        && !HAVE_GNAT_AUX_INFO (type))
>      INIT_GNAT_SPECIFIC (type);
>  
> +  /* Read DW_AT_allocated and set in type.  */
> +  attr = dwarf2_attr (die, DW_AT_allocated, cu);
> +  if (attr_form_is_block (attr))
> +    {
> +      if (attr_to_dynamic_prop (attr, die, cu, &prop))
> +        add_dyn_prop (DYN_PROP_ALLOCATED, prop, type, objfile);
> +    }
> +  else
> +    {
> +        complaint (&symfile_complaints,
> +                  _("DW_AT_allocated expected to be a block"));

It would be more helpful to the person seeing this complaint
if we told them which DIE has this attribute with an unexpected
form. I suggest we avoid "expected to be a block" and just say
"has the wrong form (<actual form>)". That way, if we ever expand
the list lif forms being supported for this attribute, we won't
have to remember to update the complaint.

So, something like the following:

        _("DW_AT_allocated attribute has wrong form (%s) at DIE 0x%x"),
        dwarf_form_name (...), die.offset.sect_off

> +    }
> +
> +  /* Read DW_AT_associated and set in type.  */
> +  attr = dwarf2_attr (die, DW_AT_associated, cu);
> +  if (attr_form_is_block (attr))
> +    {
> +      if (attr_to_dynamic_prop (attr, die, cu, &prop))
> +        add_dyn_prop (DYN_PROP_ASSOCIATED, prop, type, objfile);
> +    }
> +  else
> +    {
> +        complaint (&symfile_complaints,
> +                  _("DW_AT_associated expected to be a block"));

Same here...

> +    }
> +
>    /* Read DW_AT_data_location and set in type.  */
>    attr = dwarf2_attr (die, DW_AT_data_location, cu);
>    if (attr_to_dynamic_prop (attr, die, cu, &prop))
> diff --git a/gdb/f-typeprint.c b/gdb/f-typeprint.c
> index 590ed73..e34f4f1 100644
> --- a/gdb/f-typeprint.c
> +++ b/gdb/f-typeprint.c
> @@ -30,6 +30,8 @@
>  #include "gdbcore.h"
>  #include "target.h"
>  #include "f-lang.h"
> +#include "valprint.h"
> +#include "typeprint.h"

Do you still need the "valprint.h" include?

>  #if 0				/* Currently unused.  */
>  static void f_type_print_args (struct type *, struct ui_file *);
> @@ -53,6 +55,18 @@ f_print_type (struct type *type, const char *varstring, struct ui_file *stream,
>    enum type_code code;
>    int demangled_args;
>  
> +  if (type_not_associated (type))
> +    {
> +      val_print_not_associated (stream);
> +      return;
> +    }
> +
> +  if (type_not_allocated (type))
> +    {
> +      val_print_not_allocated (stream);
> +      return;
> +    }
> +
>    f_type_print_base (type, stream, show, level);
>    code = TYPE_CODE (type);
>    if ((varstring != NULL && *varstring != '\0')
> @@ -167,28 +181,35 @@ f_type_print_varspec_suffix (struct type *type, struct ui_file *stream,
>        if (arrayprint_recurse_level == 1)
>  	fprintf_filtered (stream, "(");
>  
> -      if (TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_ARRAY)
> -	f_type_print_varspec_suffix (TYPE_TARGET_TYPE (type), stream, 0, 0, 0,
> -				     arrayprint_recurse_level);
> -
> -      lower_bound = f77_get_lowerbound (type);
> -      if (lower_bound != 1)	/* Not the default.  */
> -	fprintf_filtered (stream, "%d:", lower_bound);
> -
> -      /* Make sure that, if we have an assumed size array, we
> -         print out a warning and print the upperbound as '*'.  */
> -
> -      if (TYPE_ARRAY_UPPER_BOUND_IS_UNDEFINED (type))
> -	fprintf_filtered (stream, "*");
> +      if (type_not_associated (type))
> +        val_print_not_associated (stream);
> +      else if (type_not_allocated (type))
> +        val_print_not_allocated (stream);
>        else
> -	{
> -	  upper_bound = f77_get_upperbound (type);
> -	  fprintf_filtered (stream, "%d", upper_bound);
> -	}
> -
> -      if (TYPE_CODE (TYPE_TARGET_TYPE (type)) != TYPE_CODE_ARRAY)
> -	f_type_print_varspec_suffix (TYPE_TARGET_TYPE (type), stream, 0, 0, 0,
> -				     arrayprint_recurse_level);
> +        {
> +          if (TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_ARRAY)
            f_type_print_varspec_suffix (TYPE_TARGET_TYPE (type), stream, 0,
                        0, 0, arrayprint_recurse_level);

The formatting here is wrong. The second line needs to be aligned
just past the opening '(' on the previous line:

            f_type_print_varspec_suffix (TYPE_TARGET_TYPE (type), stream, 0,
                                         0, 0, arrayprint_recurse_level);

> +
> +          lower_bound = f77_get_lowerbound (type);
> +          if (lower_bound != 1)	/* Not the default.  */
> +            fprintf_filtered (stream, "%d:", lower_bound);
> +
> +          /* Make sure that, if we have an assumed size array, we
> +             print out a warning and print the upperbound as '*'.  */
> +
> +          if (TYPE_ARRAY_UPPER_BOUND_IS_UNDEFINED (type))
> +            fprintf_filtered (stream, "*");
> +          else
> +            {
> +              upper_bound = f77_get_upperbound (type);
> +              fprintf_filtered (stream, "%d", upper_bound);
> +            }
> +
> +          if (TYPE_CODE (TYPE_TARGET_TYPE (type)) != TYPE_CODE_ARRAY)
> +            f_type_print_varspec_suffix (TYPE_TARGET_TYPE (type), stream, 0,
> +                            0, 0, arrayprint_recurse_level);

Same here.

> +        }
>        if (arrayprint_recurse_level == 1)
>  	fprintf_filtered (stream, ")");
>        else
> diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
> index 125af01..4de8696 100644
> --- a/gdb/gdbtypes.c
> +++ b/gdb/gdbtypes.c
> @@ -1079,7 +1079,9 @@ create_array_type_with_stride (struct type *result_type,
>  
>    TYPE_CODE (result_type) = TYPE_CODE_ARRAY;
>    TYPE_TARGET_TYPE (result_type) = element_type;
> -  if (has_static_range (TYPE_RANGE_DATA (range_type)))
> +  if (has_static_range (TYPE_RANGE_DATA (range_type))
> +      && (!type_not_associated (result_type) &&
> +      !type_not_allocated (result_type)))

GNU Coding Style: Binary operators should be at the startt of
the line, not at the end. Also, the alignment on the second line
is wrong. So:

  if (has_static_range (TYPE_RANGE_DATA (range_type))
      && (!type_not_associated (result_type)
          && !type_not_allocated (result_type)))

>      {
>        LONGEST low_bound, high_bound;
>  
> @@ -1817,6 +1819,12 @@ is_dynamic_type_internal (struct type *type, int top_level)
>  	  || TYPE_DATA_LOCATION_KIND (type) == PROP_LOCLIST))
>      return 1;
>  
> +  if (TYPE_ASSOCIATED_PROP (type))
> +    return 1;
> +
> +  if (TYPE_ALLOCATED_PROP (type))
> +    return 1;
> +
>    switch (TYPE_CODE (type))
>      {
>      case TYPE_CODE_RANGE:
> @@ -1934,23 +1942,40 @@ resolve_dynamic_array (struct type *type,
>    struct type *elt_type;
>    struct type *range_type;
>    struct type *ary_dim;
> +  struct dynamic_prop *prop;
>  
>    gdb_assert (TYPE_CODE (type) == TYPE_CODE_ARRAY);
>  
> +  type = copy_type (type);
> +
>    elt_type = type;
>    range_type = check_typedef (TYPE_INDEX_TYPE (elt_type));
>    range_type = resolve_dynamic_range (range_type, addr_stack);
>  
> +  /* Resolve allocated/associated here before creating a new array type, which
> +     will update the length of the array accordingly.  */
> +  prop = TYPE_ALLOCATED_PROP (type);
> +  if (prop != NULL && dwarf2_evaluate_property (prop, addr_stack, &value))
> +    {
> +      TYPE_DYN_PROP_ADDR (prop) = value;
> +      TYPE_DYN_PROP_KIND (prop) = PROP_CONST;
> +    }
> +  prop = TYPE_ASSOCIATED_PROP (type);
> +  if (prop != NULL && dwarf2_evaluate_property (prop, addr_stack, &value))
> +    {
> +      TYPE_DYN_PROP_ADDR (prop) = value;
> +      TYPE_DYN_PROP_KIND (prop) = PROP_CONST;
> +    }
> +
>    ary_dim = check_typedef (TYPE_TARGET_TYPE (elt_type));
>  
>    if (ary_dim != NULL && TYPE_CODE (ary_dim) == TYPE_CODE_ARRAY)
> -    elt_type = resolve_dynamic_array (ary_dim, addr_stack);
> +    elt_type = resolve_dynamic_array (TYPE_TARGET_TYPE (type), addr_stack);

This is undoing:

    commit d0d8478068ae7c01b1a504ca2fba90c1d36c5566
    Author: Pierre-Marie de Rodat <derodat@adacore.com>
    Date:   Wed Jul 22 12:25:14 2015 +0200
    Subject: gdb/gdbtypes: fix handling of typedef layers between array types

I think resolve_dynamic_array should be passed ary_dim, here.

>    else
>      elt_type = TYPE_TARGET_TYPE (type);
>  
> -  return create_array_type_with_stride (copy_type (type),
> -					elt_type, range_type,
> -					TYPE_FIELD_BITSIZE (type, 0));
> +  return create_array_type_with_stride (type, elt_type, range_type,
> +                                        TYPE_FIELD_BITSIZE (type, 0));
>  }
>  
>  /* Resolve dynamic bounds of members of the union TYPE to static
> @@ -3372,6 +3397,28 @@ types_deeply_equal (struct type *type1, struct type *type2)
>  
>    return result;
>  }
> +
> +/* Allocated status of type TYPE. Return zero if type TYPE is allocated.
> +   Otherwise return one.  */

Missing second space after the first period (GNU Coding Style).

> +
> +int
> +type_not_allocated (const struct type *type)
> +{
> +    return TYPE_ALLOCATED_PROP (type)
> +           && TYPE_DYN_PROP_KIND (TYPE_ALLOCATED_PROP (type)) == PROP_CONST
> +           && !TYPE_DYN_PROP_ADDR (TYPE_ALLOCATED_PROP (type));
> +}

The GCS (GNU Coding Style) also asks us to help automatic code formatters
in cases like this. So, although not meaningful, we enclose the condition
inside "(...)". Also, the indentation of the "return" looks wrong.

  return (TYPE_ALLOCATED_PROP (type)
          && TYPE_DYN_PROP_KIND (TYPE_ALLOCATED_PROP (type)) == PROP_CONST
          && !TYPE_DYN_PROP_ADDR (TYPE_ALLOCATED_PROP (type)));

Also, why not use a local variable, Eg:

  struct dynamic_prop *prop = TYPE_ALLOCATED_PROP (type);

  return (prop != NULL
          && TYPE_DYN_PROP_KIND (prop) == PROP_CONST
          && !TYPE_DYN_PROP_ADDR (prop))

... which seems easier to read to me, but also avoid 3 function
identical function calls.

> +
> +/* Associated status of type TYPE. Return zero if type TYPE is associated.
> +   Otherwise return one.  */
> +
> +int
> +type_not_associated (const struct type *type)
> +{
> +    return TYPE_ASSOCIATED_PROP (type)
> +           && TYPE_DYN_PROP_KIND (TYPE_ASSOCIATED_PROP (type)) == PROP_CONST
> +           && !TYPE_DYN_PROP_ADDR (TYPE_ASSOCIATED_PROP (type));
> +}

Ditto for the function comment and the function implementation.

>  
>  /* Compare one type (PARM) for compatibility with another (ARG).
>   * PARM is intended to be the parameter type of a function; and
> diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
> index f270855..330024a 100644
> --- a/gdb/gdbtypes.h
> +++ b/gdb/gdbtypes.h
> @@ -440,6 +440,16 @@ enum dynamic_prop_node_kind
>    /* A property providing a type's data location.
>       Evaluating this field yields to the location of an object's data.  */
>    DYN_PROP_DATA_LOCATION,
> +
> +  /* A property representing DW_AT_allocated.  The presence of this attribute
> +     indicates that the object of the type can be allocated/deallocated.
> +     The value can be a dwarf expression, reference, or a constant.  */
> +  DYN_PROP_ALLOCATED,
> +
> +  /* A property representing DW_AT_allocated.  The presence of this attribute
> +     indicated that the object of the type can be associated.  The value can be
> +     a dwarf expression, reference, or a constant.*/
> +  DYN_PROP_ASSOCIATED,

I don't mind the quick explanation "the object of the type can" if
you think it helps, but I think indicating what form it can take is
a recipe for this part of the comment one day becoming obsolete.

>  };
>  
>  /* * List for dynamic type attributes.  */
> @@ -1258,6 +1268,12 @@ extern void allocate_gnat_aux_type (struct type *);
>  #define TYPE_DATA_LOCATION_KIND(thistype) \
>    TYPE_DATA_LOCATION (thistype)->kind
>  
> +/* Property accessors for the type allocated/associated.  */
> +#define TYPE_ALLOCATED_PROP(thistype) \
> +  get_dyn_prop (DYN_PROP_ALLOCATED, thistype)
> +#define TYPE_ASSOCIATED_PROP(thistype) \
> +  get_dyn_prop (DYN_PROP_ASSOCIATED, thistype)
> +
>  /* Attribute accessors for dynamic properties.  */
>  #define TYPE_DYN_PROP_LIST(thistype) \
>    TYPE_MAIN_TYPE(thistype)->dyn_prop_list
> @@ -1930,4 +1946,8 @@ extern int types_equal (struct type *, struct type *);
>  
>  extern int types_deeply_equal (struct type *, struct type *);
>  
> +extern int type_not_allocated (const struct type *type);
> +
> +extern int type_not_associated (const struct type *type);
> +
>  #endif /* GDBTYPES_H */
> diff --git a/gdb/testsuite/gdb.fortran/vla-alloc-assoc.exp b/gdb/testsuite/gdb.fortran/vla-alloc-assoc.exp
> new file mode 100644
> index 0000000..ad85977
> --- /dev/null
> +++ b/gdb/testsuite/gdb.fortran/vla-alloc-assoc.exp
> @@ -0,0 +1,65 @@
> +# 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 "vla.f90"
> +
> +if { [prepare_for_testing ${testfile}.exp ${testfile} ${srcfile} \
> +    {debug f90 quiet}] } {
> +    return -1
> +}

You said:
> > I only quickly scanned this patch, as it's really huge (huge
> > is good, in this case).
> > 
> > One general comment is that we avoid re-using the same code
> > for each test. See:
> > https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Make_sure_test_executables_are_unique
> > 
> > Even if all testcases end up using the same code, I suggest
> > making one source file for each testcase, and naming the source
> > file the same as the .exp files (modulo the extension, of course).
> > That's a fairly standard practice that makes it easier to associate
> > testcase and code.
> > 
> 
> The chapter you refered to on the Testcase Cookbook page says
> the opposite:
> "[...] but if for some reason a new test reuses the sources of
> another existing test, the new test shall compile to its own
> executable [...]"
> This is exactly what I'm doing here.  I would like to avoid
> code duplications as much as possible.

I don't think it says the opposite: It _recommends_ that each test
has its own code. And if really you cannot follow that recommendation,
then you must make sure that each testcase creates an executable
that is unique to the testcase.

You are creating doubt in my mind that I may have this one wrong,
but it seems to me that "standard_testfile vla.f90" will create
an executable called "val". And you have at least two testcases
that reuse the same "vla.f90" using standard_testfile, which leads
me to believe that you have 2 testcases creating executables with
the same name.

I really don't think that avoiding code duplication brings us
much value, in the case of testcases, as we tend to adjust/augment
them as we fix bugs. But I will leave the choice to you, since
you will likely be the one living with that choice. What I ask
is that you make sure that each testcase create its own executable.


-- 
Joel