[cfe-dev] [analyzer] UninitializedObjectChecker evaluation

[Kristóf Umann via cfe-dev]

Hi!

Sorry for my late response, I'm currently too busy to start tinkering with
this checker, but the feedback is very much appreciated!

On Sat, 4 May 2019 at 16:57, Dmitri Gribenko <gribozavr at gmail.com> wrote:

> 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.
>
>
When I use CodeChecker to look at the results, I prefer the notes. However,
having different report styles for different outputs (text, plist, etc)
would be great!


> 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();
> }
>
>
Having to remember that you have to call an init function is a good example
IMO why we really should emit a report in this case. What kind of heuristic
would work well here? "If the class has a method named Init(), temporarily
suppress the report, and emit it when the variable goes out of scope
without the Init() function being called"? Can the clang-tidy checker deal
with this?


> 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;
> }
>
>
That's a very valid concern.


> 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;
> }
>
>
Hmmm, interesting. A better guarded field analysis (one of the options for
the checker) might be able to deal with this. In this specific case, x is
public, so the danger is real, but if it wasn't, and was guarded with
assert(is_valid);, I see why we shouldn't report it.


> 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.
>

Yes it does, thank you for everything! :) I'll definitely make some
improvements when I'll have some spare time.


>
> 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>*/
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20190513/65976320/attachment.html>