Bug 490279

Summary: gcc 4.4 miscompiles vavoom
Product: [Fedora] Fedora Reporter: Hans de Goede <hdegoede>
Component: vavoomAssignee: Hans de Goede <hdegoede>
Status: CLOSED RAWHIDE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: low    
Version: rawhideCC: dtimms, hdegoede, jakub, jason, moneta.mace
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2009-03-23 11:57:32 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On:    
Bug Blocks: 490236    
Attachments:
Description Flags
Simple reproducer none

Description Hans de Goede 2009-03-14 18:14:20 UTC
I had already noticed this some time ago, but I've tried debugging it myself, with little luck though I'm afraid.

vavoom, an advanced opensource doom engine derative. seems to be miscompiled
by gcc 4.4 or, perhaps more likely, has a bug which gets triggered by compiling it with gcc-4.4

The problem is that vavoom like quake uses a C-like language internally, which
gets interpreted runtime to control various aspects of the game, and the bug is
inside the interpreter. This has caused me to fail and pinpoint the cause of this.

I've asked upstream for help with pinpointing which piece of vavoom exactly gets
miscompiled.

In the mean time I'm filing this bug in case there might be possible workarounds for me to test, and / or you have some spare time and you can look in to this.

Here is a description of the behaviour I'm seeing with vavoom (and whats wrong with it).

What happens is that vavoom dies with a divide by 0
error, this is caused by the following part of
progs/common/uibase/FinaleScreen.vc:

void OnCreate()
{
        ::OnCreate();
        Background = NewChild(FinaleBackground);
        Background.FScreen = self;
        Background.ScaleX = itof(Width) / itof(Background.Width);
        Background.ScaleY = itof(Height) / itof(Background.Height);
}

The exact line setting of the divide by 0 is:
    Background.ScaleX = itof(Width) / itof(Background.Width);

The problem is that Background.Width == 0, so is Background.Height,
Height and Width. It looks like the defaultproperties of both
the class(es) are not getting set properly.

Note that the above code snippet and the defaultproperties I'm talking
about are part of code included with vavoom written in its own
C-dialect, which as said gets interpreted runtime.

To reproduce, get vavoom-1.29-2.fc11 from koji, then run doom-shareware
to make it download the shareware doom data files (it will prompt for this)
and then run vavoom like this:
vavoom -iwaddir ~/.vavoom/doom-shareware -doom

I've tried to disable all optimalization during the
compile (-O0) and that didn't help.

Comment 1 Mace Moneta 2009-03-14 18:55:30 UTC
*** Bug 490236 has been marked as a duplicate of this bug. ***

Comment 2 Jakub Jelinek 2009-03-14 19:44:51 UTC
If -O0 doesn't help, then the probability that it is apps fault, not gcc, is very very high.  In any case, if 4.3 compiled vavoom works, you could do a binary search between say 4.3 compiled -O0 objects and 4.4 compiled -O0 objects to narrow it to a single compilation unit, then, as no inlining except for always_inline is performed at -O0, you could split that into separate compilation units by functions and do a binary search again.

Most likely you just use some uninitialized variable or something similar.

Comment 3 Hans de Goede 2009-03-20 09:24:57 UTC
Thanks for the hint for mixing 43 compiled object files with 44 ones, that helped very much in pinpointing it to one file, after that it still wasn't easy. But I've found the cause and I've created a patch for vavoom fixing this.

However, this issue is caused by an (undocumented) change in the behaviour of default constructors generated by g++. IMHO this ought to be documented
here: http://gcc.gnu.org/gcc-4.4/changes.html

The change is that starting with 4.4 when g++ generates a default constructor
that will memset to 0 the memory returned by the new operator.
4.3 generated default constructors do not do this.

This caused problems with vavoom, as vavoom does some admittedly weird tricks,
where it overrides new, and then returns a pointer from new which points to
a block of memory with some variables pre-initialized. Some classes derived from the class, which overrides new, use a default constructor, causing the initialization done in the overridden new to be lost.

I'm leaving this open, because as already said IMHO this change ought to be
documented, or *maybe* it even needs to be reversed, depending on what the c++
standard says on this.

Comment 4 Jakub Jelinek 2009-03-20 10:24:27 UTC
There has been several bugfixes related to value-initialization recently, e.g.:
http://gcc.gnu.org/PR11309
http://gcc.gnu.org/PR30111
http://gcc.gnu.org/PR38232
http://gcc.gnu.org/PR39109
http://gcc.gnu.org/PR38233
From your description it is hard to guess which one it is though.  Perhaps a small testcase showing it?  Or of course if you recognize what vavoom is doing in one of the above PRs...

Comment 5 Hans de Goede 2009-03-20 11:05:35 UTC
Created attachment 336025 [details]
Simple reproducer

Ok,

Here is a simple reproducer of the change in behavior, if compiled with
gcc-4.3, this outputs:
d->a: 1

When compiled with gcc-4.4, this outputs:
d->a: 0

I'm not claiming the 4.4 behaviour is necessarily wrong or a bug, I'm just noticing that the behaviour has changed.

4.3 does not seem to do anything in Derived's default constructor (other then
calling Base's constructor). Where as 4.4 memset's the objects memory to 0, to be specific it does memset(obj, 0, sizeof(Derived)), not only initializing the derived class attributes to 0, but also those of the Base class.

Comment 6 Jakub Jelinek 2009-03-20 11:31:33 UTC
Thanks for the testcase, yes, this was changed by
http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=138355
so it is PR11309.

Comment 7 Jason Merrill 2009-03-20 13:50:20 UTC
Yes.  Following DR 178 (http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#178), the standard says that "An object whose initializer is an empty set of parentheses, i.e., (), shall be value-initialized." which, for a class such as Derived that doesn't have a user-defined constructor, can be implemented by zeroing out the object before calling the default constructor.  So "new Derived()" does this, but "new Derived" would not.

But the real problem is that even without value-initialization, the testcase has undefined behavior because operator new is accessing a non-static data member of the new Base object before its constructor has been run.  If you want to have default values of Base members, they should be set in the constructor, not operator new.

I will add some information about value-initialization to the release notes.

Comment 8 Hans de Goede 2009-03-23 11:57:32 UTC
Fixed in rawhide, closing.