diff mbox

[10/21] PR jit/63854: Fix leak of worklist within jit-recording.c

Message ID 1416393981-39626-11-git-send-email-dmalcolm@redhat.com
State New
Headers show

Commit Message

David Malcolm Nov. 19, 2014, 10:46 a.m. UTC 
Fix this leak:

160 bytes in 5 blocks are definitely lost in loss record 154 of 228
   at 0x4A0645D: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
   by 0x5D75D4F: xrealloc (xmalloc.c:177)
   by 0x4DE1710: void va_heap::reserve<gcc::jit::recording::block*>(vec<gcc::jit::recording::block*, va_heap, vl_embed>*&, unsigned int, bool) (vec.h:310)
   by 0x4DDFAB5: vec<gcc::jit::recording::block*, va_heap, vl_ptr>::reserve(unsigned int, bool) (vec.h:1428)
   by 0x4DDFBFC: vec<gcc::jit::recording::block*, va_heap, vl_ptr>::reserve_exact(unsigned int) (vec.h:1448)
   by 0x4DDE588: vec<gcc::jit::recording::block*, va_heap, vl_ptr>::create(unsigned int) (vec.h:1463)
   by 0x4DD9B9F: gcc::jit::recording::function::validate() (jit-recording.c:2191)
   by 0x4DD7AD3: gcc::jit::recording::context::validate() (jit-recording.c:1005)
   by 0x4DD7660: gcc::jit::recording::context::compile() (jit-recording.c:848)
   by 0x4DD5BD2: gcc_jit_context_compile (libgccjit.c:2014)
   by 0x401CA4: test_jit (harness.h:190)
   by 0x401D88: main (harness.h:232)

gcc/jit/ChangeLog:
	PR jit/63854
	* jit-recording.c (recording::function::validate): Convert
	"worklist" from vec<> to autovec<> to fix a leak.
---
 gcc/jit/jit-recording.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Comments

Jeff Law Nov. 19, 2014, 4:59 p.m. UTC | #1 
On 11/19/14 03:46, David Malcolm wrote:
> Fix this leak:
>
> 160 bytes in 5 blocks are definitely lost in loss record 154 of 228
>     at 0x4A0645D: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
>     by 0x5D75D4F: xrealloc (xmalloc.c:177)
>     by 0x4DE1710: void va_heap::reserve<gcc::jit::recording::block*>(vec<gcc::jit::recording::block*, va_heap, vl_embed>*&, unsigned int, bool) (vec.h:310)
>     by 0x4DDFAB5: vec<gcc::jit::recording::block*, va_heap, vl_ptr>::reserve(unsigned int, bool) (vec.h:1428)
>     by 0x4DDFBFC: vec<gcc::jit::recording::block*, va_heap, vl_ptr>::reserve_exact(unsigned int) (vec.h:1448)
>     by 0x4DDE588: vec<gcc::jit::recording::block*, va_heap, vl_ptr>::create(unsigned int) (vec.h:1463)
>     by 0x4DD9B9F: gcc::jit::recording::function::validate() (jit-recording.c:2191)
>     by 0x4DD7AD3: gcc::jit::recording::context::validate() (jit-recording.c:1005)
>     by 0x4DD7660: gcc::jit::recording::context::compile() (jit-recording.c:848)
>     by 0x4DD5BD2: gcc_jit_context_compile (libgccjit.c:2014)
>     by 0x401CA4: test_jit (harness.h:190)
>     by 0x401D88: main (harness.h:232)
>
> gcc/jit/ChangeLog:
> 	PR jit/63854
> 	* jit-recording.c (recording::function::validate): Convert
> 	"worklist" from vec<> to autovec<> to fix a leak.
JIT space, yours to approve :-)  We haven't formalized that yet, but 
it'd be silly to do anything else.

Anyway so formally, this is OK for the trunk.

jeff
David Malcolm Nov. 19, 2014, 8:02 p.m. UTC | #2 
On Wed, 2014-11-19 at 09:59 -0700, Jeff Law wrote:
> On 11/19/14 03:46, David Malcolm wrote:
> > Fix this leak:
> >
> > 160 bytes in 5 blocks are definitely lost in loss record 154 of 228
> >     at 0x4A0645D: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
> >     by 0x5D75D4F: xrealloc (xmalloc.c:177)
> >     by 0x4DE1710: void va_heap::reserve<gcc::jit::recording::block*>(vec<gcc::jit::recording::block*, va_heap, vl_embed>*&, unsigned int, bool) (vec.h:310)
> >     by 0x4DDFAB5: vec<gcc::jit::recording::block*, va_heap, vl_ptr>::reserve(unsigned int, bool) (vec.h:1428)
> >     by 0x4DDFBFC: vec<gcc::jit::recording::block*, va_heap, vl_ptr>::reserve_exact(unsigned int) (vec.h:1448)
> >     by 0x4DDE588: vec<gcc::jit::recording::block*, va_heap, vl_ptr>::create(unsigned int) (vec.h:1463)
> >     by 0x4DD9B9F: gcc::jit::recording::function::validate() (jit-recording.c:2191)
> >     by 0x4DD7AD3: gcc::jit::recording::context::validate() (jit-recording.c:1005)
> >     by 0x4DD7660: gcc::jit::recording::context::compile() (jit-recording.c:848)
> >     by 0x4DD5BD2: gcc_jit_context_compile (libgccjit.c:2014)
> >     by 0x401CA4: test_jit (harness.h:190)
> >     by 0x401D88: main (harness.h:232)
> >
> > gcc/jit/ChangeLog:
> > 	PR jit/63854
> > 	* jit-recording.c (recording::function::validate): Convert
> > 	"worklist" from vec<> to autovec<> to fix a leak.
> JIT space, yours to approve :-)  We haven't formalized that yet, but 
> it'd be silly to do anything else.

FWIW, I added myself to the MAINTAINERS file as JIT maintainer as part
of a change you reviewed as:
  https://gcc.gnu.org/ml/jit/2014-q4/msg00029.html

Is there a governance distinction here, between patch review vs
decisions of the steering committee?  i.e. do changes to the maintainers
part of the MAINTAINERS file require higher-level approval?

Presumably I should continue to send (non-trivial) jit patches to this
list and wait for review before committing to trunk?

> Anyway so formally, this is OK for the trunk.

Thanks.
Richard Biener Nov. 20, 2014, 4:01 p.m. UTC | #3 
On Wed, Nov 19, 2014 at 9:02 PM, David Malcolm <dmalcolm@redhat.com> wrote:
> On Wed, 2014-11-19 at 09:59 -0700, Jeff Law wrote:
>> On 11/19/14 03:46, David Malcolm wrote:
>> > Fix this leak:
>> >
>> > 160 bytes in 5 blocks are definitely lost in loss record 154 of 228
>> >     at 0x4A0645D: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
>> >     by 0x5D75D4F: xrealloc (xmalloc.c:177)
>> >     by 0x4DE1710: void va_heap::reserve<gcc::jit::recording::block*>(vec<gcc::jit::recording::block*, va_heap, vl_embed>*&, unsigned int, bool) (vec.h:310)
>> >     by 0x4DDFAB5: vec<gcc::jit::recording::block*, va_heap, vl_ptr>::reserve(unsigned int, bool) (vec.h:1428)
>> >     by 0x4DDFBFC: vec<gcc::jit::recording::block*, va_heap, vl_ptr>::reserve_exact(unsigned int) (vec.h:1448)
>> >     by 0x4DDE588: vec<gcc::jit::recording::block*, va_heap, vl_ptr>::create(unsigned int) (vec.h:1463)
>> >     by 0x4DD9B9F: gcc::jit::recording::function::validate() (jit-recording.c:2191)
>> >     by 0x4DD7AD3: gcc::jit::recording::context::validate() (jit-recording.c:1005)
>> >     by 0x4DD7660: gcc::jit::recording::context::compile() (jit-recording.c:848)
>> >     by 0x4DD5BD2: gcc_jit_context_compile (libgccjit.c:2014)
>> >     by 0x401CA4: test_jit (harness.h:190)
>> >     by 0x401D88: main (harness.h:232)
>> >
>> > gcc/jit/ChangeLog:
>> >     PR jit/63854
>> >     * jit-recording.c (recording::function::validate): Convert
>> >     "worklist" from vec<> to autovec<> to fix a leak.
>> JIT space, yours to approve :-)  We haven't formalized that yet, but
>> it'd be silly to do anything else.
>
> FWIW, I added myself to the MAINTAINERS file as JIT maintainer as part
> of a change you reviewed as:
>   https://gcc.gnu.org/ml/jit/2014-q4/msg00029.html
>
> Is there a governance distinction here, between patch review vs
> decisions of the steering committee?  i.e. do changes to the maintainers
> part of the MAINTAINERS file require higher-level approval?

Yes, reviewers and maintainers are appointed by the steering commitee only.

Richard.

> Presumably I should continue to send (non-trivial) jit patches to this
> list and wait for review before committing to trunk?
>
>> Anyway so formally, this is OK for the trunk.
>
> Thanks.
>
Jeff Law Nov. 20, 2014, 4:18 p.m. UTC | #4 
On 11/20/14 09:01, Richard Biener wrote:
>> Is there a governance distinction here, between patch review vs
>> decisions of the steering committee?  i.e. do changes to the maintainers
>> part of the MAINTAINERS file require higher-level approval?
>
> Yes, reviewers and maintainers are appointed by the steering commitee only.
Right.  I've already raised appointing David as the JIT maintainer to 
the steering committee.  I just need to count the votes and take 
appropriate action.

Similarly for the MPX runtime and Ilya as the MPX maintainer, & Bernd as 
the nvptx maintainer.

If there's other maintainers that need to get appointed, nobody should 
hesitate to contact one of the SC members to get the nomination in front 
of the committee.

jeff
diff mbox

Patch

diff --git a/gcc/jit/jit-recording.c b/gcc/jit/jit-recording.c
index 8daa8f2..8cce277 100644
--- a/gcc/jit/jit-recording.c
+++ b/gcc/jit/jit-recording.c
@@ -2187,8 +2187,7 @@  recording::function::validate ()
     {
       /* Iteratively walk the graph of blocks, marking their "m_is_reachable"
 	 flag, starting at the initial block.  */
-      vec<block *> worklist;
-      worklist.create (m_blocks.length ());
+      auto_vec<block *> worklist (m_blocks.length ());
       worklist.safe_push (m_blocks[0]);
       while (worklist.length () > 0)
 	{