[PATCH] Fix size recalculation of fortran arrays

[PATCH] Fix size recalculation of fortran arrays

[Hannes Domani]

My recent change regarding size calculation of arrays of stubbed types
didn't take array strides and associated/allocated type properties into
account, which basically broke fortran arrays.

Fixed by refactoring the array size calculation of
create_array_type_with_stride into a new function, and also use it for
the stubbed array size recalculation.

gdb/ChangeLog:

2020-05-01  Hannes Domani  <ssbssa@yahoo.de>

	* gdbtypes.c (calculate_static_array_size): New function.
	(create_array_type_with_stride): Use calculate_static_array_size.
	(check_typedef): Likewise.
---
 gdb/gdbtypes.c | 130 ++++++++++++++++++++++++++-----------------------
 1 file changed, 68 insertions(+), 62 deletions(-)

diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index 6648dc4d67..ad5c7ee0af 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -1177,6 +1177,62 @@ discrete_position (struct type *type, LONGEST val, LONGEST *pos)
     }
 }
 
+/* If the array has static bounds, calculate its size.  */
+
+static bool
+calculate_static_array_size (struct type *type)
+{
+  gdb_assert (TYPE_CODE (type) == TYPE_CODE_ARRAY);
+
+  struct type *range_type = TYPE_INDEX_TYPE (type);
+
+  if (get_dyn_prop (DYN_PROP_BYTE_STRIDE, type) == nullptr
+      && has_static_range (TYPE_RANGE_DATA (range_type))
+      && (!type_not_associated (type)
+	  && !type_not_allocated (type)))
+    {
+      LONGEST low_bound, high_bound;
+      int stride;
+      struct type *element_type;
+
+      /* If the array itself doesn't provide a stride value then take
+	 whatever stride the range provides.  Don't update BIT_STRIDE as
+	 we don't want to place the stride value from the range into this
+	 arrays bit size field.  */
+      stride = TYPE_FIELD_BITSIZE (type, 0);
+      if (stride == 0)
+	stride = TYPE_BIT_STRIDE (range_type);
+
+      if (get_discrete_bounds (range_type, &low_bound, &high_bound) < 0)
+	low_bound = high_bound = 0;
+      element_type = check_typedef (TYPE_TARGET_TYPE (type));
+      /* Be careful when setting the array length.  Ada arrays can be
+	 empty arrays with the high_bound being smaller than the low_bound.
+	 In such cases, the array length should be zero.  */
+      if (high_bound < low_bound)
+	TYPE_LENGTH (type) = 0;
+      else if (stride != 0)
+	{
+	  /* Ensure that the type length is always positive, even in the
+	     case where (for example in Fortran) we have a negative
+	     stride.  It is possible to have a single element array with a
+	     negative stride in Fortran (this doesn't mean anything
+	     special, it's still just a single element array) so do
+	     consider that case when touching this code.  */
+	  LONGEST element_count = std::abs (high_bound - low_bound + 1);
+	  TYPE_LENGTH (type)
+	    = ((std::abs (stride) * element_count) + 7) / 8;
+	}
+      else
+	TYPE_LENGTH (type) =
+	  TYPE_LENGTH (element_type) * (high_bound - low_bound + 1);
+
+      return true;
+    }
+
+  return false;
+}
+
 /* Create an array type using either a blank type supplied in
    RESULT_TYPE, or creating a new type, inheriting the objfile from
    RANGE_TYPE.
@@ -1222,47 +1278,17 @@ create_array_type_with_stride (struct type *result_type,
 
   TYPE_CODE (result_type) = TYPE_CODE_ARRAY;
   TYPE_TARGET_TYPE (result_type) = element_type;
-  if (byte_stride_prop == NULL
-      && has_static_range (TYPE_RANGE_DATA (range_type))
-      && (!type_not_associated (result_type)
-	  && !type_not_allocated (result_type)))
-    {
-      LONGEST low_bound, high_bound;
-      int stride;
 
-      /* If the array itself doesn't provide a stride value then take
-	 whatever stride the range provides.  Don't update BIT_STRIDE as
-	 we don't want to place the stride value from the range into this
-	 arrays bit size field.  */
-      stride = bit_stride;
-      if (stride == 0)
-	stride = TYPE_BIT_STRIDE (range_type);
+  TYPE_NFIELDS (result_type) = 1;
+  TYPE_FIELDS (result_type) =
+    (struct field *) TYPE_ZALLOC (result_type, sizeof (struct field));
+  TYPE_INDEX_TYPE (result_type) = range_type;
+  if (byte_stride_prop != NULL)
+    add_dyn_prop (DYN_PROP_BYTE_STRIDE, *byte_stride_prop, result_type);
+  else if (bit_stride > 0)
+    TYPE_FIELD_BITSIZE (result_type, 0) = bit_stride;
 
-      if (get_discrete_bounds (range_type, &low_bound, &high_bound) < 0)
-	low_bound = high_bound = 0;
-      element_type = check_typedef (element_type);
-      /* Be careful when setting the array length.  Ada arrays can be
-	 empty arrays with the high_bound being smaller than the low_bound.
-	 In such cases, the array length should be zero.  */
-      if (high_bound < low_bound)
-	TYPE_LENGTH (result_type) = 0;
-      else if (stride != 0)
-	{
-	  /* Ensure that the type length is always positive, even in the
-	     case where (for example in Fortran) we have a negative
-	     stride.  It is possible to have a single element array with a
-	     negative stride in Fortran (this doesn't mean anything
-	     special, it's still just a single element array) so do
-	     consider that case when touching this code.  */
-	  LONGEST element_count = std::abs (high_bound - low_bound + 1);
-	  TYPE_LENGTH (result_type)
-	    = ((std::abs (stride) * element_count) + 7) / 8;
-	}
-      else
-	TYPE_LENGTH (result_type) =
-	  TYPE_LENGTH (element_type) * (high_bound - low_bound + 1);
-    }
-  else
+  if (!calculate_static_array_size (result_type))
     {
       /* This type is dynamic and its length needs to be computed
          on demand.  In the meantime, avoid leaving the TYPE_LENGTH
@@ -1273,15 +1299,6 @@ create_array_type_with_stride (struct type *result_type,
       TYPE_LENGTH (result_type) = 0;
     }
 
-  TYPE_NFIELDS (result_type) = 1;
-  TYPE_FIELDS (result_type) =
-    (struct field *) TYPE_ZALLOC (result_type, sizeof (struct field));
-  TYPE_INDEX_TYPE (result_type) = range_type;
-  if (byte_stride_prop != NULL)
-    add_dyn_prop (DYN_PROP_BYTE_STRIDE, *byte_stride_prop, result_type);
-  else if (bit_stride > 0)
-    TYPE_FIELD_BITSIZE (result_type, 0) = bit_stride;
-
   /* TYPE_TARGET_STUB will take care of zero length arrays.  */
   if (TYPE_LENGTH (result_type) == 0)
     TYPE_TARGET_STUB (result_type) = 1;
@@ -2873,20 +2890,9 @@ check_typedef (struct type *type)
 	  TYPE_LENGTH (type) = TYPE_LENGTH (target_type);
 	  TYPE_TARGET_STUB (type) = 0;
 	}
-      else if (TYPE_CODE (type) == TYPE_CODE_ARRAY)
-	{
-	  struct type *range_type = check_typedef (TYPE_INDEX_TYPE (type));
-	  if (has_static_range (TYPE_RANGE_DATA (range_type)))
-	    {
-	      ULONGEST len = 0;
-	      LONGEST low_bound = TYPE_LOW_BOUND (range_type);
-	      LONGEST high_bound = TYPE_HIGH_BOUND (range_type);
-	      if (high_bound >= low_bound)
-		len = (high_bound - low_bound + 1) * TYPE_LENGTH (target_type);
-	      TYPE_LENGTH (type) = len;
-	      TYPE_TARGET_STUB (type) = 0;
-	    }
-	}
+      else if (TYPE_CODE (type) == TYPE_CODE_ARRAY
+	       && calculate_static_array_size (type))
+	TYPE_TARGET_STUB (type) = 0;
     }
 
   type = make_qualified_type (type, instance_flags, NULL);
-- 
2.26.2

Re: [PATCH] Fix size recalculation of fortran arrays

[Andrew Burgess]

* Hannes Domani via Gdb-patches <gdb-patches@sourceware.org> [2020-05-01 14:35:18 +0200]:

> My recent change regarding size calculation of arrays of stubbed types
> didn't take array strides and associated/allocated type properties into
> account, which basically broke fortran arrays.
> 
> Fixed by refactoring the array size calculation of
> create_array_type_with_stride into a new function, and also use it for
> the stubbed array size recalculation.
> 
> gdb/ChangeLog:
> 
> 2020-05-01  Hannes Domani  <ssbssa@yahoo.de>
> 
> 	* gdbtypes.c (calculate_static_array_size): New function.
> 	(create_array_type_with_stride): Use calculate_static_array_size.
> 	(check_typedef): Likewise.
> ---
>  gdb/gdbtypes.c | 130 ++++++++++++++++++++++++++-----------------------
>  1 file changed, 68 insertions(+), 62 deletions(-)
> 
> diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
> index 6648dc4d67..ad5c7ee0af 100644
> --- a/gdb/gdbtypes.c
> +++ b/gdb/gdbtypes.c
> @@ -1177,6 +1177,62 @@ discrete_position (struct type *type, LONGEST val, LONGEST *pos)
>      }
>  }
>  
> +/* If the array has static bounds, calculate its size.  */
> +
> +static bool
> +calculate_static_array_size (struct type *type)

Could you rename this to something like update_static_array_size -
calculate to me implies something will be calculated and returned.
Also please update the header comment to reference TYPE, and describe
the return values, something like:

  /* If the array TYPE has static bounds calculate and update its
     size, then return true.  Otherwise return false and leave TYPE
     unchanged.  */

With those changes this looks good.  Thanks for the quick fix.

Andrew


> +{
> +  gdb_assert (TYPE_CODE (type) == TYPE_CODE_ARRAY);
> +
> +  struct type *range_type = TYPE_INDEX_TYPE (type);
> +
> +  if (get_dyn_prop (DYN_PROP_BYTE_STRIDE, type) == nullptr
> +      && has_static_range (TYPE_RANGE_DATA (range_type))
> +      && (!type_not_associated (type)
> +	  && !type_not_allocated (type)))
> +    {
> +      LONGEST low_bound, high_bound;
> +      int stride;
> +      struct type *element_type;
> +
> +      /* If the array itself doesn't provide a stride value then take
> +	 whatever stride the range provides.  Don't update BIT_STRIDE as
> +	 we don't want to place the stride value from the range into this
> +	 arrays bit size field.  */
> +      stride = TYPE_FIELD_BITSIZE (type, 0);
> +      if (stride == 0)
> +	stride = TYPE_BIT_STRIDE (range_type);
> +
> +      if (get_discrete_bounds (range_type, &low_bound, &high_bound) < 0)
> +	low_bound = high_bound = 0;
> +      element_type = check_typedef (TYPE_TARGET_TYPE (type));
> +      /* Be careful when setting the array length.  Ada arrays can be
> +	 empty arrays with the high_bound being smaller than the low_bound.
> +	 In such cases, the array length should be zero.  */
> +      if (high_bound < low_bound)
> +	TYPE_LENGTH (type) = 0;
> +      else if (stride != 0)
> +	{
> +	  /* Ensure that the type length is always positive, even in the
> +	     case where (for example in Fortran) we have a negative
> +	     stride.  It is possible to have a single element array with a
> +	     negative stride in Fortran (this doesn't mean anything
> +	     special, it's still just a single element array) so do
> +	     consider that case when touching this code.  */
> +	  LONGEST element_count = std::abs (high_bound - low_bound + 1);
> +	  TYPE_LENGTH (type)
> +	    = ((std::abs (stride) * element_count) + 7) / 8;
> +	}
> +      else
> +	TYPE_LENGTH (type) =
> +	  TYPE_LENGTH (element_type) * (high_bound - low_bound + 1);
> +
> +      return true;
> +    }
> +
> +  return false;
> +}
> +
>  /* Create an array type using either a blank type supplied in
>     RESULT_TYPE, or creating a new type, inheriting the objfile from
>     RANGE_TYPE.
> @@ -1222,47 +1278,17 @@ create_array_type_with_stride (struct type *result_type,
>  
>    TYPE_CODE (result_type) = TYPE_CODE_ARRAY;
>    TYPE_TARGET_TYPE (result_type) = element_type;
> -  if (byte_stride_prop == NULL
> -      && has_static_range (TYPE_RANGE_DATA (range_type))
> -      && (!type_not_associated (result_type)
> -	  && !type_not_allocated (result_type)))
> -    {
> -      LONGEST low_bound, high_bound;
> -      int stride;
>  
> -      /* If the array itself doesn't provide a stride value then take
> -	 whatever stride the range provides.  Don't update BIT_STRIDE as
> -	 we don't want to place the stride value from the range into this
> -	 arrays bit size field.  */
> -      stride = bit_stride;
> -      if (stride == 0)
> -	stride = TYPE_BIT_STRIDE (range_type);
> +  TYPE_NFIELDS (result_type) = 1;
> +  TYPE_FIELDS (result_type) =
> +    (struct field *) TYPE_ZALLOC (result_type, sizeof (struct field));
> +  TYPE_INDEX_TYPE (result_type) = range_type;
> +  if (byte_stride_prop != NULL)
> +    add_dyn_prop (DYN_PROP_BYTE_STRIDE, *byte_stride_prop, result_type);
> +  else if (bit_stride > 0)
> +    TYPE_FIELD_BITSIZE (result_type, 0) = bit_stride;
>  
> -      if (get_discrete_bounds (range_type, &low_bound, &high_bound) < 0)
> -	low_bound = high_bound = 0;
> -      element_type = check_typedef (element_type);
> -      /* Be careful when setting the array length.  Ada arrays can be
> -	 empty arrays with the high_bound being smaller than the low_bound.
> -	 In such cases, the array length should be zero.  */
> -      if (high_bound < low_bound)
> -	TYPE_LENGTH (result_type) = 0;
> -      else if (stride != 0)
> -	{
> -	  /* Ensure that the type length is always positive, even in the
> -	     case where (for example in Fortran) we have a negative
> -	     stride.  It is possible to have a single element array with a
> -	     negative stride in Fortran (this doesn't mean anything
> -	     special, it's still just a single element array) so do
> -	     consider that case when touching this code.  */
> -	  LONGEST element_count = std::abs (high_bound - low_bound + 1);
> -	  TYPE_LENGTH (result_type)
> -	    = ((std::abs (stride) * element_count) + 7) / 8;
> -	}
> -      else
> -	TYPE_LENGTH (result_type) =
> -	  TYPE_LENGTH (element_type) * (high_bound - low_bound + 1);
> -    }
> -  else
> +  if (!calculate_static_array_size (result_type))
>      {
>        /* This type is dynamic and its length needs to be computed
>           on demand.  In the meantime, avoid leaving the TYPE_LENGTH
> @@ -1273,15 +1299,6 @@ create_array_type_with_stride (struct type *result_type,
>        TYPE_LENGTH (result_type) = 0;
>      }
>  
> -  TYPE_NFIELDS (result_type) = 1;
> -  TYPE_FIELDS (result_type) =
> -    (struct field *) TYPE_ZALLOC (result_type, sizeof (struct field));
> -  TYPE_INDEX_TYPE (result_type) = range_type;
> -  if (byte_stride_prop != NULL)
> -    add_dyn_prop (DYN_PROP_BYTE_STRIDE, *byte_stride_prop, result_type);
> -  else if (bit_stride > 0)
> -    TYPE_FIELD_BITSIZE (result_type, 0) = bit_stride;
> -
>    /* TYPE_TARGET_STUB will take care of zero length arrays.  */
>    if (TYPE_LENGTH (result_type) == 0)
>      TYPE_TARGET_STUB (result_type) = 1;
> @@ -2873,20 +2890,9 @@ check_typedef (struct type *type)
>  	  TYPE_LENGTH (type) = TYPE_LENGTH (target_type);
>  	  TYPE_TARGET_STUB (type) = 0;
>  	}
> -      else if (TYPE_CODE (type) == TYPE_CODE_ARRAY)
> -	{
> -	  struct type *range_type = check_typedef (TYPE_INDEX_TYPE (type));
> -	  if (has_static_range (TYPE_RANGE_DATA (range_type)))
> -	    {
> -	      ULONGEST len = 0;
> -	      LONGEST low_bound = TYPE_LOW_BOUND (range_type);
> -	      LONGEST high_bound = TYPE_HIGH_BOUND (range_type);
> -	      if (high_bound >= low_bound)
> -		len = (high_bound - low_bound + 1) * TYPE_LENGTH (target_type);
> -	      TYPE_LENGTH (type) = len;
> -	      TYPE_TARGET_STUB (type) = 0;
> -	    }
> -	}
> +      else if (TYPE_CODE (type) == TYPE_CODE_ARRAY
> +	       && calculate_static_array_size (type))
> +	TYPE_TARGET_STUB (type) = 0;
>      }
>  
>    type = make_qualified_type (type, instance_flags, NULL);
> -- 
> 2.26.2
> 

Re: [PATCH] Fix size recalculation of fortran arrays

[Hannes Domani]

 Am Freitag, 1. Mai 2020, 15:11:40 MESZ hat Andrew Burgess <andrew.burgess@embecosm.com> Folgendes geschrieben:

> * Hannes Domani via Gdb-patches <gdb-patches@sourceware.org> [2020-05-01 14:35:18 +0200]:
>
> > My recent change regarding size calculation of arrays of stubbed types
> > didn't take array strides and associated/allocated type properties into
> > account, which basically broke fortran arrays.
> >
> > Fixed by refactoring the array size calculation of
> > create_array_type_with_stride into a new function, and also use it for
> > the stubbed array size recalculation.
> >
> > gdb/ChangeLog:
> >
> > 2020-05-01  Hannes Domani  <ssbssa@yahoo.de>
> >
> >     * gdbtypes.c (calculate_static_array_size): New function.
> >     (create_array_type_with_stride): Use calculate_static_array_size.
> >     (check_typedef): Likewise.
> > ---
> >  gdb/gdbtypes.c | 130 ++++++++++++++++++++++++++-----------------------
> >  1 file changed, 68 insertions(+), 62 deletions(-)
> >
> > diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
> > index 6648dc4d67..ad5c7ee0af 100644
> > --- a/gdb/gdbtypes.c
> > +++ b/gdb/gdbtypes.c
> > @@ -1177,6 +1177,62 @@ discrete_position (struct type *type, LONGEST val, LONGEST *pos)
> >      }
> >  }
> >
> > +/* If the array has static bounds, calculate its size.  */
> > +
> > +static bool
> > +calculate_static_array_size (struct type *type)
>
> Could you rename this to something like update_static_array_size -
> calculate to me implies something will be calculated and returned.
> Also please update the header comment to reference TYPE, and describe
> the return values, something like:
>
>   /* If the array TYPE has static bounds calculate and update its
>     size, then return true.  Otherwise return false and leave TYPE
>     unchanged.  */
>
> With those changes this looks good.

Pushed with those changes, thanks.


> Thanks for the quick fix.

I'm very sorry for the breakage, but running the testsuite on Windows can
be very frustrating at times.
Maybe I'm doing something wrong, but each test only has ~66% chance of even
staring correctly.


Hannes

Re: [PATCH] Fix size recalculation of fortran arrays

[Andrew Burgess]

* Hannes Domani via Gdb-patches <gdb-patches@sourceware.org> [2020-05-01 13:46:11 +0000]:

>  Am Freitag, 1. Mai 2020, 15:11:40 MESZ hat Andrew Burgess <andrew.burgess@embecosm.com> Folgendes geschrieben:
> 
> > * Hannes Domani via Gdb-patches <gdb-patches@sourceware.org> [2020-05-01 14:35:18 +0200]:
> >
> > > My recent change regarding size calculation of arrays of stubbed types
> > > didn't take array strides and associated/allocated type properties into
> > > account, which basically broke fortran arrays.
> > >
> > > Fixed by refactoring the array size calculation of
> > > create_array_type_with_stride into a new function, and also use it for
> > > the stubbed array size recalculation.
> > >
> > > gdb/ChangeLog:
> > >
> > > 2020-05-01  Hannes Domani  <ssbssa@yahoo.de>
> > >
> > >     * gdbtypes.c (calculate_static_array_size): New function.
> > >     (create_array_type_with_stride): Use calculate_static_array_size.
> > >     (check_typedef): Likewise.
> > > ---
> > >  gdb/gdbtypes.c | 130 ++++++++++++++++++++++++++-----------------------
> > >  1 file changed, 68 insertions(+), 62 deletions(-)
> > >
> > > diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
> > > index 6648dc4d67..ad5c7ee0af 100644
> > > --- a/gdb/gdbtypes.c
> > > +++ b/gdb/gdbtypes.c
> > > @@ -1177,6 +1177,62 @@ discrete_position (struct type *type, LONGEST val, LONGEST *pos)
> > >      }
> > >  }
> > >
> > > +/* If the array has static bounds, calculate its size.  */
> > > +
> > > +static bool
> > > +calculate_static_array_size (struct type *type)
> >
> > Could you rename this to something like update_static_array_size -
> > calculate to me implies something will be calculated and returned.
> > Also please update the header comment to reference TYPE, and describe
> > the return values, something like:
> >
> >   /* If the array TYPE has static bounds calculate and update its
> >     size, then return true.  Otherwise return false and leave TYPE
> >     unchanged.  */
> >
> > With those changes this looks good.
> 
> Pushed with those changes, thanks.
> 
> 
> > Thanks for the quick fix.
> 
> I'm very sorry for the breakage, but running the testsuite on Windows can
> be very frustrating at times.
> Maybe I'm doing something wrong, but each test only has ~66% chance of even
> staring correctly.

I feel your pain.  Any time I have to do anything on Windows I know
I'm going to have a bad day.

It can be worth mentioning in your mailing list post if you are
worried that your testing might not have gone well, most reviewers
will be happy to apply a patch locally and do a test run to double
check.

Thanks,
Andrew