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

[David Malcolm <dmalcolm@redhat.com>]

On Thu, 2022-01-06 at 08:53 -0500, David Malcolm 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.


[...snip...]

> 
> > diff --git a/gcc/passes.c b/gcc/passes.c
> > index 4bea6ae5b6a..0c70ece5321 100644
> > --- a/gcc/passes.c
> > +++ b/gcc/passes.c

[...snip...]

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

...and there's a stray whitespace change here (in
pass_manager::dump_profile_report), which probably shouldn't be in the
patch.

Dave