Bug 36091 - false positive in bounds checking with forall
Summary: false positive in bounds checking with forall
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: fortran (show other bugs)
Version: 4.4.0
: P3 normal
Target Milestone: ---
Assignee: Paul Thomas
URL:
Keywords: rejects-valid
Depends on:
Blocks: 32834
  Show dependency treegraph
 
Reported: 2008-04-30 20:15 UTC by Thomas Koenig
Modified: 2009-04-06 10:52 UTC (History)
1 user (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail: 4.4.0
Last reconfirmed: 2009-03-17 08:31:49


Attachments
patch solving the problem (3.51 KB, patch)
2008-10-22 12:49 UTC, Mikael Morin
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Thomas Koenig 2008-04-30 20:15:41 UTC
Reduced test case from forall_13.f90:

$ cat f1.f90 
  integer :: p(4)

  p = (/3,1,4,2/)

  forall (i = 1:4) p(5 - p(i)) = p(5 - i)
  if (any (p .ne. (/1,2,3,4/))) call abort ()
end
$ gfortran -fbounds-check f1.f90 
$ ./a.out
At line 5 of file f1.f90
Fortran runtime error: Array reference out of bounds for array 'p', upper bound of dimension 1 exceeded (4 > 3)
$ gfortran -v
Using built-in specs.
Target: i686-pc-linux-gnu
Configured with: ../../gcc/trunk/configure --prefix=/home/ig25 --enable-maintainer-mode --enable-languages=c,fortran
Thread model: posix
gcc version 4.4.0 20080420 (experimental) (GCC)
Comment 1 Mikael Morin 2008-10-22 12:49:54 UTC
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
Comment 2 Mikael Morin 2008-10-22 12:52:03 UTC
I forgot to say that it is regression tested on x86_64-unknown-linux-gnu
Comment 3 Dominique d'Humieres 2008-10-22 14:24:30 UTC
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

Comment 4 Mikael Morin 2008-10-23 14:37:40 UTC
(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.  
Comment 5 Dominique d'Humieres 2008-10-23 20:34:57 UTC
> 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.
Comment 6 Dominique d'Humieres 2008-10-27 19:24:01 UTC
For the record, the patch in comment #1 conflicts with the patch in http://gcc.gnu.org/ml/fortran/2008-10/msg00256.html.
Comment 7 Paul Thomas 2009-03-17 08:31:49 UTC
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 
Comment 8 Paul Thomas 2009-04-06 05:26:18 UTC
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

Comment 9 Paul Thomas 2009-04-06 10:52:13 UTC
Fixed on trunk.  I am prepared to backport but the mood appears to be against it.

Thanks for the report.

Paul