[MC++] NullReferenceException caused by Optimization

ricecake

I was tearing my hair out over a NullReferenceException I was getting in Release mode, but not in Debug mode. From the stack trace in the crash dialog, I found the offending function, and tried to step through the Release executable in the debugger. It kept crashing before even trying to enter the function, so I assumed it was a problem with the parameters. After much trial and error, I found it had nothing at all to do with the parameters. To find the bug, I commented out the body of the function and incrementally uncommented stuff until the crash reappeared. To my surprise, the offending code was this:

if (run.seasons[4]) {
display->Season = S"All";
}
else {
int i = 0;
while (!run.seasons[i]) {
++i;
}
display->Season = MarshalStdToNetString(season_names[i]);
}

In this case, run.seasons is a __nogc array of 5 bools, and the values are retrieved from a database where exactly one of the elements is guaranteed to be true while the others are false. Therefore, I omitted the explicit check for the index being in range. In Debug mode, this worked perfectly. However, in Release mode, it seems there is an optimization error with this code, causing a NullReferenceException when checking the condition of the while loop. The frustrating thing was that it wouldn't indicate the line number of the error (I have enabled the Program Database for debugging in Release mode), nor did it give me an IndexOutOfBounds error either. I also recall that they fixed a loop optimization bug[^] in SP1, but this case isn't covered by it. And, last time I checked, they stopped accepting bug reports for VS .NET 2003 since VS 2005 is out. The fix was to add an explicit check in the while loop condition to make sure the index is in range:

if (run.seasons[4]) {
display->Season = S"All";
}
else {
int i = 0;
while ((i < 5) && !run.seasons[i]) {
++i;
}
display->Season = MarshalStdToNetString(season_names[i]);
}

-- Marcus Kwok

Chris Meech

No offense, but would the following avoid the optimization problem also?

else
{
int i = 0;
for ( ; i < 5; i++ )
{
if ( run.seasons[i] )
break;
}
display->Season = MarshalStdToNetString(season_names[i]);
}

It's odd that sometimes being verbose with one's source code helps you miss out on optimization bugs. :)

Chris Meech I am Canadian. [heard in a local bar] I agree with you that my argument is useless. [Red Stateler] Hey, I am part of a special bread, we are called smart people [Captain See Sharp] The zen of the soapbox is hard to attain...[Jörgen Sigvardsson] I wish I could remember what it was like to only have a short term memory.[David Kentley]

ricecake

Chris Meech wrote:

No offense

No worries, none taken at all.

Chris Meech wrote:

would the following avoid the optimization problem also?

else
{
int i = 0;
for ( ; i < 5; i++ )
{
if ( run.seasons[i] )
break;
}
display->Season = MarshalStdToNetString(season_names[i]);
}

Yes, this works.

Chris Meech wrote:

It's odd that sometimes being verbose with one's source code helps you miss out on optimization bugs. :)

Well, based on that, I came up with a way to make it short again ;) (and also, I try to avoid break when possible except in switch statements):

else {
int i = 0;
for ( ; (i < 5) && !run.seasons[i]; ++i) {
; // do nothing since we just need the value of i when the loop terminates
}
display->Season = MarshalStdToNetString(season_names[i]);
}

Though, comparing the for version with the while version when checking it in to source control, IMHO the while version looks a little cleaner, so I'm sticking with that one. Thanks for your input! -- modified at 10:35 Wednesday 28th February, 2007

-- Marcus Kwok

BrianShields

Have you verified that it's impossible for all 5 values to be false? Because that loop will run off because they all are. This is likely the case as: a) it's fixed by your explicit check. b) this is classic of discrepancies with uninitialised data in Debug & Release. Specifically, the debug code will safely terminate hitting malloc guard bytes/uninitialised flood pattern which it will see as 'true' whereas Release will spin off the end until it hits a non-zero byte (which will match an invalid index) or segfaults. When you see different control paths in debug & release it's normally to do with the flood patterns the former uses.

ricecake

BrianShields wrote:

Have you verified that it's impossible for all 5 values to be false?

Yes, the values in the array are retrieved from a database lookup, and they can only be inserted in the database if one of the values is true. Also, in the case that I was testing when I discovered this bug, index 0 was the first true value, and it correctly terminated the loop at that point in Debug mode.

-- Marcus Kwok

BrianShields

> Yes, the values in the array are retrieved from a database lookup, and they can only be inserted in the database if one of the values is true. I'd be inclined to add a throw or at least an assertion for an expected behaviour like that, it's not the same as it being impossible. I only ever saw the compiler bug you linked once and it was in a pretty munged loop (nested STL adaptors, Managed code), I'm surprised something so simple could reproduce it. Have you utterly verified that index 0 in that case was true in Release as well? Shame the MSIL output may not tell the whole story :(

ricecake

BrianShields wrote:

I only ever saw the compiler bug you linked once and it was in a pretty munged loop (nested STL adaptors, Managed code), I'm surprised something so simple could reproduce it.

Actually, the SSCCE for the loop optimization bug was pretty short itself:

// Using VS .NET 2003, when compiling without /O2, output is:
// before: abcd after: cdxx
// but compiling with /O2, output is:
// before: abcd after: cxxx
//
// This seems to be fixed with VS .NET 2003 SP1
#include <iostream>

int main()
{
char c[4];
c[0] = 'a';
c[1] = 'b';
c[2] = 'c';
c[3] = 'd';

std::cout << "before: " << c\[0\] << c\[1\] << c\[2\] << c\[3\] << " ";

for (int n = 2; n > 0; --n) {
    for (int i = 0; i < 3; i++) {
        c\[i\] = c\[i+1\];
    }
    c\[3\] = 'x';
}

std::cout << "after: " << c\[0\] << c\[1\] << c\[2\] << c\[3\] << std::endl;
return 0;

}

BrianShields wrote:

Have you utterly verified that index 0 in that case was true in Release as well?

Yes, if I comment out the original loop and display the contents of the array in a MessageBox, indeed the first element is true. It was retrieving the same data from the DB as in Debug mode anyway.

-- Marcus Kwok