[PATCH] driver: Fix several memory leaks

[PATCH] driver: Fix several memory leaks

[Alex Coplan]

[-- Attachment #1: Type: text/plain, Size: 2943 bytes --]

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().


[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 3220 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 (&lto_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 (&lto_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;