[Patch, PR Fortran/90069] Polymorphic Return Type Memory Leak Without Intermediate Variable

[Patch, PR Fortran/90069] Polymorphic Return Type Memory Leak Without Intermediate Variable

[Andre Vehreschild]

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

Hi all,

the attached patch fixes a memory leak with unlimited polymorphic return types.
The leak occurred, because an expression with side-effects was evaluated twice.
I have substituted the check for non-variable expressions followed by creating a
SAVE_EXPR with checking for trees with side effects and creating temp. variable
and freeing the memory.

Btw, I do not get the SAVE_EXPR in the old code. Is there something missing to
manifest it or is a SAVE_EXPR not meant to be evaluated twice?

Anyway, regtested ok on Linux-x86_64-Fedora_39. Ok for master?

This work is funded by the Souvereign Tech Fund. Yes, the funding has been
granted and Nicolas, Mikael and me will be working on some Fortran topics in
the next 12-18 months.

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

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-memory-leak.patch --]
[-- Type: text/x-patch, Size: 4099 bytes --]

From edd6c94b802732b0dd742ef9eca4d74aaaf6d91b Mon Sep 17 00:00:00 2001
From: Andre Vehreschild <vehre@gcc.gnu.org>
Date: Wed, 12 Jul 2023 16:52:15 +0200
Subject: [PATCH] Fix memory leak.

Prevent double call of function return class object
and free the object after copy.

gcc/fortran/ChangeLog:

	PR fortran/90069
	* trans-expr.cc (gfc_conv_procedure_call): Evaluate
	expressions with side-effects only ones and ensure
	old is freeed.

gcc/testsuite/ChangeLog:

	PR fortran/90069
	* gfortran.dg/class_76.f90: New test.
---
 gcc/fortran/trans-expr.cc              | 29 +++++++++--
 gcc/testsuite/gfortran.dg/class_76.f90 | 66 ++++++++++++++++++++++++++
 2 files changed, 92 insertions(+), 3 deletions(-)
 create mode 100644 gcc/testsuite/gfortran.dg/class_76.f90

diff --git a/gcc/fortran/trans-expr.cc b/gcc/fortran/trans-expr.cc
index dfc5b8e9b4a..38ba278f725 100644
--- a/gcc/fortran/trans-expr.cc
+++ b/gcc/fortran/trans-expr.cc
@@ -6725,9 +6725,32 @@ gfc_conv_procedure_call (gfc_se * se, gfc_symbol * sym,
 			    {
 			      tree efield;

-			      /* Evaluate arguments just once.  */
-			      if (e->expr_type != EXPR_VARIABLE)
-				parmse.expr = save_expr (parmse.expr);
+			      /* Evaluate arguments just once, when they have
+			         side effects.  */
+			      if (TREE_SIDE_EFFECTS (parmse.expr))
+				{
+				  tree cldata, zero;
+
+				  parmse.expr = gfc_evaluate_now (parmse.expr,
+								  &parmse.pre);
+
+				  /* Prevent memory leak, when old component
+				     was allocated already.  */
+				  cldata = gfc_class_data_get (parmse.expr);
+				  zero = build_int_cst (TREE_TYPE (cldata),
+							0);
+				  tmp = fold_build2_loc (input_location, NE_EXPR,
+							 logical_type_node,
+							 cldata, zero);
+				  tmp = build3_v (COND_EXPR, tmp,
+						  gfc_call_free (cldata),
+						  build_empty_stmt (
+						    input_location));
+				  gfc_add_expr_to_block (&parmse.finalblock,
+							 tmp);
+				  gfc_add_modify (&parmse.finalblock,
+						  cldata, zero);
+				}

 			      /* Set the _data field.  */
 			      tmp = gfc_class_data_get (var);
diff --git a/gcc/testsuite/gfortran.dg/class_76.f90 b/gcc/testsuite/gfortran.dg/class_76.f90
new file mode 100644
index 00000000000..1ee1e1fc25f
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/class_76.f90
@@ -0,0 +1,66 @@
+! { dg-do compile }
+! { dg-additional-options "-fdump-tree-original" }
+!
+! PR fortran/90069
+!
+! Contributed by Brad Richardson  <everythingfunctional@protonmail.com>
+!
+
+program returned_memory_leak
+    implicit none
+
+    type, abstract :: base
+    end type base
+
+    type, extends(base) :: extended
+    end type extended
+
+    type :: container
+        class(*), allocatable :: thing
+    end type
+
+    call run()
+contains
+    subroutine run()
+        type(container) :: a_container
+
+        a_container = theRightWay()
+        a_container = theWrongWay()
+    end subroutine
+
+    function theRightWay()
+        type(container) :: theRightWay
+
+        class(base), allocatable :: thing
+
+        allocate(thing, source = newAbstract())
+        theRightWay = newContainer(thing)
+    end function theRightWay
+
+    function theWrongWay()
+        type(container) :: theWrongWay
+
+        theWrongWay = newContainer(newAbstract())
+    end function theWrongWay
+
+    function  newAbstract()
+        class(base), allocatable :: newAbstract
+
+        allocate(newAbstract, source = newExtended())
+    end function newAbstract
+
+    function newExtended()
+        type(extended) :: newExtended
+    end function newExtended
+
+    function newContainer(thing)
+        class(*), intent(in) :: thing
+        type(container) :: newContainer
+
+        allocate(newContainer%thing, source = thing)
+    end function newContainer
+end program returned_memory_leak
+
+! { dg-final { scan-tree-dump-times "newabstract" 14 "original" } }
+! { dg-final { scan-tree-dump-times "__builtin_free" 8 "original" } }
+
--
2.45.1

Re: [Patch, PR Fortran/90069] Polymorphic Return Type Memory Leak Without Intermediate Variable

[Harald Anlauf]

Hi Andre,

On 5/28/24 14:10, Andre Vehreschild wrote:
> Hi all,
>
> the attached patch fixes a memory leak with unlimited polymorphic return types.
> The leak occurred, because an expression with side-effects was evaluated twice.
> I have substituted the check for non-variable expressions followed by creating a
> SAVE_EXPR with checking for trees with side effects and creating temp. variable
> and freeing the memory.

this looks good to me.  It also solves the runtime memory leak in
testcase pr114012.f90 .  Nice!

> Btw, I do not get the SAVE_EXPR in the old code. Is there something missing to
> manifest it or is a SAVE_EXPR not meant to be evaluated twice?

I was assuming that the comment in gcc/tree.h applies here:

/* save_expr (EXP) returns an expression equivalent to EXP
    but it can be used multiple times within context CTX
    and only evaluate EXP once.  */

I do not know what the practical difference between a SAVE_EXPR
and a temporary explicitly evaluated once (which you have now)
is, except that you can free the temporary cleanly.

> Anyway, regtested ok on Linux-x86_64-Fedora_39. Ok for master?

Yes, this is fine from my side.  If you are inclined to backport
to e.g. 14-branch after a grace period, that would be great.

> This work is funded by the Souvereign Tech Fund. Yes, the funding has been
> granted and Nicolas, Mikael and me will be working on some Fortran topics in
> the next 12-18 months.

This is really great news!

> Regards,
> 	Andre

Thanks for the patch!

Harald

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

Re: [Patch, PR Fortran/90069] Polymorphic Return Type Memory Leak Without Intermediate Variable

[Harald Anlauf]

Hi Andre,

On 5/28/24 14:10, Andre Vehreschild wrote:
> Hi all,
> 
> the attached patch fixes a memory leak with unlimited polymorphic return types.
> The leak occurred, because an expression with side-effects was evaluated twice.
> I have substituted the check for non-variable expressions followed by creating a
> SAVE_EXPR with checking for trees with side effects and creating temp. variable
> and freeing the memory.

this looks good to me.  It also solves the runtime memory leak in 
testcase pr114012.f90 .  Nice!

> Btw, I do not get the SAVE_EXPR in the old code. Is there something missing to
> manifest it or is a SAVE_EXPR not meant to be evaluated twice?

I was assuming that the comment in gcc/tree.h applies here:

/* save_expr (EXP) returns an expression equivalent to EXP
    but it can be used multiple times within context CTX
    and only evaluate EXP once.  */

I do not know what the practical difference between a SAVE_EXPR
and a temporary explicitly evaluated once (which you have now)
is, except that you can free the temporary cleanly.

> Anyway, regtested ok on Linux-x86_64-Fedora_39. Ok for master?

Yes, this is fine from my side.  If you are inclined to backport
to e.g. 14-branch after a grace period, that would be great.

> This work is funded by the Souvereign Tech Fund. Yes, the funding has been
> granted and Nicolas, Mikael and me will be working on some Fortran topics in
> the next 12-18 months.

This is really great news!

> Regards,
> 	Andre

Thanks for the patch!

Harald

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

Re: [Patch, PR Fortran/90069] Polymorphic Return Type Memory Leak Without Intermediate Variable

[Richard Biener]

On Tue, May 28, 2024 at 9:46 PM Harald Anlauf <anlauf@gmx.de> wrote:
>
> Hi Andre,
>
> On 5/28/24 14:10, Andre Vehreschild wrote:
> > Hi all,
> >
> > the attached patch fixes a memory leak with unlimited polymorphic return types.
> > The leak occurred, because an expression with side-effects was evaluated twice.
> > I have substituted the check for non-variable expressions followed by creating a
> > SAVE_EXPR with checking for trees with side effects and creating temp. variable
> > and freeing the memory.
>
> this looks good to me.  It also solves the runtime memory leak in
> testcase pr114012.f90 .  Nice!
>
> > Btw, I do not get the SAVE_EXPR in the old code. Is there something missing to
> > manifest it or is a SAVE_EXPR not meant to be evaluated twice?
>
> I was assuming that the comment in gcc/tree.h applies here:
>
> /* save_expr (EXP) returns an expression equivalent to EXP
>     but it can be used multiple times within context CTX
>     and only evaluate EXP once.  */
>
> I do not know what the practical difference between a SAVE_EXPR
> and a temporary explicitly evaluated once (which you have now)
> is, except that you can free the temporary cleanly.

A SAVE_EXPR is turned into the latter - a temporary plus once evaluated
side-effects - by the gimplifier.  It's sometimes more convenient to use
a SAVE_EXPR but an IMO an explicit temporary is prefered.  Note
SAVE_EXPRs have to be used from the same "evaluation context",
when you use the SAVE_EXPR from two different statements it's
by chance the temporary is initialized by the earlier one and not by
the later and used uninitialized by the earlier.  That means SAVE_EXPRs
are to be used from within a single (large) expression.

> > Anyway, regtested ok on Linux-x86_64-Fedora_39. Ok for master?
>
> Yes, this is fine from my side.  If you are inclined to backport
> to e.g. 14-branch after a grace period, that would be great.
>
> > This work is funded by the Souvereign Tech Fund. Yes, the funding has been
> > granted and Nicolas, Mikael and me will be working on some Fortran topics in
> > the next 12-18 months.
>
> This is really great news!
>
> > Regards,
> >       Andre
>
> Thanks for the patch!
>
> Harald
>
> > --
> > Andre Vehreschild * Email: vehre ad gmx dot de
>

Re: [Patch, PR Fortran/90069] Polymorphic Return Type Memory Leak Without Intermediate Variable

[Andre Vehreschild]

Hi Harald,

thanks for the review. Very much appreciated.

Commited as 2f97d98d174e3ef9f3a9a83c179d787abde5e066.

I have some patches for memory leaks I will post in the next days. I am
inclined to backport them together to 14-line, if no new bugs arise.

About the SAVE_EXPR, Richard Biener shed some light. Thanks you very much for
that.

Regards,
	Andre

On Tue, 28 May 2024 21:45:56 +0200
Harald Anlauf <anlauf@gmx.de> wrote:

> Hi Andre,
>
> On 5/28/24 14:10, Andre Vehreschild wrote:
> > Hi all,
> >
> > the attached patch fixes a memory leak with unlimited polymorphic return
> > types. The leak occurred, because an expression with side-effects was
> > evaluated twice. I have substituted the check for non-variable expressions
> > followed by creating a SAVE_EXPR with checking for trees with side effects
> > and creating temp. variable and freeing the memory.
>
> this looks good to me.  It also solves the runtime memory leak in
> testcase pr114012.f90 .  Nice!
>
> > Btw, I do not get the SAVE_EXPR in the old code. Is there something missing
> > to manifest it or is a SAVE_EXPR not meant to be evaluated twice?
>
> I was assuming that the comment in gcc/tree.h applies here:
>
> /* save_expr (EXP) returns an expression equivalent to EXP
>     but it can be used multiple times within context CTX
>     and only evaluate EXP once.  */
>
> I do not know what the practical difference between a SAVE_EXPR
> and a temporary explicitly evaluated once (which you have now)
> is, except that you can free the temporary cleanly.
>
> > Anyway, regtested ok on Linux-x86_64-Fedora_39. Ok for master?
>
> Yes, this is fine from my side.  If you are inclined to backport
> to e.g. 14-branch after a grace period, that would be great.
>
> > This work is funded by the Souvereign Tech Fund. Yes, the funding has been
> > granted and Nicolas, Mikael and me will be working on some Fortran topics in
> > the next 12-18 months.
>
> This is really great news!
>
> > Regards,
> > 	Andre
>
> Thanks for the patch!
>
> Harald
>
> > --
> > Andre Vehreschild * Email: vehre ad gmx dot de
>


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