Bug 41850 - Wrong-code with optional allocatable arrays
Summary: Wrong-code with optional allocatable arrays
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: fortran (show other bugs)
Version: 4.5.0
: P3 normal
Target Milestone: ---
Assignee: Tobias Burnus
URL: http://gcc.gnu.org/ml/fortran/2009-10...
Keywords: wrong-code
Depends on: 41907
Blocks:
  Show dependency treegraph
 
Reported: 2009-10-27 19:42 UTC by Tobias Burnus
Modified: 2009-11-02 17:45 UTC (History)
1 user (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2009-10-29 22:52:37


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Tobias Burnus 2009-10-27 19:42:43 UTC
As reported by Thomas Robitaille at
http://gcc.gnu.org/ml/fortran/2009-10/msg00220.html
the following code gives wrong-code (segfault/buserror) at run time with GCC 4.2/4.3/4.4/4.5. (4.1 does not support allocatable dummies.)

$ ./a.out
 in sub1
Bus error


module test_module
  implicit none
contains
  subroutine sub2(a)
    implicit none
    real,allocatable,intent(out),optional :: a(:)
    print *,'in sub2'
  end subroutine sub2
  subroutine sub1(a)
    implicit none
    real,allocatable,intent(out),optional :: a(:)
    print *,'in sub1'
    call sub2(a)
  end subroutine sub1
end module test_module

program test
  use test_module
  implicit none
  call sub1()
end program


The problem is that the argument can be "NULL" - and there is no check for the case "a == NULL"; i.e. the INTENT(OUT) autodeallocation block needs to be enclosed with a "if (a !=NULL)" as remarked by Dennis (http://gcc.gnu.org/ml/fortran/2009-10/msg00221.html).

sub1 (a)
{
  [...snip printing "in sub1" stuff...]
  {
    struct array1_real(kind=4) * D.555;
    if (a->data != 0B)
      {
        __builtin_free (a->data);
      }
    a->data = 0B;
    D.555 = a != 0B ? a : 0B;
    sub2 (D.555);
  }

By the way the line "D.555 = a != 0B ? a : 0B;" is redundant - and also not nice because making the alias analysis for the middle end more difficult.
Comment 1 Tobias Burnus 2009-10-29 21:59:12 UTC
Note: One also needs to ensure that this works with allocatable scalars, cf. PR 41872.
Comment 2 Tobias Burnus 2009-10-29 22:52:37 UTC
Preliminary patch. The for the second part of the patch one needs still to update the comment.  (Currently regtesting, so far no failure.)

...

Actually, I think the block (second part) can also go away for fsym == NULL. In all cases, one has
   tmp = (a == NULL) ? a : NULL;
which is really a noop. I fail to see how one can get anything else. One needs such a check for for absent arguments, but that is already handled in interface.c (or somewhere around that place) - and it is a compile-time replacement.

Index: trans-expr.c
===================================================================
--- trans-expr.c        (Revision 153727)
+++ trans-expr.c
@@ -2943,6 +2943,12 @@ gfc_conv_procedure_call (gfc_se * se, gf
                   tmp = build_fold_indirect_ref_loc (input_location,
                                                 parmse.expr);
                   tmp = gfc_trans_dealloc_allocated (tmp);
+                 if (fsym->attr.optional
+                     && e->expr_type == EXPR_VARIABLE
+                     && e->symtree->n.sym->attr.optional)
+                   tmp = fold_build3 (COND_EXPR, void_type_node,
+                                    gfc_conv_expr_present (e->symtree->n.sym),
+                                      tmp, build_empty_stmt (input_location));
                   gfc_add_expr_to_block (&se->pre, tmp);
                 }

@@ -2954,7 +2960,7 @@ gfc_conv_procedure_call (gfc_se * se, gf
         an intrinsic subroutine, however, fsym is NULL, but we might still
         have an optional argument, so we proceed to the substitution
         just in case.  */
-      if (e && (fsym == NULL || fsym->attr.optional))
+      if (e && fsym == NULL)
        {
          /* If an optional argument is itself an optional dummy argument,
             check its presence and substitute a null if absent.  */
Comment 3 Tobias Burnus 2009-10-29 23:09:06 UTC
Hmm, I get regtest failures for elemental procedures:

  gfortran.dg/bounds_check_9.f90
  gfortran.dg/bounds_check_fail_2.f90

- D.1415 = ivec != 0B ? &(*ivec.0)[(S.10 + 1) * D.1413 + D.1408] : 0B;
- set_optional (&ivec_[S.10], &D.1414, D.1415);
+ set_optional (&ivec_[S.10], &D.1414, &(*ivec.0)[(S.10 + 1) * D.1413+D.1408]);

Dummy:
  elemental subroutine set_optional(i,idef,iopt)
    integer, intent(in), optional :: iopt
Actual:
    integer, intent(in), optional :: ivec(:)
    call set_optional(ivec_,(/1,2/))
    call set_optional(ivec_,(/1,2/),ivec)

Thus, the problem is that one passes an array to a scalar in form of an elemental procedure; seemingly, one needs to take care of this special case.

Anything else?
Comment 4 Tobias Burnus 2009-10-30 10:16:07 UTC
(In reply to comment #3)
> Anything else?

Seemingly yes:
  gfortran.dg/optional_dim_3.f90
  gfortran.dg/random_4.f90
  gfortran.dg/random_7.f90

For optional_dim_3.f90, one has:
-         D.1516 = n2 != 0B ? (integer(kind=4)) *n2 : 1;
-         _gfortran_cshift1_4 (&atmp.38, &parm.34, &atmp.35, &D.1516);
+         _gfortran_cshift1_4 (&atmp.38, &parm.34, &atmp.35, n2);

which leads to:
  Fortran runtime error: Argument 'DIM' is out of range in call to 'CSHIFT'

And for random*.f90
  Fortran runtime error: RANDOM_SEED should have at most one argument present.
Comment 5 Tobias Burnus 2009-10-30 14:33:18 UTC
Patch: http://gcc.gnu.org/ml/fortran/2009-10/msg00246.html
Comment 6 Tobias Burnus 2009-11-01 12:43:56 UTC
Subject: Bug 41850

Author: burnus
Date: Sun Nov  1 12:43:42 2009
New Revision: 153793

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=153793
Log:
2009-11-01  Tobias Burnus  <burnus@net-b.de>

        PR fortran/41850
        * trans-expr.c (gfc_conv_procedure_call): Deallocate intent-out
        variables only when present. Remove unneccessary present check.

2009-11-01  Tobias Burnus  <burnus@net-b.de>

        PR fortran/41850
        * gfortran.dg/intent_out_6.f90: New testcase.


Added:
    trunk/gcc/testsuite/gfortran.dg/intent_out_6.f90
Modified:
    trunk/gcc/fortran/ChangeLog
    trunk/gcc/fortran/trans-expr.c
    trunk/gcc/testsuite/ChangeLog

Comment 7 Tobias Burnus 2009-11-01 14:35:51 UTC
Subject: Bug 41850

Author: burnus
Date: Sun Nov  1 14:35:40 2009
New Revision: 153794

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=153794
Log:
2009-11-01  Tobias Burnus  <burnus@net-b.de>

        PR fortran/41850
        * trans-expr.c (gfc_conv_procedure_call): Deallocate intent-out
        variables only when present.

2009-11-01  Tobias Burnus  <burnus@net-b.de>

        PR fortran/41850
        * gfortran.dg/intent_out_6.f90: New testcase.


Added:
    branches/gcc-4_4-branch/gcc/testsuite/gfortran.dg/intent_out_6.f90
Modified:
    branches/gcc-4_4-branch/gcc/fortran/ChangeLog
    branches/gcc-4_4-branch/gcc/fortran/trans-expr.c
    branches/gcc-4_4-branch/gcc/testsuite/ChangeLog

Comment 8 Tobias Burnus 2009-11-01 14:36:48 UTC
FIXED on the trunk (4.5) and on the 4.4 branch.