Patchwork Fix memory leak due to destroy_loop_vec_info (PR middle-end/56461)

login
register
mail settings
Submitter Jakub Jelinek
Date March 1, 2013, 8:02 p.m.
Message ID <20130301200254.GB12913@tucnak.redhat.com>
Download mbox | patch
Permalink /patch/224435/
State New
Headers show

Comments

Jakub Jelinek - March 1, 2013, 8:02 p.m.
Hi!

This fixes leaks like
==31176== 176 bytes in 2 blocks are definitely lost in loss record 432 of 541
==31176==    at 0x4A0881C: malloc (vg_replace_malloc.c:270)
==31176==    by 0x114DF6F: xrealloc (xmalloc.c:177)
==31176==    by 0x85E0AA: void va_heap::reserve<gimple_statement_d*>(vec<gimple_statement_d*, va_heap, vl_embed>*&, unsigned int, bool) (vec.h:300)
==31176==    by 0x85DE23: vec<gimple_statement_d*, va_heap, vl_ptr>::reserve(unsigned int, bool) (vec.h:1468)
==31176==    by 0xA7961C: vec<gimple_statement_d*, va_heap, vl_ptr>::reserve_exact(unsigned int) (vec.h:1482)
==31176==    by 0xA79402: vec<gimple_statement_d*, va_heap, vl_ptr>::create(unsigned int) (vec.h:1497)
==31176==    by 0xC7189C: new_loop_vec_info(loop*) (tree-vect-loop.c:871)
==31176==    by 0xC725EB: vect_analyze_loop_form(loop*) (tree-vect-loop.c:1251)
==31176==    by 0xC71DEE: vect_analyze_loop_1(loop*) (tree-vect-loop.c:1000)
==31176==    by 0xC71F6D: vect_analyze_loop_form(loop*) (tree-vect-loop.c:1098)
==31176==    by 0xC73815: vect_analyze_loop(loop*) (tree-vect-loop.c:1764)
==31176==    by 0xC8F9EA: vectorize_loops() (tree-vectorizer.c:113)
and various others, when vect_analyze_loop_form calls destroy_loop_vec_info
with false as second argument, we leak various fields.  My understanding
is that back when clean_stmts argument has been added, the if (!clean_stmts)
switch got a copy of all the cleanups the other code path did, except for the loop
actually freeing stmt_vec_info, but already shortly after it new fields
have been added and cleanup for those has been added to just one spot instead
of two.

The patch fixes it by doing all the cleanups for !clean_stmts too in the
same code path, except for the stmt_vec_info freeing.  With -O3 -mavx
I didn't see any code differences on all of gcc.dg/vect/*.c.

Also, passing false to the second to last call to destroy_loop_vec_info
where we return NULL looks like a typo to my eyes, so I've changed that too.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2013-03-01  Jakub Jelinek  <jakub@redhat.com>

	PR middle-end/56461
	* tree-vect-loop.c (destroy_loop_vec_info): For !clean_stmts, just
	set nbbs to 0 instead of having separate code path.
	(vect_analyze_loop_form): Call destroy_loop_vec_info with true
	instead of false as last argument if returning NULL.


	Jakub
Richard Guenther - March 4, 2013, 9:58 a.m.
On Fri, 1 Mar 2013, Jakub Jelinek wrote:

> Hi!
> 
> This fixes leaks like
> ==31176== 176 bytes in 2 blocks are definitely lost in loss record 432 of 541
> ==31176==    at 0x4A0881C: malloc (vg_replace_malloc.c:270)
> ==31176==    by 0x114DF6F: xrealloc (xmalloc.c:177)
> ==31176==    by 0x85E0AA: void va_heap::reserve<gimple_statement_d*>(vec<gimple_statement_d*, va_heap, vl_embed>*&, unsigned int, bool) (vec.h:300)
> ==31176==    by 0x85DE23: vec<gimple_statement_d*, va_heap, vl_ptr>::reserve(unsigned int, bool) (vec.h:1468)
> ==31176==    by 0xA7961C: vec<gimple_statement_d*, va_heap, vl_ptr>::reserve_exact(unsigned int) (vec.h:1482)
> ==31176==    by 0xA79402: vec<gimple_statement_d*, va_heap, vl_ptr>::create(unsigned int) (vec.h:1497)
> ==31176==    by 0xC7189C: new_loop_vec_info(loop*) (tree-vect-loop.c:871)
> ==31176==    by 0xC725EB: vect_analyze_loop_form(loop*) (tree-vect-loop.c:1251)
> ==31176==    by 0xC71DEE: vect_analyze_loop_1(loop*) (tree-vect-loop.c:1000)
> ==31176==    by 0xC71F6D: vect_analyze_loop_form(loop*) (tree-vect-loop.c:1098)
> ==31176==    by 0xC73815: vect_analyze_loop(loop*) (tree-vect-loop.c:1764)
> ==31176==    by 0xC8F9EA: vectorize_loops() (tree-vectorizer.c:113)
> and various others, when vect_analyze_loop_form calls destroy_loop_vec_info
> with false as second argument, we leak various fields.  My understanding
> is that back when clean_stmts argument has been added, the if (!clean_stmts)
> switch got a copy of all the cleanups the other code path did, except for the loop
> actually freeing stmt_vec_info, but already shortly after it new fields
> have been added and cleanup for those has been added to just one spot instead
> of two.
> 
> The patch fixes it by doing all the cleanups for !clean_stmts too in the
> same code path, except for the stmt_vec_info freeing.  With -O3 -mavx
> I didn't see any code differences on all of gcc.dg/vect/*.c.
> 
> Also, passing false to the second to last call to destroy_loop_vec_info
> where we return NULL looks like a typo to my eyes, so I've changed that too.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

Ok.

Thanks,
Richard.

> 2013-03-01  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR middle-end/56461
> 	* tree-vect-loop.c (destroy_loop_vec_info): For !clean_stmts, just
> 	set nbbs to 0 instead of having separate code path.
> 	(vect_analyze_loop_form): Call destroy_loop_vec_info with true
> 	instead of false as last argument if returning NULL.
> 
> --- gcc/tree-vect-loop.c.jj	2013-02-27 22:40:24.000000000 +0100
> +++ gcc/tree-vect-loop.c	2013-03-01 11:27:53.205162019 +0100
> @@ -905,23 +905,9 @@ destroy_loop_vec_info (loop_vec_info loo
>    loop = LOOP_VINFO_LOOP (loop_vinfo);
>  
>    bbs = LOOP_VINFO_BBS (loop_vinfo);
> -  nbbs = loop->num_nodes;
> +  nbbs = clean_stmts ? loop->num_nodes : 0;
>    swapped = LOOP_VINFO_OPERANDS_SWAPPED (loop_vinfo);
>  
> -  if (!clean_stmts)
> -    {
> -      free (LOOP_VINFO_BBS (loop_vinfo));
> -      free_data_refs (LOOP_VINFO_DATAREFS (loop_vinfo));
> -      free_dependence_relations (LOOP_VINFO_DDRS (loop_vinfo));
> -      LOOP_VINFO_LOOP_NEST (loop_vinfo).release ();
> -      LOOP_VINFO_MAY_MISALIGN_STMTS (loop_vinfo).release ();
> -      LOOP_VINFO_MAY_ALIAS_DDRS (loop_vinfo).release ();
> -
> -      free (loop_vinfo);
> -      loop->aux = NULL;
> -      return;
> -    }
> -
>    for (j = 0; j < nbbs; j++)
>      {
>        basic_block bb = bbs[j];
> @@ -1244,7 +1230,7 @@ vect_analyze_loop_form (struct loop *loo
>  	dump_printf_loc (MSG_MISSED_OPTIMIZATION, vect_location,
>  			 "not vectorized: number of iterations = 0.");
>        if (inner_loop_vinfo)
> -        destroy_loop_vec_info (inner_loop_vinfo, false);
> +        destroy_loop_vec_info (inner_loop_vinfo, true);
>        return NULL;
>      }
>  
> 
> 	Jakub
> 
>

Patch

--- gcc/tree-vect-loop.c.jj	2013-02-27 22:40:24.000000000 +0100
+++ gcc/tree-vect-loop.c	2013-03-01 11:27:53.205162019 +0100
@@ -905,23 +905,9 @@  destroy_loop_vec_info (loop_vec_info loo
   loop = LOOP_VINFO_LOOP (loop_vinfo);
 
   bbs = LOOP_VINFO_BBS (loop_vinfo);
-  nbbs = loop->num_nodes;
+  nbbs = clean_stmts ? loop->num_nodes : 0;
   swapped = LOOP_VINFO_OPERANDS_SWAPPED (loop_vinfo);
 
-  if (!clean_stmts)
-    {
-      free (LOOP_VINFO_BBS (loop_vinfo));
-      free_data_refs (LOOP_VINFO_DATAREFS (loop_vinfo));
-      free_dependence_relations (LOOP_VINFO_DDRS (loop_vinfo));
-      LOOP_VINFO_LOOP_NEST (loop_vinfo).release ();
-      LOOP_VINFO_MAY_MISALIGN_STMTS (loop_vinfo).release ();
-      LOOP_VINFO_MAY_ALIAS_DDRS (loop_vinfo).release ();
-
-      free (loop_vinfo);
-      loop->aux = NULL;
-      return;
-    }
-
   for (j = 0; j < nbbs; j++)
     {
       basic_block bb = bbs[j];
@@ -1244,7 +1230,7 @@  vect_analyze_loop_form (struct loop *loo
 	dump_printf_loc (MSG_MISSED_OPTIMIZATION, vect_location,
 			 "not vectorized: number of iterations = 0.");
       if (inner_loop_vinfo)
-        destroy_loop_vec_info (inner_loop_vinfo, false);
+        destroy_loop_vec_info (inner_loop_vinfo, true);
       return NULL;
     }