diff mbox series

Add checking that during RTL bbs don't mix EH and non-complex predecessor edges

Message ID 20180615191935.GI7166@tucnak
State New
Headers show
Series Add checking that during RTL bbs don't mix EH and non-complex predecessor edges | expand

Commit Message

Jakub Jelinek June 15, 2018, 7:19 p.m. UTC 
On Fri, Jun 15, 2018 at 09:06:16PM +0200, Jakub Jelinek wrote:
> The following patch fixes it by making sure we don't have EH landing pads
> that are reachable also by the crossing edges by splitting the EH landing
> pad bb we want to jump into and jumping (EDGE_CROSSING) only to the second
> half, leaving the first part with no actual instructions to be the EH
> landing pad.

The following patch adds verification that we don't have such basic blocks
during RTL optimizations.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

Eric, I wonder if sjlj doesn't need a similar fix to the PR86108 patch.
If it does, I think with the following patch it would trigger a checking
failure.  Not familiar with sjlj EH enough to try it myself.

2018-06-15  Jakub Jelinek  <jakub@redhat.com>

	* cfgrtl.c (rtl_verify_edges): Formatting fix.  If bb_has_eh_pred,
	verify all incoming edges are complex.



	Jakub

Comments

Eric Botcazou June 15, 2018, 8:21 p.m. UTC | #1 
> Eric, I wonder if sjlj doesn't need a similar fix to the PR86108 patch.
> If it does, I think with the following patch it would trigger a checking
> failure.  Not familiar with sjlj EH enough to try it myself.

Yes, it does, so please move the fix to create_forwarder_block instead, after 
renaming it into e.g. create_eh_forwarder_block.  You don't need to update 
old_bb in {dw2,sjlj}_fix_up_crossing_landing_pad.
Jakub Jelinek June 15, 2018, 8:52 p.m. UTC | #2 
On Fri, Jun 15, 2018 at 10:21:26PM +0200, Eric Botcazou wrote:
> > Eric, I wonder if sjlj doesn't need a similar fix to the PR86108 patch.
> > If it does, I think with the following patch it would trigger a checking
> > failure.  Not familiar with sjlj EH enough to try it myself.
> 
> Yes, it does, so please move the fix to create_forwarder_block instead, after 
> renaming it into e.g. create_eh_forwarder_block.  You don't need to update 
> old_bb in {dw2,sjlj}_fix_up_crossing_landing_pad.

So like this (if it passes another bootstrap/regtest together with the
cfgrtl.c change)?  It fixes the testcase too.

2018-06-15  Jakub Jelinek  <jakub@redhat.com>

	PR rtl-optimization/86108
	* bb-reorder.c (create_forwarder_block): Renamed to ...
	(create_eh_forwarder_block): ... this.  Split OLD_BB after labels and
	jump from new landing pad to the second part.
	(sjlj_fix_up_crossing_landing_pad, dw2_fix_up_crossing_landing_pad):
	Adjust callers.

--- gcc/bb-reorder.c.jj	2018-05-31 21:49:04.000000000 +0200
+++ gcc/bb-reorder.c	2018-06-15 22:47:27.750266777 +0200
@@ -1413,8 +1413,12 @@ get_uncond_jump_length (void)
    other partition wrt OLD_BB.  */
 
 static basic_block
-create_forwarder_block (rtx_code_label *new_label, basic_block old_bb)
+create_eh_forwarder_block (rtx_code_label *new_label, basic_block old_bb)
 {
+  /* Split OLD_BB, so that EH pads have always only incoming EH edges,
+     bb_has_eh_pred bbs are treated specially by DF infrastructure.  */
+  old_bb = split_block_after_labels (old_bb)->dest;
+
   /* Put the new label and a jump in the new basic block.  */
   rtx_insn *label = emit_label (new_label);
   rtx_code_label *old_label = block_label (old_bb);
@@ -1456,7 +1460,7 @@ sjlj_fix_up_crossing_landing_pad (basic_
   LABEL_PRESERVE_P (new_label) = 1;
 
   /* Create the forwarder block.  */
-  basic_block new_bb = create_forwarder_block (new_label, old_bb);
+  basic_block new_bb = create_eh_forwarder_block (new_label, old_bb);
 
   /* Create the map from old to new lp index and initialize it.  */
   unsigned *index_map = (unsigned *) alloca (lp_len * sizeof (unsigned));
@@ -1508,7 +1512,7 @@ dw2_fix_up_crossing_landing_pad (eh_land
   LABEL_PRESERVE_P (new_lp->landing_pad) = 1;
 
   /* Create the forwarder block.  */
-  basic_block new_bb = create_forwarder_block (new_lp->landing_pad, old_bb);
+  basic_block new_bb = create_eh_forwarder_block (new_lp->landing_pad, old_bb);
 
   /* Fix up the edges.  */
   for (ei = ei_start (old_bb->preds); (e = ei_safe_edge (ei)) != NULL; )


	Jakub
Eric Botcazou June 15, 2018, 9:33 p.m. UTC | #3 
> So like this (if it passes another bootstrap/regtest together with the
> cfgrtl.c change)?  It fixes the testcase too.
> 
> 2018-06-15  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR rtl-optimization/86108
> 	* bb-reorder.c (create_forwarder_block): Renamed to ...
> 	(create_eh_forwarder_block): ... this.  Split OLD_BB after labels and
> 	jump from new landing pad to the second part.
> 	(sjlj_fix_up_crossing_landing_pad, dw2_fix_up_crossing_landing_pad):
> 	Adjust callers.

OK, thanks.  I'll build the Ada compiler on x86/Windows once this is in.
Jakub Jelinek June 16, 2018, 7:09 a.m. UTC | #4 
On Fri, Jun 15, 2018 at 11:33:10PM +0200, Eric Botcazou wrote:
> > So like this (if it passes another bootstrap/regtest together with the
> > cfgrtl.c change)?  It fixes the testcase too.
> > 
> > 2018-06-15  Jakub Jelinek  <jakub@redhat.com>
> > 
> > 	PR rtl-optimization/86108
> > 	* bb-reorder.c (create_forwarder_block): Renamed to ...
> > 	(create_eh_forwarder_block): ... this.  Split OLD_BB after labels and
> > 	jump from new landing pad to the second part.
> > 	(sjlj_fix_up_crossing_landing_pad, dw2_fix_up_crossing_landing_pad):
> > 	Adjust callers.
> 
> OK, thanks.  I'll build the Ada compiler on x86/Windows once this is in.

Now committed after successful bootstrap/regtest on x86_64-linux and
i686-linux.  Is the cfgrtl.c change ok for trunk too?
http://gcc.gnu.org/ml/gcc-patches/2018-06/msg00967.html

	Jakub
Eric Botcazou June 16, 2018, 12:13 p.m. UTC | #5 
> Now committed after successful bootstrap/regtest on x86_64-linux and
> i686-linux.  Is the cfgrtl.c change ok for trunk too?
> http://gcc.gnu.org/ml/gcc-patches/2018-06/msg00967.html

Don't we actually need to verify that all incoming edges are EH or none is?
Jakub Jelinek June 20, 2018, 2:12 p.m. UTC | #6 
On Sat, Jun 16, 2018 at 02:13:32PM +0200, Eric Botcazou wrote:
> > Now committed after successful bootstrap/regtest on x86_64-linux and
> > i686-linux.  Is the cfgrtl.c change ok for trunk too?
> > http://gcc.gnu.org/ml/gcc-patches/2018-06/msg00967.html
> 
> Don't we actually need to verify that all incoming edges are EH or none is?

This also passes bootstrap/regtest for me (dwarf exceptions only though).
So is this ok for trunk instead?

2018-06-20  Jakub Jelinek  <jakub@redhat.com>

	* cfgrtl.c (rtl_verify_edges): Formatting fix.  If bb->preds has any
	EDGE_EH edges, verify they are all EDGE_EH.

--- gcc/cfgrtl.c.jj	2018-06-15 21:23:02.513600038 +0200
+++ gcc/cfgrtl.c	2018-06-20 12:57:12.079755604 +0200
@@ -2540,15 +2540,15 @@ rtl_verify_edges (void)
 	    n_abnormal++;
 	}
 
-        if (!has_crossing_edge
-	    && JUMP_P (BB_END (bb))
-	    && CROSSING_JUMP_P (BB_END (bb)))
-          {
-	    print_rtl_with_bb (stderr, get_insns (), TDF_BLOCKS | TDF_DETAILS);
-            error ("Region crossing jump across same section in bb %i",
-                   bb->index);
-            err = 1;
-          }
+      if (!has_crossing_edge
+	  && JUMP_P (BB_END (bb))
+	  && CROSSING_JUMP_P (BB_END (bb)))
+	{
+	  print_rtl_with_bb (stderr, get_insns (), TDF_BLOCKS | TDF_DETAILS);
+	  error ("Region crossing jump across same section in bb %i",
+		 bb->index);
+	  err = 1;
+	}
 
       if (n_eh && !find_reg_note (BB_END (bb), REG_EH_REGION, NULL_RTX))
 	{
@@ -2606,6 +2606,19 @@ rtl_verify_edges (void)
 	  error ("abnormal edges for no purpose in bb %i", bb->index);
 	  err = 1;
 	}
+
+      int has_eh = -1;
+      FOR_EACH_EDGE (e, ei, bb->preds)
+	{
+	  if (has_eh == -1)
+	    has_eh = (e->flags & EDGE_EH);
+	  if ((e->flags & EDGE_EH) == has_eh)
+	    continue;
+	  error ("EH incoming edge mixed with non-EH incoming edges "
+		 "in bb %i", bb->index);
+	  err = 1;
+	  break;
+	}
     }
 
   /* If there are partitions, do a sanity check on them: A basic block in


	Jakub
Eric Botcazou June 20, 2018, 2:16 p.m. UTC | #7 
> This also passes bootstrap/regtest for me (dwarf exceptions only though).
> So is this ok for trunk instead?
> 
> 2018-06-20  Jakub Jelinek  <jakub@redhat.com>
> 
> 	* cfgrtl.c (rtl_verify_edges): Formatting fix.  If bb->preds has any
> 	EDGE_EH edges, verify they are all EDGE_EH.

OK, thanks.  I'll give it a try on x86/Windows once this is in.
Eric Botcazou June 21, 2018, 5:02 p.m. UTC | #8 
> OK, thanks.  I'll give it a try on x86/Windows once this is in.

The build of the Ada runtime miserably fails because finish_eh_generation 
calls commit_edge_insertions before redirecting the EH edges from the post-
landing pads to the landing pads...

Fixed thusly, applied on the mainline as obvious. I think that's good enough 
because the only edge onto which instructions can be inserted in the entry 
edge (including  by the Alpha kludge).


	* except.c (finish_eh_generation): Commit edge insertions only after the
	EH edges have been redirected from post-landing to landing pads.
diff mbox series

Patch

--- gcc/cfgrtl.c.jj	2018-04-09 20:15:51.237631514 +0200
+++ gcc/cfgrtl.c	2018-06-15 18:15:37.479333131 +0200
@@ -2540,15 +2540,15 @@  rtl_verify_edges (void)
 	    n_abnormal++;
 	}
 
-        if (!has_crossing_edge
-	    && JUMP_P (BB_END (bb))
-	    && CROSSING_JUMP_P (BB_END (bb)))
-          {
-	    print_rtl_with_bb (stderr, get_insns (), TDF_BLOCKS | TDF_DETAILS);
-            error ("Region crossing jump across same section in bb %i",
-                   bb->index);
-            err = 1;
-          }
+      if (!has_crossing_edge
+	  && JUMP_P (BB_END (bb))
+	  && CROSSING_JUMP_P (BB_END (bb)))
+	{
+	  print_rtl_with_bb (stderr, get_insns (), TDF_BLOCKS | TDF_DETAILS);
+	  error ("Region crossing jump across same section in bb %i",
+		 bb->index);
+	  err = 1;
+	}
 
       if (n_eh && !find_reg_note (BB_END (bb), REG_EH_REGION, NULL_RTX))
 	{
@@ -2606,6 +2606,18 @@  rtl_verify_edges (void)
 	  error ("abnormal edges for no purpose in bb %i", bb->index);
 	  err = 1;
 	}
+      if (bb_has_eh_pred (bb))
+	{
+	  FOR_EACH_EDGE (e, ei, bb->preds)
+	    {
+	      if (e->flags & EDGE_COMPLEX)
+		continue;
+	      error ("EH incoming edge mixed with non-complex incoming edges "
+		     "in bb %i", bb->index);
+	      err = 1;
+	      break;
+	    }
+	}
     }
 
   /* If there are partitions, do a sanity check on them: A basic block in