Proposed patch for PR 7507 and PR 9289

["Christian Ehrhardt" <ehrhardt at mathematik dot uni-ulm dot de>]

Hi,

the following patch will fix two high priority PRs namely 9289 and 7507.
This bug is triggered by the linux kernel with recent 3.4. I'll start with
the patch (bootstraped and regtested on i686-pc-linux-gnu), the analysis
of the problem is below. Someone please take care of the patch and check
it in if appropriate as I don't have cvs access.

Index: gcc/gcc/calls.c
===================================================================
RCS file: /cvsroot/gcc/gcc/gcc/calls.c,v
retrieving revision 1.248
diff -u -r1.248 calls.c
--- gcc/gcc/calls.c	10 Jan 2003 19:49:09 -0000	1.248
+++ gcc/gcc/calls.c	17 Jan 2003 13:33:01 -0000
@@ -2068,6 +2068,35 @@
   return insn != NULL_RTX;
 }
 
+static tree
+fix_unsafe_tree (t)
+     tree t;
+{
+  switch (unsafe_for_reeval (t))
+    {
+    case 0: /* Safe.  */
+      break;
+
+    case 1: /* Mildly unsafe.  */
+      t = unsave_expr (t);
+      break;
+
+    case 2: /* Wildly unsafe.  */
+      {
+	tree var = build_decl (VAR_DECL, NULL_TREE,
+			       TREE_TYPE (t));
+	SET_DECL_RTL (var,
+		      expand_expr (t, NULL_RTX, VOIDmode, EXPAND_NORMAL));
+	t = var;
+      }
+      break;
+
+    default:
+      abort ();
+    }
+  return t;
+}
+
 /* Generate all the code for a function call
    and return an rtx for its value.
    Store the value in TARGET (specified as an rtx) if convenient.
@@ -2516,35 +2545,16 @@
 
       for (; i != end; i += inc)
 	{
-	  switch (unsafe_for_reeval (args[i].tree_value))
-	    {
-	    case 0: /* Safe.  */
-	      break;
-
-	    case 1: /* Mildly unsafe.  */
-	      args[i].tree_value = unsave_expr (args[i].tree_value);
-	      break;
-
-	    case 2: /* Wildly unsafe.  */
-	      {
-		tree var = build_decl (VAR_DECL, NULL_TREE,
-				       TREE_TYPE (args[i].tree_value));
-		SET_DECL_RTL (var,
-			      expand_expr (args[i].tree_value, NULL_RTX,
-					   VOIDmode, EXPAND_NORMAL));
-		args[i].tree_value = var;
-	      }
-	      break;
-
-	    default:
-	      abort ();
-	    }
+          args[i].tree_value = fix_unsafe_tree (args[i].tree_value);
 	  /* We need to build actparms for optimize_tail_recursion.  We can
 	     safely trash away TREE_PURPOSE, since it is unused by this
 	     function.  */
 	  if (try_tail_recursion)
 	    actparms = tree_cons (NULL_TREE, args[i].tree_value, actparms);
 	}
+      /* Do the same for the function address if it is an expression. */
+      if (!fndecl)
+        TREE_OPERAND (exp, 0) = fix_unsafe_tree (TREE_OPERAND (exp, 0));
       /* Expanding one of those dangerous arguments could have added
 	 cleanups, but otherwise give it a whirl.  */
       if (any_pending_cleanups (1))

The problem arises if gcc generates a call placeholder and the address
of the called function is a complicated expression. The problem is that
this expression will be evaluated at least twice, once in each alternative
of the call placeholder. Most of the time this doesn't hurt but e.g. if
the code contains labels/jumps the label will only be emitted for one of the
alternatives. The other alternative(s) will still jump to that label though.
This leads to call placeholder insns like this one (from x.c.00.rtl):

(call_insn 34 6 36 (nil) (call_placeholder 23 8 0 0 (cond [
  (const_string "normal") (sequence [        <=== first alternative starts here
    (note 23 0 24 0x400174a4 NOTE_INSN_BLOCK_BEG)
    (note 24 23 26 NOTE_INSN_DELETED)
    (note 26 24 28 [ 20 0 ]  NOTE_INSN_PREDICTION)
    (jump_insn 28 26 29 (nil) (set (pc)
                (label_ref 12)) -1 (nil)         <===== jump to label 12
            (nil))
    (barrier 29 28 30)
    (note 30 29 31 0x400174a4 NOTE_INSN_BLOCK_END)
    (insn 31 30 32 (nil) (set (reg/f:SI 61)
                (const_int 0 [0x0])) -1 (nil)
            (expr_list:REG_EQUAL (const_int 0 [0x0])
                (nil)))
    (insn 32 31 33 (nil) (set (reg/f:SI 62)
                (mem/s:SI (reg/f:SI 61) [3 <variable>.notifier+0 S4 A32])) -1 (nil)
            (nil))
    (call_insn 33 32 0 (nil) (set (reg:SI 0 eax)
                (call (mem:QI (reg/f:SI 62) [0 S1 A8])
                    (const_int 0 [0x0]))) -1 (nil)
            (nil)
            (nil))
    ])
  (const_string "tail_call") (sequence [ <=== second alternative starts here
    (note 8 0 9 NOTE_INSN_DELETED)
    (note 9 8 10 NOTE_INSN_DELETED)
    (note 10 9 11 0x40017c08 NOTE_INSN_BLOCK_BEG)
    (note 11 10 12 NOTE_INSN_DELETED)
    (code_label 12 11 14 2 ("start") [0 uses])     <==== label 12 is here
    (note 14 12 16 [ 20 0 ]  NOTE_INSN_PREDICTION)
    (jump_insn 16 14 17 (nil) (set (pc)
                (label_ref 12)) -1 (nil)
            (nil))
    (barrier 17 16 18)
    (note 18 17 19 0x40017c08 NOTE_INSN_BLOCK_END)
    (insn 19 18 20 (nil) (set (reg/f:SI 59)
                (const_int 0 [0x0])) -1 (nil)
            (expr_list:REG_EQUAL (const_int 0 [0x0])
                (nil)))
    (insn 20 19 21 (nil) (set (reg/f:SI 60)
                (mem/s:SI (reg/f:SI 59) [3 <variable>.notifier+0 S4 A32])) -1 (nil)
            (nil))
    (call_insn/j 21 20 22 (nil) (set (reg:SI 0 eax)
                (call (mem:QI (reg/f:SI 60) [0 S1 A8])
                    (const_int 0 [0x0]))) -1 (nil)
            (nil)
            (nil))
    (barrier 22 21 0)
    ])
  ])) -1 (nil)
    (nil)
    (nil))

This problem doesn't show with complicated argument expressions because
these are expanded in advance if needed. The patch does the same thing for
function pointers. A new function is introduced to avoid code duplication.

If the call placeholder is replaced with the normal (first) alternative
later on, find_basic_blocks (or actually cached_make_edge) will ICE
because it tries to add an edge from the current block to the block of
the target label 12. However, this label wasn't emitted at all and
hence doesn't have a basic block structure associated with it.

    regards  Christian

-- 
THAT'S ALL FOLKS!