Bug 36091

Created attachment 16527 [details]
patch solving the problem

This patch keeps the original array descriptor from gfc_conv_subref_array_arg to gfc_trans_create_temp_array so that the temporary generated can have the same bounds as the original, which is needed for bounds checking. 

However I'm wondering if this is the good way to solve the problem. 

I believe that the code generated for this:

forall (i=j:k) p(p(i)) = 1  ! bounds checking problem

should not be much different than the code generated for that:

p(p(j:k)) = 1  ! no bounds checking problem

I forgot to say that it is regression tested on x86_64-unknown-linux-gnu

I am not 100% sure that the following is due to the patch in comment #1, but the following code segfaults (at the write) after having applied it:

  integer :: i(1) = 1
  integer :: foo(3)
  foo = 17
  print *, i, foo
  write(*,*) foo([1]), foo([1]+i), [1]+1
end

(In reply to comment #3)
> I am not 100% sure that the following is due to the patch in comment #1, 
There is already something wrong on trunk, but I agree that the patch makes it worse. As a side note I'm really impressed by how fast you found a not-working case to my slowly and laboriously prepared code. 

I've been playing a bit with the testcase on trunk.

This:
  write(*,*)  foo([1]+i)

produces this code fragment: 
      integer(kind=8) A.4[2];
      struct array1_integer(kind=8) atmp.3;

      atmp.3.dtype = 521;
      atmp.3.dim[0].stride = 1;
      atmp.3.dim[0].lbound = 0;
      atmp.3.dim[0].ubound = 1;
      atmp.3.data = (void *) &A.4;
      atmp.3.offset = 0;


while this:
  write(*,*)  foo(i+[1])

produces this code fragment:
      integer(kind=8) A.4[1];
      struct array1_integer(kind=8) atmp.3;


      atmp.3.dtype = 521;
      atmp.3.dim[0].stride = 1;
      atmp.3.dim[0].lbound = 0;
      atmp.3.dim[0].ubound = 0;
      atmp.3.data = (void *) &A.4;
      atmp.3.offset = 0;



As you can see, the first case's array temporary has a size of 2, while it should be of size 1.
The code however is correct as only the first element of the array is used. 

With the patch, it is... wrong.
To be continued.  

> As a side note I'm really impressed by how fast you found a not-working
> case to my slowly and laboriously prepared code. 

Don't worry, it is always much easier to find (others') bugs than to fix them.
The problem was detected by my own quick and dirty test suite which cover
some cases not covered by the gfortran one.

For the record, the patch in comment #1 conflicts with the patch in http://gcc.gnu.org/ml/fortran/2008-10/msg00256.html.

This fixes the PR:

Index: gcc/fortran/trans-stmt.c
===================================================================
--- gcc/fortran/trans-stmt.c    (revision 144197)
+++ gcc/fortran/trans-stmt.c    (working copy)
@@ -1713,6 +1719,7 @@
 
   /* Use the temporary as the backend_decl.  */
   new_sym->backend_decl = tse.expr;
+  DECL_ARTIFICIAL (new_sym->backend_decl) = 1;
 
   /* Create a fake symtree for it.  */
   root = NULL;
Index: gcc/fortran/trans-array.c
===================================================================
--- gcc/fortran/trans-array.c   (revision 144197)
+++ gcc/fortran/trans-array.c   (working copy)
@@ -2469,7 +2469,7 @@
       gfc_conv_expr_type (&indexse, ar->start[n], gfc_array_index_type);
       gfc_add_block_to_block (&se->pre, &indexse.pre);
 
-      if (flag_bounds_check)
+      if (flag_bounds_check && !DECL_ARTIFICIAL (se->expr))
        {
          /* Check array bounds.  */
          tree cond;

I am sure that this will cause regressions with array dummy arguments but this is not intended to do anything other than demonstrate the need to mark the temporaries produced in FORALL blocks.  These masquerade as symbols.  I suspect that they should gain a temporary attribute or, alternatively, the condition above should be a bit more complex.

Paul 

Subject: Bug 36091

Author: pault
Date: Mon Apr  6 05:25:46 2009
New Revision: 145581

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=145581
Log:
2009-04-06  Paul Thomas  <pault@gcc.gnu.org>

        PR fortran/36091
        * trans-array.c (gfc_conv_array_ref): If the symbol has the
	temporary attribute use the array_spec for the bounds.
	* gfortran.h : Add the temporary field to the structure
	'symbol_attribute'.
	* trans-stmt.c (forall_make_variable_temp): Set the symbol's
	temporary attribute.

2009-04-06  Paul Thomas  <pault@gcc.gnu.org

        PR fortran/36091
        * gfortran.dg/forall_13.f90: Add -fbounds-check option.


Modified:
    trunk/gcc/fortran/ChangeLog
    trunk/gcc/fortran/gfortran.h
    trunk/gcc/fortran/trans-array.c
    trunk/gcc/fortran/trans-stmt.c
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/gfortran.dg/forall_13.f90

Fixed on trunk.  I am prepared to backport but the mood appears to be against it.

Thanks for the report.

Paul