public inbox for jit@gcc.gnu.org
 help / color / mirror / Atom feed
From: Jeff Law <jeffreyalaw@gmail.com>
To: "David Malcolm" <dmalcolm@redhat.com>,
	"Marc Nieper-Wißkirchen" <marc@nieper-wisskirchen.de>,
	gcc-patches@gcc.gnu.org
Cc: jit@gcc.gnu.org
Subject: Re: [PATCH] gcc: pass-manager: Fix memory leak. [PR jit/63854]
Date: Sat, 8 Jan 2022 09:32:29 -0700	[thread overview]
Message-ID: <f7f74178-3cbe-e8d4-5882-c825e6dbc529@gmail.com> (raw)
In-Reply-To: <2a54fac9b37d87afb009b8eb339d5ad6927454dd.camel@redhat.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


  parent reply	other threads:[~2022-01-08 16:32 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-12-19 21:30 Marc Nieper-Wißkirchen
2022-01-06 13:53 ` David Malcolm
2022-01-06 13:57   ` David Malcolm
2022-01-08  9:26     ` Marc Nieper-Wißkirchen
2022-01-08 10:07   ` Marc Nieper-Wißkirchen
2022-01-08 16:32   ` Jeff Law [this message]
2022-01-15 13:56     ` Marc Nieper-Wißkirchen
2022-01-23 13:18       ` Marc Nieper-Wißkirchen
2022-01-31 11:42         ` Marc Nieper-Wißkirchen
2022-03-11 16:31           ` Marc Nieper-Wißkirchen
2022-03-19 17:43           ` Jeff Law

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=f7f74178-3cbe-e8d4-5882-c825e6dbc529@gmail.com \
    --to=jeffreyalaw@gmail.com \
    --cc=dmalcolm@redhat.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=jit@gcc.gnu.org \
    --cc=marc@nieper-wisskirchen.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).