Re: [PATCH] Fix two potential memory leak
Thanks,
On 2019/11/26 18:15, Jan Hubicka wrote:
>> Hi,
>>
>> On 2019/11/26 16:04, Jan Hubicka wrote:
Summary variables should be deleted at the end of write_summary.
It's first newed in generate_summary, and second newed in read_summary.
Therefore, delete the first in write_summary, delete the second in
execute.
gcc/ChangeLog:
2019-11-26 Luo Xiong Hu
* ipa-pure-const.c (pure_const_write_summary): Fix memory leak.
* ipa-sra.c (ipa_sra_write_summary): Likewise.
>>>
>>> This is not going to work with -ffat-lto-objects because in this case
>>> IPA pass is executed later. So you will need
>>> if (!flag_fat_lto_objects).
>>>
>>> I think most IPA passes just waits for compiler to exit with LTO rather
>>> than free the summary. ipa-fnsummary and ipa-prop allocate optimization
>>> summaries, too. Are those freed?
>>
>> ipa-prop is a bit different, the ipcp_transformation_sum is only created in
>> read_summary, so need only delete once after execute.
>
> It also has ipa_node_params_sum and ipa_edge_args_sum which live from
> ipa-prop analysis until after ipa-inlining.
>>
>> ipa-fnsummary also forgot to free the ipa summaries at the end of
>> write_summary. The pass pass_ipa_free_fn_summary will delete all summaries
>> in execute.
>>
>> For -ffat-lto-objects, I suppose there would be no write_summary and
>> read_summary? so summaries are also newed once and delete once? Thanks.
>>
>> Anyway, lto1 will run after cc1, which means cc1 is finished, the memory is
>> freed theoretically.
>
> With -ffat-lto-object we run full optimization pipieline at compile
> time. This means that we first write summary, then execute the pass
> (which uses the summary and frees it) and proceed by compilation.
> With -fno-fat-lto-object the pipieline is terminated after summary
> writting. By quiting compiler we free all the memory.
>
> We do have a lot of places where memory leaks this way and is freed at
> exit. I see that for jit we added sume explicit free calls, but I think
> jit is not using this path.
>
> I am not against freeing things explicitly since it makes it easier to
> identify real leaks, but we should do it systematically. Thinking of
> this case probably may even lead to some memory savings because after
> summary streaming the global stream is produced which is typically the
> biggest object streamed. So freeing it is OK, but please add check for
> !fat lto objects and also free ipa-fnsummary (there is
> ipa_free_fn_summary, ipa_free_size_summary for that) and ipa-prop
> summaries.
Verified that "if (!flag_fat_lto_objects)" is required for explicitly
deleting the summaries when built with -ffat-lto-objects.
>
> I will also look into your changes for ipa-profile tomorrow (I suppose
> you noticed this while looking into summaries for that patch :)
It's true that I am moving the speculative target summaries from cgraph.h
to ipa-profile.c, code is exploded with hundred of new lines. Simple test
case could work now with write_summary/read_summary/execute, but most spec
test cases ICEd due to ggc_grow() or ggc_collect(), I debugged the code and
found that read_summary is correct streaming in the target summaries, but some
edge's summary was GCed UNEXPECTEDLY by followed ggc_grow() or ggc_collect()
before calling ipa_profile, any clue on this, please?
Great that you take time on this!:)
ipa-profile.c
...
+/* Structure containing speculative target information from profile. */
+
+struct GTY (()) speculative_call_target
+{
+ speculative_call_target (unsigned int id, int prob)
+: target_id (id), target_probability (prob)
+ {
+ }
+
+ /* Profile_id of target obtained from profile. */
+ unsigned int target_id;
+ /* Probability that call will land in function with target_id. */
+ int target_probability;
+};
+
+class speculative_call_summary
+{
+public:
+ speculative_call_summary () : speculative_call_targets () {}
+
+ vec *speculative_call_targets;
+ ~speculative_call_summary ();
+
+ void dump (FILE *f);
+
+ /* Check whether this is a empty summary. */
+ bool is_empty ();
+};
...
Xiong Hu
> Honza
>>
>> Xiong Hu
>>
>>
>>>
>>> Honza
---
gcc/ipa-pure-const.c | 3 +++
gcc/ipa-sra.c| 5 +
2 files changed, 8 insertions(+)
diff --git a/gcc/ipa-pure-const.c b/gcc/ipa-pure-const.c
index a142e0cc8f6..89f334cedac 100644
--- a/gcc/ipa-pure-const.c
+++ b/gcc/ipa-pure-const.c
@@ -1260,6 +1260,9 @@ pure_const_write_summary (void)
}
lto_destroy_simple_output_block (ob);
+
+ delete funct_state_summaries;
+ funct_state_summaries = NULL;
}
diff --git a/gcc/ipa-sra.c b/gcc/ipa-sra.c
index c6ed0f44b87..bbc5e84edfb 100644
--- a/gcc/ipa-sra.c
+++ b/gcc/ipa-sra.c
@@ -2671,6 +2671,11 @@ ipa_sra_write_summary (void)
streamer_write_char_stream (ob->main_stream, 0);
produce_asm (ob, NULL);
Re: [PATCH] Fix two potential memory leak
> Hi, > > On 2019/11/26 16:04, Jan Hubicka wrote: > > > Summary variables should be deleted at the end of write_summary. > > > It's first newed in generate_summary, and second newed in read_summary. > > > Therefore, delete the first in write_summary, delete the second in > > > execute. > > > > > > gcc/ChangeLog: > > > > > > 2019-11-26 Luo Xiong Hu > > > > > > * ipa-pure-const.c (pure_const_write_summary): Fix memory leak. > > > * ipa-sra.c (ipa_sra_write_summary): Likewise. > > > > This is not going to work with -ffat-lto-objects because in this case > > IPA pass is executed later. So you will need > > if (!flag_fat_lto_objects). > > > > I think most IPA passes just waits for compiler to exit with LTO rather > > than free the summary. ipa-fnsummary and ipa-prop allocate optimization > > summaries, too. Are those freed? > > ipa-prop is a bit different, the ipcp_transformation_sum is only created in > read_summary, so need only delete once after execute. It also has ipa_node_params_sum and ipa_edge_args_sum which live from ipa-prop analysis until after ipa-inlining. > > ipa-fnsummary also forgot to free the ipa summaries at the end of > write_summary. The pass pass_ipa_free_fn_summary will delete all summaries > in execute. > > For -ffat-lto-objects, I suppose there would be no write_summary and > read_summary? so summaries are also newed once and delete once? Thanks. > > Anyway, lto1 will run after cc1, which means cc1 is finished, the memory is > freed theoretically. With -ffat-lto-object we run full optimization pipieline at compile time. This means that we first write summary, then execute the pass (which uses the summary and frees it) and proceed by compilation. With -fno-fat-lto-object the pipieline is terminated after summary writting. By quiting compiler we free all the memory. We do have a lot of places where memory leaks this way and is freed at exit. I see that for jit we added sume explicit free calls, but I think jit is not using this path. I am not against freeing things explicitly since it makes it easier to identify real leaks, but we should do it systematically. Thinking of this case probably may even lead to some memory savings because after summary streaming the global stream is produced which is typically the biggest object streamed. So freeing it is OK, but please add check for !fat lto objects and also free ipa-fnsummary (there is ipa_free_fn_summary, ipa_free_size_summary for that) and ipa-prop summaries. I will also look into your changes for ipa-profile tomorrow (I suppose you noticed this while looking into summaries for that patch :) Honza > > Xiong Hu > > > > > > Honza > > > --- > > > gcc/ipa-pure-const.c | 3 +++ > > > gcc/ipa-sra.c| 5 + > > > 2 files changed, 8 insertions(+) > > > > > > diff --git a/gcc/ipa-pure-const.c b/gcc/ipa-pure-const.c > > > index a142e0cc8f6..89f334cedac 100644 > > > --- a/gcc/ipa-pure-const.c > > > +++ b/gcc/ipa-pure-const.c > > > @@ -1260,6 +1260,9 @@ pure_const_write_summary (void) > > > } > > > lto_destroy_simple_output_block (ob); > > > + > > > + delete funct_state_summaries; > > > + funct_state_summaries = NULL; > > > } > > > diff --git a/gcc/ipa-sra.c b/gcc/ipa-sra.c > > > index c6ed0f44b87..bbc5e84edfb 100644 > > > --- a/gcc/ipa-sra.c > > > +++ b/gcc/ipa-sra.c > > > @@ -2671,6 +2671,11 @@ ipa_sra_write_summary (void) > > > streamer_write_char_stream (ob->main_stream, 0); > > > produce_asm (ob, NULL); > > > destroy_output_block (ob); > > > + > > > + ggc_delete (func_sums); > > > + func_sums = NULL; > > > + delete call_sums; > > > + call_sums = NULL; > > > } > > > /* Read intraprocedural analysis information about E and all of its > > > outgoing > > > -- > > > 2.21.0.777.g83232e3864 > > > >
Re: [PATCH] Fix two potential memory leak
Hi, On 2019/11/26 16:04, Jan Hubicka wrote: Summary variables should be deleted at the end of write_summary. It's first newed in generate_summary, and second newed in read_summary. Therefore, delete the first in write_summary, delete the second in execute. gcc/ChangeLog: 2019-11-26 Luo Xiong Hu * ipa-pure-const.c (pure_const_write_summary): Fix memory leak. * ipa-sra.c (ipa_sra_write_summary): Likewise. This is not going to work with -ffat-lto-objects because in this case IPA pass is executed later. So you will need if (!flag_fat_lto_objects). I think most IPA passes just waits for compiler to exit with LTO rather than free the summary. ipa-fnsummary and ipa-prop allocate optimization summaries, too. Are those freed? ipa-prop is a bit different, the ipcp_transformation_sum is only created in read_summary, so need only delete once after execute. ipa-fnsummary also forgot to free the ipa summaries at the end of write_summary. The pass pass_ipa_free_fn_summary will delete all summaries in execute. For -ffat-lto-objects, I suppose there would be no write_summary and read_summary? so summaries are also newed once and delete once? Thanks. Anyway, lto1 will run after cc1, which means cc1 is finished, the memory is freed theoretically. Xiong Hu Honza --- gcc/ipa-pure-const.c | 3 +++ gcc/ipa-sra.c| 5 + 2 files changed, 8 insertions(+) diff --git a/gcc/ipa-pure-const.c b/gcc/ipa-pure-const.c index a142e0cc8f6..89f334cedac 100644 --- a/gcc/ipa-pure-const.c +++ b/gcc/ipa-pure-const.c @@ -1260,6 +1260,9 @@ pure_const_write_summary (void) } lto_destroy_simple_output_block (ob); + + delete funct_state_summaries; + funct_state_summaries = NULL; } diff --git a/gcc/ipa-sra.c b/gcc/ipa-sra.c index c6ed0f44b87..bbc5e84edfb 100644 --- a/gcc/ipa-sra.c +++ b/gcc/ipa-sra.c @@ -2671,6 +2671,11 @@ ipa_sra_write_summary (void) streamer_write_char_stream (ob->main_stream, 0); produce_asm (ob, NULL); destroy_output_block (ob); + + ggc_delete (func_sums); + func_sums = NULL; + delete call_sums; + call_sums = NULL; } /* Read intraprocedural analysis information about E and all of its outgoing -- 2.21.0.777.g83232e3864
Re: [PATCH] Fix two potential memory leak
> Summary variables should be deleted at the end of write_summary. > It's first newed in generate_summary, and second newed in read_summary. > Therefore, delete the first in write_summary, delete the second in > execute. > > gcc/ChangeLog: > > 2019-11-26 Luo Xiong Hu > > * ipa-pure-const.c (pure_const_write_summary): Fix memory leak. > * ipa-sra.c (ipa_sra_write_summary): Likewise. This is not going to work with -ffat-lto-objects because in this case IPA pass is executed later. So you will need if (!flag_fat_lto_objects). I think most IPA passes just waits for compiler to exit with LTO rather than free the summary. ipa-fnsummary and ipa-prop allocate optimization summaries, too. Are those freed? Honza > --- > gcc/ipa-pure-const.c | 3 +++ > gcc/ipa-sra.c| 5 + > 2 files changed, 8 insertions(+) > > diff --git a/gcc/ipa-pure-const.c b/gcc/ipa-pure-const.c > index a142e0cc8f6..89f334cedac 100644 > --- a/gcc/ipa-pure-const.c > +++ b/gcc/ipa-pure-const.c > @@ -1260,6 +1260,9 @@ pure_const_write_summary (void) > } > >lto_destroy_simple_output_block (ob); > + > + delete funct_state_summaries; > + funct_state_summaries = NULL; > } > > > diff --git a/gcc/ipa-sra.c b/gcc/ipa-sra.c > index c6ed0f44b87..bbc5e84edfb 100644 > --- a/gcc/ipa-sra.c > +++ b/gcc/ipa-sra.c > @@ -2671,6 +2671,11 @@ ipa_sra_write_summary (void) >streamer_write_char_stream (ob->main_stream, 0); >produce_asm (ob, NULL); >destroy_output_block (ob); > + > + ggc_delete (func_sums); > + func_sums = NULL; > + delete call_sums; > + call_sums = NULL; > } > > /* Read intraprocedural analysis information about E and all of its outgoing > -- > 2.21.0.777.g83232e3864 >
[PATCH] Fix two potential memory leak
Summary variables should be deleted at the end of write_summary. It's first newed in generate_summary, and second newed in read_summary. Therefore, delete the first in write_summary, delete the second in execute. gcc/ChangeLog: 2019-11-26 Luo Xiong Hu * ipa-pure-const.c (pure_const_write_summary): Fix memory leak. * ipa-sra.c (ipa_sra_write_summary): Likewise. --- gcc/ipa-pure-const.c | 3 +++ gcc/ipa-sra.c| 5 + 2 files changed, 8 insertions(+) diff --git a/gcc/ipa-pure-const.c b/gcc/ipa-pure-const.c index a142e0cc8f6..89f334cedac 100644 --- a/gcc/ipa-pure-const.c +++ b/gcc/ipa-pure-const.c @@ -1260,6 +1260,9 @@ pure_const_write_summary (void) } lto_destroy_simple_output_block (ob); + + delete funct_state_summaries; + funct_state_summaries = NULL; } diff --git a/gcc/ipa-sra.c b/gcc/ipa-sra.c index c6ed0f44b87..bbc5e84edfb 100644 --- a/gcc/ipa-sra.c +++ b/gcc/ipa-sra.c @@ -2671,6 +2671,11 @@ ipa_sra_write_summary (void) streamer_write_char_stream (ob->main_stream, 0); produce_asm (ob, NULL); destroy_output_block (ob); + + ggc_delete (func_sums); + func_sums = NULL; + delete call_sums; + call_sums = NULL; } /* Read intraprocedural analysis information about E and all of its outgoing -- 2.21.0.777.g83232e3864
