[Patch, fortran] PR80477 - [OOP] Polymorphic function result generates memory leak

[Patch, fortran] PR80477 - [OOP] Polymorphic function result generates memory leak

[Paul Richard Thomas]

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

Several attempts, including mine, were made to fix this bug since it
was posted. They were all attacking the wrong place. Instead of
providing the free of the class _data as part of the call to
'add_a_type' it should be included in the post block of the argument
processing in the call to 'assign_a_type'. The comment in the patch
says the rest.

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

Paul

2017-07-27  Paul Thomas  <pault@gcc.gnu.org>

    PR fortran/80477
    * trans-expr.c (gfc_conv_procedure_call): Allocatable class
    results being passed to a derived type formal argument must
    have the _data component deallocated after use.

2017-07-27  Paul Thomas  <pault@gcc.gnu.org>

    PR fortran/80477
    * gfortran.dg/class_result_7.f90: New test.

[-- Attachment #2: submit.diff --]
[-- Type: text/x-patch, Size: 3087 bytes --]

Index: gcc/fortran/trans-expr.c
===================================================================
*** gcc/fortran/trans-expr.c	(revision 262444)
--- gcc/fortran/trans-expr.c	(working copy)
*************** gfc_conv_procedure_call (gfc_se * se, gf
*** 5360,5366 ****
  		      && e->ts.type == BT_CLASS
  		      && !CLASS_DATA (e)->attr.dimension
  		      && !CLASS_DATA (e)->attr.codimension)
! 		    parmse.expr = gfc_class_data_get (parmse.expr);
  
  		  /* Wrap scalar variable in a descriptor. We need to convert
  		     the address of a pointer back to the pointer itself before,
--- 5360,5384 ----
  		      && e->ts.type == BT_CLASS
  		      && !CLASS_DATA (e)->attr.dimension
  		      && !CLASS_DATA (e)->attr.codimension)
! 		    {
! 		      parmse.expr = gfc_class_data_get (parmse.expr);
! 		      /* The result is a class temporary, whose _data component
! 			 must be freed to avoid a memory leak.  */
! 		      if (e->expr_type == EXPR_FUNCTION
! 			  && CLASS_DATA (e)->attr.allocatable)
! 			{
! 			  tree zero;
! 			  zero = build_int_cst (TREE_TYPE (parmse.expr), 0);
! 			  tmp = fold_build2_loc (input_location, NE_EXPR,
! 						 logical_type_node,
! 						 parmse.expr, zero);
! 			  tmp = build3_v (COND_EXPR, tmp,
! 					  gfc_call_free (parmse.expr),
! 					  build_empty_stmt (input_location));
! 			  gfc_add_expr_to_block (&parmse.post, tmp);
! 			  gfc_add_modify (&parmse.post, parmse.expr, zero);
! 			}
! 		    }
  
  		  /* Wrap scalar variable in a descriptor. We need to convert
  		     the address of a pointer back to the pointer itself before,
Index: gcc/testsuite/gfortran.dg/class_result_7.f90
===================================================================
*** gcc/testsuite/gfortran.dg/class_result_7.f90	(nonexistent)
--- gcc/testsuite/gfortran.dg/class_result_7.f90	(working copy)
***************
*** 0 ****
--- 1,36 ----
+ ! { dg-do compile }
+ ! { dg-options "-fdump-tree-original" }
+ !
+ !  Test the fix for PR80477
+ !
+ ! Contributed by Stefano Zaghi  <stefano.zaghi@cnr.it>
+ !
+ module a_type_m
+    implicit none
+    type :: a_type_t
+       real :: x
+    endtype
+ contains
+    subroutine assign_a_type(lhs, rhs)
+       type(a_type_t), intent(inout) :: lhs
+       type(a_type_t), intent(in)    :: rhs
+       lhs%x = rhs%x
+    end subroutine
+ 
+    function add_a_type(lhs, rhs) result( res )
+       type(a_type_t), intent(in)  :: lhs
+       type(a_type_t), intent(in)  :: rhs
+       class(a_type_t), allocatable :: res
+       allocate (a_type_t :: res)
+       res%x = lhs%x + rhs%x
+    end function
+ end module
+ 
+ program polymorphic_operators_memory_leaks
+    use a_type_m
+    implicit none
+    type(a_type_t) :: a = a_type_t(1) , b = a_type_t(2)
+    call assign_a_type (a, add_a_type(a,b))              ! generated a memory leak
+ end
+ ! { dg-final { scan-tree-dump-times "builtin_free" 1 "original" } }
+ ! { dg-final { scan-tree-dump-times "builtin_malloc" 1 "original" } }
Index: gcc/testsuite/gfortran.dg/transfer_class_3.f90
===================================================================

Re: [Patch, fortran] PR80477 - [OOP] Polymorphic function result generates memory leak

[Janus Weil]

Hi Paul,

2018-07-28 9:32 GMT+02:00 Paul Richard Thomas <paul.richard.thomas@gmail.com>:
> Several attempts, including mine, were made to fix this bug since it
> was posted. They were all attacking the wrong place. Instead of
> providing the free of the class _data as part of the call to
> 'add_a_type' it should be included in the post block of the argument
> processing in the call to 'assign_a_type'. The comment in the patch
> says the rest.
>
> Bootstrapped and regtested on FC28/x86_64 - OK for trunk?

great that you managed to solve this one! The patch looks very good to
me, but I'm afraid two details may be missing:

1) If the type has allocatable components, those need to be freed first.
2) If the type has a finalizer, that needs to be called as well.

I believe that both points can be fixed by calling the _final
component of the vtab before freeing the class data. Should not be
hard to add, I hope (gfc_add_finalizer_call might be useful).

Thanks for your efforts ...

Cheers,
Janus

Re: [Patch, fortran] PR80477 - [OOP] Polymorphic function result generates memory leak

[Paul Richard Thomas]

Hi Janus,

Yes, you are correct about the need to call the _final component. OK -
will do :-)

PR86481? This seems to be the same problem in a different place.

I am starting to wonder if the gfc_se structure doesn't need an
additional block component; se.final, in addition to se.post. This
kind of difficulty in sequencing has come up many times before. If the
deallocations were available in the final block, this could be added
to the post block of the caller. This would be a neat solution to the
cases where finalization should be implemented but is not....

Thanks

Paul
On Sat, 28 Jul 2018 at 17:37, Janus Weil <janus@gcc.gnu.org> wrote:
>
> Hi Paul,
>
> 2018-07-28 9:32 GMT+02:00 Paul Richard Thomas <paul.richard.thomas@gmail.com>:
> > Several attempts, including mine, were made to fix this bug since it
> > was posted. They were all attacking the wrong place. Instead of
> > providing the free of the class _data as part of the call to
> > 'add_a_type' it should be included in the post block of the argument
> > processing in the call to 'assign_a_type'. The comment in the patch
> > says the rest.
> >
> > Bootstrapped and regtested on FC28/x86_64 - OK for trunk?
>
> great that you managed to solve this one! The patch looks very good to
> me, but I'm afraid two details may be missing:
>
> 1) If the type has allocatable components, those need to be freed first.
> 2) If the type has a finalizer, that needs to be called as well.
>
> I believe that both points can be fixed by calling the _final
> component of the vtab before freeing the class data. Should not be
> hard to add, I hope (gfc_add_finalizer_call might be useful).
>
> Thanks for your efforts ...
>
> Cheers,
> Janus



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

Re: [Patch, fortran] PR80477 - [OOP] Polymorphic function result generates memory leak

[Janus Weil]

Hi Paul,

> Yes, you are correct about the need to call the _final component. OK -
> will do :-)

Thanks. If it turns out to be more work than anticipated, you can
certainly go ahead and commit your patch as is, before going any
further. But I certainly hope it will be a cheap addition.


> PR86481? This seems to be the same problem in a different place.

PR 65347 is related as well.


> I am starting to wonder if the gfc_se structure doesn't need an
> additional block component; se.final, in addition to se.post. This
> kind of difficulty in sequencing has come up many times before. If the
> deallocations were available in the final block, this could be added
> to the post block of the caller. This would be a neat solution to the
> cases where finalization should be implemented but is not....

I feel like I don't have enough knowledge of this structure and its
usage to comment on this. I never really managed to get comfy with
gfc_se, gfc_ss & co :(

Cheers,
Janus



> On Sat, 28 Jul 2018 at 17:37, Janus Weil <janus@gcc.gnu.org> wrote:
>>
>> Hi Paul,
>>
>> 2018-07-28 9:32 GMT+02:00 Paul Richard Thomas <paul.richard.thomas@gmail.com>:
>> > Several attempts, including mine, were made to fix this bug since it
>> > was posted. They were all attacking the wrong place. Instead of
>> > providing the free of the class _data as part of the call to
>> > 'add_a_type' it should be included in the post block of the argument
>> > processing in the call to 'assign_a_type'. The comment in the patch
>> > says the rest.
>> >
>> > Bootstrapped and regtested on FC28/x86_64 - OK for trunk?
>>
>> great that you managed to solve this one! The patch looks very good to
>> me, but I'm afraid two details may be missing:
>>
>> 1) If the type has allocatable components, those need to be freed first.
>> 2) If the type has a finalizer, that needs to be called as well.
>>
>> I believe that both points can be fixed by calling the _final
>> component of the vtab before freeing the class data. Should not be
>> hard to add, I hope (gfc_add_finalizer_call might be useful).
>>
>> Thanks for your efforts ...
>>
>> Cheers,
>> Janus
>
>
>
> --
> "If you can't explain it simply, you don't understand it well enough"
> - Albert Einstein

Re: [Patch, fortran] PR80477 - [OOP] Polymorphic function result generates memory leak

[Janus Weil]

Hi Paul,

> > Yes, you are correct about the need to call the _final component. OK -
> > will do :-)
>
> Thanks. If it turns out to be more work than anticipated, you can
> certainly go ahead and commit your patch as is, before going any
> further. But I certainly hope it will be a cheap addition.

it seems you have not committed this patch yet? Please do! It does fix
a serious problem. The finalization stuff can be taken care of later
...

Cheers,
Janus

Re: [Patch, fortran] PR80477 - [OOP] Polymorphic function result generates memory leak

[Paul Richard Thomas]

Hi Janus,

I have suffered a temporary outage due to daytime work becoming
nightime too. I was investigating this bug a bit further. It seems to
me that the namespace of the procedures is not being resolved,
otherwise resolve_types would have been called. I could commit the
patch tonight and work some more on the PR afterwards.

Cheers

Paul

On Tue, 21 Aug 2018 at 21:23, Janus Weil <janus@gcc.gnu.org> wrote:
>
> Hi Paul,
>
> > > Yes, you are correct about the need to call the _final component. OK -
> > > will do :-)
> >
> > Thanks. If it turns out to be more work than anticipated, you can
> > certainly go ahead and commit your patch as is, before going any
> > further. But I certainly hope it will be a cheap addition.
>
> it seems you have not committed this patch yet? Please do! It does fix
> a serious problem. The finalization stuff can be taken care of later
> ...
>
> Cheers,
> Janus



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

Re: [Patch, fortran] PR80477 - [OOP] Polymorphic function result generates memory leak

[Dominique d'Humières]

Hi!

> great that you managed to solve this one! The patch looks very good to
> me, but I'm afraid two details may be missing:
>
> 1) If the type has allocatable components, those need to be freed first.
> …

PR86481?

Cheers

Dominique