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

["Marc Nieper-Wißkirchen" <marc@nieper-wisskirchen.de>]

[-- Attachment #1: Type: text/plain, Size: 4479 bytes --]

Attached to this email is the patch updated to the recent renaming from *.c
to *.cc.


Am So., 23. Jan. 2022 um 14:18 Uhr schrieb Marc Nieper-Wißkirchen <
marc@nieper-wisskirchen.de>:

> Am Sa., 15. Jan. 2022 um 14:56 Uhr schrieb Marc Nieper-Wißkirchen
> <marc@nieper-wisskirchen.de>:
> >
> > Jeff, David, do you need any more input from my side?
> >
> > -- Marc
> >
> > Am Sa., 8. Jan. 2022 um 17:32 Uhr schrieb Jeff Law <
> jeffreyalaw@gmail.com>:
> > >
> > >
> > >
> > > On 1/6/2022 6:53 AM, David Malcolm via Gcc-patches wrote:
> > > > 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.
> > > My concern (and what I hadn't had time to dig into) was we initially
> > > used nofree_string_hash -- I wanted to make sure there wasn't any path
> > > where the name came from the stack (can't be free'd), was saved
> > > elsewhere (danging pointer) and the like.  ie, why were we using
> > > nofree_string_hash to begin with?  I've never really mucked around with
> > > these bits, so the analysis side kept falling off the daily todo list.
>
> The only occurrences of m_name_to_pass_map are in pass-manager.h
> (where it is defined as a private field of the class pass_manager) and
> in passes.cc. There is just one instance where a name is added to the
> map in passes.cc, namely through the put method. There, the name has
> been xstrdup'ed.
>
> The name (as a const char *) escapes the pass map in
> pass_manager::create_pass_tab through the call to
> m_name_pass_map->traverse. This inserts the name into the pass_tab,
> which is a static vec of const char *s. The pass_tab does not escape
> the translation unit of passes.c. It is used in dump_one_pass where
> the name is used as an argument to fprintf. The important point is
> that it is not freed and not further copied.
>
> > >
> > > If/once you're comfortable with the patch David, then go ahead and
> apply
> > > it on Marc's behalf.
> > >
> > > jeff
> > >
>

[-- Attachment #2: 0001-gcc-pass-manager-Fix-memory-leak.-PR-jit-63854.patch --]
[-- Type: text/x-patch, Size: 4550 bytes --]

From b7b2e4911328f6b275baafe395a1d2485e158195 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marc=20Nieper-Wi=C3=9Fkirchen?= <marc@nieper-wisskirchen.de>
Date: Sun, 19 Dec 2021 22:30:10 +0100
Subject: [PATCH] gcc: pass-manager: Fix memory leak. [PR jit/63854]
To: gcc-patches@gcc.gnu.org

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.

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.cc (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.cc      |  5 +++--
 3 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/gcc/hash-traits.h b/gcc/hash-traits.h
index b16975cfeb3..bef0bd42d04 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 6484d2c8ebc..43484da2700 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.cc b/gcc/passes.cc
index 00f856e0754..36e5b4ac45f 100644
--- a/gcc/passes.cc
+++ b/gcc/passes.cc
@@ -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)
       {
-- 
2.32.0