Bug 50684 - [4.6/4.7 Regression] Incorrect error for move_alloc on element inside intent(in) pointer
Summary: [4.6/4.7 Regression] Incorrect error for move_alloc on element inside intent(...
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: fortran (show other bugs)
Version: 4.6.1
: P4 normal
Target Milestone: 4.6.3
Assignee: Not yet assigned to anyone
URL:
Keywords: rejects-valid
Depends on:
Blocks:
 
Reported: 2011-10-10 13:23 UTC by Martin Steghöfer
Modified: 2011-12-03 12:58 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed:


Attachments
Code to reproduce the behaviour (217 bytes, text/x-fortran)
2011-10-10 13:23 UTC, Martin Steghöfer
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Steghöfer 2011-10-10 13:23:13 UTC
Created attachment 25452 [details]
Code to reproduce the behaviour

Bug that was originally posted as additional comment to bug 50570, but turned out to be a different issue:

The code in the attachment is calling the function move_alloc with something inside an intent(in) pointer as actual argument and gives the following error message:

bug.f90:22.20:
    CALL MOVE_ALLOC(POINTER_INTENT_IN_VARIABLE%VALUE, LOCAL_VALUE)
                    1
Error: 'from' argument of 'move_alloc' intrinsic at (1) cannot be INTENT(IN)

I'm not sure about what the Fortran standard says, but I don't think that giving this error is a desired behaviour because:
* According to documentation of other compilers the code should compile: http://publib.boulder.ibm.com/infocenter/comphelp/v111v131/topic/com.ibm.xlf131.aix.doc/language_ref/intent.html (section "Rules", subsection about pointer dummy arguments)
* If INTENT(IN) really tries to protect the *members* (not the pointer itself) of the derived type from being changed (that's the only scenario in which this behaviour would make sense), then it's not doing its job: Copying the pointer to a local variable I'm able to change them, as the example "POINTER_INTENT_IN_BUG_WORKING" in the attached code shows.

Notes about the attachment:
Subroutine POINTER_INTENT_IN_BUG_FAILING contains the failing code.
Subroutien POINTER_INTENT_IN_BUG_WORKING contains an alternative code that does
the same thing, but compiles (to show that the behaviour of gfortran doesn't
make sense as protection, either).
Comment 1 janus 2011-10-12 19:52:57 UTC
Some carryover form PR50570, where I proposed a draft patch:

Index: gcc/fortran/check.c
===================================================================
--- gcc/fortran/check.c (revision 179722)
+++ gcc/fortran/check.c (working copy)
@@ -458,7 +458,8 @@ variable_check (gfc_expr *e, int n, bool allow_pro
   if (e->expr_type == EXPR_VARIABLE
       && e->symtree->n.sym->attr.intent == INTENT_IN
       && (gfc_current_intrinsic_arg[n]->intent == INTENT_OUT
-         || gfc_current_intrinsic_arg[n]->intent == INTENT_INOUT))
+         || gfc_current_intrinsic_arg[n]->intent == INTENT_INOUT)
+      && !(e->symtree->n.sym->attr.pointer && e->ref))
     {
       gfc_error ("'%s' argument of '%s' intrinsic at %L cannot be INTENT(IN)",
                 gfc_current_intrinsic_arg[n]->name, gfc_current_intrinsic,


Tobias commented on this as follows ...


That won't work for:

 type(t), intent(in) :: dt
 call move_alloc(dt%allocatable, ...)

That's invalid as dt%allocatable is intent(in); it would be valid for, e.g.,
  call move_alloc(dt%dt2%ptr%allocatable, ...)
One really needs to walk the expression.
Comment 2 janus 2011-10-12 20:31:04 UTC
Btw, I think this bug, as PR50570, is a regression in 4.6 and trunk. The original test case compiles cleanly with 4.5.3!
Comment 3 janus 2011-10-12 20:44:27 UTC
(In reply to comment #1)
> 
> That won't work for:
> 
>  type(t), intent(in) :: dt
>  call move_alloc(dt%allocatable, ...)
> 
> That's invalid as dt%allocatable is intent(in);


I have to admit that I hadn't really thought about this comment yet, but now that I do, I think the patch actually works for this case, assuming you mean the following:


  TYPE MY_TYPE
    INTEGER, ALLOCATABLE :: VALUE
  END TYPE

CONTAINS
  
  SUBROUTINE sub (dt)
    type(MY_TYPE), intent(in) :: dt
    INTEGER, ALLOCATABLE :: lv
    call move_alloc(dt%VALUE, lv)
  END SUBROUTINE

end


This was rejected before, and still is with my patch.

The reasoning of the patch is that one should only throw the error for intent(in) pointers, if they are passed on without any sub-references, which I would say is correct.
Comment 4 Tobias Burnus 2011-10-12 20:54:58 UTC
(In reply to comment #3)
>   TYPE MY_TYPE
>     INTEGER, ALLOCATABLE :: VALUE
>   END TYPE

>     type(MY_TYPE), intent(in) :: dt
>     call move_alloc(dt%VALUE, lv)
> 
> This was rejected before, and still is with my patch.

But should it be rejected? move_alloc does not modify the association status of "dt" - just of "dt%VALUE". Thus, I believe this test case is valid and, hence, gfortran should not reject it.

(For what it is worth: gfortran 4.5, pgi 11.5-0 and crayftn 7.1.4.111 accept the program, while gfortran 4.6/4.7 and ifort 11.1 reject it.)
Comment 5 janus 2011-10-12 21:17:58 UTC
(In reply to comment #4)
> > This was rejected before, and still is with my patch.
> 
> But should it be rejected? move_alloc does not modify the association status of
> "dt" - just of "dt%VALUE". Thus, I believe this test case is valid and, hence,
> gfortran should not reject it.

Huh, seems I misunderstood your comment.

I can't give any hard evidence right now, but naively I would say it should indeed be rejected. Shouldn't the allocation status of the components be protected by the "intent(in)"?

The following (equivalent) variant is at least rejected by gfortran 4.5 on upwards:

  TYPE MY_TYPE
    INTEGER, ALLOCATABLE :: VALUE
  END TYPE

CONTAINS
  
  SUBROUTINE sub (dt)
    type(MY_TYPE), intent(in) :: dt
    deallocate(dt%VALUE)
  END SUBROUTINE

end


    deallocate(dt%VALUE)
               1
Error: Dummy argument 'dt' with INTENT(IN) in variable definition context (DEALLOCATE object) at (1)
Comment 6 Tobias Burnus 2011-10-12 21:42:02 UTC
(In reply to comment #4)
> But should it be rejected? move_alloc does not modify the association
> status of "dt" - just of "dt%VALUE". Thus, I believe this test case is
> valid and, hence, gfortran should not reject it.

While I still think that the program is valid, I just realized that that is not what I wrote in comment 1 - there I claimed that it is invalid.

I also realized that one shouldn't need to walk the expression (contrary to intent(in) for nonpointer variables). Your simple

+      && !(e->symtree->n.sym->attr.pointer && e->ref))

should do. Actually, I do not quite understand why it shows an error with the patch:
  !(attr.ptr && e->ref)
should be false as attr.ptr is true and e->ref it not NULL and hence true. Thus the parentheses should be true, the ! makes it false. Thus, how can be there an error message?
Comment 7 Tobias Burnus 2011-10-12 21:53:28 UTC
(In reply to comment #5)
> I can't give any hard evidence right now, but naively I would say it should
> indeed be rejected. Shouldn't the allocation status of the components be
> protected by the "intent(in)"?
> 
> The following (equivalent) variant is at least rejected by gfortran 4.5 on
> upwards:
> 
>   TYPE MY_TYPE
>     INTEGER, ALLOCATABLE :: VALUE
>   END TYPE
> CONTAINS
>   SUBROUTINE sub (dt)
>     type(MY_TYPE), intent(in) :: dt
>     deallocate(dt%VALUE)

No, that version is *not* equivalent. In the previous example you have a *pointer intent*. Those only protect the association status of the dummy - not the value. -- Pointer intents are a Fortran 2003 feature.

In this example, you have a nonpointer variable. For those, the value (and hence allocation status) is preserved - including nonpointer components. Pointer components of intent(in) nonpointer variables may be modified as (or rather: if) one changes the pointer target and not the pointer itself. -- Nonpointer intents are a Fortran 90 feature.

In any case, the example of comment 5 is clearly invalid.


Cf. also (F2008):

"C539 (R523) A nonpointer object with the INTENT (IN) attribute
            shall not appear in a variable definition context (16.6.7)."

"C540 A pointer with the INTENT (IN) attribute shall not appear
     in a pointer association context (16.6.8)."
Comment 8 janus 2011-10-12 22:07:01 UTC
(In reply to comment #7)
> > The following (equivalent) variant is at least rejected by gfortran 4.5 on
> > upwards:
> > 
> >   TYPE MY_TYPE
> >     INTEGER, ALLOCATABLE :: VALUE
> >   END TYPE
> > CONTAINS
> >   SUBROUTINE sub (dt)
> >     type(MY_TYPE), intent(in) :: dt
> >     deallocate(dt%VALUE)
> 
> No, that version is *not* equivalent. In the previous example you have a
> *pointer intent*.

Well, comment #5 is 'equivalent' to comment #3: Both have a non-pointer intent (which is what you suggested in your initial comment to my patch).


> In any case, the example of comment 5 is clearly invalid.

Ok, good that we agree on that at least ;)

By the same reasoning, comment #3 should be invalid, since they both do the same thing wrt the argument 'dt' (namely deallocating its value component).
Comment 9 Tobias Burnus 2011-10-13 07:18:32 UTC
(In reply to comment #8)
> Well, comment #5 is 'equivalent' to comment #3: Both have a non-pointer intent
> (which is what you suggested in your initial comment to my patch).

Pilot error: I read in comment 3 an nonexisting "POINTER" - without pointer attribute, I concur that it should be invalid and analoguous to comment 5. -- I shouldn't write replies around midnight.

 * * *

I think the comments of the following program are correct; however, the compilers I tested either rejected all - or they compiled it without warning. Thus, I might have misunderstood something. On the other hand, if I change "call move_alloc" to "deallocate", gfortran follows my invalid/OK pattern. I have not checked whether it works with your patch, but I think it still rejects
(3).

subroutine test (x, px)
  implicit none
  type t
    integer, allocatable :: a
  end type t

  type t2
    type(t), pointer :: ptr
    integer, allocatable :: a
  end type t2

  type(t2), intent(in) :: x
  type(t2), pointer, intent(in) :: px

  integer, allocatable :: a
  type(t2), pointer :: ta

  call move_alloc (tx, ta)      ! Invalid (1)
  call move_alloc (x%a, a)      ! Invalid (2)
  call move_alloc (x%ptr%a, a)  ! OK (3)
  call move_alloc (px%a, a)     ! OK (4)
  call move_alloc (px%ptr%a, a) ! OK (5)
end subroutine test
Comment 10 janus 2011-10-13 12:27:58 UTC
(In reply to comment #9)
> I think the comments of the following program are correct; however, the
> compilers I tested either rejected all - or they compiled it without warning.
> Thus, I might have misunderstood something.

I would agree with your comments.


> On the other hand, if I change
> "call move_alloc" to "deallocate", gfortran follows my invalid/OK pattern.

With move_alloc, gfortran-4.5 rejects only (1), gfortran-4.6 rejects all of them.


> I have not checked whether it works with your patch, but I think it still
> rejects (3).

It does. In that sense I agree that the patch is not completely correct.


>   call move_alloc (tx, ta)      ! Invalid (1)

There 'tx' here should be an 'x', I guess.


>   call move_alloc (x%a, a)      ! Invalid (2)

This one corresponds to the original test case.
Comment 11 janus 2011-10-13 12:31:38 UTC
(In reply to comment #10)
> 
> >   call move_alloc (x%a, a)      ! Invalid (2)
> 
> This one corresponds to the original test case.

Sorry, of course I meant this one:

call move_alloc (px%a, a)     ! OK (4)
Comment 12 Jakub Jelinek 2011-10-26 17:14:06 UTC
GCC 4.6.2 is being released.
Comment 13 Tobias Burnus 2011-12-03 11:30:23 UTC
Author: burnus
Date: Sat Dec  3 11:30:18 2011
New Revision: 181967

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

        PR fortran/50684
        * check.c (variable_check): Fix intent(in) check.

2011-12-03  Tobias Burnus  <burnus@net-b.de>

        PR fortran/50684
        * gfortran.dg/move_alloc_8.f90: New.


Added:
    trunk/gcc/testsuite/gfortran.dg/move_alloc_8.f90
Modified:
    trunk/gcc/fortran/ChangeLog
    trunk/gcc/fortran/check.c
    trunk/gcc/testsuite/ChangeLog
Comment 14 Tobias Burnus 2011-12-03 12:57:41 UTC
Author: burnus
Date: Sat Dec  3 12:57:38 2011
New Revision: 181969

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

        PR fortran/50684
        * check.c (variable_check): Fix intent(in) check.

2011-12-03  Tobias Burnus  <burnus@net-b.de>

        PR fortran/50684
        * gfortran.dg/move_alloc_8.f90: New.


Added:
    branches/gcc-4_6-branch/gcc/testsuite/gfortran.dg/move_alloc_8.f90
Modified:
    branches/gcc-4_6-branch/gcc/fortran/ChangeLog
    branches/gcc-4_6-branch/gcc/fortran/check.c
    branches/gcc-4_6-branch/gcc/testsuite/ChangeLog
Comment 15 Tobias Burnus 2011-12-03 12:58:22 UTC
FIXED on the trunk (4.7) and on 4.6 branch.

Thanks for the report!