Re: [PATCH 1/6] fortran: allow multi-dimensional subarrays

[Joel Brobecker <brobecker at adacore dot com>]

Hello Christoph,

On Tue, Dec 01, 2015 at 02:21:10PM +0100, christoph.t.weinmann@intel.com wrote:
> From: Christoph Weinmann <christoph.t.weinmann@intel.com>
> 
> Add an argument count for subrange expressions in Fortran.
> Based on the counted value calculate a new array with the
> elements specified by the user.  First parse the user input,
> secondly copy the desired array values into the return
> array, thirdly re-create the necessary ranges and bounds.
> 
> 1|  program prog
> 2|    integer :: ary(10,5) = (/ (i,i=1,10) (j, j=1,5) /)
> 3|  end program prog
> 
> (gdb) print ary(2:4,1:3)
> old> Syntax error in expression near ':3'
> new> $3 = ( ( 21, 31, 41) ( 22, 32, 42) ( 23, 33, 43) )
> 
> 2013-11-25  Christoph Weinmann  <christoph.t.weinmann@intel.com>
> 
> 	* eval.c (multi_f77_subscript): Remove function.
> 	* eval.c (evaluate_subrange_expr): When evaluating
> 	an array or string expression, call
> 	value_f90_subarray.
> 	* eval.c (value_f90_subarray): Add argument parsing
> 	and compute result array based on user input.
> 	* f-exp.y: Increment argument counter for every subrange
> 	expression entered by the user.
> 	* valops.c (value_slice): Call value_slice_1 with
> 	additional default argument.
> 	* valops.c (value_slice_1): Add functionality to
> 	copy and return result values based on input.
> 	* value.h: Add function definition.

Sorry for the delay in sending feedback; these kinds of patches
are difficult for me to follow because I don't know Fortran at
all, and also because they touch the core part of GDB, so we
always have to be very very careful. That's why I can only look
at these when I know I have several consecutive hours available,
which, at the moment, means during the weekends.

Just a procedural note on ChangeLog entries: Citing the filename
only once is sufficient. For instance, you should write the above
changes for eval.c as follow:

        * eval.c (multi_f77_subscript): Remove function.
        (evaluate_subrange_expr): When evaluating an array or string
        expression, call value_f90_subarray.
        (value_f90_subarray): Add argument parsing and compute result
        array based on user input.

Please take a look at:
http://www.gnu.org/prep/standards/standards.html#Style-of-Change-Logs

Also, the ChangeLog is probably stale, because it doesn't always
match the patch (no evaluate_subrange_expr, for instance).

On the technical side:

First of all, it took me a little bit of research and then a while
to wrap my head around multidimensional arrays, and in particular
what a slice of a multidimensional array might look like when
represented in memory. That's when I realized that you cannot
take such a slice without changing the representation of the array.
Let me explain: the array you're taking a slice of is laid out
in memory as one continuous buffer, with the elements ordered
in column-major order. A slice of a multi-dimensional array is
going to be bits and pieces of that memory chunk, not one continous
chunk of the region containing the whole array.

So, my first question is: what's you plan, representation wise,
when creating the type of the slice? I couldn't really figure it
out from the patch itself and regardless, I think it is going to
be useful to have that info in the code as a comment.

One way I thought about is to create a new array as a not_lval. In other
words, after determining the size of the slice, create a not_lval value,
and store the value of each element inside the not_lval value's buffer.
The downside is that the size of the slice can be very large, and thus
consume lots of memory. The other downside is that you'll no longer be
able to take the address of an element in that slice.

Another way I thought was to manipulate the type from being
an array of arrays to being an array of references to arrays
to references to arrays, etc. But I'm not actually certain
this is going to work with more than 2 dimensions, because
the reference to the first dimension isn't going to be stored
anywhere, so you won't be able to get the reference to that reference.

One important note: array ordering. I think we don't have to
worry about that in the core code, because the theory is that
row-ordering is taken into account during the array's type
struct creation (see dwarf2read.c::read_array_type). So, your
patch is good in that respect.

Please also extract from your last patch (patch #6 providing
a testcasee) the parts that test the features implemented by
this patch, and combine those with this commit, so that anyone
looking at this commit can tell the kind of features that
this patch brings.

And finally, I think and I fervently hope that the answers to
the questions above will allow us to avoid a change in valops.c.
I think we can do everything directly in value_f90_subarray.

So, here's what I suggest in terms of a plan of attack:

  1. Focus on getting the answers to the high-level questions
     I ask above (how is the array slice constructed inside
     GDB in terms of struct type and struct value layout);

  2. Start discussing the implementation based on the answer
     to (1)

  3. Focus on patch #1 exclusively for now, trying to get
     something in as soon as we can. The other patches seem
     easier to manage and will likely get accepted quicker
     once we get #1 in.

-- 
Joel