| Message ID | VI1PR08MB4029AD17208D73BDF60A216DEA610@VI1PR08MB4029.eurprd08.prod.outlook.com |
|---|---|
| State | New |
| Headers | show |
| Series | driver: Fix several memory leaks | expand |
Ping. > -----Original Message----- > From: Jit <jit-bounces@gcc.gnu.org> On Behalf Of Alex Coplan > Sent: 14 July 2020 10:08 > To: gcc-patches@gcc.gnu.org; jit@gcc.gnu.org > Subject: [PATCH] driver: Fix several memory leaks > > Updating the subject since this is really just a driver change (and > therefore needs a review from those who can approve patches there). > > Thanks, > Alex > > -----Original Message----- > From: Jit <jit-bounces@gcc.gnu.org> On Behalf Of Alex Coplan > Sent: 09 July 2020 21:13 > To: gcc-patches@gcc.gnu.org; jit@gcc.gnu.org > Cc: nd <nd@arm.com> > Subject: [PATCH] libgccjit: Fix several memory leaks in the driver > > Hello, > > This patch fixes several memory leaks in the driver, all of which relate > to the handling of static specs. We introduce functions > set_static_spec_{shared,owned}() which are used to enforce proper memory > management when updating the strings in the static_specs table. > > This is achieved by making use of the alloc_p field in the table > entries. Similarly to set_spec(), each time we update an entry, we check > whether alloc_p is set, and free the old value if so. We then set > alloc_p correctly based on whether we "own" this memory or whether we're > just taking a pointer to a shared string which we shouldn't free. > > The following table shows the number of leaks found by AddressSanitizer > when running a minimal libgccjit program on AArch64. The test program > does the whole libgccjit compilation cycle in a loop (including acquiring > and releasing the context), and the table below shows the number of leaks > for different iterations of that loop. > > +--------------+-----+-----+------+---------------+ > | # of runs > | 1 | 2 | 3 | Leaks per run | > +--------------+-----+-----+------+---------------+ > | Before patch | 463 | 940 | 1417 | 477 | > +--------------+-----+-----+------+---------------+ > | After patch | 416 | 846 | 1276 | 430 | > +--------------+-----+-----+------+---------------+ > > Ensuring that we minimize "leaks per run" (ultimately eliminating all of > them) is important in order for long-running applications to be able to > make use of in-process libgccjit. > > Testing: > * Bootstrap and regtest on aarch64-linxu-gnu, x86_64-linux-gnu. > * Bootstrap and regtest on aarch64-linux-gnu with bootstrap-asan config. > * Smoke test of libgccjit, ran regressions on a --disable-bootstrap > build on > aarch64-linux-gnu. > > OK for master? > > Thanks, > Alex > > --- > > gcc/ChangeLog: > > 2020-07-09 Alex Coplan <alex.coplan@arm.com> > > * gcc.c (set_static_spec): New. > (set_static_spec_owned): New. > (set_static_spec_shared): New. > (driver::maybe_putenv_COLLECT_LTO_WRAPPER): Use > set_static_spec_owned() to take ownership of lto_wrapper_file > such that it gets freed in driver::finalize. > (driver::maybe_run_linker): Use set_static_spec_shared() to > ensure that we don't try and free() the static string "ld", > also ensuring that any previously-allocated string in > linker_name_spec is freed. Likewise with argv0. > (driver::finalize): Use set_static_spec_shared() when resetting > specs that previously had allocated strings; remove if(0) > around call to free().
Ping^2. > -----Original Message----- > From: Gcc-patches <gcc-patches-bounces@gcc.gnu.org> On Behalf Of Alex > Coplan > Sent: 03 August 2020 16:02 > To: gcc-patches@gcc.gnu.org > Subject: RE: [PATCH] driver: Fix several memory leaks > > Ping. > > > -----Original Message----- > > From: Jit <jit-bounces@gcc.gnu.org> On Behalf Of Alex Coplan > > Sent: 14 July 2020 10:08 > > To: gcc-patches@gcc.gnu.org; jit@gcc.gnu.org > > Subject: [PATCH] driver: Fix several memory leaks > > > > Updating the subject since this is really just a driver change (and > > therefore needs a review from those who can approve patches there). > > > > Thanks, > > Alex > > > > -----Original Message----- > > From: Jit <jit-bounces@gcc.gnu.org> On Behalf Of Alex Coplan > > Sent: 09 July 2020 21:13 > > To: gcc-patches@gcc.gnu.org; jit@gcc.gnu.org > > Cc: nd <nd@arm.com> > > Subject: [PATCH] libgccjit: Fix several memory leaks in the driver > > > > Hello, > > > > This patch fixes several memory leaks in the driver, all of which > relate > > to the handling of static specs. We introduce functions > > set_static_spec_{shared,owned}() which are used to enforce proper > memory > > management when updating the strings in the static_specs table. > > > > This is achieved by making use of the alloc_p field in the table > > entries. Similarly to set_spec(), each time we update an entry, we > check > > whether alloc_p is set, and free the old value if so. We then set > > alloc_p correctly based on whether we "own" this memory or whether > we're > > just taking a pointer to a shared string which we shouldn't free. > > > > The following table shows the number of leaks found by AddressSanitizer > > when running a minimal libgccjit program on AArch64. The test program > > does the whole libgccjit compilation cycle in a loop (including > acquiring > > and releasing the context), and the table below shows the number of > leaks > > for different iterations of that loop. > > > > +--------------+-----+-----+------+---------------+ > > | # of runs > | 1 | 2 | 3 | Leaks per run | > > +--------------+-----+-----+------+---------------+ > > | Before patch | 463 | 940 | 1417 | 477 | > > +--------------+-----+-----+------+---------------+ > > | After patch | 416 | 846 | 1276 | 430 | > > +--------------+-----+-----+------+---------------+ > > > > Ensuring that we minimize "leaks per run" (ultimately eliminating all > of > > them) is important in order for long-running applications to be able to > > make use of in-process libgccjit. > > > > Testing: > > * Bootstrap and regtest on aarch64-linxu-gnu, x86_64-linux-gnu. > > * Bootstrap and regtest on aarch64-linux-gnu with bootstrap-asan > config. > > * Smoke test of libgccjit, ran regressions on a --disable-bootstrap > > build on > > aarch64-linux-gnu. > > > > OK for master? > > > > Thanks, > > Alex > > > > --- > > > > gcc/ChangeLog: > > > > 2020-07-09 Alex Coplan <alex.coplan@arm.com> > > > > * gcc.c (set_static_spec): New. > > (set_static_spec_owned): New. > > (set_static_spec_shared): New. > > (driver::maybe_putenv_COLLECT_LTO_WRAPPER): Use > > set_static_spec_owned() to take ownership of lto_wrapper_file > > such that it gets freed in driver::finalize. > > (driver::maybe_run_linker): Use set_static_spec_shared() to > > ensure that we don't try and free() the static string "ld", > > also ensuring that any previously-allocated string in > > linker_name_spec is freed. Likewise with argv0. > > (driver::finalize): Use set_static_spec_shared() when resetting > > specs that previously had allocated strings; remove if(0) > > around call to free().
diff --git a/gcc/gcc.c b/gcc/gcc.c index c0eb3c10cfd..f839971d42d 100644 --- a/gcc/gcc.c +++ b/gcc/gcc.c @@ -1908,6 +1908,47 @@ init_spec (void) specs = sl; } + +static void +set_static_spec (const char **spec, const char *value, bool alloc_p) +{ + struct spec_list *sl = NULL; + + for (unsigned i = 0; i < ARRAY_SIZE (static_specs); i++) + { + if (static_specs[i].ptr_spec == spec) + { + sl = static_specs + i; + break; + } + } + + gcc_assert (sl); + + if (sl->alloc_p) + { + const char *old = *spec; + free (const_cast <char *> (old)); + } + + *spec = value; + sl->alloc_p = alloc_p; +} + +/* Update a static spec to a new string, taking ownership of that + string's memory. */ +static void set_static_spec_owned (const char **spec, const char *val) +{ + return set_static_spec (spec, val, true); +} + +/* Update a static spec to point to a new value, but don't take + ownership of (i.e. don't free) that string. */ +static void set_static_spec_shared (const char **spec, const char *val) +{ + return set_static_spec (spec, val, false); +} + /* Change the value of spec NAME to SPEC. If SPEC is empty, then the spec is removed; If the spec starts with a + then SPEC is added to the end of the @@ -8333,7 +8374,7 @@ driver::maybe_putenv_COLLECT_LTO_WRAPPER () const if (lto_wrapper_file) { lto_wrapper_file = convert_white_space (lto_wrapper_file); - lto_wrapper_spec = lto_wrapper_file; + set_static_spec_owned (<o_wrapper_spec, lto_wrapper_file); obstack_init (&collect_obstack); obstack_grow (&collect_obstack, "COLLECT_LTO_WRAPPER=", sizeof ("COLLECT_LTO_WRAPPER=") - 1); @@ -8840,7 +8881,7 @@ driver::maybe_run_linker (const char *argv0) const { char *s = find_a_file (&exec_prefixes, "collect2", X_OK, false); if (s == NULL) - linker_name_spec = "ld"; + set_static_spec_shared (&linker_name_spec, "ld"); } #if HAVE_LTO_PLUGIN > 0 @@ -8864,7 +8905,7 @@ driver::maybe_run_linker (const char *argv0) const linker_plugin_file_spec = convert_white_space (temp_spec); } #endif - lto_gcc_spec = argv0; + set_static_spec_shared (<o_gcc_spec, argv0); } /* Rebuild the COMPILER_PATH and LIBRARY_PATH environment variables @@ -10806,9 +10847,9 @@ driver::finalize () just_machine_suffix = 0; gcc_exec_prefix = 0; gcc_libexec_prefix = 0; - md_exec_prefix = MD_EXEC_PREFIX; - md_startfile_prefix = MD_STARTFILE_PREFIX; - md_startfile_prefix_1 = MD_STARTFILE_PREFIX_1; + set_static_spec_shared (&md_exec_prefix, MD_EXEC_PREFIX); + set_static_spec_shared (&md_startfile_prefix, MD_STARTFILE_PREFIX); + set_static_spec_shared (&md_startfile_prefix_1, MD_STARTFILE_PREFIX_1); multilib_dir = 0; multilib_os_dir = 0; multiarch_dir = 0; @@ -10832,8 +10873,7 @@ driver::finalize () spec_list *sl = &static_specs[i]; if (sl->alloc_p) { - if (0) - free (const_cast <char *> (*(sl->ptr_spec))); + free (const_cast <char *> (*(sl->ptr_spec))); sl->alloc_p = false; } *(sl->ptr_spec) = sl->default_ptr;
Updating the subject since this is really just a driver change (and therefore needs a review from those who can approve patches there). Thanks, Alex -----Original Message----- From: Jit <jit-bounces@gcc.gnu.org> On Behalf Of Alex Coplan Sent: 09 July 2020 21:13 To: gcc-patches@gcc.gnu.org; jit@gcc.gnu.org Cc: nd <nd@arm.com> Subject: [PATCH] libgccjit: Fix several memory leaks in the driver Hello, This patch fixes several memory leaks in the driver, all of which relate to the handling of static specs. We introduce functions set_static_spec_{shared,owned}() which are used to enforce proper memory management when updating the strings in the static_specs table. This is achieved by making use of the alloc_p field in the table entries. Similarly to set_spec(), each time we update an entry, we check whether alloc_p is set, and free the old value if so. We then set alloc_p correctly based on whether we "own" this memory or whether we're just taking a pointer to a shared string which we shouldn't free. The following table shows the number of leaks found by AddressSanitizer when running a minimal libgccjit program on AArch64. The test program does the whole libgccjit compilation cycle in a loop (including acquiring and releasing the context), and the table below shows the number of leaks for different iterations of that loop. +--------------+-----+-----+------+---------------+ | # of runs > | 1 | 2 | 3 | Leaks per run | +--------------+-----+-----+------+---------------+ | Before patch | 463 | 940 | 1417 | 477 | +--------------+-----+-----+------+---------------+ | After patch | 416 | 846 | 1276 | 430 | +--------------+-----+-----+------+---------------+ Ensuring that we minimize "leaks per run" (ultimately eliminating all of them) is important in order for long-running applications to be able to make use of in-process libgccjit. Testing: * Bootstrap and regtest on aarch64-linxu-gnu, x86_64-linux-gnu. * Bootstrap and regtest on aarch64-linux-gnu with bootstrap-asan config. * Smoke test of libgccjit, ran regressions on a --disable-bootstrap build on aarch64-linux-gnu. OK for master? Thanks, Alex --- gcc/ChangeLog: 2020-07-09 Alex Coplan <alex.coplan@arm.com> * gcc.c (set_static_spec): New. (set_static_spec_owned): New. (set_static_spec_shared): New. (driver::maybe_putenv_COLLECT_LTO_WRAPPER): Use set_static_spec_owned() to take ownership of lto_wrapper_file such that it gets freed in driver::finalize. (driver::maybe_run_linker): Use set_static_spec_shared() to ensure that we don't try and free() the static string "ld", also ensuring that any previously-allocated string in linker_name_spec is freed. Likewise with argv0. (driver::finalize): Use set_static_spec_shared() when resetting specs that previously had allocated strings; remove if(0) around call to free().