Bug 57590 - [OOP] wrong code with class variables of different shapes
Summary: [OOP] wrong code with class variables of different shapes
Status: NEW
Alias: None
Product: gcc
Classification: Unclassified
Component: fortran (show other bugs)
Version: 4.9.0
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords: wrong-code
Depends on:
Blocks:
 
Reported: 2013-06-11 18:46 UTC by Mikael Morin
Modified: 2017-09-18 18:27 UTC (History)
1 user (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2013-06-27 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Mikael Morin 2013-06-11 18:46:57 UTC
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).
Comment 1 Mikael Morin 2013-06-11 18:49:21 UTC
(In reply to Mikael Morin from comment #0)
> This bug was found while trying to fix pr57297.

By the way pr57297 is the reverse problem (not enough sharing instead of too much).
Comment 2 Dominique d'Humieres 2013-06-27 09:24:47 UTC
Confirmed.
Comment 3 janus 2013-08-20 12:20:16 UTC
(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?
Comment 4 janus 2013-08-20 13:07:23 UTC
(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.
Comment 5 janus 2013-08-20 13:19:50 UTC
(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'.  */
Comment 6 Mikael Morin 2013-08-20 13:48:03 UTC
(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).
Comment 7 Mikael Morin 2013-08-20 13:51:33 UTC
(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.
Comment 8 janus 2013-08-20 14:33:57 UTC
(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
Comment 9 Mikael Morin 2013-08-20 14:44:50 UTC
(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.
Comment 10 janus 2013-08-20 15:06:10 UTC
(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)
Comment 11 janus 2013-08-20 15:28:33 UTC
(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.