[Bug fortran/57590] New: [OOP] class containers are recycled between symbols more than they should be

[Bug fortran/57590] New: [OOP] class containers are recycled between symbols more than they should be

[mikael at gcc dot gnu.org]

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

            Bug ID: 57590
           Summary: [OOP] class containers are recycled between symbols
                    more than they should be
           Product: gcc
           Version: 4.9.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: fortran
          Assignee: unassigned at gcc dot gnu.org
          Reporter: mikael at gcc dot gnu.org

This bug was found while trying to fix pr57297.
Related bugs: pr52227, pr51610, pr53951.

We (gfortran developpers) have decided to generate OOP class containers using
gfortran AST:
we generate a (fortran) fake variable of a (fortran) fake type holding the
object's meta data (virtual table, ...) and a pointer to the actual data
("_data"). Then, at translation stage OOP containers are generated as if they
were regular fortran variables.
As a result of this, the attributes and array spec of an entity become that of
the fake variable's type. So we should generate a different class container
type for any combination of rank, attribute, and array spec.  It's not the case
currently, as shown by the following testcase (which doesn't seem invalid as
far as I know).


  type t
  end type t

  type(t) :: b(3), c(5), d(10), e(11)

  call s3(b)
  call s5(c)
  call sa(d)
  call sn(size(e,dim=1), e)

 contains

  subroutine s3(a)
    class(t), dimension(3) :: a
    print *, shape(a)
  end subroutine s3

  subroutine s5(a)
    class(t), dimension(5) :: a
    print *, shape(a)
  end subroutine s5

  subroutine sa(a)
    class(t), dimension(:) :: a
    print *, shape(a)
  end subroutine sa

  subroutine sn(n, a)
    integer :: n
    class(t), dimension(n) :: a
    print *, shape(a)
  end subroutine sn
end


This prints 3 four times, instead of the expected: 3, 5, 10 and 11, because
s3::a's class container is reused for every 'a' dummy argument of 's5', 'sa'
and 'sn'.
Every of the 'a' dummy argument should have its own class container type.
This means that we have to discriminate not only on the array spec type, but
also for AS_EXPLICIT on the bounds, and on the bounds' expressions (in the 'sn'
case).

[Bug fortran/57590] [OOP] class containers are recycled between symbols more than they should be

[dominiq at lps dot ens.fr]

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

Dominique d'Humieres <dominiq at lps dot ens.fr> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2013-06-27
     Ever confirmed|0                           |1

--- Comment #2 from Dominique d'Humieres <dominiq at lps dot ens.fr> ---
Confirmed.

[Bug fortran/57590] [OOP] wrong code with class variables of different shapes

[janus at gcc dot gnu.org]

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

janus at gcc dot gnu.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |wrong-code
                 CC|                            |janus at gcc dot gnu.org
            Summary|[OOP] class containers are  |[OOP] wrong code with class
                   |recycled between symbols    |variables of different
                   |more than they should be    |shapes

--- Comment #3 from janus at gcc dot gnu.org ---
(In reply to Mikael Morin from comment #0)
> Every of the 'a' dummy argument should have its own class container type.
> This means that we have to discriminate not only on the array spec type, but
> also for AS_EXPLICIT on the bounds, and on the bounds' expressions (in the
> 'sn' case).

I think in general it's ok to use the same class container type for all of
them, but we should not fix the array spec of the _data component at compile
time (but use something like AS_ASSUMED_SHAPE instead?).

As the dump shows, we set up a proper array descriptor for _data anyway, we
just need to use it:

  {
    struct array1_t parm.15;
    struct __class_MAIN___T_1_0 class.14;

    class.14._vptr = (struct __vtype_MAIN___T * {ref-all}) &__vtab_MAIN___T;
    parm.15.dtype = 41;
    parm.15.dim[0].lbound = 1;
    parm.15.dim[0].ubound = 5;
    parm.15.dim[0].stride = 1;
    parm.15.data = (void *) &c[0];
    parm.15.offset = -1;
    class.14._data = parm.15;
    s5 (&class.14);
  }


Maybe one can even get rid of the different class containers for different
ranks if one uses AS_ASSUMED_RANK?

[Bug fortran/57590] [OOP] wrong code with class variables of different shapes

[janus at gcc dot gnu.org]

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

--- Comment #4 from janus at gcc dot gnu.org ---
(In reply to janus from comment #3)
> I think in general it's ok to use the same class container type for all of
> them, but we should not fix the array spec of the _data component at compile
> time (but use something like AS_ASSUMED_SHAPE instead?).

In fact the following simple patch makes the test case produce the expected
output:


Index: gcc/fortran/class.c
===================================================================
--- gcc/fortran/class.c    (revision 201871)
+++ gcc/fortran/class.c    (working copy)
@@ -637,6 +637,8 @@ gfc_build_class_symbol (gfc_typespec *ts, symbol_a
       c->attr.codimension = attr->codimension;
       c->attr.abstract = fclass->attr.abstract;
       c->as = (*as);
+      if (c->as->type == AS_EXPLICIT)
+    c->as->type = AS_ASSUMED_SHAPE;
       c->initializer = NULL;

       /* Add component '_vptr'.  */


Will check if this survives a regtest.

[Bug fortran/57590] [OOP] wrong code with class variables of different shapes

[janus at gcc dot gnu.org]

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

janus at gcc dot gnu.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |janus at gcc dot gnu.org

--- Comment #5 from janus at gcc dot gnu.org ---
(In reply to janus from comment #4)
> Will check if this survives a regtest.

Certainly not! At least we need to check if an as is present at all ...


Index: gcc/fortran/class.c
===================================================================
--- gcc/fortran/class.c    (revision 201871)
+++ gcc/fortran/class.c    (working copy)
@@ -636,7 +636,12 @@ gfc_build_class_symbol (gfc_typespec *ts, symbol_a
       c->attr.dimension = attr->dimension;
       c->attr.codimension = attr->codimension;
       c->attr.abstract = fclass->attr.abstract;
-      c->as = (*as);
+      if (*as)
+    {
+      c->as = (*as);
+      if (c->as->type == AS_EXPLICIT)
+        c->as->type = AS_ASSUMED_SHAPE;
+    }
       c->initializer = NULL;

       /* Add component '_vptr'.  */

[Bug fortran/57590] [OOP] wrong code with class variables of different shapes

[mikael at gcc dot gnu.org]

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

--- Comment #6 from Mikael Morin <mikael at gcc dot gnu.org> ---
(In reply to janus from comment #3)
> (In reply to Mikael Morin from comment #0)
> > Every of the 'a' dummy argument should have its own class container type.
> > This means that we have to discriminate not only on the array spec type, but
> > also for AS_EXPLICIT on the bounds, and on the bounds' expressions (in the
> > 'sn' case).
> 
> I think in general it's ok to use the same class container type for all of
> them, but we should not fix the array spec of the _data component at compile
> time (but use something like AS_ASSUMED_SHAPE instead?).
OK, makes some sense.
But then we have to keep the original array spec somewhere.

What we can do:
 (1) Store array spec, pointer etc in the class container using
     attr->class_pointer, class_as, etc fields different from the regular
     attr->pointer, as, etc.
     This is currently done for attr->pointer, we need to extend it to as,
     attr->dimension, attr->codimension, attr.allocatable.
     Then, the code generation has to test for attr->is_class or something
     to know whether to use foo or class_foo.

 (2) Keep the fields in their original location, that is don't clear as,
     attr->pointer, attr->allocatable, attr->dimension, attr->codimension
     in gfc_build_class_symbol.
     This as the benefit over (1) of not needing extra room, and avoiding
     the foo->attr->bar vs CLASS_DATA(foo)->attr->bar vs
     CLASS_DATA(foo)->attr->class_bar hell.
     It needs basically the same adjustments at translation stage to not
     use the array spec etc of a class container.

The two above need some adjustements at translation stage, which defeats the
purpose of generating the class container using front-end structures only.
Thus, I'm more in favor of (3):
 (3) Don't use gfc_build_class_symbol, or delay it as much as possible until 
     code generation.  We are all dreamers, aren't we ? ;-)

> 
> As the dump shows, we set up a proper array descriptor for _data anyway, we
> just need to use it:
Hm, no; we just need to not clear the original array spec, or keep it somewhere
so that we can use it.  The original array spec is a constant, while data
loaded from the descriptor is not, until some middle-end inter process
optimizations guessed otherwise.

> 
> Maybe one can even get rid of the different class containers for different
> ranks if one uses AS_ASSUMED_RANK?
I think it's better for the middle-end to have types as much separated as
possible.  Unless of course we are forced to have the same type (as in
pr57297).

[Bug fortran/57590] [OOP] wrong code with class variables of different shapes

[mikael at gcc dot gnu.org]

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

--- Comment #7 from Mikael Morin <mikael at gcc dot gnu.org> ---
(In reply to janus from comment #5)
> (In reply to janus from comment #4)
> > Will check if this survives a regtest.
> 
> Certainly not! At least we need to check if an as is present at all ...
> 
> 
> Index: gcc/fortran/class.c
> ===================================================================
> --- gcc/fortran/class.c	(revision 201871)
> +++ gcc/fortran/class.c	(working copy)
> @@ -636,7 +636,12 @@ gfc_build_class_symbol (gfc_typespec *ts, symbol_a
>        c->attr.dimension = attr->dimension;
>        c->attr.codimension = attr->codimension;
>        c->attr.abstract = fclass->attr.abstract;
> -      c->as = (*as);
> +      if (*as)
> +	{
> +	  c->as = (*as);
> +	  if (c->as->type == AS_EXPLICIT)
> +	    c->as->type = AS_ASSUMED_SHAPE;
> +	}
>        c->initializer = NULL;
>  
>        /* Add component '_vptr'.  */

Alright, certainly not the grand solution that I had in mind. It defeats the
purpose of specifying array bounds explicitly. On the other hand, as it fixes a
wrong code, not that bad.
>From gcc-bugs-return-428118-listarch-gcc-bugs=gcc.gnu.org@gcc.gnu.org Tue Aug 20 14:19:28 2013
Return-Path: <gcc-bugs-return-428118-listarch-gcc-bugs=gcc.gnu.org@gcc.gnu.org>
Delivered-To: listarch-gcc-bugs@gcc.gnu.org
Received: (qmail 2135 invoked by alias); 20 Aug 2013 14:19:27 -0000
Mailing-List: contact gcc-bugs-help@gcc.gnu.org; run by ezmlm
Precedence: bulk
List-Id: <gcc-bugs.gcc.gnu.org>
List-Archive: <http://gcc.gnu.org/ml/gcc-bugs/>
List-Post: <mailto:gcc-bugs@gcc.gnu.org>
List-Help: <mailto:gcc-bugs-help@gcc.gnu.org>
Sender: gcc-bugs-owner@gcc.gnu.org
Delivered-To: mailing list gcc-bugs@gcc.gnu.org
Received: (qmail 2107 invoked by uid 48); 20 Aug 2013 14:19:26 -0000
From: "janus at gcc dot gnu.org" <gcc-bugzilla@gcc.gnu.org>
To: gcc-bugs@gcc.gnu.org
Subject: [Bug fortran/53655] [F03] "default initializer" warnings
Date: Tue, 20 Aug 2013 14:19:00 -0000
X-Bugzilla-Reason: CC
X-Bugzilla-Type: changed
X-Bugzilla-Watch-Reason: None
X-Bugzilla-Product: gcc
X-Bugzilla-Component: fortran
X-Bugzilla-Version: 4.8.0
X-Bugzilla-Keywords: diagnostic
X-Bugzilla-Severity: normal
X-Bugzilla-Who: janus at gcc dot gnu.org
X-Bugzilla-Status: RESOLVED
X-Bugzilla-Priority: P3
X-Bugzilla-Assigned-To: janus at gcc dot gnu.org
X-Bugzilla-Target-Milestone: ---
X-Bugzilla-Flags:
X-Bugzilla-Changed-Fields: bug_status resolution
Message-ID: <bug-53655-4-b6TIctjzj3@http.gcc.gnu.org/bugzilla/>
In-Reply-To: <bug-53655-4@http.gcc.gnu.org/bugzilla/>
References: <bug-53655-4@http.gcc.gnu.org/bugzilla/>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
X-Bugzilla-URL: http://gcc.gnu.org/bugzilla/
Auto-Submitted: auto-generated
MIME-Version: 1.0
X-SW-Source: 2013-08/txt/msg01042.txt.bz2
Content-length: 1171

http://gcc.gnu.org/bugzilla/show_bug.cgi?idS655

janus at gcc dot gnu.org changed:

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

--- Comment #7 from janus at gcc dot gnu.org ---
(In reply to janus from comment #6)
> The patch in comment 4 regtests cleanly. Will commit as obvious.


Author: janus
Date: Tue Aug 20 14:16:26 2013
New Revision: 201884

URL: http://gcc.gnu.org/viewcvs?rev 1884&root=gcc&view=rev
Log:
2013-08-20  Janus Weil  <janus@gcc.gnu.org>

    PR fortran/53655
    * trans-decl.c (generate_local_decl): Check if type has any components.

2013-08-20  Janus Weil  <janus@gcc.gnu.org>

    PR fortran/53655
    * gfortran.dg/intent_out_8.f90: New.

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


After this fixes case a), case b) turned out to be a non-bug and case c) has
been fixed earlier, I think we can close this PR.

[Bug fortran/57590] [OOP] wrong code with class variables of different shapes

[janus at gcc dot gnu.org]

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

--- Comment #8 from janus at gcc dot gnu.org ---
(In reply to janus from comment #5)
> Index: gcc/fortran/class.c
> ===================================================================
> --- gcc/fortran/class.c	(revision 201871)
> +++ gcc/fortran/class.c	(working copy)
> @@ -636,7 +636,12 @@ gfc_build_class_symbol (gfc_typespec *ts, symbol_a
>        c->attr.dimension = attr->dimension;
>        c->attr.codimension = attr->codimension;
>        c->attr.abstract = fclass->attr.abstract;
> -      c->as = (*as);
> +      if (*as)
> +	{
> +	  c->as = (*as);
> +	  if (c->as->type == AS_EXPLICIT)
> +	    c->as->type = AS_ASSUMED_SHAPE;
> +	}
>        c->initializer = NULL;
>  
>        /* Add component '_vptr'.  */


This shows the following testsuite failure:

FAIL: gfortran.dg/coarray_poly_3.f90  -O   (test for errors, line 25)
FAIL: gfortran.dg/coarray_poly_3.f90  -O  (test for excess errors)


That is, on the following code ...

function func() ! { dg-error "shall not be a coarray or have a coarray
component" }
  type t
  end type t
  class(t), allocatable :: func[*]
end

... it does not give the expected error, but instead:

coarray_poly_3.f90:25:

function func() ! { dg-error "shall not be a coarray or have a coarray
component" }
1
Error: Assumed shape array at (1) must be a dummy argument
>From gcc-bugs-return-428120-listarch-gcc-bugs=gcc.gnu.org@gcc.gnu.org Tue Aug 20 14:44:52 2013
Return-Path: <gcc-bugs-return-428120-listarch-gcc-bugs=gcc.gnu.org@gcc.gnu.org>
Delivered-To: listarch-gcc-bugs@gcc.gnu.org
Received: (qmail 27228 invoked by alias); 20 Aug 2013 14:44:51 -0000
Mailing-List: contact gcc-bugs-help@gcc.gnu.org; run by ezmlm
Precedence: bulk
List-Id: <gcc-bugs.gcc.gnu.org>
List-Archive: <http://gcc.gnu.org/ml/gcc-bugs/>
List-Post: <mailto:gcc-bugs@gcc.gnu.org>
List-Help: <mailto:gcc-bugs-help@gcc.gnu.org>
Sender: gcc-bugs-owner@gcc.gnu.org
Delivered-To: mailing list gcc-bugs@gcc.gnu.org
Received: (qmail 27202 invoked by uid 48); 20 Aug 2013 14:44:50 -0000
From: "mikael at gcc dot gnu.org" <gcc-bugzilla@gcc.gnu.org>
To: gcc-bugs@gcc.gnu.org
Subject: [Bug fortran/57590] [OOP] wrong code with class variables of different shapes
Date: Tue, 20 Aug 2013 14:44:00 -0000
X-Bugzilla-Reason: CC
X-Bugzilla-Type: changed
X-Bugzilla-Watch-Reason: None
X-Bugzilla-Product: gcc
X-Bugzilla-Component: fortran
X-Bugzilla-Version: 4.9.0
X-Bugzilla-Keywords: wrong-code
X-Bugzilla-Severity: normal
X-Bugzilla-Who: mikael at gcc dot gnu.org
X-Bugzilla-Status: ASSIGNED
X-Bugzilla-Priority: P3
X-Bugzilla-Assigned-To: janus at gcc dot gnu.org
X-Bugzilla-Target-Milestone: ---
X-Bugzilla-Flags:
X-Bugzilla-Changed-Fields:
Message-ID: <bug-57590-4-U2G1hzdxSq@http.gcc.gnu.org/bugzilla/>
In-Reply-To: <bug-57590-4@http.gcc.gnu.org/bugzilla/>
References: <bug-57590-4@http.gcc.gnu.org/bugzilla/>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
X-Bugzilla-URL: http://gcc.gnu.org/bugzilla/
Auto-Submitted: auto-generated
MIME-Version: 1.0
X-SW-Source: 2013-08/txt/msg01044.txt.bz2
Content-length: 348

http://gcc.gnu.org/bugzilla/show_bug.cgi?idW590

--- Comment #9 from Mikael Morin <mikael at gcc dot gnu.org> ---
(In reply to janus from comment #8)
> Error: Assumed shape array at (1) must be a dummy argument

I suppose s/AS_ASSUMED_SHAPE/AS_DEFERRED/ would do for this case, but the
problem remains the same: the original array spec is lost.

[Bug fortran/57590] [OOP] wrong code with class variables of different shapes

[janus at gcc dot gnu.org]

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

--- Comment #10 from janus at gcc dot gnu.org ---
(In reply to Mikael Morin from comment #9)
> (In reply to janus from comment #8)
> > Error: Assumed shape array at (1) must be a dummy argument
> 
> I suppose s/AS_ASSUMED_SHAPE/AS_DEFERRED/ would do for this case

That makes more sense anyway. I always get a bit confused by the
'assumed'/'deferred' nomenclature in the Fortran standard.


> but the problem remains the same: the original array spec is lost.

Yes, however any CLASS variable must be an allocatable, pointer or dummy. For
the first two, the shape must be deferred anyway.

One remaining problem here is that with the AS_DEFERRED patch, the following is
not rejected (as was the case before):

type :: t
end type
class(t), dimension(3), pointer :: c3
end

With unpatched trunk one gets:

class(t), dimension(3), pointer :: c3
                                     1
Error: Array pointer 'c3' at (1) must have a deferred shape or assumed rank


This check would need to be done before building the class container. Further
problems can appear with dummies. Here is a variant of comment 0:

  type t
  end type t

  type(t) :: b(2)

  call s3(b)

 contains

  subroutine s3(a)
    class(t), dimension(3) :: a
    print *, shape(a)
  end subroutine s3

end 

However, one doesn't get any warning or error here (neither with nor without
the patch). With a type(t) dummy, one gets:

  call s3(b)
          1
Warning: Actual argument contains too few elements for dummy argument 'a' (2/3)
at (1)

[Bug fortran/57590] [OOP] wrong code with class variables of different shapes

[janus at gcc dot gnu.org]

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

--- Comment #11 from janus at gcc dot gnu.org ---
(In reply to Mikael Morin from comment #6)
> The two above need some adjustements at translation stage, which defeats the
> purpose of generating the class container using front-end structures only.
> Thus, I'm more in favor of (3):
>  (3) Don't use gfc_build_class_symbol, or delay it as much as possible until 
>      code generation.

Yes, that might indeed be beneficial. I have already thought about this on a
few occasions.

It would be a larger project for sure, but it could simplify many things. Of
course there may also be some pitfalls lurking. I think the cleanest approach
would be to keep all the polymorphics as plain BT_CLASS variables (with all the
attributes as they are) in the front end and during resolution, and only
generate the class containers and vtabs at translation stage.