Re: [Patch, fortran] PR83076 - [8 Regression] ICE in gfc_deallocate_scalar_with_status, at fortran/trans.c:1598

Re: [Patch, fortran] PR83076 - [8 Regression] ICE in gfc_deallocate_scalar_with_status, at fortran/trans.c:1598

[Dominique d'Humières]

Dear Paul,

> Bootstrapped and regtested on FC23/x86_64 - OK for trunk?

See my comment 7 in the PR.

Dominique

Re: [Patch, fortran] PR83076 - [8 Regression] ICE in gfc_deallocate_scalar_with_status, at fortran/trans.c:1598

[Paul Richard Thomas]

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

Hi All,

This is a complete rework of the patch and of the original mechanism
for adding caf token fields and finding them.

In this patch, the token fields are added to the derived types after
all the components have been resolved. This is done so that all the
tokens appear at the very end of the derived type, including the
hidden string lengths. This avoids the present situation, where the
token appears immediately after its associated component such that the
the derived types are not compatible with modules or libraries
compiled without -fcoarray selected. All trans-types has to do now is
to find the component and have the component token field point to its
backend_decl. PR83319 is fixed by unconditionally adding the token
field to the descriptor, when -fcoarray=lib whatever the value of
codimen.

This is something of a belt-and-braces approach, in that the token
fields will sometimes be added when not needed. However, it is better
that than the ICEs that occur when they are missing.

Bootstrapped and regtested on FC23/x86_64 - OK for trunk and 7-branch?

Paul

2017-12-26  Paul Thomas  <pault@gcc.gnu.org>

    PR fortran/83076
    * resolve.c (resolve_fl_derived0): Add caf_token fields for
    allocatable and pointer scalars, when -fcoarray selected.
    * trans-types.c (gfc_copy_dt_decls_ifequal): Copy the token
    field as well as the backend_decl.
    (gfc_get_derived_type): Flag GFC_FCOARRAY_LIB for module
    derived types that are not vtypes. Components with caf_token
    attribute are pvoid types. For a component requiring it, find
    the caf_token field and have the component token field point to
    its backend_decl.

    PR fortran/83319
    *trans-types.c (gfc_get_array_descriptor_base): Add the token
    field to the descriptor even when codimen not set.


2017-12-26  Paul Thomas  <pault@gcc.gnu.org>

    PR fortran/83076
    * gfortran.dg/coarray_45.f90 : New test.

    PR fortran/83319
    * gfortran.dg/coarray_46.f90 : New test.


On 3 December 2017 at 23:48, Dominique d'Humières
<dominiq@tournesol.lps.ens.fr> wrote:
> Dear Paul,
>
>> Bootstrapped and regtested on FC23/x86_64 - OK for trunk?
>
> See my comment 7 in the PR.
>
> Dominique
>



-- 
"If you can't explain it simply, you don't understand it well enough"
- Albert Einstein

[-- Attachment #2: submit.diff --]
[-- Type: text/plain, Size: 7293 bytes --]

Index: gcc/fortran/gfortran.h
===================================================================
*** gcc/fortran/gfortran.h	(revision 256000)
--- gcc/fortran/gfortran.h	(working copy)
*************** typedef struct
*** 870,876 ****
    unsigned alloc_comp:1, pointer_comp:1, proc_pointer_comp:1,
  	   private_comp:1, zero_comp:1, coarray_comp:1, lock_comp:1,
  	   event_comp:1, defined_assign_comp:1, unlimited_polymorphic:1,
! 	   has_dtio_procs:1;
  
    /* This is a temporary selector for SELECT TYPE or an associate
       variable for SELECT_TYPE or ASSOCIATE.  */
--- 870,876 ----
    unsigned alloc_comp:1, pointer_comp:1, proc_pointer_comp:1,
  	   private_comp:1, zero_comp:1, coarray_comp:1, lock_comp:1,
  	   event_comp:1, defined_assign_comp:1, unlimited_polymorphic:1,
! 	   has_dtio_procs:1, caf_token:1;
  
    /* This is a temporary selector for SELECT TYPE or an associate
       variable for SELECT_TYPE or ASSOCIATE.  */
Index: gcc/fortran/resolve.c
===================================================================
*** gcc/fortran/resolve.c	(revision 256000)
--- gcc/fortran/resolve.c	(working copy)
*************** resolve_fl_derived0 (gfc_symbol *sym)
*** 13992,13997 ****
--- 13992,14022 ----
    if (!success)
      return false;
  
+   /* Now add the caf token field, where needed.  */
+   if (flag_coarray != GFC_FCOARRAY_NONE
+       && !sym->attr.is_class && !sym->attr.vtype)
+     {
+       for (c = sym->components; c; c = c->next)
+ 	if (!c->attr.dimension && !c->attr.codimension
+ 	    && (c->attr.allocatable || c->attr.pointer))
+ 	  {
+ 	    char name[GFC_MAX_SYMBOL_LEN+9];
+ 	    gfc_component *token;
+ 	    sprintf (name, "_caf_%s", c->name);
+ 	    token = gfc_find_component (sym, name, true, true, NULL);
+ 	    if (token == NULL)
+ 	      {
+ 		if (!gfc_add_component (sym, name, &token))
+ 		  return false;
+ 		token->ts.type = BT_VOID;
+ 		token->ts.kind = gfc_default_integer_kind;
+ 		token->attr.access = ACCESS_PRIVATE;
+ 		token->attr.artificial = 1;
+ 		token->attr.caf_token = 1;
+ 	      }
+ 	  }
+     }
+ 
    check_defined_assignments (sym);
  
    if (!sym->attr.defined_assign_comp && super_type)
Index: gcc/fortran/trans-types.c
===================================================================
*** gcc/fortran/trans-types.c	(revision 256000)
--- gcc/fortran/trans-types.c	(working copy)
*************** gfc_get_array_descriptor_base (int dimen
*** 1837,1843 ****
        TREE_NO_WARNING (decl) = 1;
      }
  
!   if (flag_coarray == GFC_FCOARRAY_LIB && codimen)
      {
        decl = gfc_add_field_to_struct_1 (fat_type,
  					get_identifier ("token"),
--- 1837,1843 ----
        TREE_NO_WARNING (decl) = 1;
      }
  
!   if (flag_coarray == GFC_FCOARRAY_LIB)
      {
        decl = gfc_add_field_to_struct_1 (fat_type,
  					get_identifier ("token"),
*************** gfc_copy_dt_decls_ifequal (gfc_symbol *f
*** 2373,2378 ****
--- 2373,2379 ----
    for (; to_cm; to_cm = to_cm->next, from_cm = from_cm->next)
      {
        to_cm->backend_decl = from_cm->backend_decl;
+       to_cm->caf_token = from_cm->caf_token;
        if (from_cm->ts.type == BT_UNION)
          gfc_get_union_type (to_cm->ts.u.derived);
        else if (from_cm->ts.type == BT_DERIVED
*************** gfc_get_derived_type (gfc_symbol * deriv
*** 2483,2488 ****
--- 2484,2493 ----
    gfc_dt_list *dt;
    gfc_namespace *ns;
    tree tmp;
+   bool coarray_flag;
+ 
+   coarray_flag = flag_coarray == GFC_FCOARRAY_LIB
+ 		 && derived->module && !derived->attr.vtype;
  
    gcc_assert (!derived->attr.pdt_template);
  
*************** gfc_get_derived_type (gfc_symbol * deriv
*** 2677,2683 ****
  	  field_type = build_pointer_type (tmp);
  	}
        else if (c->ts.type == BT_DERIVED || c->ts.type == BT_CLASS)
!         field_type = c->ts.u.derived->backend_decl;
        else
  	{
  	  if (c->ts.type == BT_CHARACTER
--- 2682,2690 ----
  	  field_type = build_pointer_type (tmp);
  	}
        else if (c->ts.type == BT_DERIVED || c->ts.type == BT_CLASS)
! 	field_type = c->ts.u.derived->backend_decl;
!       else if (c->attr.caf_token)
! 	field_type = pvoid_type_node;
        else
  	{
  	  if (c->ts.type == BT_CHARACTER
*************** gfc_get_derived_type (gfc_symbol * deriv
*** 2762,2780 ****
  	  && !(c->ts.type == BT_DERIVED
  	       && strcmp (c->name, "_data") == 0))
  	GFC_DECL_PTR_ARRAY_P (c->backend_decl) = 1;
- 
-       /* Do not add a caf_token field for classes' data components.  */
-       if (codimen && !c->attr.dimension && !c->attr.codimension
- 	  && (c->attr.allocatable || c->attr.pointer)
- 	  && c->caf_token == NULL_TREE && strcmp ("_data", c->name) != 0)
- 	{
- 	  char caf_name[GFC_MAX_SYMBOL_LEN];
- 	  snprintf (caf_name, GFC_MAX_SYMBOL_LEN, "_caf_%s", c->name);
- 	  c->caf_token = gfc_add_field_to_struct (typenode,
- 						  get_identifier (caf_name),
- 						  pvoid_type_node, &chain);
- 	  TREE_NO_WARNING (c->caf_token) = 1;
- 	}
      }
  
    /* Now lay out the derived type, including the fields.  */
--- 2769,2774 ----
*************** gfc_get_derived_type (gfc_symbol * deriv
*** 2800,2805 ****
--- 2794,2817 ----
  
  copy_derived_types:
  
+   for (c = derived->components; c; c = c->next)
+     {
+       /* Do not add a caf_token field for class container components.  */
+       if ((codimen || coarray_flag)
+ 	  && !c->attr.dimension && !c->attr.codimension
+ 	  && (c->attr.allocatable || c->attr.pointer)
+ 	  && !derived->attr.is_class)
+ 	{
+ 	  char caf_name[GFC_MAX_SYMBOL_LEN];
+ 	  gfc_component *token;
+ 	  snprintf (caf_name, GFC_MAX_SYMBOL_LEN, "_caf_%s", c->name);
+ 	  token = gfc_find_component (derived, caf_name, true, true, NULL);
+ 	  gcc_assert (token);
+ 	  c->caf_token = token->backend_decl;
+ 	  TREE_NO_WARNING (c->caf_token) = 1;
+ 	}
+     }
+ 
    for (dt = gfc_derived_types; dt; dt = dt->next)
      gfc_copy_dt_decls_ifequal (derived, dt->derived, false);
  
Index: gcc/testsuite/gfortran.dg/coarray_45.f90
===================================================================
*** gcc/testsuite/gfortran.dg/coarray_45.f90	(nonexistent)
--- gcc/testsuite/gfortran.dg/coarray_45.f90	(working copy)
***************
*** 0 ****
--- 1,24 ----
+ ! { dg-do compile }
+ ! { dg-options "-fcoarray=lib -lcaf_single " }
+ !
+ ! Test the fix for PR83076
+ !
+ module m
+    type t
+       integer, pointer :: z
+    end type
+    type(t) :: ptr
+ contains
+    function g(x)
+       type(t) :: x[*]
+       if (associated (x%z, ptr%z)) deallocate (x%z) ! This used to ICE with -fcoarray=lib
+    end
+ end module
+ 
+   use m
+ contains
+    function f(x)
+       type(t) :: x[*]
+       if (associated (x%z, ptr%z)) deallocate (x%z)
+    end
+ end
Index: gcc/testsuite/gfortran.dg/coarray_46.f90
===================================================================
*** gcc/testsuite/gfortran.dg/coarray_46.f90	(nonexistent)
--- gcc/testsuite/gfortran.dg/coarray_46.f90	(working copy)
***************
*** 0 ****
--- 1,17 ----
+ ! { dg-do compile }
+ ! { dg-options "-fcoarray=lib -lcaf_single" }
+ !
+ ! Test the fix for PR83319
+ !
+ module foo_module
+   implicit none
+   type foo
+     integer, allocatable :: i(:)
+   end type
+ end module
+ 
+   use foo_module
+   implicit none
+   type(foo), save :: bar[*]
+   allocate(bar%i(1))     ! Used to ICE here.
+ end

Re: [Patch, fortran] PR83076 - [8 Regression] ICE in gfc_deallocate_scalar_with_status, at fortran/trans.c:1598

[Thomas Koenig]

Hi Paul,

> This is a complete rework of the patch and of the original mechanism
> for adding caf token fields and finding them.
> 
> In this patch, the token fields are added to the derived types after
> all the components have been resolved. This is done so that all the
> tokens appear at the very end of the derived type, including the
> hidden string lengths. This avoids the present situation, where the
> token appears immediately after its associated component such that the
> the derived types are not compatible with modules or libraries
> compiled without -fcoarray selected. All trans-types has to do now is
> to find the component and have the component token field point to its
> backend_decl. PR83319 is fixed by unconditionally adding the token
> field to the descriptor, when -fcoarray=lib whatever the value of
> codimen.

This looks very good.

I just have one question: Will this break binary compatibility for
the 7-branch? Or will this only break compatibility for something
that never worked anyway?

Regards

	Thomas

Re: [Patch, fortran] PR83076 - [8 Regression] ICE in gfc_deallocate_scalar_with_status, at fortran/trans.c:1598

[Paul Richard Thomas]

Hi Thomas,

It will break binary compatibility for caf with scalar,
allocatable/pointer components. This comes about because I decided
that the caf tokens for thes components, should come after all other
components, rather than immediately after the "owner" component. This
has the advantage, that they are then binary compatible with external
functions or modules that are not compiled with -fcoarray = lib.

Arrays will continue to be binary compatible.

Thanks

Paul


On 27 December 2017 at 16:09, Thomas Koenig <tkoenig@netcologne.de> wrote:
> Hi Paul,
>
>> This is a complete rework of the patch and of the original mechanism
>> for adding caf token fields and finding them.
>>
>> In this patch, the token fields are added to the derived types after
>> all the components have been resolved. This is done so that all the
>> tokens appear at the very end of the derived type, including the
>> hidden string lengths. This avoids the present situation, where the
>> token appears immediately after its associated component such that the
>> the derived types are not compatible with modules or libraries
>> compiled without -fcoarray selected. All trans-types has to do now is
>> to find the component and have the component token field point to its
>> backend_decl. PR83319 is fixed by unconditionally adding the token
>> field to the descriptor, when -fcoarray=lib whatever the value of
>> codimen.
>
>
> This looks very good.
>
> I just have one question: Will this break binary compatibility for
> the 7-branch? Or will this only break compatibility for something
> that never worked anyway?
>
> Regards
>
>         Thomas
>



-- 
"If you can't explain it simply, you don't understand it well enough"
- Albert Einstein

Re: [Patch, fortran] PR83076 - [8 Regression] ICE in gfc_deallocate_scalar_with_status, at fortran/trans.c:1598

[Thomas Koenig]

Hi Paul,

> It will break binary compatibility for caf with scalar,
> allocatable/pointer components. This comes about because I decided
> that the caf tokens for thes components, should come after all other
> components, rather than immediately after the "owner" component. This
> has the advantage, that they are then binary compatible with external
> functions or modules that are not compiled with -fcoarray = lib.

So, is this something that we can do for gcc-7 (where we shold maintain
binary compatibility)?

Regards

	Thomas

Re: [Patch, fortran] PR83076 - [8 Regression] ICE in gfc_deallocate_scalar_with_status, at fortran/trans.c:1598

[Paul Richard Thomas]

Hi Thomas,

That's a good question. I have kept Damian in copy. It must be said
that the OpenCoarray folk are more concerned with PR83319, where there
is no problem of binary compatibility. I am awaiting some input from
him.

Cheers

Paul


On 27 December 2017 at 17:50, Thomas Koenig <tkoenig@netcologne.de> wrote:
> Hi Paul,
>
>> It will break binary compatibility for caf with scalar,
>> allocatable/pointer components. This comes about because I decided
>> that the caf tokens for thes components, should come after all other
>> components, rather than immediately after the "owner" component. This
>> has the advantage, that they are then binary compatible with external
>> functions or modules that are not compiled with -fcoarray = lib.
>
>
> So, is this something that we can do for gcc-7 (where we shold maintain
> binary compatibility)?
>
> Regards
>
>         Thomas



-- 
"If you can't explain it simply, you don't understand it well enough"
- Albert Einstein

Re: [Patch, fortran] PR83076 - [8 Regression] ICE in gfc_deallocate_scalar_with_status, at fortran/trans.c:1598

[Thomas Koenig]

Hi Paul,

by the way, the patch is OK for trunk. It is just gcc-7 that I am
worried about.

Regards

	Thomas

Re: [Patch, fortran] PR83076 - [8 Regression] ICE in gfc_deallocate_scalar_with_status, at fortran/trans.c:1598

[Dominique d'Humières]

It also fixes pr78983 and partially pr80235.

Thanks

Dominique

> Le 27 déc. 2017 à 19:04, Thomas Koenig <tkoenig@netcologne.de> a écrit :
> 
> Hi Paul,
> 
> by the way, the patch is OK for trunk. It is just gcc-7 that I am
> worried about.
> 
> Regards
> 
> 	Thomas

Re: [Patch, fortran] PR83076 - [8 Regression] ICE in gfc_deallocate_scalar_with_status, at fortran/trans.c:1598

[Damian Rouson]

 
Hi Paul,

Does breaking binary compatibility simply mean that CAF codes will need to be recompiled (which is fine) or does it mean that there will need to be work done on OpenCoarrays to support the changes in this patch (which is unlikely to happen soon without new contributors to OpenCoarrays)? If it’s the latter, could you make the appropriate changes to OpenCoarrays before committing this? We’re still dealing with some regressions that Andre introduced a year ago through changes in gfortran that have never been reflected in OpenCoarrays. In fact, just within the past week, the number of such regressions was finally reduced from 3 to 2 for the first time in many months. I’m hopeful that we can continue vectoring toward 0 rather than increasing the number. It’s already the case that several projects won’t even beginning evaluating CAF for production use until our 2 current regressions get fixed and there’s one project that is still stuck using GCC 6 because of the current regressions. We actually had to publish results using GCC 6 for that reason (see https://github.com/sourceryinstitute/coarray-icar-paw17/blob/master/main.pdf).  

Damian  

On December 27, 2017 at 10:04:13 AM, Thomas Koenig (tkoenig@netcologne.de(mailto:tkoenig@netcologne.de)) wrote:

> Hi Paul,
>  
> by the way, the patch is OK for trunk. It is just gcc-7 that I am
> worried about.
>  
> Regards
>  
> Thomas

Re: [Patch, fortran] PR83076 - [8 Regression] ICE in gfc_deallocate_scalar_with_status, at fortran/trans.c:1598

[Thomas Koenig]

Hi Damian,

> Does breaking binary compatibility simply mean that CAF codes will need to be recompiled (which is fine)

Well... not really. We are not supposed to break binary compatibility
in a release.  For gcc-8, we have greater freedom because we had to
do it anyway.

Now, the interesting question is the impact. If we break binary
compatibilty for something that never worked anyway or was useless, or
something that was broken by a gcc-7 regression, I think we're fine.

If not, well... one possible decision would be to wait for gcc-8 to
fix this.

 > or does it mean that there will need to be work done on OpenCoarrays 
to support the changes

This, I don't know, never having looked at the OpenCoarrays source.

Regards

	Thomas

Re: [Patch, fortran] PR83076 - [8 Regression] ICE in gfc_deallocate_scalar_with_status, at fortran/trans.c:1598

[Damian Rouson]

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

 
Thanks for the additional information Thomas. It sounds like I should test Paul’s patch. I should be able to do so today and will post the results by tomorrow. I’m adding OpenCoarrays developer Zaak Beekman to the cc and attaching the patch again in case he wants to try it as well. 

Zaak, the full thread is at https://gcc.gnu.org/ml/fortran/ and starts with a message from Paul on November 29.

Damian  

On December 27, 2017 at 11:09:29 AM, Thomas Koenig (tkoenig@netcologne.de(mailto:tkoenig@netcologne.de)) wrote:

> Hi Damian,
>  
> > Does breaking binary compatibility simply mean that CAF codes will need to be recompiled (which is fine)
>  
> Well... not really. We are not supposed to break binary compatibility
> in a release. For gcc-8, we have greater freedom because we had to
> do it anyway.
>  
> Now, the interesting question is the impact. If we break binary
> compatibilty for something that never worked anyway or was useless, or
> something that was broken by a gcc-7 regression, I think we're fine.
>  
> If not, well... one possible decision would be to wait for gcc-8 to
> fix this.
>  
> > or does it mean that there will need to be work done on OpenCoarrays
> to support the changes
>  
> This, I don't know, never having looked at the OpenCoarrays source.
>  
> Regards
>  
> Thomas

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

Index: gcc/fortran/trans-types.c
===================================================================
*** gcc/fortran/trans-types.c	(revision 255161)
--- gcc/fortran/trans-types.c	(working copy)
*************** gfc_get_derived_type (gfc_symbol * deriv
*** 2483,2488 ****
--- 2483,2492 ----
    gfc_dt_list *dt;
    gfc_namespace *ns;
    tree tmp;
+   bool coarray_flag;
+
+   coarray_flag = flag_coarray == GFC_FCOARRAY_LIB
+ 		 && derived->module && !derived->attr.vtype;

    gcc_assert (!derived->attr.pdt_template);

*************** gfc_get_derived_type (gfc_symbol * deriv
*** 2523,2534 ****
        return derived->backend_decl;
      }

!   /* If use associated, use the module type for this one.  */
    if (derived->backend_decl == NULL
        && derived->attr.use_assoc
        && derived->module
        && gfc_get_module_backend_decl (derived))
!     goto copy_derived_types;

    /* The derived types from an earlier namespace can be used as the
       canonical type.  */
--- 2527,2545 ----
        return derived->backend_decl;
      }

!   /* If use associated, use the module type for this one, except for the case
!      where codimensions are present or where a caf_token is needed for pointer
!      or allocatable components. */
    if (derived->backend_decl == NULL
        && derived->attr.use_assoc
        && derived->module
        && gfc_get_module_backend_decl (derived))
!     {
!       if (coarray_flag || codimen)
! 	got_canonical = true;
!       else
! 	goto copy_derived_types;
!     }

    /* The derived types from an earlier namespace can be used as the
       canonical type.  */
*************** gfc_get_derived_type (gfc_symbol * deriv
*** 2764,2770 ****
  	GFC_DECL_PTR_ARRAY_P (c->backend_decl) = 1;

        /* Do not add a caf_token field for classes' data components.  */
!       if (codimen && !c->attr.dimension && !c->attr.codimension
  	  && (c->attr.allocatable || c->attr.pointer)
  	  && c->caf_token == NULL_TREE && strcmp ("_data", c->name) != 0)
  	{
--- 2775,2782 ----
  	GFC_DECL_PTR_ARRAY_P (c->backend_decl) = 1;

        /* Do not add a caf_token field for classes' data components.  */
!       if ((codimen || coarray_flag)
! 	  && !c->attr.dimension && !c->attr.codimension
  	  && (c->attr.allocatable || c->attr.pointer)
  	  && c->caf_token == NULL_TREE && strcmp ("_data", c->name) != 0)
  	{
Index: gcc/testsuite/gfortran.dg/coarray_45.f90
===================================================================
*** gcc/testsuite/gfortran.dg/coarray_45.f90	(nonexistent)
--- gcc/testsuite/gfortran.dg/coarray_45.f90	(working copy)
***************
*** 0 ****
--- 1,24 ----
+ ! { dg-do compile }
+ ! { dg-options "-fcoarray=lib -lcaf_single " }
+ !
+ ! Test the fix for PR83076
+ !
+ module m
+    type t
+       integer, pointer :: z
+    end type
+    type(t) :: ptr
+ contains
+    function g(x)
+       type(t) :: x[*]
+       if (associated (x%z, ptr%z)) deallocate (x%z) ! This used to ICE with -fcoarray=lib
+    end
+ end module
+
+   use m
+ contains
+    function f(x)
+       type(t) :: x[*]
+       if (associated (x%z, ptr%z)) deallocate (x%z)
+    end
+ end

Re: [Patch, fortran] PR83076 - [8 Regression] ICE in gfc_deallocate_scalar_with_status, at fortran/trans.c:1598

[Paul Richard Thomas]

Hi All,

OK - I'll hold back until I hear from Damian & Zaak.

Cheers

Paul

On 27 December 2017 at 21:06, Damian Rouson
<damian@sourceryinstitute.org> wrote:
>
> Thanks for the additional information Thomas. It sounds like I should test Paul’s patch. I should be able to do so today and will post the results by tomorrow. I’m adding OpenCoarrays developer Zaak Beekman to the cc and attaching the patch again in case he wants to try it as well.
>
> Zaak, the full thread is at https://gcc.gnu.org/ml/fortran/ and starts with a message from Paul on November 29.
>
> Damian
>
> On December 27, 2017 at 11:09:29 AM, Thomas Koenig (tkoenig@netcologne.de(mailto:tkoenig@netcologne.de)) wrote:
>
>> Hi Damian,
>>
>> > Does breaking binary compatibility simply mean that CAF codes will need to be recompiled (which is fine)
>>
>> Well... not really. We are not supposed to break binary compatibility
>> in a release. For gcc-8, we have greater freedom because we had to
>> do it anyway.
>>
>> Now, the interesting question is the impact. If we break binary
>> compatibilty for something that never worked anyway or was useless, or
>> something that was broken by a gcc-7 regression, I think we're fine.
>>
>> If not, well... one possible decision would be to wait for gcc-8 to
>> fix this.
>>
>> > or does it mean that there will need to be work done on OpenCoarrays
>> to support the changes
>>
>> This, I don't know, never having looked at the OpenCoarrays source.
>>
>> Regards
>>
>> Thomas



-- 
"If you can't explain it simply, you don't understand it well enough"
- Albert Einstein

Re: [Patch, fortran] PR83076 - [8 Regression] ICE in gfc_deallocate_scalar_with_status, at fortran/trans.c:1598

[Andre Vehreschild]

Hi all,

as long as the computation where the token can be found is adapted in the same
way, i.e. the token's offset in the derived type monitors the changed position,
everything is fine. When I remember correctly, then this is done
automatically by the routines setting up the caf_ref-chain for referencing into
coarrays of derived type's (trans-intrinsic.c:~1239 for example). So if
everything works, ok for trunk and gcc-7.


Regards,
	Andre

On Thu, 28 Dec 2017 11:37:00 +0000
Paul Richard Thomas <paul.richard.thomas@gmail.com> wrote:

> Hi All,
> 
> OK - I'll hold back until I hear from Damian & Zaak.
> 
> Cheers
> 
> Paul
> 
> On 27 December 2017 at 21:06, Damian Rouson
> <damian@sourceryinstitute.org> wrote:
> >
> > Thanks for the additional information Thomas. It sounds like I should test
> > Paul’s patch. I should be able to do so today and will post the results by
> > tomorrow. I’m adding OpenCoarrays developer Zaak Beekman to the cc and
> > attaching the patch again in case he wants to try it as well.
> >
> > Zaak, the full thread is at https://gcc.gnu.org/ml/fortran/ and starts with
> > a message from Paul on November 29.
> >
> > Damian
> >
> > On December 27, 2017 at 11:09:29 AM, Thomas Koenig
> > (tkoenig@netcologne.de(mailto:tkoenig@netcologne.de)) wrote: 
> >> Hi Damian,
> >>  
> >> > Does breaking binary compatibility simply mean that CAF codes will need
> >> > to be recompiled (which is fine)  
> >>
> >> Well... not really. We are not supposed to break binary compatibility
> >> in a release. For gcc-8, we have greater freedom because we had to
> >> do it anyway.
> >>
> >> Now, the interesting question is the impact. If we break binary
> >> compatibilty for something that never worked anyway or was useless, or
> >> something that was broken by a gcc-7 regression, I think we're fine.
> >>
> >> If not, well... one possible decision would be to wait for gcc-8 to
> >> fix this.
> >>  
> >> > or does it mean that there will need to be work done on OpenCoarrays  
> >> to support the changes
> >>
> >> This, I don't know, never having looked at the OpenCoarrays source.
> >>
> >> Regards
> >>
> >> Thomas  
> 
> 
> 


-- 
Andre Vehreschild * Email: vehre ad gmx dot de 

Re: [Patch, fortran] PR83076 - [8 Regression] ICE in gfc_deallocate_scalar_with_status, at fortran/trans.c:1598

[Damian Rouson]

I applied the patch the trunk and confirmed that it doesn’t break any previously
passing OpenCoarrays tests.  Is that sufficient or should I also try applying the 
patch to the 7 branch?

Damian

Re: [Patch, fortran] PR83076 - [8 Regression] ICE in gfc_deallocate_scalar_with_status, at fortran/trans.c:1598

[Paul Richard Thomas]

Committed to trunk as revision 256065.

Damian, it would be good if you would confirm that there are no issues
with applying the patch to 7-branch.

Thanks for all the help.

Paul

On 28 December 2017 at 19:58, Damian Rouson
<damian@sourceryinstitute.org> wrote:
> I applied the patch the trunk and confirmed that it doesn’t break any previously
> passing OpenCoarrays tests.  Is that sufficient or should I also try applying the
> patch to the 7 branch?
>
> Damian
>
>



-- 
"If you can't explain it simply, you don't understand it well enough"
- Albert Einstein

Re: [Patch, fortran] PR83076 - [8 Regression] ICE in gfc_deallocate_scalar_with_status, at fortran/trans.c:1598

[Damian Rouson]

I have now confirmed that the patch works the same for the 7 branch: it doesn’t break any previously passing tests.  

Damian

On January 1, 2018 at 9:44:59 AM, Paul Richard Thomas (paul.richard.thomas@gmail.com) wrote:

Committed to trunk as revision 256065.  

Damian, it would be good if you would confirm that there are no issues  
with applying the patch to 7-branch.  

Thanks for all the help.  

Paul  

On 28 December 2017 at 19:58, Damian Rouson  
<damian@sourceryinstitute.org> wrote:  
> I applied the patch the trunk and confirmed that it doesn’t break any previously  
> passing OpenCoarrays tests. Is that sufficient or should I also try applying the  
> patch to the 7 branch?  
>  
> Damian  
>  
>  



--  
"If you can't explain it simply, you don't understand it well enough"  
- Albert Einstein  

Re: [Patch, fortran] PR83076 - [8 Regression] ICE in gfc_deallocate_scalar_with_status, at fortran/trans.c:1598

[Paul Richard Thomas]

Many thanks, Damian. I will commit soonish; probably tomorrow.

Paul


On 3 January 2018 at 00:22, Damian Rouson <damian@sourceryinstitute.org> wrote:
> I have now confirmed that the patch works the same for the 7 branch: it
> doesn’t break any previously passing tests.
>
> Damian
>
> On January 1, 2018 at 9:44:59 AM, Paul Richard Thomas
> (paul.richard.thomas@gmail.com) wrote:
>
> Committed to trunk as revision 256065.
>
> Damian, it would be good if you would confirm that there are no issues
> with applying the patch to 7-branch.
>
> Thanks for all the help.
>
> Paul
>
> On 28 December 2017 at 19:58, Damian Rouson
> <damian@sourceryinstitute.org> wrote:
>> I applied the patch the trunk and confirmed that it doesn’t break any
>> previously
>> passing OpenCoarrays tests. Is that sufficient or should I also try
>> applying the
>> patch to the 7 branch?
>>
>> Damian
>>
>>
>
>
>
> --
> "If you can't explain it simply, you don't understand it well enough"
> - Albert Einstein



-- 
"If you can't explain it simply, you don't understand it well enough"
- Albert Einstein

[Patch, fortran] PR83076 - [8 Regression] ICE in gfc_deallocate_scalar_with_status, at fortran/trans.c:1598

[Paul Richard Thomas]

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

This problem is not really a regression but is a "feature" that was
exposed by my patch for PR81447.

The testcase fails because the caf token for the pointer component is
not present in the type. This is fixed in trans-types.c
(gfc_get_derived_type) in the manner described in the ChangeLog.

Bootstrapped and regtested on FC23/x86_64 - OK for trunk?

I would be grateful if caf aficionados would give the patch a whirl on
their favourite codes.

Cheers

Paul

2017-11-29  Paul Thomas  <pault@gcc.gnu.org>

    PR fortran/83076
    * trans-types.c (gfc_get_derived_type): Flag GFC_FCOARRAY_LIB
    for module derived types that are not vtypes. Use this flag to
    use the module backend_decl as the canonical type and to build
    the type anew, ensuring that scalar allocatable and pointer
    components have the caf token field added.

2017-11-29  Paul Thomas  <pault@gcc.gnu.org>

    PR fortran/83076
    * gfortran.dg/coarray_45.f90 : New test.

[-- Attachment #2: submit.diff --]
[-- Type: text/plain, Size: 3132 bytes --]

Index: gcc/fortran/trans-types.c
===================================================================
*** gcc/fortran/trans-types.c	(revision 255161)
--- gcc/fortran/trans-types.c	(working copy)
*************** gfc_get_derived_type (gfc_symbol * deriv
*** 2483,2488 ****
--- 2483,2492 ----
    gfc_dt_list *dt;
    gfc_namespace *ns;
    tree tmp;
+   bool coarray_flag;
+
+   coarray_flag = flag_coarray == GFC_FCOARRAY_LIB
+ 		 && derived->module && !derived->attr.vtype;

    gcc_assert (!derived->attr.pdt_template);

*************** gfc_get_derived_type (gfc_symbol * deriv
*** 2523,2534 ****
        return derived->backend_decl;
      }

!   /* If use associated, use the module type for this one.  */
    if (derived->backend_decl == NULL
        && derived->attr.use_assoc
        && derived->module
        && gfc_get_module_backend_decl (derived))
!     goto copy_derived_types;

    /* The derived types from an earlier namespace can be used as the
       canonical type.  */
--- 2527,2545 ----
        return derived->backend_decl;
      }

!   /* If use associated, use the module type for this one, except for the case
!      where codimensions are present or where a caf_token is needed for pointer
!      or allocatable components. */
    if (derived->backend_decl == NULL
        && derived->attr.use_assoc
        && derived->module
        && gfc_get_module_backend_decl (derived))
!     {
!       if (coarray_flag || codimen)
! 	got_canonical = true;
!       else
! 	goto copy_derived_types;
!     }

    /* The derived types from an earlier namespace can be used as the
       canonical type.  */
*************** gfc_get_derived_type (gfc_symbol * deriv
*** 2764,2770 ****
  	GFC_DECL_PTR_ARRAY_P (c->backend_decl) = 1;

        /* Do not add a caf_token field for classes' data components.  */
!       if (codimen && !c->attr.dimension && !c->attr.codimension
  	  && (c->attr.allocatable || c->attr.pointer)
  	  && c->caf_token == NULL_TREE && strcmp ("_data", c->name) != 0)
  	{
--- 2775,2782 ----
  	GFC_DECL_PTR_ARRAY_P (c->backend_decl) = 1;

        /* Do not add a caf_token field for classes' data components.  */
!       if ((codimen || coarray_flag)
! 	  && !c->attr.dimension && !c->attr.codimension
  	  && (c->attr.allocatable || c->attr.pointer)
  	  && c->caf_token == NULL_TREE && strcmp ("_data", c->name) != 0)
  	{
Index: gcc/testsuite/gfortran.dg/coarray_45.f90
===================================================================
*** gcc/testsuite/gfortran.dg/coarray_45.f90	(nonexistent)
--- gcc/testsuite/gfortran.dg/coarray_45.f90	(working copy)
***************
*** 0 ****
--- 1,24 ----
+ ! { dg-do compile }
+ ! { dg-options "-fcoarray=lib -lcaf_single " }
+ !
+ ! Test the fix for PR83076
+ !
+ module m
+    type t
+       integer, pointer :: z
+    end type
+    type(t) :: ptr
+ contains
+    function g(x)
+       type(t) :: x[*]
+       if (associated (x%z, ptr%z)) deallocate (x%z) ! This used to ICE with -fcoarray=lib
+    end
+ end module
+
+   use m
+ contains
+    function f(x)
+       type(t) :: x[*]
+       if (associated (x%z, ptr%z)) deallocate (x%z)
+    end
+ end