This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
[PATCH] Fix middle-end/15054 (overlapping stack temporaries)
- From: Ulrich Weigand <weigand at i1 dot informatik dot uni-erlangen dot de>
- To: gcc-patches at gcc dot gnu dot org
- Date: Thu, 29 Apr 2004 15:36:19 +0200 (CEST)
- Subject: [PATCH] Fix middle-end/15054 (overlapping stack temporaries)
Hello,
the following patch fixes PR middle-end/15054, a bug caused by
overlapping stack temporaries.
The sequence of events leading to the overlap in test case
(also appended below) was as follows:
- When expanding the call
insert (element());
in main (), the C++ front end uses a TARGET_EXPR to encapsulate
the temporary 'element' object whose address is bound to a
reference.
- In expanding the call, the middle end would use
expand_start_target_temps to set up a binding representing the
live range of the temporary.
This increases the temp_slot_level from 2 to 4, and subsequently
sets the target_temp_slot_level to 4 as well.
- Due to inline function expansion, some other expressions are
now expanded first, before expanding the TARGET_EXPR itself.
In the process of these expansions, the temp_slot_level is
now increased to 7.
- At this point the TARGET_EXPR is expanded, creating a temporary
via this code snippet in expand_expr_real:
target = assign_temp (type, 2, 0, 1);
/* All temp slots at this level must not conflict. */
preserve_temp_slots (target);
Note that due to the argument keep = 2 of assign_temp, the new
temporary is put to temp level 4 (target_temp_slot_level) instead
of the current level 7.
Since preserve_temp_slots only affects temps of the current level,
this means that the new TARGET_EXPR temp remains at level 4 even
after the preserve_temp_slots call.
- The rest of the inline function expansion now proceeds, finally
winding down the temp slot level back to 4.
- At the end of expanding the function call, expand_expr_stmt_value does:
/* If this expression is part of a ({...}) and is in memory, we may have
to preserve temporaries. */
preserve_temp_slots (value);
/* Free any temporaries used to evaluate this expression. Any temporary
used as a result of this expression will already have been preserved
above. */
free_temp_slots ();
This free_temp_slots call frees all temporaries of level 4, including
the temporary 'element' object slot. Note that this is not prevented
by the preserve_temp_slots call as 'value' represents the returned
'pointer' object, and its level was already decremented to 3 by a
previous preserve_temp_slots call (at a time the 'element' temporary
didn't even exist yet) -- thus preserve_temp_slots does nothing.
The 'element' temporary slot is thus now marked as not-in-use.
- The middle-end now calls expand_end_target_temps to close the binding
level representing the temporary object; this in turn now expands
the 'element' destructor on the temporary.
- In the course of expanding the destructor, a new temporary is required.
The temp-slot reuse logic finds that the slot originally holding the
'element' temporary is marked as not-in-use, and reuses it for the new
temporary.
So far, so good. The question is now, how to fix this. The way I propose
to do this is based on the assumption that the intention of the
expand_start_target_temps / expand_end_target_temps pair is to ensure
any TARGET_EXPR temp expanded within lives until the end of the cleanups
expanded in expand_end_target_temps.
The way to ensure that is to enforce that TARGET_EXPR temps get allocated
at a temp slot level *lower* than the one in effect immediately after
expand_start_target_temps returns.
Apparently, the preserve_temp_slots call during TARGET_EXPR expansion
tries to achieve this, but it fails whenever temp_slot_level is already
higher than target_temp_slot_level at this point.
Thus, I propose to remove this preserve_temp_slots call, and instead
set target_temp_slot_level to a lower level right from the start.
Now, the way target_temp_slot_level is set is:
expand_start_bindings (...);
target_temp_slot_level = temp_slot_level;
The expand_start_bindings routine remembers the old target_temp_slot_level,
so that it can be restored in expand_end_bindings. Also, it increments
the temp_slot_level.
Therefore, I cannot simply swap the two lines, because then the restore
wouldn't work any more.
Thus I propose to add a new flag to expand_start_bindings that would
tell it to set the target_temp_slot_level *after* saving the old
target_temp_slot_level, but *before* incrementing the temp_slot_level.
This guarantees that TARGET_EXPR temporaries will in any case live until
after all cleanup actions in expand_end_bindings are done.
Bootstrapped/regtested on s390-ibm-linux and s390x-ibm-linux.
OK?
Bye,
Ulrich
ChangeLog:
PR middle-end/15054
* expr.c (expand_expr_real): Do not set target_temp_slot_level,
call expand_start_bindings with flag 4 set instead.
Do not call preserve_temp_slots on a TARGET_EXPR temp.
* stmt.c (expand_start_bindings_and_block): New flag 4 to
lift target_temp_slot_level to current level.
(expand_start_target_temps): Do not set target_temp_slot_level,
call expand_start_bindings with flag 4 set instead.
testsuite/ChangeLog:
PR middle-end/15054
* g++.dg/opt/pr15054.C: New test.
Index: gcc/expr.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/expr.c,v
retrieving revision 1.639
diff -c -p -r1.639 expr.c
*** gcc/expr.c 25 Apr 2004 23:52:13 -0000 1.639
--- gcc/expr.c 29 Apr 2004 01:58:25 -0000
*************** expand_expr_real (tree exp, rtx target,
*** 7384,7392 ****
{
/* Start a new binding layer that will keep track of all cleanup
actions to be performed. */
! expand_start_bindings (2);
!
! target_temp_slot_level = temp_slot_level;
op0 = expand_expr (TREE_OPERAND (exp, 0), target, tmode, modifier);
/* If we're going to use this value, load it up now. */
--- 7384,7390 ----
{
/* Start a new binding layer that will keep track of all cleanup
actions to be performed. */
! expand_start_bindings (6);
op0 = expand_expr (TREE_OPERAND (exp, 0), target, tmode, modifier);
/* If we're going to use this value, load it up now. */
*************** expand_expr_real (tree exp, rtx target,
*** 8525,8532 ****
else
{
target = assign_temp (type, 2, 0, 1);
- /* All temp slots at this level must not conflict. */
- preserve_temp_slots (target);
SET_DECL_RTL (slot, target);
if (TREE_ADDRESSABLE (slot))
put_var_into_stack (slot, /*rescan=*/false);
--- 8523,8528 ----
*************** expand_expr_real (tree exp, rtx target,
*** 8941,8948 ****
/* Start a new binding layer that will keep track of all cleanup
actions to be performed. */
! expand_start_bindings (2);
! target_temp_slot_level = temp_slot_level;
expand_decl_cleanup (NULL_TREE, cleanup);
op0 = expand_expr (try_block, target, tmode, modifier);
--- 8937,8943 ----
/* Start a new binding layer that will keep track of all cleanup
actions to be performed. */
! expand_start_bindings (6);
expand_decl_cleanup (NULL_TREE, cleanup);
op0 = expand_expr (try_block, target, tmode, modifier);
*************** expand_expr_real (tree exp, rtx target,
*** 8957,8964 ****
}
else
{
! expand_start_bindings (2);
! target_temp_slot_level = temp_slot_level;
expand_decl_cleanup (NULL_TREE, finally_block);
op0 = expand_expr (try_block, target, tmode, modifier);
--- 8952,8958 ----
}
else
{
! expand_start_bindings (6);
expand_decl_cleanup (NULL_TREE, finally_block);
op0 = expand_expr (try_block, target, tmode, modifier);
Index: gcc/stmt.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/stmt.c,v
retrieving revision 1.353
diff -c -p -r1.353 stmt.c
*** gcc/stmt.c 12 Apr 2004 21:25:45 -0000 1.353
--- gcc/stmt.c 29 Apr 2004 01:58:26 -0000
*************** tail_recursion_args (tree actuals, tree
*** 3391,3396 ****
--- 3391,3401 ----
and BLOCKs.) If this flag is set, MARK_ENDS should be zero
when expand_end_bindings is called.
+ 4 - Nonzero if the temporary level of TARGET_EXPRs should be
+ increased to the current level. This has the effect of
+ preserving all temporaries created via TARGET_EXPR until
+ after expand_end_bindings has finished.
+
If we are creating a NOTE_INSN_BLOCK_BEG note, a BLOCK may
optionally be supplied. If so, it becomes the NOTE_BLOCK for the
note. */
*************** expand_start_bindings_and_block (int fla
*** 3402,3407 ****
--- 3407,3413 ----
rtx note;
int exit_flag = ((flags & 1) != 0);
int block_flag = ((flags & 2) == 0);
+ int target_temp_flag = ((flags & 4) != 0);
/* If a BLOCK is supplied, then the caller should be requesting a
NOTE_INSN_BLOCK_BEG note. */
*************** expand_start_bindings_and_block (int fla
*** 3454,3459 ****
--- 3460,3469 ----
block_stack = thisblock;
nesting_stack = thisblock;
+ /* Increase target temp level if requested. */
+ if (target_temp_flag)
+ target_temp_slot_level = temp_slot_level;
+
/* Make a new level for allocating stack slots. */
push_temp_slots ();
}
*************** expand_start_target_temps (void)
*** 3473,3481 ****
/* Start a new binding layer that will keep track of all cleanup
actions to be performed. */
! expand_start_bindings (2);
!
! target_temp_slot_level = temp_slot_level;
}
void
--- 3483,3489 ----
/* Start a new binding layer that will keep track of all cleanup
actions to be performed. */
! expand_start_bindings (6);
}
void
*** /dev/null Mon Mar 15 14:37:15 2004
--- gcc/testsuite/g++.dg/opt/pr15054.C Thu Apr 29 04:06:20 2004
***************
*** 0 ****
--- 1,36 ----
+ // PR middle-end/15054
+ // This used to abort due to overlapping stack temporaries.
+
+ // { dg-do run }
+ // { dg-options "-O" }
+
+ extern "C" void abort (void);
+
+ struct pointer
+ {
+ void* ptr;
+
+ pointer(void* x = 0) : ptr(x) {}
+ pointer(const pointer& x) : ptr(x.ptr) {}
+ };
+
+ struct element
+ {
+ int canary;
+
+ element() : canary(123) { }
+ ~element() { pointer(); if (canary != 123) abort (); }
+ };
+
+ inline pointer
+ insert(const element& x)
+ {
+ return pointer(new element(x));
+ }
+
+ int
+ main (void)
+ {
+ insert(element());
+ return 0;
+ }
--
Dr. Ulrich Weigand
weigand@informatik.uni-erlangen.de