/analyze for Visual Studio—the Ugly (part 3), False Positives

/analyze is a powerful Visual C++ feature that can help to find bugs. However it sometimes identifies problems when none exist. Unfortunately these false positives, like those reported here, make /analyze less useful because the serious bugs end up drowned by the cries of ‘wolf’. In this post I describe another frustrating false positive that shows one example of the compiler utterly failing to understand aliasing.

Update: the /analyze in Visual Studio 2012 doesn’t warn on these constructs anymore. But unfortunately they have gone from false positives to false negatives. In the GetInt() example if you comment out some of the initialization then there is no warning, even though a partially uninitialized object is being returned. Better? I’m not sure.

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

Uninitialized data

We have some code that reads from a file or network stream and initializes various variables. When we need to initialize an integer we call this function:

int GetInt(const char* p)
{
    int result;
    char* pDest = (char*)&result;
    pDest[0] = p[0];
    pDest[1] = p[1];
    pDest[2] = p[2];
    pDest[3] = p[3];
    return result; // warning C6001: Using uninitialized memory ‘result’
}

The compiler is unable or unwilling to tell that we have written to all four bytes of result so it gives a warning when we return it.

One could argue that the compiler is technically correct. The rules around aliasing in the C++ language are a bit esoteric so while I believe that this is legal, I am not certain. Using a union is a way to inform the compiler that aliasing is occurring so I tried that:

union intAlias
{
    int i;
    char c[4];
};

int GetIntFixAttempt(const char* p)
{
    intAlias result;
    char* pDest = result.c;
    pDest[0] = p[0];
    pDest[1] = p[1];
    pDest[2] = p[2];
    pDest[3] = p[3];
    return result.i; // warning C6001: Using uninitialized memory ‘result’
}

No dice. However I was on the right track. I next tried rewriting the code like this, to make it easier for the compiler to follow the trail:

union intAlias
{
    int i;
    char c[4];
};

int GetIntFixed(const char* p)
{
    intAlias result;
    result.c[0] = p[0];
    result.c[1] = p[1];
    result.c[2] = p[2];
    result.c[3] = p[3];
    return result.i; // No warning!
}

Success at last! It’s nice to know that I can find code that will let /analyze realize that I’m not using uninitialized memory, but I’d rather not have to go through such gymnastics. All of the information about how the code will execute is available so there is no theoretical reason why the compiler can’t reason correctly about it, but it fails.

If the compiler wanted to give me a warning about dodgy aliasing then I could respect that, but its current warning is just wrong, and this makes /analyze more difficult to use.

About brucedawson

I'm a programmer, working for Google, focusing on optimization and reliability. Nothing's more fun than making code run 10x faster. Unless it's eliminating large numbers of bugs. I also unicycle. And play (ice) hockey. And juggle.
This entry was posted in Code analysis, Code Reliability, Programming, Visual Studio. Bookmark the permalink.

10 Responses to /analyze for Visual Studio—the Ugly (part 3), False Positives

  1. I wonder if using the restrict keyword would’ve helped the compiler figure it out.
    Maybe the compiler incorrectly assumes that pDest and p could potentially point to the same memory address?

  2. brucedawson says:

    I tried the __restrict keyword on all of the pointers, and it made no difference.

    This is actually an illegal use for the __restrict keyword anyway. The __restrict keyword says that a pointer does not alias. The ‘pDest’ pointer in the GetInt* functions does alias so using __restrict could theoretically lead to problems. Adding __restrict to the ‘p’ pointer is legal, but shouldn’t be necessary.

  3. Well I meant putting an __restrict on p, obviously not on pDest.
    I can imagine that it would be hard for a compiler to figure out this situation though, it would have to remember that ‘pDest’ basically *is* ‘result’. Which would complicate just about everything because it would have to support this for every pointer. (basically keep a list of local variable aliases for every pointer) I suppose a smart compiler could detect if a pointer is only set once to a local variable and then use that variable with some special flag internally to allow pointer behavior with it. That way it could detect cases like this and make it easier to optimize..

  4. brucedawson says:

    That makes sense. But the problem still comes down to the compiler (or at least the /analyze part) not figuring out that pDest basically *is* result. Without that, we’ve got nothing. I haven’t done tests to see if the optimizer can figure this out — I suspect it is totally separate from the /analyze part.

  5. Ben Harper says:

    John Carmack made a pertinent comment about such things, saying that if the compiler can’t figure out that it’s safe, then it’s probably hard for a human to figure that out too, and this likely to get it wrong. So keeping /analyze pacified may result in more comprehensible code.

  6. Pingback: Try /Analyze for Free | Random ASCII

  7. Pingback: Live Streaming Κάλυψη Συνεδρείων

  8. snakedoctor says:

    doesn’t int GetInt(const char* p) assign a value to local memory that falls out of scope after it’s returned? i.e. will be null/zero when it is returned?

    • brucedawson says:

      GetInt is returning ‘result’ by value, so the fact that ‘result’ goes out of scope does not matter. Returning a pointer to a local variable that is going out of scope is a separate issue and its consequences are far more serious than being null/zero when returned.

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s