diff mbox

[1/2] fix memory chunk corruption for opts_obstack (PR jit/68446)

Message ID 1452888274-46515-1-git-send-email-dmalcolm@redhat.com
State New
Headers show

Commit Message

David Malcolm Jan. 15, 2016, 8:04 p.m. UTC
There can be multiple gcc_options instances, each with
a call to
  init_options_struct
matched with a call to
  finalize_options_struct
whereas the
  opts_obstack
is a singleton.  Each gcc_options instance can potentially use the
opts_obstack singleton.

r230264 (aka 25faed340686df8d7bb2242dc8d04285976922b6) fixed
a large memory leak (1.2MB) of the opts_obstack, by making
initialization of the opts_obstack be idempotent
(in init_opts_obstack).

This works if we only have one in-process run of the compiler.
Unfortunately this commit broke much of libgccjit's test suite,
which now fails with memory corruption errors.

The root cause of the breakage is that toplev::finalize cleans up the
opts_obstack using:

  obstack_free (opts_obstack, NULL);

Calling obstack_free with NULL leaves an obstack in an uninitialized
state and hence a reinitialization is required;
libiberty/obstacks.texi has:

  Note that if @var{object} is a null pointer, the result is an
  *uninitialized* obstack.  [my emphasis]

Hence opts_obstack reverts to an uninitialized state - but further
calls to initialize it are rejected by the idempotency code in
init_opts_obstack, and we then attempt to allocate from an
uninitialized obstack.

In particular, the obstack's "chunk" field becomes invalid, but isn't
unset. The underlying 64KB chunk(s) are returned to memory_block_pool's
m_blocks linked-list of 64KB free chunks, and they get reused by othe
obstacks e.g. for bitmaps.  However, given that opts_obstack fails
to be reinitialized, opts_obstack.chunks points at a freed chunk.
Hence, on the 2nd iteration of a jit testcase, it gets used to
allocate copies of the options, but this out of a chunk that's being
used by a different memory_block_pool user, so chaos ensues: we have
64KB chunks of memory being erroneously shared between different
memory-pool users.

The following patch removes idempotency from init_opts_obstack, and
replaces the call to init_opts_stack from init_options_struct with
an assert that the singleton opts_stack is already initialized,
adding in the necessary per-compile initialization of opts_stack
(we already have per-compile cleanup).

Or to put it another way, previously, we had this pattern of calls:

  - for each jit compile:
    - toplev:
      - multiple calls to init_options_struct matched with calls to
        finalize_options_struct; the first call to init_options_struct
        idempotently initializes opts_obstack.
      - obstack_free (&opts_obstack, NULL);
(leading to corrupt opts_obstack on the 2nd iteration of jit compilation)

and with this patch we instead have this:

  - for each jit compile:
    - toplev:
      - init_opts_obstack
      - multiple calls to init_options_struct matched with calls to
        finalize_options_struct
      - obstack_free (&opts_obstack, NULL);

(I don't like that opts_obstack is global state, but it seems risky
to try to fix that at this stage).

The patch also adds code to reset save_decoded_options and
save_decoded_options_count when freeing opts_obstack, since these
saved options are allocated from out of opts_obstack and hence
also become invalid when it's freed.

Successfully bootstrapped&regrtested on x86_64-pc-linux-gnu in
conjunction with the followup patch.

Fixes all of the jit test suite, apart from test-threads.c (which is
broken for a different reason).  I've also verified manually under
Valgrind that this also keeps the fix for the large leak reported in
the PR that motivated r230264).

OK for trunk?  (assuming it bootstraps&regrtests by itself)

gcc/ChangeLog:
	PR jit/68446
	* gcc.c (driver::decode_argv): Add call to
	init_opts_obstack before init_options_struct.
	* opts.c (init_opts_obstack): Remove idempotency.
	(init_options_struct): Replace call to init_opts_obstack
	with a gcc_assert to verify that it has already been called.
	* toplev.c (toplev::main): Add call to init_opts_obstack before
	calls to init_options_struct.
	(toplev::finalize): Move cleanup of opts_obstack next to
	cleanup of save_decoded_options, clearing the latter, and
	save_decoded_options_count.
---
 gcc/gcc.c    |  1 +
 gcc/opts.c   | 14 +++++---------
 gcc/toplev.c |  7 ++++++-
 3 files changed, 12 insertions(+), 10 deletions(-)

Comments

Jakub Jelinek Jan. 18, 2016, 11:58 p.m. UTC | #1
On Fri, Jan 15, 2016 at 03:04:33PM -0500, David Malcolm wrote:
> OK for trunk?  (assuming it bootstraps&regrtests by itself)
> 
> gcc/ChangeLog:
> 	PR jit/68446
> 	* gcc.c (driver::decode_argv): Add call to
> 	init_opts_obstack before init_options_struct.
> 	* opts.c (init_opts_obstack): Remove idempotency.
> 	(init_options_struct): Replace call to init_opts_obstack
> 	with a gcc_assert to verify that it has already been called.
> 	* toplev.c (toplev::main): Add call to init_opts_obstack before
> 	calls to init_options_struct.
> 	(toplev::finalize): Move cleanup of opts_obstack next to
> 	cleanup of save_decoded_options, clearing the latter, and
> 	save_decoded_options_count.

Ok.

	Jakub
diff mbox

Patch

diff --git a/gcc/gcc.c b/gcc/gcc.c
index 319a073..c191fde 100644
--- a/gcc/gcc.c
+++ b/gcc/gcc.c
@@ -7191,6 +7191,7 @@  driver::decode_argv (int argc, const char **argv)
   global_init_params ();
   finish_params ();
 
+  init_opts_obstack ();
   init_options_struct (&global_options, &global_options_set);
 
   decode_cmdline_options_to_array (argc, argv,
diff --git a/gcc/opts.c b/gcc/opts.c
index 2add158..9437535 100644
--- a/gcc/opts.c
+++ b/gcc/opts.c
@@ -266,18 +266,12 @@  add_comma_separated_to_vector (void **pvec, const char *arg)
   *pvec = v;
 }
 
-/* Initialize opts_obstack if not initialized.  */
+/* Initialize opts_obstack.  */
 
 void
 init_opts_obstack (void)
 {
-  static bool opts_obstack_initialized = false;
-
-  if (!opts_obstack_initialized)
-    {
-      opts_obstack_initialized = true;
-      gcc_obstack_init (&opts_obstack);
-    }
+  gcc_obstack_init (&opts_obstack);
 }
 
 /* Initialize OPTS and OPTS_SET before using them in parsing options.  */
@@ -287,7 +281,9 @@  init_options_struct (struct gcc_options *opts, struct gcc_options *opts_set)
 {
   size_t num_params = get_num_compiler_params ();
 
-  init_opts_obstack ();
+  /* Ensure that opts_obstack has already been initialized by the time
+     that we initialize any gcc_options instances (PR jit/68446).  */
+  gcc_assert (opts_obstack.chunk_size > 0);
 
   *opts = global_options_init;
 
diff --git a/gcc/toplev.c b/gcc/toplev.c
index 8bab3e5..580c439 100644
--- a/gcc/toplev.c
+++ b/gcc/toplev.c
@@ -2050,6 +2050,7 @@  toplev::main (int argc, char **argv)
   /* One-off initialization of options that does not need to be
      repeated when options are added for particular functions.  */
   init_options_once ();
+  init_opts_obstack ();
 
   /* Initialize global options structures; this must be repeated for
      each structure used for parsing options.  */
@@ -2131,11 +2132,15 @@  toplev::finalize (void)
   finalize_options_struct (&global_options);
   finalize_options_struct (&global_options_set);
 
+  /* save_decoded_options uses opts_obstack, so these must
+     be cleaned up together.  */
+  obstack_free (&opts_obstack, NULL);
   XDELETEVEC (save_decoded_options);
+  save_decoded_options = NULL;
+  save_decoded_options_count = 0;
 
   /* Clean up the context (and pass_manager etc). */
   delete g;
   g = NULL;
 
-  obstack_free (&opts_obstack, NULL);
 }