Bug 3736 – corrupted struct returned by function with optimizations (-O)

Status
RESOLVED
Resolution
FIXED
Severity
critical
Priority
P2
Component
dmd
Product
D
Version
D1 (retired)
Platform
Other
OS
Windows
Creation time
2010-01-23T10:02:00Z
Last change time
2014-02-16T15:24:21Z
Keywords
patch, wrong-code
Assigned to
nobody
Creator
spam
Blocks
3761

Comments

Comment #0 by spam — 2010-01-23T10:02:04Z
building the following code with -O corrupts the return value of getFoo. sorry could not reduce it more: import std.stdio; struct Foo { float x=0; } struct Bar { Foo p0; Foo p1; } Bar a = {Foo(0),Foo(400)}; Foo getFoo(Foo _pos) { Bar stillok = a; float d0 = (a.p0.x - _pos.x); float d1 = (a.p0.x - _pos.x); if( d0 > d1 ) { return stillok.p0; } else { return stillok.p1; } } void main() { writefln("%s", getFoo( Foo(100) ).x ); } // prints 6.91062e-39 under dmd2039 // prints 3.2372e-39 under dmd2030
Comment #1 by clugdbug — 2010-01-24T23:53:23Z
This isn't actually a regression. It fails on D2.00, and D1.020, 1.041, 1.055 as well. Reduced test case: --------------- struct Foo { int x; } Foo getFoo(Foo irrelevant) { Foo p = Foo(400); if ( p.x > p.x ) return irrelevant; else return p; } void main() { assert(getFoo( Foo(0) ).x == 400); }
Comment #2 by clugdbug — 2010-02-07T13:47:44Z
The problem has something to do with the transformation from if(cond) return xxx; else return yyy; into return cond ? xxx : yyy; If I comment out blockopt.c brcombine() lines 734-764, the test case doesn't fail. That doesn't necessarily mean that the problem lies there, though.
Comment #3 by clugdbug — 2010-02-08T21:02:14Z
This seems to be a problem with transformations which copy the type 'Ety' without also copying the size 'Enumbytes', but you need the struct size to be retained. when the struct size is zero, blocks get dropped and bad codegen results. This particular bug is fixed by adding: in optelem(), cgelem.c line 4376 (this is called with op==OPcond). if (!OTrtol(op) && op != OPparam && op != OPcolon && op != OPcolon2 && e1->Eoper == OPcomma) { // Convert ((a,b) op c) to (a,(b op c)) e1->Ety = e->Ety; + e1->Enumbytes = e->Enumbytes; e->E1 = e1->E2; e1->E2 = e; e = e1; goto beg; } blockopt.c, brcombine() line 755: if (EOP(b3->Belem)) continue; ty = (bc2 == BCretexp) ? b2->Belem->Ety : TYvoid; e = el_bin(OPcolon2,ty,b2->Belem,b3->Belem); b->Belem = el_bin(OPcond,ty,b->Belem,e); } + b->Belem->Enumbytes = b2->Belem->Enumbytes; b->BC = bc2; A dozen lines further down the same function are two other calls to el_bin(OPcond,...) or el_bin(OPcolon2,...) in brcombine in blockopt.c. So I suspect line 797 should also be added: e = el_bin(OPcolon2,b2->Belem->Ety, b2->Belem,b3->Belem); e = el_bin(OPcond,e->Ety,b->Belem,e); + e->Enumbytes=b2->Belem->Enumbytes; } But this is just speculation, I don't have a test case which fails there. There are no other instances of el_bin(OPcond,...) or el_bin(OPcolon2,...) anywhere else in the backend. I suspect there are other cases in the backend where types are transferred but Enumbytes is lost. (there are several in el_comma in cgelem.c). But mostly this probably just results in lost optimisation opportunities.
Comment #4 by clugdbug — 2010-02-10T00:58:30Z
Here is a test case which fails in DMC 8.42n. It hits the breakpoint when compiled with -O. ---------- struct Foo { int x; }; Foo getFoo(Foo irrelevant) { Foo p; p.x=400; if ( p.x > p.x ) return irrelevant; else return p; } void main() { Foo z; z.x=0; int y = getFoo( z ).x; if (y!=400) _asm int 3; } ----------
Comment #5 by bugzilla — 2010-02-12T01:36:21Z
Changeset 380
Comment #6 by Kosmonaut — 2010-02-12T11:48:07Z
(In reply to comment #5) > Changeset 380 http://www.dsource.org/projects/dmd/changeset/380
Comment #7 by bugzilla — 2010-03-08T22:22:18Z
Fixed dmd 1.057 and 2.041