False positive: deadcode.DeadStores wrongly reports that value stored in variable is not used

[CiderMan]

We have recently upgraded the version of Clang that we use for static analysis and we are getting a false positive reported. I've downloaded v14.0.0 from GitHub and confirmed that it still reports the false positive. A simple example is:

int GetNext(void *, int, int *);

int Foo(void *Pointer)
{
   int Handle = -1;
   int Instance = 0;

   while (((Handle = GetNext(Pointer, Handle, &Instance)) != -1) && (Instance != 0))
   {
      return 1;
   }

   return 0;
}

This reports:

$ /c/Program\ Files/LLVM/bin/clang.exe --analyze  test_case.c
test_case.c:8:13: warning: Although the value stored to 'Handle' is used in the enclosing expression, the value is never actually read from 'Handle' [deadcode.DeadStores]
   while (((Handle = GetNext(Pointer, Handle, &Instance)) != -1) && (Instance != 0))
            ^        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.

However, Handle is used to store the return value from one call to GetNext so that it can be passed to the next call, so I do not think that the warning is correct. As per the documentation, the warning can be suppressed by adding (void)Handle; in the body of the loop but it feels like this warning is not accurate and should not be reported.

Thanks,

Steve.

[llvmbot]

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

[steakhal]

I believe, your original bug-report was slightly different. This one is probably not the one you intended.

It seems like a true-positive report in its current form. You use while () but you immediately return from the loop; hence it's semantically equivalent to an if statement. Consequently, the value stored to Handle is never read - since there is no iteration that could reuse it.

The warning should be suppressed by this:

int Foo(void *Pointer) {
   int Handle = -1;
   int Instance = 0;

   if (GetNext(Pointer, Handle, &Instance)) != -1 && Instance != 0)
      return 1;
   return 0;
}

Or an even shorter one (but arguably less readable):

int Foo(void *Pointer) {
   int Handle = -1;
   int Instance = 0;
   return GetNext(Pointer, Handle, &Instance)) != -1 && Instance != 0;
}

[CiderMan]

Ah, you are right. This was not my code and the original was a lot more complicated (!). In simplifying it, it is clear that the code didn't do what they intended - but I had missed that. Luckily Clang did not!

I actually persuaded the developer to refactor their code to be more readable and maintainable - which also resulted in Clang being happy because the bug was fixed in the reworked version.

Thanks for looking at it and spotting the problem.