[PATCH 03/21] PR jit/63854: Fix memory leaks within context/pass_manager/dump_manager
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
Re: [PATCH 03/21] PR jit/63854: Fix memory leaks within context/pass_manager/dump_manager
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
