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

[Jeff Law]

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