Code in never executed branches not ignored

[Job Noorman]

Take the following example:

int main()
{
    int array[4];

    if (false)
        array[4] = 1;

}

Cppcheck reports an out of bounds access here although the branch is clearly never executed. Am I correct in considering this a bug?

I would like to add that this example is clearly contrived and would never occur in real code. However, it was actually distilled from a real code base were both the condition and the array access depended on defines which, in a particular configuration, ended-up producing code similar to this example. Therefore, it would be helpful if Cppcheck could simply ignore code in branches that will never be executed.

[Job Noorman]

I found a similar example which reports an unexpected error. Given the following assert definition:

#define assert(c) if(!(c)) exit(1)

Then this reports an out of bounds condition:

int array[4];
assert(false);
array[4] = 1;

Interestingly, the following does not:

int array[4];
int i = 4;
assert(false);
array[i] = 1;

Similarly, the following also does not give an error:

int array[4];
int i = 4;
if (false)
    array[i] = 1;

(The last two examples do report an error without the assert.)

So it seems that there is an inconsistency in how Cppcheck handles variables versus literals.

[Daniel Marjamäki]

hmm.. I can appreciate that you want to prevent warnings in unreachable code. However for these specific examples I would say that the code is contrived and should be rewritten. Is is possible to write a bit more realistic example codes?

It is true that literals and variables are handled differently. For literals and constants Cppcheck can see the value without any flow analysis.

If both the size and index are literals I think can be motivated to write a warning. Imagine:

void f() {
    int array[4];
    if (sizeof(int)==2)
        array[4] = 0;
}

Whenever "sizeof(int) == 2" is true, there will be array index out of bounds. If the condition is always false then it should be removed and if not the array index should be changed.

[Daniel Marjamäki]

I still believe that some warnings are ok to write for unreachable code. For instance saying that "x is assigned to itself" here:

    if (false)
         x = x;

so if we add some kind of filter for unreachable code it seems to me it should not be applied for all checks.

[Job Noorman]

I'll try to make the problem we have a bit more concrete.

We have products that can have a number of antennas which are identified in code like this:

enum Antennas
{
    ANT_MAIN = 0,
    ANT_AUX1,
    ANT_AUX2,
    ANT_LAST
};

This enum is then used throughout the code as follows:

void f()
{
    int antenna_info[ANT_LAST];

    if (ANT_AUX2 < ANT_LAST)
        antenna_info[ANT_AUX2] = 42;
}

This works fine in configurations with all three antennas. However, if the product has less antennas, the enum is defined like this (this example has two antennas):

enum Antennas
{
    ANT_MAIN = 0,
    ANT_AUX1,
    ANT_LAST,
    ANT_AUX2
};

Up until now, it has not been necessary to change the code that uses this enum since the compiler will see that the above if-branch is unreachable and not emit code for it. Cppcheck, however, does go through the branch and issues diagnostics about it.

I agree with your examples and that unreachable code should still be checked. Maybe there could be some form of flow analysis on literals? If the if-statement is rewritten as follows, all is fine for Cppcheck:

auto index = ANT_AUX2;
if (index < ANT_LAST)
    antenna_info[index] = 42;

So maybe if Cppcheck could figure out that the index used for an array access, although it is a literal, was checked before to be within the array's bounds, it could ignore the out-of-bounds access?

[Daniel Marjamäki]

I agree.

Your suggestion is good.

However maybe I was too offensive. It might be best that we just remove all ValueFlow data in unreachable code. Then all checks that rely on ValueFlow will be silenced in unreachable code. This could be fairly straightforward so if you have time please feel free to help us with it. You'd add a function in our lib/valueflow.cpp that looks up conditions that are always false and then remove all ValueFlow data in the conditional code.