Re: [PUSHED] gdb/fortran: Support negative array stride in one limited case

[Simon Marchi via Gdb-patches <gdb-patches@sourceware.org>]

Hi Andrew,

Sorry to revive this thread.

I build my GDB with ASan, and I see the gdb.fortran/vla-type.exp test crashing GDB with:

ptype fivedynarr(2)
/home/smarchi/src/binutils-gdb/gdb/valarith.c:1171:10: runtime error: signed integer overflow: 16777216 * 140737351048816 cannot be represented in type 'long int'

Commit 5bbd8269fa8d ("gdb/fortran: array stride support") is the first one where it does
this.

Simon

On 2020-02-25 11:35 a.m., Andrew Burgess wrote:
> I've pushed this fix now.  The exact patch I pushed is included below.
> 
> 
> Thanks,
> Andrew
> 
> 
> 
> 
> ---
> 
> This commit adds support for negative Fortran array strides in one
> limited case, that is the case of a single element array with a
> negative array stride.
> 
> The changes in this commit will be required in order for more general
> negative array stride support to work correctly, however, right now
> other problems in GDB prevent negative array strides from working in
> the general case.
> 
> The reason negative array strides don't currently work in the general
> case is that when dealing with such arrays, the base address for the
> objects data is actually the highest addressed element, subsequent
> elements are then accessed with a negative offset from that address,
> and GDB is not currently happy with this configuration.
> 
> The changes here can be summarised as, stop treating signed values as
> unsigned, specifically, the array stride, and offsets calculated using
> the array stride.
> 
> This issue was identified on the mailing list by Sergio:
> 
>   https://sourceware.org/ml/gdb-patches/2020-01/msg00360.html
> 
> The test for this issue is a new one written by me as the copyright
> status of the original test is currently unknown.
> 
> gdb/ChangeLog:
> 
> 	* gdbtypes.c (create_array_type_with_stride): Handle negative
> 	array strides.
> 	* valarith.c (value_subscripted_rvalue): Likewise.
> 
> gdb/testsuite/ChangeLog:
> 
> 	* gdb.fortran/derived-type-striding.exp: Add a new test.
> 	* gdb.fortran/derived-type-striding.f90: Add pointer variable for
> 	new test.
> ---
>  gdb/ChangeLog                                       |  6 ++++++
>  gdb/gdbtypes.c                                      | 17 +++++++++++++----
>  gdb/testsuite/ChangeLog                             |  6 ++++++
>  gdb/testsuite/gdb.fortran/derived-type-striding.exp |  2 ++
>  gdb/testsuite/gdb.fortran/derived-type-striding.f90 |  2 ++
>  gdb/valarith.c                                      |  4 ++--
>  6 files changed, 31 insertions(+), 6 deletions(-)
> 
> diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
> index 85758930491..ef110b30445 100644
> --- a/gdb/gdbtypes.c
> +++ b/gdb/gdbtypes.c
> @@ -1223,7 +1223,7 @@ create_array_type_with_stride (struct type *result_type,
>  	  && !type_not_allocated (result_type)))
>      {
>        LONGEST low_bound, high_bound;
> -      unsigned int stride;
> +      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
> @@ -1241,9 +1241,18 @@ create_array_type_with_stride (struct type *result_type,
>  	 In such cases, the array length should be zero.  */
>        if (high_bound < low_bound)
>  	TYPE_LENGTH (result_type) = 0;
> -      else if (stride > 0)
> -	TYPE_LENGTH (result_type) =
> -	  (stride * (high_bound - low_bound + 1) + 7) / 8;
> +      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 = abs (high_bound - low_bound + 1);
> +	  TYPE_LENGTH (result_type)
> +	    = ((abs (stride) * element_count) + 7) / 8;
> +	}
>        else
>  	TYPE_LENGTH (result_type) =
>  	  TYPE_LENGTH (element_type) * (high_bound - low_bound + 1);
> diff --git a/gdb/testsuite/gdb.fortran/derived-type-striding.exp b/gdb/testsuite/gdb.fortran/derived-type-striding.exp
> index 094843ca8b1..639dc4c9528 100644
> --- a/gdb/testsuite/gdb.fortran/derived-type-striding.exp
> +++ b/gdb/testsuite/gdb.fortran/derived-type-striding.exp
> @@ -41,3 +41,5 @@ gdb_test "p point_dimension" "= \\\(2, 2, 2, 2, 2, 2, 2, 2, 2\\\)"
>  # Test mixed type derived type.
>  if { $gcc_with_broken_stride } { setup_kfail *-*-* gcc/92775 }
>  gdb_test "p point_mixed_dimension" "= \\\(3, 3, 3, 3\\\)"
> +
> +gdb_test "p cloud_slice" " = \\\(\\\( x = 1, y = 2, z = 3 \\\)\\\)"
> diff --git a/gdb/testsuite/gdb.fortran/derived-type-striding.f90 b/gdb/testsuite/gdb.fortran/derived-type-striding.f90
> index 26829f51dc0..fb537579faa 100644
> --- a/gdb/testsuite/gdb.fortran/derived-type-striding.f90
> +++ b/gdb/testsuite/gdb.fortran/derived-type-striding.f90
> @@ -28,9 +28,11 @@ program derived_type_member_stride
>      type(mixed_cartesian), dimension(10), target :: mixed_cloud
>      integer(kind=8), dimension(:), pointer :: point_dimension => null()
>      integer(kind=8), dimension(:), pointer :: point_mixed_dimension => null()
> +    type(cartesian), dimension(:), pointer :: cloud_slice => null()
>      cloud(:)%x = 1
>      cloud(:)%y = 2
>      cloud(:)%z = 3
> +    cloud_slice => cloud(3:2:-2)
>      point_dimension => cloud(1:9)%y
>      mixed_cloud(:)%x = 1
>      mixed_cloud(:)%y = 2
> diff --git a/gdb/valarith.c b/gdb/valarith.c
> index 79b148602bb..be0e0731bee 100644
> --- a/gdb/valarith.c
> +++ b/gdb/valarith.c
> @@ -187,7 +187,7 @@ value_subscripted_rvalue (struct value *array, LONGEST index, LONGEST lowerbound
>  {
>    struct type *array_type = check_typedef (value_type (array));
>    struct type *elt_type = check_typedef (TYPE_TARGET_TYPE (array_type));
> -  ULONGEST elt_size = type_length_units (elt_type);
> +  LONGEST elt_size = type_length_units (elt_type);
>  
>    /* Fetch the bit stride and convert it to a byte stride, assuming 8 bits
>       in a byte.  */
> @@ -199,7 +199,7 @@ value_subscripted_rvalue (struct value *array, LONGEST index, LONGEST lowerbound
>        elt_size = stride / (unit_size * 8);
>      }
>  
> -  ULONGEST elt_offs = elt_size * (index - lowerbound);
> +  LONGEST elt_offs = elt_size * (index - lowerbound);
>  
>    if (index < lowerbound
>        || (!TYPE_ARRAY_UPPER_BOUND_IS_UNDEFINED (array_type)
>