* Proposed patch for PR 7507 and PR 9289
@ 2003-01-17 22:56 Christian Ehrhardt
2003-01-17 23:18 ` Dale Johannesen
2003-01-21 17:21 ` Richard Henderson
0 siblings, 2 replies; 7+ messages in thread
From: Christian Ehrhardt @ 2003-01-17 22:56 UTC (permalink / raw)
To: gcc, gcc-bugs, gcc-patches
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!
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Proposed patch for PR 7507 and PR 9289
2003-01-17 22:56 Proposed patch for PR 7507 and PR 9289 Christian Ehrhardt
@ 2003-01-17 23:18 ` Dale Johannesen
2003-01-18 7:59 ` Christian Ehrhardt
2003-01-21 17:21 ` Richard Henderson
1 sibling, 1 reply; 7+ messages in thread
From: Dale Johannesen @ 2003-01-17 23:18 UTC (permalink / raw)
To: Christian Ehrhardt; +Cc: Dale Johannesen, gcc, gcc-bugs, gcc-patches
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Proposed patch for PR 7507 and PR 9289
2003-01-17 23:18 ` Dale Johannesen
@ 2003-01-18 7:59 ` Christian Ehrhardt
2003-01-18 9:19 ` Dale Johannesen
0 siblings, 1 reply; 7+ messages in thread
From: Christian Ehrhardt @ 2003-01-18 7:59 UTC (permalink / raw)
To: Dale Johannesen; +Cc: gcc, gcc-bugs, gcc-patches
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!
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Proposed patch for PR 7507 and PR 9289
2003-01-18 7:59 ` Christian Ehrhardt
@ 2003-01-18 9:19 ` Dale Johannesen
2003-01-19 15:28 ` Alexandre Oliva
0 siblings, 1 reply; 7+ messages in thread
From: Dale Johannesen @ 2003-01-18 9:19 UTC (permalink / raw)
To: Christian Ehrhardt; +Cc: Dale Johannesen, gcc, gcc-bugs, gcc-patches
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Proposed patch for PR 7507 and PR 9289
2003-01-18 9:19 ` Dale Johannesen
@ 2003-01-19 15:28 ` Alexandre Oliva
0 siblings, 0 replies; 7+ messages in thread
From: Alexandre Oliva @ 2003-01-19 15:28 UTC (permalink / raw)
To: Dale Johannesen; +Cc: Christian Ehrhardt, gcc, gcc-bugs, gcc-patches
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Proposed patch for PR 7507 and PR 9289
2003-01-17 22:56 Proposed patch for PR 7507 and PR 9289 Christian Ehrhardt
2003-01-17 23:18 ` Dale Johannesen
@ 2003-01-21 17:21 ` Richard Henderson
2003-01-21 18:24 ` Christian Ehrhardt
1 sibling, 1 reply; 7+ messages in thread
From: Richard Henderson @ 2003-01-21 17:21 UTC (permalink / raw)
To: Christian Ehrhardt; +Cc: gcc, gcc-bugs, gcc-patches
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~
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Proposed patch for PR 7507 and PR 9289
2003-01-21 17:21 ` Richard Henderson
@ 2003-01-21 18:24 ` Christian Ehrhardt
0 siblings, 0 replies; 7+ messages in thread
From: Christian Ehrhardt @ 2003-01-21 18:24 UTC (permalink / raw)
To: Richard Henderson, gcc, gcc-bugs, gcc-patches
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!
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2003-01-21 13:14 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2003-01-17 22:56 Proposed patch for PR 7507 and PR 9289 Christian Ehrhardt
2003-01-17 23:18 ` Dale Johannesen
2003-01-18 7:59 ` Christian Ehrhardt
2003-01-18 9:19 ` Dale Johannesen
2003-01-19 15:28 ` Alexandre Oliva
2003-01-21 17:21 ` Richard Henderson
2003-01-21 18:24 ` Christian Ehrhardt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).