[jit] Fix state issue in gcse.c

[jit] Fix state issue in gcse.c

[David Malcolm]

Committed to branch dmalcolm/jit:

Turning optimizations up from 0 to 3 showed a segfault on the 2nd
iteration of test-linked-list.c in get_data_from_adhoc_loc whilst
garbage-collecting.

Investigation revealed the issue to be a CFG from the previous compile
being kept alive by this GC root in gcse.c:
	  static GTY(()) rtx test_insn;

This wouldn't it itself be an issue, but one (or more) of the edges had:

  (gdb) p /x e->goto_locus
  $9 = 0x80000000

and was thus treated as an ADHOC_LOC.  Hence, this line in the edge_def's
gt_ggc_mx routine:

  8313	  tree block = LOCATION_BLOCK (e->goto_locus);

led to a call (via LOCATION_BLOCK) to get_data_from_adhoc_loc:

152	void *
153	get_data_from_adhoc_loc (struct line_maps *set, source_location loc)
154	{
155	  linemap_assert (IS_ADHOC_LOC (loc));
156	  return set->location_adhoc_data_map.data[loc & MAX_SOURCE_LOCATION].data;
157	}

but at this point, the ad-hoc location data from the previous in-process
compile no longer makes sense, and, with:
  (gdb) p set->location_adhoc_data_map
  $5 = {htab = 0x60fbb0, curr_loc = 0, allocated = 0, data = 0x0}

the read though the NULL "data" segfaults.

gcse.c appears to create test_insn on-demand as part of the implementation of
can_assign_to_reg_without_clobbers_p.

Hence it seems to make most sense to simply clear test_insn in toplev_finalize,
which this commit does.

With this, the whole test suite now runs successfully with optimizations
on, so also turn this up (in harness.h) from 0 to 3.

gcc/
	* gcse.c (gcse_c_finalize): New, to clear test_insn between
	in-process compiles.
	* gcse.h (gcse_c_finalize): New.
	* toplev.c: Include "gcse.h" so that we can...
	(toplev_finalize): Call gcse_c_finalize.

gcc/testsuite/
	* jit.dg/harness.h (set_options): Increase optimization level from
	0 to 3.
---
 gcc/ChangeLog.jit              | 8 ++++++++
 gcc/gcse.c                     | 5 +++++
 gcc/gcse.h                     | 2 ++
 gcc/testsuite/ChangeLog.jit    | 5 +++++
 gcc/testsuite/jit.dg/harness.h | 2 +-
 gcc/toplev.c                   | 2 ++
 6 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/gcc/ChangeLog.jit b/gcc/ChangeLog.jit
index 5c14fcc..77ac44c 100644
--- a/gcc/ChangeLog.jit
+++ b/gcc/ChangeLog.jit
@@ -1,5 +1,13 @@
 2014-03-11  David Malcolm  <dmalcolm@redhat.com>
 
+	* gcse.c (gcse_c_finalize): New, to clear test_insn between
+	in-process compiles.
+	* gcse.h (gcse_c_finalize): New.
+	* toplev.c: Include "gcse.h" so that we can...
+	(toplev_finalize): Call gcse_c_finalize.
+
+2014-03-11  David Malcolm  <dmalcolm@redhat.com>
+
 	* dwarf2out.c (dwarf2out_c_finalize): Release base_types.
 
 2014-03-10  David Malcolm  <dmalcolm@redhat.com>
diff --git a/gcc/gcse.c b/gcc/gcse.c
index bb9ba15..f2409ec 100644
--- a/gcc/gcse.c
+++ b/gcc/gcse.c
@@ -4226,4 +4226,9 @@ make_pass_rtl_hoist (gcc::context *ctxt)
   return new pass_rtl_hoist (ctxt);
 }
 
+void gcse_c_finalize (void)
+{
+  test_insn = NULL;
+}
+
 #include "gt-gcse.h"
diff --git a/gcc/gcse.h b/gcc/gcse.h
index e1dea21..f459be2 100644
--- a/gcc/gcse.h
+++ b/gcc/gcse.h
@@ -39,4 +39,6 @@ extern struct target_gcse *this_target_gcse;
 #define this_target_gcse (&default_target_gcse)
 #endif
 
+void gcse_c_finalize (void);
+
 #endif
diff --git a/gcc/testsuite/ChangeLog.jit b/gcc/testsuite/ChangeLog.jit
index ec8af3d..5a84bfd 100644
--- a/gcc/testsuite/ChangeLog.jit
+++ b/gcc/testsuite/ChangeLog.jit
@@ -1,3 +1,8 @@
+2014-03-11  David Malcolm  <dmalcolm@redhat.com>
+
+	* jit.dg/harness.h (set_options): Increase optimization level from
+	0 to 3.
+
 2014-03-07  David Malcolm  <dmalcolm@redhat.com>
 
 	* jit.dg/test-functions.c (create_test_of_hidden_function): New,
diff --git a/gcc/testsuite/jit.dg/harness.h b/gcc/testsuite/jit.dg/harness.h
index aa98028..e67ac36 100644
--- a/gcc/testsuite/jit.dg/harness.h
+++ b/gcc/testsuite/jit.dg/harness.h
@@ -132,7 +132,7 @@ static void set_options (gcc_jit_context *ctxt, const char *argv0)
   gcc_jit_context_set_int_option (
     ctxt,
     GCC_JIT_INT_OPTION_OPTIMIZATION_LEVEL,
-    0);
+    3);
   gcc_jit_context_set_bool_option (
     ctxt,
     GCC_JIT_BOOL_OPTION_DEBUGINFO,
diff --git a/gcc/toplev.c b/gcc/toplev.c
index 9de1c2d..f1ac560 100644
--- a/gcc/toplev.c
+++ b/gcc/toplev.c
@@ -79,6 +79,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "pass_manager.h"
 #include "dwarf2out.h"
 #include "ipa-reference.h"
+#include "gcse.h"
 
 #if defined(DBX_DEBUGGING_INFO) || defined(XCOFF_DEBUGGING_INFO)
 #include "dbxout.h"
@@ -2000,6 +2001,7 @@ void toplev_finalize (void)
   cgraphbuild_c_finalize ();
   cgraphunit_c_finalize ();
   dwarf2out_c_finalize ();
+  gcse_c_finalize ();
   ipa_c_finalize ();
   ipa_reference_c_finalize ();
   predict_c_finalize ();
-- 
1.7.11.7

Re: [jit] Fix state issue in gcse.c

[Steven Bosscher]

On Tue, Mar 11, 2014 at 8:00 PM, David Malcolm wrote:
> Investigation revealed the issue to be a CFG from the previous compile
> being kept alive by this GC root in gcse.c:
>           static GTY(()) rtx test_insn;
>
> This wouldn't it itself be an issue, but one (or more) of the edges had:

But this is an issue! This is not supposed to be possible.

test_insn is not in the insns chain and also not in a basic block, so
it should never keep a CFG alive.

Your patch papers over a bigger issue: How does test_insn end up
preventing the CFG from being garbage-collected.

Ciao!
Steven

Re: [jit] Fix state issue in gcse.c

[Steven Bosscher]

On Sat, Apr 19, 2014 at 3:24 PM, Steven Bosscher wrote:
> On Tue, Mar 11, 2014 at 8:00 PM, David Malcolm wrote:
>> Investigation revealed the issue to be a CFG from the previous compile
>> being kept alive by this GC root in gcse.c:
>>           static GTY(()) rtx test_insn;
>>
>> This wouldn't it itself be an issue, but one (or more) of the edges had:
>
> But this is an issue! This is not supposed to be possible.
>
> test_insn is not in the insns chain and also not in a basic block, so
> it should never keep a CFG alive.
>
> Your patch papers over a bigger issue: How does test_insn end up
> preventing the CFG from being garbage-collected.


The only way this could happen, that I can think of, is a reference
from SET_SRC into the insn stream (like a label_ref).

Can you please try and see if the patch below fixes your problem?

Ciao!
Steven



        * gcse.c (can_assign_to_reg_without_clobbers_p): Do not let pointers
        from test_insn into GGC space escape via SET_SRC.

Index: gcse.c
===================================================================
--- gcse.c      (revision 209530)
+++ gcse.c      (working copy)
@@ -849,6 +849,7 @@ can_assign_to_reg_without_clobbers_p (rtx x)
 {
   int num_clobbers = 0;
   int icode;
+  bool can_assign = false;

   /* If this is a valid operand, we are OK.  If it's VOIDmode, we aren't.  */
   if (general_operand (x, GET_MODE (x)))
@@ -866,6 +867,7 @@ can_assign_to_reg_without_clobbers_p (rtx x)
                                                   FIRST_PSEUDO_REGISTER * 2),
                                      const0_rtx));
       NEXT_INSN (test_insn) = PREV_INSN (test_insn) = 0;
+      INSN_LOCATION (test_insn) = UNKNOWN_LOCATION;
     }

   /* Now make an insn like the one we would make when GCSE'ing and see if
@@ -874,16 +876,19 @@ can_assign_to_reg_without_clobbers_p (rtx x)
   SET_SRC (PATTERN (test_insn)) = x;

   icode = recog (PATTERN (test_insn), test_insn, &num_clobbers);
-  if (icode < 0)
-    return false;
-
-  if (num_clobbers > 0 && added_clobbers_hard_reg_p (icode))
-    return false;
-
-  if (targetm.cannot_copy_insn_p && targetm.cannot_copy_insn_p (test_insn))
-    return false;
-
-  return true;
+
+  /* If the test insn is valid and doesn't need clobbers, and the target also
+     has no objections, we're good.  */
+  if (icode != 0
+      && (num_clobbers == 0 || !added_clobbers_hard_reg_p (icode))
+      && ! (targetm.cannot_copy_insn_p
+           && targetm.cannot_copy_insn_p (test_insn)))
+    can_assign = true;
+
+  /* Make sure test_insn doesn't have any pointers into GC space.  */
+  SET_SRC (PATTERN (test_insn)) = NULL_RTX;
+
+  return can_assign;
 }

 /* Return nonzero if the operands of expression X are unchanged from the

Re: [jit] Fix state issue in gcse.c

[David Malcolm]

On Sat, 2014-04-19 at 15:55 +0200, Steven Bosscher wrote:
> On Sat, Apr 19, 2014 at 3:24 PM, Steven Bosscher wrote:
> > On Tue, Mar 11, 2014 at 8:00 PM, David Malcolm wrote:
> >> Investigation revealed the issue to be a CFG from the previous compile
> >> being kept alive by this GC root in gcse.c:
> >>           static GTY(()) rtx test_insn;
> >>
> >> This wouldn't it itself be an issue, but one (or more) of the edges had:
> >
> > But this is an issue! This is not supposed to be possible.
> >
> > test_insn is not in the insns chain and also not in a basic block, so
> > it should never keep a CFG alive.
> >
> > Your patch papers over a bigger issue: How does test_insn end up
> > preventing the CFG from being garbage-collected.
> 
> 
> The only way this could happen, that I can think of, is a reference
> from SET_SRC into the insn stream (like a label_ref).
> 
> Can you please try and see if the patch below fixes your problem?

Thanks; your patch does indeed fix the issue: with it, I no longer need
the band-aid from my patch to be able to run the jit testsuite with full
optimization.

> 
> Ciao!
> Steven
> 
> 
> 
>         * gcse.c (can_assign_to_reg_without_clobbers_p): Do not let pointers
>         from test_insn into GGC space escape via SET_SRC.
> 
> Index: gcse.c
> ===================================================================
> --- gcse.c      (revision 209530)
> +++ gcse.c      (working copy)
> @@ -849,6 +849,7 @@ can_assign_to_reg_without_clobbers_p (rtx x)
>  {
>    int num_clobbers = 0;
>    int icode;
> +  bool can_assign = false;
> 
>    /* If this is a valid operand, we are OK.  If it's VOIDmode, we aren't.  */
>    if (general_operand (x, GET_MODE (x)))
> @@ -866,6 +867,7 @@ can_assign_to_reg_without_clobbers_p (rtx x)
>                                                    FIRST_PSEUDO_REGISTER * 2),
>                                       const0_rtx));
>        NEXT_INSN (test_insn) = PREV_INSN (test_insn) = 0;
> +      INSN_LOCATION (test_insn) = UNKNOWN_LOCATION;
>      }
> 
>    /* Now make an insn like the one we would make when GCSE'ing and see if
> @@ -874,16 +876,19 @@ can_assign_to_reg_without_clobbers_p (rtx x)
>    SET_SRC (PATTERN (test_insn)) = x;
> 
>    icode = recog (PATTERN (test_insn), test_insn, &num_clobbers);
> -  if (icode < 0)
> -    return false;
> -
> -  if (num_clobbers > 0 && added_clobbers_hard_reg_p (icode))
> -    return false;
> -
> -  if (targetm.cannot_copy_insn_p && targetm.cannot_copy_insn_p (test_insn))
> -    return false;
> -
> -  return true;
> +
> +  /* If the test insn is valid and doesn't need clobbers, and the target also
> +     has no objections, we're good.  */
> +  if (icode != 0
> +      && (num_clobbers == 0 || !added_clobbers_hard_reg_p (icode))
> +      && ! (targetm.cannot_copy_insn_p
> +           && targetm.cannot_copy_insn_p (test_insn)))
> +    can_assign = true;
> +
> +  /* Make sure test_insn doesn't have any pointers into GC space.  */
> +  SET_SRC (PATTERN (test_insn)) = NULL_RTX;
> +
> +  return can_assign;
>  }
> 
>  /* Return nonzero if the operands of expression X are unchanged from the

Re: [jit] Fix state issue in gcse.c

[Steven Bosscher]

On Fri, Apr 25, 2014 at 9:48 PM, David Malcolm wrote:
> Thanks; your patch does indeed fix the issue: with it, I no longer need
> the band-aid from my patch to be able to run the jit testsuite with full
> optimization.

Good. In the mean time I tested the patch, and found a typo:

+  if (icode != 0

should be:

+  if (icode >= 0

I'll post the patch for trunk this weekend.

Ciao!
Steven