Proposed patch for PR 7507 and PR 9289

Proposed patch for PR 7507 and PR 9289

[Christian Ehrhardt]

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!

Re: Proposed patch for PR 7507 and PR 9289

[Dale Johannesen]

On Friday, January 17, 2003, at 10:19  AM, Christian Ehrhardt wrote:
> 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 is (at least) the 3rd bug like this that has shown up.  Might it 
be a
good idea to rewrite the low-level label infrastructure to allow
duplicate labels in the placeholder alternatives?  That might be 
stabler.

Re: Proposed patch for PR 7507 and PR 9289

[Christian Ehrhardt]

On Fri, Jan 17, 2003 at 10:31:34AM -0800, Dale Johannesen wrote:
> This is (at least) the 3rd bug like this that has shown up.  Might it 
> be a
> good idea to rewrite the low-level label infrastructure to allow
> duplicate labels in the placeholder alternatives?  That might be 
> stabler.

Assume that we actually allow labels in a call placeholder.
This would mean that we generate different CODE_LABEL insns for
each alternative in the call placeholder because same insn can't
be in different chains. As long as the only jumps to the label are
from within the same call placeholder this is fine but there may
well be jumps from outside the call placeholder. These jumps would
have a hard time to figure out what the label number is that they
should jump to. Think of code like this:

struct task_struct {
        int (*notifier)(void);
};
int __dequeue_signal(void )
{
        goto start;
        (
                ({
                        start:
                        (struct task_struct *) 0x4711;
                        goto start;
                })->notifier
        )();
        return 0;
}

This would mean that we'd have to attach something like a LABEL_PLACEHOLDER
insn to the tree. This insn would contain all real CODE_LABEL insns
emitted for the label in different chains. This will upon another can
of worms. After all I don't think this approach is the right thing.

   regards   Christian

-- 
THAT'S ALL FOLKS!

Re: Proposed patch for PR 7507 and PR 9289

[Dale Johannesen]

On Friday, January 17, 2003, at 05:45  PM, Christian Ehrhardt wrote:

> On Fri, Jan 17, 2003 at 10:31:34AM -0800, Dale Johannesen wrote:
>> This is (at least) the 3rd bug like this that has shown up.  Might it
>> be a
>> good idea to rewrite the low-level label infrastructure to allow
>> duplicate labels in the placeholder alternatives?  That might be
>> stabler.
>
> Assume that we actually allow labels in a call placeholder.
> This would mean that we generate different CODE_LABEL insns for
> each alternative in the call placeholder because same insn can't
> be in different chains. As long as the only jumps to the label are
> from within the same call placeholder this is fine but there may
> well be jumps from outside the call placeholder. These jumps would
> have a hard time to figure out what the label number is that they
> should jump to....

I had in mind multiple labels with the same number (but different 
insns),
in different placeholders.  Exactly one of them would survive after the
placeholders are resolved.  If you can construct a case where one 
placeholder
contains a label jumped to from outside and another doesn't, that would 
break.
That's not possible, because choosing the placeholder without the label 
would
result in invalid RTL.

This was just an idea thrown out, I didn't intend it to block approval 
of your
patch.  I have no authority, but the patch looks correct to me.

Re: Proposed patch for PR 7507 and PR 9289

[Alexandre Oliva]

On Jan 18, 2003, Dale Johannesen <dalej@apple.com> wrote:

> I had in mind multiple labels with the same number (but different
> insns),

If they're different insns, jumps will still point to only one of
them, since the code_label in the label_ref is just a pointer to the
insn that defines the label.

-- 
Alexandre Oliva   Enjoy Guarana', see http://www.ic.unicamp.br/~oliva/
Red Hat GCC Developer                 aoliva@{redhat.com, gcc.gnu.org}
CS PhD student at IC-Unicamp        oliva@{lsd.ic.unicamp.br, gnu.org}
Free Software Evangelist                Professional serial bug killer

Re: Proposed patch for PR 7507 and PR 9289

[Richard Henderson]

On Fri, Jan 17, 2003 at 07:19:42PM +0100, Christian Ehrhardt wrote:
> the following patch will fix two high priority PRs namely 9289 and 7507.

Applied.


r~

Re: Proposed patch for PR 7507 and PR 9289

[Christian Ehrhardt]

On Tue, Jan 21, 2003 at 12:03:26AM -0800, Richard Henderson wrote:
> On Fri, Jan 17, 2003 at 07:19:42PM +0100, Christian Ehrhardt wrote:
> > the following patch will fix two high priority PRs namely 9289 and 7507.

Thanks. But shouldn't this go into 3.2 as well? It is marked as a
3.2 regression.

     regards   Christian Ehrhardt

-- 
THAT'S ALL FOLKS!