Re: [PATCH 17/21] PR jit/63854: Fix leaking vec in jit

[Trevor Saunders <tsaunders at mozilla dot com>]

On Wed, Nov 19, 2014 at 05:46:17AM -0500, David Malcolm wrote:
> This fixes various leaks of vec buffers seen via valgrind within jit
> (both recording and playback).
> 
> Various vec<> within jit::recording are converted to auto_vec<>.
> 
> Various playback::wrapper subclasses containing vec<> gain a finalizer
> so they can release the vec when they are collected.

It seems kind of tempting to just use a virtual dtor as the finalizer,
but I'm not sure how to make that work nicely with your overriding of
operator new.

Trev

> 
> gcc/jit/ChangeLog:
> 	PR jit/63854
> 	* jit-playback.c (gcc::jit::playback::compound_type::set_fields):
> 	Convert param from const vec<playback::field *> & to
> 	const auto_vec<playback::field *> *.
> 	(gcc::jit::playback::context::new_function_type): Convert param
> 	"param_types" from vec<type *> * to const auto_vec<type *> *.
> 	(gcc::jit::playback::context::new_function): Convert param
> 	"params" from vec<param *> * to const auto_vec<param *> *.
> 	(gcc::jit::playback::context::build_call): Convert param "args"
> 	from vec<rvalue *> to const auto_vec<rvalue *> *.
> 	(gcc::jit::playback::context::new_call): Likewise.
> 	(gcc::jit::playback::context::new_call_through_ptr): Likewise.
> 	(wrapper_finalizer): New function.
> 	(gcc::jit::playback::wrapper::operator new): Call the finalizer
> 	variant of ggc_internal_cleared_alloc, supplying
> 	wrapper_finalizer.
> 	(gcc::jit::playback::function::finalizer): New.
> 	(gcc::jit::playback::block::finalizer): New.
> 	(gcc::jit::playback::source_file::finalizer): New.
> 	(gcc::jit::playback::source_line::finalizer): New.
> 
> 	* jit-playback.h
> 	(gcc::jit::playback::context::new_function_type): Convert param
> 	"param_types" from vec<type *> * to const auto_vec<type *> *.
> 	(gcc::jit::playback::context::new_function): Convert param
> 	"params" from vec<param *> * to const auto_vec<param *> *.
> 	(gcc::jit::playback::context::new_call): Convert param
> 	"args" from vec<rvalue *> to const auto_vec<rvalue *> *.
> 	(gcc::jit::playback::context::new_call_through_ptr): Likewise.
> 	(gcc::jit::playback::context::build_call): Likewise.
> 	(gcc::jit::playback::context): Convert fields "m_functions",
> 	"m_source_files", "m_cached_locations" from vec to auto_vec.
> 	(gcc::jit::playback::wrapper::finalizer): New virtual function.
> 	(gcc::jit::playback::compound_type::set_fields): Convert param fro
> 	const vec<playback::field *> & to
> 	const auto_vec<playback::field *> *.
> 	(gcc::jit::playback::function::finalizer): New.
> 	(gcc::jit::playback::block::finalizer): New.
> 	(gcc::jit::playback::source_file::finalizer): New.
> 	(gcc::jit::playback::source_line::finalizer): New.
> 
> 	* jit-recording.c
> 	(gcc::jit::recording::function_type::replay_into): Convert local
> 	from a vec into an auto_vec.
> 	(gcc::jit::recording::fields::replay_into): Likewise.
> 	(gcc::jit::recording::function::replay_into): Likewise.
> 	(gcc::jit::recording::call::replay_into): Likewise.
> 	(gcc::jit::recording::call_through_ptr::replay_into): Likewise.
> 
> 	* jit-recording.h (gcc::jit::recording::context): Convert fields
> 	"m_mementos", "m_compound_types", "m_functions" from vec<> to
> 	auto_vec <>.
> 	(gcc::jit::recording::function_type::get_param_types): Convert
> 	return type from vec<type *> to const vec<type *> &.
> 	(gcc::jit::recording::function_type): Convert field
> 	"m_param_types" from a vec<> to an auto_vec<>.
> 	(gcc::jit::recording::fields): Likewise for field "m_fields".
> 	(gcc::jit::recording::function::get_params): Convert return type
> 	from vec <param *> to const vec<param *> &.
> 	(gcc::jit::recording::function): Convert fields "m_params",
> 	"m_locals", "m_blocks" from vec<> to auto_vec<>.
> 	(gcc::jit::recording::block): Likewise for field "m_statements".
> 	vec<> to auto_vec<>.
> 	(gcc::jit::recording::call): Likewise for field "m_args".
> 	(gcc::jit::recording::call_through_ptr): Likewise.
> ---
>  gcc/jit/jit-playback.c  | 73 +++++++++++++++++++++++++++++++++++++++++--------
>  gcc/jit/jit-playback.h  | 27 ++++++++++++------
>  gcc/jit/jit-recording.c | 16 +++++------
>  gcc/jit/jit-recording.h | 26 +++++++++---------
>  4 files changed, 100 insertions(+), 42 deletions(-)
> 
> diff --git a/gcc/jit/jit-playback.c b/gcc/jit/jit-playback.c
> index 285a3ef..8fdfa29 100644
> --- a/gcc/jit/jit-playback.c
> +++ b/gcc/jit/jit-playback.c
> @@ -285,15 +285,15 @@ new_compound_type (location *loc,
>  }
>  
>  void
> -playback::compound_type::set_fields (const vec<playback::field *> &fields)
> +playback::compound_type::set_fields (const auto_vec<playback::field *> *fields)
>  {
>    /* Compare with c/c-decl.c: finish_struct. */
>    tree t = as_tree ();
>  
>    tree fieldlist = NULL;
> -  for (unsigned i = 0; i < fields.length (); i++)
> +  for (unsigned i = 0; i < fields->length (); i++)
>      {
> -      field *f = fields[i];
> +      field *f = (*fields)[i];
>        DECL_CONTEXT (f->as_tree ()) = t;
>        fieldlist = chainon (f->as_tree (), fieldlist);
>      }
> @@ -309,7 +309,7 @@ playback::compound_type::set_fields (const vec<playback::field *> &fields)
>  playback::type *
>  playback::context::
>  new_function_type (type *return_type,
> -		   vec<type *> *param_types,
> +		   const auto_vec<type *> *param_types,
>  		   int is_variadic)
>  {
>    int i;
> @@ -361,7 +361,7 @@ new_function (location *loc,
>  	      enum gcc_jit_function_kind kind,
>  	      type *return_type,
>  	      const char *name,
> -	      vec<param *> *params,
> +	      const auto_vec<param *> *params,
>  	      int is_variadic,
>  	      enum built_in_function builtin_id)
>  {
> @@ -770,12 +770,12 @@ playback::rvalue *
>  playback::context::
>  build_call (location *loc,
>  	    tree fn_ptr,
> -	    vec<rvalue *> args)
> +	    const auto_vec<rvalue *> *args)
>  {
>    vec<tree, va_gc> *tree_args;
> -  vec_alloc (tree_args, args.length ());
> -  for (unsigned i = 0; i < args.length (); i++)
> -    tree_args->quick_push (args[i]->as_tree ());
> +  vec_alloc (tree_args, args->length ());
> +  for (unsigned i = 0; i < args->length (); i++)
> +    tree_args->quick_push ((*args)[i]->as_tree ());
>  
>    if (loc)
>      set_tree_location (fn_ptr, loc);
> @@ -806,7 +806,7 @@ playback::rvalue *
>  playback::context::
>  new_call (location *loc,
>  	  function *func,
> -	  vec<rvalue *> args)
> +	  const auto_vec<rvalue *> *args)
>  {
>    tree fndecl;
>  
> @@ -828,7 +828,7 @@ playback::rvalue *
>  playback::context::
>  new_call_through_ptr (location *loc,
>  		      rvalue *fn_ptr,
> -		      vec<rvalue *> args)
> +		      const auto_vec<rvalue *> *args)
>  {
>    gcc_assert (fn_ptr);
>    tree t_fn_ptr = fn_ptr->as_tree ();
> @@ -1079,6 +1079,18 @@ get_address (location *loc)
>    return new rvalue (get_context (), ptr);
>  }
>  
> +/* The wrapper subclasses are GC-managed, but can own non-GC memory.
> +   Provide this finalization hook for calling then they are collected,
> +   which calls the finalizer vfunc.  This allows them to call "release"
> +   on any vec<> within them.  */
> +
> +static void
> +wrapper_finalizer (void *ptr)
> +{
> +  playback::wrapper *wrapper = reinterpret_cast <playback::wrapper *> (ptr);
> +  wrapper->finalizer ();
> +}
> +
>  /* gcc::jit::playback::wrapper subclasses are GC-managed:
>     allocate them using ggc_internal_cleared_alloc.  */
>  
> @@ -1086,7 +1098,8 @@ void *
>  playback::wrapper::
>  operator new (size_t sz)
>  {
> -  return ggc_internal_cleared_alloc (sz MEM_STAT_INFO);
> +  return ggc_internal_cleared_alloc (sz, wrapper_finalizer, 0, 1);
> +
>  }
>  
>  /* Constructor for gcc:jit::playback::function.  */
> @@ -1128,6 +1141,15 @@ gt_ggc_mx ()
>    gt_ggc_m_9tree_node (m_inner_block);
>  }
>  
> +/* Don't leak vec's internal buffer (in non-GC heap) when we are
> +   GC-ed.  */
> +
> +void
> +playback::function::finalizer ()
> +{
> +  m_blocks.release ();
> +}
> +
>  /* Get the return type of a playback function, in tree form.  */
>  
>  tree
> @@ -1262,6 +1284,15 @@ postprocess ()
>      }
>  }
>  
> +/* Don't leak vec's internal buffer (in non-GC heap) when we are
> +   GC-ed.  */
> +
> +void
> +playback::block::finalizer ()
> +{
> +  m_stmts.release ();
> +}
> +
>  /* Add an eval of the rvalue to the function's statement list.  */
>  
>  void
> @@ -2024,6 +2055,15 @@ playback::source_file::source_file (tree filename) :
>  {
>  }
>  
> +/* Don't leak vec's internal buffer (in non-GC heap) when we are
> +   GC-ed.  */
> +
> +void
> +playback::source_file::finalizer ()
> +{
> +  m_source_lines.release ();
> +}
> +
>  /* Construct a playback::source_line for the given line
>     within this source file, if one doesn't exist already.  */
>  
> @@ -2056,6 +2096,15 @@ playback::source_line::source_line (source_file *file, int line_num) :
>  {
>  }
>  
> +/* Don't leak vec's internal buffer (in non-GC heap) when we are
> +   GC-ed.  */
> +
> +void
> +playback::source_line::finalizer ()
> +{
> +  m_locations.release ();
> +}
> +
>  /* Construct a playback::location for the given column
>     within this line of a specific source file, if one doesn't exist
>     already.  */
> diff --git a/gcc/jit/jit-playback.h b/gcc/jit/jit-playback.h
> index dcb19bf..30e9229 100644
> --- a/gcc/jit/jit-playback.h
> +++ b/gcc/jit/jit-playback.h
> @@ -71,7 +71,7 @@ public:
>  
>    type *
>    new_function_type (type *return_type,
> -		     vec<type *> *param_types,
> +		     const auto_vec<type *> *param_types,
>  		     int is_variadic);
>  
>    param *
> @@ -84,7 +84,7 @@ public:
>  		enum gcc_jit_function_kind kind,
>  		type *return_type,
>  		const char *name,
> -		vec<param *> *params,
> +		const auto_vec<param *> *params,
>  		int is_variadic,
>  		enum built_in_function builtin_id);
>  
> @@ -128,12 +128,12 @@ public:
>    rvalue *
>    new_call (location *loc,
>  	    function *func,
> -	    vec<rvalue *> args);
> +	    const auto_vec<rvalue *> *args);
>  
>    rvalue *
>    new_call_through_ptr (location *loc,
>  			rvalue *fn_ptr,
> -			vec<rvalue *> args);
> +			const auto_vec<rvalue *> *args);
>  
>    rvalue *
>    new_cast (location *loc,
> @@ -214,7 +214,7 @@ private:
>    rvalue *
>    build_call (location *loc,
>  	      tree fn_ptr,
> -	      vec<rvalue *> args);
> +	      const auto_vec<rvalue *> *args);
>  
>    tree
>    build_cast (location *loc,
> @@ -240,14 +240,14 @@ private:
>    char *m_path_s_file;
>    char *m_path_so_file;
>  
> -  vec<function *> m_functions;
> +  auto_vec<function *> m_functions;
>    tree m_char_array_type_node;
>    tree m_const_char_ptr;
>  
>    /* Source location handling.  */
> -  vec<source_file *> m_source_files;
> +  auto_vec<source_file *> m_source_files;
>  
> -  vec<std::pair<tree, location *> > m_cached_locations;
> +  auto_vec<std::pair<tree, location *> > m_cached_locations;
>  };
>  
>  /* A temporary wrapper object.
> @@ -263,6 +263,10 @@ public:
>    /* Allocate in the GC heap.  */
>    void *operator new (size_t sz);
>  
> +  /* Some wrapper subclasses contain vec<> and so need to
> +     release them when they are GC-ed.  */
> +  virtual void finalizer () { }
> +
>  };
>  
>  class type : public wrapper
> @@ -297,7 +301,7 @@ public:
>      : type (inner)
>    {}
>  
> -  void set_fields (const vec<field *> &fields);
> +  void set_fields (const auto_vec<field *> *fields);
>  };
>  
>  class field : public wrapper
> @@ -319,6 +323,7 @@ public:
>    function(context *ctxt, tree fndecl, enum gcc_jit_function_kind kind);
>  
>    void gt_ggc_mx ();
> +  void finalizer ();
>  
>    tree get_return_type_as_tree () const;
>  
> @@ -366,6 +371,8 @@ public:
>    block (function *func,
>  	 const char *name);
>  
> +  void finalizer ();
> +
>    tree as_label_decl () const { return m_label_decl; }
>  
>    void
> @@ -500,6 +507,7 @@ class source_file : public wrapper
>  {
>  public:
>    source_file (tree filename);
> +  void finalizer ();
>  
>    source_line *
>    get_source_line (int line_num);
> @@ -520,6 +528,7 @@ class source_line : public wrapper
>  {
>  public:
>    source_line (source_file *file, int line_num);
> +  void finalizer ();
>  
>    location *
>    get_location (recording::location *rloc, int column_num);
> diff --git a/gcc/jit/jit-recording.c b/gcc/jit/jit-recording.c
> index 8cce277..8069afc 100644
> --- a/gcc/jit/jit-recording.c
> +++ b/gcc/jit/jit-recording.c
> @@ -1602,7 +1602,7 @@ void
>  recording::function_type::replay_into (replayer *r)
>  {
>    /* Convert m_param_types to a vec of playback type.  */
> -  vec <playback::type *> param_types;
> +  auto_vec <playback::type *> param_types;
>    int i;
>    recording::type *type;
>    param_types.create (m_param_types.length ());
> @@ -1859,11 +1859,11 @@ recording::fields::fields (compound_type *struct_or_union,
>  void
>  recording::fields::replay_into (replayer *)
>  {
> -  vec<playback::field *> playback_fields;
> +  auto_vec<playback::field *> playback_fields;
>    playback_fields.create (m_fields.length ());
>    for (unsigned i = 0; i < m_fields.length (); i++)
>      playback_fields.safe_push (m_fields[i]->playback_field ());
> -  m_struct_or_union->playback_compound_type ()->set_fields (playback_fields);
> +  m_struct_or_union->playback_compound_type ()->set_fields (&playback_fields);
>  }
>  
>  /* Override the default implementation of
> @@ -2032,7 +2032,7 @@ void
>  recording::function::replay_into (replayer *r)
>  {
>    /* Convert m_params to a vec of playback param.  */
> -  vec <playback::param *> params;
> +  auto_vec <playback::param *> params;
>    int i;
>    recording::param *param;
>    params.create (m_params.length ());
> @@ -2848,14 +2848,14 @@ recording::call::call (recording::context *ctxt,
>  void
>  recording::call::replay_into (replayer *r)
>  {
> -  vec<playback::rvalue *> playback_args;
> +  auto_vec<playback::rvalue *> playback_args;
>    playback_args.create (m_args.length ());
>    for (unsigned i = 0; i< m_args.length (); i++)
>      playback_args.safe_push (m_args[i]->playback_rvalue ());
>  
>    set_playback_obj (r->new_call (playback_location (r, m_loc),
>  				 m_func->playback_function (),
> -				 playback_args));
> +				 &playback_args));
>  }
>  
>  /* Implementation of recording::memento::make_debug_string for
> @@ -2925,14 +2925,14 @@ recording::call_through_ptr::call_through_ptr (recording::context *ctxt,
>  void
>  recording::call_through_ptr::replay_into (replayer *r)
>  {
> -  vec<playback::rvalue *> playback_args;
> +  auto_vec<playback::rvalue *> playback_args;
>    playback_args.create (m_args.length ());
>    for (unsigned i = 0; i< m_args.length (); i++)
>      playback_args.safe_push (m_args[i]->playback_rvalue ());
>  
>    set_playback_obj (r->new_call_through_ptr (playback_location (r, m_loc),
>  					     m_fn_ptr->playback_rvalue (),
> -					     playback_args));
> +					     &playback_args));
>  }
>  
>  /* Implementation of recording::memento::make_debug_string for
> diff --git a/gcc/jit/jit-recording.h b/gcc/jit/jit-recording.h
> index bb1a2ee..4ea8ef1 100644
> --- a/gcc/jit/jit-recording.h
> +++ b/gcc/jit/jit-recording.h
> @@ -242,11 +242,11 @@ private:
>    bool m_bool_options[GCC_JIT_NUM_BOOL_OPTIONS];
>  
>    /* Recorded API usage.  */
> -  vec<memento *> m_mementos;
> +  auto_vec<memento *> m_mementos;
>  
>    /* Specific recordings, for use by dump_to_file.  */
> -  vec<compound_type *> m_compound_types;
> -  vec<function *> m_functions;
> +  auto_vec<compound_type *> m_compound_types;
> +  auto_vec<function *> m_functions;
>  
>    type *m_basic_types[NUM_GCC_JIT_TYPES];
>    type *m_FILE_type;
> @@ -622,7 +622,7 @@ public:
>    void replay_into (replayer *);
>  
>    type * get_return_type () const { return m_return_type; }
> -  vec<type *> get_param_types () const { return m_param_types; }
> +  const vec<type *> &get_param_types () const { return m_param_types; }
>    int is_variadic () const { return m_is_variadic; }
>  
>    string * make_debug_string_with_ptr ();
> @@ -633,7 +633,7 @@ public:
>  
>  private:
>    type *m_return_type;
> -  vec<type *> m_param_types;
> +  auto_vec<type *> m_param_types;
>    int m_is_variadic;
>  };
>  
> @@ -749,7 +749,7 @@ private:
>  
>  private:
>    compound_type *m_struct_or_union;
> -  vec<field *> m_fields;
> +  auto_vec<field *> m_fields;
>  };
>  
>  class union_ : public compound_type
> @@ -897,7 +897,7 @@ public:
>  
>    type *get_return_type () const { return m_return_type; }
>    string * get_name () const { return m_name; }
> -  vec<param *> get_params () const { return m_params; }
> +  const vec<param *> &get_params () const { return m_params; }
>  
>    /* Get the given param by index.
>       Implements the post-error-checking part of
> @@ -920,11 +920,11 @@ private:
>    enum gcc_jit_function_kind m_kind;
>    type *m_return_type;
>    string *m_name;
> -  vec<param *> m_params;
> +  auto_vec<param *> m_params;
>    int m_is_variadic;
>    enum built_in_function m_builtin_id;
> -  vec<local *> m_locals;
> -  vec<block *> m_blocks;
> +  auto_vec<local *> m_locals;
> +  auto_vec<block *> m_blocks;
>  };
>  
>  class block : public memento
> @@ -1011,7 +1011,7 @@ private:
>    function *m_func;
>    int m_index;
>    string *m_name;
> -  vec<statement *> m_statements;
> +  auto_vec<statement *> m_statements;
>    bool m_has_been_terminated;
>    bool m_is_reachable;
>  
> @@ -1222,7 +1222,7 @@ private:
>  
>  private:
>    function *m_func;
> -  vec<rvalue *> m_args;
> +  auto_vec<rvalue *> m_args;
>  };
>  
>  class call_through_ptr : public rvalue
> @@ -1241,7 +1241,7 @@ private:
>  
>  private:
>    rvalue *m_fn_ptr;
> -  vec<rvalue *> m_args;
> +  auto_vec<rvalue *> m_args;
>  };
>  
>  class array_access : public lvalue
> -- 
> 1.8.5.3
>