[Bug fortran/44582] New: Gfotran generates wrong results due to wrong ABI in function with array return

[Bug fortran/44582] New: Gfotran generates wrong results due to wrong ABI in function with array return

[yin at absoft dot com]

the function with array return must create a temporary array to hold the
returned value and transfer the value to destination array after function call.
Gfortran directly pass the destination array as the first fake argument. This
is wrong because destination is supposed to assigned to a value from the call.
If the function does nothing to the return array. the destination array will
have a wrong value. 

This is a example to show the bug

program sw

    integer,parameter::             M=500,N=60
    double precision,dimension(M,N,3)::             u
    double precision,dimension(M,N)::               dudx

    dudx(1,1) = 5.0
    dudx = ddx(u(:,:,1))

    contains


    function ddx(array)
    implicit double precision (a-h,o-z)
    double precision::          array(:,:)
    double precision::          ddx(size(array,dim=1),size(array,dim=2))

    print *, ddx(1,1)

    end function ddx

end program sw

gfortran returns 5.0
it should be 0.0 or a chaotic number.


-- 
           Summary: Gfotran generates wrong results due to wrong ABI in
                    function with array return
           Product: gcc
           Version: fortran-dev
            Status: UNCONFIRMED
          Severity: critical
          Priority: P3
         Component: fortran
        AssignedTo: unassigned at gcc dot gnu dot org
        ReportedBy: yin at absoft dot com
 GCC build triplet: x86-64
  GCC host triplet: x86-64
GCC target triplet: x86-64


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44582

[Bug fortran/44582] Gfotran generates wrong results due to wrong ABI in function with array return

[yin at absoft dot com]

------- Comment #1 from yin at absoft dot com  2010-06-18 18:16 -------
Created an attachment (id=20940)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=20940&action=view)
bug example


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44582

[Bug fortran/44582] Gfotran generates wrong results due to wrong ABI in function with array return

[yin at absoft dot com]

------- Comment #2 from yin at absoft dot com  2010-06-18 18:23 -------
it should be 0.0 always, NOT to be chaotic number like C, because when ddx is
declared, it has to be initialized to 0.0 by fortran standard. 


-- 

yin at absoft dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|Gfotran generates wrong     |Gfotran generates wrong
                   |results due to wrong ABI in |results due to wrong ABI in
                   |function with array return  |function with array return


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44582

[Bug fortran/44582] gfortran generates wrong results due to wrong ABI in function with array return

[kargl at gcc dot gnu dot org]

------- Comment #3 from kargl at gcc dot gnu dot org  2010-06-18 18:49 -------
(In reply to comment #2)
> it should be 0.0 always, NOT to be chaotic number like C, because when ddx is
> declared, it has to be initialized to 0.0 by fortran standard. 
> 

Can you point the language in the Fortran Standard that
supports your claim?


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44582

[Bug fortran/44582] gfortran generates wrong results due to wrong ABI in function with array return

[yin at absoft dot com]

------- Comment #4 from yin at absoft dot com  2010-06-18 19:00 -------
O.K. I doublechecked the standard. The array declared does not need to be
initialized in this case. So the return value could be any number. However,
this kind of implementation should fail in a certian case. I am trying to write
a small example to prove this. I will post it later.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44582

[Bug fortran/44582] gfortran generates wrong results due to wrong ABI in function with array return

[kargl at gcc dot gnu dot org]

------- Comment #5 from kargl at gcc dot gnu dot org  2010-06-18 19:10 -------
(In reply to comment #4)
> O.K. I doublechecked the standard. The array declared does not need to be
> initialized in this case. So the return value could be any number. However,
> this kind of implementation should fail in a certian case. I am trying to write
> a small example to prove this. I will post it later.
> 

Well, actually the array needs to be initialized, but not
by the compiler.  It is the responsibility of the programmer
to initialize his/her variables.  Techincally, your function
is invalid because

    function ddx(array)
    implicit double precision (a-h,o-z)
    double precision::          array(:,:)
    double precision::          ddx(size(array,dim=1),size(array,dim=2))

    print *, ddx(1,1)

    end function ddx

the print statement references an undefined variable.  See section
16.5.1:

  (1) An array is defined if and only if all of its elements are defined.

A careful reading of 16.5 might be useful.


-- 

kargl at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|                            |INVALID


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44582

[Bug fortran/44582] gfortran generates wrong results due to wrong ABI in function with array return

[burnus at gcc dot gnu dot org]

------- Comment #6 from burnus at gcc dot gnu dot org  2010-06-19 09:47 -------
(In reply to comment #0)
> the function with array return must create a temporary array to hold the
> returned value and transfer the value to destination array after function
> call.

> gfortran returns 5.0
> it should be 0.0 or a chaotic number.

Well, it is an arbitrary/chaotic/random number: In case of gfortran it happens
to be the previous value of the assignment (namely 5.0), but I do not see why
an always changing number - be once 0.0 or 1.0e88 or 42.0 is better. 


By the way, I have now filled PR 44589 as gfortran does not warn that the
return variable in "ddx" has not been initialized.



> O.K. I doublechecked the standard. The array declared does not need to be
> initialized in this case. So the return value could be any number. However,
> this kind of implementation should fail in a certian case. I am trying to
>  write a small example to prove this. I will post it later.


Thanks for alerting us of the following issue.

I believe the following program is an example where a temporary is needed as
first the RHS has to be evaluated before it is assigned to the LHS. Thus, the
optimization can be done - but only for PURE procedures.


The program below should print twice "10 10 10 10 10" but due to the bug, it
prints zeros for the first print and the tens only for the second print
statement.

program test
  implicit none
  integer :: a(5)
  a = 5
  a = f()
  print *, a
  a = 5
  print *, f()
contains
  function f()
     integer :: f(size(a))
     f = -5
     f = a - f
  end function f
end program test


The check is done in trans-expr.c's gfc_trans_arrayfunc_assign - here an extra
check (expr2->value.function.esym && expr2->value.function.esym->attr.pure)
seems to be needed.


-- 

burnus at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |burnus at gcc dot gnu dot
                   |                            |org
OtherBugsDependingO|                            |34640
              nThis|                            |
             Status|RESOLVED                    |UNCONFIRMED
  GCC build triplet|x86-64                      |
   GCC host triplet|x86-64                      |
 GCC target triplet|x86-64                      |
           Keywords|                            |wrong-code
         Resolution|INVALID                     |
            Version|fortran-dev                 |4.6.0


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44582

[Bug fortran/44582] gfortran generates wrong results due to wrong ABI in function with array return

[pault at gcc dot gnu dot org]

------- Comment #7 from pault at gcc dot gnu dot org  2010-06-19 12:30 -------
(In reply to comment #6)

> The program below should print twice "10 10 10 10 10" but due to the bug, it
> prints zeros for the first print and the tens only for the second print

Yes, indeed.  This goes back to gcc-4.3 at least.

> The check is done in trans-expr.c's gfc_trans_arrayfunc_assign - here an extra
> check (expr2->value.function.esym && expr2->value.function.esym->attr.pure)
> seems to be needed.
> 

I wonder if it is sufficient to test if there are any host associated
variables?

I have my thinking cap on:

(i) Is this standard violating?  I guess so, since the result is aliasing the
host associated symbol 'a'.
(ii) Is this only an issue for host association?
(iii) Is it sufficient to check if the lhs is host associated into the function
call?

Many thanks for flagging this up to us. It's really nice to know that the
leader of the Absoft fortran team is helping us out :-)

Cheers

Paul Thomas


-- 

pault at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |pault at gcc dot gnu dot org


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44582

[Bug fortran/44582] gfortran generates wrong results due to wrong ABI in function with array return

[pault at gcc dot gnu dot org]

------- Comment #8 from pault at gcc dot gnu dot org  2010-06-19 14:55 -------
Created an attachment (id=20942)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=20942&action=view)
Fix for PR, with testcase

This is less restrictive than requiring pure functions but is still correct, I
believe.

Bootstraps and regtests on FC9/x86_64 - OK for 4.3 to 4.6?

Paul


-- 

pault at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|unassigned at gcc dot gnu   |pault at gcc dot gnu dot org
                   |dot org                     |
             Status|UNCONFIRMED                 |ASSIGNED


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44582

[Bug fortran/44582] gfortran generates wrong results due to wrong ABI in function with array return

[pault at gcc dot gnu dot org]

------- Comment #9 from pault at gcc dot gnu dot org  2010-06-19 16:42 -------
(In reply to comment #8)
> Created an attachment (id=20942)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=20942&action=view) [edit]

Tobias correctly points out various cases that are still not correct.

It looks to me as if his proposal of only allowing PURE is the "correct" way to
go.  "correct" == "not so mind boggling as to render implementation
impossible".

Paul


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44582

[Bug fortran/44582] gfortran generates wrong results due to wrong ABI in function with array return

[pault at gcc dot gnu dot org]

------- Comment #10 from pault at gcc dot gnu dot org  2010-06-20 17:45 -------
Created an attachment (id=20948)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=20948&action=view)
A patch for the PR

I think this correctly takes account of last night's discussion on #gfortran.

Bootstraps and regtests OK on FC9/x86_64.

Note that I forgot to add a call to abort in myabort.  Clearly, I will do that.
The present version allowed me to confirm that trunk fails on all the pertinent
tests.

Paul


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44582

[Bug fortran/44582] gfortran generates wrong results due to wrong ABI in function with array return

[burnus at gcc dot gnu dot org]

------- Comment #11 from burnus at gcc dot gnu dot org  2010-06-20 18:31 -------
+   /* A temporary is not needed if the function is not contained and the
+      variable local. */
+   if (!sym->attr.use_assoc
+       && !sym->attr.in_common
+       && !sym->attr.pointer
+       && !sym->attr.target
+       && expr2->value.function.esym
+       && !expr2->value.function.esym->attr.contained)
+     return false;

Does use_assoc also include host-associated variables - it should for this
check. (I have not checked.)

Additionally allowed without temporary:
   sym->attr.dummy && sym->attr.intent == INTENT_OUT
as
  "If a dummy argument has INTENT (OUT), the actual argument becomes
   undefined at the time the association is established"
thus also any access via any method to that variable is undefined - and thus
invalid.

I think that the LHS is a dummy argument is a very common case and thus it
makes sense to optimize for INTENT(OUT).


+       && expr2->value.function.esym
+       && !expr2->value.function.esym->attr.contained)

Doesn't this not also unnecessarily prohibit
  contains
    subroutine a()
      dimension :: x(4)
      x = f()
    end subroutine
    function f()
as "f" is contained (in the same namespace as "a"? Or is this not set for the
"sym" as available in the namespace of "a"?


Otherwise, the patch looks OK.

> +   /* TODO a function that could correctly be declared PURE but is not
> +      could do with returning false as well.  */

(Well said, but not easily to be implemented. Actually, that could be a weaker
check as pure routines may not do I/O (on file units) or use (ERROR) STOP and
the argument INTENT(IN)/VALUE constraints do not matter either.)


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44582

[Bug fortran/44582] gfortran generates wrong results due to wrong ABI in function with array return

[dominiq at lps dot ens dot fr]

------- Comment #12 from dominiq at lps dot ens dot fr  2010-06-20 20:10 -------
With the patch in comment #10, the modified test for pr31538 from
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=31538#c5


 integer :: a(-4:1), b(0:4)
 b = 5
! a(-4:1) = b(0:4) ! Error: different shape for Array
!                  ! assignment at (1) on dimension 1 (6/5)
!
! gfortran: Array bound mismatch for dimension 1 of array 'f'
! NAG f95:  Rank 1 of array operand has extent 5 instead of 2
 i = 0
 a(i:1) = f(b)
contains
  function f(x)
    integer :: x(:),f(size(x))
    f = x
  end function
end

is no longer detected with -fckeck=bounds while the test

    program main
      integer, dimension (2) :: x
      x = (/ 1, 2 /)
      x = foo ()
      if (sum (x) .ne. 103) call abort
    contains
      function foo()
        integer, dimension (2) :: foo
        foo (1) = 100
        foo (2) = sum (x)
      end function
    end program main

pass.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44582

[Bug fortran/44582] gfortran generates wrong results due to wrong ABI in function with array return

[paul dot richard dot thomas at gmail dot com]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 2148 bytes --]



------- Comment #13 from paul dot richard dot thomas at gmail dot com  2010-06-22 04:36 -------
Subject: Re:  gfortran generates wrong results due to wrong 
        ABI in function with array return

Dear Tobias,

Thanks for looking through the patch.

> Does use_assoc also include host-associated variables - it should for this
> check. (I have not checked.)

No, it doesn't - I will add such a check.
>
> Additionally allowed without temporary:
>   sym->attr.dummy && sym->attr.intent == INTENT_OUT
> as
>  "If a dummy argument has INTENT (OUT), the actual argument becomes
>   undefined at the time the association is established"
> thus also any access via any method to that variable is undefined - and thus
> invalid.
>
> I think that the LHS is a dummy argument is a very common case and thus it
> makes sense to optimize for INTENT(OUT).
>

Hmmm!  I'll have to think about this business of dummies and their
intent.  Perhaps you could give me an example, where this causes
aliasing?

>
> +       && expr2->value.function.esym
> +       && !expr2->value.function.esym->attr.contained)
>
> Doesn't this not also unnecessarily prohibit
>  contains
>    subroutine a()
>      dimension :: x(4)
>      x = f()
>    end subroutine
>    function f()
> as "f" is contained (in the same namespace as "a"? Or is this not set for the
> "sym" as available in the namespace of "a"?

The check for host association of x is needed to suppress this case
from producing a temporary.

>
>
> Otherwise, the patch looks OK.
>
>> +   /* TODO a function that could correctly be declared PURE but is not
>> +      could do with returning false as well.  */
>
> (Well said, but not easily to be implemented. Actually, that could be a weaker
> check as pure routines may not do I/O (on file units) or use (ERROR) STOP and
> the argument INTENT(IN)/VALUE constraints do not matter either.)

Indeed.  I'll have a think about this, whilst I am about it.  It could
be done in resolve.c in parallel with the checks for purity.  I'll
make it a separate patch, though.

Thanks

Paul


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44582

[Bug fortran/44582] gfortran generates wrong results due to wrong ABI in function with array return

[burnus at gcc dot gnu dot org]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 1318 bytes --]



------- Comment #14 from burnus at gcc dot gnu dot org  2010-06-22 05:47 -------
> > Additionally allowed without temporary:
> >   sym->attr.dummy && sym->attr.intent == INTENT_OUT
> > as
> >  "If a dummy argument has INTENT (OUT), the actual argument becomes
> >   undefined at the time the association is established"
> > thus also any access via any method to that variable is undefined - and thus
> > invalid.
> >
> > I think that the LHS is a dummy argument is a very common case and thus it
> > makes sense to optimize for INTENT(OUT).
> 
> Hmmm!  I'll have to think about this business of dummies and their
> intent.  Perhaps you could give me an example, where this causes
> aliasing?

Example:

  real :: a(100)
  call test(a)
contains
  subroutine test(x)
    real, INTENT(OUT) :: x(:)
    x = f()
  end subroutine test

Here, no temporary is needed for "x = f()": The dummy "x" is INTENT(OUT) thus
the actual argument (i.e. "a") becomes undefined. Thus, the following function
is invalid as "a" is (also) undefined in "f":

  function f()
    real :: f(100)
    f = a
  end function f
end

I think that your patch will generate a temporary in this case - it shouldn't.
(Unfortunately, one cannot make similar assumptions for other intents :-(


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44582

[Bug fortran/44582] gfortran generates wrong results due to wrong ABI in function with array return

[pault at gcc dot gnu dot org]

------- Comment #15 from pault at gcc dot gnu dot org  2010-06-27 16:17 -------
Created an attachment (id=21017)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=21017&action=view)
An improved patch for the PR

Tobias,

I think that this does it - if anything it is on the conservative side still
but it does deal with comment #14

OK for trunk?

Paul


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44582

[Bug fortran/44582] gfortran generates wrong results due to wrong ABI in function with array return

[pault at gcc dot gnu dot org]

------- Comment #16 from pault at gcc dot gnu dot org  2010-06-27 16:33 -------
(In reply to comment #15)

> OK for trunk?

Sorry, forget this for a moment - its causes regressions.

Paul


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44582

[Bug fortran/44582] gfortran generates wrong results due to wrong ABI in function with array return

[paul dot richard dot thomas at gmail dot com]

------- Comment #17 from paul dot richard dot thomas at gmail dot com  2010-06-28 06:47 -------
Subject: Re:  gfortran generates wrong results due to wrong 
        ABI in function with array return

OK, I have regtested a submittable version this morning.  I will do
the business tonight.

Paul


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44582

[Bug fortran/44582] gfortran generates wrong results due to wrong ABI in function with array return

[pault at gcc dot gnu dot org]

------- Comment #18 from pault at gcc dot gnu dot org  2010-06-29 18:58 -------
Subject: Bug 44582

Author: pault
Date: Tue Jun 29 18:57:43 2010
New Revision: 161550

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

        PR fortran/44582
        * trans-expr.c (arrayfunc_assign_needs_temporary): New function
        to determine if a function assignment can be made without a
        temporary.
        (gfc_trans_arrayfunc_assign): Move all the conditions that
        suppress the direct function call to the above new functon and
        call it.

2010-06-29  Paul Thomas  <pault@gcc.gnu.org>

        PR fortran/44582
        * gfortran.dg/aliasing_array_result_1.f90 : New test.


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


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44582

[Bug fortran/44582] gfortran generates wrong results due to wrong ABI in function with array return

[pault at gcc dot gnu dot org]

------- Comment #19 from pault at gcc dot gnu dot org  2010-06-29 19:03 -------
Subject: Bug 44582

Author: pault
Date: Tue Jun 29 19:03:41 2010
New Revision: 161551

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

        PR fortran/44582
        * trans-expr.c (arrayfunc_assign_needs_temporary): New function
        to determine if a function assignment can be made without a
        temporary.
        (gfc_trans_arrayfunc_assign): Move all the conditions that
        suppress the direct function call to the above new functon and
        call it.

2010-06-29  Paul Thomas  <pault@gcc.gnu.org>

        PR fortran/44582
        * gfortran.dg/aliasing_array_result_1.f90 : New test.


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


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44582

[Bug fortran/44582] gfortran generates wrong results due to wrong ABI in function with array return

[pault at gcc dot gnu dot org]

------- Comment #20 from pault at gcc dot gnu dot org  2010-07-10 17:09 -------
Subject: Bug 44582

Author: pault
Date: Sat Jul 10 17:08:48 2010
New Revision: 162041

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=162041
Log:
2010-07-10  Paul Thomas  <pault@gcc.gnu.org>

        PR fortran/44582
        * trans-expr.c (arrayfunc_assign_needs_temporary): New function
        to determine if a function assignment can be made without a
        temporary.
        (gfc_trans_arrayfunc_assign): Move all the conditions that
        suppress the direct function call to the above new functon and
        call it.

        PR fortran/44773
        * trans-expr.c (arrayfunc_assign_needs_temporary): No temporary
        if the lhs has never been host associated, as well as not being
        use associated, a pointer or a target.
        * resolve.c (resolve_variable): Mark variables that are host
        associated.
        * gfortran.h: Add the host_assoc bit to the symbol_attribute
        structure.

2010-07-10  Paul Thomas  <pault@gcc.gnu.org>

        PR fortran/44582
        * gfortran.dg/aliasing_array_result_1.f90 : New test.


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


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44582

[Bug fortran/44582] gfortran generates wrong results due to wrong ABI in function with array return

[pault at gcc dot gnu dot org]

------- Comment #21 from pault at gcc dot gnu dot org  2010-07-16 04:49 -------
I tried to fix 4.3 but failed to find an easy way of overcoming problems with
4.3.  Since this bug has been present for 10 years without being reported, I
feel quite relaxed about leaving 4.3 as it is.

Fixed on 4.4-4.6

Thanks for the report!

Cheers

Paul


-- 

pault at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|                            |FIXED


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44582