[PATCH] D45532: [StaticAnalyzer] Checker to find uninitialized fields after a constructor call

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

That's a lot of code, so it'll take me some time to understand what's going on here. You might be able to help me by the large patch in smaller logical chunks.

The idea looks reasonable. We're having specific warnings for, eg., assigning an uninitialized value, which fire much more often now that we have proper constructor support (eg. they'll fire in the copy constructor when the value was not initialized in the "actual" constructor). While some bugs can be found this way, i agree that it's not the best way to find them.

That said, your checker finds non-bugs, which is always risky. Sometimes the user should be allowed to initialize the field after construction but before the first use as a potentially important performance optimization. Did you find any such intentionally uninitialized fields with your checker? Were there many of those? If a user wants to have his codebase "analyzer-clean", would he be able to suppress the warning without changing the behavior of the program?

If union support is causing problems and you have a lot of workaround in the checker for these problems, i'll definitely suggest removing these workarounds from the initial commit and replacing them with FIXME comments (even if it assumes completely suppressing all warnings on classes that mention unions anywhere in the chain) because i'd much rather move towards better modeling in RegionStore than having checkers work around that. You might also be interested in https://reviews.llvm.org/D45241.

> In most cases, all fields of a union is regarded as unknown. I checked these cases by regarding unknown fields as uninitialized.

The whole point of making them unknown, as far as i understand, was to suppress uninitialized field warnings. So i feel what we're doing here is a bit upside down.



================
Comment at: test/Analysis/ctor-uninitialized-member.cpp:683
+// Note that the rules for unions are different in C++ and C.
+// http://lists.llvm.org/pipermail/cfe-dev/242017-March/42052912.html
+
----------------
I managed to find the thread, but this link doesn't work for me.


================
Comment at: test/Analysis/ctor-uninitialized-member.cpp:869
+void f44() {
+  ContainsUnionWithSimpleUnionTest2(); // xpected-warning{{1 uninitialized field}}
+}
----------------
Hmm, shouldn't it say "expected"? Do these tests actually work?


https://reviews.llvm.org/D45532