[cfe-dev] [analyzer] UninitializedObjectChecker evaluation

[Dmitri Gribenko via cfe-dev]

Hi,

The results of the UninitializedObject checker significantly overlap
with the clang-tidy checker cppcoreguidelines-pro-type-member-init.
When both checkers trigger, messages from
cppcoreguidelines-pro-type-member-init are significantly more clear
and less noisy.

$ cat /tmp/t.cc
struct Foo {
  Foo() : y(0) {}
  int x;
  int y;
};
void entry1() {
  Foo x;
}

$ ./bin/clang-tidy -checks="-*,clang-analyzer*" /tmp/t.cc
/tmp/t.cc:2:17: warning: 1 uninitialized field at the end of the
constructor call [clang-analyzer-optin.cplusplus.UninitializedObject]
  Foo() : y(0) {}
                ^
/tmp/t.cc:3:7: note: uninitialized field 'this->x'
  int x;
      ^
/tmp/t.cc:7:7: note: Calling default constructor for 'Foo'
  Foo x;
      ^
/tmp/t.cc:2:17: note: 1 uninitialized field at the end of the constructor call
  Foo() : y(0) {}
                ^

$ ./bin/clang-tidy -checks="-*,cppcoreguidelines-pro-type-member-init" /tmp/t.cc
/tmp/t.cc:2:3: warning: constructor does not initialize these fields:
x [cppcoreguidelines-pro-type-member-init]
  Foo() : y(0) {}
  ^

Note how cppcoreguidelines-pro-type-member-init packs the same
information into one message.  The primary message from
UninitializedObject checker says there's "1 uninitialized field". As a
user, my first question is "which?" -- and to figure it out I have to
parse the rest of the output, where two notes out of three have
completely irrelevant information.  There's no way to call this
constructor in a way that will initialize `Foo::x`, so why print all
that?  Also, the phrasing is passive, I like the phrasing from
cppcoreguidelines-pro-type-member-init much better.

cppcoreguidelines-pro-type-member-init does not trigger on templates,
while UninitializedObject checker does.  For example:

template<typename T>
struct Foo {
  Foo() : y(0) {}
  T x;
  T y;
};
void entry1() {
  Foo<int> x;
}

It is unclear why cppcoreguidelines-pro-type-member-init decided to
not look into templates.

One source of false positives for the UninitializedObject checker are
`Init()` methods, for example:

bool ReadIntFromFile(int *x);
struct Foo {
  Foo() : y(0) {}
  int x;
  int y;
  bool Init() {
    return ReadIntFromFile(&x);
  }
};
bool entry1() {
  Foo x;
  return x.Init();
}

It is also unclear how to annotate member variables that are
intentionally left uninitialized:

struct Foo {
  Foo() : has_cached_value(false) {}
  bool has_cached_value;
  int cached_value;
};
bool entry1() {
  Foo x;
}

In many cases it is possible to rewrite the code using std::optional,
but not everywhere.

Another source of false positives is the `is_valid` pattern:

bool ReadIntFromFile(int *x);
struct Foo {
  Foo() {
    int temp;
    if (!ReadIntFromFile(&temp)) {
      is_valid = false;
      return;
    }
    x = temp;
    is_valid = true;
  }
  int x;
  bool is_valid;
};
void entry1() {
  Foo x;
}

Inlining in the Clang Static Analyzer helps the UninitializedObject
checker to see through abstractions.  In the following example
UninitializedObject checker is silent (as it should be), while
cppcoreguidelines-pro-type-member-init complains:

struct Foo {
  Foo() {
    Unset();
  }
  void Unset() {
    has_cached_value = false;
    x = 0;
  }
  bool has_cached_value;
  int x;
};
void entry1() {
  Foo x;
}

To summarize, I have two findings.

First, I haven't found a case where taking the call site context into
account helps this checker.  Fundamentally, this check is about
assuming nothing at the entry to the constructor, and checking a
strong postcondition (all fields are initialized) at the exit.

Second, the `Init()` and `is_valid` patterns are fundamentally at odds
with this checker, because it does not try to detect an uninitialized
memory read, it tries to detect a postcondition violation at the
constructor exit.

Hope this helps.

Dmitri

-- 
main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if
(j){printf("%d\n",i);}}} /*Dmitri Gribenko <gribozavr at gmail.com>*/