Re: [PATCH 8/N] Fix memory leak in tree-vectorizer.h

[Richard Biener <richard.guenther@gmail.com>]

On Tue, Dec 8, 2015 at 12:30 PM, Martin Liška <mliska@suse.cz> wrote:
> Hello.
>
> The patch removes memory leaks that are caused by overwriting an existing
> item in stmt_vec_info_vec (in set_vinfo_for_stmt). My first attempt was to call
> free_stmt_vec_info for old entries that are overwritten, but it caused double frees
> as there are some references between stmt_vec_infos.
>
> So that I've decided to allocate all stmt_vec_info structures from a memory pool, which
> is released in free_stmt_vec_info_vec routine. Replacing 'vec' (used for simd_clone_info and same_align_regs) to 'auto_vec'
> helps to reduce another leaks. To be honest, the solution is not ideal as destructor
> of there auto_vec is not called, however with the patch applied, there is just a single memory leak
> in the whole test-suite related to tree-vect-stmts.c (which is unrelated to these vectors).
>
> Patch can bootstrap and survives regression tests on x86_64-unknown-linux-gnu.
>
> Ready for trunk?

 new_stmt_vec_info (gimple *stmt, vec_info *vinfo)
 {
   stmt_vec_info res;
-  res = (stmt_vec_info) xcalloc (1, sizeof (struct _stmt_vec_info));
+  res = stmt_vec_info_pool.allocate ();

previously it was zeroed memory now it is no longer (AFAIK).

ICK.  You do

+struct _stmt_vec_info
+{
+  _stmt_vec_info (): type ((enum stmt_vec_info_type) 0), live (false),
+  in_pattern_p (false), stmt (NULL), vinfo (0), vectype (NULL_TREE),
+  vectorized_stmt (NULL), data_ref_info (0), dr_base_address (NULL_TREE),
+  dr_init (NULL_TREE), dr_offset (NULL_TREE), dr_step (NULL_TREE),
+  dr_aligned_to (NULL_TREE), loop_phi_evolution_base_unchanged (NULL_TREE),
+  loop_phi_evolution_part (NULL_TREE), related_stmt (NULL),
pattern_def_seq (0),
+  same_align_refs (0), simd_clone_info (0), def_type ((enum vect_def_type) 0),
+  slp_type ((enum slp_vect_type) 0), first_element (NULL), next_element (NULL),
+  same_dr_stmt (NULL), size (0), store_count (0), gap (0), min_neg_dist (0),
+  relevant ((enum vect_relevant) 0), vectorizable (false),
+  gather_scatter_p (false), strided_p (false), simd_lane_access_p (false),
+  v_reduc_type ((enum vect_reduction_type) 0) {}

where I think a

  _stmt_vec_info () { memset (this, 0, sizeof (_stmt_vec_info)); }

would be much nicer.  Or just keep the struct as a POD and add that memset
at the allocation point.  (and double-check the C++ alloc-pool doesn't end up
zeroing everything twice that way...)

Note that overwriting an existing entry in stmt_vec_info_vec should not happen.
In which path does that it happen?  We probably should assert the entry
is NULL.

Thus your fix shouldn't be necessary.

Thanks,
Richard.


> Martin