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
