Re: [PATCH] gcc: pass-manager: Fix memory leak. [PR jit/63854]

[David Malcolm <dmalcolm@redhat.com>]

On Sun, 2021-12-19 at 22:30 +0100, Marc Nieper-Wißkirchen wrote:
> This patch fixes a memory leak in the pass manager. In the existing
> code,
> the m_name_to_pass_map is allocated in
> pass_manager::register_pass_name, but
> never deallocated.  This is fixed by adding a deletion in
> pass_manager::~pass_manager.  Moreover the string keys in
> m_name_to_pass_map are
> all dynamically allocated.  To free them, this patch adds a new hash
> trait for
> string hashes that are to be freed when the corresponding hash entry
> is removed.
> 
> This fix is particularly relevant for using GCC as a library through
> libgccjit.
> The memory leak also occurs when libgccjit is instructed to use an
> external
> driver.
> 
> Before the patch, compiling the hello world example of libgccjit with
> the external driver under Valgrind shows a loss of 12,611 (48 direct)
> bytes.  After the patch, no memory leaks are reported anymore.
> (Memory leaks occurring when using the internal driver are mostly in
> the driver code in gcc/gcc.c and have to be fixed separately.)
> 
> The patch has been tested by fully bootstrapping the compiler with
> the
> frontends C, C++, Fortran, LTO, ObjC, JIT and running the test suite
> under a x86_64-pc-linux-gnu host.

Thanks for the patch.

It looks correct to me, given that pass_manager::register_pass_name
does an xstrdup and puts the result in the map.

That said:
- I'm not officially a reviewer for this part of gcc (though I probably
touched this code last)
- is it cleaner to instead change m_name_to_pass_map's key type from
const char * to char *, to convey that the map "owns" the name?  That
way we probably wouldn't need struct typed_const_free_remove, and (I
hope) works better with the type system.

Dave

> 
> gcc/ChangeLog:
> 
>         PR jit/63854
>         * hash-traits.h (struct typed_const_free_remove): New.
>         (struct free_string_hash): New.
>         * pass_manager.h: Use free_string_hash.
>         * passes.c (pass_manager::register_pass_name): Use
> free_string_hash.
>         (pass_manager::~pass_manager): Delete allocated
> m_name_to_pass_map.
> ---
>  gcc/hash-traits.h  | 17 +++++++++++++++++
>  gcc/pass_manager.h |  3 +--
>  gcc/passes.c       |  5 +++--
>  3 files changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/gcc/hash-traits.h b/gcc/hash-traits.h
> index 6f0373ec27f..1c08d6874ab 100644
> --- a/gcc/hash-traits.h
> +++ b/gcc/hash-traits.h
> @@ -28,6 +28,11 @@ struct typed_free_remove
>    static inline void remove (Type *p);
>  };
>  
> +template <typename Type>
> +struct typed_const_free_remove
> +{
> +  static inline void remove (const Type *p);
> +};
>  
>  /* Remove with free.  */
>  
> @@ -38,6 +43,13 @@ typed_free_remove <Type>::remove (Type *p)
>    free (p);
>  }
>  
> +template <typename Type>
> +inline void
> +typed_const_free_remove <Type>::remove (const Type *p)
> +{
> +  free (const_cast <Type *> (p));
> +}
> +
>  /* Helpful type for removing with delete.  */
>  
>  template <typename Type>
> @@ -305,6 +317,11 @@ struct ggc_ptr_hash : pointer_hash <T>,
> ggc_remove <T *> {};
>  template <typename T>
>  struct ggc_cache_ptr_hash : pointer_hash <T>, ggc_cache_remove <T *>
> {};
>  
> +/* Traits for string elements that should be freed when an element
> is
> +   deleted.  */
> +
> +struct free_string_hash : string_hash, typed_const_free_remove
> <char> {};
> +
>  /* Traits for string elements that should not be freed when an
> element
>     is deleted.  */
>  
> diff --git a/gcc/pass_manager.h b/gcc/pass_manager.h
> index aaf72cf6803..f5615e1fda8 100644
> --- a/gcc/pass_manager.h
> +++ b/gcc/pass_manager.h
> @@ -106,7 +106,7 @@ private:
>  
>  private:
>    context *m_ctxt;
> -  hash_map<nofree_string_hash, opt_pass *> *m_name_to_pass_map;
> +  hash_map<free_string_hash, opt_pass *> *m_name_to_pass_map;
>  
>    /* References to all of the individual passes.
>       These fields are generated via macro expansion.
> @@ -146,4 +146,3 @@ private:
>  } // namespace gcc
>  
>  #endif /* ! GCC_PASS_MANAGER_H */
> -
> diff --git a/gcc/passes.c b/gcc/passes.c
> index 4bea6ae5b6a..0c70ece5321 100644
> --- a/gcc/passes.c
> +++ b/gcc/passes.c
> @@ -903,7 +903,7 @@ void
>  pass_manager::register_pass_name (opt_pass *pass, const char *name)
>  {
>    if (!m_name_to_pass_map)
> -    m_name_to_pass_map = new hash_map<nofree_string_hash, opt_pass
> *> (256);
> +    m_name_to_pass_map = new hash_map<free_string_hash, opt_pass *>
> (256);
>  
>    if (m_name_to_pass_map->get (name))
>      return; /* Ignore plugin passes.  */
> @@ -1674,6 +1674,7 @@ pass_manager::~pass_manager ()
>    GCC_PASS_LISTS
>  #undef DEF_PASS_LIST
>  
> +  delete m_name_to_pass_map;
>  }
>  
>  /* If we are in IPA mode (i.e., current_function_decl is NULL), call
> @@ -1943,7 +1944,7 @@ pass_manager::dump_profile_report () const
>            "                                 |in count     |out
> prob     "
>            "|in count                  |out prob                  "
>            "|size               |time                      |\n");
> -          
> +
>    for (int i = 1; i < passes_by_id_size; i++)
>      if (profile_record[i].run)
>        {