[PATCH] Fix middle-end/15054 (overlapping stack temporaries)

[Ulrich Weigand <weigand at i1 dot informatik dot uni-erlangen dot de>]

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