* [PATCH] libgccjit: Fix several memory leaks in the driver
@ 2020-07-09 20:13 Alex Coplan
2020-08-20 22:26 ` Joseph Myers
0 siblings, 1 reply; 2+ messages in thread
From: Alex Coplan @ 2020-07-09 20:13 UTC (permalink / raw)
To: gcc-patches, jit; +Cc: David Malcolm, nd
[-- Attachment #1: Type: text/plain, Size: 2471 bytes --]
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().
[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 3118 bytes --]
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);
+}
+
\f
/* 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;
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [PATCH] libgccjit: Fix several memory leaks in the driver
2020-07-09 20:13 [PATCH] libgccjit: Fix several memory leaks in the driver Alex Coplan
@ 2020-08-20 22:26 ` Joseph Myers
0 siblings, 0 replies; 2+ messages in thread
From: Joseph Myers @ 2020-08-20 22:26 UTC (permalink / raw)
To: Alex Coplan; +Cc: gcc-patches, jit, nd
On Thu, 9 Jul 2020, Alex Coplan wrote:
> 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().
OK with a comment added to set_static_spec, documenting the semantics of
the function and its arguments.
--
Joseph S. Myers
joseph@codesourcery.com
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2020-08-20 22:26 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-09 20:13 [PATCH] libgccjit: Fix several memory leaks in the driver Alex Coplan
2020-08-20 22:26 ` Joseph Myers
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).