/analyze for Visual Studio–the Ugly (part 2), False Negatives

/analyze is a powerful Visual C++ feature that can help to find bugs. However it misses some issues that it really should be able to find, two of which are discussed here today.

Update: The /analyze in Visual Studio 2012 still doesn’t warn about these problems.

These tests are all done with Visual Studio 2010 SP1, C/C++ Optimizing Compiler Version 16.00.40219.01 for 80×86.

Static analysis tools like /analyze have to maintain a delicate balance between being too aggressive about reporting warnings (leading to too many false positives) and too timid (leading to too many false negatives). The bugs I’m describing today are both false negatives – errors that /analyze could have but it didn’t. There are inevitably some bugs that static analysis will not catch and I am calling these particular issues ‘bugs’ because it is my opinion that these are issues that /analyze can and should report. Both of these warnings show up when building with gcc, but not with Visual Studio’s /analyze.

Returning Local Variable Addresses

The first one is pretty clear. It has to do with the serious bug of returning the address of a local variable. Consider this code:

const char* GetName1()
{
    char name[100];
    strcpy_s( name, “This is a name” );

    // warning C4172: returning address of local variable or temporary
    return name;
}

As the comment suggests, Visual Studio gives an appropriate warning. From the warning number (starting with a 4) you can even tell that this is not a /analyze specific warning. The regular compiler gives this warning. Hurray!

Now consider this code:

struct FooBar
{
    char m_name[100];
};

const char* GetName2()
{
    FooBar object;
    strcpy_s( object.m_name, “This is a name” );

    return object.m_name;
}

The runtime behavior is identical, with the only change being that we are returning the address of a member variable of a local. Unfortunately that is enough to fool Visual Studio. No warning. Fail.

This is not a theoretical problem. Pretty much this exact bug was found in our code base. If it wasn’t for gcc we wouldn’t have found it. Visual Studio needs to detect this error.

Impossible Conditions

The next bug concerns the testing of impossible conditions, as shown with this code:

void ImpossibleTest( unsigned short x )
{
    if ( x == -1 )
    {
        printf( “It’s a miracle!\n” );
    }
}

As the saying goes, this condition will only be true for sufficiently large values of –1. Since x is unsigned it will never be equal to –1, and no amount of integral promotion magic will change that.

These types of impossible conditions are easy for the compiler to check for. The number on the left has a fixed range, the number on the right is a constant, and there is no overlap.

It is true that this type of warning can be very noisy. Some types of code, especially those involving templates, can reasonably generate these types of comparisons, so it may be that there is nothing to fix. But these conditions also often indicate a bug. Generating a warning, with a unique warning number so that developers can choose whether to ignore it, is easy and appropriate.

Summary

I think Visual Studio, with or without /analyze, should detect these two warnings. Developers who disagree could easily suppress those particular warnings. I hope that future versions of Visual Studio improve on this.