Bug 80477 - [OOP] Polymorphic function result generates memory leak
Summary: [OOP] Polymorphic function result generates memory leak
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: fortran (show other bugs)
Version: 6.3.1
: P3 normal
Target Milestone: 8.3
Assignee: Paul Thomas
URL:
Keywords: wrong-code
Depends on:
Blocks: 86754
  Show dependency treegraph
 
Reported: 2017-04-21 04:39 UTC by Stefano Zaghi
Modified: 2019-01-26 17:23 UTC (History)
3 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2017-04-22 00:00:00


Attachments
Code test to raise the memory leaks (641 bytes, text/plain)
2017-04-21 04:39 UTC, Stefano Zaghi
Details
simple inheritance leaker (526 bytes, text/plain)
2017-04-26 08:38 UTC, Stefano Zaghi
Details
new_patch.diff (681 bytes, text/plain)
2017-04-26 11:52 UTC, paul.richard.thomas@gmail.com
Details
A fix for the PR (1.05 KB, patch)
2018-07-28 07:34 UTC, Paul Thomas
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Stefano Zaghi 2017-04-21 04:39:16 UTC
Created attachment 41241 [details]
Code test to raise the memory leaks

Dear all,
I faced with a possible serious bug: polymorphic functions (necessary to define polymorphic operators in OOP programs) generate memory leaks making OOP program not feasible.

In the attached example "polymorphic_operators_memory_leaks.f90" I show the possible bug in less than 100 lines. Essentially, the derived type "a_type_t" defines the operators "=" and "+" in a polymorphic way (in order to allow "a_type_t" to be extended while all other "agents" using "a_type_t" do not need to be changed, OOP). The assignment operator does not create memory leaks because it is subroutine that does not allocate temporary. On the contrary, the operator "+" is a function that allocates a temporary: this temporary generates the memory leaks because it is not correctly destroyed after its usage.

I checked the above conclusion with "valgrind" and it confirms that the "+" operator (function) generates the leaks. Note that the same code compiled and run with Intel Fortran (17.0.2) does not generates memory leaks and it works as expected. Moreover, is the "+" operator/function is modified disallowing polymorphic (namely changing "class(a_type_t), allocatable :: res" with "type(a_type_t) :: res" the leaks disappear, thus I could conclude that the leaks are generated only for polymorphic function results.

A word of advice: the test is designed to point out the memory leaks occurrences, running it your RAM will be quickly consumed! Please, be rapid to quickly kill it before your OS start swapping on HD. To test it with valgrind I added a little "ui": it can be executed without arguments, thus its loop will be very long (and your RAM totally consumed) or you can pass one integer argument and the loop will do the iterations you provided, e.g. "a.out 1" will execute 1 iteration loop. This is written into the code comments.

Please, consider that if this is really a bug, all serious OOP programs are indeed impossible to be done.

Thank you very much for your help.
Comment 1 Stefano Zaghi 2017-04-21 07:19:54 UTC
I forget to say that I test this issue with GNU gfortran gcc version 6.3.1 20170306 and gcc version 7.0.0 20161206 (experimental).

My best regards.
Comment 2 Stefano Zaghi 2017-04-21 12:56:02 UTC
A very kind and great Fortraner (Chris MacMackin) has just let me know that a very similar (or really the same) bug has been already reported (60913) here

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=60913

Probably my test is slightly more simple.

I read that the other bug report is dated 2014: can I conclude that such a bug will need a long time to be fixed? 

Please, consider that this is not a "polemic" question: I consider all of you my superheros! However, this bug really stops me to use OOP thus I have to decide about my future work: without GNU I have no other compilers, thus if I will decide to invest more on OOP I have probably to left Fortran, probably for Julia...

My best regards.
Comment 3 janus 2017-04-22 11:18:43 UTC
Confirmed. Here is the most reduced test case I could construct from your original example:


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))              ! generates memory leak
end


valgrind shows the following output for all gfortran versions I tried (from 4.7 to trunk):

==6131== HEAP SUMMARY:
==6131==     in use at exit: 4 bytes in 1 blocks
==6131==   total heap usage: 20 allocs, 19 frees, 12,012 bytes allocated
==6131== 
==6131== LEAK SUMMARY:
==6131==    definitely lost: 4 bytes in 1 blocks
==6131==    indirectly lost: 0 bytes in 0 blocks


I think it's indeed a duplicate of PR 60913.
Comment 4 janus 2017-04-22 11:29:46 UTC
(In reply to janus from comment #3)
> Confirmed. Here is the most reduced test case I could construct from your
> original example:

-fdump-tree-original shows the following dump for this case:


polymorphic_operators_memory_leaks ()
{
  static struct a_type_t a = {.x=1.0e+0};
  static struct a_type_t b = {.x=2.0e+0};

  {
    struct __class_a_type_m_A_type_t_a D.3528;

    D.3528 = add_a_type (&a, &b);
    assign_a_type (&a, D.3528._data);
  }
}


As you can see we generate a temporary called 'D.3528' for the polymorphic function result. All we have to do is to deallocate this temporary at the end.

When changing the function result from CLASS to TYPE, this deallocation seems to be done properly (valgrind shows no leak), and the dump looks like this:

polymorphic_operators_memory_leaks ()
{
  static struct a_type_t a = {.x=1.0e+0};
  static struct a_type_t b = {.x=2.0e+0};

  {
    struct a_type_t * D.3511;
    struct a_type_t D.3512;

    D.3511 = add_a_type (&a, &b);
    D.3512 = *D.3511;
    __builtin_free ((void *) D.3511);
    D.3511 = 0B;
    assign_a_type (&a, &D.3512);
  }
}
Comment 5 janus 2017-04-22 11:38:44 UTC
(In reply to Stefano Zaghi from comment #2)
> I read that the other bug report is dated 2014: can I conclude that such a
> bug will need a long time to be fixed? 

Not necessarily.  It just takes someone to do it ;)

Note that most gfortran developers actually sacrifice their spare time to contribute, without receiving any kind of financial reward for it.

I contributed large parts of the OOP implementation when I was a student (with some funding via GSoC). Nowadays my day job and private life keeps me from spending too much time on gfotran, but I still try to contribute the occasional bug fix if I can.
Comment 6 Stefano Zaghi 2017-04-22 14:43:07 UTC
Dear Janus,

thank you very much for your help, it is really appreciated.

> Note that most gfortran developers actually sacrifice their spare time to  contribute, without receiving any kind of financial reward for it.

> I contributed large parts of the OOP implementation when I was a student (with some funding via GSoC). Nowadays my day job and private life keeps me from spending too much time on gfotran, but I still try to contribute the occasional bug fix if I can.

As I tried to clarify to Steve, mine was absolutely not a polemic question: although my contributions are definitely not equal to yours, I am also an active FOSS developer doing things for the community spending my free time, I clearly understand the FOSS developing effort. 

What I meant was "because the bug is known from 2014, can I conclude that there are not any developer who takes care (for time/interest lacking) about this in a reasonable time?". It is just a hint-request about a roadmap (I have to decide how to progress my work, gnu was a key feature), absolutely no critics about you, you are my superheros. My concern was that the bug was very difficult to solve and you are too few to solve it.

I am going to create a detailed report (within a github repository) taking into account many combinations: it seems that if I add an allocatable component alongside the static one, the leaks disappear (although this does not happen in a real,omplex code where both static and dynamic components are present). If it is of some help, I would like to share with you.

My best regards.
Comment 7 janus 2017-04-22 15:14:52 UTC
(In reply to Stefano Zaghi from comment #6)
> As I tried to clarify to Steve, mine was absolutely not a polemic question:

No offense taken. Asking questions is not a crime ;)

 
> What I meant was "because the bug is known from 2014, can I conclude that
> there are not any developer who takes care (for time/interest lacking) about
> this in a reasonable time?". It is just a hint-request about a roadmap

I'm sorry to disappoint you, but there simply is no roadmap and I'm not able to provide one. That's just not how things work with gfortran.

If you absolutely critically depend on some feature being available in a certain time-frame, then your best options are probably:
a) Try to implement it yourself or
b) pay someone to implement it.



> My concern was that the bug was
> very difficult to solve and you are too few to solve it.

I don't think the bug is incredibly difficult to solve. I already have a basic understanding of what's missing (see my analysis above) and I might look into fixing it soon. BUT: I simply can not give you any roadmaps or guarantees for that.

And yes, gfortran could definitely use some more manpower. If you are willing to help (or know someone who is), you're certainly welcome.


> I am going to create a detailed report (within a github repository) taking
> into account many combinations: it seems that if I add an allocatable
> component alongside the static one, the leaks disappear (although this does
> not happen in a real,omplex code where both static and dynamic components
> are present). If it is of some help, I would like to share with you.

Sure, that will probably help to understand the problem. Thanks for your efforts.
Comment 8 Stefano Zaghi 2017-04-23 05:23:20 UTC
Dear Janus,

> No offense taken. Asking questions is not a crime ;)

Good, thank you for the clarification.

> I'm sorry to disappoint you, but there simply is no roadmap and I'm not able to  provide one. That's just not how things work with gfortran.

> If you absolutely critically depend on some feature being available in a certain > time-frame, then your best options are probably:
> a) Try to implement it yourself or
> b) pay someone to implement it.

No disappointing at all: this is, indeed, the answer I was searching for taking a decision. If there is not a "timeline" for this bug, because the option a) is not up to me and option b) is currently impossible, this means that I have to left GNU if I cannot find a workaround for the leaks quickly.

> I don't think the bug is incredibly difficult to solve. I already have a basic > understanding of what's missing (see my analysis above) and I might look into  fixing it soon. BUT: I simply can not give you any roadmaps or guarantees for that.

> And yes, gfortran could definitely use some more manpower. If you are willing to help (or know someone who is), you're certainly welcome.

I really would like to help, but I am not up to the task (I never go over tutorial-level with C). Anyhow, if you can share your idea about the issue is placed (files involved, rationale...) it could be a starting a point: I have a colleague well-versed in C, maybe in some eons I could be of some help.

> Sure, that will probably help to understand the problem. Thanks for your efforts.

You are much more than welcome. After 25 April I'll provide my report.

My best regards.
Comment 9 janus 2017-04-23 11:02:09 UTC
Anyway, getting back to the discussion of the actual bug ...

(In reply to janus from comment #4)
> When changing the function result from CLASS to TYPE, this deallocation
> seems to be done properly (valgrind shows no leak), and the dump looks like
> this:
> 
> [...]

Based on this observation, I cooked up a draft patch that tries to modify the code which does the freeing for TYPEs and make it work for CLASSes:


Index: gcc/fortran/trans-expr.c
===================================================================
--- gcc/fortran/trans-expr.c	(revision 247083)
+++ gcc/fortran/trans-expr.c	(working copy)
@@ -6131,15 +6131,26 @@ gfc_conv_procedure_call (gfc_se * se, gfc_symbol *
   /* Allocatable scalar function results must be freed and nullified
      after use. This necessitates the creation of a temporary to
      hold the result to prevent duplicate calls.  */
-  if (!byref && sym->ts.type != BT_CHARACTER
-      && sym->attr.allocatable && !sym->attr.dimension)
+  if (!byref && sym->ts.type != BT_CHARACTER)
     {
-      tmp = gfc_create_var (TREE_TYPE (se->expr), NULL);
-      gfc_add_modify (&se->pre, tmp, se->expr);
-      se->expr = tmp;
-      tmp = gfc_call_free (tmp);
-      gfc_add_expr_to_block (&post, tmp);
-      gfc_add_modify (&post, se->expr, build_int_cst (TREE_TYPE (se->expr), 0));
+      if (sym->attr.allocatable && !sym->attr.dimension)
+	{
+	  tmp = gfc_create_var (TREE_TYPE (se->expr), NULL);
+	  gfc_add_modify (&se->pre, tmp, se->expr);
+	  se->expr = tmp;
+	  gfc_add_expr_to_block (&post, gfc_call_free (tmp));
+	  gfc_add_modify (&post, tmp, build_int_cst (TREE_TYPE (tmp), 0));
+	}
+      else if (sym->ts.type == BT_CLASS && CLASS_DATA (sym)->attr.allocatable
+	       && !CLASS_DATA (sym)->attr.dimension)
+	{
+	  tmp = gfc_create_var (TREE_TYPE (se->expr), NULL);
+	  gfc_add_modify (&se->pre, tmp, se->expr);
+	  se->expr = tmp;
+	  tmp = gfc_class_data_get (tmp);
+	  gfc_add_expr_to_block (&post, gfc_call_free (tmp));
+	  gfc_add_modify (&post, tmp, build_int_cst (TREE_TYPE (tmp), 0));
+	}
     }
 
   /* If we have a pointer function, but we don't want a pointer, e.g.


Unfortunately it does not work, because it only creates a copy of the class container, but frees up the class data too early:


polymorphic_operators_memory_leaks ()
{
  static struct a_type_t a = {.x=1.0e+0};
  static struct a_type_t b = {.x=2.0e+0};

  {
    struct __class_a_type_m_A_type_t_a D.3528;
    struct __class_a_type_m_A_type_t_a D.3529;

    D.3528 = add_a_type (&a, &b);
    D.3529 = D.3528;
    __builtin_free ((void *) D.3528._data);
    D.3528._data = 0B;
    assign_a_type (&a, D.3529._data);
  }
}


I guess I could use some help here (I always tend to get a bit lost in the trans-stage). Paul, do you by chance have any idea how to handle this properly?
Comment 10 Stefano Zaghi 2017-04-24 06:38:41 UTC
Dear all,


here https://github.com/szaghi/leaks_hunter you can find my report. Into the report I shown all the test I have done, I provide the sources and the scripts I used to generate them.

As FortranFan and Francesco Salvadore pointed out, it seems that a simple workaround exists: add an allocatable into types which have only static components and have polymorphic result-functions. I'll try this workaround into the real program after 25 April.

I hope this report you for your patch.

My best regards.
Comment 11 paul.richard.thomas@gmail.com 2017-04-24 12:49:07 UTC
Hi Janus,

I'll take a look tonight. I believe, without the source in front of me, that

s/gfc_add_expr_to_block (&post, gfc_call_free
(tmp));/gfc_add_expr_to_block (&se->post, gfc_call_free (tmp));

Cheers

Paul

On 23 April 2017 at 12:02, janus at gcc dot gnu.org
<gcc-bugzilla@gcc.gnu.org> wrote:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80477
>
> janus at gcc dot gnu.org changed:
>
>            What    |Removed                     |Added
> ----------------------------------------------------------------------------
>                  CC|                            |pault at gcc dot gnu.org
>
> --- Comment #9 from janus at gcc dot gnu.org ---
> Anyway, getting back to the discussion of the actual bug ...
>
> (In reply to janus from comment #4)
>> When changing the function result from CLASS to TYPE, this deallocation
>> seems to be done properly (valgrind shows no leak), and the dump looks like
>> this:
>>
>> [...]
>
> Based on this observation, I cooked up a draft patch that tries to modify the
> code which does the freeing for TYPEs and make it work for CLASSes:
>
>
> Index: gcc/fortran/trans-expr.c
> ===================================================================
> --- gcc/fortran/trans-expr.c    (revision 247083)
> +++ gcc/fortran/trans-expr.c    (working copy)
> @@ -6131,15 +6131,26 @@ gfc_conv_procedure_call (gfc_se * se, gfc_symbol *
>    /* Allocatable scalar function results must be freed and nullified
>       after use. This necessitates the creation of a temporary to
>       hold the result to prevent duplicate calls.  */
> -  if (!byref && sym->ts.type != BT_CHARACTER
> -      && sym->attr.allocatable && !sym->attr.dimension)
> +  if (!byref && sym->ts.type != BT_CHARACTER)
>      {
> -      tmp = gfc_create_var (TREE_TYPE (se->expr), NULL);
> -      gfc_add_modify (&se->pre, tmp, se->expr);
> -      se->expr = tmp;
> -      tmp = gfc_call_free (tmp);
> -      gfc_add_expr_to_block (&post, tmp);
> -      gfc_add_modify (&post, se->expr, build_int_cst (TREE_TYPE (se->expr),
> 0));
> +      if (sym->attr.allocatable && !sym->attr.dimension)
> +       {
> +         tmp = gfc_create_var (TREE_TYPE (se->expr), NULL);
> +         gfc_add_modify (&se->pre, tmp, se->expr);
> +         se->expr = tmp;
> +         gfc_add_expr_to_block (&post, gfc_call_free (tmp));
> +         gfc_add_modify (&post, tmp, build_int_cst (TREE_TYPE (tmp), 0));
> +       }
> +      else if (sym->ts.type == BT_CLASS && CLASS_DATA (sym)->attr.allocatable
> +              && !CLASS_DATA (sym)->attr.dimension)
> +       {
> +         tmp = gfc_create_var (TREE_TYPE (se->expr), NULL);
> +         gfc_add_modify (&se->pre, tmp, se->expr);
> +         se->expr = tmp;
> +         tmp = gfc_class_data_get (tmp);
> +         gfc_add_expr_to_block (&post, gfc_call_free (tmp));
> +         gfc_add_modify (&post, tmp, build_int_cst (TREE_TYPE (tmp), 0));
> +       }
>      }
>
>    /* If we have a pointer function, but we don't want a pointer, e.g.
>
>
> Unfortunately it does not work, because it only creates a copy of the class
> container, but frees up the class data too early:
>
>
> polymorphic_operators_memory_leaks ()
> {
>   static struct a_type_t a = {.x=1.0e+0};
>   static struct a_type_t b = {.x=2.0e+0};
>
>   {
>     struct __class_a_type_m_A_type_t_a D.3528;
>     struct __class_a_type_m_A_type_t_a D.3529;
>
>     D.3528 = add_a_type (&a, &b);
>     D.3529 = D.3528;
>     __builtin_free ((void *) D.3528._data);
>     D.3528._data = 0B;
>     assign_a_type (&a, D.3529._data);
>   }
> }
>
>
> I guess I could use some help here (I always tend to get a bit lost in the
> trans-stage). Paul, do you by chance have any idea how to handle this properly?
>
> --
> You are receiving this mail because:
> You are on the CC list for the bug.
Comment 12 Stefano Zaghi 2017-04-26 08:38:29 UTC
Created attachment 41267 [details]
simple inheritance leaker
Comment 13 Stefano Zaghi 2017-04-26 08:41:09 UTC
Dear all,

I have done further test about Vipul's workaround, you can find my complete report here

https://github.com/szaghi/leaks_hunter#results

Essentially, my current conclusion is that the workaround does not work always. In particular the "simple inheritance leaker" test shows that the leak is still generated no matter the workaround is used. In this last case, I think that issues could be placed into the "child_t" finalizer: you can find the generated code here

https://github.com/szaghi/leaks_hunter#note

I hope this can help for your patch.


My best regards.
Comment 14 janus 2017-04-26 10:59:15 UTC
(In reply to paul.richard.thomas@gmail.com from comment #11)
> I'll take a look tonight. I believe, without the source in front of me, that
> 
> s/gfc_add_expr_to_block (&post, gfc_call_free
> (tmp));/gfc_add_expr_to_block (&se->post, gfc_call_free (tmp));

Thanks for the suggestion, Paul. I just tried it, but unfortunately it makes exactly zero difference, probably because gfc_conv_procedure_call has at the very end:

      gfc_add_block_to_block (&se->post, &post);
Comment 15 Stefano Zaghi 2017-04-26 11:03:01 UTC
Dear all,

I add that the workaround (inserting an allocatable inside the type being a result of polymorphic function) if used into a real code (https://github.com/szaghi/FORESEER) does not solve the memory leak and generates

Error in `./exe/foreseer_test_shock_tube': free(): invalid pointer: 0x00007f04e4f80b00

I have not yet minimized this last example.

I do not know if this is of some help.

My best regards.
Comment 16 paul.richard.thomas@gmail.com 2017-04-26 11:52:42 UTC
Created attachment 41270 [details]
new_patch.diff

Hi Janus,

The attached does what you want to the testcase. For CLASS objects, it
is the data that has to be copied to a variable, that data freed and
the _data field pointed to the variable. There are about 70 failing
tests. I can continue debugging tonight or leave it to you.

Best regards

Paul

On 23 April 2017 at 12:02, janus at gcc dot gnu.org
<gcc-bugzilla@gcc.gnu.org> wrote:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80477
>
> janus at gcc dot gnu.org changed:
>
>            What    |Removed                     |Added
> ----------------------------------------------------------------------------
>                  CC|                            |pault at gcc dot gnu.org
>
> --- Comment #9 from janus at gcc dot gnu.org ---
> Anyway, getting back to the discussion of the actual bug ...
>
> (In reply to janus from comment #4)
>> When changing the function result from CLASS to TYPE, this deallocation
>> seems to be done properly (valgrind shows no leak), and the dump looks like
>> this:
>>
>> [...]
>
> Based on this observation, I cooked up a draft patch that tries to modify the
> code which does the freeing for TYPEs and make it work for CLASSes:
>
>
> Index: gcc/fortran/trans-expr.c
> ===================================================================
> --- gcc/fortran/trans-expr.c    (revision 247083)
> +++ gcc/fortran/trans-expr.c    (working copy)
> @@ -6131,15 +6131,26 @@ gfc_conv_procedure_call (gfc_se * se, gfc_symbol *
>    /* Allocatable scalar function results must be freed and nullified
>       after use. This necessitates the creation of a temporary to
>       hold the result to prevent duplicate calls.  */
> -  if (!byref && sym->ts.type != BT_CHARACTER
> -      && sym->attr.allocatable && !sym->attr.dimension)
> +  if (!byref && sym->ts.type != BT_CHARACTER)
>      {
> -      tmp = gfc_create_var (TREE_TYPE (se->expr), NULL);
> -      gfc_add_modify (&se->pre, tmp, se->expr);
> -      se->expr = tmp;
> -      tmp = gfc_call_free (tmp);
> -      gfc_add_expr_to_block (&post, tmp);
> -      gfc_add_modify (&post, se->expr, build_int_cst (TREE_TYPE (se->expr),
> 0));
> +      if (sym->attr.allocatable && !sym->attr.dimension)
> +       {
> +         tmp = gfc_create_var (TREE_TYPE (se->expr), NULL);
> +         gfc_add_modify (&se->pre, tmp, se->expr);
> +         se->expr = tmp;
> +         gfc_add_expr_to_block (&post, gfc_call_free (tmp));
> +         gfc_add_modify (&post, tmp, build_int_cst (TREE_TYPE (tmp), 0));
> +       }
> +      else if (sym->ts.type == BT_CLASS && CLASS_DATA (sym)->attr.allocatable
> +              && !CLASS_DATA (sym)->attr.dimension)
> +       {
> +         tmp = gfc_create_var (TREE_TYPE (se->expr), NULL);
> +         gfc_add_modify (&se->pre, tmp, se->expr);
> +         se->expr = tmp;
> +         tmp = gfc_class_data_get (tmp);
> +         gfc_add_expr_to_block (&post, gfc_call_free (tmp));
> +         gfc_add_modify (&post, tmp, build_int_cst (TREE_TYPE (tmp), 0));
> +       }
>      }
>
>    /* If we have a pointer function, but we don't want a pointer, e.g.
>
>
> Unfortunately it does not work, because it only creates a copy of the class
> container, but frees up the class data too early:
>
>
> polymorphic_operators_memory_leaks ()
> {
>   static struct a_type_t a = {.x=1.0e+0};
>   static struct a_type_t b = {.x=2.0e+0};
>
>   {
>     struct __class_a_type_m_A_type_t_a D.3528;
>     struct __class_a_type_m_A_type_t_a D.3529;
>
>     D.3528 = add_a_type (&a, &b);
>     D.3529 = D.3528;
>     __builtin_free ((void *) D.3528._data);
>     D.3528._data = 0B;
>     assign_a_type (&a, D.3529._data);
>   }
> }
>
>
> I guess I could use some help here (I always tend to get a bit lost in the
> trans-stage). Paul, do you by chance have any idea how to handle this properly?
>
> --
> You are receiving this mail because:
> You are on the CC list for the bug.
Comment 17 janus 2017-04-26 19:12:45 UTC
Hi Paul,

(In reply to paul.richard.thomas@gmail.com from comment #16)
> The attached does what you want to the testcase. For CLASS objects, it
> is the data that has to be copied to a variable, that data freed and
> the _data field pointed to the variable. There are about 70 failing
> tests. I can continue debugging tonight or leave it to you.

thanks a lot for the patch. I also managed to cook up a working patch this afternoon, which I think is very similar to yours, however it even has 211 failures in the testsuite :(

I will try sifting out these regressions, unless you beat me to it ...
Comment 18 janus 2017-04-26 20:41:33 UTC
(In reply to paul.richard.thomas@gmail.com from comment #16)
> The attached does what you want to the testcase. For CLASS objects, it
> is the data that has to be copied to a variable, that data freed and
> the _data field pointed to the variable. There are about 70 failing
> tests.

I actually see 52 testsuite failures with your patch, namely in the files:

FAIL: gfortran.dg/alloc_comp_class_4.f03   -O0  execution test
FAIL: gfortran.dg/class_result_1.f03   -O1  (internal compiler error)
FAIL: gfortran.dg/class_to_type_4.f90   -O0  execution test
FAIL: gfortran.dg/init_flag_15.f03   -O0  execution test
FAIL: gfortran.dg/submodule_6.f08   -O0  execution test
FAIL: gfortran.dg/typebound_operator_8.f03   -O0  execution test


It seems the patch currently fails at runtime for cases where the class variable has allocatable components, like this reduction of alloc_comp_class_4.f03:


module test_pr58586_mod
  implicit none

  type :: c
     integer, allocatable :: a
  end type

contains

  subroutine add_class_c (d)
    class(c), value :: d
  end subroutine

  class(c) function c_init2()
    allocatable :: c_init2
  end function

end module test_pr58586_mod

program test_pr58586
  use test_pr58586_mod

  call add_class_c(c_init2())

end program


The dump is:


test_pr58586 ()
{
  {
    struct __class_test_pr58586_mod_C_a D.3598;
    struct c D.3599;

    D.3598 = c_init2 ();
    D.3599 = *D.3598._data;
    __builtin_free ((void *) D.3598._data);
    D.3598._data = 0B;
    D.3598._data = &D.3599;
    add_class_c (D.3598);
    if (D.3598._data != 0B)
      {
        if (D.3598._data->a != 0B)
          {
            __builtin_free ((void *) D.3598._data->a);
            D.3598._data->a = 0B;
          }
        __builtin_free ((void *) D.3598._data);
        D.3598._data = 0B;
      }
  }
}
Comment 19 janus 2017-05-06 11:59:42 UTC
I think this PR is very much related to PR 65347 ("Final subroutine not called for function result") ...

Deallocation and finalization of function results are very similar. Both require a temporary to be generated. And IIRC we even use the finalization wrapper for deallocating polymorphic variables in other cases (even if they have no actual FINAL procedures). Such an approach would fix both PRs in one go.
Comment 20 janus 2017-05-06 12:33:21 UTC
(In reply to janus from comment #19)
> And IIRC we even use the finalization
> wrapper for deallocating polymorphic variables in other cases (even if they
> have no actual FINAL procedures).

In fact the finalization wrapper itself does not take care of deallocating the variable (since finalization also applies to non-allocatable variables), but it does deallocate any allocatable components (if existent).

Plus: For any polymorphic variable, we need to check at *runtime* whether finalization is necessary, since an extended type may have finalizers, even if the base class does not.

The typical code that is generated for the deallocation of a polymorphic variable 'c' looks like this:

      if (c._data != 0B)
        {
          if (c._vptr->_final != 0B)
            {
              {
                struct array0_t desc.0;

                desc.0.dtype = 40;
                desc.0.data = (void * restrict) c._data;
                c._vptr->_final (&desc.0, c._vptr->_size, 0);
              }
            }
          __builtin_free ((void *) c._data);
        }
Comment 21 Jakub Jelinek 2018-05-02 10:10:22 UTC
GCC 8.1 has been released.
Comment 22 Jakub Jelinek 2018-07-26 11:23:12 UTC
GCC 8.2 has been released.
Comment 23 Paul Thomas 2018-07-28 07:34:57 UTC
Created attachment 44457 [details]
A fix for the PR

I have just posted the attached fix to the list.

Should it be backported - it is very innocuous?

Paul
Comment 24 Paul Thomas 2018-08-28 11:36:24 UTC
Author: pault
Date: Tue Aug 28 11:35:52 2018
New Revision: 263916

URL: https://gcc.gnu.org/viewcvs?rev=263916&root=gcc&view=rev
Log:
2017-08-28  Paul Thomas  <pault@gcc.gnu.org>

	PR fortran/80477
	* trans-expr.c (gfc_conv_procedure_call): Allocatable class
	scalar results being passed to a derived type formal argument
	are finalized if possible. Otherwise, rely on existing code for
	deallocation. Make the deallocation of allocatable result
	components conditional on finalization not taking place. Make
	the freeing of data components after finalization conditional
	on the data being NULL.
	(gfc_trans_arrayfunc_assign): Change the gcc_assert to a
	condition to return NULL_TREE.
	(gfc_trans_assignment_1): If the assignment is class to class
	and the rhs expression must be finalized but the assignment
	is not marked as a polymorphic assignment, use the vptr copy
	function instead of gfc_trans_scalar_assign.

	PR fortran/86481
	* trans-expr.c (gfc_conv_expr_reference): Do not add the post
	block to the pre block if the expression is to be finalized.
	* trans-stmt.c (gfc_trans_allocate): If the expr3 must be
	finalized, load the post block into a finalization block and
	add it right at the end of the allocation block.

2017-08-28  Paul Thomas  <pault@gcc.gnu.org>

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

	PR fortran/86481
	* gfortran.dg/allocate_with_source_25.f90: New test.

Added:
    trunk/gcc/testsuite/gfortran.dg/allocate_with_source_25.f90
    trunk/gcc/testsuite/gfortran.dg/class_result_7.f90
    trunk/gcc/testsuite/gfortran.dg/class_result_8.f90
    trunk/gcc/testsuite/gfortran.dg/class_result_9.f90
Modified:
    trunk/gcc/fortran/ChangeLog
    trunk/gcc/fortran/trans-expr.c
    trunk/gcc/fortran/trans-stmt.c
    trunk/gcc/testsuite/ChangeLog
Comment 25 Martin Liška 2018-11-19 14:51:38 UTC
Paul: Can the bug be marked as resolved?
Comment 26 zed.three 2018-12-16 19:06:28 UTC
Thank you for looking at the finalisation stuff, Paul, it's really appreciated!

It wasn't clear to me from the patch notes if you expect the following to be fixed:

  subroutine assign_a_type(lhs, rhs)
    class(a_type_t), intent(inout) :: lhs
    class(a_type_t), intent(in)    :: rhs
    lhs%x = rhs%x
  end subroutine assign_a_type

or 

  class(a_type_t), allocatable :: c
  c = add_a_type(a, b)

These still generate memory leaks (detected using -fsanitize=address)

I'm using trunk (r267184, git bf96f3)

I've been trying to dig into the code myself, mostly as a learning exercise. Am I right in thinking that gfc_conv_procedure_call handles the whole statement? i.e. that finalisation both of the lhs and function result are (or should be) done here?
Comment 27 Paul Thomas 2019-01-26 17:23:13 UTC
(In reply to zed.three from comment #26)
> Thank you for looking at the finalisation stuff, Paul, it's really
> appreciated!
> 
> It wasn't clear to me from the patch notes if you expect the following to be
> fixed:
> 
>   subroutine assign_a_type(lhs, rhs)
>     class(a_type_t), intent(inout) :: lhs
>     class(a_type_t), intent(in)    :: rhs
>     lhs%x = rhs%x
>   end subroutine assign_a_type
> 
> or 
> 
>   class(a_type_t), allocatable :: c
>   c = add_a_type(a, b)
> 
> These still generate memory leaks (detected using -fsanitize=address)
> 
> I'm using trunk (r267184, git bf96f3)
> 
> I've been trying to dig into the code myself, mostly as a learning exercise.
> Am I right in thinking that gfc_conv_procedure_call handles the whole
> statement? i.e. that finalisation both of the lhs and function result are
> (or should be) done here?

I am unable to reproduce the memory leaks that you mention, either with -fsanitize=address or valgrind.

The statement is handled by gfc_conv_procedure_call as you say.

I am closing this PR since it is fixed on trunk.

Paul