diff mbox series

[Fortran,Coarray,PR,37336] Fix crash in finalizer when derived type coarray is already freed.

Message ID 20230928144505.77e03a8e@vepi2
State Accepted
Headers
Series [Fortran,Coarray,PR,37336] Fix crash in finalizer when derived type coarray is already freed. |

Checks

Context Check Description
snail/gcc-patch-check success Github commit url

Commit Message

Andre Vehreschild Sept. 28, 2023, 12:45 p.m. UTC 
  Hi all,

attached patch fixes a crash in coarray programs when an allocatable derived
typed coarray was freed explicitly. The generated cleanup code did not take
into account, that the coarray may have been deallocated already. The patch
fixes this by moving the statements accessing components inside the derived type
into the block guard by its allocated check.

Regtested ok on f37/x86_64. Ok for master?

Regards,
	Andre
--
Andre Vehreschild * Email: vehre ad gmx dot de
Fortran: Free alloc. comp. in allocated coarrays only.

    When freeing allocatable components of an allocatable coarray, add
    a check that the coarray is still allocated, before accessing the
    components.

    This patch adds to PR fortran/37336, but does not fix it completely.

    gcc/fortran/ChangeLog:
            PR fortran/37336
            * trans-array.cc (structure_alloc_comps): Deref coarray.
            (gfc_trans_deferred_array): Add freeing of components after
            check for allocated coarray.

    gcc/testsuite/ChangeLog:
            PR fortran/37336
            * gfortran.dg/coarray/alloc_comp_6.f90: New test.

Comments

Paul Richard Thomas Sept. 28, 2023, 6:21 p.m. UTC | #1 
Hi Andre,

The patch looks fine to me. Since you mention it in the comment, is it
worth declaring the derived type 'foo' in a module and giving it a
final routine?

Thanks for the patch.

Paul

On Thu, 28 Sept 2023 at 13:45, Andre Vehreschild via Fortran
<fortran@gcc.gnu.org> wrote:
>
> Hi all,
>
> attached patch fixes a crash in coarray programs when an allocatable derived
> typed coarray was freed explicitly. The generated cleanup code did not take
> into account, that the coarray may have been deallocated already. The patch
> fixes this by moving the statements accessing components inside the derived type
> into the block guard by its allocated check.
>
> Regtested ok on f37/x86_64. Ok for master?
>
> Regards,
>         Andre
> --
> Andre Vehreschild * Email: vehre ad gmx dot de
Andre Vehreschild Sept. 29, 2023, 10:01 a.m. UTC | #2 
Hi Paul,

thanks for the quick review. I've added a testcase with a module and a
finalizer in the derived type. This also is no problem.

Regtests ok on x86_64_linux_gnu/f37. Ok for trunk?

Regards,
	Andre

On Thu, 28 Sep 2023 19:21:12 +0100
Paul Richard Thomas <paul.richard.thomas@gmail.com> wrote:

> Hi Andre,
>
> The patch looks fine to me. Since you mention it in the comment, is it
> worth declaring the derived type 'foo' in a module and giving it a
> final routine?
>
> Thanks for the patch.
>
> Paul
>
> On Thu, 28 Sept 2023 at 13:45, Andre Vehreschild via Fortran
> <fortran@gcc.gnu.org> wrote:
> >
> > Hi all,
> >
> > attached patch fixes a crash in coarray programs when an allocatable derived
> > typed coarray was freed explicitly. The generated cleanup code did not take
> > into account, that the coarray may have been deallocated already. The patch
> > fixes this by moving the statements accessing components inside the derived
> > type into the block guard by its allocated check.
> >
> > Regtested ok on f37/x86_64. Ok for master?
> >
> > Regards,
> >         Andre
> > --
> > Andre Vehreschild * Email: vehre ad gmx dot de


--
Andre Vehreschild * Email: vehre ad gmx dot de
Fortran: Free alloc. comp. in allocated coarrays only.

    When freeing allocatable components of an allocatable coarray, add
    a check that the coarray is still allocated, before accessing the
    components.

    This patch adds to PR fortran/37336, but does not fix it completely.

    gcc/fortran/ChangeLog:
            PR fortran/37336
            * trans-array.cc (structure_alloc_comps): Deref coarray.
            (gfc_trans_deferred_array): Add freeing of components after
            check for allocated coarray.

    gcc/testsuite/ChangeLog:
            PR fortran/37336
            * gfortran.dg/coarray/alloc_comp_6.f90: New test.
            * gfortran.dg/coarray/alloc_comp_7.f90: New test.
Paul Richard Thomas Sept. 29, 2023, 12:38 p.m. UTC | #3 
Hi Andre,

Yes indeed - it's fine for trunk and, I would suggest, 13-branch.

Cheers

Paul

On Fri, 29 Sept 2023 at 11:01, Andre Vehreschild <vehre@gmx.de> wrote:
>
> Hi Paul,
>
> thanks for the quick review. I've added a testcase with a module and a
> finalizer in the derived type. This also is no problem.
>
> Regtests ok on x86_64_linux_gnu/f37. Ok for trunk?
>
> Regards,
>         Andre
>
> On Thu, 28 Sep 2023 19:21:12 +0100
> Paul Richard Thomas <paul.richard.thomas@gmail.com> wrote:
>
> > Hi Andre,
> >
> > The patch looks fine to me. Since you mention it in the comment, is it
> > worth declaring the derived type 'foo' in a module and giving it a
> > final routine?
> >
> > Thanks for the patch.
> >
> > Paul
> >
> > On Thu, 28 Sept 2023 at 13:45, Andre Vehreschild via Fortran
> > <fortran@gcc.gnu.org> wrote:
> > >
> > > Hi all,
> > >
> > > attached patch fixes a crash in coarray programs when an allocatable derived
> > > typed coarray was freed explicitly. The generated cleanup code did not take
> > > into account, that the coarray may have been deallocated already. The patch
> > > fixes this by moving the statements accessing components inside the derived
> > > type into the block guard by its allocated check.
> > >
> > > Regtested ok on f37/x86_64. Ok for master?
> > >
> > > Regards,
> > >         Andre
> > > --
> > > Andre Vehreschild * Email: vehre ad gmx dot de
>
>
> --
> Andre Vehreschild * Email: vehre ad gmx dot de
Andre Vehreschild Sept. 29, 2023, 1:13 p.m. UTC | #4 
Hi Paul,

thanks. Commit to trunk as a680274616ec6b26ccfdcee400ed7f54e341d40c
and backported to gcc-13 as d9b3269bdccac2db9200303494c4e82f2aeb7bbc

Thanks for the fast review.

Regards,
	Andre

On Fri, 29 Sep 2023 13:38:57 +0100
Paul Richard Thomas <paul.richard.thomas@gmail.com> wrote:

> Hi Andre,
>
> Yes indeed - it's fine for trunk and, I would suggest, 13-branch.
>
> Cheers
>
> Paul
>
> On Fri, 29 Sept 2023 at 11:01, Andre Vehreschild <vehre@gmx.de> wrote:
> >
> > Hi Paul,
> >
> > thanks for the quick review. I've added a testcase with a module and a
> > finalizer in the derived type. This also is no problem.
> >
> > Regtests ok on x86_64_linux_gnu/f37. Ok for trunk?
> >
> > Regards,
> >         Andre
> >
> > On Thu, 28 Sep 2023 19:21:12 +0100
> > Paul Richard Thomas <paul.richard.thomas@gmail.com> wrote:
> >
> > > Hi Andre,
> > >
> > > The patch looks fine to me. Since you mention it in the comment, is it
> > > worth declaring the derived type 'foo' in a module and giving it a
> > > final routine?
> > >
> > > Thanks for the patch.
> > >
> > > Paul
> > >
> > > On Thu, 28 Sept 2023 at 13:45, Andre Vehreschild via Fortran
> > > <fortran@gcc.gnu.org> wrote:
> > > >
> > > > Hi all,
> > > >
> > > > attached patch fixes a crash in coarray programs when an allocatable
> > > > derived typed coarray was freed explicitly. The generated cleanup code
> > > > did not take into account, that the coarray may have been deallocated
> > > > already. The patch fixes this by moving the statements accessing
> > > > components inside the derived type into the block guard by its
> > > > allocated check.
> > > >
> > > > Regtested ok on f37/x86_64. Ok for master?
> > > >
> > > > Regards,
> > > >         Andre
> > > > --
> > > > Andre Vehreschild * Email: vehre ad gmx dot de
> >
> >
> > --
> > Andre Vehreschild * Email: vehre ad gmx dot de


--
Andre Vehreschild * Email: vehre ad gmx dot de
Andre Vehreschild Sept. 30, 2023, 1:16 p.m. UTC | #5 
Hi all,

back porting to gcc-13 unfortunately caused a regression due to
gfc_deallocate_with_status() having a different parameter count. This is fixed
as obvious by 874b895fffd921659b37dc05bc94eea48e9a0157.

Sorry for breaking gfortran-13. I still don't know why it checkout fine on my
system in the beginning. I must have done something wrong.

Please accept my apologies and regards,
	Andre

On Fri, 29 Sep 2023 15:13:56 +0200
Andre Vehreschild via Fortran <fortran@gcc.gnu.org> wrote:

> Hi Paul,
>
> thanks. Commit to trunk as a680274616ec6b26ccfdcee400ed7f54e341d40c
> and backported to gcc-13 as d9b3269bdccac2db9200303494c4e82f2aeb7bbc
>
> Thanks for the fast review.
>
> Regards,
> 	Andre
>
> On Fri, 29 Sep 2023 13:38:57 +0100
> Paul Richard Thomas <paul.richard.thomas@gmail.com> wrote:
>
> > Hi Andre,
> >
> > Yes indeed - it's fine for trunk and, I would suggest, 13-branch.
> >
> > Cheers
> >
> > Paul
> >
> > On Fri, 29 Sept 2023 at 11:01, Andre Vehreschild <vehre@gmx.de> wrote:
> > >
> > > Hi Paul,
> > >
> > > thanks for the quick review. I've added a testcase with a module and a
> > > finalizer in the derived type. This also is no problem.
> > >
> > > Regtests ok on x86_64_linux_gnu/f37. Ok for trunk?
> > >
> > > Regards,
> > >         Andre
> > >
> > > On Thu, 28 Sep 2023 19:21:12 +0100
> > > Paul Richard Thomas <paul.richard.thomas@gmail.com> wrote:
> > >
> > > > Hi Andre,
> > > >
> > > > The patch looks fine to me. Since you mention it in the comment, is it
> > > > worth declaring the derived type 'foo' in a module and giving it a
> > > > final routine?
> > > >
> > > > Thanks for the patch.
> > > >
> > > > Paul
> > > >
> > > > On Thu, 28 Sept 2023 at 13:45, Andre Vehreschild via Fortran
> > > > <fortran@gcc.gnu.org> wrote:
> > > > >
> > > > > Hi all,
> > > > >
> > > > > attached patch fixes a crash in coarray programs when an allocatable
> > > > > derived typed coarray was freed explicitly. The generated cleanup code
> > > > > did not take into account, that the coarray may have been deallocated
> > > > > already. The patch fixes this by moving the statements accessing
> > > > > components inside the derived type into the block guard by its
> > > > > allocated check.
> > > > >
> > > > > Regtested ok on f37/x86_64. Ok for master?
> > > > >
> > > > > Regards,
> > > > >         Andre
> > > > > --
> > > > > Andre Vehreschild * Email: vehre ad gmx dot de
> > >
> > >
> > > --
> > > Andre Vehreschild * Email: vehre ad gmx dot de
>
>
> --
> Andre Vehreschild * Email: vehre ad gmx dot de


--
Andre Vehreschild * Email: vehre ad gmx dot de
Paul Richard Thomas Oct. 1, 2023, 7:38 p.m. UTC | #6 
Hi Andre,

All is well that ends well! Thanks for working on this.

Regards

Paul

On Sat, 30 Sept 2023 at 14:16, Andre Vehreschild <vehre@gmx.de> wrote:
>
> Hi all,
>
> back porting to gcc-13 unfortunately caused a regression due to
> gfc_deallocate_with_status() having a different parameter count. This is fixed
> as obvious by 874b895fffd921659b37dc05bc94eea48e9a0157.
>
> Sorry for breaking gfortran-13. I still don't know why it checkout fine on my
> system in the beginning. I must have done something wrong.
>
> Please accept my apologies and regards,
>         Andre
>
> On Fri, 29 Sep 2023 15:13:56 +0200
> Andre Vehreschild via Fortran <fortran@gcc.gnu.org> wrote:
>
> > Hi Paul,
> >
> > thanks. Commit to trunk as a680274616ec6b26ccfdcee400ed7f54e341d40c
> > and backported to gcc-13 as d9b3269bdccac2db9200303494c4e82f2aeb7bbc
> >
> > Thanks for the fast review.
> >
> > Regards,
> >       Andre
> >
> > On Fri, 29 Sep 2023 13:38:57 +0100
> > Paul Richard Thomas <paul.richard.thomas@gmail.com> wrote:
> >
> > > Hi Andre,
> > >
> > > Yes indeed - it's fine for trunk and, I would suggest, 13-branch.
> > >
> > > Cheers
> > >
> > > Paul
> > >
> > > On Fri, 29 Sept 2023 at 11:01, Andre Vehreschild <vehre@gmx.de> wrote:
> > > >
> > > > Hi Paul,
> > > >
> > > > thanks for the quick review. I've added a testcase with a module and a
> > > > finalizer in the derived type. This also is no problem.
> > > >
> > > > Regtests ok on x86_64_linux_gnu/f37. Ok for trunk?
> > > >
> > > > Regards,
> > > >         Andre
> > > >
> > > > On Thu, 28 Sep 2023 19:21:12 +0100
> > > > Paul Richard Thomas <paul.richard.thomas@gmail.com> wrote:
> > > >
> > > > > Hi Andre,
> > > > >
> > > > > The patch looks fine to me. Since you mention it in the comment, is it
> > > > > worth declaring the derived type 'foo' in a module and giving it a
> > > > > final routine?
> > > > >
> > > > > Thanks for the patch.
> > > > >
> > > > > Paul
> > > > >
> > > > > On Thu, 28 Sept 2023 at 13:45, Andre Vehreschild via Fortran
> > > > > <fortran@gcc.gnu.org> wrote:
> > > > > >
> > > > > > Hi all,
> > > > > >
> > > > > > attached patch fixes a crash in coarray programs when an allocatable
> > > > > > derived typed coarray was freed explicitly. The generated cleanup code
> > > > > > did not take into account, that the coarray may have been deallocated
> > > > > > already. The patch fixes this by moving the statements accessing
> > > > > > components inside the derived type into the block guard by its
> > > > > > allocated check.
> > > > > >
> > > > > > Regtested ok on f37/x86_64. Ok for master?
> > > > > >
> > > > > > Regards,
> > > > > >         Andre
> > > > > > --
> > > > > > Andre Vehreschild * Email: vehre ad gmx dot de
> > > >
> > > >
> > > > --
> > > > Andre Vehreschild * Email: vehre ad gmx dot de
> >
> >
> > --
> > Andre Vehreschild * Email: vehre ad gmx dot de
>
>
> --
> Andre Vehreschild * Email: vehre ad gmx dot de
diff mbox series

Patch

diff --git a/gcc/fortran/trans-array.cc b/gcc/fortran/trans-array.cc
index e0fc8ebc46b..8e94a9a469f 100644
--- a/gcc/fortran/trans-array.cc
+++ b/gcc/fortran/trans-array.cc
@@ -9320,6 +9320,12 @@  structure_alloc_comps (gfc_symbol * der_type, tree decl, tree dest,
       gfc_add_expr_to_block (&fnblock, tmp);
     }

+  /* Still having a descriptor array of rank == 0 here, indicates an
+     allocatable coarrays.  Dereference it correctly.  */
+  if (GFC_DESCRIPTOR_TYPE_P (decl_type))
+    {
+      decl = build_fold_indirect_ref (gfc_conv_array_data (decl));
+    }
   /* Otherwise, act on the components or recursively call self to
      act on a chain of components.  */
   for (c = der_type->components; c; c = c->next)
@@ -11507,7 +11513,11 @@  gfc_trans_deferred_array (gfc_symbol * sym, gfc_wrapped_block * block)
     {
       int rank;
       rank = sym->as ? sym->as->rank : 0;
-      tmp = gfc_deallocate_alloc_comp (sym->ts.u.derived, descriptor, rank);
+      tmp = gfc_deallocate_alloc_comp (sym->ts.u.derived, descriptor, rank,
+				       (sym->attr.codimension
+					&& flag_coarray == GFC_FCOARRAY_LIB)
+				       ? GFC_STRUCTURE_CAF_MODE_IN_COARRAY
+				       : 0);
       gfc_add_expr_to_block (&cleanup, tmp);
     }

@@ -11521,9 +11531,11 @@  gfc_trans_deferred_array (gfc_symbol * sym, gfc_wrapped_block * block)
 					NULL_TREE, NULL_TREE, true, e,
 					sym->attr.codimension
 					? GFC_CAF_COARRAY_DEREGISTER
-					: GFC_CAF_COARRAY_NOCOARRAY);
+					: GFC_CAF_COARRAY_NOCOARRAY,
+					NULL_TREE, gfc_finish_block (&cleanup));
       if (e)
 	gfc_free_expr (e);
+      gfc_init_block (&cleanup);
       gfc_add_expr_to_block (&cleanup, tmp);
     }

diff --git a/gcc/testsuite/gfortran.dg/coarray/alloc_comp_6.f90 b/gcc/testsuite/gfortran.dg/coarray/alloc_comp_6.f90
new file mode 100644
index 00000000000..e8a74db2c18
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/coarray/alloc_comp_6.f90
@@ -0,0 +1,29 @@ 
+! { dg-do run }
+
+program alloc_comp_6
+
+  implicit none
+
+  type :: foo
+    real :: x
+    integer, allocatable :: y(:)
+  end type
+
+  call check()
+
+contains
+
+  subroutine check()
+    block
+      type(foo), allocatable :: example[:] ! needs to be a coarray
+
+      allocate(example[*])
+      allocate(example%y(10))
+      example%x = 3.4
+      example%y = 4
+
+      deallocate(example)
+    end block  ! example%y shall not be accessed here by the finalizer,
+               ! because example is already deallocated
+  end subroutine check
+end program alloc_comp_6