2009-05-26

"If" conditional does not work properly

While debugging, I noticed that an "if" conditional displayed some pretty strange behavior.

I had code similar to this:

if (SomeFunctionReturningBoolean() == true)
{
// do something...
print "1";
}
else
{
// do something else.
print "2";
}

Strangely, even though SomeFunctionReturningBoolean() returned true (as the debugger helpfully noted) - the "else" clause was executed, rather than the "if" clause.

I started investigating this.

First, I suspected a mismatch between the code and the executable. I rebuilt from scratch, but no dice. I manually cleaned all of the intermediate files, nope - same strange behavior.

I then changed the code to the following:

bool fResult = SomeFunctionReturningBoolean();

if (fResult == true)
{
// do something...
print "1";
}
else
{
// do something else.
print "2";
}

fResult clearly held "true", but "2" was still printed.

Next, I changed the code to:
bool fResult = SomeFunctionReturningBoolean();

if (fResult == true)
{
// do something...
print "1";
}

if (fResult == false)
{
// do something else.
print "2";
}

Neither "1" nor "2" were printed. Strangely, this seemed to work:

bool fResult = SomeFunctionReturningBoolean();

if (fResult)
{
// do something...
print "1";
}
else
{
// do something else.
print "2";
}

it printed "1". I started wondering whether the function was indeed returning true. I changed to disassembly mode, and to my surprise - I noticed that the function was returning 0x6E, and that the equality check was:

cmp eax, 1

Going deeper into the code, I discovered that a function used by SomeFunctionReturningBoolean() as a basis for its return coded simply did not return any value. The programmer was annoyed by some of the warnings returned by an external include file, and disabled all of the warnings using a pragma - so the "Not all code paths return a value" warning simply did not appear when I compiled the code.

The Visual Studio 2008 debugger treats a bool variable with a value of 0 as false, and it displays "false" as its value in any variable windows. If the bool variable contains anything but 0 - the value displayed is "true". This is generally correct, since non-zero values are handled as "true" by most conditional assembly instructions. However, a comparison instruction like
if (variable == true)
is compiled to
cmp eax, 1
which does not check for "true"-ness, but rather for equality with 1 - which is the value that "true" signifies in the current Microsoft C++ world.

So - the actual source code compared the bool to a specific value - 1, and since 0x6E is not equal to 1 - the check failed and we executed the "else" clause.

So, what did we learn from this issue?
  1. Never disable all of the warnings using a pragma - disable specific warnings, otherwise you'll lose important warnings that might appear.
  2. The VS2008 debugger displays "true"/"false" values in the variables window based on a check of "true"-ness, not an equality to the "true" value.
  3. The "Go to assembly" can be very, very useful at times. You should familiarize yourself with it.

3 comments:

  1. 1. == true is gay. Your if's should be self explanatory. and always != FALSE, never == true.
    2. It only works if your function returns a bool (it won't work if your function returns an int a BOOL or any other typedef).
    3. !!x will make it 0 if x is 0, 1 if x isn't zero.

    ReplyDelete
  2. 1. How can a coding style be gay? :) If you think about it - the only way I could've discovered this error is with the "== true" syntax. with a "if (x)" syntax, I would never realize that there is a problem with the return value. What if it was meant to be false? It is only luck that the return value was meant to signify success.

    2. I personally believe in checking the return value, always. This prevents cases in which you assume that a return value is "true"-thful, when really only 0 is "success" and non-zero are error codes. This happens a lot with int return values. All in all, I am very much for the intrinsic bool type, and I do believe that checking for == true is the way to go.

    3. This is correct, but so what?

    ReplyDelete