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
