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)
> {
