diff mbox

[jit] Visit parent contexts in disassociate_from_playback

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

Commit Message

David Malcolm March 7, 2014, 2 a.m. UTC 
Committed to branch dmalcolm/jit:

My libgccjit port of GNU Octave's JIT was segfaulting due to corruption
of playback::location values when running Octave's JIT unittests.  After
lots of debugging, I found that recording::locations in a parent context
weren't having their playback::locations cleaned away between compiles,
and hence continue to point to a GC-allocated object for which no marking
is performed (by design: these objects aren't meant to outlive the compile
and hence should always be collected).

Hence we have pointers to supposedly-dead objects that don't get NULLed,
and point to bytes that eventually get repurposed by the GC, leading
to crashes.

The fix is to ensure that when clearing the playback objects from all
recording objects we visit those in parent contexts (and above), and not
just those in the bottommost context.

This bug was elusive, since most recording classes blithely
overwrite the old invalid pointers on replay, but recording::location
checks for a pre-existing playback::location (since
gcc_jit_context_dump_to_file can lead to playback::locations being needed
before they would normally be created), and can thus reuse the now-invalid
pointer - provided it's in a parent context.

We can reproduce this bug by using gcc_jit_context_dump_to_file in
test-nested-contexts.c on each context, to set up fake "source locations"
on the various entities in each context.  Doing so also gives us test
coverage of that function.

gcc/jit/
	* internal-api.c (gcc::jit::recording::context::
	disassociate_from_playback): Recursively visit parent contexts.

gcc/testsuite/
	* jit.dg/test-nested-contexts.c (main): Dump the contexts to
	files, setting up source locations, and adding test coverage for
	gcc_jit_context_dump_to_file.
---
 gcc/jit/ChangeLog.jit                       |  5 +++++
 gcc/jit/internal-api.c                      |  4 ++++
 gcc/testsuite/ChangeLog.jit                 |  6 ++++++
 gcc/testsuite/jit.dg/test-nested-contexts.c | 12 ++++++++++++
 4 files changed, 27 insertions(+)
diff mbox

Patch

diff --git a/gcc/jit/ChangeLog.jit b/gcc/jit/ChangeLog.jit
index 3e2799f..b0058b9 100644
--- a/gcc/jit/ChangeLog.jit
+++ b/gcc/jit/ChangeLog.jit
@@ -1,3 +1,8 @@ 
+2014-03-06  David Malcolm  <dmalcolm@redhat.com>
+
+	* internal-api.c (gcc::jit::recording::context::
+	disassociate_from_playback): Recursively visit parent contexts.
+
 2014-03-05  David Malcolm  <dmalcolm@redhat.com>
 
 	* libgccjit.h (gcc_jit_function_dump_to_dot): New.
diff --git a/gcc/jit/internal-api.c b/gcc/jit/internal-api.c
index 62301b2..f80c40b 100644
--- a/gcc/jit/internal-api.c
+++ b/gcc/jit/internal-api.c
@@ -243,6 +243,10 @@  recording::context::disassociate_from_playback ()
 {
   int i;
   memento *m;
+
+  if (m_parent_ctxt)
+    m_parent_ctxt->disassociate_from_playback ();
+
   FOR_EACH_VEC_ELT (m_mementos, i, m)
     {
       m->set_playback_obj (NULL);
diff --git a/gcc/testsuite/ChangeLog.jit b/gcc/testsuite/ChangeLog.jit
index f66a844..52b606e 100644
--- a/gcc/testsuite/ChangeLog.jit
+++ b/gcc/testsuite/ChangeLog.jit
@@ -1,3 +1,9 @@ 
+2014-03-06  David Malcolm  <dmalcolm@redhat.com>
+
+	* jit.dg/test-nested-contexts.c (main): Dump the contexts to
+	files, setting up source locations, and adding test coverage for
+	gcc_jit_context_dump_to_file.
+
 2014-03-04  David Malcolm  <dmalcolm@redhat.com>
 
 	* jit.dg/test-error-mismatching-types-in-call.c: New test case,
diff --git a/gcc/testsuite/jit.dg/test-nested-contexts.c b/gcc/testsuite/jit.dg/test-nested-contexts.c
index e34d2dd..16bc63f 100644
--- a/gcc/testsuite/jit.dg/test-nested-contexts.c
+++ b/gcc/testsuite/jit.dg/test-nested-contexts.c
@@ -552,6 +552,10 @@  main (int argc, char **argv)
       /* No errors should have occurred.  */
       CHECK_VALUE (gcc_jit_context_get_first_error (top_level.ctxt), NULL);
 
+      gcc_jit_context_dump_to_file (top_level.ctxt,
+				    "dump-of-test-nested-contexts-top.c",
+				    1);
+
       for (j = 1; j <= NUM_MIDDLE_ITERATIONS; j++)
 	{
 	  /* Create and populate the middle-level context, using
@@ -575,6 +579,10 @@  main (int argc, char **argv)
 	  CHECK_VALUE (gcc_jit_context_get_first_error (middle_level.ctxt),
 		       NULL);
 
+	  gcc_jit_context_dump_to_file (middle_level.ctxt,
+					"dump-of-test-nested-contexts-middle.c",
+					1);
+
 	  gcc_jit_result *middle_result =
 	    gcc_jit_context_compile (middle_level.ctxt);
 	  CHECK_NON_NULL (middle_result);
@@ -607,6 +615,10 @@  main (int argc, char **argv)
 	      CHECK_VALUE (gcc_jit_context_get_first_error (bottom_level.ctxt),
 			   NULL);
 
+	      gcc_jit_context_dump_to_file (bottom_level.ctxt,
+					    "dump-of-test-nested-contexts-bottom.c",
+					    1);
+
 	      gcc_jit_result *bottom_result =
 		gcc_jit_context_compile (bottom_level.ctxt);
 	      verify_bottom_code (bottom_level.ctxt, bottom_result);