Bug 15054 - [3.4 Regression] Bad code due to overlapping stack temporaries
Summary: [3.4 Regression] Bad code due to overlapping stack temporaries
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: middle-end (show other bugs)
Version: 3.4.0
: P2 critical
Target Milestone: 3.4.1
Assignee: Not yet assigned to anyone
URL:
Keywords: wrong-code
Depends on:
Blocks:
 
Reported: 2004-04-21 19:13 UTC by Ulrich Weigand
Modified: 2004-10-30 21:10 UTC (History)
2 users (show)

See Also:
Host:
Target: s390-ibm-linux
Build:
Known to work: 3.0.4 tree-ssa 4.0.0
Known to fail: 3.3.3 3.2.3 3.4.0
Last reconfirmed: 2004-04-21 19:14:30


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Ulrich Weigand 2004-04-21 19:13:18 UTC
The testcase appended below triggers the abort,
because two temporaries that are concurrently
used get assigned the same stack slot; thus the
constructor of the 'pointer' temporary built
inside the 'element' destructor will clobber
the 'canary' member of the current 'element'.

To reproduce, build the file with -O (both
-O0 and -O2 work correctly).



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;
}
Comment 1 Andrew Pinski 2004-04-21 19:27:53 UTC
Confirmed. A regression from 3.0.4, it does not work in 3.2.3 either.
Comment 2 Andrew Pinski 2004-04-21 20:30:57 UTC
Note it is opimized down to the abort.

(insn 29 28 30 (parallel [
            (set (reg/v/f:SI 71 [ this ])
                (plus:SI (reg/f:SI 54 virtual-stack-vars)
                    (const_int -32 [0xffffffe0])))
            (clobber (reg:CC 17 flags))
        ]) -1 (nil)
    (nil))
(insn 34 33 35 (set (mem/s/j:SI (reg/v/f:SI 71 [ this ]) [0 <variable>.canary+0 S4 A32])
        (const_int 123 [0x7b])) -1 (nil)
    (nil))

(insn 41 40 42 (parallel [
            (set (reg/v/f:SI 70 [ x ])
                (plus:SI (reg/f:SI 54 virtual-stack-vars)
                    (const_int -32 [0xffffffe0])))
            (clobber (reg:CC 17 flags))
        ]) -1 (nil)
    (nil))

(insn 51 50 52 (parallel [
            (set (reg/v/f:SI 72 [ this ])
                (plus:SI (reg/f:SI 54 virtual-stack-vars)
                    (const_int -16 [0xfffffff0])))
            (clobber (reg:CC 17 flags))
        ]) -1 (nil)
    (nil))
(insn 52 51 53 (set (mem/i:SI (reg/f:SI 56 virtual-outgoing-args) [0 S4 A32])
        (const_int 4 [0x4])) -1 (nil)
    (nil))

Comment 3 Ulrich Weigand 2004-04-22 13:46:00 UTC
Why did you change the component to 'middle-end'?  The bug is already present
on the RTL level right after expand (this is on i386):

(insn 144 142 145 (parallel[
            (set (reg/v/f:SI 74)
                (plus:SI (reg/f:SI 54 virtual-stack-vars)
                    (const_int -32 [0xffffffe0])))
            (clobber (reg:CC 17 flags))
        ] ) -1 (nil)
    (nil))

(insn 156 153 158 (parallel[
            (set (reg/v/f:SI 75)
                (plus:SI (reg/f:SI 54 virtual-stack-vars)
                    (const_int -32 [0xffffffe0])))
            (clobber (reg:CC 17 flags))
        ] ) -1 (nil)
    (nil))

(insn 158 156 159 (set (reg/v/f:SI 76)
        (const_int 0 [0x0])) -1 (nil)
    (nil))

(insn 164 162 165 (set (mem/s/j:SI (reg/v/f:SI 75) [0 <variable>.ptr+0 S4 A32])
        (reg/v/f:SI 76)) -1 (nil)
    (nil))

(insn 169 167 170 (set (reg:CCZ 17 flags)
        (compare:CCZ (mem/s/j:SI (reg/v/f:SI 74) [0 <variable>.canary+0 S4 A32])
            (const_int 123 [0x7b]))) -1 (nil)
    (nil))

See how <variable>.ptr and <variable>.canary share the same stack slot?

That this is subsequently optimized away is only the logical conclusion,
and not a middle-end bug.  The bug is the overlapping stack slot assignment.
Comment 4 Andrew Pinski 2004-04-22 14:51:35 UTC
Because middle-end is where the conversion between trees to RTL happens.
"GCC's middle end; folks, expand_*, etc. Target dependent parts and optimization passes have their 
own component"  See how expand is part of it.
Comment 5 Ulrich Weigand 2004-04-29 19:50:24 UTC
See http://gcc.gnu.org/ml/gcc-patches/2004-04/msg01906.html for a potential fix (awaiting review).  
Comment 6 Andrew Pinski 2004-04-30 17:06:51 UTC
Note this is fixed on the tree-ssa as it is optimized before getting to the expand.
Comment 7 CVS Commits 2004-05-01 11:37:49 UTC
Subject: Bug 15054

CVSROOT:	/cvs/gcc
Module name:	gcc
Changes by:	uweigand@gcc.gnu.org	2004-05-01 11:37:39

Modified files:
	gcc            : ChangeLog expr.c function.c 
	gcc/testsuite  : ChangeLog 
Added files:
	gcc/testsuite/g++.dg/opt: pr15054.C 

Log message:
	PR middle-end/15054
	* expr.c (expand_expr_real): Do not call preserve_temp_slots
	on a TARGET_EXPR temp.
	* function.c (assign_stack_temp_for_type): Set 'keep' flag for
	TARGET_EXPR temp slots.
	
	PR middle-end/15054
	* g++.dg/opt/pr15054.C: New test.

Patches:
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/ChangeLog.diff?cvsroot=gcc&r1=2.3539&r2=2.3540
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/expr.c.diff?cvsroot=gcc&r1=1.641&r2=1.642
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/function.c.diff?cvsroot=gcc&r1=1.513&r2=1.514
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/ChangeLog.diff?cvsroot=gcc&r1=1.3713&r2=1.3714
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/g++.dg/opt/pr15054.C.diff?cvsroot=gcc&r1=NONE&r2=1.1

Comment 8 Andrew Pinski 2004-05-01 12:23:22 UTC
Fixed on the mainline so far.
Comment 9 CVS Commits 2004-05-05 12:44:17 UTC
Subject: Bug 15054

CVSROOT:	/cvs/gcc
Module name:	gcc
Branch: 	hammer-3_3-branch
Changes by:	matz@gcc.gnu.org	2004-05-05 12:44:10

Added files:
	gcc/testsuite/g++.dg/opt: pr15054.C 

Log message:
	Backport from mainline
	2004-05-01  Ulrich Weigand  <uweigand@de.ibm.com>
	
	PR middle-end/15054
	* expr.c (expand_expr_real): Do not call preserve_temp_slots
	on a TARGET_EXPR temp.
	* function.c (assign_stack_temp_for_type): Set 'keep' flag for
	TARGET_EXPR temp slots.

Patches:
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/g++.dg/opt/pr15054.C.diff?cvsroot=gcc&only_with_tag=hammer-3_3-branch&r1=NONE&r2=1.1.2.1

Comment 10 CVS Commits 2004-05-16 20:27:35 UTC
Subject: Bug 15054

CVSROOT:	/cvs/gcc
Module name:	gcc
Branch: 	gcc-3_3-branch
Changes by:	gdr@gcc.gnu.org	2004-05-16 20:27:22

Modified files:
	gcc            : ChangeLog expr.c function.c 
Added files:
	gcc/testsuite/g++.dg/opt: pr15054.C 

Log message:
	Backport from mainline
	2004-05-01  Ulrich Weigand  <uweigand@de.ibm.com>
	PR middle-end/15054
	* expr.c (expand_expr_real): Do not call preserve_temp_slots
	on a TARGET_EXPR temp.
	* function.c (assign_stack_temp_for_type): Set 'keep' flag for
	TARGET_EXPR temp slots.

Patches:
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/ChangeLog.diff?cvsroot=gcc&only_with_tag=gcc-3_3-branch&r1=1.16114.2.979&r2=1.16114.2.980
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/expr.c.diff?cvsroot=gcc&only_with_tag=gcc-3_3-branch&r1=1.498.2.30&r2=1.498.2.31
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/function.c.diff?cvsroot=gcc&only_with_tag=gcc-3_3-branch&r1=1.389.2.15&r2=1.389.2.16
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/g++.dg/opt/pr15054.C.diff?cvsroot=gcc&only_with_tag=gcc-3_3-branch&r1=NONE&r2=1.1.8.1

Comment 11 Gabriel Dos Reis 2004-05-16 20:30:51 UTC
Fixed for 3.3.4.  Now only 3.4.x regression.

(I've just been bitten by this bug in my own code :-()
Comment 12 Gabriel Dos Reis 2004-05-17 15:03:13 UTC
Subject: Re:  [3.4 Regression] Bad code due to overlapping stack temporaries

Ulrich Weigand <Ulrich.Weigand@de.ibm.com> writes:

| Hello Gaby,
| 
| sorry for not committing this earlier; I was waiting for Mark's approval to
| apply to both branches simultaneously ...

Hey Ulrich,

   There is no need to sorry!  I came to appreciate the patch when my
own code got screwed :-)

-- Gaby
Comment 13 Wolfgang Bangerth 2004-05-17 15:40:56 UTC
Mark, what is your word on this for 3.4.1? 
 
W. 
Comment 14 Mark Mitchell 2004-05-18 07:35:04 UTC
This patch is OK for 3.4.1.  Thanks!
Comment 15 CVS Commits 2004-05-18 11:08:28 UTC
Subject: Bug 15054

CVSROOT:	/cvs/gcc
Module name:	gcc
Branch: 	gcc-3_4-branch
Changes by:	uweigand@gcc.gnu.org	2004-05-18 11:08:22

Modified files:
	gcc            : ChangeLog expr.c function.c 
	gcc/testsuite  : ChangeLog 
Added files:
	gcc/testsuite/g++.dg/opt: pr15054.C 

Log message:
	ChangeLog:
	
	PR middle-end/15054
	* expr.c (expand_expr_real): Do not call preserve_temp_slots
	on a TARGET_EXPR temp.
	* function.c (assign_stack_temp_for_type): Set 'keep' flag for
	TARGET_EXPR temp slots.
	
	testsuite/ChangeLog:
	
	PR middle-end/15054
	* g++.dg/opt/pr15054.C: New test.

Patches:
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/ChangeLog.diff?cvsroot=gcc&only_with_tag=gcc-3_4-branch&r1=2.2326.2.444&r2=2.2326.2.445
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/expr.c.diff?cvsroot=gcc&only_with_tag=gcc-3_4-branch&r1=1.615.4.12&r2=1.615.4.13
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/function.c.diff?cvsroot=gcc&only_with_tag=gcc-3_4-branch&r1=1.483.4.12&r2=1.483.4.13
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/ChangeLog.diff?cvsroot=gcc&only_with_tag=gcc-3_4-branch&r1=1.3389.2.175&r2=1.3389.2.176
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/g++.dg/opt/pr15054.C.diff?cvsroot=gcc&only_with_tag=gcc-3_4-branch&r1=NONE&r2=1.1.12.1

Comment 16 Andrew Pinski 2004-05-18 11:49:28 UTC
Fixed.