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

[Andrew Burgess <andrew.burgess@embecosm.com>]

* Simon Marchi <simon.marchi@polymtl.ca> [2020-12-12 01:33:21 -0500]:

> 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

Thanks for bringing this to my attention.  I'll take a look at this
next week.

Andrew


> 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)
> >