Fix nasty bug in reg-stack.c

[Eric Botcazou <ebotcazou at adacore dot com>]

The bug was introduced 6 years ago but its occurrences apparently are quite 
rare since we detected it only very recently in our 4.3-based Ada compiler.

The end of convert_regs reads:

  inserted |= compensate_edges ();

  clear_aux_for_blocks ();

  fixup_abnormal_edges ();
  if (inserted)
    commit_edge_insertions ();

There is a nasty ordering problem in these lines.  fixup_abnormal_edges is 
intended to fix up the CFG by moving insns that were inserted past the last 
insn in a BB before compensate_edges is run onto the fallthrough edge.  And
compensate_edges also inserts insns onto (fallthrough) edges.  Now inserting 
insns on edges is a FIFO process so, if an insn was inserted past the last 
insn in a BB and a compensation insn is inserted afterward on the fallthrough 
edge, then they end up being swapped after the call to fixup_abnormal_edges.

A possible fix would be to use in the earlier phases the same mechanism used in 
compensate_edges: inserting insns on edges directly (except for the case of a 
single successor).  This seems inappropriate at this point of the lifetime of 
the reg-stack.c code.  The attached fix moves the call to fixup_abnormal_edges 
up to before the call to compensate_edges instead; this in turn requires some 
changes to fixup_abnormal_edges itself, mostly the uncoupling of the fixup and 
the finalization codes.

Tested on i586-suse-linux, applied on the mainline.


2011-03-26  Eric Botcazou  <ebotcazou@adacore.com>

	* basic-block.h (fixup_abnormal_edges): Adjust prototype.
	* reload1.c (reload): Adjust call to fixup_abnormal_edges.  Rediscover
	basic blocks and call commit_edge_insertions directly.
	(fixup_abnormal_edges): Move from here to...
	* cfgrtl.c (fixup_abnormal_edges): ...here.  Only insert instructions
	on the edges and return whether some have actually been inserted.
	* reg-stack.c (convert_regs): Fix up abnormal edges before inserting
	compensation code.


-- 
Eric Botcazou

Index: basic-block.h
===================================================================
--- basic-block.h	(revision 171404)
+++ basic-block.h	(working copy)
@@ -798,6 +798,7 @@ extern basic_block force_nonfallthru (ed
 extern rtx block_label (basic_block);
 extern bool purge_all_dead_edges (void);
 extern bool purge_dead_edges (basic_block);
+extern bool fixup_abnormal_edges (void);
 
 /* In cfgbuild.c.  */
 extern void find_many_sub_basic_blocks (sbitmap);
@@ -814,7 +815,6 @@ extern bool delete_unreachable_blocks (v
 extern bool mark_dfs_back_edges (void);
 extern void set_edge_can_fallthru_flag (void);
 extern void update_br_prob_note (basic_block);
-extern void fixup_abnormal_edges (void);
 extern bool inside_basic_block_p (const_rtx);
 extern bool control_flow_insn_p (const_rtx);
 extern rtx get_last_bb_insn (basic_block);
Index: reload1.c
===================================================================
--- reload1.c	(revision 171404)
+++ reload1.c	(working copy)
@@ -721,6 +721,7 @@ reload (rtx first, int global)
   rtx insn;
   struct elim_table *ep;
   basic_block bb;
+  bool inserted;
 
   /* Make sure even insns with volatile mem refs are recognizable.  */
   init_recog ();
@@ -1299,7 +1300,21 @@ reload (rtx first, int global)
   /* Free all the insn_chain structures at once.  */
   obstack_free (&reload_obstack, reload_startobj);
   unused_insn_chains = 0;
-  fixup_abnormal_edges ();
+
+  inserted = fixup_abnormal_edges ();
+
+  /* We've possibly turned single trapping insn into multiple ones.  */
+  if (cfun->can_throw_non_call_exceptions)
+    {
+      sbitmap blocks;
+      blocks = sbitmap_alloc (last_basic_block);
+      sbitmap_ones (blocks);
+      find_many_sub_basic_blocks (blocks);
+      sbitmap_free (blocks);
+    }
+
+  if (inserted)
+    commit_edge_insertions ();
 
   /* Replacing pseudos with their memory equivalents might have
      created shared rtx.  Subsequent passes would get confused
@@ -9112,112 +9127,3 @@ add_auto_inc_notes (rtx insn, rtx x)
     }
 }
 #endif
-
-/* This is used by reload pass, that does emit some instructions after
-   abnormal calls moving basic block end, but in fact it wants to emit
-   them on the edge.  Looks for abnormal call edges, find backward the
-   proper call and fix the damage.
-
-   Similar handle instructions throwing exceptions internally.  */
-void
-fixup_abnormal_edges (void)
-{
-  bool inserted = false;
-  basic_block bb;
-
-  FOR_EACH_BB (bb)
-    {
-      edge e;
-      edge_iterator ei;
-
-      /* Look for cases we are interested in - calls or instructions causing
-         exceptions.  */
-      FOR_EACH_EDGE (e, ei, bb->succs)
-	{
-	  if (e->flags & EDGE_ABNORMAL_CALL)
-	    break;
-	  if ((e->flags & (EDGE_ABNORMAL | EDGE_EH))
-	      == (EDGE_ABNORMAL | EDGE_EH))
-	    break;
-	}
-      if (e && !CALL_P (BB_END (bb))
-	  && !can_throw_internal (BB_END (bb)))
-	{
-	  rtx insn;
-
-	  /* Get past the new insns generated.  Allow notes, as the insns
-	     may be already deleted.  */
-	  insn = BB_END (bb);
-	  while ((NONJUMP_INSN_P (insn) || NOTE_P (insn))
-		 && !can_throw_internal (insn)
-		 && insn != BB_HEAD (bb))
-	    insn = PREV_INSN (insn);
-
-	  if (CALL_P (insn) || can_throw_internal (insn))
-	    {
-	      rtx stop, next;
-
-	      stop = NEXT_INSN (BB_END (bb));
-	      BB_END (bb) = insn;
-	      insn = NEXT_INSN (insn);
-
-	      e = find_fallthru_edge (bb->succs);
-
-	      while (insn && insn != stop)
-		{
-		  next = NEXT_INSN (insn);
-		  if (INSN_P (insn))
-		    {
-		      delete_insn (insn);
-
-		      /* Sometimes there's still the return value USE.
-			 If it's placed after a trapping call (i.e. that
-			 call is the last insn anyway), we have no fallthru
-			 edge.  Simply delete this use and don't try to insert
-			 on the non-existent edge.  */
-		      if (GET_CODE (PATTERN (insn)) != USE)
-			{
-			  /* We're not deleting it, we're moving it.  */
-			  INSN_DELETED_P (insn) = 0;
-			  PREV_INSN (insn) = NULL_RTX;
-			  NEXT_INSN (insn) = NULL_RTX;
-
-			  insert_insn_on_edge (insn, e);
-			  inserted = true;
-			}
-		    }
-		  else if (!BARRIER_P (insn))
-		    set_block_for_insn (insn, NULL);
-		  insn = next;
-		}
-	    }
-
-	  /* It may be that we don't find any such trapping insn.  In this
-	     case we discovered quite late that the insn that had been
-	     marked as can_throw_internal in fact couldn't trap at all.
-	     So we should in fact delete the EH edges out of the block.  */
-	  else
-	    purge_dead_edges (bb);
-	}
-    }
-
-  /* We've possibly turned single trapping insn into multiple ones.  */
-  if (cfun->can_throw_non_call_exceptions)
-    {
-      sbitmap blocks;
-      blocks = sbitmap_alloc (last_basic_block);
-      sbitmap_ones (blocks);
-      find_many_sub_basic_blocks (blocks);
-      sbitmap_free (blocks);
-    }
-
-  if (inserted)
-    commit_edge_insertions ();
-
-#ifdef ENABLE_CHECKING
-  /* Verify that we didn't turn one trapping insn into many, and that
-     we found and corrected all of the problems wrt fixups on the
-     fallthru edge.  */
-  verify_flow_info ();
-#endif
-}
Index: cfgrtl.c
===================================================================
--- cfgrtl.c	(revision 171404)
+++ cfgrtl.c	(working copy)
@@ -30,6 +30,8 @@ along with GCC; see the file COPYING3.
 	 insert_insn_on_edge, commit_edge_insertions
      - CFG updating after insn simplification
 	 purge_dead_edges, purge_all_dead_edges
+     - CFG fixing after coarse manipulation
+	fixup_abnormal_edges
 
    Functions not supposed for generic use:
      - Infrastructure to determine quickly basic block for insn
@@ -2471,6 +2473,95 @@ purge_all_dead_edges (void)
   return purged;
 }
 
+/* This is used by a few passes that emit some instructions after abnormal
+   calls, moving the basic block's end, while they in fact do want to emit
+   them on the fallthru edge.  Look for abnormal call edges, find backward
+   the call in the block and insert the instructions on the edge instead.
+
+   Similarly, handle instructions throwing exceptions internally.
+
+   Return true when instructions have been found and inserted on edges.  */
+
+bool
+fixup_abnormal_edges (void)
+{
+  bool inserted = false;
+  basic_block bb;
+
+  FOR_EACH_BB (bb)
+    {
+      edge e;
+      edge_iterator ei;
+
+      /* Look for cases we are interested in - calls or instructions causing
+         exceptions.  */
+      FOR_EACH_EDGE (e, ei, bb->succs)
+	if ((e->flags & EDGE_ABNORMAL_CALL)
+	    || ((e->flags & (EDGE_ABNORMAL | EDGE_EH))
+		== (EDGE_ABNORMAL | EDGE_EH)))
+	  break;
+
+      if (e && !CALL_P (BB_END (bb)) && !can_throw_internal (BB_END (bb)))
+	{
+	  rtx insn;
+
+	  /* Get past the new insns generated.  Allow notes, as the insns
+	     may be already deleted.  */
+	  insn = BB_END (bb);
+	  while ((NONJUMP_INSN_P (insn) || NOTE_P (insn))
+		 && !can_throw_internal (insn)
+		 && insn != BB_HEAD (bb))
+	    insn = PREV_INSN (insn);
+
+	  if (CALL_P (insn) || can_throw_internal (insn))
+	    {
+	      rtx stop, next;
+
+	      e = find_fallthru_edge (bb->succs);
+
+	      stop = NEXT_INSN (BB_END (bb));
+	      BB_END (bb) = insn;
+
+	      for (insn = NEXT_INSN (insn); insn != stop; insn = next)
+		{
+		  next = NEXT_INSN (insn);
+		  if (INSN_P (insn))
+		    {
+		      delete_insn (insn);
+
+		      /* Sometimes there's still the return value USE.
+			 If it's placed after a trapping call (i.e. that
+			 call is the last insn anyway), we have no fallthru
+			 edge.  Simply delete this use and don't try to insert
+			 on the non-existent edge.  */
+		      if (GET_CODE (PATTERN (insn)) != USE)
+			{
+			  /* We're not deleting it, we're moving it.  */
+			  INSN_DELETED_P (insn) = 0;
+			  PREV_INSN (insn) = NULL_RTX;
+			  NEXT_INSN (insn) = NULL_RTX;
+
+			  insert_insn_on_edge (insn, e);
+			  inserted = true;
+			}
+		    }
+		  else if (!BARRIER_P (insn))
+		    set_block_for_insn (insn, NULL);
+		}
+	    }
+
+	  /* It may be that we don't find any trapping insn.  In this
+	     case we discovered quite late that the insn that had been
+	     marked as can_throw_internal in fact couldn't trap at all.
+	     So we should in fact delete the EH edges out of the block.  */
+	  else
+	    purge_dead_edges (bb);
+	}
+    }
+
+  return inserted;
+}
+
 /* Same as split_block but update cfg_layout structures.  */
 
 static basic_block
Index: reg-stack.c
===================================================================
--- reg-stack.c	(revision 171404)
+++ reg-stack.c	(working copy)
@@ -3150,11 +3150,14 @@ convert_regs (void)
 	cfg_altered |= convert_regs_2 (b);
     }
 
+  /* We must fix up abnormal edges before inserting compensation code
+     because both mechanisms insert insns on edges.  */
+  inserted |= fixup_abnormal_edges ();
+
   inserted |= compensate_edges ();
 
   clear_aux_for_blocks ();
 
-  fixup_abnormal_edges ();
   if (inserted)
     commit_edge_insertions ();