[Patch, fortran] [4.7/4.8 Regression] [OOP] ICE on invalid: gfc_variable_attr(): Bad array reference

[Patch, fortran] [4.7/4.8 Regression] [OOP] ICE on invalid: gfc_variable_attr(): Bad array reference

[Paul Richard Thomas]

[-- Attachment #1: Type: text/plain, Size: 931 bytes --]

Dear All,

As noted by Janus in comment #2 of the PR, "I think the function
'copy_ts_from_selector_to_associate' comes too early (namely during
parsing). It tries to resolve the target expr, which should rather
wait until resolution stage!?!"

It turned out that the function of the call to gfc_resolve_expr was to
fix up the selector array reference type.  This has been done
explicitly in this patch.

Bootstraps and regtests on FC17/x86_64 - OK for trunk and 4.7?

A fix for PRs 53876, 54990 and 54992 is on its way, as soon as I am
back from getting some groceries :-)

Cheers

Paul

2013-01-02   Paul Thomas  <pault@gcc.gnu.org>

    PR fortran/55172
    * match.c (copy_ts_from_selector_to_associate): Remove call to
    gfc_resolve_expr and replace it with explicit setting of the
    array reference type.

2013-01-02   Paul Thomas  <pault@gcc.gnu.org>

    PR fortran/55172
    * gfortran.dg/select_type_31.f03: New test.

[-- Attachment #2: pr55172.diff --]
[-- Type: application/octet-stream, Size: 2436 bytes --]

Index: gcc/fortran/match.c
===================================================================
*** gcc/fortran/match.c	(revision 194770)
--- gcc/fortran/match.c	(working copy)
*************** copy_ts_from_selector_to_associate (gfc_
*** 5144,5155 ****
  {
    gfc_ref *ref;
    gfc_symbol *assoc_sym;
  
    assoc_sym = associate->symtree->n.sym;
  
-   /* Ensure that any array reference is resolved.  */
-   gfc_resolve_expr (selector);
- 
    /* At this stage the expression rank and arrayspec dimensions have
       not been completely sorted out. We must get the expr2->rank
       right here, so that the correct class container is obtained.  */
--- 5144,5153 ----
  {
    gfc_ref *ref;
    gfc_symbol *assoc_sym;
+   int i;
  
    assoc_sym = associate->symtree->n.sym;
  
    /* At this stage the expression rank and arrayspec dimensions have
       not been completely sorted out. We must get the expr2->rank
       right here, so that the correct class container is obtained.  */
*************** copy_ts_from_selector_to_associate (gfc_
*** 5161,5166 ****
--- 5159,5177 ----
  	&& CLASS_DATA (selector)->as
  	&& ref && ref->type == REF_ARRAY)
      {
+       /* Ensure that the array reference type is set.  */
+       if (ref->u.ar.type == AR_UNKNOWN)
+ 	{
+ 	  ref->u.ar.type = AR_ELEMENT;
+ 	  for (i = 0; i < ref->u.ar.dimen; i++)
+ 	    if (ref->u.ar.dimen_type[i] == DIMEN_RANGE
+ 		|| ref->u.ar.dimen_type[i] == DIMEN_VECTOR)
+ 	      {
+ 		ref->u.ar.type = AR_SECTION;
+ 		break;
+ 	      }
+ 	}
+ 
        if (ref->u.ar.type == AR_FULL)
  	selector->rank = CLASS_DATA (selector)->as->rank;
        else if (ref->u.ar.type == AR_SECTION)
Index: gcc/testsuite/gfortran.dg/select_type_31.f03
===================================================================
*** gcc/testsuite/gfortran.dg/select_type_31.f03	(revision 0)
--- gcc/testsuite/gfortran.dg/select_type_31.f03	(working copy)
***************
*** 0 ****
--- 1,19 ----
+ ! { dg-do compile }
+ ! Test the fix for PR55172.
+ !
+ ! Contributed by Arjen Markus  <arjen.markus@deltares.nl>
+ !
+ module gn
+   type :: ncb
+   end type ncb
+   type, public :: tn
+      class(ncb), allocatable, dimension(:) :: cb
+   end type tn
+ contains
+   integer function name(self)
+     implicit none
+     class (tn), intent(in) :: self
+     select type (component => self%cb(i)) ! { dg-error "has no IMPLICIT type" }
+     end select
+   end function name
+ end module gn

Re: [Patch, fortran] [4.7/4.8 Regression] [OOP] ICE on invalid: gfc_variable_attr(): Bad array reference

[Tobias Burnus]

Dear Paul,

Paul Richard Thomas wrote:
> As noted by Janus in comment #2 of the PR, "I think the function
> 'copy_ts_from_selector_to_associate' comes too early (namely during
> parsing). It tries to resolve the target expr, which should rather
> wait until resolution stage!?!"
>
> It turned out that the function of the call to gfc_resolve_expr was to
> fix up the selector array reference type.  This has been done
> explicitly in this patch.
>
> Bootstraps and regtests on FC17/x86_64 - OK for trunk and 4.7?

It looks mostly okay; however, you do not handle vector sections 
correctly, which leads to an ICE. Without your patch, one gets:
    Error: CLASS selector at (1) needs a temporary which is not yet 
implemented

With your patch, it fails as one has:
(gdb) p ref->next->u.ar.type
$7 = AR_ELEMENT

(gdb) p ref->next->u.ar.dimen_type
$8 = {DIMEN_VECTOR, 0, 0, 0, 0, 0, 0}

Please fix the DIMEN_VECTOR handling and add a test case for it (e.g. 
the one below). Could you also check whether we have a PR for that "not 
yet implemented" error?

module gn
   type :: ncb
   end type ncb
   type, public :: tn
      class(ncb), allocatable, dimension(:) :: cb
   end type tn
contains
   integer function name(self)
     implicit none
     class (tn), intent(in) :: self
     select type (component => self%cb([4,7+1]))
     end select
   end function name
end module gn


I am not quite sure whether the following ICE has the same cause or a 
different one, but it also ICEs with your patch applied:

module gn
   type :: ncb
   end type ncb
   type, public :: tn
      class(ncb), allocatable :: cb[:]
   end type tn
contains
   integer function name(self)
     implicit none
     class (tn), intent(in) :: self
     select type (component => self%cb[4])
! ALSO FAILS: "(component => self%cb)"
     end select
   end function name
end module gn



Tobias

Re: [Patch, fortran] [4.7/4.8 Regression] [OOP] ICE on invalid: gfc_variable_attr(): Bad array reference

[Paul Richard Thomas]

[-- Attachment #1: Type: text/plain, Size: 1273 bytes --]

Dear Tobias,

First of all, thanks for the review!  I still owe you my comments on
FINAL; I got lost in trying to fix these various regressions :-)  I
promise that I'll come back to you first thing tomorrow.

>
> It looks mostly okay; however, you do not handle vector sections correctly,
> which leads to an ICE. Without your patch, one gets:
>    Error: CLASS selector at (1) needs a temporary which is not yet
> implemented
>
> With your patch, it fails as one has:

This was fairly easily fixed - see attached.

....snip...

> I am not quite sure whether the following ICE has the same cause or a
> different one, but it also ICEs with your patch applied:
>
> module gn
>   type :: ncb
>   end type ncb
>   type, public :: tn
>      class(ncb), allocatable :: cb[:]
>   end type tn
> contains
>   integer function name(self)
>     implicit none
>     class (tn), intent(in) :: self
>     select type (component => self%cb[4])
> ! ALSO FAILS: "(component => self%cb)"
>     end select
>   end function name
> end module gn

This co-array example was never OK, as far as I can tell.  The error
is similar to that of the PR.  However, co-arrays were just never
handled at all.... again, as far as I can tell.  I'll have a go at it
tomorrow night.

With best regards

Paul

[-- Attachment #2: pr55172a.diff --]
[-- Type: application/octet-stream, Size: 2694 bytes --]

Index: gcc/fortran/match.c
===================================================================
*** gcc/fortran/match.c	(revision 194770)
--- gcc/fortran/match.c	(working copy)
*************** copy_ts_from_selector_to_associate (gfc_
*** 5144,5155 ****
  {
    gfc_ref *ref;
    gfc_symbol *assoc_sym;
  
    assoc_sym = associate->symtree->n.sym;
  
-   /* Ensure that any array reference is resolved.  */
-   gfc_resolve_expr (selector);
- 
    /* At this stage the expression rank and arrayspec dimensions have
       not been completely sorted out. We must get the expr2->rank
       right here, so that the correct class container is obtained.  */
--- 5144,5153 ----
  {
    gfc_ref *ref;
    gfc_symbol *assoc_sym;
+   int i;
  
    assoc_sym = associate->symtree->n.sym;
  
    /* At this stage the expression rank and arrayspec dimensions have
       not been completely sorted out. We must get the expr2->rank
       right here, so that the correct class container is obtained.  */
*************** copy_ts_from_selector_to_associate (gfc_
*** 5161,5166 ****
--- 5159,5181 ----
  	&& CLASS_DATA (selector)->as
  	&& ref && ref->type == REF_ARRAY)
      {
+       /* Ensure that the array reference type is set.  We cannot use
+ 	 gfc_resolve_expr at this point, so the usable parts of
+ 	 resolve.c(resolve_array_ref) are employed to do it.  */
+       if (ref->u.ar.type == AR_UNKNOWN)
+ 	{
+ 	  ref->u.ar.type = AR_ELEMENT;
+ 	  for (i = 0; i < ref->u.ar.dimen + ref->u.ar.codimen; i++)
+ 	    if (ref->u.ar.dimen_type[i] == DIMEN_RANGE
+ 		|| ref->u.ar.dimen_type[i] == DIMEN_VECTOR
+ 		|| (ref->u.ar.dimen_type[i] == DIMEN_UNKNOWN
+ 		    && ref->u.ar.start[i] && ref->u.ar.start[i]->rank))
+ 	      {
+ 		ref->u.ar.type = AR_SECTION;
+ 		break;
+ 	      }
+ 	}
+ 
        if (ref->u.ar.type == AR_FULL)
  	selector->rank = CLASS_DATA (selector)->as->rank;
        else if (ref->u.ar.type == AR_SECTION)
Index: gcc/testsuite/gfortran.dg/select_type_31.f03
===================================================================
*** gcc/testsuite/gfortran.dg/select_type_31.f03	(revision 0)
--- gcc/testsuite/gfortran.dg/select_type_31.f03	(working copy)
***************
*** 0 ****
--- 1,19 ----
+ ! { dg-do compile }
+ ! Test the fix for PR55172.
+ !
+ ! Contributed by Arjen Markus  <arjen.markus@deltares.nl>
+ !
+ module gn
+   type :: ncb
+   end type ncb
+   type, public :: tn
+      class(ncb), allocatable, dimension(:) :: cb
+   end type tn
+ contains
+   integer function name(self)
+     implicit none
+     class (tn), intent(in) :: self
+     select type (component => self%cb(i)) ! { dg-error "has no IMPLICIT type" }
+     end select
+   end function name
+ end module gn

Re: [Patch, fortran] [4.7/4.8 Regression] [OOP] ICE on invalid: gfc_variable_attr(): Bad array reference

[Tobias Burnus]

Dear Paul,

First, the new patch is fine from my side. (Although, I think the test 
case should also include the vector-section example.) Thanks for working 
on that regression.


Paul Richard Thomas wrote:
> First of all, thanks for the review!  I still owe you my comments on
> FINAL; I got lost in trying to fix these various regressions :-)  I
> promise that I'll come back to you first thing tomorrow.

I am looking forward to them - and in particular to a patch review of 
the FINAL patches. I am also interested in your comment to my LOGICAL in 
BIND(C) procedures patch, namely 
http://gcc.gnu.org/ml/fortran/2012-12/msg00200.html

>> It looks mostly okay; however, you do not handle vector sections correctly,
>> which leads to an ICE. Without your patch, one gets:
>>     Error: CLASS selector at (1) needs a temporary which is not yet
>> implemented
>>
>> With your patch, it fails as one has:
> This was fairly easily fixed - see attached.

Thanks. By the way, I have filled a PR to track this "not yet 
implemented" issue: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55849


>> I am not quite sure whether the following ICE has the same cause or a
>> different one, but it also ICEs with your patch applied:
>>      select type (component => self%cb[4])
> This co-array example was never OK, as far as I can tell. The error
> is similar to that of the PR.  However, co-arrays were just never
> handled at all.... again, as far as I can tell.  I'll have a go at it
> tomorrow night.

I recall that we did add some coarray support for polymorphic variables. 
At least with coarray arrays (contrary to coarray scalars) it seems to 
compile. But it is very likely that select type never worked with 
coarrays or coarray scalars.

Note that the coindexd

     select type (component => self%cb[4])
is invalid (C803; PR55850 (a)). However, the same failure occurs for noncoindexed valid selector:
     select type (component => self%cb)


(See also PR 55850 for some other SELECT TYPE issues I found.)

Tobias

Re: [Patch, fortran] [4.7/4.8 Regression] [OOP] ICE on invalid: gfc_variable_attr(): Bad array reference

[Paul Richard Thomas]

Dear Tobias,

Thanks for the review.  The patched trunk is just now bootstrapping
and regtesting prior to commitment.

I have added a check for co-indexed selectors in resolve.c and have
added tests in select_type_31.f03 for co-indexing and vector indices.

I am now turning my attention to FINAL :-)

Cheers

Paul

PS BTW co-arrays fail similarly as selectors in SELECT_TYPE and in
ASSOCIATE.  Do I assume correctly that the associate variable should
be a co-array?

On 2 January 2013 23:48, Tobias Burnus <burnus@net-b.de> wrote:
> Dear Paul,
>
> First, the new patch is fine from my side. (Although, I think the test case
> should also include the vector-section example.) Thanks for working on that
> regression.
>
>
>
> Paul Richard Thomas wrote:
>>
>> First of all, thanks for the review!  I still owe you my comments on
>> FINAL; I got lost in trying to fix these various regressions :-)  I
>> promise that I'll come back to you first thing tomorrow.
>
>
> I am looking forward to them - and in particular to a patch review of the
> FINAL patches. I am also interested in your comment to my LOGICAL in BIND(C)
> procedures patch, namely http://gcc.gnu.org/ml/fortran/2012-12/msg00200.html
>
>
>>> It looks mostly okay; however, you do not handle vector sections
>>> correctly,
>>> which leads to an ICE. Without your patch, one gets:
>>>     Error: CLASS selector at (1) needs a temporary which is not yet
>>> implemented
>>>
>>> With your patch, it fails as one has:
>>
>> This was fairly easily fixed - see attached.
>
>
> Thanks. By the way, I have filled a PR to track this "not yet implemented"
> issue: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55849
>
>
>>> I am not quite sure whether the following ICE has the same cause or a
>>> different one, but it also ICEs with your patch applied:
>>>      select type (component => self%cb[4])
>>
>> This co-array example was never OK, as far as I can tell. The error
>>
>> is similar to that of the PR.  However, co-arrays were just never
>> handled at all.... again, as far as I can tell.  I'll have a go at it
>> tomorrow night.
>
>
> I recall that we did add some coarray support for polymorphic variables. At
> least with coarray arrays (contrary to coarray scalars) it seems to compile.
> But it is very likely that select type never worked with coarrays or coarray
> scalars.
>
> Note that the coindexd
>
>
>     select type (component => self%cb[4])
> is invalid (C803; PR55850 (a)). However, the same failure occurs for
> noncoindexed valid selector:
>     select type (component => self%cb)
>
>
> (See also PR 55850 for some other SELECT TYPE issues I found.)
>
> Tobias



-- 
The knack of flying is learning how to throw yourself at the ground and miss.
       --Hitchhikers Guide to the Galaxy

Re: [Patch, fortran] [4.7/4.8 Regression] [OOP] ICE on invalid: gfc_variable_attr(): Bad array reference

[Paul Richard Thomas]

Dear Tobias,

Committed as revision 194916.

I am leaving the PR open to deal with 4.7.... just as soon as I have
downloaded it!

Thanks again for the careful review.

Cheers

Paul

On 2 January 2013 23:48, Tobias Burnus <burnus@net-b.de> wrote:
> Dear Paul,
>
> First, the new patch is fine from my side. (Although, I think the test case
> should also include the vector-section example.) Thanks for working on that
> regression.
>
>
>
> Paul Richard Thomas wrote:
>>
>> First of all, thanks for the review!  I still owe you my comments on
>> FINAL; I got lost in trying to fix these various regressions :-)  I
>> promise that I'll come back to you first thing tomorrow.
>
>
> I am looking forward to them - and in particular to a patch review of the
> FINAL patches. I am also interested in your comment to my LOGICAL in BIND(C)
> procedures patch, namely http://gcc.gnu.org/ml/fortran/2012-12/msg00200.html
>
>
>>> It looks mostly okay; however, you do not handle vector sections
>>> correctly,
>>> which leads to an ICE. Without your patch, one gets:
>>>     Error: CLASS selector at (1) needs a temporary which is not yet
>>> implemented
>>>
>>> With your patch, it fails as one has:
>>
>> This was fairly easily fixed - see attached.
>
>
> Thanks. By the way, I have filled a PR to track this "not yet implemented"
> issue: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55849
>
>
>>> I am not quite sure whether the following ICE has the same cause or a
>>> different one, but it also ICEs with your patch applied:
>>>      select type (component => self%cb[4])
>>
>> This co-array example was never OK, as far as I can tell. The error
>>
>> is similar to that of the PR.  However, co-arrays were just never
>> handled at all.... again, as far as I can tell.  I'll have a go at it
>> tomorrow night.
>
>
> I recall that we did add some coarray support for polymorphic variables. At
> least with coarray arrays (contrary to coarray scalars) it seems to compile.
> But it is very likely that select type never worked with coarrays or coarray
> scalars.
>
> Note that the coindexd
>
>
>     select type (component => self%cb[4])
> is invalid (C803; PR55850 (a)). However, the same failure occurs for
> noncoindexed valid selector:
>     select type (component => self%cb)
>
>
> (See also PR 55850 for some other SELECT TYPE issues I found.)
>
> Tobias



--
The knack of flying is learning how to throw yourself at the ground and miss.
       --Hitchhikers Guide to the Galaxy