Re: middle-end/9997: Coelesce stack slots for disjoint scopes.

[Jim Wilson <wilson at tuliptree dot org>]

The following reply was made to PR middle-end/9997; it has been noted by GNATS.

From: Jim Wilson <wilson@tuliptree.org>
To: Daniel Jacobowitz <drow@mvista.com>
Cc: gcc-gnats@gcc.gnu.org
Subject: Re: middle-end/9997: Coelesce stack slots for disjoint scopes.
Date: 29 Apr 2003 23:12:45 -0400

 --=-CssSwvW0ZmvCLU9WCSS5
 Content-Type: text/plain
 Content-Transfer-Encoding: 7bit
 
 On Sun, 2003-03-09 at 15:45, Daniel Jacobowitz wrote:
 > On Sun, Mar 09, 2003 at 11:32:47AM -0500, Jim Wilson wrote:
 > The patch is below; however, it causes one new failure:
 > FAIL: g++.dg/opt/alias2.C execution test
 
 This is failing because we have two objects on the stack, which are
 allocated to overlapping slots, and which have overlapping lifetimes.
 When we try to copy fields from one to the other, we accidentally
 clobber a field in the first one before we read it.
 
 This is obvious using the 2003-04-14 snapshot.  I get:
        movl    -36(%ebp), %eax
        movl    %eax, -24(%ebp)
        /* movl  -24(%ebp), %eax deleted by optimizer as redundant */
        movl    %eax, -12(%ebp)
 Note that the objects are 12 bytes apart, and we are copying two fields
 which are 12 bytes apart.  Thus the first copy clobbers the source of
 the second copy before we read it.
 
 The same problem exists in current sources, but is less obvious, because
 the two objects are not 12 bytes apart, and thus they still overlap, but
 it works by accident.
         movl    -36(%ebp), %eax
         movl    %eax, -40(%ebp)
         movl    -24(%ebp), %eax
         movl    %eax, -28(%ebp)
 
 The failure occurs because we have a TARGET_EXPR with a cleanup.  In
 genrtl_stmt_expr_value, we call expand_start_target_temps which calls
 push_temp_slots.  The current temp_slot_level is stored in
 target_temp_slot_level.  Then we call expand_stmt_expr_value which
 evaluates the stmt expr.  Along the way, we evaluate a TARGET_EXPR which
 creates a stack level at target_temp_slot_level, and gives it a cleanup.
 When we get to the end of expand_stmt_expr_value, we call
 free_temp_slots.  Since there were no push_temp_slots calls along the
 way, target_temp_slot_level == temp_slot_level, and this frees the slot
 allocated to the TARGET_EXPR.  Then when we call the cleanup
 (destructor), we are reading from a freed stack slot, and we have a
 problem.
 
 It appears that we need another set of push_temp_slot/pop_temp_slot
 calls here, or else we need to mark the TARGET_EXPR stack slot as kept
 until the end of the binding level.  I have mostly convinced myself that
 marking it as kept is the right solution.
 
 It appears that this was broken when the C++ front end was converted to
 generate an entire function as a tree.  This was such a large change
 that I don't think it makes any sense to try to track down exactly why
 or how this got broken.
 
 This gives me the following patch.  I haven't tried testing it yet,
 other than against the alias2.C testcase.  Since this has one part that
 reduces stack space usage and one part that increases stack space usage,
 we need to check to see what kind of effect this has on C++
 performance.  Hopefully it doesn't make things worse.  It seems OK for
 the alias2.C testcase.  Main uses the same stack space before and after
 the patch.
 
 Jim
 
 
 --=-CssSwvW0ZmvCLU9WCSS5
 Content-Disposition: attachment; filename=tmp.file.2
 Content-Transfer-Encoding: quoted-printable
 Content-Type: text/x-patch; name=tmp.file.2; charset=UTF-8
 
 Index: function.c
 =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
 =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
 =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
 RCS file: /cvs/gcc/gcc/gcc/function.c,v
 retrieving revision 1.421
 diff -p -r1.421 function.c
 *** function.c	22 Apr 2003 12:09:09 -0000	1.421
 --- function.c	30 Apr 2003 05:36:43 -0000
 *************** assign_stack_temp_for_type (mode, size,=20
 *** 796,802 ****
     if (keep =3D=3D 2)
       {
         p->level =3D target_temp_slot_level;
 !       p->keep =3D 0;
       }
     else if (keep =3D=3D 3)
       {
 --- 796,802 ----
     if (keep =3D=3D 2)
       {
         p->level =3D target_temp_slot_level;
 !       p->keep =3D 1;
       }
     else if (keep =3D=3D 3)
       {
 *************** preserve_temp_slots (x)
 *** 1134,1139 ****
 --- 1134,1143 ----
  =20
         return;
       }
 +=20
 +   /* If X is a constant, then we don't need to preserve stack slots.=C2=
 =A0 */
 +   if (CONSTANT_P (x))
 +     return;
  =20
     /* If X is a register that is being used as a pointer, see if we have
        a temporary slot we know it points to.  To be consistent with
 
 --=-CssSwvW0ZmvCLU9WCSS5--