* [PATCH 11/21] PR jit/63854: Fix leak of "avail" within tree-ssa-pre.c
2014-01-01 0:00 [PATCH 00/21] PR 63854: Fix various memory leaks David Malcolm
@ 2014-01-01 0:00 ` David Malcolm
2014-01-01 0:00 ` Richard Biener
2014-01-01 0:00 ` [PATCH 06/21] PR jit/63854: Fix leak of opts_obstack David Malcolm
` (19 subsequent siblings)
20 siblings, 1 reply; 69+ messages in thread
From: David Malcolm @ 2014-01-01 0:00 UTC (permalink / raw)
To: gcc-patches, jit; +Cc: David Malcolm
Fix this leak:
120 bytes in 5 blocks are definitely lost in loss record 141 of 227
at 0x4A0645D: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
by 0x5D75D8F: xrealloc (xmalloc.c:177)
by 0x550DEBA: void va_heap::reserve<pre_expr_d*>(vec<pre_expr_d*, va_heap, vl_embed>*&, unsigned int, bool) (vec.h:310)
by 0x550D509: vec<pre_expr_d*, va_heap, vl_ptr>::reserve(unsigned int, bool) (vec.h:1428)
by 0x550DA3E: vec<pre_expr_d*, va_heap, vl_ptr>::reserve_exact(unsigned int) (vec.h:1448)
by 0x550D167: vec<pre_expr_d*, va_heap, vl_ptr>::safe_grow(unsigned int) (vec.h:1576)
by 0x55076FE: do_regular_insertion(basic_block_def*, basic_block_def*) (tree-ssa-pre.c:3209)
by 0x5508379: insert_aux(basic_block_def*) (tree-ssa-pre.c:3526)
by 0x55083DC: insert_aux(basic_block_def*) (tree-ssa-pre.c:3536)
by 0x55083DC: insert_aux(basic_block_def*) (tree-ssa-pre.c:3536)
by 0x55084C0: insert() (tree-ssa-pre.c:3559)
by 0x550C5BC: (anonymous namespace)::pass_pre::execute(function*) (tree-ssa-pre.c:4805)
gcc/ChangeLog:
PR jit/63854
* tree-ssa-pre.c (do_regular_insertion): Convert "avail" from
vec<> to auto_vec<> to fix a leak.
---
gcc/tree-ssa-pre.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/gcc/tree-ssa-pre.c b/gcc/tree-ssa-pre.c
index ea99198..027dc1c 100644
--- a/gcc/tree-ssa-pre.c
+++ b/gcc/tree-ssa-pre.c
@@ -3202,7 +3202,7 @@ do_regular_insertion (basic_block block, basic_block dom)
bool new_stuff = false;
vec<pre_expr> exprs;
pre_expr expr;
- vec<pre_expr> avail = vNULL;
+ auto_vec<pre_expr> avail;
int i;
exprs = sorted_array_from_bitmap_set (ANTIC_IN (block));
--
1.8.5.3
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH 11/21] PR jit/63854: Fix leak of "avail" within tree-ssa-pre.c
2014-01-01 0:00 ` [PATCH 11/21] PR jit/63854: Fix leak of "avail" within tree-ssa-pre.c David Malcolm
@ 2014-01-01 0:00 ` Richard Biener
0 siblings, 0 replies; 69+ messages in thread
From: Richard Biener @ 2014-01-01 0:00 UTC (permalink / raw)
To: David Malcolm; +Cc: GCC Patches, jit
On Wed, Nov 19, 2014 at 11:46 AM, David Malcolm <dmalcolm@redhat.com> wrote:
> Fix this leak:
Ok.
Thanks,
Richard.
> 120 bytes in 5 blocks are definitely lost in loss record 141 of 227
> at 0x4A0645D: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
> by 0x5D75D8F: xrealloc (xmalloc.c:177)
> by 0x550DEBA: void va_heap::reserve<pre_expr_d*>(vec<pre_expr_d*, va_heap, vl_embed>*&, unsigned int, bool) (vec.h:310)
> by 0x550D509: vec<pre_expr_d*, va_heap, vl_ptr>::reserve(unsigned int, bool) (vec.h:1428)
> by 0x550DA3E: vec<pre_expr_d*, va_heap, vl_ptr>::reserve_exact(unsigned int) (vec.h:1448)
> by 0x550D167: vec<pre_expr_d*, va_heap, vl_ptr>::safe_grow(unsigned int) (vec.h:1576)
> by 0x55076FE: do_regular_insertion(basic_block_def*, basic_block_def*) (tree-ssa-pre.c:3209)
> by 0x5508379: insert_aux(basic_block_def*) (tree-ssa-pre.c:3526)
> by 0x55083DC: insert_aux(basic_block_def*) (tree-ssa-pre.c:3536)
> by 0x55083DC: insert_aux(basic_block_def*) (tree-ssa-pre.c:3536)
> by 0x55084C0: insert() (tree-ssa-pre.c:3559)
> by 0x550C5BC: (anonymous namespace)::pass_pre::execute(function*) (tree-ssa-pre.c:4805)
>
> gcc/ChangeLog:
> PR jit/63854
> * tree-ssa-pre.c (do_regular_insertion): Convert "avail" from
> vec<> to auto_vec<> to fix a leak.
> ---
> gcc/tree-ssa-pre.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/gcc/tree-ssa-pre.c b/gcc/tree-ssa-pre.c
> index ea99198..027dc1c 100644
> --- a/gcc/tree-ssa-pre.c
> +++ b/gcc/tree-ssa-pre.c
> @@ -3202,7 +3202,7 @@ do_regular_insertion (basic_block block, basic_block dom)
> bool new_stuff = false;
> vec<pre_expr> exprs;
> pre_expr expr;
> - vec<pre_expr> avail = vNULL;
> + auto_vec<pre_expr> avail;
> int i;
>
> exprs = sorted_array_from_bitmap_set (ANTIC_IN (block));
> --
> 1.8.5.3
>
^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH 06/21] PR jit/63854: Fix leak of opts_obstack
2014-01-01 0:00 [PATCH 00/21] PR 63854: Fix various memory leaks David Malcolm
2014-01-01 0:00 ` [PATCH 11/21] PR jit/63854: Fix leak of "avail" within tree-ssa-pre.c David Malcolm
@ 2014-01-01 0:00 ` David Malcolm
2014-01-01 0:00 ` Richard Biener
2014-01-01 0:00 ` [PATCH 15/21] PR jit/63854: lra.c: Fix leak of point_freq_vec's buffer when calling lra_inheritance David Malcolm
` (18 subsequent siblings)
20 siblings, 1 reply; 69+ messages in thread
From: David Malcolm @ 2014-01-01 0:00 UTC (permalink / raw)
To: gcc-patches, jit; +Cc: David Malcolm
This was leaking about 4KB per iteration:
16,256 bytes in 4 blocks are definitely lost in loss record 233 of 239
at 0x4A0645D: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
by 0x5D75C17: xmalloc (xmalloc.c:147)
by 0x30958842DB: _obstack_begin (obstack.c:184)
by 0x5D1D317: init_options_struct(gcc_options*, gcc_options*) (opts.c:279)
by 0x532DB0C: toplev::main(int, char**) (toplev.c:2081)
by 0x4DE766F: gcc::jit::playback::context::compile() (jit-playback.c:1615)
by 0x4DD76DA: gcc::jit::recording::context::compile() (jit-recording.c:861)
by 0x4DD5BD2: gcc_jit_context_compile (libgccjit.c:2014)
by 0x401CA4: test_jit (harness.h:190)
by 0x401D88: main (harness.h:232)
gcc/ChangeLog:
PR jit/63854
* toplev.c (toplev::finalize): Free opts_obstack.
---
gcc/toplev.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/gcc/toplev.c b/gcc/toplev.c
index 291b84d..0d617c2 100644
--- a/gcc/toplev.c
+++ b/gcc/toplev.c
@@ -2178,4 +2178,6 @@ toplev::finalize (void)
/* Clean up the context (and pass_manager etc). */
delete g;
g = NULL;
+
+ obstack_free (&opts_obstack, NULL);
}
--
1.8.5.3
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH 06/21] PR jit/63854: Fix leak of opts_obstack
2014-01-01 0:00 ` [PATCH 06/21] PR jit/63854: Fix leak of opts_obstack David Malcolm
@ 2014-01-01 0:00 ` Richard Biener
0 siblings, 0 replies; 69+ messages in thread
From: Richard Biener @ 2014-01-01 0:00 UTC (permalink / raw)
To: David Malcolm; +Cc: GCC Patches, jit
On Wed, Nov 19, 2014 at 11:46 AM, David Malcolm <dmalcolm@redhat.com> wrote:
> This was leaking about 4KB per iteration:
Ok.
Thanks,
Richard.
> 16,256 bytes in 4 blocks are definitely lost in loss record 233 of 239
> at 0x4A0645D: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
> by 0x5D75C17: xmalloc (xmalloc.c:147)
> by 0x30958842DB: _obstack_begin (obstack.c:184)
> by 0x5D1D317: init_options_struct(gcc_options*, gcc_options*) (opts.c:279)
> by 0x532DB0C: toplev::main(int, char**) (toplev.c:2081)
> by 0x4DE766F: gcc::jit::playback::context::compile() (jit-playback.c:1615)
> by 0x4DD76DA: gcc::jit::recording::context::compile() (jit-recording.c:861)
> by 0x4DD5BD2: gcc_jit_context_compile (libgccjit.c:2014)
> by 0x401CA4: test_jit (harness.h:190)
> by 0x401D88: main (harness.h:232)
>
> gcc/ChangeLog:
> PR jit/63854
> * toplev.c (toplev::finalize): Free opts_obstack.
> ---
> gcc/toplev.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/gcc/toplev.c b/gcc/toplev.c
> index 291b84d..0d617c2 100644
> --- a/gcc/toplev.c
> +++ b/gcc/toplev.c
> @@ -2178,4 +2178,6 @@ toplev::finalize (void)
> /* Clean up the context (and pass_manager etc). */
> delete g;
> g = NULL;
> +
> + obstack_free (&opts_obstack, NULL);
> }
> --
> 1.8.5.3
>
^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH 15/21] PR jit/63854: lra.c: Fix leak of point_freq_vec's buffer when calling lra_inheritance
2014-01-01 0:00 [PATCH 00/21] PR 63854: Fix various memory leaks David Malcolm
2014-01-01 0:00 ` [PATCH 11/21] PR jit/63854: Fix leak of "avail" within tree-ssa-pre.c David Malcolm
2014-01-01 0:00 ` [PATCH 06/21] PR jit/63854: Fix leak of opts_obstack David Malcolm
@ 2014-01-01 0:00 ` David Malcolm
2014-01-01 0:00 ` Jeff Law
2014-01-01 0:00 ` [PATCH 03/21] PR jit/63854: Fix memory leaks within context/pass_manager/dump_manager David Malcolm
` (17 subsequent siblings)
20 siblings, 1 reply; 69+ messages in thread
From: David Malcolm @ 2014-01-01 0:00 UTC (permalink / raw)
To: gcc-patches, jit; +Cc: David Malcolm
Valgrind showed a leak of 1640 bytes per iteration of one of the jit
testcases (I think this is per-function in a compile):
8,200 bytes in 5 blocks are definitely lost in loss record 214 of 223
at 0x4A0645D: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
by 0x5D75D1F: xrealloc (xmalloc.c:177)
by 0x4E0C94E: void va_heap::reserve<int>(vec<int, va_heap, vl_embed>*&, unsigned int, bool) (vec.h:310)
by 0x4E0C7A3: vec<int, va_heap, vl_ptr>::reserve(unsigned int, bool) (vec.h:1428)
by 0x4E0C55A: vec<int, va_heap, vl_ptr>::reserve_exact(unsigned int) (vec.h:1448)
by 0x4E0C2C6: vec<int, va_heap, vl_ptr>::create(unsigned int) (vec.h:1463)
by 0x519B316: lra_create_live_ranges(bool) (lra-lives.c:1248)
by 0x5179BD7: lra(_IO_FILE*) (lra.c:2297)
by 0x5126C4E: do_reload() (ira.c:5380)
by 0x5127098: (anonymous namespace)::pass_reload::execute(function*) (ira.c:5550)
by 0x52235BD: execute_one_pass(opt_pass*) (passes.c:2306)
by 0x5223834: execute_pass_list_1(opt_pass*) (passes.c:2358)
which is this line:
point_freq_vec.create (get_max_uid () * 2);
point_freq_vec's buffer is released in lra_clear_live_ranges.
Am unfamiliar with LRA, but my reading of the lra code is that the
"live_p" boolean tracks whether or not we need to call
lra_clear_live_ranges.
Debugging calls to the above line and calls to lra_clear_live_ranges
showed a mismatch; the call to lra_create_live_ranges before
lra_inheritance wasn't setting live_p.
Setting it after that callsite fixes the leak (though perhaps the code
could be cleaned up by having live_p be set/cleared by the
allocation/clear functions themselves, rather than by their callers?
Vladimir?)
gcc/ChangeLog:
PR jit/63854
* lra.c (lra): After creating live ranges in preparation for call
to lra_inheritance, set live_p to true.
---
gcc/lra.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/gcc/lra.c b/gcc/lra.c
index 9309d5e..ec122c7 100644
--- a/gcc/lra.c
+++ b/gcc/lra.c
@@ -2297,6 +2297,7 @@ lra (FILE *f)
actual_call_used_reg_set, which is needed during
lra_inheritance. */
lra_create_live_ranges (true, true);
+ live_p = true;
}
lra_inheritance ();
}
--
1.8.5.3
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH 15/21] PR jit/63854: lra.c: Fix leak of point_freq_vec's buffer when calling lra_inheritance
2014-01-01 0:00 ` [PATCH 15/21] PR jit/63854: lra.c: Fix leak of point_freq_vec's buffer when calling lra_inheritance David Malcolm
@ 2014-01-01 0:00 ` Jeff Law
0 siblings, 0 replies; 69+ messages in thread
From: Jeff Law @ 2014-01-01 0:00 UTC (permalink / raw)
To: David Malcolm, gcc-patches, jit
On 11/19/14 03:46, David Malcolm wrote:
> Valgrind showed a leak of 1640 bytes per iteration of one of the jit
> testcases (I think this is per-function in a compile):
>
> 8,200 bytes in 5 blocks are definitely lost in loss record 214 of 223
> at 0x4A0645D: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
> by 0x5D75D1F: xrealloc (xmalloc.c:177)
> by 0x4E0C94E: void va_heap::reserve<int>(vec<int, va_heap, vl_embed>*&, unsigned int, bool) (vec.h:310)
> by 0x4E0C7A3: vec<int, va_heap, vl_ptr>::reserve(unsigned int, bool) (vec.h:1428)
> by 0x4E0C55A: vec<int, va_heap, vl_ptr>::reserve_exact(unsigned int) (vec.h:1448)
> by 0x4E0C2C6: vec<int, va_heap, vl_ptr>::create(unsigned int) (vec.h:1463)
> by 0x519B316: lra_create_live_ranges(bool) (lra-lives.c:1248)
> by 0x5179BD7: lra(_IO_FILE*) (lra.c:2297)
> by 0x5126C4E: do_reload() (ira.c:5380)
> by 0x5127098: (anonymous namespace)::pass_reload::execute(function*) (ira.c:5550)
> by 0x52235BD: execute_one_pass(opt_pass*) (passes.c:2306)
> by 0x5223834: execute_pass_list_1(opt_pass*) (passes.c:2358)
>
> which is this line:
> point_freq_vec.create (get_max_uid () * 2);
>
> point_freq_vec's buffer is released in lra_clear_live_ranges.
>
> Am unfamiliar with LRA, but my reading of the lra code is that the
> "live_p" boolean tracks whether or not we need to call
> lra_clear_live_ranges.
>
> Debugging calls to the above line and calls to lra_clear_live_ranges
> showed a mismatch; the call to lra_create_live_ranges before
> lra_inheritance wasn't setting live_p.
>
> Setting it after that callsite fixes the leak (though perhaps the code
> could be cleaned up by having live_p be set/cleared by the
> allocation/clear functions themselves, rather than by their callers?
> Vladimir?)
>
> gcc/ChangeLog:
> PR jit/63854
> * lra.c (lra): After creating live ranges in preparation for call
> to lra_inheritance, set live_p to true.
OK for the trunk.
And yes, a followup where live_p's handling moves into
lra_create_live_ranges or something similar would be good.
Jeff
^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH 03/21] PR jit/63854: Fix memory leaks within context/pass_manager/dump_manager
2014-01-01 0:00 [PATCH 00/21] PR 63854: Fix various memory leaks David Malcolm
` (2 preceding siblings ...)
2014-01-01 0:00 ` [PATCH 15/21] PR jit/63854: lra.c: Fix leak of point_freq_vec's buffer when calling lra_inheritance David Malcolm
@ 2014-01-01 0:00 ` David Malcolm
2014-01-01 0:00 ` Jeff Law
2014-01-01 0:00 ` [PATCH 17/21] PR jit/63854: Fix leaking vec in jit David Malcolm
` (16 subsequent siblings)
20 siblings, 1 reply; 69+ messages in thread
From: David Malcolm @ 2014-01-01 0:00 UTC (permalink / raw)
To: gcc-patches, jit; +Cc: David Malcolm
We weren't cleaning up the context, pass_manager or dump_manager.
An earlier version of the context and pass_manager classes had them
allocated in the GC-heap, but they were moved to the system heap
before that patch was committed; they were never cleaned up, so e.g.
all passes leak on each in-process iteration.
Cleaning all of this up fixes the largest of the leaks in the PR,
about 60KB per iteration:
==57820== 80,560 (88 direct, 80,472 indirect) bytes in 1 blocks are definitely lost in loss record 913 of 917
==57820== at 0x4A06965: operator new(unsigned long) (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==57820== by 0x536A462: make_pass_dce(gcc::context*) (tree-ssa-dce.c:1703)
==57820== by 0x518B8DA: gcc::pass_manager::pass_manager(gcc::context*) (pass-instances.def:173)
==57820== by 0x4E8D6D9: gcc::context::context() (context.c:37)
==57820== by 0x4E2ED19: toplev::main(int, char**) (toplev.c:1211)
==57820== by 0x4E43186: gcc::jit::playback::context::compile() (jit-playback.c:1615)
==57820== by 0x4E4018D: gcc::jit::recording::context::compile() (jit-recording.c:861)
==57820== by 0x401CA4: test_jit (harness.h:190)
==57820== by 0x401D88: main (harness.h:232)
==57820==
==57820== 161,488 (352 direct, 161,136 indirect) bytes in 4 blocks are definitely lost in loss record 917 of 917
==57820== at 0x4A06965: operator new(unsigned long) (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==57820== by 0x5553102: make_pass_insert_vzeroupper(gcc::context*) (i386.c:2581)
==57820== by 0x55571DF: ix86_option_override() (i386.c:4325)
==57820== by 0x4E2EEF4: toplev::main(int, char**) (toplev.c:1295)
==57820== by 0x4E43186: gcc::jit::playback::context::compile() (jit-playback.c:1615)
==57820== by 0x4E4018D: gcc::jit::recording::context::compile() (jit-recording.c:861)
==57820== by 0x401CA4: test_jit (harness.h:190)
==57820== by 0x401D88: main (harness.h:232)
All of this additional cleanup happens within toplev::finalize, and
hence we don't call it from cc1, cc1plus, etc, only from libgccjit.so.
Note that some calls to register_pass_info were using a static:
static struct register_pass_info
on the stack e.g.:
opt_pass *pass_handle_trap_shadows = make_pass_handle_trap_shadows (g);
static struct register_pass_info handle_trap_shadows_info
= { pass_handle_trap_shadows, "eh_ranges",
1, PASS_POS_INSERT_AFTER
};
This led to crashes on the 2nd iteration: the on-stack struct was only
being built on the first iteration, using the result of the first call
to the "make_pass_" function. Subsequent calls would thus be
registering a freed pass, giving a use-after-free error (which was
previously hidden because we weren't freeing them).
The fix is to remove the "static" from such structs.
This also fixes a leak of strings within dump_file_info for passes.
gcc/ChangeLog:
PR jit/63854
* config/alpha/alpha.c (alpha_option_override): Remove static from
"handle_trap_shadows_info" and "align_insns_info".
* config/i386/i386.c (ix86_option_override): Likewise for
"insert_vzeroupper_info".
* config/rl78/rl78.c (rl78_asm_file_start): Likewise for
"rl78_devirt_info" and "rl78_move_elim_info".
* config/rs6000/rs6000.c (rs6000_option_override): Likewise for
"analyze_swaps_info".
* context.c (gcc::context::~context): New.
* context.h (gcc::context::~context): New.
* dumpfile.c (dump_files): Add "false" initializers for new field
"owns_strings".
(gcc::dump_manager::~dump_manager): New.
(gcc::dump_manager::dump_register): Add param "take_ownership".
* dumpfile.h (struct dump_file_info): Add field "owns_strings".
(gcc::dump_manager::~dump_manager): New.
(gcc::dump_manager::dump_register): Add param "take_ownership".
* pass_manager.h (gcc::pass_manager::operator delete): New.
(gcc::pass_manager::~pass_manager): New.
* passes.c (pass_manager::register_one_dump_file): Pass "true" to
new "owns_strings" argument to dump_register.
(pass_manager::operator delete): New.
(delete_pass_tree): New function.
(pass_manager::~pass_manager): New.
* statistics.c (statistics_early_init): Pass "false" to
new "owns_strings" argument to dump_register.
* toplev.c (toplev::finalize): Clean up the context and thus the
things it owns.
---
gcc/config/alpha/alpha.c | 4 ++--
gcc/config/i386/i386.c | 2 +-
gcc/config/rl78/rl78.c | 4 ++--
gcc/config/rs6000/rs6000.c | 2 +-
gcc/context.c | 6 ++++++
gcc/context.h | 1 +
gcc/dumpfile.c | 47 ++++++++++++++++++++++++++++++++++------------
gcc/dumpfile.h | 11 ++++++++++-
gcc/pass_manager.h | 2 ++
gcc/passes.c | 39 +++++++++++++++++++++++++++++++++++++-
gcc/statistics.c | 3 ++-
gcc/toplev.c | 4 ++++
12 files changed, 104 insertions(+), 21 deletions(-)
diff --git a/gcc/config/alpha/alpha.c b/gcc/config/alpha/alpha.c
index 05db3dc..1c89288 100644
--- a/gcc/config/alpha/alpha.c
+++ b/gcc/config/alpha/alpha.c
@@ -399,13 +399,13 @@ alpha_option_override (void)
};
opt_pass *pass_handle_trap_shadows = make_pass_handle_trap_shadows (g);
- static struct register_pass_info handle_trap_shadows_info
+ struct register_pass_info handle_trap_shadows_info
= { pass_handle_trap_shadows, "eh_ranges",
1, PASS_POS_INSERT_AFTER
};
opt_pass *pass_align_insns = make_pass_align_insns (g);
- static struct register_pass_info align_insns_info
+ struct register_pass_info align_insns_info
= { pass_align_insns, "shorten",
1, PASS_POS_INSERT_BEFORE
};
diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index 3166e03..784982d 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -4323,7 +4323,7 @@ static void
ix86_option_override (void)
{
opt_pass *pass_insert_vzeroupper = make_pass_insert_vzeroupper (g);
- static struct register_pass_info insert_vzeroupper_info
+ struct register_pass_info insert_vzeroupper_info
= { pass_insert_vzeroupper, "reload",
1, PASS_POS_INSERT_AFTER
};
diff --git a/gcc/config/rl78/rl78.c b/gcc/config/rl78/rl78.c
index 7b85cf8..86d2992 100644
--- a/gcc/config/rl78/rl78.c
+++ b/gcc/config/rl78/rl78.c
@@ -284,7 +284,7 @@ rl78_asm_file_start (void)
}
opt_pass *rl78_devirt_pass = make_pass_rl78_devirt (g);
- static struct register_pass_info rl78_devirt_info =
+ struct register_pass_info rl78_devirt_info =
{
rl78_devirt_pass,
"pro_and_epilogue",
@@ -293,7 +293,7 @@ rl78_asm_file_start (void)
};
opt_pass *rl78_move_elim_pass = make_pass_rl78_move_elim (g);
- static struct register_pass_info rl78_move_elim_info =
+ struct register_pass_info rl78_move_elim_info =
{
rl78_move_elim_pass,
"bbro",
diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
index 4f66840..506daa1 100644
--- a/gcc/config/rs6000/rs6000.c
+++ b/gcc/config/rs6000/rs6000.c
@@ -4202,7 +4202,7 @@ rs6000_option_override (void)
It's convenient to do it here (like i386 does). */
opt_pass *pass_analyze_swaps = make_pass_analyze_swaps (g);
- static struct register_pass_info analyze_swaps_info
+ struct register_pass_info analyze_swaps_info
= { pass_analyze_swaps, "cse1", 1, PASS_POS_INSERT_BEFORE };
register_pass (&analyze_swaps_info);
diff --git a/gcc/context.c b/gcc/context.c
index 9279be4..d132946 100644
--- a/gcc/context.c
+++ b/gcc/context.c
@@ -38,3 +38,9 @@ gcc::context::context ()
m_dumps = new gcc::dump_manager ();
m_passes = new gcc::pass_manager (this);
}
+
+gcc::context::~context ()
+{
+ delete m_passes;
+ delete m_dumps;
+}
diff --git a/gcc/context.h b/gcc/context.h
index 689ae5a..2bf28a7 100644
--- a/gcc/context.h
+++ b/gcc/context.h
@@ -32,6 +32,7 @@ class context
{
public:
context ();
+ ~context ();
/* The flag shows if there are symbols to be streamed for offloading. */
bool have_offload;
diff --git a/gcc/dumpfile.c b/gcc/dumpfile.c
index fca7b51..c2cd89b 100644
--- a/gcc/dumpfile.c
+++ b/gcc/dumpfile.c
@@ -49,29 +49,29 @@ int dump_flags;
TREE_DUMP_INDEX enumeration in dumpfile.h. */
static struct dump_file_info dump_files[TDI_end] =
{
- {NULL, NULL, NULL, NULL, NULL, NULL, NULL, 0, 0, 0, 0, 0, 0},
+ {NULL, NULL, NULL, NULL, NULL, NULL, NULL, 0, 0, 0, 0, 0, 0, false},
{".cgraph", "ipa-cgraph", NULL, NULL, NULL, NULL, NULL, TDF_IPA,
- 0, 0, 0, 0, 0},
+ 0, 0, 0, 0, 0, false},
{".type-inheritance", "ipa-type-inheritance", NULL, NULL, NULL, NULL, NULL, TDF_IPA,
- 0, 0, 0, 0, 0},
+ 0, 0, 0, 0, 0, false},
{".tu", "translation-unit", NULL, NULL, NULL, NULL, NULL, TDF_TREE,
- 0, 0, 0, 0, 1},
+ 0, 0, 0, 0, 1, false},
{".class", "class-hierarchy", NULL, NULL, NULL, NULL, NULL, TDF_TREE,
- 0, 0, 0, 0, 2},
+ 0, 0, 0, 0, 2, false},
{".original", "tree-original", NULL, NULL, NULL, NULL, NULL, TDF_TREE,
- 0, 0, 0, 0, 3},
+ 0, 0, 0, 0, 3, false},
{".gimple", "tree-gimple", NULL, NULL, NULL, NULL, NULL, TDF_TREE,
- 0, 0, 0, 0, 4},
+ 0, 0, 0, 0, 4, false},
{".nested", "tree-nested", NULL, NULL, NULL, NULL, NULL, TDF_TREE,
- 0, 0, 0, 0, 5},
+ 0, 0, 0, 0, 5, false},
#define FIRST_AUTO_NUMBERED_DUMP 6
{NULL, "tree-all", NULL, NULL, NULL, NULL, NULL, TDF_TREE,
- 0, 0, 0, 0, 0},
+ 0, 0, 0, 0, 0, false},
{NULL, "rtl-all", NULL, NULL, NULL, NULL, NULL, TDF_RTL,
- 0, 0, 0, 0, 0},
+ 0, 0, 0, 0, 0, false},
{NULL, "ipa-all", NULL, NULL, NULL, NULL, NULL, TDF_IPA,
- 0, 0, 0, 0, 0},
+ 0, 0, 0, 0, 0, false},
};
/* Define a name->number mapping for a dump flag value. */
@@ -148,10 +148,32 @@ gcc::dump_manager::dump_manager ():
{
}
+gcc::dump_manager::~dump_manager ()
+{
+ for (size_t i = 0; i < m_extra_dump_files_in_use; i++)
+ {
+ dump_file_info *dfi = &m_extra_dump_files[i];
+ /* suffix, swtch, glob are statically allocated for the entries
+ in dump_files, and for statistics, but are dynamically allocated
+ for those for passes. */
+ if (dfi->owns_strings)
+ {
+ XDELETEVEC (const_cast <char *> (dfi->suffix));
+ XDELETEVEC (const_cast <char *> (dfi->swtch));
+ XDELETEVEC (const_cast <char *> (dfi->glob));
+ }
+ /* These, if non-NULL, are always dynamically allocated. */
+ XDELETEVEC (const_cast <char *> (dfi->pfilename));
+ XDELETEVEC (const_cast <char *> (dfi->alt_filename));
+ }
+ XDELETEVEC (m_extra_dump_files);
+}
+
unsigned int
gcc::dump_manager::
dump_register (const char *suffix, const char *swtch, const char *glob,
- int flags, int optgroup_flags)
+ int flags, int optgroup_flags,
+ bool take_ownership)
{
int num = m_next_dump++;
@@ -175,6 +197,7 @@ dump_register (const char *suffix, const char *swtch, const char *glob,
m_extra_dump_files[count].pflags = flags;
m_extra_dump_files[count].optgroup_flags = optgroup_flags;
m_extra_dump_files[count].num = num;
+ m_extra_dump_files[count].owns_strings = take_ownership;
return count + TDI_end;
}
diff --git a/gcc/dumpfile.h b/gcc/dumpfile.h
index 75949b7..d650174 100644
--- a/gcc/dumpfile.h
+++ b/gcc/dumpfile.h
@@ -118,6 +118,9 @@ struct dump_file_info
int pstate; /* state of pass-specific stream */
int alt_state; /* state of the -fopt-info stream */
int num; /* dump file number */
+ bool owns_strings; /* fields "suffix", "swtch", "glob" can be
+ const strings, or can be dynamically
+ allocated, needing free. */
};
/* In dumpfile.c */
@@ -164,10 +167,16 @@ class dump_manager
public:
dump_manager ();
+ ~dump_manager ();
+ /* Register a dumpfile.
+
+ TAKE_OWNERSHIP determines whether callee takes ownership of strings
+ SUFFIX, SWTCH, and GLOB. */
unsigned int
dump_register (const char *suffix, const char *swtch, const char *glob,
- int flags, int optgroup_flags);
+ int flags, int optgroup_flags,
+ bool take_ownership);
/* Return the dump_file_info for the given phase. */
struct dump_file_info *
diff --git a/gcc/pass_manager.h b/gcc/pass_manager.h
index 9f4d67b..82857a9 100644
--- a/gcc/pass_manager.h
+++ b/gcc/pass_manager.h
@@ -47,8 +47,10 @@ class pass_manager
{
public:
void *operator new (size_t sz);
+ void operator delete (void *ptr);
pass_manager (context *ctxt);
+ ~pass_manager ();
void register_pass (struct register_pass_info *pass_info);
void register_one_dump_file (opt_pass *pass);
diff --git a/gcc/passes.c b/gcc/passes.c
index c818d8a..f7a0170 100644
--- a/gcc/passes.c
+++ b/gcc/passes.c
@@ -776,7 +776,8 @@ pass_manager::register_one_dump_file (opt_pass *pass)
if (optgroup_flags == OPTGROUP_NONE)
optgroup_flags = OPTGROUP_OTHER;
id = dumps->dump_register (dot_name, flag_name, glob_name, flags,
- optgroup_flags);
+ optgroup_flags,
+ true);
set_pass_for_id (id, pass);
full_name = concat (prefix, pass->name, num, NULL);
register_pass_name (pass, full_name);
@@ -1527,6 +1528,12 @@ pass_manager::operator new (size_t sz)
return xcalloc (1, sz);
}
+void
+pass_manager::operator delete (void *ptr)
+{
+ free (ptr);
+}
+
pass_manager::pass_manager (context *ctxt)
: all_passes (NULL), all_small_ipa_passes (NULL), all_lowering_passes (NULL),
all_regular_ipa_passes (NULL),
@@ -1584,6 +1591,36 @@ pass_manager::pass_manager (context *ctxt)
register_dump_files (all_passes);
}
+static void
+delete_pass_tree (opt_pass *pass)
+{
+ while (pass)
+ {
+ /* Recurse into child passes. */
+ delete_pass_tree (pass->sub);
+
+ opt_pass *next = pass->next;
+
+ /* Delete this pass. */
+ delete pass;
+
+ /* Iterate onto sibling passes. */
+ pass = next;
+ }
+}
+
+pass_manager::~pass_manager ()
+{
+ XDELETEVEC (passes_by_id);
+
+ /* Call delete_pass_tree on each of the pass_lists. */
+#define DEF_PASS_LIST(LIST) \
+ delete_pass_tree (*pass_lists[PASS_LIST_NO_##LIST]);
+ GCC_PASS_LISTS
+#undef DEF_PASS_LIST
+
+}
+
/* If we are in IPA mode (i.e., current_function_decl is NULL), call
function CALLBACK for every function in the call graph. Otherwise,
call CALLBACK on the current function. */
diff --git a/gcc/statistics.c b/gcc/statistics.c
index b3e63de..0ceb2e9 100644
--- a/gcc/statistics.c
+++ b/gcc/statistics.c
@@ -270,7 +270,8 @@ statistics_early_init (void)
gcc::dump_manager *dumps = g->get_dumps ();
statistics_dump_nr = dumps->dump_register (".statistics", "statistics",
"statistics", TDF_TREE,
- OPTGROUP_NONE);
+ OPTGROUP_NONE,
+ false);
}
/* Init the statistics. */
diff --git a/gcc/toplev.c b/gcc/toplev.c
index 4b4e568..876279f 100644
--- a/gcc/toplev.c
+++ b/gcc/toplev.c
@@ -2172,4 +2172,8 @@ toplev::finalize (void)
finalize_options_struct (&global_options);
finalize_options_struct (&global_options_set);
+
+ /* Clean up the context (and pass_manager etc). */
+ delete g;
+ g = NULL;
}
--
1.8.5.3
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH 03/21] PR jit/63854: Fix memory leaks within context/pass_manager/dump_manager
2014-01-01 0:00 ` [PATCH 03/21] PR jit/63854: Fix memory leaks within context/pass_manager/dump_manager David Malcolm
@ 2014-01-01 0:00 ` Jeff Law
0 siblings, 0 replies; 69+ messages in thread
From: Jeff Law @ 2014-01-01 0:00 UTC (permalink / raw)
To: David Malcolm, gcc-patches, jit
On 11/19/14 03:46, David Malcolm wrote:
> We weren't cleaning up the context, pass_manager or dump_manager.
>
> An earlier version of the context and pass_manager classes had them
> allocated in the GC-heap, but they were moved to the system heap
> before that patch was committed; they were never cleaned up, so e.g.
> all passes leak on each in-process iteration.
>
> Cleaning all of this up fixes the largest of the leaks in the PR,
> about 60KB per iteration:
>
> ==57820== 80,560 (88 direct, 80,472 indirect) bytes in 1 blocks are definitely lost in loss record 913 of 917
> ==57820== at 0x4A06965: operator new(unsigned long) (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==57820== by 0x536A462: make_pass_dce(gcc::context*) (tree-ssa-dce.c:1703)
> ==57820== by 0x518B8DA: gcc::pass_manager::pass_manager(gcc::context*) (pass-instances.def:173)
> ==57820== by 0x4E8D6D9: gcc::context::context() (context.c:37)
> ==57820== by 0x4E2ED19: toplev::main(int, char**) (toplev.c:1211)
> ==57820== by 0x4E43186: gcc::jit::playback::context::compile() (jit-playback.c:1615)
> ==57820== by 0x4E4018D: gcc::jit::recording::context::compile() (jit-recording.c:861)
> ==57820== by 0x401CA4: test_jit (harness.h:190)
> ==57820== by 0x401D88: main (harness.h:232)
> ==57820==
> ==57820== 161,488 (352 direct, 161,136 indirect) bytes in 4 blocks are definitely lost in loss record 917 of 917
> ==57820== at 0x4A06965: operator new(unsigned long) (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==57820== by 0x5553102: make_pass_insert_vzeroupper(gcc::context*) (i386.c:2581)
> ==57820== by 0x55571DF: ix86_option_override() (i386.c:4325)
> ==57820== by 0x4E2EEF4: toplev::main(int, char**) (toplev.c:1295)
> ==57820== by 0x4E43186: gcc::jit::playback::context::compile() (jit-playback.c:1615)
> ==57820== by 0x4E4018D: gcc::jit::recording::context::compile() (jit-recording.c:861)
> ==57820== by 0x401CA4: test_jit (harness.h:190)
> ==57820== by 0x401D88: main (harness.h:232)
>
> All of this additional cleanup happens within toplev::finalize, and
> hence we don't call it from cc1, cc1plus, etc, only from libgccjit.so.
>
> Note that some calls to register_pass_info were using a static:
> static struct register_pass_info
> on the stack e.g.:
>
> opt_pass *pass_handle_trap_shadows = make_pass_handle_trap_shadows (g);
> static struct register_pass_info handle_trap_shadows_info
> = { pass_handle_trap_shadows, "eh_ranges",
> 1, PASS_POS_INSERT_AFTER
> };
>
> This led to crashes on the 2nd iteration: the on-stack struct was only
> being built on the first iteration, using the result of the first call
> to the "make_pass_" function. Subsequent calls would thus be
> registering a freed pass, giving a use-after-free error (which was
> previously hidden because we weren't freeing them).
>
> The fix is to remove the "static" from such structs.
>
> This also fixes a leak of strings within dump_file_info for passes.
>
> gcc/ChangeLog:
> PR jit/63854
> * config/alpha/alpha.c (alpha_option_override): Remove static from
> "handle_trap_shadows_info" and "align_insns_info".
> * config/i386/i386.c (ix86_option_override): Likewise for
> "insert_vzeroupper_info".
> * config/rl78/rl78.c (rl78_asm_file_start): Likewise for
> "rl78_devirt_info" and "rl78_move_elim_info".
> * config/rs6000/rs6000.c (rs6000_option_override): Likewise for
> "analyze_swaps_info".
> * context.c (gcc::context::~context): New.
> * context.h (gcc::context::~context): New.
> * dumpfile.c (dump_files): Add "false" initializers for new field
> "owns_strings".
> (gcc::dump_manager::~dump_manager): New.
> (gcc::dump_manager::dump_register): Add param "take_ownership".
> * dumpfile.h (struct dump_file_info): Add field "owns_strings".
> (gcc::dump_manager::~dump_manager): New.
> (gcc::dump_manager::dump_register): Add param "take_ownership".
> * pass_manager.h (gcc::pass_manager::operator delete): New.
> (gcc::pass_manager::~pass_manager): New.
> * passes.c (pass_manager::register_one_dump_file): Pass "true" to
> new "owns_strings" argument to dump_register.
> (pass_manager::operator delete): New.
> (delete_pass_tree): New function.
> (pass_manager::~pass_manager): New.
> * statistics.c (statistics_early_init): Pass "false" to
> new "owns_strings" argument to dump_register.
> * toplev.c (toplev::finalize): Clean up the context and thus the
> things it owns.
You could strdup the strings coming out of the tables, then remove the
take_ownership stuff. There's an obvious tradeoff there, memory for a
bit more simplicity.
I'm OK with the patch as-is.
jeff
^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH 17/21] PR jit/63854: Fix leaking vec in jit
2014-01-01 0:00 [PATCH 00/21] PR 63854: Fix various memory leaks David Malcolm
` (3 preceding siblings ...)
2014-01-01 0:00 ` [PATCH 03/21] PR jit/63854: Fix memory leaks within context/pass_manager/dump_manager David Malcolm
@ 2014-01-01 0:00 ` David Malcolm
2014-01-01 0:00 ` Jeff Law
2014-01-01 0:00 ` [PATCH 16/21] PR jit/63854: Add all_late_ipa_passes to GCC_PASS_LISTS David Malcolm
` (15 subsequent siblings)
20 siblings, 1 reply; 69+ messages in thread
From: David Malcolm @ 2014-01-01 0:00 UTC (permalink / raw)
To: gcc-patches, jit; +Cc: David Malcolm
This fixes various leaks of vec buffers seen via valgrind within jit
(both recording and playback).
Various vec<> within jit::recording are converted to auto_vec<>.
Various playback::wrapper subclasses containing vec<> gain a finalizer
so they can release the vec when they are collected.
gcc/jit/ChangeLog:
PR jit/63854
* jit-playback.c (gcc::jit::playback::compound_type::set_fields):
Convert param from const vec<playback::field *> & to
const auto_vec<playback::field *> *.
(gcc::jit::playback::context::new_function_type): Convert param
"param_types" from vec<type *> * to const auto_vec<type *> *.
(gcc::jit::playback::context::new_function): Convert param
"params" from vec<param *> * to const auto_vec<param *> *.
(gcc::jit::playback::context::build_call): Convert param "args"
from vec<rvalue *> to const auto_vec<rvalue *> *.
(gcc::jit::playback::context::new_call): Likewise.
(gcc::jit::playback::context::new_call_through_ptr): Likewise.
(wrapper_finalizer): New function.
(gcc::jit::playback::wrapper::operator new): Call the finalizer
variant of ggc_internal_cleared_alloc, supplying
wrapper_finalizer.
(gcc::jit::playback::function::finalizer): New.
(gcc::jit::playback::block::finalizer): New.
(gcc::jit::playback::source_file::finalizer): New.
(gcc::jit::playback::source_line::finalizer): New.
* jit-playback.h
(gcc::jit::playback::context::new_function_type): Convert param
"param_types" from vec<type *> * to const auto_vec<type *> *.
(gcc::jit::playback::context::new_function): Convert param
"params" from vec<param *> * to const auto_vec<param *> *.
(gcc::jit::playback::context::new_call): Convert param
"args" from vec<rvalue *> to const auto_vec<rvalue *> *.
(gcc::jit::playback::context::new_call_through_ptr): Likewise.
(gcc::jit::playback::context::build_call): Likewise.
(gcc::jit::playback::context): Convert fields "m_functions",
"m_source_files", "m_cached_locations" from vec to auto_vec.
(gcc::jit::playback::wrapper::finalizer): New virtual function.
(gcc::jit::playback::compound_type::set_fields): Convert param fro
const vec<playback::field *> & to
const auto_vec<playback::field *> *.
(gcc::jit::playback::function::finalizer): New.
(gcc::jit::playback::block::finalizer): New.
(gcc::jit::playback::source_file::finalizer): New.
(gcc::jit::playback::source_line::finalizer): New.
* jit-recording.c
(gcc::jit::recording::function_type::replay_into): Convert local
from a vec into an auto_vec.
(gcc::jit::recording::fields::replay_into): Likewise.
(gcc::jit::recording::function::replay_into): Likewise.
(gcc::jit::recording::call::replay_into): Likewise.
(gcc::jit::recording::call_through_ptr::replay_into): Likewise.
* jit-recording.h (gcc::jit::recording::context): Convert fields
"m_mementos", "m_compound_types", "m_functions" from vec<> to
auto_vec <>.
(gcc::jit::recording::function_type::get_param_types): Convert
return type from vec<type *> to const vec<type *> &.
(gcc::jit::recording::function_type): Convert field
"m_param_types" from a vec<> to an auto_vec<>.
(gcc::jit::recording::fields): Likewise for field "m_fields".
(gcc::jit::recording::function::get_params): Convert return type
from vec <param *> to const vec<param *> &.
(gcc::jit::recording::function): Convert fields "m_params",
"m_locals", "m_blocks" from vec<> to auto_vec<>.
(gcc::jit::recording::block): Likewise for field "m_statements".
vec<> to auto_vec<>.
(gcc::jit::recording::call): Likewise for field "m_args".
(gcc::jit::recording::call_through_ptr): Likewise.
---
gcc/jit/jit-playback.c | 73 +++++++++++++++++++++++++++++++++++++++++--------
gcc/jit/jit-playback.h | 27 ++++++++++++------
gcc/jit/jit-recording.c | 16 +++++------
gcc/jit/jit-recording.h | 26 +++++++++---------
4 files changed, 100 insertions(+), 42 deletions(-)
diff --git a/gcc/jit/jit-playback.c b/gcc/jit/jit-playback.c
index 285a3ef..8fdfa29 100644
--- a/gcc/jit/jit-playback.c
+++ b/gcc/jit/jit-playback.c
@@ -285,15 +285,15 @@ new_compound_type (location *loc,
}
void
-playback::compound_type::set_fields (const vec<playback::field *> &fields)
+playback::compound_type::set_fields (const auto_vec<playback::field *> *fields)
{
/* Compare with c/c-decl.c: finish_struct. */
tree t = as_tree ();
tree fieldlist = NULL;
- for (unsigned i = 0; i < fields.length (); i++)
+ for (unsigned i = 0; i < fields->length (); i++)
{
- field *f = fields[i];
+ field *f = (*fields)[i];
DECL_CONTEXT (f->as_tree ()) = t;
fieldlist = chainon (f->as_tree (), fieldlist);
}
@@ -309,7 +309,7 @@ playback::compound_type::set_fields (const vec<playback::field *> &fields)
playback::type *
playback::context::
new_function_type (type *return_type,
- vec<type *> *param_types,
+ const auto_vec<type *> *param_types,
int is_variadic)
{
int i;
@@ -361,7 +361,7 @@ new_function (location *loc,
enum gcc_jit_function_kind kind,
type *return_type,
const char *name,
- vec<param *> *params,
+ const auto_vec<param *> *params,
int is_variadic,
enum built_in_function builtin_id)
{
@@ -770,12 +770,12 @@ playback::rvalue *
playback::context::
build_call (location *loc,
tree fn_ptr,
- vec<rvalue *> args)
+ const auto_vec<rvalue *> *args)
{
vec<tree, va_gc> *tree_args;
- vec_alloc (tree_args, args.length ());
- for (unsigned i = 0; i < args.length (); i++)
- tree_args->quick_push (args[i]->as_tree ());
+ vec_alloc (tree_args, args->length ());
+ for (unsigned i = 0; i < args->length (); i++)
+ tree_args->quick_push ((*args)[i]->as_tree ());
if (loc)
set_tree_location (fn_ptr, loc);
@@ -806,7 +806,7 @@ playback::rvalue *
playback::context::
new_call (location *loc,
function *func,
- vec<rvalue *> args)
+ const auto_vec<rvalue *> *args)
{
tree fndecl;
@@ -828,7 +828,7 @@ playback::rvalue *
playback::context::
new_call_through_ptr (location *loc,
rvalue *fn_ptr,
- vec<rvalue *> args)
+ const auto_vec<rvalue *> *args)
{
gcc_assert (fn_ptr);
tree t_fn_ptr = fn_ptr->as_tree ();
@@ -1079,6 +1079,18 @@ get_address (location *loc)
return new rvalue (get_context (), ptr);
}
+/* The wrapper subclasses are GC-managed, but can own non-GC memory.
+ Provide this finalization hook for calling then they are collected,
+ which calls the finalizer vfunc. This allows them to call "release"
+ on any vec<> within them. */
+
+static void
+wrapper_finalizer (void *ptr)
+{
+ playback::wrapper *wrapper = reinterpret_cast <playback::wrapper *> (ptr);
+ wrapper->finalizer ();
+}
+
/* gcc::jit::playback::wrapper subclasses are GC-managed:
allocate them using ggc_internal_cleared_alloc. */
@@ -1086,7 +1098,8 @@ void *
playback::wrapper::
operator new (size_t sz)
{
- return ggc_internal_cleared_alloc (sz MEM_STAT_INFO);
+ return ggc_internal_cleared_alloc (sz, wrapper_finalizer, 0, 1);
+
}
/* Constructor for gcc:jit::playback::function. */
@@ -1128,6 +1141,15 @@ gt_ggc_mx ()
gt_ggc_m_9tree_node (m_inner_block);
}
+/* Don't leak vec's internal buffer (in non-GC heap) when we are
+ GC-ed. */
+
+void
+playback::function::finalizer ()
+{
+ m_blocks.release ();
+}
+
/* Get the return type of a playback function, in tree form. */
tree
@@ -1262,6 +1284,15 @@ postprocess ()
}
}
+/* Don't leak vec's internal buffer (in non-GC heap) when we are
+ GC-ed. */
+
+void
+playback::block::finalizer ()
+{
+ m_stmts.release ();
+}
+
/* Add an eval of the rvalue to the function's statement list. */
void
@@ -2024,6 +2055,15 @@ playback::source_file::source_file (tree filename) :
{
}
+/* Don't leak vec's internal buffer (in non-GC heap) when we are
+ GC-ed. */
+
+void
+playback::source_file::finalizer ()
+{
+ m_source_lines.release ();
+}
+
/* Construct a playback::source_line for the given line
within this source file, if one doesn't exist already. */
@@ -2056,6 +2096,15 @@ playback::source_line::source_line (source_file *file, int line_num) :
{
}
+/* Don't leak vec's internal buffer (in non-GC heap) when we are
+ GC-ed. */
+
+void
+playback::source_line::finalizer ()
+{
+ m_locations.release ();
+}
+
/* Construct a playback::location for the given column
within this line of a specific source file, if one doesn't exist
already. */
diff --git a/gcc/jit/jit-playback.h b/gcc/jit/jit-playback.h
index dcb19bf..30e9229 100644
--- a/gcc/jit/jit-playback.h
+++ b/gcc/jit/jit-playback.h
@@ -71,7 +71,7 @@ public:
type *
new_function_type (type *return_type,
- vec<type *> *param_types,
+ const auto_vec<type *> *param_types,
int is_variadic);
param *
@@ -84,7 +84,7 @@ public:
enum gcc_jit_function_kind kind,
type *return_type,
const char *name,
- vec<param *> *params,
+ const auto_vec<param *> *params,
int is_variadic,
enum built_in_function builtin_id);
@@ -128,12 +128,12 @@ public:
rvalue *
new_call (location *loc,
function *func,
- vec<rvalue *> args);
+ const auto_vec<rvalue *> *args);
rvalue *
new_call_through_ptr (location *loc,
rvalue *fn_ptr,
- vec<rvalue *> args);
+ const auto_vec<rvalue *> *args);
rvalue *
new_cast (location *loc,
@@ -214,7 +214,7 @@ private:
rvalue *
build_call (location *loc,
tree fn_ptr,
- vec<rvalue *> args);
+ const auto_vec<rvalue *> *args);
tree
build_cast (location *loc,
@@ -240,14 +240,14 @@ private:
char *m_path_s_file;
char *m_path_so_file;
- vec<function *> m_functions;
+ auto_vec<function *> m_functions;
tree m_char_array_type_node;
tree m_const_char_ptr;
/* Source location handling. */
- vec<source_file *> m_source_files;
+ auto_vec<source_file *> m_source_files;
- vec<std::pair<tree, location *> > m_cached_locations;
+ auto_vec<std::pair<tree, location *> > m_cached_locations;
};
/* A temporary wrapper object.
@@ -263,6 +263,10 @@ public:
/* Allocate in the GC heap. */
void *operator new (size_t sz);
+ /* Some wrapper subclasses contain vec<> and so need to
+ release them when they are GC-ed. */
+ virtual void finalizer () { }
+
};
class type : public wrapper
@@ -297,7 +301,7 @@ public:
: type (inner)
{}
- void set_fields (const vec<field *> &fields);
+ void set_fields (const auto_vec<field *> *fields);
};
class field : public wrapper
@@ -319,6 +323,7 @@ public:
function(context *ctxt, tree fndecl, enum gcc_jit_function_kind kind);
void gt_ggc_mx ();
+ void finalizer ();
tree get_return_type_as_tree () const;
@@ -366,6 +371,8 @@ public:
block (function *func,
const char *name);
+ void finalizer ();
+
tree as_label_decl () const { return m_label_decl; }
void
@@ -500,6 +507,7 @@ class source_file : public wrapper
{
public:
source_file (tree filename);
+ void finalizer ();
source_line *
get_source_line (int line_num);
@@ -520,6 +528,7 @@ class source_line : public wrapper
{
public:
source_line (source_file *file, int line_num);
+ void finalizer ();
location *
get_location (recording::location *rloc, int column_num);
diff --git a/gcc/jit/jit-recording.c b/gcc/jit/jit-recording.c
index 8cce277..8069afc 100644
--- a/gcc/jit/jit-recording.c
+++ b/gcc/jit/jit-recording.c
@@ -1602,7 +1602,7 @@ void
recording::function_type::replay_into (replayer *r)
{
/* Convert m_param_types to a vec of playback type. */
- vec <playback::type *> param_types;
+ auto_vec <playback::type *> param_types;
int i;
recording::type *type;
param_types.create (m_param_types.length ());
@@ -1859,11 +1859,11 @@ recording::fields::fields (compound_type *struct_or_union,
void
recording::fields::replay_into (replayer *)
{
- vec<playback::field *> playback_fields;
+ auto_vec<playback::field *> playback_fields;
playback_fields.create (m_fields.length ());
for (unsigned i = 0; i < m_fields.length (); i++)
playback_fields.safe_push (m_fields[i]->playback_field ());
- m_struct_or_union->playback_compound_type ()->set_fields (playback_fields);
+ m_struct_or_union->playback_compound_type ()->set_fields (&playback_fields);
}
/* Override the default implementation of
@@ -2032,7 +2032,7 @@ void
recording::function::replay_into (replayer *r)
{
/* Convert m_params to a vec of playback param. */
- vec <playback::param *> params;
+ auto_vec <playback::param *> params;
int i;
recording::param *param;
params.create (m_params.length ());
@@ -2848,14 +2848,14 @@ recording::call::call (recording::context *ctxt,
void
recording::call::replay_into (replayer *r)
{
- vec<playback::rvalue *> playback_args;
+ auto_vec<playback::rvalue *> playback_args;
playback_args.create (m_args.length ());
for (unsigned i = 0; i< m_args.length (); i++)
playback_args.safe_push (m_args[i]->playback_rvalue ());
set_playback_obj (r->new_call (playback_location (r, m_loc),
m_func->playback_function (),
- playback_args));
+ &playback_args));
}
/* Implementation of recording::memento::make_debug_string for
@@ -2925,14 +2925,14 @@ recording::call_through_ptr::call_through_ptr (recording::context *ctxt,
void
recording::call_through_ptr::replay_into (replayer *r)
{
- vec<playback::rvalue *> playback_args;
+ auto_vec<playback::rvalue *> playback_args;
playback_args.create (m_args.length ());
for (unsigned i = 0; i< m_args.length (); i++)
playback_args.safe_push (m_args[i]->playback_rvalue ());
set_playback_obj (r->new_call_through_ptr (playback_location (r, m_loc),
m_fn_ptr->playback_rvalue (),
- playback_args));
+ &playback_args));
}
/* Implementation of recording::memento::make_debug_string for
diff --git a/gcc/jit/jit-recording.h b/gcc/jit/jit-recording.h
index bb1a2ee..4ea8ef1 100644
--- a/gcc/jit/jit-recording.h
+++ b/gcc/jit/jit-recording.h
@@ -242,11 +242,11 @@ private:
bool m_bool_options[GCC_JIT_NUM_BOOL_OPTIONS];
/* Recorded API usage. */
- vec<memento *> m_mementos;
+ auto_vec<memento *> m_mementos;
/* Specific recordings, for use by dump_to_file. */
- vec<compound_type *> m_compound_types;
- vec<function *> m_functions;
+ auto_vec<compound_type *> m_compound_types;
+ auto_vec<function *> m_functions;
type *m_basic_types[NUM_GCC_JIT_TYPES];
type *m_FILE_type;
@@ -622,7 +622,7 @@ public:
void replay_into (replayer *);
type * get_return_type () const { return m_return_type; }
- vec<type *> get_param_types () const { return m_param_types; }
+ const vec<type *> &get_param_types () const { return m_param_types; }
int is_variadic () const { return m_is_variadic; }
string * make_debug_string_with_ptr ();
@@ -633,7 +633,7 @@ public:
private:
type *m_return_type;
- vec<type *> m_param_types;
+ auto_vec<type *> m_param_types;
int m_is_variadic;
};
@@ -749,7 +749,7 @@ private:
private:
compound_type *m_struct_or_union;
- vec<field *> m_fields;
+ auto_vec<field *> m_fields;
};
class union_ : public compound_type
@@ -897,7 +897,7 @@ public:
type *get_return_type () const { return m_return_type; }
string * get_name () const { return m_name; }
- vec<param *> get_params () const { return m_params; }
+ const vec<param *> &get_params () const { return m_params; }
/* Get the given param by index.
Implements the post-error-checking part of
@@ -920,11 +920,11 @@ private:
enum gcc_jit_function_kind m_kind;
type *m_return_type;
string *m_name;
- vec<param *> m_params;
+ auto_vec<param *> m_params;
int m_is_variadic;
enum built_in_function m_builtin_id;
- vec<local *> m_locals;
- vec<block *> m_blocks;
+ auto_vec<local *> m_locals;
+ auto_vec<block *> m_blocks;
};
class block : public memento
@@ -1011,7 +1011,7 @@ private:
function *m_func;
int m_index;
string *m_name;
- vec<statement *> m_statements;
+ auto_vec<statement *> m_statements;
bool m_has_been_terminated;
bool m_is_reachable;
@@ -1222,7 +1222,7 @@ private:
private:
function *m_func;
- vec<rvalue *> m_args;
+ auto_vec<rvalue *> m_args;
};
class call_through_ptr : public rvalue
@@ -1241,7 +1241,7 @@ private:
private:
rvalue *m_fn_ptr;
- vec<rvalue *> m_args;
+ auto_vec<rvalue *> m_args;
};
class array_access : public lvalue
--
1.8.5.3
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH 17/21] PR jit/63854: Fix leaking vec in jit
2014-01-01 0:00 ` [PATCH 17/21] PR jit/63854: Fix leaking vec in jit David Malcolm
@ 2014-01-01 0:00 ` Jeff Law
0 siblings, 0 replies; 69+ messages in thread
From: Jeff Law @ 2014-01-01 0:00 UTC (permalink / raw)
To: David Malcolm, gcc-patches, jit
On 11/19/14 03:46, David Malcolm wrote:
> This fixes various leaks of vec buffers seen via valgrind within jit
> (both recording and playback).
>
> Various vec<> within jit::recording are converted to auto_vec<>.
>
> Various playback::wrapper subclasses containing vec<> gain a finalizer
> so they can release the vec when they are collected.
>
> gcc/jit/ChangeLog:
> PR jit/63854
> * jit-playback.c (gcc::jit::playback::compound_type::set_fields):
> Convert param from const vec<playback::field *> & to
> const auto_vec<playback::field *> *.
> (gcc::jit::playback::context::new_function_type): Convert param
> "param_types" from vec<type *> * to const auto_vec<type *> *.
> (gcc::jit::playback::context::new_function): Convert param
> "params" from vec<param *> * to const auto_vec<param *> *.
> (gcc::jit::playback::context::build_call): Convert param "args"
> from vec<rvalue *> to const auto_vec<rvalue *> *.
> (gcc::jit::playback::context::new_call): Likewise.
> (gcc::jit::playback::context::new_call_through_ptr): Likewise.
> (wrapper_finalizer): New function.
> (gcc::jit::playback::wrapper::operator new): Call the finalizer
> variant of ggc_internal_cleared_alloc, supplying
> wrapper_finalizer.
> (gcc::jit::playback::function::finalizer): New.
> (gcc::jit::playback::block::finalizer): New.
> (gcc::jit::playback::source_file::finalizer): New.
> (gcc::jit::playback::source_line::finalizer): New.
>
> * jit-playback.h
> (gcc::jit::playback::context::new_function_type): Convert param
> "param_types" from vec<type *> * to const auto_vec<type *> *.
> (gcc::jit::playback::context::new_function): Convert param
> "params" from vec<param *> * to const auto_vec<param *> *.
> (gcc::jit::playback::context::new_call): Convert param
> "args" from vec<rvalue *> to const auto_vec<rvalue *> *.
> (gcc::jit::playback::context::new_call_through_ptr): Likewise.
> (gcc::jit::playback::context::build_call): Likewise.
> (gcc::jit::playback::context): Convert fields "m_functions",
> "m_source_files", "m_cached_locations" from vec to auto_vec.
> (gcc::jit::playback::wrapper::finalizer): New virtual function.
> (gcc::jit::playback::compound_type::set_fields): Convert param fro
> const vec<playback::field *> & to
> const auto_vec<playback::field *> *.
> (gcc::jit::playback::function::finalizer): New.
> (gcc::jit::playback::block::finalizer): New.
> (gcc::jit::playback::source_file::finalizer): New.
> (gcc::jit::playback::source_line::finalizer): New.
>
> * jit-recording.c
> (gcc::jit::recording::function_type::replay_into): Convert local
> from a vec into an auto_vec.
> (gcc::jit::recording::fields::replay_into): Likewise.
> (gcc::jit::recording::function::replay_into): Likewise.
> (gcc::jit::recording::call::replay_into): Likewise.
> (gcc::jit::recording::call_through_ptr::replay_into): Likewise.
>
> * jit-recording.h (gcc::jit::recording::context): Convert fields
> "m_mementos", "m_compound_types", "m_functions" from vec<> to
> auto_vec <>.
> (gcc::jit::recording::function_type::get_param_types): Convert
> return type from vec<type *> to const vec<type *> &.
> (gcc::jit::recording::function_type): Convert field
> "m_param_types" from a vec<> to an auto_vec<>.
> (gcc::jit::recording::fields): Likewise for field "m_fields".
> (gcc::jit::recording::function::get_params): Convert return type
> from vec <param *> to const vec<param *> &.
> (gcc::jit::recording::function): Convert fields "m_params",
> "m_locals", "m_blocks" from vec<> to auto_vec<>.
> (gcc::jit::recording::block): Likewise for field "m_statements".
> vec<> to auto_vec<>.
> (gcc::jit::recording::call): Likewise for field "m_args".
> (gcc::jit::recording::call_through_ptr): Likewise.
OK.
Jeff
^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH 16/21] PR jit/63854: Add all_late_ipa_passes to GCC_PASS_LISTS
2014-01-01 0:00 [PATCH 00/21] PR 63854: Fix various memory leaks David Malcolm
` (4 preceding siblings ...)
2014-01-01 0:00 ` [PATCH 17/21] PR jit/63854: Fix leaking vec in jit David Malcolm
@ 2014-01-01 0:00 ` David Malcolm
2014-01-01 0:00 ` Richard Biener
2014-01-01 0:00 ` [PATCH 10/21] PR jit/63854: Fix leak of worklist within jit-recording.c David Malcolm
` (14 subsequent siblings)
20 siblings, 1 reply; 69+ messages in thread
From: David Malcolm @ 2014-01-01 0:00 UTC (permalink / raw)
To: gcc-patches, jit; +Cc: David Malcolm
Valgrind showed a per-iteration leak of pass_ipa_pta (and presumably
pass_omp_simd_clone):
704 (352 direct, 352 indirect) bytes in 4 blocks are definitely lost in loss record 198 of 241
at 0x4A06965: operator new(unsigned long) (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
by 0x555A55D: make_pass_ipa_pta(gcc::context*) (tree-ssa-structalias.c:7425)
by 0x5219FDA: gcc::pass_manager::pass_manager(gcc::context*) (pass-instances.def:141)
by 0x4E4F5D1: gcc::context::context() (context.c:37)
by 0x532C07B: general_init(char const*) (toplev.c:1212)
by 0x532DB5E: toplev::main(int, char**) (toplev.c:2074)
by 0x4DE76AF: gcc::jit::playback::context::compile() (jit-playback.c:1615)
by 0x4DD76DA: gcc::jit::recording::context::compile() (jit-recording.c:861)
by 0x4DD5BD2: gcc_jit_context_compile (libgccjit.c:2014)
by 0x401DC4: test_jit (harness.h:190)
by 0x401EA8: main (harness.h:232)
Investigation showed that ~pass_manager wasn't deleting these
passes since for some reason GCC_PASS_LISTS didn't contain
all_late_ipa_passes and so wasn't calling delete_pass_tree on it.
Add it to GCC_PASS_LISTS, fixing the leak.
gcc/ChangeLog:
PR jit/63854
* pass_manager.h (GCC_PASS_LISTS): Add all_late_ipa_passes.
---
gcc/pass_manager.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/gcc/pass_manager.h b/gcc/pass_manager.h
index 82857a9..e6658ad 100644
--- a/gcc/pass_manager.h
+++ b/gcc/pass_manager.h
@@ -29,6 +29,7 @@ struct register_pass_info;
DEF_PASS_LIST (all_lowering_passes) \
DEF_PASS_LIST (all_small_ipa_passes) \
DEF_PASS_LIST (all_regular_ipa_passes) \
+ DEF_PASS_LIST (all_late_ipa_passes) \
DEF_PASS_LIST (all_passes)
#define DEF_PASS_LIST(LIST) PASS_LIST_NO_##LIST,
--
1.8.5.3
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH 16/21] PR jit/63854: Add all_late_ipa_passes to GCC_PASS_LISTS
2014-01-01 0:00 ` [PATCH 16/21] PR jit/63854: Add all_late_ipa_passes to GCC_PASS_LISTS David Malcolm
@ 2014-01-01 0:00 ` Richard Biener
0 siblings, 0 replies; 69+ messages in thread
From: Richard Biener @ 2014-01-01 0:00 UTC (permalink / raw)
To: David Malcolm; +Cc: GCC Patches, jit
On Wed, Nov 19, 2014 at 11:46 AM, David Malcolm <dmalcolm@redhat.com> wrote:
> Valgrind showed a per-iteration leak of pass_ipa_pta (and presumably
> pass_omp_simd_clone):
Ok.
Thanks,
Richard.
> 704 (352 direct, 352 indirect) bytes in 4 blocks are definitely lost in loss record 198 of 241
> at 0x4A06965: operator new(unsigned long) (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
> by 0x555A55D: make_pass_ipa_pta(gcc::context*) (tree-ssa-structalias.c:7425)
> by 0x5219FDA: gcc::pass_manager::pass_manager(gcc::context*) (pass-instances.def:141)
> by 0x4E4F5D1: gcc::context::context() (context.c:37)
> by 0x532C07B: general_init(char const*) (toplev.c:1212)
> by 0x532DB5E: toplev::main(int, char**) (toplev.c:2074)
> by 0x4DE76AF: gcc::jit::playback::context::compile() (jit-playback.c:1615)
> by 0x4DD76DA: gcc::jit::recording::context::compile() (jit-recording.c:861)
> by 0x4DD5BD2: gcc_jit_context_compile (libgccjit.c:2014)
> by 0x401DC4: test_jit (harness.h:190)
> by 0x401EA8: main (harness.h:232)
>
> Investigation showed that ~pass_manager wasn't deleting these
> passes since for some reason GCC_PASS_LISTS didn't contain
> all_late_ipa_passes and so wasn't calling delete_pass_tree on it.
>
> Add it to GCC_PASS_LISTS, fixing the leak.
>
> gcc/ChangeLog:
> PR jit/63854
> * pass_manager.h (GCC_PASS_LISTS): Add all_late_ipa_passes.
> ---
> gcc/pass_manager.h | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/gcc/pass_manager.h b/gcc/pass_manager.h
> index 82857a9..e6658ad 100644
> --- a/gcc/pass_manager.h
> +++ b/gcc/pass_manager.h
> @@ -29,6 +29,7 @@ struct register_pass_info;
> DEF_PASS_LIST (all_lowering_passes) \
> DEF_PASS_LIST (all_small_ipa_passes) \
> DEF_PASS_LIST (all_regular_ipa_passes) \
> + DEF_PASS_LIST (all_late_ipa_passes) \
> DEF_PASS_LIST (all_passes)
>
> #define DEF_PASS_LIST(LIST) PASS_LIST_NO_##LIST,
> --
> 1.8.5.3
>
^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH 10/21] PR jit/63854: Fix leak of worklist within jit-recording.c
2014-01-01 0:00 [PATCH 00/21] PR 63854: Fix various memory leaks David Malcolm
` (5 preceding siblings ...)
2014-01-01 0:00 ` [PATCH 16/21] PR jit/63854: Add all_late_ipa_passes to GCC_PASS_LISTS David Malcolm
@ 2014-01-01 0:00 ` David Malcolm
2014-01-01 0:00 ` Jeff Law
2014-01-01 0:00 ` [PATCH 18/21] PR jit/63854: Add "long-term" allocator to gcc::context David Malcolm
` (13 subsequent siblings)
20 siblings, 1 reply; 69+ messages in thread
From: David Malcolm @ 2014-01-01 0:00 UTC (permalink / raw)
To: gcc-patches, jit; +Cc: David Malcolm
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(-)
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)
{
--
1.8.5.3
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH 10/21] PR jit/63854: Fix leak of worklist within jit-recording.c
2014-01-01 0:00 ` [PATCH 10/21] PR jit/63854: Fix leak of worklist within jit-recording.c David Malcolm
@ 2014-01-01 0:00 ` Jeff Law
2014-01-01 0:00 ` David Malcolm
0 siblings, 1 reply; 69+ messages in thread
From: Jeff Law @ 2014-01-01 0:00 UTC (permalink / raw)
To: David Malcolm, gcc-patches, jit
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
^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH 10/21] PR jit/63854: Fix leak of worklist within jit-recording.c
2014-01-01 0:00 ` Jeff Law
@ 2014-01-01 0:00 ` David Malcolm
2014-01-01 0:00 ` Richard Biener
0 siblings, 1 reply; 69+ messages in thread
From: David Malcolm @ 2014-01-01 0:00 UTC (permalink / raw)
To: Jeff Law; +Cc: gcc-patches, jit
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.
^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH 10/21] PR jit/63854: Fix leak of worklist within jit-recording.c
2014-01-01 0:00 ` David Malcolm
@ 2014-01-01 0:00 ` Richard Biener
2014-01-01 0:00 ` Jeff Law
0 siblings, 1 reply; 69+ messages in thread
From: Richard Biener @ 2014-01-01 0:00 UTC (permalink / raw)
To: David Malcolm; +Cc: Jeff Law, GCC Patches, jit
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.
>
^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH 10/21] PR jit/63854: Fix leak of worklist within jit-recording.c
2014-01-01 0:00 ` Richard Biener
@ 2014-01-01 0:00 ` Jeff Law
0 siblings, 0 replies; 69+ messages in thread
From: Jeff Law @ 2014-01-01 0:00 UTC (permalink / raw)
To: Richard Biener, David Malcolm; +Cc: GCC Patches, jit
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
^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH 18/21] PR jit/63854: Add "long-term" allocator to gcc::context
2014-01-01 0:00 [PATCH 00/21] PR 63854: Fix various memory leaks David Malcolm
` (6 preceding siblings ...)
2014-01-01 0:00 ` [PATCH 10/21] PR jit/63854: Fix leak of worklist within jit-recording.c David Malcolm
@ 2014-01-01 0:00 ` David Malcolm
2014-01-01 0:00 ` Joseph Myers
2014-01-01 0:00 ` [PATCH 01/21] PR jit/63854: Fix memory leak within gcc_options David Malcolm
` (12 subsequent siblings)
20 siblings, 1 reply; 69+ messages in thread
From: David Malcolm @ 2014-01-01 0:00 UTC (permalink / raw)
To: gcc-patches, jit; +Cc: David Malcolm
Some places in the startup code use char * values that can sometimes be
string literals, and can sometimes be dynamically built using xstrdup or
concat.
This isn't a problem for cc1 etc since this is only called once, but
for libgccjit they are small per-invocation leaks.
There's no clean way to release them: retrofitting logic to decide if
we're dealing with a string literal vs a dynamically-allocated buffer
(and if something else is pointing to said buffer) is messy and
error-prone; they are also unconnected to the GC.
Hence the cleanest way to fix the leak is to add a new "long-term"
allocator, to gcc::context. This gives back buffers that will persist
until the gcc::context is cleaned away.
Using this fixes these leaks seen via valgrind:
28 bytes in 4 blocks are definitely lost in loss record 13 of 209
at 0x4A0645D: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
by 0x5D76417: xmalloc (xmalloc.c:147)
by 0x5D76509: xstrdup (xstrdup.c:34)
by 0x532CC38: process_options() (toplev.c:1316)
by 0x532DFF4: do_compile() (toplev.c:1976)
by 0x532E3B0: toplev::main(int, char**) (toplev.c:2117)
by 0x4DE7983: gcc::jit::playback::context::compile() (jit-playback.c:1646)
by 0x4DD791A: gcc::jit::recording::context::compile() (jit-recording.c:861)
by 0x4DD5E12: gcc_jit_context_compile (libgccjit.c:2014)
by 0x401CA4: test_jit (harness.h:190)
by 0x401D88: main (harness.h:232)
108 bytes in 12 blocks are definitely lost in loss record 135 of 241
at 0x4A0645D: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
by 0x5D75E07: xmalloc (xmalloc.c:147)
by 0x5D7098B: concat (concat.c:147)
by 0x563A342: build_common_builtin_nodes() (tree.c:10145)
by 0x4DC91FC: jit_langhook_init() (dummy-frontend.c:128)
by 0x532D3F4: lang_dependent_init(char const*) (toplev.c:1790)
by 0x532D9C8: do_compile() (toplev.c:2007)
by 0x532DCAC: toplev::main(int, char**) (toplev.c:2118)
by 0x4DE76AF: gcc::jit::playback::context::compile() (jit-playback.c:1615)
by 0x4DD76DA: gcc::jit::recording::context::compile() (jit-recording.c:861)
by 0x4DD5BD2: gcc_jit_context_compile (libgccjit.c:2014)
by 0x401DC4: test_jit (harness.h:190)
108 bytes in 12 blocks are definitely lost in loss record 136 of 241
at 0x4A0645D: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
by 0x5D75E07: xmalloc (xmalloc.c:147)
by 0x5D7098B: concat (concat.c:147)
by 0x563A3B6: build_common_builtin_nodes() (tree.c:10151)
by 0x4DC91FC: jit_langhook_init() (dummy-frontend.c:128)
by 0x532D3F4: lang_dependent_init(char const*) (toplev.c:1790)
by 0x532D9C8: do_compile() (toplev.c:2007)
by 0x532DCAC: toplev::main(int, char**) (toplev.c:2118)
by 0x4DE76AF: gcc::jit::playback::context::compile() (jit-playback.c:1615)
by 0x4DD76DA: gcc::jit::recording::context::compile() (jit-recording.c:861)
by 0x4DD5BD2: gcc_jit_context_compile (libgccjit.c:2014)
by 0x401DC4: test_jit (harness.h:190)
136 bytes in 4 blocks are definitely lost in loss record 129 of 209
at 0x4A0645D: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
by 0x5D76417: xmalloc (xmalloc.c:147)
by 0x532BE28: init_asm_output(char const*) (toplev.c:936)
by 0x532DB34: lang_dependent_init(char const*) (toplev.c:1795)
by 0x532E0CC: do_compile() (toplev.c:2006)
by 0x532E3B0: toplev::main(int, char**) (toplev.c:2117)
by 0x4DE7983: gcc::jit::playback::context::compile() (jit-playback.c:1646)
by 0x4DD791A: gcc::jit::recording::context::compile() (jit-recording.c:861)
by 0x4DD5E12: gcc_jit_context_compile (libgccjit.c:2014)
by 0x401CA4: test_jit (harness.h:190)
by 0x401D88: main (harness.h:232)
gcc/ChangeLog:
PR jit/63854
* context.c (gcc::context::context): Initialize m_longterm_obstack.
(gcc::context::~context): Free m_longterm_obstack.
(gcc::context::longterm_xalloc): New method.
(gcc::context::longterm_xstrdup): New method.
* context.h: Include obstack.h
(gcc::context::longterm_xalloc): New method.
(gcc::context::longterm_xallocvec<T>): New method
(gcc::context::longterm_xstrdup): New method.
(gcc::context::m_longterm_obstack): New field.
* toplev.c (init_asm_output): Use g->longterm_xallocvec <char>
rather than XNEWVEC when allocating "dumpname" to avoid a leak.
(process_options): Likewise, use g->longterm_xstrdup rather than
xstrdup when allocating "name" (and hence aux_base_name).
* tree.c: Include context.h.
(build_common_builtin_nodes): When writing dynamically-allocated
entries into built_in_names, use g->longterm_xstrdup to take a
copy of the buffer acquired by concat to avoid a leak.
---
gcc/context.c | 18 ++++++++++++++++++
gcc/context.h | 46 ++++++++++++++++++++++++++++++++++++++++++++++
gcc/toplev.c | 4 ++--
gcc/tree.c | 17 +++++++++++------
4 files changed, 77 insertions(+), 8 deletions(-)
diff --git a/gcc/context.c b/gcc/context.c
index d132946..ead9f33 100644
--- a/gcc/context.c
+++ b/gcc/context.c
@@ -37,10 +37,28 @@ gcc::context::context ()
before the pass manager. */
m_dumps = new gcc::dump_manager ();
m_passes = new gcc::pass_manager (this);
+ obstack_init (&m_longterm_obstack);
}
gcc::context::~context ()
{
delete m_passes;
delete m_dumps;
+ obstack_free (&m_longterm_obstack, NULL);
+}
+
+void *
+gcc::context::longterm_xalloc (size_t sz)
+{
+ return obstack_alloc (&m_longterm_obstack, sz);
+}
+
+char *
+gcc::context::longterm_xstrdup (const char *str)
+{
+ gcc_assert (str);
+ size_t sz = strlen (str) + 1;
+ char *buf = reinterpret_cast <char *> (longterm_xalloc (sz));
+ memcpy (buf, str, sz);
+ return buf;
}
diff --git a/gcc/context.h b/gcc/context.h
index 2bf28a7..068f973 100644
--- a/gcc/context.h
+++ b/gcc/context.h
@@ -20,6 +20,8 @@ along with GCC; see the file COPYING3. If not see
#ifndef GCC_CONTEXT_H
#define GCC_CONTEXT_H
+#include <obstack.h>
+
namespace gcc {
class pass_manager;
@@ -45,6 +47,40 @@ public:
dump_manager *get_dumps () {gcc_assert (m_dumps); return m_dumps; }
+ /* Long-term allocation.
+
+ GCC's startup code sometimes uses a mix of static and dynamic memory
+ allocation, with no cleanup of the latter. Such allocations were
+ written under the assumption that the buffers can persist for the
+ rest of the compile and be effectively freed when the process
+ terminates.
+
+ This works for cc1 etc since these allocations are only called once,
+ but for libgccjit they are small per-invocation leaks.
+
+ The following functions provide a simple way to retrofit cleanup
+ into such places that weren't expecting the compiler to be called
+ more than once in-process.
+
+ They allocate buffers that will persist until the context is
+ deleted, when they are all cleaned up at once. */
+
+ /* Allocate a buffer, to persist until the context is deleted, without
+ needing explicit cleanup. */
+ void *longterm_xalloc (size_t) ATTRIBUTE_RETURNS_NONNULL;
+
+ /* Allocate an array of T, to persist until the context is deleted,
+ without needing explicit cleanup. */
+ template <typename T>
+ T *longterm_xallocvec (size_t len) ATTRIBUTE_RETURNS_NONNULL;
+
+ /* Copy a string into a memory buffer without fail.
+
+ As per libiberty's xstrdup, but the buffer is longterm-allocated so
+ that it persists until the context is deleted, without needing
+ explicit cleanup. */
+ char *longterm_xstrdup (const char *) ATTRIBUTE_RETURNS_NONNULL;
+
private:
/* Pass-management. */
pass_manager *m_passes;
@@ -52,10 +88,20 @@ private:
/* Dump files. */
dump_manager *m_dumps;
+ /* Long-term allocation is implemented using an obstack. */
+ struct obstack m_longterm_obstack;
+
}; // class context
} // namespace gcc
+template <typename T>
+inline T *
+gcc::context::longterm_xallocvec (size_t len)
+{
+ return reinterpret_cast <T *> (longterm_xalloc (sizeof (T) * len));
+}
+
/* The global singleton context aka "g".
(the name is chosen to be easy to type in a debugger). */
extern gcc::context *g;
diff --git a/gcc/toplev.c b/gcc/toplev.c
index 0181a3f..c0c418c 100644
--- a/gcc/toplev.c
+++ b/gcc/toplev.c
@@ -936,7 +936,7 @@ init_asm_output (const char *name)
if (asm_file_name == 0)
{
int len = strlen (dump_base_name);
- char *dumpname = XNEWVEC (char, len + 6);
+ char *dumpname = g->longterm_xallocvec <char> (len + 6);
memcpy (dumpname, dump_base_name, len + 1);
strip_off_ending (dumpname, len);
@@ -1332,7 +1332,7 @@ process_options (void)
;
else if (main_input_filename)
{
- char *name = xstrdup (lbasename (main_input_filename));
+ char *name = g->longterm_xstrdup (lbasename (main_input_filename));
strip_off_ending (name, strlen (name));
aux_base_name = name;
diff --git a/gcc/tree.c b/gcc/tree.c
index 3d1d637..ac358bc 100644
--- a/gcc/tree.c
+++ b/gcc/tree.c
@@ -88,6 +88,7 @@ along with GCC; see the file COPYING3. If not see
#include "intl.h"
#include "wide-int.h"
#include "builtins.h"
+#include "context.h"
/* Tree code classes. */
@@ -10126,6 +10127,7 @@ build_common_builtin_nodes (void)
enum built_in_function mcode, dcode;
tree type, inner_type;
const char *prefix = "__";
+ char *name;
if (targetm.libfunc_gnu_prefix)
prefix = "__gnu_";
@@ -10147,15 +10149,18 @@ build_common_builtin_nodes (void)
*q = TOLOWER (*p);
*q = '\0';
- built_in_names[mcode] = concat (prefix, "mul", mode_name_buf, "3",
- NULL);
- local_define_builtin (built_in_names[mcode], ftype, mcode,
+ name = concat (prefix, "mul", mode_name_buf, "3", NULL);
+ built_in_names[mcode] = g->longterm_xstrdup (name);
+ free (name);
+ local_define_builtin (built_in_names[mcode], ftype, mcode,
built_in_names[mcode],
ECF_CONST | ECF_NOTHROW | ECF_LEAF);
- built_in_names[dcode] = concat (prefix, "div", mode_name_buf, "3",
- NULL);
- local_define_builtin (built_in_names[dcode], ftype, dcode,
+ name = concat (prefix, "div", mode_name_buf, "3", NULL);
+ built_in_names[dcode] = g->longterm_xstrdup (name);
+ free (name);
+
+ local_define_builtin (built_in_names[dcode], ftype, dcode,
built_in_names[dcode],
ECF_CONST | ECF_NOTHROW | ECF_LEAF);
}
--
1.8.5.3
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH 18/21] PR jit/63854: Add "long-term" allocator to gcc::context
2014-01-01 0:00 ` [PATCH 18/21] PR jit/63854: Add "long-term" allocator to gcc::context David Malcolm
@ 2014-01-01 0:00 ` Joseph Myers
2015-01-01 0:00 ` PING: " David Malcolm
0 siblings, 1 reply; 69+ messages in thread
From: Joseph Myers @ 2014-01-01 0:00 UTC (permalink / raw)
To: David Malcolm; +Cc: gcc-patches, jit
On Wed, 19 Nov 2014, David Malcolm wrote:
> There's no clean way to release them: retrofitting logic to decide if
> we're dealing with a string literal vs a dynamically-allocated buffer
> (and if something else is pointing to said buffer) is messy and
> error-prone; they are also unconnected to the GC.
This is not an objection to your patch, but the obvious approach would
seem to me to be to ensure that anything that might be allocated or might
be a string constant (or maybe a string from argv in some cases?) is
always allocated - use xstrdup rather than putting a string constant there
directly. Once such pointers are always allocated, they can safely be
freed.
--
Joseph S. Myers
joseph@codesourcery.com
^ permalink raw reply [flat|nested] 69+ messages in thread
* PING: Re: [PATCH 18/21] PR jit/63854: Add "long-term" allocator to gcc::context
2014-01-01 0:00 ` Joseph Myers
@ 2015-01-01 0:00 ` David Malcolm
2015-01-01 0:00 ` Joseph Myers
0 siblings, 1 reply; 69+ messages in thread
From: David Malcolm @ 2015-01-01 0:00 UTC (permalink / raw)
To: Joseph Myers; +Cc: gcc-patches, jit
On Wed, 2014-11-19 at 17:36 +0000, Joseph Myers wrote:
> On Wed, 19 Nov 2014, David Malcolm wrote:
>
> > There's no clean way to release them: retrofitting logic to decide if
> > we're dealing with a string literal vs a dynamically-allocated buffer
> > (and if something else is pointing to said buffer) is messy and
> > error-prone; they are also unconnected to the GC.
>
> This is not an objection to your patch, but the obvious approach would
> seem to me to be to ensure that anything that might be allocated or might
> be a string constant (or maybe a string from argv in some cases?) is
> always allocated - use xstrdup rather than putting a string constant there
> directly. Once such pointers are always allocated, they can safely be
> freed.
Maybe, though this approach avoids having to retrofit xstrdup calls into
all these places in favor of allowing you to freely mingle string
constants with allocated memory, and it's only relevant for the jit - so
I was assuming that my approach would be preferable. Though I guess
it's an extra cognitive load on maintainers: a new kind of allocation
Is the patch acceptable in stage3 (assuming it still
applies/bootstraps/etc)?
https://gcc.gnu.org/ml/gcc-patches/2014-11/msg02420.html
It fixes some leaks in jit (that don't affect cc1 etc al) and makes
valgrind of "make check-jit" closer to being clean.
^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH 01/21] PR jit/63854: Fix memory leak within gcc_options
2014-01-01 0:00 [PATCH 00/21] PR 63854: Fix various memory leaks David Malcolm
` (7 preceding siblings ...)
2014-01-01 0:00 ` [PATCH 18/21] PR jit/63854: Add "long-term" allocator to gcc::context David Malcolm
@ 2014-01-01 0:00 ` David Malcolm
2014-01-01 0:00 ` Richard Biener
2014-01-01 0:00 ` [PATCH 12/21] PR jit/63854: Add a valgrind suppresion file David Malcolm
` (11 subsequent siblings)
20 siblings, 1 reply; 69+ messages in thread
From: David Malcolm @ 2014-01-01 0:00 UTC (permalink / raw)
To: gcc-patches, jit; +Cc: David Malcolm
Valgrind shows this fixes ~1 KB of leak per iteration (on x86_64) by
plugging these leaks allocated at opts.c lines 286 and 289:
==57820== 2,752 bytes in 4 blocks are definitely lost in loss record 875 of 917
==57820== at 0x4A0645D: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==57820== by 0x59A6747: xmalloc (xmalloc.c:147)
==57820== by 0x595542A: init_options_struct(gcc_options*, gcc_options*) (opts.c:286)
==57820== by 0x4E2ED61: toplev::main(int, char**) (toplev.c:2081)
==57820== by 0x4E43186: gcc::jit::playback::context::compile() (jit-playback.c:1615)
==57820== by 0x4E4018D: gcc::jit::recording::context::compile() (jit-recording.c:861)
==57820== by 0x401CA4: test_jit (harness.h:190)
==57820== by 0x401D88: main (harness.h:232)
==57820==
==57820== 2,752 bytes in 4 blocks are definitely lost in loss record 876 of 917
==57820== at 0x4A081D4: calloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==57820== by 0x59A6780: xcalloc (xmalloc.c:162)
==57820== by 0x595543E: init_options_struct(gcc_options*, gcc_options*) (opts.c:289)
==57820== by 0x4E2ED61: toplev::main(int, char**) (toplev.c:2081)
==57820== by 0x4E43186: gcc::jit::playback::context::compile() (jit-playback.c:1615)
==57820== by 0x4E4018D: gcc::jit::recording::context::compile() (jit-recording.c:861)
==57820== by 0x401CA4: test_jit (harness.h:190)
==57820== by 0x401D88: main (harness.h:232)
gcc/ChangeLog:
PR jit/63854
* opts.c (finalize_options_struct): New.
* opts.h (finalize_options_struct): New.
* toplev.c (toplev::finalize): Call finalize_options_struct
on global_options and global_options_set.
---
gcc/opts.c | 8 ++++++++
gcc/opts.h | 1 +
gcc/toplev.c | 3 +++
3 files changed, 12 insertions(+)
diff --git a/gcc/opts.c b/gcc/opts.c
index d22882b..dabd3c6 100644
--- a/gcc/opts.c
+++ b/gcc/opts.c
@@ -307,6 +307,14 @@ init_options_struct (struct gcc_options *opts, struct gcc_options *opts_set)
targetm_common.option_init_struct (opts);
}
+/* Release any allocations owned by OPTS. */
+
+void
+finalize_options_struct (struct gcc_options *opts)
+{
+ XDELETEVEC (opts->x_param_values);
+}
+
/* If indicated by the optimization level LEVEL (-Os if SIZE is set,
-Ofast if FAST is set, -Og if DEBUG is set), apply the option DEFAULT_OPT
to OPTS and OPTS_SET, diagnostic context DC, location LOC, with language
diff --git a/gcc/opts.h b/gcc/opts.h
index f694082..c3ec942 100644
--- a/gcc/opts.h
+++ b/gcc/opts.h
@@ -325,6 +325,7 @@ extern void decode_cmdline_options_to_array (unsigned int argc,
extern void init_options_once (void);
extern void init_options_struct (struct gcc_options *opts,
struct gcc_options *opts_set);
+extern void finalize_options_struct (struct gcc_options *opts);
extern void decode_cmdline_options_to_array_default_mask (unsigned int argc,
const char **argv,
struct cl_decoded_option **decoded_options,
diff --git a/gcc/toplev.c b/gcc/toplev.c
index 2e48047..4b4e568 100644
--- a/gcc/toplev.c
+++ b/gcc/toplev.c
@@ -2169,4 +2169,7 @@ toplev::finalize (void)
ipa_cp_c_finalize ();
ipa_reference_c_finalize ();
params_c_finalize ();
+
+ finalize_options_struct (&global_options);
+ finalize_options_struct (&global_options_set);
}
--
1.8.5.3
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH 01/21] PR jit/63854: Fix memory leak within gcc_options
2014-01-01 0:00 ` [PATCH 01/21] PR jit/63854: Fix memory leak within gcc_options David Malcolm
@ 2014-01-01 0:00 ` Richard Biener
0 siblings, 0 replies; 69+ messages in thread
From: Richard Biener @ 2014-01-01 0:00 UTC (permalink / raw)
To: David Malcolm; +Cc: GCC Patches, jit
On Wed, Nov 19, 2014 at 11:46 AM, David Malcolm <dmalcolm@redhat.com> wrote:
> Valgrind shows this fixes ~1 KB of leak per iteration (on x86_64) by
> plugging these leaks allocated at opts.c lines 286 and 289:
>
> ==57820== 2,752 bytes in 4 blocks are definitely lost in loss record 875 of 917
> ==57820== at 0x4A0645D: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==57820== by 0x59A6747: xmalloc (xmalloc.c:147)
> ==57820== by 0x595542A: init_options_struct(gcc_options*, gcc_options*) (opts.c:286)
> ==57820== by 0x4E2ED61: toplev::main(int, char**) (toplev.c:2081)
> ==57820== by 0x4E43186: gcc::jit::playback::context::compile() (jit-playback.c:1615)
> ==57820== by 0x4E4018D: gcc::jit::recording::context::compile() (jit-recording.c:861)
> ==57820== by 0x401CA4: test_jit (harness.h:190)
> ==57820== by 0x401D88: main (harness.h:232)
> ==57820==
> ==57820== 2,752 bytes in 4 blocks are definitely lost in loss record 876 of 917
> ==57820== at 0x4A081D4: calloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==57820== by 0x59A6780: xcalloc (xmalloc.c:162)
> ==57820== by 0x595543E: init_options_struct(gcc_options*, gcc_options*) (opts.c:289)
> ==57820== by 0x4E2ED61: toplev::main(int, char**) (toplev.c:2081)
> ==57820== by 0x4E43186: gcc::jit::playback::context::compile() (jit-playback.c:1615)
> ==57820== by 0x4E4018D: gcc::jit::recording::context::compile() (jit-recording.c:861)
> ==57820== by 0x401CA4: test_jit (harness.h:190)
> ==57820== by 0x401D88: main (harness.h:232)
Ok.
Thanks,
Richard.
> gcc/ChangeLog:
> PR jit/63854
> * opts.c (finalize_options_struct): New.
> * opts.h (finalize_options_struct): New.
> * toplev.c (toplev::finalize): Call finalize_options_struct
> on global_options and global_options_set.
> ---
> gcc/opts.c | 8 ++++++++
> gcc/opts.h | 1 +
> gcc/toplev.c | 3 +++
> 3 files changed, 12 insertions(+)
>
> diff --git a/gcc/opts.c b/gcc/opts.c
> index d22882b..dabd3c6 100644
> --- a/gcc/opts.c
> +++ b/gcc/opts.c
> @@ -307,6 +307,14 @@ init_options_struct (struct gcc_options *opts, struct gcc_options *opts_set)
> targetm_common.option_init_struct (opts);
> }
>
> +/* Release any allocations owned by OPTS. */
> +
> +void
> +finalize_options_struct (struct gcc_options *opts)
> +{
> + XDELETEVEC (opts->x_param_values);
> +}
> +
> /* If indicated by the optimization level LEVEL (-Os if SIZE is set,
> -Ofast if FAST is set, -Og if DEBUG is set), apply the option DEFAULT_OPT
> to OPTS and OPTS_SET, diagnostic context DC, location LOC, with language
> diff --git a/gcc/opts.h b/gcc/opts.h
> index f694082..c3ec942 100644
> --- a/gcc/opts.h
> +++ b/gcc/opts.h
> @@ -325,6 +325,7 @@ extern void decode_cmdline_options_to_array (unsigned int argc,
> extern void init_options_once (void);
> extern void init_options_struct (struct gcc_options *opts,
> struct gcc_options *opts_set);
> +extern void finalize_options_struct (struct gcc_options *opts);
> extern void decode_cmdline_options_to_array_default_mask (unsigned int argc,
> const char **argv,
> struct cl_decoded_option **decoded_options,
> diff --git a/gcc/toplev.c b/gcc/toplev.c
> index 2e48047..4b4e568 100644
> --- a/gcc/toplev.c
> +++ b/gcc/toplev.c
> @@ -2169,4 +2169,7 @@ toplev::finalize (void)
> ipa_cp_c_finalize ();
> ipa_reference_c_finalize ();
> params_c_finalize ();
> +
> + finalize_options_struct (&global_options);
> + finalize_options_struct (&global_options_set);
> }
> --
> 1.8.5.3
>
^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH 12/21] PR jit/63854: Add a valgrind suppresion file
2014-01-01 0:00 [PATCH 00/21] PR 63854: Fix various memory leaks David Malcolm
` (8 preceding siblings ...)
2014-01-01 0:00 ` [PATCH 01/21] PR jit/63854: Fix memory leak within gcc_options David Malcolm
@ 2014-01-01 0:00 ` David Malcolm
2014-01-01 0:00 ` Richard Biener
2014-01-01 0:00 ` [PATCH 20/21] PR jit/63854: Fix leak in ipa-icf.c David Malcolm
` (10 subsequent siblings)
20 siblings, 1 reply; 69+ messages in thread
From: David Malcolm @ 2014-01-01 0:00 UTC (permalink / raw)
To: gcc-patches, jit; +Cc: David Malcolm
Valgrind complains about uninitialized data within sparseset_bit_p.
Provide a suppression file to silence these warnings.
Valgrind requires suppression files for C++ code to use the mangled
names, so we do that here.
contrib/ChangeLog:
PR jit/63854
* valgrind.supp: New.
---
contrib/valgrind.supp | 11 +++++++++++
1 file changed, 11 insertions(+)
create mode 100644 contrib/valgrind.supp
diff --git a/contrib/valgrind.supp b/contrib/valgrind.supp
new file mode 100644
index 0000000..ec9ff02
--- /dev/null
+++ b/contrib/valgrind.supp
@@ -0,0 +1,11 @@
+{
+ suppress-uninit-cond-with-sparseset_bit_p
+ Memcheck:Cond
+ fun:_ZL15sparseset_bit_pP13sparseset_defm
+}
+
+{
+ suppress-uninit-use-with-sparseset_bit_p
+ Memcheck:Value8
+ fun:_ZL15sparseset_bit_pP13sparseset_defm
+}
--
1.8.5.3
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH 12/21] PR jit/63854: Add a valgrind suppresion file
2014-01-01 0:00 ` [PATCH 12/21] PR jit/63854: Add a valgrind suppresion file David Malcolm
@ 2014-01-01 0:00 ` Richard Biener
2014-01-01 0:00 ` Jeff Law
0 siblings, 1 reply; 69+ messages in thread
From: Richard Biener @ 2014-01-01 0:00 UTC (permalink / raw)
To: David Malcolm; +Cc: GCC Patches, jit
On Wed, Nov 19, 2014 at 11:46 AM, David Malcolm <dmalcolm@redhat.com> wrote:
> Valgrind complains about uninitialized data within sparseset_bit_p.
> Provide a suppression file to silence these warnings.
>
> Valgrind requires suppression files for C++ code to use the mangled
> names, so we do that here.
There is --enable-valgrind-annotations to get the same effect by GCC
telling valgrind about this (and more).
Richard.
> contrib/ChangeLog
> PR jit/63854
> * valgrind.supp: New.
> ---
> contrib/valgrind.supp | 11 +++++++++++
> 1 file changed, 11 insertions(+)
> create mode 100644 contrib/valgrind.supp
>
> diff --git a/contrib/valgrind.supp b/contrib/valgrind.supp
> new file mode 100644
> index 0000000..ec9ff02
> --- /dev/null
> +++ b/contrib/valgrind.supp
> @@ -0,0 +1,11 @@
> +{
> + suppress-uninit-cond-with-sparseset_bit_p
> + Memcheck:Cond
> + fun:_ZL15sparseset_bit_pP13sparseset_defm
> +}
> +
> +{
> + suppress-uninit-use-with-sparseset_bit_p
> + Memcheck:Value8
> + fun:_ZL15sparseset_bit_pP13sparseset_defm
> +}
> --
> 1.8.5.3
>
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH 12/21] PR jit/63854: Add a valgrind suppresion file
2014-01-01 0:00 ` Richard Biener
@ 2014-01-01 0:00 ` Jeff Law
2014-01-01 0:00 ` David Malcolm
0 siblings, 1 reply; 69+ messages in thread
From: Jeff Law @ 2014-01-01 0:00 UTC (permalink / raw)
To: Richard Biener, David Malcolm; +Cc: GCC Patches, jit
On 11/19/14 04:47, Richard Biener wrote:
> On Wed, Nov 19, 2014 at 11:46 AM, David Malcolm <dmalcolm@redhat.com> wrote:
>> Valgrind complains about uninitialized data within sparseset_bit_p.
>> Provide a suppression file to silence these warnings.
>>
>> Valgrind requires suppression files for C++ code to use the mangled
>> names, so we do that here.
>
> There is --enable-valgrind-annotations to get the same effect by GCC
> telling valgrind about this (and more).
Right. See VALGRIND_DISCARD. Is that not covering this case?
Jeff
^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH 12/21] PR jit/63854: Add a valgrind suppresion file
2014-01-01 0:00 ` Jeff Law
@ 2014-01-01 0:00 ` David Malcolm
2015-01-01 0:00 ` Hans-Peter Nilsson
0 siblings, 1 reply; 69+ messages in thread
From: David Malcolm @ 2014-01-01 0:00 UTC (permalink / raw)
To: Jeff Law; +Cc: Richard Biener, GCC Patches, jit
On Wed, 2014-11-19 at 10:09 -0700, Jeff Law wrote:
> On 11/19/14 04:47, Richard Biener wrote:
> > On Wed, Nov 19, 2014 at 11:46 AM, David Malcolm <dmalcolm@redhat.com> wrote:
> >> Valgrind complains about uninitialized data within sparseset_bit_p.
> >> Provide a suppression file to silence these warnings.
> >>
> >> Valgrind requires suppression files for C++ code to use the mangled
> >> names, so we do that here.
> >
> > There is --enable-valgrind-annotations to get the same effect by GCC
> > telling valgrind about this (and more).
> Right. See VALGRIND_DISCARD. Is that not covering this case?
I simply didn't spot the option, and was running without it.
I'll drop the new file, and document that people running the jit
testsuite under valgrind need to use that configure option.
^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH 12/21] PR jit/63854: Add a valgrind suppresion file
2014-01-01 0:00 ` David Malcolm
@ 2015-01-01 0:00 ` Hans-Peter Nilsson
0 siblings, 0 replies; 69+ messages in thread
From: Hans-Peter Nilsson @ 2015-01-01 0:00 UTC (permalink / raw)
To: David Malcolm; +Cc: Jeff Law, Richard Biener, GCC Patches, jit
On Wed, 19 Nov 2014, David Malcolm wrote:
> On Wed, 2014-11-19 at 10:09 -0700, Jeff Law wrote:
> > On 11/19/14 04:47, Richard Biener wrote:
> > > On Wed, Nov 19, 2014 at 11:46 AM, David Malcolm <dmalcolm@redhat.com> wrote:
> > >> Valgrind complains about uninitialized data within sparseset_bit_p.
> > >> Provide a suppression file to silence these warnings.
> > >>
> > >> Valgrind requires suppression files for C++ code to use the mangled
> > >> names, so we do that here.
> > >
> > > There is --enable-valgrind-annotations to get the same effect by GCC
> > > telling valgrind about this (and more).
> > Right. See VALGRIND_DISCARD. Is that not covering this case?
>
> I simply didn't spot the option, and was running without it.
>
> I'll drop the new file, and document that people running the jit
> testsuite under valgrind need to use that configure option.
IMHO, making --enable-valgrind-annotations the default when
headers are found and when in gcc development is in DEV-PHASE =
experimental (i.e. not for releases) would be even better.
Anyone opposed? I thought it already was the default!
The overhead is IIRC a few weird NOP instructions per
VALGRIND_DISCARD (& Co.) annotation.
brgds, H-P
(PS. I care a little bit since I added them in the first place.)
^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH 20/21] PR jit/63854: Fix leak in ipa-icf.c
2014-01-01 0:00 [PATCH 00/21] PR 63854: Fix various memory leaks David Malcolm
` (9 preceding siblings ...)
2014-01-01 0:00 ` [PATCH 12/21] PR jit/63854: Add a valgrind suppresion file David Malcolm
@ 2014-01-01 0:00 ` David Malcolm
2014-01-01 0:00 ` Richard Biener
2014-01-01 0:00 ` [PATCH 13/21] PR jit/63854: Add support for running "make check-jit" under valgrind David Malcolm
` (9 subsequent siblings)
20 siblings, 1 reply; 69+ messages in thread
From: David Malcolm @ 2014-01-01 0:00 UTC (permalink / raw)
To: gcc-patches, jit; +Cc: David Malcolm
ipa-icf.c was leaking 16 bytes per iteration in the jit testcases here:
80 bytes in 5 blocks are definitely lost in loss record 94 of 199
at 0x4A0645D: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
by 0x5D764B7: xmalloc (xmalloc.c:147)
by 0x5C962FF: ipa_icf::sem_item_optimizer::get_group_by_hash(unsigned int, ipa_icf::sem_item_type) (ipa-icf.c:1503)
by 0x5C96C8C: ipa_icf::sem_item_optimizer::build_hash_based_classes() (ipa-icf.c:1723)
by 0x5C96765: ipa_icf::sem_item_optimizer::execute() (ipa-icf.c:1616)
by 0x5C98E81: ipa_icf::ipa_icf_driver() (ipa-icf.c:2373)
by 0x5C993C8: ipa_icf::pass_ipa_icf::execute(function*) (ipa-icf.c:2421)
by 0x5223CCD: execute_one_pass(opt_pass*) (passes.c:2306)
by 0x5224BA7: execute_ipa_pass_list(opt_pass*) (passes.c:2700)
by 0x4E49B0D: ipa_passes() (cgraphunit.c:2088)
by 0x4E49E2C: symbol_table::compile() (cgraphunit.c:2172)
by 0x4E4A1C2: symbol_table::finalize_compilation_unit() (cgraphunit.c:2325)
by not freeing the allocation here:
congruence_class_group *item = XNEW (congruence_class_group);
in sem_item_optimizer::get_group_by_hash.
Fix it.
gcc/ChangeLog:
PR jit/63854
* ipa-icf.c (sem_item_optimizer::~sem_item_optimizer): Free each
congruence_class_group *.
---
gcc/ipa-icf.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/gcc/ipa-icf.c b/gcc/ipa-icf.c
index 2d5fcf5..b0ba82e 100644
--- a/gcc/ipa-icf.c
+++ b/gcc/ipa-icf.c
@@ -1322,6 +1322,7 @@ sem_item_optimizer::~sem_item_optimizer ()
delete (*it)->classes[i];
(*it)->classes.release ();
+ free (*it);
}
m_items.release ();
--
1.8.5.3
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH 20/21] PR jit/63854: Fix leak in ipa-icf.c
2014-01-01 0:00 ` [PATCH 20/21] PR jit/63854: Fix leak in ipa-icf.c David Malcolm
@ 2014-01-01 0:00 ` Richard Biener
0 siblings, 0 replies; 69+ messages in thread
From: Richard Biener @ 2014-01-01 0:00 UTC (permalink / raw)
To: David Malcolm; +Cc: GCC Patches, jit
On Wed, Nov 19, 2014 at 11:46 AM, David Malcolm <dmalcolm@redhat.com> wrote:
> ipa-icf.c was leaking 16 bytes per iteration in the jit testcases here:
Ok.
Tahnks,
Richard.
> 80 bytes in 5 blocks are definitely lost in loss record 94 of 199
> at 0x4A0645D: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
> by 0x5D764B7: xmalloc (xmalloc.c:147)
> by 0x5C962FF: ipa_icf::sem_item_optimizer::get_group_by_hash(unsigned int, ipa_icf::sem_item_type) (ipa-icf.c:1503)
> by 0x5C96C8C: ipa_icf::sem_item_optimizer::build_hash_based_classes() (ipa-icf.c:1723)
> by 0x5C96765: ipa_icf::sem_item_optimizer::execute() (ipa-icf.c:1616)
> by 0x5C98E81: ipa_icf::ipa_icf_driver() (ipa-icf.c:2373)
> by 0x5C993C8: ipa_icf::pass_ipa_icf::execute(function*) (ipa-icf.c:2421)
> by 0x5223CCD: execute_one_pass(opt_pass*) (passes.c:2306)
> by 0x5224BA7: execute_ipa_pass_list(opt_pass*) (passes.c:2700)
> by 0x4E49B0D: ipa_passes() (cgraphunit.c:2088)
> by 0x4E49E2C: symbol_table::compile() (cgraphunit.c:2172)
> by 0x4E4A1C2: symbol_table::finalize_compilation_unit() (cgraphunit.c:2325)
>
> by not freeing the allocation here:
>
> congruence_class_group *item = XNEW (congruence_class_group);
>
> in sem_item_optimizer::get_group_by_hash.
>
> Fix it.
>
> gcc/ChangeLog:
> PR jit/63854
> * ipa-icf.c (sem_item_optimizer::~sem_item_optimizer): Free each
> congruence_class_group *.
> ---
> gcc/ipa-icf.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/gcc/ipa-icf.c b/gcc/ipa-icf.c
> index 2d5fcf5..b0ba82e 100644
> --- a/gcc/ipa-icf.c
> +++ b/gcc/ipa-icf.c
> @@ -1322,6 +1322,7 @@ sem_item_optimizer::~sem_item_optimizer ()
> delete (*it)->classes[i];
>
> (*it)->classes.release ();
> + free (*it);
> }
>
> m_items.release ();
> --
> 1.8.5.3
>
^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH 13/21] PR jit/63854: Add support for running "make check-jit" under valgrind
2014-01-01 0:00 [PATCH 00/21] PR 63854: Fix various memory leaks David Malcolm
` (10 preceding siblings ...)
2014-01-01 0:00 ` [PATCH 20/21] PR jit/63854: Fix leak in ipa-icf.c David Malcolm
@ 2014-01-01 0:00 ` David Malcolm
2014-01-01 0:00 ` Jeff Law
2014-01-01 0:00 ` [PATCH 21/21] PR jit/63854: Fix leaks in test-fuzzer.c David Malcolm
` (8 subsequent siblings)
20 siblings, 1 reply; 69+ messages in thread
From: David Malcolm @ 2014-01-01 0:00 UTC (permalink / raw)
To: gcc-patches, jit; +Cc: David Malcolm
This commit updates jit.exp so that if RUN_UNDER_VALGRIND is present
in the environment, all of the built client code using libgccjit.so is
run under valgrind, with --leak-check=full.
Hence:
RUN_UNDER_VALGRIND= make check-jit
will run all jit testcases under valgrind (taking 27 mins on my
machine).
Results are written to testsuite/jit/test-FOO.exe.valgrind.txt
jit.exp automatically parses these result file, looking for lines of
the form
definitely lost: 11,316 bytes in 235 blocks
indirectly lost: 352 bytes in 4 blocks
in the valgrind log's summary footer, adding PASSes if they are zero
bytes, and, for now generating XFAILs for non-zero bytes.
Sadly this diverges jit.exp's fixed_host_execute further from DejaGnu's
host_execute, but I don't see a clean way to fix that.
This currently adds 63 PASSes and 49 XFAILs to jit.sum, giving:
# of expected passes 2481
# of expected failures 49
gcc/testsuite/ChangeLog:
PR jit/63854
* jit.dg/jit.exp (report_leak): New.
(parse_valgrind_logfile): New.
(fixed_host_execute): Detect if RUN_UNDER_VALGRIND is present
in the environment, and if so, run the executable under
valgrind, capturing valgrind's output to a logfile. Parse the
log file, generating PASSes and XFAILs for the summary of leaks.
---
gcc/testsuite/jit.dg/jit.exp | 79 +++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 78 insertions(+), 1 deletion(-)
diff --git a/gcc/testsuite/jit.dg/jit.exp b/gcc/testsuite/jit.dg/jit.exp
index 531e929..26ab127 100644
--- a/gcc/testsuite/jit.dg/jit.exp
+++ b/gcc/testsuite/jit.dg/jit.exp
@@ -23,6 +23,48 @@ load_lib target-libpath.exp
load_lib gcc.exp
load_lib dejagnu.exp
+# Look for lines of the form:
+# definitely lost: 11,316 bytes in 235 blocks
+# indirectly lost: 352 bytes in 4 blocks
+# Ideally these would report zero bytes lost (which is a PASS);
+# for now, report non-zero leaks as XFAILs.
+proc report_leak {kind name logfile line} {
+ set match [regexp "$kind lost: .*" $line result]
+ if $match {
+ verbose "Saw \"$result\" within \"$line\"" 3
+ # Extract bytes and blocks.
+ # These can contain commas as well as numerals,
+ # but we only care about whether we have zero.
+ regexp "$kind lost: (.+) bytes in (.+) blocks" \
+ $result -> bytes blocks
+ verbose "bytes: '$bytes'" 2
+ verbose "blocks: '$blocks'" 2
+ if { $bytes == 0 } {
+ pass "$name: $logfile: $result"
+ } else {
+ xfail "$name: $logfile: $result"
+ }
+ }
+}
+
+proc parse_valgrind_logfile {name logfile} {
+ verbose "parse_valgrind_logfile: $logfile" 2
+ if [catch {set f [open $logfile]}] {
+ fail "$name: unable to read $logfile"
+ return
+ }
+
+ while { [gets $f line] >= 0 } {
+ # Strip off the PID prefix e.g. ==7675==
+ set line [regsub "==\[0-9\]*== " $line ""]
+ verbose $line 2
+
+ report_leak "definitely" $name $logfile $line
+ report_leak "indirectly" $name $logfile $line
+ }
+ close $f
+}
+
# This is host_execute from dejagnu.exp commit
# 126a089777158a7891ff975473939f08c0e31a1c
# with the following patch applied, and renaming to "fixed_host_execute".
@@ -49,8 +91,11 @@ load_lib dejagnu.exp
# if there was a problem.
#
proc fixed_host_execute {args} {
+ global env
global text
global spawn_id
+ global srcdir
+ verbose "srcdir: $srcdir" 2
set timeoutmsg "Timed out: Never got started, "
set timeout 100
@@ -75,7 +120,28 @@ proc fixed_host_execute {args} {
# spawn the executable and look for the DejaGnu output messages from the
# test case.
# spawn -noecho -open [open "|./${executable}" "r"]
- spawn -noecho "./${executable}" ${params}
+
+ set run_under_valgrind [info exists env(RUN_UNDER_VALGRIND)]
+
+ if $run_under_valgrind {
+ set valgrind_logfile "${executable}.valgrind.txt"
+ set valgrind_params {"valgrind"}
+ lappend valgrind_params "--leak-check=full"
+ # srcdir is within gcc/testsuite; locate "contrib" relative to it:
+ lappend valgrind_params "--suppressions=${srcdir}/../../contrib/valgrind.supp"
+ lappend valgrind_params "--log-file=${valgrind_logfile}"
+ } else {
+ set valgrind_params {}
+ }
+ verbose "valgrind_params: $valgrind_params" 2
+
+ set args ${valgrind_params}
+ lappend args "./${executable}"
+ set args [concat $args ${params}]
+ verbose "args: $args" 2
+
+ eval spawn -noecho $args
+
expect_after full_buffer { error "got full_buffer" }
set prefix "\[^\r\n\]*"
@@ -147,6 +213,17 @@ proc fixed_host_execute {args} {
# force a close of the executable to be safe.
catch close
+ # valgrind might not have finished writing the log out
+ # before we parse it; need to wait for spawnee to
+ # finish.
+ catch wait reason
+ verbose "reason: $reason" 2
+
+ if $run_under_valgrind {
+ upvar 2 name name
+ parse_valgrind_logfile $name $valgrind_logfile
+ }
+
return ""
}
--
1.8.5.3
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH 13/21] PR jit/63854: Add support for running "make check-jit" under valgrind
2014-01-01 0:00 ` [PATCH 13/21] PR jit/63854: Add support for running "make check-jit" under valgrind David Malcolm
@ 2014-01-01 0:00 ` Jeff Law
2014-01-01 0:00 ` [PATCH 00/05] Fixes to jit.exp David Malcolm
` (3 more replies)
0 siblings, 4 replies; 69+ messages in thread
From: Jeff Law @ 2014-01-01 0:00 UTC (permalink / raw)
To: David Malcolm, gcc-patches, jit
On 11/19/14 03:46, David Malcolm wrote:
> This commit updates jit.exp so that if RUN_UNDER_VALGRIND is present
> in the environment, all of the built client code using libgccjit.so is
> run under valgrind, with --leak-check=full.
>
> Hence:
> RUN_UNDER_VALGRIND= make check-jit
> will run all jit testcases under valgrind (taking 27 mins on my
> machine).
>
> Results are written to testsuite/jit/test-FOO.exe.valgrind.txt
>
> jit.exp automatically parses these result file, looking for lines of
> the form
> definitely lost: 11,316 bytes in 235 blocks
> indirectly lost: 352 bytes in 4 blocks
> in the valgrind log's summary footer, adding PASSes if they are zero
> bytes, and, for now generating XFAILs for non-zero bytes.
>
> Sadly this diverges jit.exp's fixed_host_execute further from DejaGnu's
> host_execute, but I don't see a clean way to fix that.
>
> This currently adds 63 PASSes and 49 XFAILs to jit.sum, giving:
> # of expected passes 2481
> # of expected failures 49
>
> gcc/testsuite/ChangeLog:
> PR jit/63854
> * jit.dg/jit.exp (report_leak): New.
> (parse_valgrind_logfile): New.
> (fixed_host_execute): Detect if RUN_UNDER_VALGRIND is present
> in the environment, and if so, run the executable under
> valgrind, capturing valgrind's output to a logfile. Parse the
> log file, generating PASSes and XFAILs for the summary of leaks.
OK for the trunk. FWIW, I'd love to see a mode where we can easily do
this for the other testsuites as well.
All this work is definitely appreciated -- Jakub usually does similar
stuff later in the release process, so you're offloading some stuff from
him, which definitely helps.
jeff
^ permalink raw reply [flat|nested] 69+ messages in thread* [PATCH 00/05] Fixes to jit.exp
2014-01-01 0:00 ` Jeff Law
@ 2014-01-01 0:00 ` David Malcolm
2014-01-01 0:00 ` [PATCH 13/21] PR jit/63854: Add support for running "make check-jit" under valgrind David Malcolm
` (2 subsequent siblings)
3 siblings, 0 replies; 69+ messages in thread
From: David Malcolm @ 2014-01-01 0:00 UTC (permalink / raw)
To: gcc-patches, jit; +Cc: Jeff Law
Various fixes/improvements to jit.exp
Successfully bootstrapped & regrtested on x86_64-unknown-linux-gnu
(Fedora 20).
OK for trunk?
(am not yet officially blessed as the JIT maintainer, so I require
review for the non-obvious patches)
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH 13/21] PR jit/63854: Add support for running "make check-jit" under valgrind
2014-01-01 0:00 ` Jeff Law
2014-01-01 0:00 ` [PATCH 00/05] Fixes to jit.exp David Malcolm
@ 2014-01-01 0:00 ` David Malcolm
2014-01-01 0:00 ` Jeff Law
2014-01-01 0:00 ` [PATCH 00/05] Fixes to jit.exp David Malcolm
2014-01-01 0:00 ` Running cc1 etc under valgrind (was Re: [PATCH 13/21] PR jit/63854: Add support for running "make check-jit" under valgrind) David Malcolm
3 siblings, 1 reply; 69+ messages in thread
From: David Malcolm @ 2014-01-01 0:00 UTC (permalink / raw)
To: Jeff Law; +Cc: gcc-patches, jit
On Wed, 2014-11-19 at 09:57 -0700, Jeff Law wrote:
> On 11/19/14 03:46, David Malcolm wrote:
> > This commit updates jit.exp so that if RUN_UNDER_VALGRIND is present
> > in the environment, all of the built client code using libgccjit.so is
> > run under valgrind, with --leak-check=full.
> >
> > Hence:
> > RUN_UNDER_VALGRIND= make check-jit
> > will run all jit testcases under valgrind (taking 27 mins on my
> > machine).
> >
> > Results are written to testsuite/jit/test-FOO.exe.valgrind.txt
> >
> > jit.exp automatically parses these result file, looking for lines of
> > the form
> > definitely lost: 11,316 bytes in 235 blocks
> > indirectly lost: 352 bytes in 4 blocks
> > in the valgrind log's summary footer, adding PASSes if they are zero
> > bytes, and, for now generating XFAILs for non-zero bytes.
> >
> > Sadly this diverges jit.exp's fixed_host_execute further from DejaGnu's
> > host_execute, but I don't see a clean way to fix that.
> >
> > This currently adds 63 PASSes and 49 XFAILs to jit.sum, giving:
> > # of expected passes 2481
> > # of expected failures 49
> >
> > gcc/testsuite/ChangeLog:
> > PR jit/63854
> > * jit.dg/jit.exp (report_leak): New.
> > (parse_valgrind_logfile): New.
> > (fixed_host_execute): Detect if RUN_UNDER_VALGRIND is present
> > in the environment, and if so, run the executable under
> > valgrind, capturing valgrind's output to a logfile. Parse the
> > log file, generating PASSes and XFAILs for the summary of leaks.
> OK for the trunk.
Thanks - though the patch I posted uses the contrib/valgrind.supp file,
which I added before seeing the --enable-valgrind-annotations configure
option that does a better job of this. So I'll revise it to remove that
suppression file (and add some usage notes to the internals/index.rst
document).
> FWIW, I'd love to see a mode where we can easily do
> this for the other testsuites as well.
I suspect that the functions report_leak and parse_valgrind_logfile
could be moved into a lib/valgrind.exp at some point if someone wants to
reuse them - maybe adding a param to specify if we expect it to be clean
(PASS) vs leaky (XFAIL) - I'm close to having most of the jit testcases
be valgrind-clean.
> All this work is definitely appreciated -- Jakub usually does similar
> stuff later in the release process, so you're offloading some stuff from
> him, which definitely helps.
Yeah - leaks are a much bigger issue for libgccjit.so than for e.g. cc1:
people embedding it into long-running processes like, say, an X server
aren't going to be happy about slow leaks.
Dave
^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH 13/21] PR jit/63854: Add support for running "make check-jit" under valgrind
2014-01-01 0:00 ` [PATCH 13/21] PR jit/63854: Add support for running "make check-jit" under valgrind David Malcolm
@ 2014-01-01 0:00 ` Jeff Law
0 siblings, 0 replies; 69+ messages in thread
From: Jeff Law @ 2014-01-01 0:00 UTC (permalink / raw)
To: David Malcolm; +Cc: gcc-patches, jit
On 11/19/14 10:11, David Malcolm wrote:
>
> Thanks - though the patch I posted uses the contrib/valgrind.supp file,
> which I added before seeing the --enable-valgrind-annotations configure
> option that does a better job of this. So I'll revise it to remove that
> suppression file (and add some usage notes to the internals/index.rst
> document).
Sounds good.
>
>> All this work is definitely appreciated -- Jakub usually does similar
>> stuff later in the release process, so you're offloading some stuff from
>> him, which definitely helps.
>
> Yeah - leaks are a much bigger issue for libgccjit.so than for e.g. cc1:
> people embedding it into long-running processes like, say, an X server
> aren't going to be happy about slow leaks.
Yea, so imagine shoving libbfd into a link server, which had a GC
collected class around bfd *. One of the key features of the link
server was that it cached information across invocations.
Sooo, we tended to never let go of the underlying BFD objects. Which
wouldn't have been too bad, except that the underlying BFD objects tend
to want to cache section contents.
So this server sat on a mount point and anytime you referenced a file,
it'd consult its little database of "how do I build XYZ". You didn't
have to reference too many files before the damn server ate all your
memory. All in the name of automagically reordering executables...
But I digress...
Jeff
^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH 00/05] Fixes to jit.exp
2014-01-01 0:00 ` Jeff Law
2014-01-01 0:00 ` [PATCH 00/05] Fixes to jit.exp David Malcolm
2014-01-01 0:00 ` [PATCH 13/21] PR jit/63854: Add support for running "make check-jit" under valgrind David Malcolm
@ 2014-01-01 0:00 ` David Malcolm
2014-01-01 0:00 ` [PATCH 01/05] jit.exp: Avoid embedding full paths into test results David Malcolm
` (4 more replies)
2014-01-01 0:00 ` Running cc1 etc under valgrind (was Re: [PATCH 13/21] PR jit/63854: Add support for running "make check-jit" under valgrind) David Malcolm
3 siblings, 5 replies; 69+ messages in thread
From: David Malcolm @ 2014-01-01 0:00 UTC (permalink / raw)
To: gcc-patches, jit; +Cc: Jeff Law, David Malcolm
Various fixes/improvements to jit.exp
Successfully bootstrapped & regrtested on x86_64-unknown-linux-gnu
(Fedora 20).
OK for trunk?
(am not yet officially blessed as the JIT maintainer, so I require
review for the non-obvious patches)
^ permalink raw reply [flat|nested] 69+ messages in thread* [PATCH 01/05] jit.exp: Avoid embedding full paths into test results
2014-01-01 0:00 ` [PATCH 00/05] Fixes to jit.exp David Malcolm
@ 2014-01-01 0:00 ` David Malcolm
2014-01-01 0:00 ` Mike Stump
2014-01-01 0:00 ` [PATCH 03/05] jit.exp: fix timeout bug inherited from dejagnu.exp David Malcolm
` (3 subsequent siblings)
4 siblings, 1 reply; 69+ messages in thread
From: David Malcolm @ 2014-01-01 0:00 UTC (permalink / raw)
To: gcc-patches, jit; +Cc: Jeff Law, David Malcolm
This makes it easier to compare jit.sum files from different runs
gcc/testsuite/ChangeLog:
* jit.dg/jit.exp (jit-dg-test): Use $name rathen than $prog
when calling jit_check_compile to avoid embedding the full path of
the testcase into the test results.
---
gcc/testsuite/jit.dg/jit.exp | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/gcc/testsuite/jit.dg/jit.exp b/gcc/testsuite/jit.dg/jit.exp
index 531e929..135dbad 100644
--- a/gcc/testsuite/jit.dg/jit.exp
+++ b/gcc/testsuite/jit.dg/jit.exp
@@ -202,7 +202,8 @@ proc jit-dg-test { prog do_what extra_tool_flags } {
# Create the test executable:
set comp_output [gcc_target_compile $prog $output_file $do_what \
"{additional_flags=$extra_tool_flags}"]
- if ![jit_check_compile "$prog" "initial compilation" \
+ upvar 1 name name
+ if ![jit_check_compile "$name" "initial compilation" \
$output_file $comp_output] then {
return
}
--
1.8.5.3
^ permalink raw reply [flat|nested] 69+ messages in thread* [PATCH 03/05] jit.exp: fix timeout bug inherited from dejagnu.exp
2014-01-01 0:00 ` [PATCH 00/05] Fixes to jit.exp David Malcolm
2014-01-01 0:00 ` [PATCH 01/05] jit.exp: Avoid embedding full paths into test results David Malcolm
@ 2014-01-01 0:00 ` David Malcolm
2014-01-01 0:00 ` [PATCH 04/05] jit.exp: Verify the exit status of the spawnee David Malcolm
` (2 subsequent siblings)
4 siblings, 0 replies; 69+ messages in thread
From: David Malcolm @ 2014-01-01 0:00 UTC (permalink / raw)
To: gcc-patches, jit; +Cc: Jeff Law, David Malcolm
DejaGnu's host_execute has an erroneous line that appears to be a
merge conflict that went awry followed up by a reindent, and jit.exp
has inherited this bug within "fixed_host_execute".
Fix it within jit.exp.
Reported to DejaGnu as:
http://lists.gnu.org/archive/html/dejagnu/2014-11/msg00001.html
gcc/testsuite/ChangeLog:
* jit.dg/jit.exp (fixed_host_execute): Fix timeout bug.
---
gcc/testsuite/jit.dg/jit.exp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/gcc/testsuite/jit.dg/jit.exp b/gcc/testsuite/jit.dg/jit.exp
index 9179a15..2edd048 100644
--- a/gcc/testsuite/jit.dg/jit.exp
+++ b/gcc/testsuite/jit.dg/jit.exp
@@ -199,7 +199,7 @@ proc fixed_host_execute {args} {
incr timetol
exp_continue
} else {
- - catch close
+ catch close
return "Timed out executing test case"
}
}
--
1.8.5.3
^ permalink raw reply [flat|nested] 69+ messages in thread* [PATCH 04/05] jit.exp: Verify the exit status of the spawnee
2014-01-01 0:00 ` [PATCH 00/05] Fixes to jit.exp David Malcolm
2014-01-01 0:00 ` [PATCH 01/05] jit.exp: Avoid embedding full paths into test results David Malcolm
2014-01-01 0:00 ` [PATCH 03/05] jit.exp: fix timeout bug inherited from dejagnu.exp David Malcolm
@ 2014-01-01 0:00 ` David Malcolm
2014-01-01 0:00 ` [PATCH 02/05] PR jit/63854: Add support for running "make check-jit" under valgrind David Malcolm
2014-01-01 0:00 ` [PATCH 05/05] Add command-line option-parsing to jit testcases David Malcolm
4 siblings, 0 replies; 69+ messages in thread
From: David Malcolm @ 2014-01-01 0:00 UTC (permalink / raw)
To: gcc-patches, jit; +Cc: Jeff Law, David Malcolm
gcc/testsuite/ChangeLog:
* jit.dg/jit.exp (verify_exit_status): New function.
(fixed_host_execute): Verify the exit status of the spawnee.
---
gcc/testsuite/jit.dg/jit.exp | 30 ++++++++++++++++++++++++++++++
1 file changed, 30 insertions(+)
diff --git a/gcc/testsuite/jit.dg/jit.exp b/gcc/testsuite/jit.dg/jit.exp
index 2edd048..a37ccc7 100644
--- a/gcc/testsuite/jit.dg/jit.exp
+++ b/gcc/testsuite/jit.dg/jit.exp
@@ -65,6 +65,35 @@ proc parse_valgrind_logfile {name logfile} {
close $f
}
+# Given WRES, the result from "wait", issue a PASS
+# if the spawnee exited cleanly, or a FAIL for various kinds of
+# unexpected exits.
+
+proc verify_exit_status { executable wres } {
+ lassign $wres pid spawnid os_error_flag value
+ verbose "pid: $pid" 3
+ verbose "spawnid: $spawnid" 3
+ verbose "os_error_flag: $os_error_flag" 3
+ verbose "value: $value" 3
+
+ # Detect segfaults etc:
+ if { [llength $wres] > 4 } {
+ if { [lindex $wres 4] == "CHILDKILLED" } {
+ fail "$executable killed: $wres"
+ return
+ }
+ }
+ if { $os_error_flag != 0 } {
+ fail "$executable: OS error: $wres"
+ return
+ }
+ if { $value != 0 } {
+ fail "$executable: non-zero exit code: $wres"
+ return
+ }
+ pass "$executable exited cleanly"
+}
+
# This is host_execute from dejagnu.exp commit
# 126a089777158a7891ff975473939f08c0e31a1c
# with the following patch applied, and renaming to "fixed_host_execute".
@@ -214,6 +243,7 @@ proc fixed_host_execute {args} {
catch wait wres
verbose "wres: $wres" 2
+ verify_exit_status $executable $wres
if $run_under_valgrind {
upvar 2 name name
--
1.8.5.3
^ permalink raw reply [flat|nested] 69+ messages in thread* [PATCH 02/05] PR jit/63854: Add support for running "make check-jit" under valgrind
2014-01-01 0:00 ` [PATCH 00/05] Fixes to jit.exp David Malcolm
` (2 preceding siblings ...)
2014-01-01 0:00 ` [PATCH 04/05] jit.exp: Verify the exit status of the spawnee David Malcolm
@ 2014-01-01 0:00 ` David Malcolm
2014-01-01 0:00 ` [PATCH 05/05] Add command-line option-parsing to jit testcases David Malcolm
4 siblings, 0 replies; 69+ messages in thread
From: David Malcolm @ 2014-01-01 0:00 UTC (permalink / raw)
To: gcc-patches, jit; +Cc: Jeff Law, David Malcolm
This commit updates jit.exp so that if RUN_UNDER_VALGRIND is present
in the environment, all of the built client code using libgccjit.so is
run under valgrind, with --leak-check=full.
Hence:
RUN_UNDER_VALGRIND= make check-jit
will run all jit testcases under valgrind (taking 27 mins on my
machine).
Results are written to testsuite/jit/test-FOO.exe.valgrind.txt
jit.exp automatically parses these result files, looking for lines of
the form
definitely lost: 11,316 bytes in 235 blocks
indirectly lost: 352 bytes in 4 blocks
in the valgrind log's summary footer, adding PASSes if they are zero
bytes, and, for now generating XFAILs for non-zero bytes.
Sadly this diverges jit.exp's fixed_host_execute further from DejaGnu's
host_execute, but I don't see a clean way to fix that.
This currently adds 63 PASSes and 49 XFAILs to jit.sum, giving:
# of expected passes 2481
# of expected failures 49
This is somewhat revised from the previous patch: it eliminates the
suppression file, and reinstates the "catch close".
gcc/testsuite/ChangeLog:
PR jit/63854
* jit.dg/jit.exp (report_leak): New.
(parse_valgrind_logfile): New.
(fixed_host_execute): Detect if RUN_UNDER_VALGRIND is present
in the environment, and if so, run the executable under
valgrind, capturing valgrind's output to a logfile. Parse the
log file, generating PASSes and XFAILs for the summary of leaks.
Use "wait" before "close": valgrind might not have finished
writing the log out before we parse it, so we need to wait for
the spawnee to finish.
---
gcc/testsuite/jit.dg/jit.exp | 78 +++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 77 insertions(+), 1 deletion(-)
diff --git a/gcc/testsuite/jit.dg/jit.exp b/gcc/testsuite/jit.dg/jit.exp
index 135dbad..9179a15 100644
--- a/gcc/testsuite/jit.dg/jit.exp
+++ b/gcc/testsuite/jit.dg/jit.exp
@@ -23,6 +23,48 @@ load_lib target-libpath.exp
load_lib gcc.exp
load_lib dejagnu.exp
+# Look for lines of the form:
+# definitely lost: 11,316 bytes in 235 blocks
+# indirectly lost: 352 bytes in 4 blocks
+# Ideally these would report zero bytes lost (which is a PASS);
+# for now, report non-zero leaks as XFAILs.
+proc report_leak {kind name logfile line} {
+ set match [regexp "$kind lost: .*" $line result]
+ if $match {
+ verbose "Saw \"$result\" within \"$line\"" 4
+ # Extract bytes and blocks.
+ # These can contain commas as well as numerals,
+ # but we only care about whether we have zero.
+ regexp "$kind lost: (.+) bytes in (.+) blocks" \
+ $result -> bytes blocks
+ verbose "bytes: '$bytes'" 4
+ verbose "blocks: '$blocks'" 4
+ if { $bytes == 0 } {
+ pass "$name: $logfile: $result"
+ } else {
+ xfail "$name: $logfile: $result"
+ }
+ }
+}
+
+proc parse_valgrind_logfile {name logfile} {
+ verbose "parse_valgrind_logfile: $logfile" 2
+ if [catch {set f [open $logfile]}] {
+ fail "$name: unable to read $logfile"
+ return
+ }
+
+ while { [gets $f line] >= 0 } {
+ # Strip off the PID prefix e.g. ==7675==
+ set line [regsub "==\[0-9\]*== " $line ""]
+ verbose $line 2
+
+ report_leak "definitely" $name $logfile $line
+ report_leak "indirectly" $name $logfile $line
+ }
+ close $f
+}
+
# This is host_execute from dejagnu.exp commit
# 126a089777158a7891ff975473939f08c0e31a1c
# with the following patch applied, and renaming to "fixed_host_execute".
@@ -49,6 +91,7 @@ load_lib dejagnu.exp
# if there was a problem.
#
proc fixed_host_execute {args} {
+ global env
global text
global spawn_id
@@ -75,7 +118,28 @@ proc fixed_host_execute {args} {
# spawn the executable and look for the DejaGnu output messages from the
# test case.
# spawn -noecho -open [open "|./${executable}" "r"]
- spawn -noecho "./${executable}" ${params}
+
+ # Run under valgrind if RUN_UNDER_VALGRIND is present in the environment.
+ # Note that it's best to configure gcc with --enable-valgrind-annotations
+ # when testing under valgrind.
+ set run_under_valgrind [info exists env(RUN_UNDER_VALGRIND)]
+ if $run_under_valgrind {
+ set valgrind_logfile "${executable}.valgrind.txt"
+ set valgrind_params {"valgrind"}
+ lappend valgrind_params "--leak-check=full"
+ lappend valgrind_params "--log-file=${valgrind_logfile}"
+ } else {
+ set valgrind_params {}
+ }
+ verbose "valgrind_params: $valgrind_params" 2
+
+ set args ${valgrind_params}
+ lappend args "./${executable}"
+ set args [concat $args ${params}]
+ verbose "args: $args" 2
+
+ eval spawn -noecho $args
+
expect_after full_buffer { error "got full_buffer" }
set prefix "\[^\r\n\]*"
@@ -144,6 +208,18 @@ proc fixed_host_execute {args} {
}
}
+ # Use "wait" before "close": valgrind might not have finished
+ # writing the log out before we parse it, so we need to wait for
+ # the spawnee to finish.
+
+ catch wait wres
+ verbose "wres: $wres" 2
+
+ if $run_under_valgrind {
+ upvar 2 name name
+ parse_valgrind_logfile $name $valgrind_logfile
+ }
+
# force a close of the executable to be safe.
catch close
--
1.8.5.3
^ permalink raw reply [flat|nested] 69+ messages in thread* [PATCH 05/05] Add command-line option-parsing to jit testcases
2014-01-01 0:00 ` [PATCH 00/05] Fixes to jit.exp David Malcolm
` (3 preceding siblings ...)
2014-01-01 0:00 ` [PATCH 02/05] PR jit/63854: Add support for running "make check-jit" under valgrind David Malcolm
@ 2014-01-01 0:00 ` David Malcolm
2014-01-01 0:00 ` PING: " David Malcolm
4 siblings, 1 reply; 69+ messages in thread
From: David Malcolm @ 2014-01-01 0:00 UTC (permalink / raw)
To: gcc-patches, jit; +Cc: Jeff Law, David Malcolm
Add command-line option-parsing to the testcases, so that we can
manipulate them without needing a recompile (e.g. varying
optimization levels etc).
This uses getopt_long, which is a GNU extension to libc. Is that
acceptable?
Implement a --num-iterations option, to override the default of 5.
When running tests under valgrind, pass in "--num-iterations=1",
speeding up the tests by a factor of 5.
gcc/testsuite/ChangeLog:
* jit.dg/harness.h (num_iterations): New variable.
(parse_options): New function.
(main): Use "num_iterations", rather than hardcoding 5.
* jit.dg/jit.exp (fixed_host_execute): When running tests under
valgrind, pass in "--num-iterations=1".
---
gcc/testsuite/jit.dg/harness.h | 46 +++++++++++++++++++++++++++++++++++++++---
gcc/testsuite/jit.dg/jit.exp | 5 +++++
2 files changed, 48 insertions(+), 3 deletions(-)
diff --git a/gcc/testsuite/jit.dg/harness.h b/gcc/testsuite/jit.dg/harness.h
index bff64de..a30b66e 100644
--- a/gcc/testsuite/jit.dg/harness.h
+++ b/gcc/testsuite/jit.dg/harness.h
@@ -247,17 +247,57 @@ extract_progname (const char *argv0)
}
#ifndef TEST_PROVIDES_MAIN
+
+/* Option parsing, for varying how we run the built testcases
+ (e.g. when poking at things in the debugger). */
+
+#include <getopt.h>
+int num_iterations = 5;
+
+static void
+parse_options (int argc, char **argv)
+{
+ while (1)
+ {
+ static struct option long_options[] =
+ {
+ {"num-iterations", required_argument, 0, 'i'},
+ {0, 0, 0, 0}
+ };
+
+ int option_index = 0;
+ /* getopt_long is a GNU extension to libc. */
+ int c = getopt_long (argc, argv, "i:",
+ long_options, &option_index);
+ if (c == -1)
+ break;
+
+ switch (c)
+ {
+ case 'i':
+ num_iterations = atoi (optarg);
+ break;
+ default:
+ fprintf (stderr, "Usage: %s [--num-iterations NUM]\n",
+ argv[0]);
+ exit(EXIT_FAILURE);
+ }
+ }
+}
+
int
main (int argc, char **argv)
{
int i;
- for (i = 1; i <= 5; i++)
+ parse_options (argc, argv);
+
+ for (i = 1; i <= num_iterations; i++)
{
snprintf (test, sizeof (test),
"%s iteration %d of %d",
- extract_progname (argv[0]),
- i, 5);
+ extract_progname (argv[0]),
+ i, num_iterations);
//printf ("ITERATION %d\n", i);
test_jit (argv[0], NULL);
diff --git a/gcc/testsuite/jit.dg/jit.exp b/gcc/testsuite/jit.dg/jit.exp
index a37ccc7..438aabd 100644
--- a/gcc/testsuite/jit.dg/jit.exp
+++ b/gcc/testsuite/jit.dg/jit.exp
@@ -157,6 +157,11 @@ proc fixed_host_execute {args} {
set valgrind_params {"valgrind"}
lappend valgrind_params "--leak-check=full"
lappend valgrind_params "--log-file=${valgrind_logfile}"
+ # When running under valgrind, speed things up by only running one
+ # in-process iteration of the testcase, rather than the default of 5.
+ # Only testcases that use the "main" from harness.h
+ # (#ifndef TEST_PROVIDES_MAIN) will respond to this option.
+ lappend params "--num-iterations=1"
} else {
set valgrind_params {}
}
--
1.8.5.3
^ permalink raw reply [flat|nested] 69+ messages in thread* PING: Re: [PATCH 05/05] Add command-line option-parsing to jit testcases
2014-01-01 0:00 ` [PATCH 05/05] Add command-line option-parsing to jit testcases David Malcolm
@ 2014-01-01 0:00 ` David Malcolm
2014-01-01 0:00 ` Mike Stump
0 siblings, 1 reply; 69+ messages in thread
From: David Malcolm @ 2014-01-01 0:00 UTC (permalink / raw)
To: gcc-patches; +Cc: jit, Jeff Law
On Tue, 2014-11-25 at 20:34 -0500, David Malcolm wrote:
> Add command-line option-parsing to the testcases, so that we can
> manipulate them without needing a recompile (e.g. varying
> optimization levels etc).
>
> This uses getopt_long, which is a GNU extension to libc. Is that
> acceptable?
Ping. Specifically, is it acceptable to use getopt_long within the jit
testcases, or should I find another way to do this (e.g. just getopt)?
Sorry that it wasn't obvious that I was asking for review on this one.
> Implement a --num-iterations option, to override the default of 5.
>
> When running tests under valgrind, pass in "--num-iterations=1",
> speeding up the tests by a factor of 5.
>
> gcc/testsuite/ChangeLog:
> * jit.dg/harness.h (num_iterations): New variable.
> (parse_options): New function.
> (main): Use "num_iterations", rather than hardcoding 5.
> * jit.dg/jit.exp (fixed_host_execute): When running tests under
> valgrind, pass in "--num-iterations=1".
> ---
> gcc/testsuite/jit.dg/harness.h | 46 +++++++++++++++++++++++++++++++++++++++---
> gcc/testsuite/jit.dg/jit.exp | 5 +++++
> 2 files changed, 48 insertions(+), 3 deletions(-)
>
> diff --git a/gcc/testsuite/jit.dg/harness.h b/gcc/testsuite/jit.dg/harness.h
> index bff64de..a30b66e 100644
> --- a/gcc/testsuite/jit.dg/harness.h
> +++ b/gcc/testsuite/jit.dg/harness.h
> @@ -247,17 +247,57 @@ extract_progname (const char *argv0)
> }
>
> #ifndef TEST_PROVIDES_MAIN
> +
> +/* Option parsing, for varying how we run the built testcases
> + (e.g. when poking at things in the debugger). */
> +
> +#include <getopt.h>
> +int num_iterations = 5;
> +
> +static void
> +parse_options (int argc, char **argv)
> +{
> + while (1)
> + {
> + static struct option long_options[] =
> + {
> + {"num-iterations", required_argument, 0, 'i'},
> + {0, 0, 0, 0}
> + };
> +
> + int option_index = 0;
> + /* getopt_long is a GNU extension to libc. */
> + int c = getopt_long (argc, argv, "i:",
> + long_options, &option_index);
> + if (c == -1)
> + break;
> +
> + switch (c)
> + {
> + case 'i':
> + num_iterations = atoi (optarg);
> + break;
> + default:
> + fprintf (stderr, "Usage: %s [--num-iterations NUM]\n",
> + argv[0]);
> + exit(EXIT_FAILURE);
> + }
> + }
> +}
> +
> int
> main (int argc, char **argv)
> {
> int i;
>
> - for (i = 1; i <= 5; i++)
> + parse_options (argc, argv);
> +
> + for (i = 1; i <= num_iterations; i++)
> {
> snprintf (test, sizeof (test),
> "%s iteration %d of %d",
> - extract_progname (argv[0]),
> - i, 5);
> + extract_progname (argv[0]),
> + i, num_iterations);
>
> //printf ("ITERATION %d\n", i);
> test_jit (argv[0], NULL);
> diff --git a/gcc/testsuite/jit.dg/jit.exp b/gcc/testsuite/jit.dg/jit.exp
> index a37ccc7..438aabd 100644
> --- a/gcc/testsuite/jit.dg/jit.exp
> +++ b/gcc/testsuite/jit.dg/jit.exp
> @@ -157,6 +157,11 @@ proc fixed_host_execute {args} {
> set valgrind_params {"valgrind"}
> lappend valgrind_params "--leak-check=full"
> lappend valgrind_params "--log-file=${valgrind_logfile}"
> + # When running under valgrind, speed things up by only running one
> + # in-process iteration of the testcase, rather than the default of 5.
> + # Only testcases that use the "main" from harness.h
> + # (#ifndef TEST_PROVIDES_MAIN) will respond to this option.
> + lappend params "--num-iterations=1"
> } else {
> set valgrind_params {}
> }
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: PING: Re: [PATCH 05/05] Add command-line option-parsing to jit testcases
2014-01-01 0:00 ` PING: " David Malcolm
@ 2014-01-01 0:00 ` Mike Stump
2014-01-01 0:00 ` David Malcolm
0 siblings, 1 reply; 69+ messages in thread
From: Mike Stump @ 2014-01-01 0:00 UTC (permalink / raw)
To: David Malcolm; +Cc: gcc-patches, jit, Jeff Law
On Dec 8, 2014, at 12:44 PM, David Malcolm <dmalcolm@redhat.com> wrote:
> On Tue, 2014-11-25 at 20:34 -0500, David Malcolm wrote:
>> Add command-line option-parsing to the testcases, so that we can
>> manipulate them without needing a recompile (e.g. varying
>> optimization levels etc).
>>
>> This uses getopt_long, which is a GNU extension to libc. Is that
>> acceptable?
>
> Ping. Specifically, is it acceptable to use getopt_long within the jit
> testcases, or should I find another way to do this (e.g. just getopt)?
So, the standard by which we measure, does it kill building or testing of ada on mingwin? If it does, then no, not acceptable. I’d like to think there is nothing you can do in jit.exp that could do that. So, from this perspective, yeah, feel free to do what you want. Git it done first. The second person that wants to port your code to a new machine (a different machine) will trip over all the bad things you did, and someone will then have to fix them.
If you only use what gcc already sues, you will be portable to everything gcc is portable to. If you use GNU extensions to libc, well, that isn’t portable enough in general. Heck, even what’s in libc isn’t portable enough, that’s half the reason why we have autoconf in the first place.
If jit is on by default everywhere, then you need to be portable everywhere. If only on for linux, then well, it already has GNU extensions in libc. I don’t know if it is on by default and you didn’t say, so, hard to comment on it.
^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: PING: Re: [PATCH 05/05] Add command-line option-parsing to jit testcases
2014-01-01 0:00 ` Mike Stump
@ 2014-01-01 0:00 ` David Malcolm
2014-01-01 0:00 ` Mike Stump
0 siblings, 1 reply; 69+ messages in thread
From: David Malcolm @ 2014-01-01 0:00 UTC (permalink / raw)
To: Mike Stump; +Cc: gcc-patches, jit, Jeff Law
On Mon, 2014-12-08 at 13:57 -0800, Mike Stump wrote:
> On Dec 8, 2014, at 12:44 PM, David Malcolm <dmalcolm@redhat.com>
> wrote:
> > On Tue, 2014-11-25 at 20:34 -0500, David Malcolm wrote:
> >> Add command-line option-parsing to the testcases, so that we can
> >> manipulate them without needing a recompile (e.g. varying
> >> optimization levels etc).
> >>
> >> This uses getopt_long, which is a GNU extension to libc. Is that
> >> acceptable?
> >
> > Ping. Specifically, is it acceptable to use getopt_long within the
> jit
> > testcases, or should I find another way to do this (e.g. just
> getopt)?
>
> So, the standard by which we measure, does it kill building or testing
> of ada on mingwin? If it does, then no, not acceptable. I’d like to
> think there is nothing you can do in jit.exp that could do that. So,
> from this perspective, yeah, feel free to do what you want. Git it
> done first. The second person that wants to port your code to a new
> machine (a different machine) will trip over all the bad things you
> did, and someone will then have to fix them.
>
> If you only use what gcc already sues, you will be portable to
> everything gcc is portable to. If you use GNU extensions to libc,
> well, that isn’t portable enough in general. Heck, even what’s in
> libc isn’t portable enough, that’s half the reason why we have
> autoconf in the first place.
>
> If jit is on by default everywhere, then you need to be portable
> everywhere. If only on for linux, then well, it already has GNU
> extensions in libc. I don’t know if it is on by default and you
> didn’t say, so, hard to comment on it.
Thanks.
The only stuff I'm using getopt_long for is to make the binaries built
by jit.exp be more flexible e.g. so that I can turn down the number of
iterations they run when running under valgrind (and potentially other
tweaks, so e.g. I can experiment with them under gdb without having to
recompile them)
Hence I think we can simply fall back to ignoring argv on hosts that
don't support getopt_long; it should merely make the testsuite less
flexible. Not sure how best to encode such a test though - check for it
in jit.exp, or in configure, I suppose.
Dave
^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: PING: Re: [PATCH 05/05] Add command-line option-parsing to jit testcases
2014-01-01 0:00 ` David Malcolm
@ 2014-01-01 0:00 ` Mike Stump
0 siblings, 0 replies; 69+ messages in thread
From: Mike Stump @ 2014-01-01 0:00 UTC (permalink / raw)
To: David Malcolm; +Cc: gcc-patches, jit, Jeff Law
On Dec 8, 2014, at 5:29 PM, David Malcolm <dmalcolm@redhat.com> wrote:
> The only stuff I'm using getopt_long for is to make the binaries built
> by jit.exp be more flexible e.g. so that I can turn down the number of
> iterations they run when running under valgrind (and potentially other
> tweaks, so e.g. I can experiment with them under gdb without having to
> recompile them)
>
> Hence I think we can simply fall back to ignoring argv on hosts that
> don't support getopt_long; it should merely make the testsuite less
> flexible. Not sure how best to encode such a test though - check for it
> in jit.exp, or in configure, I suppose.
I’d just check in jit.exp. See target-supports.exp for ideas.
^ permalink raw reply [flat|nested] 69+ messages in thread
* Running cc1 etc under valgrind (was Re: [PATCH 13/21] PR jit/63854: Add support for running "make check-jit" under valgrind)
2014-01-01 0:00 ` Jeff Law
` (2 preceding siblings ...)
2014-01-01 0:00 ` [PATCH 00/05] Fixes to jit.exp David Malcolm
@ 2014-01-01 0:00 ` David Malcolm
2014-01-01 0:00 ` David Malcolm
3 siblings, 1 reply; 69+ messages in thread
From: David Malcolm @ 2014-01-01 0:00 UTC (permalink / raw)
To: Jeff Law; +Cc: gcc-patches, jit
On Wed, 2014-11-19 at 09:57 -0700, Jeff Law wrote:
> On 11/19/14 03:46, David Malcolm wrote:
> > This commit updates jit.exp so that if RUN_UNDER_VALGRIND is present
> > in the environment, all of the built client code using libgccjit.so is
> > run under valgrind, with --leak-check=full.
> >
> > Hence:
> > RUN_UNDER_VALGRIND= make check-jit
> > will run all jit testcases under valgrind (taking 27 mins on my
> > machine).
> >
> > Results are written to testsuite/jit/test-FOO.exe.valgrind.txt
> >
> > jit.exp automatically parses these result file, looking for lines of
> > the form
> > definitely lost: 11,316 bytes in 235 blocks
> > indirectly lost: 352 bytes in 4 blocks
> > in the valgrind log's summary footer, adding PASSes if they are zero
> > bytes, and, for now generating XFAILs for non-zero bytes.
> >
> > Sadly this diverges jit.exp's fixed_host_execute further from DejaGnu's
> > host_execute, but I don't see a clean way to fix that.
> >
> > This currently adds 63 PASSes and 49 XFAILs to jit.sum, giving:
> > # of expected passes 2481
> > # of expected failures 49
> >
> > gcc/testsuite/ChangeLog:
> > PR jit/63854
> > * jit.dg/jit.exp (report_leak): New.
> > (parse_valgrind_logfile): New.
> > (fixed_host_execute): Detect if RUN_UNDER_VALGRIND is present
> > in the environment, and if so, run the executable under
> > valgrind, capturing valgrind's output to a logfile. Parse the
> > log file, generating PASSes and XFAILs for the summary of leaks.
> OK for the trunk. FWIW, I'd love to see a mode where we can easily do
> this for the other testsuites as well.
Many of the cleanups in these patches are called from toplev::finalize,
or something called from there, so that they're called by libgccjit.so,
but not called by cc1/cc1plus etc.
In general, cc1 etc don't need to bother free-ing everything, and can
instead simply exit.
But if you're running under valgrind, you'd probably want them to call
toplev::finalize before exiting, to make the valgrind log shorter.
So perhaps cc1 etc could detect if they're being run under valgrind, and
call toplev::finalize in the appropriate place?
Or maybe this could be a command-line option?
[I think I prefer autodetection, fwiw]
Thoughts?
Dave
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: Running cc1 etc under valgrind (was Re: [PATCH 13/21] PR jit/63854: Add support for running "make check-jit" under valgrind)
2014-01-01 0:00 ` Running cc1 etc under valgrind (was Re: [PATCH 13/21] PR jit/63854: Add support for running "make check-jit" under valgrind) David Malcolm
@ 2014-01-01 0:00 ` David Malcolm
0 siblings, 0 replies; 69+ messages in thread
From: David Malcolm @ 2014-01-01 0:00 UTC (permalink / raw)
To: Jeff Law; +Cc: gcc-patches, jit
On Wed, 2014-11-19 at 13:38 -0500, David Malcolm wrote:
> On Wed, 2014-11-19 at 09:57 -0700, Jeff Law wrote:
> > On 11/19/14 03:46, David Malcolm wrote:
> > > This commit updates jit.exp so that if RUN_UNDER_VALGRIND is present
> > > in the environment, all of the built client code using libgccjit.so is
> > > run under valgrind, with --leak-check=full.
> > >
> > > Hence:
> > > RUN_UNDER_VALGRIND= make check-jit
> > > will run all jit testcases under valgrind (taking 27 mins on my
> > > machine).
> > >
> > > Results are written to testsuite/jit/test-FOO.exe.valgrind.txt
> > >
> > > jit.exp automatically parses these result file, looking for lines of
> > > the form
> > > definitely lost: 11,316 bytes in 235 blocks
> > > indirectly lost: 352 bytes in 4 blocks
> > > in the valgrind log's summary footer, adding PASSes if they are zero
> > > bytes, and, for now generating XFAILs for non-zero bytes.
> > >
> > > Sadly this diverges jit.exp's fixed_host_execute further from DejaGnu's
> > > host_execute, but I don't see a clean way to fix that.
> > >
> > > This currently adds 63 PASSes and 49 XFAILs to jit.sum, giving:
> > > # of expected passes 2481
> > > # of expected failures 49
> > >
> > > gcc/testsuite/ChangeLog:
> > > PR jit/63854
> > > * jit.dg/jit.exp (report_leak): New.
> > > (parse_valgrind_logfile): New.
> > > (fixed_host_execute): Detect if RUN_UNDER_VALGRIND is present
> > > in the environment, and if so, run the executable under
> > > valgrind, capturing valgrind's output to a logfile. Parse the
> > > log file, generating PASSes and XFAILs for the summary of leaks.
> > OK for the trunk. FWIW, I'd love to see a mode where we can easily do
> > this for the other testsuites as well.
>
> Many of the cleanups in these patches are called from toplev::finalize,
> or something called from there, so that they're called by libgccjit.so,
> but not called by cc1/cc1plus etc.
>
> In general, cc1 etc don't need to bother free-ing everything, and can
> instead simply exit.
>
> But if you're running under valgrind, you'd probably want them to call
> toplev::finalize before exiting, to make the valgrind log shorter.
>
> So perhaps cc1 etc could detect if they're being run under valgrind, and
> call toplev::finalize in the appropriate place?
>
> Or maybe this could be a command-line option?
>
> [I think I prefer autodetection, fwiw]
It turns out that this isn't necessary (I think), since the pointers are
typically still reachable.
^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH 21/21] PR jit/63854: Fix leaks in test-fuzzer.c
2014-01-01 0:00 [PATCH 00/21] PR 63854: Fix various memory leaks David Malcolm
` (11 preceding siblings ...)
2014-01-01 0:00 ` [PATCH 13/21] PR jit/63854: Add support for running "make check-jit" under valgrind David Malcolm
@ 2014-01-01 0:00 ` David Malcolm
2014-01-01 0:00 ` Jeff Law
2014-01-01 0:00 ` [PATCH 14/21] PR jit/63854: Fix leak of paths within jump threading David Malcolm
` (7 subsequent siblings)
20 siblings, 1 reply; 69+ messages in thread
From: David Malcolm @ 2014-01-01 0:00 UTC (permalink / raw)
To: gcc-patches, jit; +Cc: David Malcolm
gcc/testsuite/ChangeLog:
PR jit/63854
* jit.dg/test-fuzzer.c (fuzzer_init): Free malloced buffers.
(make_random_function): Free ff->locals.
---
gcc/testsuite/jit.dg/test-fuzzer.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/gcc/testsuite/jit.dg/test-fuzzer.c b/gcc/testsuite/jit.dg/test-fuzzer.c
index f363f8f..b501792 100644
--- a/gcc/testsuite/jit.dg/test-fuzzer.c
+++ b/gcc/testsuite/jit.dg/test-fuzzer.c
@@ -105,6 +105,11 @@ fuzzer_init (fuzzer *f, gcc_jit_context *ctxt, unsigned int seed)
for (i = 0; i < num_funcs; i++)
f->funcs[f->num_funcs++] = make_random_function (f);
+
+ /* Now clean out f. */
+ free (f->types);
+ free (f->funcs);
+ free (f->globals);
}
/* Get random int in inclusive range [min, max]. */
@@ -309,6 +314,7 @@ make_random_function (fuzzer *f)
gcc_jit_function *result = ff->fn;
+ free (ff->locals);
free (ff->params);
free (ff);
--
1.8.5.3
^ permalink raw reply [flat|nested] 69+ messages in thread* [PATCH 14/21] PR jit/63854: Fix leak of paths within jump threading
2014-01-01 0:00 [PATCH 00/21] PR 63854: Fix various memory leaks David Malcolm
` (12 preceding siblings ...)
2014-01-01 0:00 ` [PATCH 21/21] PR jit/63854: Fix leaks in test-fuzzer.c David Malcolm
@ 2014-01-01 0:00 ` David Malcolm
2014-01-01 0:00 ` Richard Biener
2014-01-01 0:00 ` [PATCH 09/21] PR jit/63854: Don't leak producer_string in dwarf2out.c David Malcolm
` (6 subsequent siblings)
20 siblings, 1 reply; 69+ messages in thread
From: David Malcolm @ 2014-01-01 0:00 UTC (permalink / raw)
To: gcc-patches, jit; +Cc: David Malcolm
Paths are allocated as:
vec<jump_thread_edge *> *path = new vec<jump_thread_edge *> ();
i.e. the vec itself is on the heap, so a mere:
path->release ();
is merely releasing the buffer the vec holds, not the vec itself.
This patch updates the two sites that release paths to also delete them,
fixing leaks like this seen by valgrind:
160 bytes in 20 blocks are definitely lost in loss record 165 of 241
at 0x4A06965: operator new(unsigned long) (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
by 0x556E5AF: thread_across_edge(gimple_statement_base*, edge_def*, bool, vec<tree_node*, va_heap, vl_ptr>*, tree_node
* (*)(gimple_statement_base*, gimple_statement_base*)) (tree-ssa-threadedge.c:1124)
by 0x547B73B: dom_opt_dom_walker::thread_across_edge(edge_def*) (tree-ssa-dom.c:1184)
by 0x5480183: dom_opt_dom_walker::after_dom_children(basic_block_def*) (tree-ssa-dom.c:2015)
by 0x5C1C5F7: dom_walker::walk(basic_block_def*) (domwalk.c:218)
by 0x547AE83: (anonymous namespace)::pass_dominator::execute(function*) (tree-ssa-dom.c:919)
by 0x52235BD: execute_one_pass(opt_pass*) (passes.c:2306)
by 0x5223834: execute_pass_list_1(opt_pass*) (passes.c:2358)
by 0x5223865: execute_pass_list_1(opt_pass*) (passes.c:2359)
by 0x52238A2: execute_pass_list(function*, opt_pass*) (passes.c:2369)
by 0x4E4888F: cgraph_node::expand() (cgraphunit.c:1773)
by 0x4E48F29: expand_all_functions() (cgraphunit.c:1909)
[Space-wise, a vec <T, A, vl_ptr> is just a pointer, hence the 8 bytes
per block seen in the valgrind report]
gcc/ChangeLog:
PR jit/63854
* tree-ssa-threadedge.c (thread_across_edge): Don't just release
"path", delete it.
* tree-ssa-threadupdate.c (delete_jump_thread_path): Likewise.
---
gcc/tree-ssa-threadedge.c | 1 +
gcc/tree-ssa-threadupdate.c | 1 +
2 files changed, 2 insertions(+)
diff --git a/gcc/tree-ssa-threadedge.c b/gcc/tree-ssa-threadedge.c
index d5b9696..9ee1cae 100644
--- a/gcc/tree-ssa-threadedge.c
+++ b/gcc/tree-ssa-threadedge.c
@@ -1149,6 +1149,7 @@ thread_across_edge (gimple dummy_cond,
through the vector entries. */
gcc_assert (path->length () == 0);
path->release ();
+ delete path;
/* A negative status indicates the target block was deemed too big to
duplicate. Just quit now rather than trying to use the block as
diff --git a/gcc/tree-ssa-threadupdate.c b/gcc/tree-ssa-threadupdate.c
index 151ed83..5cb9f86 100644
--- a/gcc/tree-ssa-threadupdate.c
+++ b/gcc/tree-ssa-threadupdate.c
@@ -2481,6 +2481,7 @@ delete_jump_thread_path (vec<jump_thread_edge *> *path)
for (unsigned int i = 0; i < path->length (); i++)
delete (*path)[i];
path->release();
+ delete path;
}
/* Register a jump threading opportunity. We queue up all the jump
--
1.8.5.3
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH 14/21] PR jit/63854: Fix leak of paths within jump threading
2014-01-01 0:00 ` [PATCH 14/21] PR jit/63854: Fix leak of paths within jump threading David Malcolm
@ 2014-01-01 0:00 ` Richard Biener
0 siblings, 0 replies; 69+ messages in thread
From: Richard Biener @ 2014-01-01 0:00 UTC (permalink / raw)
To: David Malcolm; +Cc: GCC Patches, jit
On Wed, Nov 19, 2014 at 11:46 AM, David Malcolm <dmalcolm@redhat.com> wrote:
> Paths are allocated as:
> vec<jump_thread_edge *> *path = new vec<jump_thread_edge *> ();
> i.e. the vec itself is on the heap, so a mere:
> path->release ();
> is merely releasing the buffer the vec holds, not the vec itself.
Ok.
Thanks,
RIchard.
> This patch updates the two sites that release paths to also delete them,
> fixing leaks like this seen by valgrind:
> 160 bytes in 20 blocks are definitely lost in loss record 165 of 241
> at 0x4A06965: operator new(unsigned long) (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
> by 0x556E5AF: thread_across_edge(gimple_statement_base*, edge_def*, bool, vec<tree_node*, va_heap, vl_ptr>*, tree_node
> * (*)(gimple_statement_base*, gimple_statement_base*)) (tree-ssa-threadedge.c:1124)
> by 0x547B73B: dom_opt_dom_walker::thread_across_edge(edge_def*) (tree-ssa-dom.c:1184)
> by 0x5480183: dom_opt_dom_walker::after_dom_children(basic_block_def*) (tree-ssa-dom.c:2015)
> by 0x5C1C5F7: dom_walker::walk(basic_block_def*) (domwalk.c:218)
> by 0x547AE83: (anonymous namespace)::pass_dominator::execute(function*) (tree-ssa-dom.c:919)
> by 0x52235BD: execute_one_pass(opt_pass*) (passes.c:2306)
> by 0x5223834: execute_pass_list_1(opt_pass*) (passes.c:2358)
> by 0x5223865: execute_pass_list_1(opt_pass*) (passes.c:2359)
> by 0x52238A2: execute_pass_list(function*, opt_pass*) (passes.c:2369)
> by 0x4E4888F: cgraph_node::expand() (cgraphunit.c:1773)
> by 0x4E48F29: expand_all_functions() (cgraphunit.c:1909)
>
> [Space-wise, a vec <T, A, vl_ptr> is just a pointer, hence the 8 bytes
> per block seen in the valgrind report]
>
> gcc/ChangeLog:
> PR jit/63854
> * tree-ssa-threadedge.c (thread_across_edge): Don't just release
> "path", delete it.
> * tree-ssa-threadupdate.c (delete_jump_thread_path): Likewise.
> ---
> gcc/tree-ssa-threadedge.c | 1 +
> gcc/tree-ssa-threadupdate.c | 1 +
> 2 files changed, 2 insertions(+)
>
> diff --git a/gcc/tree-ssa-threadedge.c b/gcc/tree-ssa-threadedge.c
> index d5b9696..9ee1cae 100644
> --- a/gcc/tree-ssa-threadedge.c
> +++ b/gcc/tree-ssa-threadedge.c
> @@ -1149,6 +1149,7 @@ thread_across_edge (gimple dummy_cond,
> through the vector entries. */
> gcc_assert (path->length () == 0);
> path->release ();
> + delete path;
>
> /* A negative status indicates the target block was deemed too big to
> duplicate. Just quit now rather than trying to use the block as
> diff --git a/gcc/tree-ssa-threadupdate.c b/gcc/tree-ssa-threadupdate.c
> index 151ed83..5cb9f86 100644
> --- a/gcc/tree-ssa-threadupdate.c
> +++ b/gcc/tree-ssa-threadupdate.c
> @@ -2481,6 +2481,7 @@ delete_jump_thread_path (vec<jump_thread_edge *> *path)
> for (unsigned int i = 0; i < path->length (); i++)
> delete (*path)[i];
> path->release();
> + delete path;
> }
>
> /* Register a jump threading opportunity. We queue up all the jump
> --
> 1.8.5.3
>
^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH 09/21] PR jit/63854: Don't leak producer_string in dwarf2out.c
2014-01-01 0:00 [PATCH 00/21] PR 63854: Fix various memory leaks David Malcolm
` (13 preceding siblings ...)
2014-01-01 0:00 ` [PATCH 14/21] PR jit/63854: Fix leak of paths within jump threading David Malcolm
@ 2014-01-01 0:00 ` David Malcolm
2014-01-01 0:00 ` Richard Biener
2014-01-01 0:00 ` [PATCH 04/21] PR jit/63854: Fix memory leak within bb-reorder.c David Malcolm
` (5 subsequent siblings)
20 siblings, 1 reply; 69+ messages in thread
From: David Malcolm @ 2014-01-01 0:00 UTC (permalink / raw)
To: gcc-patches, jit; +Cc: David Malcolm
Fix this small per-iteration leak with debuginfo:
424 bytes in 4 blocks are definitely lost in loss record 185 of 230
at 0x4A0645D: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
by 0x5D75CA7: xmalloc (xmalloc.c:147)
by 0x4ECE9E4: gen_producer_string() (dwarf2out.c:19489)
by 0x4EDB2C8: dwarf2out_finish(char const*) (dwarf2out.c:24257)
by 0x532AD3B: compile_file() (toplev.c:623)
by 0x532D9E1: do_compile() (toplev.c:2020)
by 0x532DC54: toplev::main(int, char**) (toplev.c:2117)
by 0x4DE766F: gcc::jit::playback::context::compile() (jit-playback.c:1615)
by 0x4DD76DA: gcc::jit::recording::context::compile() (jit-recording.c:861)
by 0x4DD5BD2: gcc_jit_context_compile (libgccjit.c:2014)
by 0x401CA4: test_jit (harness.h:190)
by 0x401D88: main (harness.h:232)
gcc/ChangeLog:
PR jit/63854
* dwarf2out.c (dwarf2out_c_finalize): Free producer_string.
---
gcc/dwarf2out.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
index b16883f..9069f9a 100644
--- a/gcc/dwarf2out.c
+++ b/gcc/dwarf2out.c
@@ -24741,6 +24741,8 @@ dwarf2out_c_finalize (void)
frame_pointer_fb_offset = 0;
frame_pointer_fb_offset_valid = false;
base_types.release ();
+ XDELETEVEC (producer_string);
+ producer_string = NULL;
}
#include "gt-dwarf2out.h"
--
1.8.5.3
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH 09/21] PR jit/63854: Don't leak producer_string in dwarf2out.c
2014-01-01 0:00 ` [PATCH 09/21] PR jit/63854: Don't leak producer_string in dwarf2out.c David Malcolm
@ 2014-01-01 0:00 ` Richard Biener
0 siblings, 0 replies; 69+ messages in thread
From: Richard Biener @ 2014-01-01 0:00 UTC (permalink / raw)
To: David Malcolm; +Cc: GCC Patches, jit
On Wed, Nov 19, 2014 at 11:46 AM, David Malcolm <dmalcolm@redhat.com> wrote:
> Fix this small per-iteration leak with debuginfo:
Ok.
Thanks,
Richard.
> 424 bytes in 4 blocks are definitely lost in loss record 185 of 230
> at 0x4A0645D: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
> by 0x5D75CA7: xmalloc (xmalloc.c:147)
> by 0x4ECE9E4: gen_producer_string() (dwarf2out.c:19489)
> by 0x4EDB2C8: dwarf2out_finish(char const*) (dwarf2out.c:24257)
> by 0x532AD3B: compile_file() (toplev.c:623)
> by 0x532D9E1: do_compile() (toplev.c:2020)
> by 0x532DC54: toplev::main(int, char**) (toplev.c:2117)
> by 0x4DE766F: gcc::jit::playback::context::compile() (jit-playback.c:1615)
> by 0x4DD76DA: gcc::jit::recording::context::compile() (jit-recording.c:861)
> by 0x4DD5BD2: gcc_jit_context_compile (libgccjit.c:2014)
> by 0x401CA4: test_jit (harness.h:190)
> by 0x401D88: main (harness.h:232)
>
> gcc/ChangeLog:
> PR jit/63854
> * dwarf2out.c (dwarf2out_c_finalize): Free producer_string.
> ---
> gcc/dwarf2out.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
> index b16883f..9069f9a 100644
> --- a/gcc/dwarf2out.c
> +++ b/gcc/dwarf2out.c
> @@ -24741,6 +24741,8 @@ dwarf2out_c_finalize (void)
> frame_pointer_fb_offset = 0;
> frame_pointer_fb_offset_valid = false;
> base_types.release ();
> + XDELETEVEC (producer_string);
> + producer_string = NULL;
> }
>
> #include "gt-dwarf2out.h"
> --
> 1.8.5.3
>
^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH 04/21] PR jit/63854: Fix memory leak within bb-reorder.c
2014-01-01 0:00 [PATCH 00/21] PR 63854: Fix various memory leaks David Malcolm
` (14 preceding siblings ...)
2014-01-01 0:00 ` [PATCH 09/21] PR jit/63854: Don't leak producer_string in dwarf2out.c David Malcolm
@ 2014-01-01 0:00 ` David Malcolm
2014-01-01 0:00 ` Richard Biener
2014-01-01 0:00 ` [PATCH 08/21] PR jit/63854: Add ira_costs_c_finalize David Malcolm
` (4 subsequent siblings)
20 siblings, 1 reply; 69+ messages in thread
From: David Malcolm @ 2014-01-01 0:00 UTC (permalink / raw)
To: gcc-patches, jit; +Cc: David Malcolm
Valgrind showed this leaking 200 bytes per iteration in one of
my testcases:
1,000 bytes in 5 blocks are definitely lost in loss record 200 of 241
at 0x4A083AA: realloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
by 0x5D75C5C: xrealloc (xmalloc.c:179)
by 0x4E10734: void va_heap::reserve<basic_block_def*>(vec<basic_block_def*, va_heap, vl_embed>*&, unsigned int, bool) (vec.h:310
)
by 0x4E105BB: vec<basic_block_def*, va_heap, vl_ptr>::reserve(unsigned int, bool) (vec.h:1428)
by 0x4E15B37: vec<basic_block_def*, va_heap, vl_ptr>::safe_push(basic_block_def* const&) (vec.h:1537)
by 0x5B61F44: find_rarely_executed_basic_blocks_and_crossing_edges() (bb-reorder.c:1614)
by 0x5B63E90: (anonymous namespace)::pass_partition_blocks::execute(function*) (bb-reorder.c:2711)
by 0x522354D: execute_one_pass(opt_pass*) (passes.c:2306)
by 0x52237C4: execute_pass_list_1(opt_pass*) (passes.c:2358)
by 0x52237F5: execute_pass_list_1(opt_pass*) (passes.c:2359)
by 0x5223832: execute_pass_list(function*, opt_pass*) (passes.c:2369)
by 0x4E4884F: cgraph_node::expand() (cgraphunit.c:1773)
Fix is trivial.
gcc/ChangeLog:
PR jit/63854
* bb-reorder.c
(find_rarely_executed_basic_blocks_and_crossing_edges): Convert
local bbs_in_hot_partition from vec<> to auto_vec<>.
---
gcc/bb-reorder.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/gcc/bb-reorder.c b/gcc/bb-reorder.c
index 1f7c3ee..4ca3bb2 100644
--- a/gcc/bb-reorder.c
+++ b/gcc/bb-reorder.c
@@ -1582,7 +1582,7 @@ find_rarely_executed_basic_blocks_and_crossing_edges (void)
edge e;
edge_iterator ei;
unsigned int cold_bb_count = 0;
- vec<basic_block> bbs_in_hot_partition = vNULL;
+ auto_vec<basic_block> bbs_in_hot_partition;
/* Mark which partition (hot/cold) each basic block belongs in. */
FOR_EACH_BB_FN (bb, cfun)
--
1.8.5.3
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH 04/21] PR jit/63854: Fix memory leak within bb-reorder.c
2014-01-01 0:00 ` [PATCH 04/21] PR jit/63854: Fix memory leak within bb-reorder.c David Malcolm
@ 2014-01-01 0:00 ` Richard Biener
0 siblings, 0 replies; 69+ messages in thread
From: Richard Biener @ 2014-01-01 0:00 UTC (permalink / raw)
To: David Malcolm; +Cc: GCC Patches, jit
On Wed, Nov 19, 2014 at 11:46 AM, David Malcolm <dmalcolm@redhat.com> wrote:
> Valgrind showed this leaking 200 bytes per iteration in one of
> my testcases:
Ok.
Thanks,
Richard.
> 1,000 bytes in 5 blocks are definitely lost in loss record 200 of 241
> at 0x4A083AA: realloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
> by 0x5D75C5C: xrealloc (xmalloc.c:179)
> by 0x4E10734: void va_heap::reserve<basic_block_def*>(vec<basic_block_def*, va_heap, vl_embed>*&, unsigned int, bool) (vec.h:310
> )
> by 0x4E105BB: vec<basic_block_def*, va_heap, vl_ptr>::reserve(unsigned int, bool) (vec.h:1428)
> by 0x4E15B37: vec<basic_block_def*, va_heap, vl_ptr>::safe_push(basic_block_def* const&) (vec.h:1537)
> by 0x5B61F44: find_rarely_executed_basic_blocks_and_crossing_edges() (bb-reorder.c:1614)
> by 0x5B63E90: (anonymous namespace)::pass_partition_blocks::execute(function*) (bb-reorder.c:2711)
> by 0x522354D: execute_one_pass(opt_pass*) (passes.c:2306)
> by 0x52237C4: execute_pass_list_1(opt_pass*) (passes.c:2358)
> by 0x52237F5: execute_pass_list_1(opt_pass*) (passes.c:2359)
> by 0x5223832: execute_pass_list(function*, opt_pass*) (passes.c:2369)
> by 0x4E4884F: cgraph_node::expand() (cgraphunit.c:1773)
>
> Fix is trivial.
>
> gcc/ChangeLog:
> PR jit/63854
> * bb-reorder.c
> (find_rarely_executed_basic_blocks_and_crossing_edges): Convert
> local bbs_in_hot_partition from vec<> to auto_vec<>.
> ---
> gcc/bb-reorder.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/gcc/bb-reorder.c b/gcc/bb-reorder.c
> index 1f7c3ee..4ca3bb2 100644
> --- a/gcc/bb-reorder.c
> +++ b/gcc/bb-reorder.c
> @@ -1582,7 +1582,7 @@ find_rarely_executed_basic_blocks_and_crossing_edges (void)
> edge e;
> edge_iterator ei;
> unsigned int cold_bb_count = 0;
> - vec<basic_block> bbs_in_hot_partition = vNULL;
> + auto_vec<basic_block> bbs_in_hot_partition;
>
> /* Mark which partition (hot/cold) each basic block belongs in. */
> FOR_EACH_BB_FN (bb, cfun)
> --
> 1.8.5.3
>
^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH 08/21] PR jit/63854: Add ira_costs_c_finalize
2014-01-01 0:00 [PATCH 00/21] PR 63854: Fix various memory leaks David Malcolm
` (15 preceding siblings ...)
2014-01-01 0:00 ` [PATCH 04/21] PR jit/63854: Fix memory leak within bb-reorder.c David Malcolm
@ 2014-01-01 0:00 ` David Malcolm
2014-01-01 0:00 ` Richard Biener
2014-01-01 0:00 ` [PATCH 07/21] PR jit/63854: Fix leak of optimization_summary_obstack David Malcolm
` (3 subsequent siblings)
20 siblings, 1 reply; 69+ messages in thread
From: David Malcolm @ 2014-01-01 0:00 UTC (permalink / raw)
To: gcc-patches, jit; +Cc: David Malcolm
this_target_ira_int->free_ira_costs () is called by ira_init_costs,
but this isn't called after the compile, causing noise when running
under valgrind.
This patch adds a ira_costs_c_finalize function to clean this up,
and calls it from toplev::finalize (and hence this isn't called by
cc1/cc1plus/etc, just by libgccjit.so).
Doing so eliminates about 27KB of "leak" from the valgrind report.
gcc/ChangeLog:
PR jit/63854
* ira-costs.c (ira_costs_c_finalize): New function.
* ira.h (ira_costs_c_finalize): New prototype.
* toplev.c (toplev::finalize): Call ira_costs_c_finalize.
---
gcc/ira-costs.c | 6 ++++++
gcc/ira.h | 3 +++
gcc/toplev.c | 1 +
3 files changed, 10 insertions(+)
diff --git a/gcc/ira-costs.c b/gcc/ira-costs.c
index 122815b..2dabead 100644
--- a/gcc/ira-costs.c
+++ b/gcc/ira-costs.c
@@ -2356,3 +2356,9 @@ ira_adjust_equiv_reg_cost (unsigned regno, int cost)
else
regno_equiv_gains[regno] += cost;
}
+
+void
+ira_costs_c_finalize (void)
+{
+ this_target_ira_int->free_ira_costs ();
+}
diff --git a/gcc/ira.h b/gcc/ira.h
index b294ea1..d62656c 100644
--- a/gcc/ira.h
+++ b/gcc/ira.h
@@ -199,4 +199,7 @@ extern bool ira_bad_reload_regno (int, rtx, rtx);
extern void ira_adjust_equiv_reg_cost (unsigned, int);
+/* ira-costs.c */
+extern void ira_costs_c_finalize (void);
+
#endif /* GCC_IRA_H */
diff --git a/gcc/toplev.c b/gcc/toplev.c
index 0d617c2..0181a3f 100644
--- a/gcc/toplev.c
+++ b/gcc/toplev.c
@@ -2168,6 +2168,7 @@ toplev::finalize (void)
gcse_c_finalize ();
ipa_cp_c_finalize ();
ipa_reference_c_finalize ();
+ ira_costs_c_finalize ();
params_c_finalize ();
finalize_options_struct (&global_options);
--
1.8.5.3
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH 08/21] PR jit/63854: Add ira_costs_c_finalize
2014-01-01 0:00 ` [PATCH 08/21] PR jit/63854: Add ira_costs_c_finalize David Malcolm
@ 2014-01-01 0:00 ` Richard Biener
0 siblings, 0 replies; 69+ messages in thread
From: Richard Biener @ 2014-01-01 0:00 UTC (permalink / raw)
To: David Malcolm; +Cc: GCC Patches, jit
On Wed, Nov 19, 2014 at 11:46 AM, David Malcolm <dmalcolm@redhat.com> wrote:
> this_target_ira_int->free_ira_costs () is called by ira_init_costs,
> but this isn't called after the compile, causing noise when running
> under valgrind.
>
> This patch adds a ira_costs_c_finalize function to clean this up,
> and calls it from toplev::finalize (and hence this isn't called by
> cc1/cc1plus/etc, just by libgccjit.so).
>
> Doing so eliminates about 27KB of "leak" from the valgrind report.
Ok.
Thanks,
Richard.
> gcc/ChangeLog:
> PR jit/63854
> * ira-costs.c (ira_costs_c_finalize): New function.
> * ira.h (ira_costs_c_finalize): New prototype.
> * toplev.c (toplev::finalize): Call ira_costs_c_finalize.
> ---
> gcc/ira-costs.c | 6 ++++++
> gcc/ira.h | 3 +++
> gcc/toplev.c | 1 +
> 3 files changed, 10 insertions(+)
>
> diff --git a/gcc/ira-costs.c b/gcc/ira-costs.c
> index 122815b..2dabead 100644
> --- a/gcc/ira-costs.c
> +++ b/gcc/ira-costs.c
> @@ -2356,3 +2356,9 @@ ira_adjust_equiv_reg_cost (unsigned regno, int cost)
> else
> regno_equiv_gains[regno] += cost;
> }
> +
> +void
> +ira_costs_c_finalize (void)
> +{
> + this_target_ira_int->free_ira_costs ();
> +}
> diff --git a/gcc/ira.h b/gcc/ira.h
> index b294ea1..d62656c 100644
> --- a/gcc/ira.h
> +++ b/gcc/ira.h
> @@ -199,4 +199,7 @@ extern bool ira_bad_reload_regno (int, rtx, rtx);
>
> extern void ira_adjust_equiv_reg_cost (unsigned, int);
>
> +/* ira-costs.c */
> +extern void ira_costs_c_finalize (void);
> +
> #endif /* GCC_IRA_H */
> diff --git a/gcc/toplev.c b/gcc/toplev.c
> index 0d617c2..0181a3f 100644
> --- a/gcc/toplev.c
> +++ b/gcc/toplev.c
> @@ -2168,6 +2168,7 @@ toplev::finalize (void)
> gcse_c_finalize ();
> ipa_cp_c_finalize ();
> ipa_reference_c_finalize ();
> + ira_costs_c_finalize ();
> params_c_finalize ();
>
> finalize_options_struct (&global_options);
> --
> 1.8.5.3
>
^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH 07/21] PR jit/63854: Fix leak of optimization_summary_obstack
2014-01-01 0:00 [PATCH 00/21] PR 63854: Fix various memory leaks David Malcolm
` (16 preceding siblings ...)
2014-01-01 0:00 ` [PATCH 08/21] PR jit/63854: Add ira_costs_c_finalize David Malcolm
@ 2014-01-01 0:00 ` David Malcolm
2014-01-01 0:00 ` Richard Biener
2014-01-01 0:00 ` [PATCH 02/21] PR jit/63854: Fix memory leak of reginfo.c: valid_mode_changes_obstack David Malcolm
` (2 subsequent siblings)
20 siblings, 1 reply; 69+ messages in thread
From: David Malcolm @ 2014-01-01 0:00 UTC (permalink / raw)
To: gcc-patches, jit; +Cc: David Malcolm
This was leaking ~4KB per iteration:
16,256 bytes in 4 blocks are definitely lost in loss record 234 of 239
at 0x4A0645D: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
by 0x5D75C17: xmalloc (xmalloc.c:147)
by 0x30958842DB: _obstack_begin (obstack.c:184)
by 0x4DFECDE: bitmap_obstack_initialize(bitmap_obstack*) (bitmap.c:338)
by 0x5CA3C1D: ipa_init() (ipa-reference.c:431)
by 0x5CA3FB1: generate_summary() (ipa-reference.c:551)
by 0x5CA4650: propagate() (ipa-reference.c:684)
by 0x5CA5BEE: (anonymous namespace)::pass_ipa_reference::execute(function*) (ipa-reference.c:1178)
by 0x522354D: execute_one_pass(opt_pass*) (passes.c:2306)
by 0x5224427: execute_ipa_pass_list(opt_pass*) (passes.c:2700)
by 0x4E495C1: ipa_passes() (cgraphunit.c:2088)
by 0x4E498E0: symbol_table::compile() (cgraphunit.c:2172)
gcc/ChangeLog:
PR jit/63854
* ipa-reference.c (ipa_reference_c_finalize): Release
optimization_summary_obstack.
---
gcc/ipa-reference.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/gcc/ipa-reference.c b/gcc/ipa-reference.c
index b421f63..1ce06d1 100644
--- a/gcc/ipa-reference.c
+++ b/gcc/ipa-reference.c
@@ -1193,5 +1193,9 @@ make_pass_ipa_reference (gcc::context *ctxt)
void
ipa_reference_c_finalize (void)
{
- ipa_init_p = false;
+ if (ipa_init_p)
+ {
+ bitmap_obstack_release (&optimization_summary_obstack);
+ ipa_init_p = false;
+ }
}
--
1.8.5.3
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH 07/21] PR jit/63854: Fix leak of optimization_summary_obstack
2014-01-01 0:00 ` [PATCH 07/21] PR jit/63854: Fix leak of optimization_summary_obstack David Malcolm
@ 2014-01-01 0:00 ` Richard Biener
0 siblings, 0 replies; 69+ messages in thread
From: Richard Biener @ 2014-01-01 0:00 UTC (permalink / raw)
To: David Malcolm; +Cc: GCC Patches, jit
On Wed, Nov 19, 2014 at 11:46 AM, David Malcolm <dmalcolm@redhat.com> wrote:
> This was leaking ~4KB per iteration:
Ok.
Thanks,
Richard.
> 16,256 bytes in 4 blocks are definitely lost in loss record 234 of 239
> at 0x4A0645D: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
> by 0x5D75C17: xmalloc (xmalloc.c:147)
> by 0x30958842DB: _obstack_begin (obstack.c:184)
> by 0x4DFECDE: bitmap_obstack_initialize(bitmap_obstack*) (bitmap.c:338)
> by 0x5CA3C1D: ipa_init() (ipa-reference.c:431)
> by 0x5CA3FB1: generate_summary() (ipa-reference.c:551)
> by 0x5CA4650: propagate() (ipa-reference.c:684)
> by 0x5CA5BEE: (anonymous namespace)::pass_ipa_reference::execute(function*) (ipa-reference.c:1178)
> by 0x522354D: execute_one_pass(opt_pass*) (passes.c:2306)
> by 0x5224427: execute_ipa_pass_list(opt_pass*) (passes.c:2700)
> by 0x4E495C1: ipa_passes() (cgraphunit.c:2088)
> by 0x4E498E0: symbol_table::compile() (cgraphunit.c:2172)
>
> gcc/ChangeLog:
> PR jit/63854
> * ipa-reference.c (ipa_reference_c_finalize): Release
> optimization_summary_obstack.
> ---
> gcc/ipa-reference.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/gcc/ipa-reference.c b/gcc/ipa-reference.c
> index b421f63..1ce06d1 100644
> --- a/gcc/ipa-reference.c
> +++ b/gcc/ipa-reference.c
> @@ -1193,5 +1193,9 @@ make_pass_ipa_reference (gcc::context *ctxt)
> void
> ipa_reference_c_finalize (void)
> {
> - ipa_init_p = false;
> + if (ipa_init_p)
> + {
> + bitmap_obstack_release (&optimization_summary_obstack);
> + ipa_init_p = false;
> + }
> }
> --
> 1.8.5.3
>
^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH 02/21] PR jit/63854: Fix memory leak of reginfo.c: valid_mode_changes_obstack
2014-01-01 0:00 [PATCH 00/21] PR 63854: Fix various memory leaks David Malcolm
` (17 preceding siblings ...)
2014-01-01 0:00 ` [PATCH 07/21] PR jit/63854: Fix leak of optimization_summary_obstack David Malcolm
@ 2014-01-01 0:00 ` David Malcolm
2014-01-01 0:00 ` Richard Biener
2014-01-01 0:00 ` [PATCH 19/21] PR jit/63854: Fix leak of ipa hooks David Malcolm
2014-01-01 0:00 ` [PATCH 05/21] PR jit/63854: Fix memory leak of save_decoded_options David Malcolm
20 siblings, 1 reply; 69+ messages in thread
From: David Malcolm @ 2014-01-01 0:00 UTC (permalink / raw)
To: gcc-patches, jit; +Cc: David Malcolm
Valgrind shows this fixes ~4 KB of leak per iteration (on x86_64) by
plugging this leak allocated at reginfo.c:1327:
gcc_obstack_init (&valid_mode_changes_obstack);
==57820== 16,256 bytes in 4 blocks are definitely lost in loss record 906 of 917
==57820== at 0x4A0645D: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==57820== by 0x59A6747: xmalloc (xmalloc.c:147)
==57820== by 0x30958842DB: _obstack_begin (obstack.c:184)
==57820== by 0x51C1EC1: init_subregs_of_mode() (reginfo.c:1327)
==57820== by 0x50D2A38: init_costs() (ira-costs.c:2181)
==57820== by 0x50D74A8: ira_costs() (ira-costs.c:2211)
==57820== by 0x50D1326: ira_build() (ira-build.c:3459)
==57820== by 0x50C909C: (anonymous namespace)::pass_ira::execute(function*) (ira.c:5227)
==57820== by 0x51884F7: execute_one_pass(opt_pass*) (passes.c:2269)
==57820== by 0x5188B75: execute_pass_list_1(opt_pass*) (passes.c:2321)
==57820== by 0x5188B87: execute_pass_list_1(opt_pass*) (passes.c:2322)
==57820== by 0x5188BC8: execute_pass_list(function*, opt_pass*) (passes.c:2332)
gcc/ChangeLog:
PR jit/63854
* reginfo.c (finish_subregs_of_mode): Replace obstack_finish with
obstack_free when cleaning up valid_mode_changes_obstack.
---
gcc/reginfo.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/gcc/reginfo.c b/gcc/reginfo.c
index efe23cd..c2daf22 100644
--- a/gcc/reginfo.c
+++ b/gcc/reginfo.c
@@ -1343,7 +1343,7 @@ void
finish_subregs_of_mode (void)
{
XDELETEVEC (valid_mode_changes);
- obstack_finish (&valid_mode_changes_obstack);
+ obstack_free (&valid_mode_changes_obstack, NULL);
}
/* Free all data attached to the structure. This isn't a destructor because
--
1.8.5.3
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH 02/21] PR jit/63854: Fix memory leak of reginfo.c: valid_mode_changes_obstack
2014-01-01 0:00 ` [PATCH 02/21] PR jit/63854: Fix memory leak of reginfo.c: valid_mode_changes_obstack David Malcolm
@ 2014-01-01 0:00 ` Richard Biener
0 siblings, 0 replies; 69+ messages in thread
From: Richard Biener @ 2014-01-01 0:00 UTC (permalink / raw)
To: David Malcolm; +Cc: GCC Patches, jit
On Wed, Nov 19, 2014 at 11:46 AM, David Malcolm <dmalcolm@redhat.com> wrote:
> Valgrind shows this fixes ~4 KB of leak per iteration (on x86_64) by
> plugging this leak allocated at reginfo.c:1327:
> gcc_obstack_init (&valid_mode_changes_obstack);
Ok.
Thanks,
Richard.
> ==57820== 16,256 bytes in 4 blocks are definitely lost in loss record 906 of 917
> ==57820== at 0x4A0645D: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==57820== by 0x59A6747: xmalloc (xmalloc.c:147)
> ==57820== by 0x30958842DB: _obstack_begin (obstack.c:184)
> ==57820== by 0x51C1EC1: init_subregs_of_mode() (reginfo.c:1327)
> ==57820== by 0x50D2A38: init_costs() (ira-costs.c:2181)
> ==57820== by 0x50D74A8: ira_costs() (ira-costs.c:2211)
> ==57820== by 0x50D1326: ira_build() (ira-build.c:3459)
> ==57820== by 0x50C909C: (anonymous namespace)::pass_ira::execute(function*) (ira.c:5227)
> ==57820== by 0x51884F7: execute_one_pass(opt_pass*) (passes.c:2269)
> ==57820== by 0x5188B75: execute_pass_list_1(opt_pass*) (passes.c:2321)
> ==57820== by 0x5188B87: execute_pass_list_1(opt_pass*) (passes.c:2322)
> ==57820== by 0x5188BC8: execute_pass_list(function*, opt_pass*) (passes.c:2332)
>
> gcc/ChangeLog:
> PR jit/63854
> * reginfo.c (finish_subregs_of_mode): Replace obstack_finish with
> obstack_free when cleaning up valid_mode_changes_obstack.
> ---
> gcc/reginfo.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/gcc/reginfo.c b/gcc/reginfo.c
> index efe23cd..c2daf22 100644
> --- a/gcc/reginfo.c
> +++ b/gcc/reginfo.c
> @@ -1343,7 +1343,7 @@ void
> finish_subregs_of_mode (void)
> {
> XDELETEVEC (valid_mode_changes);
> - obstack_finish (&valid_mode_changes_obstack);
> + obstack_free (&valid_mode_changes_obstack, NULL);
> }
>
> /* Free all data attached to the structure. This isn't a destructor because
> --
> 1.8.5.3
>
^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH 19/21] PR jit/63854: Fix leak of ipa hooks
2014-01-01 0:00 [PATCH 00/21] PR 63854: Fix various memory leaks David Malcolm
` (18 preceding siblings ...)
2014-01-01 0:00 ` [PATCH 02/21] PR jit/63854: Fix memory leak of reginfo.c: valid_mode_changes_obstack David Malcolm
@ 2014-01-01 0:00 ` David Malcolm
2014-01-01 0:00 ` Jeff Law
2014-01-01 0:00 ` [PATCH 05/21] PR jit/63854: Fix memory leak of save_decoded_options David Malcolm
20 siblings, 1 reply; 69+ messages in thread
From: David Malcolm @ 2014-01-01 0:00 UTC (permalink / raw)
To: gcc-patches, jit; +Cc: David Malcolm
This fixes three leaks in IPA seen in jit testcases with valgrind:
This one:
96 bytes in 4 blocks are definitely lost in loss record 102 of 205
at 0x4A0645D: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
by 0x5D76447: xmalloc (xmalloc.c:147)
by 0x4E35C23: symbol_table::add_cgraph_insertion_hook(void (*)(cgraph_node*, void*), void*) (cgraph.c:383)
by 0x51070C6: ipa_register_cgraph_hooks() (ipa-prop.c:3864)
by 0x5C753D8: ipcp_generate_summary() (ipa-cp.c:3786)
by 0x5223540: execute_ipa_summary_passes(ipa_opt_pass_d*) (passes.c:2102)
by 0x4E49A60: ipa_passes() (cgraphunit.c:2074)
by 0x4E49E2C: symbol_table::compile() (cgraphunit.c:2172)
by 0x4E4A1C2: symbol_table::finalize_compilation_unit() (cgraphunit.c:2325)
by 0x4DC999C: jit_langhook_write_globals() (dummy-frontend.c:216)
by 0x532B3A6: compile_file() (toplev.c:583)
by 0x532E15F: do_compile() (toplev.c:2020)
appears to be caused by
ipa-prop.c (ipa_register_cgraph_hooks)
unconditionally inserting ipa_add_new_function as
"function_insertion_hook_holder", rather than if the latter is
non-NULL, like the other hooks.
These two in ipa-reference.c:
96 bytes in 4 blocks are definitely lost in loss record 103 of 205
at 0x4A0645D: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
by 0x5D76447: xmalloc (xmalloc.c:147)
by 0x4E35AA9: symbol_table::add_cgraph_removal_hook(void (*)(cgraph_node*, void*), void*) (cgraph.c:329)
by 0x5CA446E: ipa_init() (ipa-reference.c:435)
by 0x5CA47D1: generate_summary() (ipa-reference.c:551)
by 0x5CA4E70: propagate() (ipa-reference.c:684)
by 0x5CA640E: (anonymous namespace)::pass_ipa_reference::execute(function*) (ipa-reference.c:1178)
by 0x5223CC1: execute_one_pass(opt_pass*) (passes.c:2306)
by 0x5224B9B: execute_ipa_pass_list(opt_pass*) (passes.c:2700)
by 0x4E49B0D: ipa_passes() (cgraphunit.c:2088)
by 0x4E49E2C: symbol_table::compile() (cgraphunit.c:2172)
by 0x4E4A1C2: symbol_table::finalize_compilation_unit() (cgraphunit.c:2325)
96 bytes in 4 blocks are definitely lost in loss record 104 of 205
at 0x4A0645D: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
by 0x5D76447: xmalloc (xmalloc.c:147)
by 0x4E35E29: symbol_table::add_cgraph_duplication_hook(void (*)(cgraph_node*, cgraph_node*, void*), void*) (cgraph.c:453)
by 0x5CA4493: ipa_init() (ipa-reference.c:437)
by 0x5CA47D1: generate_summary() (ipa-reference.c:551)
by 0x5CA4E70: propagate() (ipa-reference.c:684)
by 0x5CA640E: (anonymous namespace)::pass_ipa_reference::execute(function*) (ipa-reference.c:1178)
by 0x5223CC1: execute_one_pass(opt_pass*) (passes.c:2306)
by 0x5224B9B: execute_ipa_pass_list(opt_pass*) (passes.c:2700)
by 0x4E49B0D: ipa_passes() (cgraphunit.c:2088)
by 0x4E49E2C: symbol_table::compile() (cgraphunit.c:2172)
by 0x4E4A1C2: symbol_table::finalize_compilation_unit() (cgraphunit.c:2325)
appear to be due to th hooks never being removed.
My patch hacks in a removal of them into ipa_reference_c_finalize, but
I suspect there's a cleaner place to put this.
gcc/ChangeLog:
PR jit/63854
* ipa-prop.c (ipa_register_cgraph_hooks): Guard insertion of
ipa_add_new_function on function_insertion_hook_holder being
non-NULL.
* ipa-reference.c (ipa_reference_c_finalize): Remove
node_removal_hook_holder and node_duplication_hook_holder if
they've been added to symtab.
* toplev.c (toplev::finalize): Call ipa_reference_c_finalize
before cgraph_c_finalize so that the former can access "symtab".
---
gcc/ipa-prop.c | 3 ++-
gcc/ipa-reference.c | 11 +++++++++++
gcc/toplev.c | 4 +++-
3 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c
index f87243c..a8238ac 100644
--- a/gcc/ipa-prop.c
+++ b/gcc/ipa-prop.c
@@ -3552,7 +3552,8 @@ ipa_register_cgraph_hooks (void)
if (!node_duplication_hook_holder)
node_duplication_hook_holder =
symtab->add_cgraph_duplication_hook (&ipa_node_duplication_hook, NULL);
- function_insertion_hook_holder =
+ if (!function_insertion_hook_holder)
+ function_insertion_hook_holder =
symtab->add_cgraph_insertion_hook (&ipa_add_new_function, NULL);
}
diff --git a/gcc/ipa-reference.c b/gcc/ipa-reference.c
index 1ce06d1..b046f9e 100644
--- a/gcc/ipa-reference.c
+++ b/gcc/ipa-reference.c
@@ -1198,4 +1198,15 @@ ipa_reference_c_finalize (void)
bitmap_obstack_release (&optimization_summary_obstack);
ipa_init_p = false;
}
+
+ if (node_removal_hook_holder)
+ {
+ symtab->remove_cgraph_removal_hook (node_removal_hook_holder);
+ node_removal_hook_holder = NULL;
+ }
+ if (node_duplication_hook_holder)
+ {
+ symtab->remove_cgraph_duplication_hook (node_duplication_hook_holder);
+ node_duplication_hook_holder = NULL;
+ }
}
diff --git a/gcc/toplev.c b/gcc/toplev.c
index c0c418c..96e7af9 100644
--- a/gcc/toplev.c
+++ b/gcc/toplev.c
@@ -2162,12 +2162,14 @@ toplev::finalize (void)
rtl_initialized = false;
this_target_rtl->target_specific_initialized = false;
+ /* Needs to be called before cgraph_c_finalize since it uses symtab. */
+ ipa_reference_c_finalize ();
+
cgraph_c_finalize ();
cgraphunit_c_finalize ();
dwarf2out_c_finalize ();
gcse_c_finalize ();
ipa_cp_c_finalize ();
- ipa_reference_c_finalize ();
ira_costs_c_finalize ();
params_c_finalize ();
--
1.8.5.3
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH 19/21] PR jit/63854: Fix leak of ipa hooks
2014-01-01 0:00 ` [PATCH 19/21] PR jit/63854: Fix leak of ipa hooks David Malcolm
@ 2014-01-01 0:00 ` Jeff Law
2014-01-01 0:00 ` PING " David Malcolm
0 siblings, 1 reply; 69+ messages in thread
From: Jeff Law @ 2014-01-01 0:00 UTC (permalink / raw)
To: David Malcolm, gcc-patches, jit
On 11/19/14 03:46, David Malcolm wrote:
> This fixes three leaks in IPA seen in jit testcases with valgrind:
>
> This one:
> 96 bytes in 4 blocks are definitely lost in loss record 102 of 205
> at 0x4A0645D: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
> by 0x5D76447: xmalloc (xmalloc.c:147)
> by 0x4E35C23: symbol_table::add_cgraph_insertion_hook(void (*)(cgraph_node*, void*), void*) (cgraph.c:383)
> by 0x51070C6: ipa_register_cgraph_hooks() (ipa-prop.c:3864)
> by 0x5C753D8: ipcp_generate_summary() (ipa-cp.c:3786)
> by 0x5223540: execute_ipa_summary_passes(ipa_opt_pass_d*) (passes.c:2102)
> by 0x4E49A60: ipa_passes() (cgraphunit.c:2074)
> by 0x4E49E2C: symbol_table::compile() (cgraphunit.c:2172)
> by 0x4E4A1C2: symbol_table::finalize_compilation_unit() (cgraphunit.c:2325)
> by 0x4DC999C: jit_langhook_write_globals() (dummy-frontend.c:216)
> by 0x532B3A6: compile_file() (toplev.c:583)
> by 0x532E15F: do_compile() (toplev.c:2020)
>
> appears to be caused by
> ipa-prop.c (ipa_register_cgraph_hooks)
> unconditionally inserting ipa_add_new_function as
> "function_insertion_hook_holder", rather than if the latter is
> non-NULL, like the other hooks.
>
> These two in ipa-reference.c:
> 96 bytes in 4 blocks are definitely lost in loss record 103 of 205
> at 0x4A0645D: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
> by 0x5D76447: xmalloc (xmalloc.c:147)
> by 0x4E35AA9: symbol_table::add_cgraph_removal_hook(void (*)(cgraph_node*, void*), void*) (cgraph.c:329)
> by 0x5CA446E: ipa_init() (ipa-reference.c:435)
> by 0x5CA47D1: generate_summary() (ipa-reference.c:551)
> by 0x5CA4E70: propagate() (ipa-reference.c:684)
> by 0x5CA640E: (anonymous namespace)::pass_ipa_reference::execute(function*) (ipa-reference.c:1178)
> by 0x5223CC1: execute_one_pass(opt_pass*) (passes.c:2306)
> by 0x5224B9B: execute_ipa_pass_list(opt_pass*) (passes.c:2700)
> by 0x4E49B0D: ipa_passes() (cgraphunit.c:2088)
> by 0x4E49E2C: symbol_table::compile() (cgraphunit.c:2172)
> by 0x4E4A1C2: symbol_table::finalize_compilation_unit() (cgraphunit.c:2325)
>
> 96 bytes in 4 blocks are definitely lost in loss record 104 of 205
> at 0x4A0645D: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
> by 0x5D76447: xmalloc (xmalloc.c:147)
> by 0x4E35E29: symbol_table::add_cgraph_duplication_hook(void (*)(cgraph_node*, cgraph_node*, void*), void*) (cgraph.c:453)
> by 0x5CA4493: ipa_init() (ipa-reference.c:437)
> by 0x5CA47D1: generate_summary() (ipa-reference.c:551)
> by 0x5CA4E70: propagate() (ipa-reference.c:684)
> by 0x5CA640E: (anonymous namespace)::pass_ipa_reference::execute(function*) (ipa-reference.c:1178)
> by 0x5223CC1: execute_one_pass(opt_pass*) (passes.c:2306)
> by 0x5224B9B: execute_ipa_pass_list(opt_pass*) (passes.c:2700)
> by 0x4E49B0D: ipa_passes() (cgraphunit.c:2088)
> by 0x4E49E2C: symbol_table::compile() (cgraphunit.c:2172)
> by 0x4E4A1C2: symbol_table::finalize_compilation_unit() (cgraphunit.c:2325)
>
> appear to be due to th hooks never being removed.
>
> My patch hacks in a removal of them into ipa_reference_c_finalize, but
> I suspect there's a cleaner place to put this.
>
> gcc/ChangeLog:
> PR jit/63854
> * ipa-prop.c (ipa_register_cgraph_hooks): Guard insertion of
> ipa_add_new_function on function_insertion_hook_holder being
> non-NULL.
> * ipa-reference.c (ipa_reference_c_finalize): Remove
> node_removal_hook_holder and node_duplication_hook_holder if
> they've been added to symtab.
> * toplev.c (toplev::finalize): Call ipa_reference_c_finalize
> before cgraph_c_finalize so that the former can access "symtab".
I'm going to let Jan own this one.
jeff
^ permalink raw reply [flat|nested] 69+ messages in thread
* PING Re: [PATCH 19/21] PR jit/63854: Fix leak of ipa hooks
2014-01-01 0:00 ` Jeff Law
@ 2014-01-01 0:00 ` David Malcolm
2014-01-01 0:00 ` Jan Hubicka
0 siblings, 1 reply; 69+ messages in thread
From: David Malcolm @ 2014-01-01 0:00 UTC (permalink / raw)
To: Jan Hubicka; +Cc: gcc-patches, jit, Jeff Law
On Wed, 2014-11-19 at 12:58 -0700, Jeff Law wrote:
> On 11/19/14 03:46, David Malcolm wrote:
> > This fixes three leaks in IPA seen in jit testcases with valgrind:
> >
> > This one:
> > 96 bytes in 4 blocks are definitely lost in loss record 102 of 205
> > at 0x4A0645D: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
> > by 0x5D76447: xmalloc (xmalloc.c:147)
> > by 0x4E35C23: symbol_table::add_cgraph_insertion_hook(void (*)(cgraph_node*, void*), void*) (cgraph.c:383)
> > by 0x51070C6: ipa_register_cgraph_hooks() (ipa-prop.c:3864)
> > by 0x5C753D8: ipcp_generate_summary() (ipa-cp.c:3786)
> > by 0x5223540: execute_ipa_summary_passes(ipa_opt_pass_d*) (passes.c:2102)
> > by 0x4E49A60: ipa_passes() (cgraphunit.c:2074)
> > by 0x4E49E2C: symbol_table::compile() (cgraphunit.c:2172)
> > by 0x4E4A1C2: symbol_table::finalize_compilation_unit() (cgraphunit.c:2325)
> > by 0x4DC999C: jit_langhook_write_globals() (dummy-frontend.c:216)
> > by 0x532B3A6: compile_file() (toplev.c:583)
> > by 0x532E15F: do_compile() (toplev.c:2020)
> >
> > appears to be caused by
> > ipa-prop.c (ipa_register_cgraph_hooks)
> > unconditionally inserting ipa_add_new_function as
> > "function_insertion_hook_holder", rather than if the latter is
> > non-NULL, like the other hooks.
> >
> > These two in ipa-reference.c:
> > 96 bytes in 4 blocks are definitely lost in loss record 103 of 205
> > at 0x4A0645D: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
> > by 0x5D76447: xmalloc (xmalloc.c:147)
> > by 0x4E35AA9: symbol_table::add_cgraph_removal_hook(void (*)(cgraph_node*, void*), void*) (cgraph.c:329)
> > by 0x5CA446E: ipa_init() (ipa-reference.c:435)
> > by 0x5CA47D1: generate_summary() (ipa-reference.c:551)
> > by 0x5CA4E70: propagate() (ipa-reference.c:684)
> > by 0x5CA640E: (anonymous namespace)::pass_ipa_reference::execute(function*) (ipa-reference.c:1178)
> > by 0x5223CC1: execute_one_pass(opt_pass*) (passes.c:2306)
> > by 0x5224B9B: execute_ipa_pass_list(opt_pass*) (passes.c:2700)
> > by 0x4E49B0D: ipa_passes() (cgraphunit.c:2088)
> > by 0x4E49E2C: symbol_table::compile() (cgraphunit.c:2172)
> > by 0x4E4A1C2: symbol_table::finalize_compilation_unit() (cgraphunit.c:2325)
> >
> > 96 bytes in 4 blocks are definitely lost in loss record 104 of 205
> > at 0x4A0645D: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
> > by 0x5D76447: xmalloc (xmalloc.c:147)
> > by 0x4E35E29: symbol_table::add_cgraph_duplication_hook(void (*)(cgraph_node*, cgraph_node*, void*), void*) (cgraph.c:453)
> > by 0x5CA4493: ipa_init() (ipa-reference.c:437)
> > by 0x5CA47D1: generate_summary() (ipa-reference.c:551)
> > by 0x5CA4E70: propagate() (ipa-reference.c:684)
> > by 0x5CA640E: (anonymous namespace)::pass_ipa_reference::execute(function*) (ipa-reference.c:1178)
> > by 0x5223CC1: execute_one_pass(opt_pass*) (passes.c:2306)
> > by 0x5224B9B: execute_ipa_pass_list(opt_pass*) (passes.c:2700)
> > by 0x4E49B0D: ipa_passes() (cgraphunit.c:2088)
> > by 0x4E49E2C: symbol_table::compile() (cgraphunit.c:2172)
> > by 0x4E4A1C2: symbol_table::finalize_compilation_unit() (cgraphunit.c:2325)
> >
> > appear to be due to th hooks never being removed.
> >
> > My patch hacks in a removal of them into ipa_reference_c_finalize, but
> > I suspect there's a cleaner place to put this.
> >
> > gcc/ChangeLog:
> > PR jit/63854
> > * ipa-prop.c (ipa_register_cgraph_hooks): Guard insertion of
> > ipa_add_new_function on function_insertion_hook_holder being
> > non-NULL.
> > * ipa-reference.c (ipa_reference_c_finalize): Remove
> > node_removal_hook_holder and node_duplication_hook_holder if
> > they've been added to symtab.
> > * toplev.c (toplev::finalize): Call ipa_reference_c_finalize
> > before cgraph_c_finalize so that the former can access "symtab".
> I'm going to let Jan own this one.
>
> jeff
Jan: please can you have a look at this one; patch can be seen at:
https://gcc.gnu.org/ml/gcc-patches/2014-11/msg02415.html
Thanks
Dave
^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: PING Re: [PATCH 19/21] PR jit/63854: Fix leak of ipa hooks
2014-01-01 0:00 ` PING " David Malcolm
@ 2014-01-01 0:00 ` Jan Hubicka
0 siblings, 0 replies; 69+ messages in thread
From: Jan Hubicka @ 2014-01-01 0:00 UTC (permalink / raw)
To: David Malcolm; +Cc: Jan Hubicka, gcc-patches, jit, Jeff Law
> > > My patch hacks in a removal of them into ipa_reference_c_finalize, but
> > > I suspect there's a cleaner place to put this.
> > >
> > > gcc/ChangeLog:
> > > PR jit/63854
> > > * ipa-prop.c (ipa_register_cgraph_hooks): Guard insertion of
> > > ipa_add_new_function on function_insertion_hook_holder being
> > > non-NULL.
> > > * ipa-reference.c (ipa_reference_c_finalize): Remove
> > > node_removal_hook_holder and node_duplication_hook_holder if
> > > they've been added to symtab.
> > > * toplev.c (toplev::finalize): Call ipa_reference_c_finalize
> > > before cgraph_c_finalize so that the former can access "symtab".
> > I'm going to let Jan own this one.
> >
> > jeff
>
> Jan: please can you have a look at this one; patch can be seen at:
> https://gcc.gnu.org/ml/gcc-patches/2014-11/msg02415.html
Patch is OK,
thanks!
I hope some of this mess will go away soon with Martin's summary reorg.
Honza
>
>
> Thanks
> Dave
^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH 05/21] PR jit/63854: Fix memory leak of save_decoded_options
2014-01-01 0:00 [PATCH 00/21] PR 63854: Fix various memory leaks David Malcolm
` (19 preceding siblings ...)
2014-01-01 0:00 ` [PATCH 19/21] PR jit/63854: Fix leak of ipa hooks David Malcolm
@ 2014-01-01 0:00 ` David Malcolm
2014-01-01 0:00 ` Richard Biener
20 siblings, 1 reply; 69+ messages in thread
From: David Malcolm @ 2014-01-01 0:00 UTC (permalink / raw)
To: gcc-patches, jit; +Cc: David Malcolm
This commit fixes this leak from opts-common.c, about 4KB per iteration.
==57820== 18,816 (2,560 direct, 16,256 indirect) bytes in 4 blocks are definitely lost in loss record 907 of 917
==57820== at 0x4A083AA: realloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==57820== by 0x59A67CC: xrealloc (xmalloc.c:179)
==57820== by 0x59587C9: decode_cmdline_options_to_array(unsigned int, char const**, unsigned int, cl_decoded_option**, unsigned int*) (opts-common.c:885)
==57820== by 0x4E2ED90: toplev::main(int, char**) (toplev.c:2089)
==57820== by 0x4E43186: gcc::jit::playback::context::compile() (jit-playback.c:1615)
==57820== by 0x4E4018D: gcc::jit::recording::context::compile() (jit-recording.c:861)
==57820== by 0x401CA4: test_jit (harness.h:190)
==57820== by 0x401D88: main (harness.h:232)
gcc/ChangeLog:
PR jit/63854
* toplev.c (toplev::finalize): Clean up save_decoded_options.
---
gcc/toplev.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/gcc/toplev.c b/gcc/toplev.c
index 876279f..291b84d 100644
--- a/gcc/toplev.c
+++ b/gcc/toplev.c
@@ -2173,6 +2173,8 @@ toplev::finalize (void)
finalize_options_struct (&global_options);
finalize_options_struct (&global_options_set);
+ XDELETEVEC (save_decoded_options);
+
/* Clean up the context (and pass_manager etc). */
delete g;
g = NULL;
--
1.8.5.3
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH 05/21] PR jit/63854: Fix memory leak of save_decoded_options
2014-01-01 0:00 ` [PATCH 05/21] PR jit/63854: Fix memory leak of save_decoded_options David Malcolm
@ 2014-01-01 0:00 ` Richard Biener
0 siblings, 0 replies; 69+ messages in thread
From: Richard Biener @ 2014-01-01 0:00 UTC (permalink / raw)
To: David Malcolm; +Cc: GCC Patches, jit
On Wed, Nov 19, 2014 at 11:46 AM, David Malcolm <dmalcolm@redhat.com> wrote:
> This commit fixes this leak from opts-common.c, about 4KB per iteration.
Ok.
Thanks,
Richard.
> ==57820== 18,816 (2,560 direct, 16,256 indirect) bytes in 4 blocks are definitely lost in loss record 907 of 917
> ==57820== at 0x4A083AA: realloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==57820== by 0x59A67CC: xrealloc (xmalloc.c:179)
> ==57820== by 0x59587C9: decode_cmdline_options_to_array(unsigned int, char const**, unsigned int, cl_decoded_option**, unsigned int*) (opts-common.c:885)
> ==57820== by 0x4E2ED90: toplev::main(int, char**) (toplev.c:2089)
> ==57820== by 0x4E43186: gcc::jit::playback::context::compile() (jit-playback.c:1615)
> ==57820== by 0x4E4018D: gcc::jit::recording::context::compile() (jit-recording.c:861)
> ==57820== by 0x401CA4: test_jit (harness.h:190)
> ==57820== by 0x401D88: main (harness.h:232)
>
> gcc/ChangeLog:
> PR jit/63854
> * toplev.c (toplev::finalize): Clean up save_decoded_options.
> ---
> gcc/toplev.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/gcc/toplev.c b/gcc/toplev.c
> index 876279f..291b84d 100644
> --- a/gcc/toplev.c
> +++ b/gcc/toplev.c
> @@ -2173,6 +2173,8 @@ toplev::finalize (void)
> finalize_options_struct (&global_options);
> finalize_options_struct (&global_options_set);
>
> + XDELETEVEC (save_decoded_options);
> +
> /* Clean up the context (and pass_manager etc). */
> delete g;
> g = NULL;
> --
> 1.8.5.3
>
^ permalink raw reply [flat|nested] 69+ messages in thread