This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: Memory leak in ifcvt.c
Updated as richard has requested.
2008-06-30 Kenneth Zadeck <zadeck@naturalbridge.com>
* df-scan.c (df_scan_free_ref_vec, df_scan_free_mws_vec): New
macros.
(df_scan_free_internal): Free data structures not
allocated in storage pools.
(df_mw_hardreg_chain_delete_eq_uses): Use df_scan_free_mws_vec.
(df_refs_add_to_chains): Use df_scan_free_ref_vec and
df_scan_free_mws_vec.
* dse.c (dse_step6): Free offset_map_p and offset_map_n
unconditionally.
2008-06-30 Kenneth Zadeck <zadeck@naturalbridge.com>
* ifcvt.c (cond_move_process_if_block): Free vectors on false
return.
Committed as revisions 137284 and 137285.
Kenny
Richard Guenther wrote:
On Mon, Jun 30, 2008 at 2:29 PM, Kenneth Zadeck
<zadeck@naturalbridge.com> wrote:
These two patches fix 4 storage leaks. Three of them are related to df.
The other I just fixed because dannyb tortured me. They are all fairly
obvious problems once you see the valgrind memory report.
I must say that valgrind --leak-check=full is somewhat frustrating to use.
I do not know what the "full" parameter is supposed to mean. When you fix a
storage leak and retest, valgrind seems to just find more storage leaks that
were unrelated to the ones that it reports. I just kept fixing ones in
parts that I knew about until there were no more that were obviously mine.
This DOES NOT mean that there are not any more that are mine, it simply
means that there are no more that are mine in the first few that it is
willing to report.
Ok for commit?
Kenny
2008-06-30 Kenneth Zadeck <zadeck@naturalbridge.com>
* df-scan.c (df_scan_free_internal): Free data structures not
allocated in storage pools.
* dse.c (dse_step6): Free offset_map_p and offset_map_n
unconditionally.
This is ok if you add a helper-function with comments for the
idiom
+ if (r && *r)
+ free (r);
otherwise that's pretty confusing to me ;)
2008-06-30 Kenneth Zadeck <zadeck@naturalbridge.com>
* ifcvt.c (cond_move_process_if_block): Free vectors on false
return.
This is ok.
Thanks,
Richard.
Daniel Berlin wrote:
The way i saw it was with the attached file running valgrind
--leak-check=full ./cc1 -O2 regex.i
(It also shows df leaks memory, i sent the trace to stevenb)
I only noticed it while i was debugging something else.
On Sat, Jun 28, 2008 at 7:40 PM, Kenneth Zadeck
<zadeck@naturalbridge.com> wrote:
Daniel Berlin wrote:
Kenny, I think you were the one who touched this file last, i could be
wrong.
Anyway, cond_move_process_if_block leaks allocated vectors (about
5-10k memory per function compiled).
WHat happens is this:
check_cond_move_block pushes things onto a heap allocated vector with
vec_safe_push.
Sometimes, it decides it can't do a conditional move after pushing
things onto the vector, and returns false.
Which causes cond_move_process_if to return FALSE here:
/* Make sure the blocks are suitable. */
if (!check_cond_move_block (then_bb, then_vals, then_regs, cond)
|| (else_bb && !check_cond_move_block (else_bb, else_vals,
else_regs, cond)))
return FALSE;
However, the *only* place that then_regs and else_regs are freed is at
the bottom of cond_move_process_if right before it returns TRUE.
So when it returns false, it leaks the reg vectors.
Simple fix is to free them before we return false (note that this also
means you have to split up the conditional above, since the
conditional will leak then_regs if it returns false because of the
second part.
--Dan
i will fix this, thanks.
is there a particular test case or platform?
kenny
Index: df-scan.c
===================================================================
--- df-scan.c (revision 137283)
+++ df-scan.c (working copy)
@@ -60,6 +60,21 @@ along with GCC; see the file COPYING3.
#define EPILOGUE_USES(REGNO) 0
#endif
+/* The following two macros free the vecs that hold either the refs or
+ the mw refs. They are a little tricky because the vec has 0
+ elements is special and is not to be freed. */
+#define df_scan_free_ref_vec(V) \
+ do { \
+ if (V && *V) \
+ free (V); \
+ } while (0)
+
+#define df_scan_free_mws_vec(V) \
+ do { \
+ if (V && *V) \
+ free (V); \
+ } while (0)
+
/* The bitmap_obstack is used to hold some static variables that
should not be reset after each function is compiled. */
@@ -174,11 +189,43 @@ struct df_scan_problem_data
typedef struct df_scan_bb_info *df_scan_bb_info_t;
+
+/* Internal function to shut down the scanning problem. */
static void
df_scan_free_internal (void)
{
struct df_scan_problem_data *problem_data
= (struct df_scan_problem_data *) df_scan->problem_data;
+ unsigned int i;
+ basic_block bb;
+
+ /* The vectors that hold the refs are not pool allocated because
+ they come in many sizes. This makes them impossible to delete
+ all at once. */
+ for (i = 0; i < DF_INSN_SIZE(); i++)
+ {
+ struct df_insn_info *insn_info = DF_INSN_UID_GET(i);
+ /* Skip the insns that have no insn_info or have been
+ deleted. */
+ if (insn_info)
+ {
+ df_scan_free_ref_vec (insn_info->defs);
+ df_scan_free_ref_vec (insn_info->uses);
+ df_scan_free_ref_vec (insn_info->eq_uses);
+ df_scan_free_mws_vec (insn_info->mw_hardregs);
+ }
+ }
+
+ FOR_ALL_BB (bb)
+ {
+ unsigned int bb_index = bb->index;
+ struct df_scan_bb_info *bb_info = df_scan_get_bb_info (bb_index);
+ if (bb_info)
+ {
+ df_scan_free_ref_vec (bb_info->artificial_defs);
+ df_scan_free_ref_vec (bb_info->artificial_uses);
+ }
+ }
free (df->def_info.refs);
free (df->def_info.begin);
@@ -1977,7 +2024,7 @@ df_mw_hardreg_chain_delete_eq_uses (stru
if (count == 0)
{
- free (insn_info->mw_hardregs);
+ df_scan_free_mws_vec (insn_info->mw_hardregs);
insn_info->mw_hardregs = df_null_mw_rec;
return 0;
}
@@ -2515,8 +2562,7 @@ df_refs_add_to_chains (struct df_collect
chain specially. */
if (collection_rec->def_vec)
{
- if (insn_rec->defs && *insn_rec->defs)
- free (insn_rec->defs);
+ df_scan_free_ref_vec (insn_rec->defs);
insn_rec->defs
= df_install_refs (bb, collection_rec->def_vec,
collection_rec->next_def,
@@ -2525,8 +2571,7 @@ df_refs_add_to_chains (struct df_collect
}
if (collection_rec->use_vec)
{
- if (insn_rec->uses && *insn_rec->uses)
- free (insn_rec->uses);
+ df_scan_free_ref_vec (insn_rec->uses);
insn_rec->uses
= df_install_refs (bb, collection_rec->use_vec,
collection_rec->next_use,
@@ -2535,8 +2580,7 @@ df_refs_add_to_chains (struct df_collect
}
if (collection_rec->eq_use_vec)
{
- if (insn_rec->eq_uses && *insn_rec->eq_uses)
- free (insn_rec->eq_uses);
+ df_scan_free_ref_vec (insn_rec->eq_uses);
insn_rec->eq_uses
= df_install_refs (bb, collection_rec->eq_use_vec,
collection_rec->next_eq_use,
@@ -2545,8 +2589,7 @@ df_refs_add_to_chains (struct df_collect
}
if (collection_rec->mw_vec)
{
- if (insn_rec->mw_hardregs && *insn_rec->mw_hardregs)
- free (insn_rec->mw_hardregs);
+ df_scan_free_mws_vec (insn_rec->mw_hardregs);
insn_rec->mw_hardregs
= df_install_mws (collection_rec->mw_vec,
collection_rec->next_mw);
@@ -2556,15 +2599,13 @@ df_refs_add_to_chains (struct df_collect
{
struct df_scan_bb_info *bb_info = df_scan_get_bb_info (bb->index);
- if (bb_info->artificial_defs && *bb_info->artificial_defs)
- free (bb_info->artificial_defs);
+ df_scan_free_ref_vec (bb_info->artificial_defs);
bb_info->artificial_defs
= df_install_refs (bb, collection_rec->def_vec,
collection_rec->next_def,
df->def_regs,
&df->def_info, false);
- if (bb_info->artificial_uses && *bb_info->artificial_uses)
- free (bb_info->artificial_uses);
+ df_scan_free_ref_vec (bb_info->artificial_uses);
bb_info->artificial_uses
= df_install_refs (bb, collection_rec->use_vec,
collection_rec->next_use,
Index: dse.c
===================================================================
--- dse.c (revision 137283)
+++ dse.c (working copy)
@@ -3156,43 +3156,30 @@ dse_step6 (bool global_done)
group_info_t group;
basic_block bb;
- if (global_done)
- {
- for (i = 0; VEC_iterate (group_info_t, rtx_group_vec, i, group); i++)
- {
- free (group->offset_map_n);
- free (group->offset_map_p);
- BITMAP_FREE (group->store1_n);
- BITMAP_FREE (group->store1_p);
- BITMAP_FREE (group->store2_n);
- BITMAP_FREE (group->store2_p);
- BITMAP_FREE (group->group_kill);
- }
-
- FOR_ALL_BB (bb)
- {
- bb_info_t bb_info = bb_table[bb->index];
- BITMAP_FREE (bb_info->gen);
- if (bb_info->kill)
- BITMAP_FREE (bb_info->kill);
- if (bb_info->in)
- BITMAP_FREE (bb_info->in);
- if (bb_info->out)
- BITMAP_FREE (bb_info->out);
- }
- }
- else
+ for (i = 0; VEC_iterate (group_info_t, rtx_group_vec, i, group); i++)
{
- for (i = 0; VEC_iterate (group_info_t, rtx_group_vec, i, group); i++)
- {
- BITMAP_FREE (group->store1_n);
- BITMAP_FREE (group->store1_p);
- BITMAP_FREE (group->store2_n);
- BITMAP_FREE (group->store2_p);
- BITMAP_FREE (group->group_kill);
- }
+ free (group->offset_map_n);
+ free (group->offset_map_p);
+ BITMAP_FREE (group->store1_n);
+ BITMAP_FREE (group->store1_p);
+ BITMAP_FREE (group->store2_n);
+ BITMAP_FREE (group->store2_p);
+ BITMAP_FREE (group->group_kill);
}
+ if (global_done)
+ FOR_ALL_BB (bb)
+ {
+ bb_info_t bb_info = bb_table[bb->index];
+ BITMAP_FREE (bb_info->gen);
+ if (bb_info->kill)
+ BITMAP_FREE (bb_info->kill);
+ if (bb_info->in)
+ BITMAP_FREE (bb_info->in);
+ if (bb_info->out)
+ BITMAP_FREE (bb_info->out);
+ }
+
if (clear_alias_sets)
{
BITMAP_FREE (clear_alias_sets);
@@ -3217,7 +3204,6 @@ dse_step6 (bool global_done)
}
-
/* -------------------------------------------------------------------------
DSE
------------------------------------------------------------------------- */
Index: ifcvt.c
===================================================================
--- ifcvt.c (revision 137272)
+++ ifcvt.c (working copy)
@@ -2614,7 +2614,11 @@ cond_move_process_if_block (struct noce_
/* Make sure the blocks are suitable. */
if (!check_cond_move_block (then_bb, then_vals, then_regs, cond)
|| (else_bb && !check_cond_move_block (else_bb, else_vals, else_regs, cond)))
- return FALSE;
+ {
+ VEC_free (int, heap, then_regs);
+ VEC_free (int, heap, else_regs);
+ return FALSE;
+ }
/* Make sure the blocks can be used together. If the same register
is set in both blocks, and is not set to a constant in both
@@ -2635,7 +2639,11 @@ cond_move_process_if_block (struct noce_
if (!CONSTANT_P (then_vals[reg])
&& !CONSTANT_P (else_vals[reg])
&& !rtx_equal_p (then_vals[reg], else_vals[reg]))
- return FALSE;
+ {
+ VEC_free (int, heap, then_regs);
+ VEC_free (int, heap, else_regs);
+ return FALSE;
+ }
}
}
@@ -2649,7 +2657,11 @@ cond_move_process_if_block (struct noce_
branches, since if we convert we are going to always execute
them. */
if (c > MAX_CONDITIONAL_EXECUTE)
- return FALSE;
+ {
+ VEC_free (int, heap, then_regs);
+ VEC_free (int, heap, else_regs);
+ return FALSE;
+ }
/* Try to emit the conditional moves. First do the then block,
then do anything left in the else blocks. */
@@ -2661,11 +2673,17 @@ cond_move_process_if_block (struct noce_
then_vals, else_vals, true)))
{
end_sequence ();
+ VEC_free (int, heap, then_regs);
+ VEC_free (int, heap, else_regs);
return FALSE;
}
seq = end_ifcvt_sequence (if_info);
if (!seq)
- return FALSE;
+ {
+ VEC_free (int, heap, then_regs);
+ VEC_free (int, heap, else_regs);
+ return FALSE;
+ }
loc_insn = first_active_insn (then_bb);
if (!loc_insn)
@@ -2698,7 +2716,6 @@ cond_move_process_if_block (struct noce_
VEC_free (int, heap, then_regs);
VEC_free (int, heap, else_regs);
-
return TRUE;
}