Re: Memory leaks (detected by Valgrind)

["Marc Nieper-Wißkirchen" <marc.nieper+gnu@gmail.com>]

Am Sa., 18. Dez. 2021 um 17:45 Uhr schrieb David Malcolm <dmalcolm@redhat.com>:
>
> On Sat, 2021-12-18 at 14:57 +0100, Marc Nieper-Wißkirchen wrote:
> > Am Sa., 18. Dez. 2021 um 00:23 Uhr schrieb David Malcolm <
> > dmalcolm@redhat.com>:
> > >
> > > On Fri, 2021-12-17 at 10:52 +0000, Alex Coplan via Jit wrote:
> > > > Hi,
> > > >
> > > > > -----Original Message-----
> > > > > From: Jit <jit-bounces+alex.coplan=arm.com@gcc.gnu.org> On
> > > > > Behalf
> > > > > Of Marc
> > > > > Nieper-Wißkirchen via Jit
> > > > > Sent: 17 December 2021 10:29
> > > > > To: Mark Wielaard <mark@klomp.org>
> > > > > Cc: Marc Nieper-Wißkirchen <marc.nieper+gnu@gmail.com>;
> > > > > jit@gcc.gnu.org
> > > > > Subject: Re: Memory leaks (detected by Valgrind)
> > > > >
> > > > > Thanks!
> > > > >
> > > > > With `--enable-valgrind-annotations`, the "uses of
> > > > > uninitialized
> > > > > values"
> > > > > have gone away, but a lot of small leaks are still present:
> > > >
> > > > Memory leaks with libgccjit are a known issue, see
> > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63854
> > >
> > > FWIW, it looks like all of the remaining leaks are in gcc.c (the
> > > "driver" code, for the "gcc" binary).   IIRC most of this relates
> > > to
> > > long-standing code that was written with the assumption that it
> > > will
> > > only be run once and not persist in memory, and so it tends to mix
> > > up
> > > string literals and dynamically-allocated strings without bothering
> > > to
> > > free them (the pointer values persist to the end of "main" when run
> > > from gcc, but get cleared when run from libgccjit.so).
> > >
> > > Patches to clean these up would be great.  That said, I'm not the
> > > driver/gcc.c maintainer, so I can't formally approve them (but can
> > > post
> > > +1 emails if they look good to me)  [1]
> >
> > I am currently trying to find my way through the code in gcc.c.
> > During
> > my experiments, I fixed a single leak where a dynamically allocated
> > string was assigned in place of a statically allocated one by moving
> > the dynamically allocated string onto the obstack. I hope that such
> > an
> > approach is acceptable.
>
> IIRC I tried that approach, and the maintainer (Joseph Myers, I think),
> wasn't keen on it, but this was a few years ago.  I've tried finding
> the discussion in the archives, but couldn't.  IIRC, he preferred
> adding a flag to strings in gcc.c, tracking if they needed to be freed.
> I'm CCing Joseph.   FWIW there's a class in libcpp/include/line-map.h:
> class label_text which does this.  Perhaps it could be used for this
> (and maybe renamed???)

Okay, no problem to do it that way.

> Commit 9376dd63e6a2d94823f6faf8212c9f37bef5a656 added the code to run
> gcc.c in-process (before it was run in a subprocess), so maybe the
> discussion was some time around then? (2015-08-25)
>
> Note that it's still possible to run this code in a subprocess rather
> than in-process (and thus workaround the leak) using
> gcc_jit_context_set_bool_use_external_driver.

There are still leaks using the external driver as far as I can tell.
In passes.c, the pass map is not deleted. It seems that the following
fixes it:

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;
 }

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

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.  */

--

Marc