Created attachment 25856 [details]
Source archive

Revision 171100 is OK
revision  171653 segfault .

The miscompilation is triggered by -ffrontend-optimize, work-around: use -fno-frontend-optimize.
Revision 171653 is dealing with the frontend optimization. If I am not mistaken, it is the only change dealing with frontend optimization after r171100, although I don't understand how this revision could cause a miscompilation.

The crash occurs in __mo_dec_matrix_MOD_sum_vector.

(In reply to comment #3)
> The miscompilation is triggered by -ffrontend-optimize, work-around: use
> -fno-frontend-optimize.
> Revision 171653 is dealing with the frontend optimization. If I am not
> mistaken, it is the only change dealing with frontend optimization after
> r171100, although I don't understand how this revision could cause a
> miscompilation.
> 
> The crash occurs in __mo_dec_matrix_MOD_sum_vector.

Looks like a bug in the application, but the interesting
coding style makes it hard for me to read.  If one removes

    call delete_storage (x)

in sum_vector the problem goes away.  That is, it looks like
over-active memory management hidden beneath a layer of 
obfusication.

On Sat, Nov 19, 2011 at 12:40:49AM +0000, kargl at gcc dot gnu.org wrote:
> > The miscompilation is triggered by -ffrontend-optimize, work-around: use
> > -fno-frontend-optimize.
> > Revision 171653 is dealing with the frontend optimization. If I am not
> > mistaken, it is the only change dealing with frontend optimization after
> > r171100, although I don't understand how this revision could cause a
> > miscompilation.
> > 
> > The crash occurs in __mo_dec_matrix_MOD_sum_vector.
> 
> Looks like a bug in the application, but the interesting
> coding style makes it hard for me to read.  If one removes
> 
>     call delete_storage (x)
> 
> in sum_vector the problem goes away.  That is, it looks like
> over-active memory management hidden beneath a layer of 
> obfusication.
> 

Having looked at the code some more, I believe it is noconforming,
and this PR should be closed with INVALID,

(In reply to comment #3)
> The miscompilation is triggered by -ffrontend-optimize, work-around: use
> -fno-frontend-optimize.
> Revision 171653 is dealing with the frontend optimization. If I am not
> mistaken, it is the only change dealing with frontend optimization after
> r171100, although I don't understand how this revision could cause a
> miscompilation.
> 
> The crash occurs in __mo_dec_matrix_MOD_sum_vector.

Aha.  Compiling just main.f90 with -fno-frontend-optimize solves
the problem.

Thanks,
Harald

A few comments:

a) The call to delete_storage makes the code invalid in several
   respects

b) This function should not have been inlined (i.e. the bug not
   exposed) without the -faggressive-function-elimination flag.

c) The reason why this function call was inlined was that the
   implicit_pure attribute is set on the function.  This is
   bogus.


      /* Only eliminate potentially impure functions if the
	 user specifically requested it.  */
      if (!gfc_option.flag_aggressive_function_elimination
	  && !(*e)->value.function.esym->attr.pure
	  && !(*e)->value.function.esym->attr.implicit_pure)
	return 0;

(gdb) p (*e)->value.function.esym->attr.pure
$8 = 0
(gdb) p (*e)->value.function.esym->attr.implicit_pure
$9 = 1

Paul, I think you introduced the implicit_pure attribute.  Do
you have any idea why this could be set in this case?

(In reply to comment #6)
> Aha.  Compiling just main.f90 with -fno-frontend-optimize solves
> the problem.

Comparing -fdump-tree-original for main.f90 at -O0  without
and with -fno-frontend-optimize shows:

--- noopt/main.f90.003t.original        2011-11-19 10:51:21.000000000 +0100
+++ opt/main.f90.003t.original  2011-11-19 10:52:10.000000000 +0100
@@ -7,6 +7,7 @@
   static struct t_vector xi = {.info=0B, .n=0, .n_s=0, .alloc_l=0, .s={.data=0B
}};

   {
+    struct t_vector __var_1;
     integer(kind=4) overflow.2;
     logical(kind=4) D.1818;
     character(kind=4) size.1;
@@ -259,14 +260,9 @@
       _gfortran_transfer_character_write (&dt_parm.11, &"Before u:"[1]{lb: 1 sz
: 1}, 9);
       _gfortran_st_write_done (&dt_parm.11);
     }
-    {
-      struct t_vector D.1860;
-      struct t_vector D.1859;
-
-      D.1859 = vector_times_vector (&xi, &wi);
-      D.1860 = vector_times_vector (&xi, &wi);
-      u = sum_vector (&D.1859) + sum_vector (&D.1860);
-    }
+    __var_1 = vector_times_vector (&xi, &wi);
+    u = sum_vector (&__var_1) + sum_vector (&__var_1);
+    L.4:;
     {
       struct __st_parameter_dt dt_parm.12;


This won't work.  The implementation of the management
of temporaries does not allow that the same instance
is used more than once.

The other case with the commented lines exchanged:

+++ opt2/main.f90.003t.original 2011-11-19 10:59:23.000000000 +0100
@@ -7,6 +7,7 @@
   static struct t_vector xi = {.info=0B, .n=0, .n_s=0, .alloc_l=0, .s={.data=0B
}};

   {
+    struct t_vector __var_1;
     integer(kind=4) overflow.2;
     logical(kind=4) D.1818;
     character(kind=4) size.1;
@@ -259,16 +260,14 @@
       _gfortran_transfer_character_write (&dt_parm.11, &"Before u:"[1]{lb: 1 sz
: 1}, 9);
       _gfortran_st_write_done (&dt_parm.11);
     }
+    __var_1 = vector_times_vector (&xi, &wi);
     {
       struct t_vector D.1862;
-      struct t_vector D.1860;
-      struct t_vector D.1859;

-      D.1859 = vector_times_vector (&xi, &wi);
-      D.1860 = vector_times_vector (&xi, &wi);
-      D.1862 = add_vectors (&D.1859, &D.1860);
+      D.1862 = add_vectors (&__var_1, &__var_1);
       u = sum_vector (&D.1862);
     }
+    L.4:;
     {
       struct __st_parameter_dt dt_parm.12;


Almost the same here, with aliasing of the arguments leading
to the slightly different crash.

(In reply to comment #7)
> c) The reason why this function call was inlined was that the
>    implicit_pure attribute is set on the function.  This is
>    bogus.

Good point.

Adding:

  volatile :: xi, wi

in main solves the problem.

Am 19.11.2011 11:18, schrieb anlauf at gmx dot de:
> This won't work.  The implementation of the management
> of temporaries does not allow that the same instance
> is used more than once.

If I understand your code, you are modifying the arguments of your 
function and evaluating that function more than once in a single
expression.  This is illegal in Fortran, so gfortran could, in
principle, do anything with it.  I would advise you to fix your code to
be standard-conforming.

Because such code is unfortunately quite common, gfortran by default
does not do such optimizations unless directed to be. The fact that
it does here nonetheless is, indeed, a bug.

(In reply to comment #10)
The code does memory management similar to that required by
TR15581 for allocatable DT components and allocatable function
results, but it also has to work for compilers that do not
support TR15581.  It does so by overloading everything needed.

> If I understand your code, you are modifying the arguments of your 
> function and evaluating that function more than once in a single
> expression.

The expression xi*wi allocates a temporary, which needs to
get deallocated after it was used to avoid a memory leak.
All bookkeeping is done with the temporaries.  Functions
check whether they access a variable or a temporary.
Using the temporaries is not a pure operation, which is
consistent with the way the functions are declared.

> This is illegal in Fortran, so gfortran could, in
> principle, do anything with it.

I thought that program variables are somewhat different
from temporaries created from expressions.

In Fortran 95 I do not see any standard-conforming way
for a program to access that very temporary more than once.
If the compiler decides to use it twice, bad things happen.

(Same if a compiler decided to call deallocate twice.)

> I would advise you to fix your code to
> be standard-conforming.

Do you have a suggestion without introducing a memory leak?

> Because such code is unfortunately quite common, gfortran by default
> does not do such optimizations unless directed to be. The fact that
> it does here nonetheless is, indeed, a bug.

On Sat, Nov 19, 2011 at 10:57:23AM +0000, tkoenig at netcologne dot de wrote:
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51218
> 
> --- Comment #10 from tkoenig at netcologne dot de <tkoenig at netcologne dot de> 2011-11-19 10:57:23 UTC ---
> Am 19.11.2011 11:18, schrieb anlauf at gmx dot de:
> > This won't work.  The implementation of the management
> > of temporaries does not allow that the same instance
> > is used more than once.
> 
> If I understand your code, you are modifying the arguments of your 
> function and evaluating that function more than once in a single
> expression.  This is illegal in Fortran, so gfortran could, in
> principle, do anything with it.  I would advise you to fix your code to
> be standard-conforming.
> 
> Because such code is unfortunately quite common, gfortran by default
> does not do such optimizations unless directed to be. The fact that
> it does here nonetheless is, indeed, a bug.
> 

The code is also invalid because it manipulates the
pointer (not the target) of a derived-type dummy
argument with the intent(in) attribute.  The code
has at least two bugs and the PR should be closed.

On Sat, Nov 19, 2011 at 11:46:28AM +0000, anlauf at gmx dot de wrote:
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51218
> 
> --- Comment #11 from Harald Anlauf <anlauf at gmx dot de> 2011-11-19 11:46:28 UTC ---
> (In reply to comment #10)
> The code does memory management similar to that required by

s/does/tries to do/

> TR15581 for allocatable DT components and allocatable function
> results, but it also has to work for compilers that do not
> support TR15581.  It does so by overloading everything needed.
> 
> > If I understand your code, you are modifying the arguments of your 
> > function and evaluating that function more than once in a single
> > expression.
> 
> The expression xi*wi allocates a temporary, which needs to
> get deallocated after it was used to avoid a memory leak.
> All bookkeeping is done with the temporaries.  Functions
> check whether they access a variable or a temporary.
> Using the temporaries is not a pure operation, which is
> consistent with the way the functions are declared.

*If* the compiler generates a temporary for xi*wi, the
compiler will/should generate the necessary code to
garbage collect any memory used by that temporary.

> > I would advise you to fix your code to
> > be standard-conforming.
> 
> Do you have a suggestion without introducing a memory leak?
> 

Let the compiler do its job?
Don't manipulate pointers in a non-conforming way?

Tobias,

Why did you mark this PR with the "wrong-code" keyword?
The code is invalid, so gfortran can generated anything
it wants.

(In reply to comment #14)
> Tobias, Why did you mark this PR with the "wrong-code" keyword?

Because it generates "wrong-code" and I wasn't completely convinced that there is no bug lurking in implicit_pure. Thus, for me the status is an "unconfirmed" bug which would be - if confirmed - a wrong-code 4.7 regression.

However, it looks as if the code is invalid and so far I didn't come up with something valid which gets wrongly treated.

 * * *

(In reply to comment #11)
> The code does memory management similar to that required by
> TR15581 for allocatable DT components and allocatable function
> results, but it also has to work for compilers that do not
> support TR15581.  It does so by overloading everything needed.

I think that by now most compilers support TR 15581, thus, you could consider to solve the issue by changing the code to TR 15581.

At the moment you have code which might work by chance with some compilers but fail with others since it is invalid.


> > I would advise you to fix your code to
> > be standard-conforming.
> 
> Do you have a suggestion without introducing a memory leak?

Unfortunately not. The mix of having to use INTENT(IN) because of OPERATOR(...) combined with the need to modify the argument looks extremely fragile and - as this issue shows - also breaks in practice. In principle, using TARGET, you could allow the variable to alias - but only if you do not use INTENT(IN) ...

I think there is no simple way out, if (and only if) you need to be compatible to pre-TR15581 compilers. One other suggestion would be to compile the code with -O0.

For gfortran, I do not see any possibility to "fix" is, which doesn't cause severe performance regressions. The problem is not just the implicit_pure attribute or the front-end optimization. The Fortran front end also passes the INTENT(IN) to the middle end, which allows also the middle end to certain optimizations. And, here, the user has explicitly entered the INTENT(IN) himself.

Alias issues are one of the most important problems which hamper optimization. Thus, compilers try to make use of anything in the standard which rules out aliasing. If they can't, temporary arrays have to be introduced, code may not be optimized away, loop vectorization might be impossible etc. I think one of the strengths of gfortran is that it makes a lot of effort to avoid array temporaries and similar alias related issues.

Thus, unless a practical suggestion how gfortran should be changed comes up - or a scenario is found where gfortran generates invalid code for a valid program, I am inclined to close the bug as INVALID, which would be in line of the comments of Thomas and Steve.

(In reply to comment #15)
> Because it generates "wrong-code" and I wasn't completely convinced that there
> is no bug lurking in implicit_pure. Thus, for me the status is an "unconfirmed"
> bug which would be - if confirmed - a wrong-code 4.7 regression.

I found a shorter code which shows that functions with
side-effects are erroneously treated as pure, see below.

>  * * *
> 
> (In reply to comment #11)
> > The code does memory management similar to that required by
> > TR15581 for allocatable DT components and allocatable function
> > results, but it also has to work for compilers that do not
> > support TR15581.  It does so by overloading everything needed.
> 
> I think that by now most compilers support TR 15581, thus, you could consider
> to solve the issue by changing the code to TR 15581.

Well, most /= all.

At work I (have to) use a compiler which does not support TR 15581.
The (hard-/software) vendor (non-European, non-US) has been successfully
declining user demands to support F2003.

> At the moment you have code which might work by chance with some compilers but
> fail with others since it is invalid.

Well, I understand that the code does bad thing.
One thing it relies on is that the compiler recognizes
that the bad function are not pure, as they have a
side effect (e.g. accessing module variable call_level).
If a side effect is able to disable critical optimizations,
then I'm optimistic that the code will work on most platforms.

Now as promised, here's the reduced example:


module a
  implicit none
  integer :: neval = 0
! integer, volatile :: neval = 0   ! Won't change anything...
contains
  subroutine inc_eval
    neval = neval + 1
  end subroutine inc_eval
end module a

module b
  use a
  implicit none
contains
  function f(x)
    real :: f
    real, intent(in) :: x
    f = x
  end function f

! impure &       ! This won't help...
  function g(x)
    real :: g
    real, intent(in) :: x
    ! g is not pure, it has a side-effect!
    call inc_eval
!   print *, "In g!"
    g = x
  end function g
end module b

program gfcbug114a
  use a
  use b
  implicit none
  real :: x = 1, y = 1, t, u, v, w
  print *, (neval == 0) ! 0 initially
  t = f(x)*f(y)
  print *, (neval == 0)
  u = f(x)*f(y) + f(x)*f(y)
  print *, (neval == 0)
  v = g(x)*g(y)
  print *, (neval == 2)
  w = g(x)*g(y) + g(x)*g(y)
  print *, (neval == 6)
  print *, neval ! 6???
  print *, t, u, v, w
end program gfcbug114a


Running without optimization, I get:

 T
 T
 T
 T
 T
           6
   1.00000000       2.00000000       1.00000000       2.00000000

With optimization, the result is:

 T
 T
 T
 T
 F
           4
   1.00000000       2.00000000       1.00000000       2.00000000


Thus the number of function evaluations of the non-pure
function g deepends on the optimization.

Enabling the commented out print in g(),
I get the expected result.

Setting g as impure, as well as making neval volatile
does not help.  I do think that this qualifies as a bug.

(In reply to comment #16)
> One thing it relies on is that the compiler recognizes
> that the bad function are not pure, as they have a
> side effect (e.g. accessing module variable call_level).
> If a side effect is able to disable critical optimizations,
> then I'm optimistic that the code will work on most platforms.
> 
> Now as promised, here's the reduced example:

Thanks for the example!

Untested patch:

--- a/gcc/fortran/resolve.c
+++ b/gcc/fortran/resolve.c
@@ -3257,6 +3255,7 @@ pure_subroutine (gfc_code *c, gfc_symbol *sym)
   else if (gfc_pure (NULL))
     gfc_error ("Subroutine call to '%s' at %L is not PURE", sym->name,
               &c->loc);
+  gfc_current_ns->proc_name->attr.implicit_pure = 0;
 }

On Mon, Nov 21, 2011 at 08:02:20PM +0000, burnus at gcc dot gnu.org wrote:
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51218
> 
> --- Comment #17 from Tobias Burnus <burnus at gcc dot gnu.org> 2011-11-21 20:02:20 UTC ---
> (In reply to comment #16)
> > One thing it relies on is that the compiler recognizes
> > that the bad function are not pure, as they have a
> > side effect (e.g. accessing module variable call_level).
> > If a side effect is able to disable critical optimizations,
> > then I'm optimistic that the code will work on most platforms.
> > 
> > Now as promised, here's the reduced example:
> 
> Thanks for the example!
> 
> Untested patch:
> 
> --- a/gcc/fortran/resolve.c
> +++ b/gcc/fortran/resolve.c
> @@ -3257,6 +3255,7 @@ pure_subroutine (gfc_code *c, gfc_symbol *sym)
>    else if (gfc_pure (NULL))
>      gfc_error ("Subroutine call to '%s' at %L is not PURE", sym->name,
>                &c->loc);
> +  gfc_current_ns->proc_name->attr.implicit_pure = 0;
>  }

Harald's example has function calls not subroutines.  I would
expect that you need to set this in pure_function as well.

Also, does this type of change inhibit reason why 
implicit_pure was added in the first place?

(In reply to comment #18)
> > @@ -3257,6 +3255,7 @@ pure_subroutine (gfc_code *c, gfc_symbol *sym)
> > +  gfc_current_ns->proc_name->attr.implicit_pure = 0;
> 
> Harald's example has function calls not subroutines.  I would
> expect that you need to set this in pure_function as well.

Hmm, I had the feeling that such a check is already there, but I have not really audited resolve.c

> Also, does this type of change inhibit reason why 
> implicit_pure was added in the first place?

One probably should add a check whether the called procedure has attr.implicit_pure has set - and only is not mark the current namespace as impure. Then, I don't think it will inhibit the reason for the initial change. And even if it does, having correct code is more important. We might be able to mark more code as implicit none - but first we need to make sure that code which is effectively not pure is also not marked as implicit_pure.

(In reply to comment #17)
> Untested patch:
> 
> --- a/gcc/fortran/resolve.c
> +++ b/gcc/fortran/resolve.c
> @@ -3257,6 +3255,7 @@ pure_subroutine (gfc_code *c, gfc_symbol *sym)
>    else if (gfc_pure (NULL))
>      gfc_error ("Subroutine call to '%s' at %L is not PURE", sym->name,
>                &c->loc);
> +  gfc_current_ns->proc_name->attr.implicit_pure = 0;
>  }

The patch works for the testcase in comment #16.
It did not produce any regressions in my tests.

(It also solves my original problem, which is a nice by-product.)

Thanks,
Harald

Author: burnus
Date: Thu Nov 24 17:57:41 2011
New Revision: 181698

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

        PR fortran/51218
        * resolve.c (pure_subroutine): If called subroutine is
        impure, unset implicit_pure.
        (resolve_function): Move impure check to simplify code.

2011-11-24  Tobias Burnus  <burnus@net-b.de>

        PR fortran/51218
        * gfortran.dg/implicit_pure_1.f90: New.


Added:
    trunk/gcc/testsuite/gfortran.dg/implicit_pure_1.f90
Modified:
    trunk/gcc/fortran/ChangeLog
    trunk/gcc/fortran/resolve.c
    trunk/gcc/testsuite/ChangeLog

Author: burnus
Date: Thu Nov 24 20:44:28 2011
New Revision: 181699

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

        PR fortran/51218
        * resolve.c (pure_subroutine): If called subroutine is
        impure, unset implicit_pure.
        (resolve_function): Move impure check to simplify code.

2011-11-24  Tobias Burnus  <burnus@net-b.de>

        PR fortran/51218
        * gfortran.dg/implicit_pure_1.f90: New.


Added:
    branches/gcc-4_6-branch/gcc/testsuite/gfortran.dg/implicit_pure_1.f90
Modified:
    branches/gcc-4_6-branch/gcc/fortran/ChangeLog
    branches/gcc-4_6-branch/gcc/fortran/resolve.c
    branches/gcc-4_6-branch/gcc/testsuite/ChangeLog

FIXED - hopefully completely. Thanks for the bugreport and the (valid) testcase.

Am 24.11.2011 21:51, schrieb burnus at gcc dot gnu.org:
> Thanks for the bugreport and the (valid)
> testcase.
>

To be pedantic, the test case was not valid; however, as a
design choice, we chose to support it, because the error
it had is very common.

(In reply to comment #24)
> > Thanks for the bugreport and the (valid)
> > testcase.
> 
> To be pedantic, the test case was not valid

Can you tell me what's wrong with the test case of comment 16? It looks perfectly valid to me.

(One might argue about the result the program should print, i.e. whether a compiler may optimize "a = f() + f()" to "a= 2*f()" even if "f" is impure. I think the standard is a bit ambiguous about that one. But independently how one decides, I fail to see how it makes the program invalid. [And I stand firmly on the side that the compiler should not do this optimization (by default for any -O... level). On the other hand, I am also in favour of pure functions and prefer subroutines if the procedure is not pure.])

Am 25.11.2011 18:44, schrieb burnus at gcc dot gnu.org:
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51218
>
> --- Comment #25 from Tobias Burnus<burnus at gcc dot gnu.org>  2011-11-25 17:44:29 UTC ---
> (In reply to comment #24)
>>> Thanks for the bugreport and the (valid)
>>> testcase.
>>
>> To be pedantic, the test case was not valid
>
> Can you tell me what's wrong with the test case of comment 16? It looks
> perfectly valid to me.

I'l defer to authority here :-)

To quote Dick Hendrickson in the thread

ttps://groups.google.com/group/comp.lang.fortran/browse_thread/thread/6dfe2c3088f89725/3f69c230c86db253?hl=de&ie=UTF-8&q=functions+*+side+effects+group:comp.lang.fortran+author:Thomas+author:Koenig#3f69c230c86db253

# In this case, I think the standard is clear.  The processor is
# allowed to evaluate f(3) once or twice.  By the words
# Richard quoted, a function is not allowed to affect or be
# affected by anything else in the statement.  So, one evaluation
# of f(3) can't change the result of the other, and the processor
# is free to evaluate f(3) + f(3) as 2*f(3).  It
# is processor dependent.

Richard Maine sort of disagreed, he thinks the program is illegal.

So, either way, optimizing away a function call would be OK.