[cfe-commits] [PATCH] Checking for sometimes-uninitialized variables

[Richard Smith]

Hi,

The attached patch adds a new classification of uninitialized variables,
between 'always used uninitialized' and 'may be used uninitialized', which
(unlike 'may be used uninitialized') has no false positives (after making
one conservative assumption), and produces more warnings than 'always used
uninitialized'.

The core idea is to assume that for each condition which was explicitly
written in the source code, and isn't constant, there exists a live code
path which will cause it to be true and a live code path which will cause
it to be false. If every such code path definitely leads to an
uninitialized variable use, we classify the use as being 'sometimes
uninitialized' rather than 'may be uninitialized'.

I've run this across a large body of code, and found mostly bugs, but also
two flavors of "false positive":

* Dead conditions

int n = ...;
int x;
if (n < 0) { x = 0; }
else if (n >= 0) { x = 1; }
return x;

- and -

int x;
for (int n = 0; n < size; ++n) { // Redundant "n < size" test.
 // This is somehow guaranteed to happen for some n.
 if (f(n) == k) {
   x = n;
   break;
 }
}
return x;

* Value dependence within function calls

bool hasFoo = false;
int foo;
if (complex_condition) {
  hasFoo = true;
  foo = something;
}
f(hasFoo, foo);

Here, f doesn't use its second argument if its first argument is false.
However, an lvalue-to-rvalue conversion on an uninitialized object has
undefined behavior (in C++ at least), so it doesn't seem unreasonable to
complain about this.

Is this appropriate for including in trunk clang? It seems to provide a
better tradeoff between false positives and false negatives than either of
the existing warning options for us. If so, should these warnings be part
of -Wuninitialized, or should they be kept separate?

- Richard


Examples (for more, see the testcases in the patch):

1) Every time an 'if' condition is not taken, uninitialized use inevitably
results:

int test_if_false(bool b) {
  int x; // expected-note {{variable}}
  if (b) x = 1; // expected-note {{whenever 'if' condition is false}}
  return x; // expected-warning {{sometimes uninit}}
}

test/Analysis/uninit-sometimes.cpp:8:10: warning: variable 'x' is sometimes
uninitialized when used here [-Wsometimes-uninitialized]
  return x; // expected-warning {{sometimes uninit}}
         ^
test/Analysis/uninit-sometimes.cpp:7:7: note: uninitialized use occurs
whenever 'if' condition is false
  if (b) x = 1; // expected-note {{whenever 'if' condition is false}}
      ^
test/Analysis/uninit-sometimes.cpp:6:8: note: initialize the variable 'x'
to silence this warning
  int x; // expected-note {{variable}}
       ^
        = 0


2) Every time a 'for' condition is false, uninitialized use inevitably
results:

int test_for_false(int k) {
  int x; // expected-note {{variable}}
  for (int n = 0; n < k; ++n) { // expected-note {{whenever 'for' loop
terminates normally}}
    if (maybe()) {
      x = n;
      break;
    }
  }
  return x; // expected-warning {{sometimes uninit}}
}

test/Analysis/uninit-sometimes.cpp:70:10: warning: variable 'x' is
sometimes uninitialized when used here [-Wsometimes-uninitialized]
  return x; // expected-warning {{sometimes uninit}}
         ^
test/Analysis/uninit-sometimes.cpp:64:19: note: uninitialized use occurs
whenever 'for' loop terminates normally
  for (int n = 0; n < k; ++n) { // expected-note {{whenever 'for' loop
terminates normally}}
                  ^~~~~
test/Analysis/uninit-sometimes.cpp:63:8: note: initialize the variable 'x'
to silence this warning
  int x; // expected-note {{variable}}
       ^
        = 0


3) No warning for uninitialized use resulting from a switch not matching
any case: we don't assume that can happen.

int test_switch_suppress_1(int k) {
  int x;
  switch (k) {
  case 0:
    x = 0;
    break;
  case 1:
    x = 1;
    break;
  }
  return x; // no-warning
}


4) No warning if there's a loop between a conditional initialization and a
loop: we can't be sure the loop terminates.

int test_no_false_positive_1(int k) {
  int x;
  if (k)
    x = 5;
  while (!k)
    maybe();
  return x;
}
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120517/a6f84f5e/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: sometimes-uninit.diff
Type: application/octet-stream
Size: 30793 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120517/a6f84f5e/attachment.obj>