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

  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.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(-)
  

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

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
  

Am Do., 6. Jan. 2022 um 14:57 Uhr schrieb David Malcolm via Jit
<jit@gcc.gnu.org>:

> [...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.

There was stray whitespace in that line the unpatched version of
`passes.c`, which my Emacs silently cleaned up.

Shall I retain this whitespace although it should probably haven't
been there in the first place? Or should I just add a remark in the
patch notes about that?

Thanks,

Marc
  

Thanks for replying so quickly!

Am Do., 6. Jan. 2022 um 14:53 Uhr schrieb David Malcolm <dmalcolm@redhat.com>:

[...]

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

I am a newcomer to the codebase of GCC and haven't yet been able to
figure out whom to contact. I bothered you because the patch is mostly
relevant for the libgccjit frontend.

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

The problem with that approach is that we would then need a new
version of string_hash in hash-traits.h, say owned_string_hash, which
derives from pointer_hash <char> and not pointer_hash <const char>.
This would add roughly as much code as struct typed_const_free_remove.
Using the hypothetical owned_string_hash in the definition of
m_name_to_pass_map in passes.c would then produce a map taking "char
*" strings instead of "const char *" strings. This, however, would
then lead to problems in pass_manager::register_pass_name where name
is a "const char *" string (coming from outside) but
m_name_to_pass_map->get would take a "char *" string.

I don't see how to resolve this without bigger refactoring, so I think
my struct typed_const_free_remove approach is less intrusive. This
conveys at least that the key isn't changed by the hashmap operations
and that it is yet owned (because this is something that
typed_const_free_remove presupposes.

Thanks,

Marc

[...]
  

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.

If/once you're comfortable with the patch David, then go ahead and apply 
it on Marc's behalf.

jeff
  

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.
>
> If/once you're comfortable with the patch David, then go ahead and apply
> it on Marc's behalf.
>
> jeff
>
  

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

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

Hi Jeff and David,

any news on this fix?

Thanks,

Marc

Am Mo., 31. Jan. 2022 um 12:42 Uhr schrieb Marc Nieper-Wißkirchen
<marc@nieper-wisskirchen.de>:
>
> 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
>> > >
  

On 1/31/2022 4:42 AM, Marc Nieper-Wißkirchen wrote:
> 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
I'd hoped David would handle this and I've been insanely busy. 
Regardless, I've pushed this to the trunk.  Thanks and sorry for the 
long delays.

jeff