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

[Joel Brobecker <brobecker at adacore dot com>]

On Fri, Jan 22, 2016 at 08:22:53AM +0100, Keven Boell wrote:
> On 20.01.2016 11:18, Yao Qi wrote:
> > Keven Boell <keven.boell@linux.intel.com> writes:
> > 
> >> Fortran provide types whose values may be dynamically allocated
> >> or associated with a variable under explicit program control.
> >> The purpose of this commit is
> >>   * to read allocated/associated DWARF tags and store them in
> >>     the dynamic property list of main_type.
> >>   * enable GDB to print the value of a dynamic array in Fortran
> >>     in case the type is allocated or associated (pointer to
> >>     dynamic array).
> >>
> >> Examples:
> >> (gdb) p vla_not_allocated
> >> $1 = <not allocated>
> >>
> >> (gdb) p vla_allocated
> >> $1 = (1, 2, 3)
> >>
> >> (gdb) p vla_ptr_not_associated
> >> $1 = <not associated>
> >>
> >> (gdb) p vla_ptr_associated
> >> $1 = (1, 2, 3)
> >>
> >> Add basic test coverage for most dynamic array use-cases
> >> in Fortran.
> >> The commit contains the following tests:
> >>   * Ensure that values of Fortran dynamic arrays
> >>     can be evaluated correctly in various ways and states.
> >>   * Ensure that Fortran primitives can be evaluated
> >>     correctly when used as a dynamic array.
> >>   * Dynamic arrays passed to subroutines and handled
> >>     in different ways inside the routine.
> >>   * Ensure that the ptype of dynamic arrays in
> >>     Fortran can be printed in GDB correctly.
> >>   * Ensure that dynamic arrays in different states
> >>     (allocated/associated) can be evaluated.
> >>   * Dynamic arrays passed to functions and returned from
> >>     functions.
> >>   * History values of dynamic arrays can be accessed and
> >>     printed again with the correct values.
> >>   * Dynamic array evaluations using MI protocol.
> >>   * Sizeof output of dynamic arrays in various states.
> >>
> >> The patch was tested using the test suite on Ubuntu 12.04 64bit.
> > 
> > Hi Keven,
> > The test cases added by this commit fail on some other OS and targets,
> > see this thread, https://sourceware.org/ml/gdb-testers/2015-q4/msg02136.html
> > can you take a look?
> > 
> 
> Hi Yao, Joel,
> 
> I don't think I will be able to fix the failures on the mentioned
> hosts/targets before you create the branch, as I need to replicate the
> environment on my end first to start investigating.  Therefore I
> suggest to revert the change for now. Sorry if this caused any
> inconvenience.

I don't think that reverting without more investigation is a good
idea. I quickly looked at a good dozen of those reports, and, as
far as I know, these are new FAILs, so we don't know that they are
regressions. I could also be compiler issues.

There is also the fact that reviewing these patches took me not
just hours, but actually days in total (I don't know how much time
Keven et al also spent answering all my comments). I would prefer
re-doing all that work only if we have confirmation that this is
causing a critical problem that we can't fix without Keven's help.

That being said - it doesn't bode well for the future of this
feature if the authors don't have time to look into issues they
create...

Thanks,
-- 
Joel