[1/6] Fix memory leak in cilk

On 11/23/2015 02:48 PM, marxin wrote:
> gcc/c/ChangeLog:
>
> 2015-11-20  Martin Liska  <mliska@suse.cz>
>
> 	PR c++/68312
> 	* c-array-notation.c (fix_builtin_array_notation_fn):
> 	Use release_vec_vec instead of vec::release.
> 	(build_array_notation_expr): Likewise.
> 	(fix_conditional_array_notations_1): Likewise.
> 	(fix_array_notation_expr): Likewise.
> 	(fix_array_notation_call_expr): Likewise.
>
> gcc/cp/ChangeLog:
>
> 2015-11-20  Martin Liska  <mliska@suse.cz>
>
> 	PR c++/68312
> 	* cp-array-notation.c (expand_sec_reduce_builtin):
> 	Likewise.
> 	(create_array_refs): Replace argument with const reference.
> 	(expand_an_in_modify_expr): Likewise.
> 	(cp_expand_cond_array_notations): Likewise.
> 	(expand_unary_array_notation_exprs): Likewise.
>
> gcc/c-family/ChangeLog:
>
> 2015-11-20  Martin Liska  <mliska@suse.cz>
>
> 	PR c++/68312
> 	* array-notation-common.c (cilkplus_extract_an_triplets):
> 	Release vector of vectors.
> 	* cilk.c (gimplify_cilk_spawn): Free allocated memory.
>
> gcc/ChangeLog:
>
> 2015-11-20  Martin Liska  <mliska@suse.cz>
>
> 	PR c++/68312
> 	* vec.h (release_vec_vec): New function.
> ---
>   gcc/c-family/array-notation-common.c |  2 ++
>   gcc/c-family/cilk.c                  |  1 +
>   gcc/c/c-array-notation.c             | 38 ++++++++++----------------
>   gcc/cp/cp-array-notation.c           | 52 ++++++++++++++++++------------------
>   gcc/vec.h                            | 12 +++++++++
>   5 files changed, 55 insertions(+), 50 deletions(-)
>
> diff --git a/gcc/c-family/array-notation-common.c b/gcc/c-family/array-notation-common.c
> index 4f7072b..5f2209d 100644
> --- a/gcc/c-family/array-notation-common.c
> +++ b/gcc/c-family/array-notation-common.c
> @@ -636,6 +636,8 @@ cilkplus_extract_an_triplets (vec<tree, va_gc> *list, size_t size, size_t rank,
>   	      fold_build1 (CONVERT_EXPR, integer_type_node,
>   			   ARRAY_NOTATION_STRIDE (ii_tree));
>   	  }
> +
> +  release_vec_vec (array_exprs);
>   }
>
>   /* Replaces all the __sec_implicit_arg functions in LIST with the induction
> diff --git a/gcc/c-family/cilk.c b/gcc/c-family/cilk.c
> index e75e20c..1167b2b 100644
> --- a/gcc/c-family/cilk.c
> +++ b/gcc/c-family/cilk.c
> @@ -844,6 +844,7 @@ gimplify_cilk_spawn (tree *spawn_p)
>   			    call2, build_empty_stmt (EXPR_LOCATION (call1)));
>     append_to_statement_list (spawn_expr, spawn_p);
>
> +  free (arg_array);
>     return GS_OK;
>   }
>
> diff --git a/gcc/c/c-array-notation.c b/gcc/c/c-array-notation.c
> index 21f8684..49f5f7b 100644
> --- a/gcc/c/c-array-notation.c
> +++ b/gcc/c/c-array-notation.c
> @@ -98,7 +98,7 @@ make_triplet_val_inv (location_t loc, tree *value)
>
>   static void
>   create_cmp_incr (location_t loc, vec<an_loop_parts> *node, size_t rank,
> -		 vec<vec<an_parts> > an_info)
> +		 const vec<vec<an_parts> > &an_info)
>   {
>     for (size_t ii = 0; ii < rank; ii++)
>       {
> @@ -122,7 +122,7 @@ create_cmp_incr (location_t loc, vec<an_loop_parts> *node, size_t rank,
>   */
>
>   static vec<tree, va_gc> *
> -create_array_refs (location_t loc, vec<vec<an_parts> > an_info,
> +create_array_refs (location_t loc, const vec<vec<an_parts> > &an_info,
>   		   vec<an_loop_parts> an_loop_info, size_t size, size_t rank)
>   {
>     tree ind_mult, ind_incr;
> @@ -205,7 +205,7 @@ fix_builtin_array_notation_fn (tree an_builtin_fn, tree *new_var)
>     location_t location = UNKNOWN_LOCATION;
>     tree loop_with_init = alloc_stmt_list ();
>     vec<vec<an_parts> > an_info = vNULL;
> -  vec<an_loop_parts> an_loop_info = vNULL;
> +  auto_vec<an_loop_parts> an_loop_info;
>     enum built_in_function an_type =
>       is_cilkplus_reduce_builtin (CALL_EXPR_FN (an_builtin_fn));
>     if (an_type == BUILT_IN_NONE)
> @@ -593,8 +593,7 @@ fix_builtin_array_notation_fn (tree an_builtin_fn, tree *new_var)
>       }
>     append_to_statement_list_force (body, &loop_with_init);
>
> -  an_info.release ();
> -  an_loop_info.release ();
> +  release_vec_vec (an_info);
>
>     return loop_with_init;
>   }
> @@ -614,7 +613,7 @@ build_array_notation_expr (location_t location, tree lhs, tree lhs_origtype,
>     tree array_expr_lhs = NULL_TREE, array_expr_rhs = NULL_TREE;
>     tree array_expr = NULL_TREE;
>     tree an_init = NULL_TREE;
> -  vec<tree> cond_expr = vNULL;
> +  auto_vec<tree> cond_expr;
>     tree body, loop_with_init = alloc_stmt_list();
>     tree scalar_mods = NULL_TREE;
>     vec<tree, va_gc> *rhs_array_operand = NULL, *lhs_array_operand = NULL;
> @@ -624,7 +623,7 @@ build_array_notation_expr (location_t location, tree lhs, tree lhs_origtype,
>     tree new_modify_expr, new_var = NULL_TREE, builtin_loop = NULL_TREE;
>     size_t rhs_list_size = 0, lhs_list_size = 0;
>     vec<vec<an_parts> > lhs_an_info = vNULL, rhs_an_info = vNULL;
> -  vec<an_loop_parts> lhs_an_loop_info = vNULL, rhs_an_loop_info = vNULL;
> +  auto_vec<an_loop_parts> lhs_an_loop_info, rhs_an_loop_info;
>
>     /* If either of this is true, an error message must have been send out
>        already.  Not necessary to send out multiple error messages.  */
> @@ -881,14 +880,9 @@ build_array_notation_expr (location_t location, tree lhs, tree lhs_origtype,
>       }
>     append_to_statement_list_force (body, &loop_with_init);
>
> -  lhs_an_info.release ();
> -  lhs_an_loop_info.release ();
> +  release_vec_vec (lhs_an_info);
>     if (rhs_rank)
> -    {
> -      rhs_an_info.release ();
> -      rhs_an_loop_info.release ();
> -    }
> -  cond_expr.release ();
> +    release_vec_vec (rhs_an_info);
>     return loop_with_init;
>   }
>
> @@ -909,7 +903,7 @@ fix_conditional_array_notations_1 (tree stmt)
>     location_t location = EXPR_LOCATION (stmt);
>     tree body = NULL_TREE, loop_with_init = alloc_stmt_list ();
>     vec<vec<an_parts> > an_info = vNULL;
> -  vec<an_loop_parts> an_loop_info = vNULL;
> +  auto_vec<an_loop_parts> an_loop_info;
>
>     if (TREE_CODE (stmt) == COND_EXPR)
>       cond = COND_EXPR_COND (stmt);
> @@ -1005,9 +999,7 @@ fix_conditional_array_notations_1 (tree stmt)
>         body = pop_stmt_list (new_loop);
>       }
>     append_to_statement_list_force (body, &loop_with_init);
> -
> -  an_loop_info.release ();
> -  an_info.release ();
> +  release_vec_vec (an_info);
>
>     return loop_with_init;
>   }
> @@ -1048,7 +1040,7 @@ fix_array_notation_expr (location_t location, enum tree_code code,
>     tree loop_init;
>     tree body, loop_with_init = alloc_stmt_list ();
>     vec<vec<an_parts> > an_info = vNULL;
> -  vec<an_loop_parts> an_loop_info = vNULL;
> +  auto_vec<an_loop_parts> an_loop_info;
>
>     if (!find_rank (location, arg.value, arg.value, false, &rank))
>       {
> @@ -1110,8 +1102,7 @@ fix_array_notation_expr (location_t location, enum tree_code code,
>       }
>     append_to_statement_list_force (body, &loop_with_init);
>     arg.value = loop_with_init;
> -  an_info.release ();
> -  an_loop_info.release ();
> +  release_vec_vec (an_info);
>     return arg;
>   }
>
> @@ -1128,7 +1119,7 @@ fix_array_notation_call_expr (tree arg)
>     tree body, loop_with_init = alloc_stmt_list ();
>     location_t location = UNKNOWN_LOCATION;
>     vec<vec<an_parts> > an_info = vNULL;
> -  vec<an_loop_parts> an_loop_info = vNULL;
> +  auto_vec<an_loop_parts> an_loop_info;
>
>     if (TREE_CODE (arg) == CALL_EXPR
>         && is_cilkplus_reduce_builtin (CALL_EXPR_FN (arg)))
> @@ -1194,8 +1185,7 @@ fix_array_notation_call_expr (tree arg)
>         body = pop_stmt_list (new_loop);
>       }
>     append_to_statement_list_force (body, &loop_with_init);
> -  an_loop_info.release ();
> -  an_info.release ();
> +  release_vec_vec (an_info);
>     return loop_with_init;
>   }
>
> diff --git a/gcc/cp/cp-array-notation.c b/gcc/cp/cp-array-notation.c
> index 84d7925..86f0323 100644
> --- a/gcc/cp/cp-array-notation.c
> +++ b/gcc/cp/cp-array-notation.c
> @@ -97,7 +97,7 @@ make_triplet_val_inv (tree *value)
>   */
>
>   static vec<tree, va_gc> *
> -create_array_refs (location_t loc, vec<vec<an_parts> > an_info,
> +create_array_refs (location_t loc, const vec<vec<an_parts> > &an_info,
>   		   vec<an_loop_parts> an_loop_info, size_t size,  size_t rank)
>   {
>     tree ind_mult, ind_incr;
> @@ -134,7 +134,7 @@ create_array_refs (location_t loc, vec<vec<an_parts> > an_info,
>
>   void
>   create_cmp_incr (location_t loc, vec <an_loop_parts> *node, size_t rank,
> -		 vec<vec<an_parts> > an_info, tsubst_flags_t complain)
> +		 const vec<vec<an_parts> > &an_info, tsubst_flags_t complain)
>   {
>     for (size_t ii = 0; ii < rank; ii++)
>       {
> @@ -210,7 +210,7 @@ expand_sec_reduce_builtin (tree an_builtin_fn, tree *new_var)
>     enum tree_code code = NOP_EXPR;
>     location_t location = UNKNOWN_LOCATION;
>     vec<vec<an_parts> > an_info = vNULL;
> -  vec<an_loop_parts> an_loop_info = vNULL;
> +  auto_vec<an_loop_parts> an_loop_info;
>     enum built_in_function an_type =
>       is_cilkplus_reduce_builtin (CALL_EXPR_FN (an_builtin_fn));
>     vec <tree, va_gc> *func_args;
> @@ -494,8 +494,7 @@ expand_sec_reduce_builtin (tree an_builtin_fn, tree *new_var)
>       }
>     append_to_statement_list (body, &loop_with_init);
>
> -  an_info.release ();
> -  an_loop_info.release ();
> +  release_vec_vec (an_info);
>
>     return loop_with_init;
>   }
> @@ -512,7 +511,7 @@ expand_an_in_modify_expr (location_t location, tree lhs,
>     tree array_expr_lhs = NULL_TREE, array_expr_rhs = NULL_TREE;
>     tree array_expr = NULL_TREE;
>     tree body = NULL_TREE;
> -  vec<tree> cond_expr = vNULL;
> +  auto_vec<tree> cond_expr;
>     vec<tree, va_gc> *lhs_array_operand = NULL, *rhs_array_operand = NULL;
>     size_t lhs_rank = 0, rhs_rank = 0, ii = 0;
>     vec<tree, va_gc> *rhs_list = NULL, *lhs_list = NULL;
> @@ -521,7 +520,8 @@ expand_an_in_modify_expr (location_t location, tree lhs,
>     bool found_builtin_fn = false;
>     tree an_init, loop_with_init = alloc_stmt_list ();
>     vec<vec<an_parts> > lhs_an_info = vNULL, rhs_an_info = vNULL;
> -  vec<an_loop_parts> lhs_an_loop_info = vNULL, rhs_an_loop_info = vNULL;
> +  auto_vec<an_loop_parts> lhs_an_loop_info, rhs_an_loop_info;
> +  tree lhs_len, rhs_len;
>
>     if (!find_rank (location, rhs, rhs, false, &rhs_rank))
>       return error_mark_node;
> @@ -645,11 +645,11 @@ expand_an_in_modify_expr (location_t location, tree lhs,
>   						 rhs_an_info)))
>       {
>         pop_stmt_list (an_init);
> -      return error_mark_node;
> +      goto error;
>       }
> -  tree rhs_len = ((rhs_list_size > 0 && rhs_rank > 0) ?
> +  rhs_len = ((rhs_list_size > 0 && rhs_rank > 0) ?
>       rhs_an_info[0][0].length : NULL_TREE);
> -  tree lhs_len = ((lhs_list_size > 0 && lhs_rank > 0) ?
> +  lhs_len = ((lhs_list_size > 0 && lhs_rank > 0) ?
>       lhs_an_info[0][0].length : NULL_TREE);
>     if (lhs_list_size > 0 && rhs_list_size > 0 && lhs_rank > 0 && rhs_rank > 0
>         && TREE_CODE (lhs_len) == INTEGER_CST && rhs_len
> @@ -658,7 +658,7 @@ expand_an_in_modify_expr (location_t location, tree lhs,
>       {
>         error_at (location, "length mismatch between LHS and RHS");
>         pop_stmt_list (an_init);
> -      return error_mark_node;
> +      goto error;
>       }
>      for (ii = 0; ii < lhs_rank; ii++)
>        {
> @@ -676,7 +676,7 @@ expand_an_in_modify_expr (location_t location, tree lhs,
>   						  lhs_an_loop_info, lhs_rank,
>   						  lhs);
>          if (!rhs_array_operand)
> -	 return error_mark_node;
> +	 goto error;
>        }
>     replace_array_notations (&rhs, true, rhs_list, rhs_array_operand);
>     rhs_list_size = 0;
> @@ -717,7 +717,7 @@ expand_an_in_modify_expr (location_t location, tree lhs,
>   						 rhs_an_loop_info, rhs_rank,
>   						 rhs);
>         if (!rhs_array_operand)
> -	return error_mark_node;
> +	goto error;
>         replace_array_notations (&rhs, true, rhs_list, rhs_array_operand);
>       }
>
> @@ -768,16 +768,18 @@ expand_an_in_modify_expr (location_t location, tree lhs,
>       }
>     append_to_statement_list (body, &loop_with_init);
>
> -  lhs_an_info.release ();
> -  lhs_an_loop_info.release ();
> +  release_vec_vec (lhs_an_info);
>     if (rhs_rank)
> -    {
> -      rhs_an_info.release ();
> -      rhs_an_loop_info.release ();
> -    }
> -  cond_expr.release ();
> +    release_vec_vec (rhs_an_info);
>
>     return loop_with_init;
> +
> +error:
> +  release_vec_vec (lhs_an_info);
> +  if (rhs_rank)
> +    release_vec_vec (rhs_an_info);
> +
> +  return error_mark_node;
>   }
>
>   /* Helper function for expand_conditonal_array_notations.  Encloses the
> @@ -796,7 +798,7 @@ cp_expand_cond_array_notations (tree orig_stmt)
>     tree loop_with_init = alloc_stmt_list ();
>     location_t location = UNKNOWN_LOCATION;
>     vec<vec<an_parts> > an_info = vNULL;
> -  vec<an_loop_parts> an_loop_info = vNULL;
> +  auto_vec<an_loop_parts> an_loop_info;
>
>     if (TREE_CODE (orig_stmt) == COND_EXPR)
>       {
> @@ -957,8 +959,7 @@ cp_expand_cond_array_notations (tree orig_stmt)
>       }
>     append_to_statement_list (body, &loop_with_init);
>
> -  an_info.release ();
> -  an_loop_info.release ();
> +  release_vec_vec (an_info);
>
>     return loop_with_init;
>   }
> @@ -977,7 +978,7 @@ expand_unary_array_notation_exprs (tree orig_stmt)
>     location_t location = EXPR_LOCATION (orig_stmt);
>     tree an_init, loop_with_init = alloc_stmt_list ();
>     vec<vec<an_parts> > an_info = vNULL;
> -  vec<an_loop_parts> an_loop_info = vNULL;
> +  auto_vec<an_loop_parts> an_loop_info;
>
>     if (!find_rank (location, orig_stmt, orig_stmt, true, &rank))
>       return error_mark_node;
> @@ -1060,8 +1061,7 @@ expand_unary_array_notation_exprs (tree orig_stmt)
>       }
>     append_to_statement_list (body, &loop_with_init);
>
> -  an_info.release ();
> -  an_loop_info.release ();
> +  release_vec_vec (an_info);
>
>     return loop_with_init;
>   }
> diff --git a/gcc/vec.h b/gcc/vec.h
> index 3e6e882..ed300b4 100644
> --- a/gcc/vec.h
> +++ b/gcc/vec.h
> @@ -1702,6 +1702,18 @@ vec<T, va_heap, vl_ptr>::using_auto_storage () const
>     return m_vec->m_vecpfx.m_using_auto_storage;
>   }
>
> +/* Release VEC and call release of all element vectors.  */
> +
> +template<typename T>
> +inline void
> +release_vec_vec (vec<vec<T> > &vec)
> +{
> +  for (unsigned i = 0; i < vec.length (); i++)
> +    vec[i].release ();
> +
> +  vec.release ();
> +}
> +
>   #if (GCC_VERSION >= 3000)
>   # pragma GCC poison m_vec m_vecpfx m_vecdata
>   #endif
>

Hi.

I'm sending v2 of the patch, where I removed adding of 'const' to certain function arguments.
Apart from that, I found one more leak related to cilk. As I've retested in valgrind, there
should not be any memory leak related to cilk.

Ready to be installed?
Thanks,
Martin

On 11/26/2015 09:59 PM, Martin Liška wrote:
> I'm sending v2 of the patch, where I removed adding of 'const' to
> certain function arguments.
> Apart from that, I found one more leak related to cilk. As I've retested
> in valgrind, there
> should not be any memory leak related to cilk.
>
> Ready to be installed?

Already improved, wasn't it? (Please don't quote the entire previous 
patch next time).


Bernd

On 11/27/2015 10:21 AM, Bernd Schmidt wrote:
> Already improved, wasn't it? (Please don't quote the entire previous patch next time).
> 
> 
> Bernd

Thanks, I just applied nits pointed by you.
Patch installed as r231001.

Thank you,
Martin