Deadstore checker giving false positives on function pointers after https://reviews.llvm.org/D126534

[tauchris]

Commit 16cb3be62600 introduced new true-positive findings in C, but also introduced at least one new false positive for function pointers. Here's the failing case: https://godbolt.org/z/63zzcT8ch. Code is also shown here:

typedef int (*FNPTR)(int x, int y, int p);

int defaultFn(int x, int y, int p)
{
    return x+y+p;
}

int altFn1(int x, int y, int p)
{
    return x+y+p;
}

int altFn2(int x, int y, int p)
{
    return x+y+p;
}

FNPTR foo(int x)
{
    static char* strings[] = {"string1", "string2", "string3"};
    FNPTR fp = defaultFn;
    if (x < 0) {
        fp = altFn1;
    }
    else if (x == 0) {
        fp = altFn2;
    }
    else {
        fp = 0;
    }
    return fp;
}

In this code, the assignment FNPTR fp = defaultFn; triggers the checker warning. I believe this is a false positive, because a function pointer does not have storage.

This change was reviewed here: https://reviews.llvm.org/D126534

[llvmbot]

@llvm/issue-subscribers-clang-static-analyzer

[bjope]

I don't know really, but isn't the function pointer assignment making the function as having "address taken" or something like that? And I believe that could impact if the function can be removed etc (e.g. if it is unused otherwise or inlined everywhere). I guess the function itself has storage.

If you for example change the defaultFn in your example to "static int defaultFn" instead of "int defaultFn", then you will see that the dead store of the defaultFn function pointer prevents the defaultFn function from being removed. Whereas if you remove the dead store then the defaultFn function would disappear from the asm output.

[tauchris]

Excellent point, Bjorn. I see what you are saying. If the function is static, as you suggest, then whether there is storage for defaultFn depends on whether the useless assignment is present in the code. I suppose the same would be true even if defaultFn were not static, if the function isn't called anywhere else, and can therefore be discarded at link time.

So, I suppose there is a valid argument that the warning is a true positive, though it's a bit sketchy, since the function pointer assignment might cause storage that could otherwise be discarded--or it might not...

Perhaps this just needs to be called out in the documentation, as it is a unique corner case that has a substantially different mechanism compared to an unused string literal?

[balazs-benics-sonarsource]

IMO since there it can be proven that fp is overwritten on all paths, the initializer expression is dead.
Consider if the initializer expression would be much more convoluted and expensive to compute.
In that case, one would expect the analyzer to point out that code is actually never used, which is a code-smell.

That being said, I believe I would classify this as a TP.

So, I suppose there is a valid argument that the warning is a true positive, though it's a bit sketchy, since the function pointer assignment might cause storage that could otherwise be discarded--or it might not...

It shouldn't be observable, but I'm not sure. Given that the compiler can prove that the address of the function never escapes, it can do whatever it wants under the "as-if" rule. In this case since defaultFn is not static, the compiler likely needs to generate the code for it, but the linker still might decide to strip it.
But I believe, none of these should influence the checker.

[tauchris]

Checker doesn't currently warn for initializer expressions that do not impact object code size, unless I'm missing something.... E.g.,

{
   int what = 4;  // no warning here.
   if (x) {
      what = 10;
   } else {
      what = 20;
   }
   ...
}

It is odd that a declaration with an initializer consisting of a primitive-type scalar literal does not trigger a warning, but moving the initialization to a separate assignment statement does. From the LIT test, it does appear that this is intentional (and always has been)-- but perhaps that's a separate bug in the checker? Personally, I think warning for a primitive scalar literal initializer in a declaration could be a nuisance. It's a common enough defensive coding strategy to always initialize variables -- seems reasonable to me to warn only if the cost of an unnecessary initialization is substantial, but not otherwise.

In any case, though, I'm convinced that the function pointer case is a true positive by these arguments, and I agree that the checker should not be concerned with whether the compiler or linker might be able to optimize out an otherwise unused function. I think the community might benefit from a bit more detail in the documentation, though-- at a bare minimum, perhaps the documentation should explain the difference in behavior between an assignment and a declaration with initializer list, since that seems to be intentional?

[steakhal]

Yea, convinced. We should ignore this case as its a common defensive programming, indeed.

We should fix this. I'm not saying I voluntier though.
Thanks for reporting this @tauchris I've seen quite some issues uncovered by you. Many thanks for putting the effort into minimizing and reporting.