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

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus added a comment.

In https://reviews.llvm.org/D45532#1145512, @sylvestre.ledru wrote:

> If you are interested, I executed this checker on Firefox code. Results can 
> be found here
>  http://sylvestre.ledru.info/reports/fx-scan-build/


Absolutely, thank you! :)

> Andi reported this bug https://bugs.llvm.org/show_bug.cgi?id=37965

Well, that is intentional: not the pointer, but the pointee is uninitialized, 
as you can see from the note message. Now with that being said, I have had an 
other colleague of mine complain about a report, as he didn't see that the note 
message said "pointee" not "pointer", so maybe there's a point in trying to 
come up with a better message.

In https://reviews.llvm.org/D45532#1145592, @george.karpenkov wrote:

> @sylvestre.ledru Have you found any actual bugs using this checker?
>  @Szelethus Interesting, it seems that the pointer itself is initialized, but 
> not what it's pointing to.


Exactly.

> I think we should just check the fields directly, and do not attempt to 
> traverse the pointer hierarchy.

Hmm, that's one way of thinking, but I think it's more beneficial to check 
pointers too.

I'll take a look at some results and try to get back to you with some stats to 
support my view on this issue :)


Repository:
  rC Clang

https://reviews.llvm.org/D45532



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov added a comment.

@sylvestre.ledru Have you found any actual bugs using this checker?
@Szelethus Interesting, it seems that the pointer itself is initialized, but 
not what it's pointing to.
I think we should just check the fields directly, and do not attempt to 
traverse the pointer hierarchy.


Repository:
  rC Clang

https://reviews.llvm.org/D45532



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sylvestre Ledru via Phabricator via cfe-commits]

sylvestre.ledru added a comment.

If you are interested, I executed this checker on Firefox code. Results can be 
found here
http://sylvestre.ledru.info/reports/fx-scan-build/

Andi reported this bug https://bugs.llvm.org/show_bug.cgi?id=37965


Repository:
  rC Clang

https://reviews.llvm.org/D45532



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Umann Kristóf via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC334935: [analyzer] Checker for uninitialized C++ objects 
(authored by Szelethus, committed by ).

Repository:
  rC Clang

https://reviews.llvm.org/D45532

Files:
  include/clang/StaticAnalyzer/Checkers/Checkers.td
  lib/StaticAnalyzer/Checkers/CMakeLists.txt
  lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
  test/Analysis/Inputs/system-header-simulator-for-cxx-uninitialized-object.h
  test/Analysis/cxx-uninitialized-object-inheritance.cpp
  test/Analysis/cxx-uninitialized-object-ptr-ref.cpp
  test/Analysis/cxx-uninitialized-object.cpp

Index: lib/StaticAnalyzer/Checkers/CMakeLists.txt
===
--- lib/StaticAnalyzer/Checkers/CMakeLists.txt
+++ lib/StaticAnalyzer/Checkers/CMakeLists.txt
@@ -92,6 +92,7 @@
   UndefResultChecker.cpp
   UndefinedArraySubscriptChecker.cpp
   UndefinedAssignmentChecker.cpp
+  UninitializedObjectChecker.cpp
   UnixAPIChecker.cpp
   UnreachableCodeChecker.cpp
   VforkChecker.cpp
Index: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
+++ lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
@@ -0,0 +1,669 @@
+//===- UninitializedObjectChecker.cpp *- C++ -*-==//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+// This file defines a checker that reports uninitialized fields in objects
+// created after a constructor call.
+//
+// This checker has an option "Pedantic" (boolean). If its not set or is set to
+// false, the checker won't emit warnings for objects that don't have at least
+// one initialized field. This may be set with
+// `-analyzer-config alpha.cplusplus.UninitializedObject:Pedantic=true`.
+//
+//===--===//
+
+#include "ClangSACheckers.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include 
+
+using namespace clang;
+using namespace clang::ento;
+
+namespace {
+
+class UninitializedObjectChecker : public Checker {
+  std::unique_ptr BT_uninitField;
+
+public:
+  bool IsPedantic; // Will be initialized when registering the checker.
+
+  UninitializedObjectChecker()
+  : BT_uninitField(new BuiltinBug(this, "Uninitialized fields")) {}
+  void checkEndFunction(CheckerContext ) const;
+};
+
+llvm::ImmutableListFactory Factory;
+
+/// Represents a field chain. A field chain is a vector of fields where the
+/// first element of the chain is the object under checking (not stored), and
+/// every other element is a field, and the element that precedes it is the
+/// object that contains it.
+///
+/// Note that this class is immutable, and new fields may only be added through
+/// constructor calls.
+class FieldChainInfo {
+  using FieldChain = llvm::ImmutableList;
+
+  FieldChain Chain;
+
+  const bool IsDereferenced = false;
+
+public:
+  FieldChainInfo() = default;
+
+  FieldChainInfo(const FieldChainInfo , const bool IsDereferenced)
+  : Chain(Other.Chain), IsDereferenced(IsDereferenced) {}
+
+  FieldChainInfo(const FieldChainInfo , const FieldRegion *FR,
+ const bool IsDereferenced = false);
+
+  bool contains(const FieldRegion *FR) const { return Chain.contains(FR); }
+  bool isPointer() const;
+
+  /// If this is a fieldchain whose last element is an uninitialized region of a
+  /// pointer type, `IsDereferenced` will store whether the pointer itself or
+  /// the pointee is uninitialized.
+  bool isDereferenced() const;
+  const FieldDecl *getEndOfChain() const;
+  void print(llvm::raw_ostream ) const;
+
+private:
+  /// Prints every element except the last to `Out`. Since ImmutableLists store
+  /// elements in reverse order, and have no reverse iterators, we use a
+  /// recursive function to print the fieldchain correctly. The last element in
+  /// the chain is to be printed by `print`.
+  static void printTail(llvm::raw_ostream ,
+const llvm::ImmutableListImpl *L);
+  friend struct FieldChainInfoComparator;
+};
+
+struct FieldChainInfoComparator {
+  bool operator()(const FieldChainInfo , const FieldChainInfo ) {
+assert(!lhs.Chain.isEmpty() && !rhs.Chain.isEmpty() &&
+   "Attempted to store an empty fieldchain!");
+return *lhs.Chain.begin() < *rhs.Chain.begin();
+  }
+};
+
+using UninitFieldSet = std::set;
+
+/// Searches for and stores uninitialized fields in a non-union object.
+class FindUninitializedFields {
+  ProgramStateRef State;
+  const 

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

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus updated this revision to Diff 151685.
Szelethus added a comment.

Whoo! Thank you all for helping me with the reviews. Very excited about the 
upcoming fixes, I have some neat ideas for some already.

- Rebased to 8186139d6aa0619e2057ae617c074e486a7b2f2b (revision 334929),
- Added a `FIXME` around base class handling as a reminder that this topic 
still needs to be discussed,
- Re-run tests on some projects just to be sure.


https://reviews.llvm.org/D45532

Files:
  include/clang/StaticAnalyzer/Checkers/Checkers.td
  lib/StaticAnalyzer/Checkers/CMakeLists.txt
  lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
  test/Analysis/Inputs/system-header-simulator-for-cxx-uninitialized-object.h
  test/Analysis/cxx-uninitialized-object-inheritance.cpp
  test/Analysis/cxx-uninitialized-object-ptr-ref.cpp
  test/Analysis/cxx-uninitialized-object.cpp

Index: test/Analysis/cxx-uninitialized-object.cpp
===
--- /dev/null
+++ test/Analysis/cxx-uninitialized-object.cpp
@@ -0,0 +1,1058 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.cplusplus.UninitializedObject -analyzer-config alpha.cplusplus.UninitializedObject:Pedantic=true -std=c++11 -DPEDANTIC -verify %s
+
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.cplusplus.UninitializedObject -std=c++11 -verify %s
+
+//===--===//
+// Default constructor test.
+//===--===//
+
+class CompilerGeneratedConstructorTest {
+  int a, b, c, d, e, f, g, h, i, j;
+
+public:
+  CompilerGeneratedConstructorTest() = default;
+};
+
+void fCompilerGeneratedConstructorTest() {
+  CompilerGeneratedConstructorTest();
+}
+
+#ifdef PEDANTIC
+class DefaultConstructorTest {
+  int a; // expected-note{{uninitialized field 'this->a'}}
+
+public:
+  DefaultConstructorTest();
+};
+
+DefaultConstructorTest::DefaultConstructorTest() = default;
+
+void fDefaultConstructorTest() {
+  DefaultConstructorTest(); // expected-warning{{1 uninitialized field}}
+}
+#else
+class DefaultConstructorTest {
+  int a;
+
+public:
+  DefaultConstructorTest();
+};
+
+DefaultConstructorTest::DefaultConstructorTest() = default;
+
+void fDefaultConstructorTest() {
+  DefaultConstructorTest();
+}
+#endif // PEDANTIC
+
+//===--===//
+// Initializer list test.
+//===--===//
+
+class InitListTest1 {
+  int a;
+  int b;
+
+public:
+  InitListTest1()
+  : a(1),
+b(2) {
+// All good!
+  }
+};
+
+void fInitListTest1() {
+  InitListTest1();
+}
+
+class InitListTest2 {
+  int a;
+  int b; // expected-note{{uninitialized field 'this->b'}}
+
+public:
+  InitListTest2()
+  : a(3) {} // expected-warning{{1 uninitialized field}}
+};
+
+void fInitListTest2() {
+  InitListTest2();
+}
+
+class InitListTest3 {
+  int a; // expected-note{{uninitialized field 'this->a'}}
+  int b;
+
+public:
+  InitListTest3()
+  : b(4) {} // expected-warning{{1 uninitialized field}}
+};
+
+void fInitListTest3() {
+  InitListTest3();
+}
+
+//===--===//
+// Constructor body test.
+//===--===//
+
+class CtorBodyTest1 {
+  int a, b;
+
+public:
+  CtorBodyTest1() {
+a = 5;
+b = 6;
+// All good!
+  }
+};
+
+void fCtorBodyTest1() {
+  CtorBodyTest1();
+}
+
+class CtorBodyTest2 {
+  int a;
+  int b; // expected-note{{uninitialized field 'this->b'}}
+
+public:
+  CtorBodyTest2() {
+a = 7; // expected-warning{{1 uninitialized field}}
+  }
+};
+
+void fCtorBodyTest2() {
+  CtorBodyTest2();
+}
+
+class CtorBodyTest3 {
+  int a; // expected-note{{uninitialized field 'this->a'}}
+  int b;
+
+public:
+  CtorBodyTest3() {
+b = 8; // expected-warning{{1 uninitialized field}}
+  }
+};
+
+void fCtorBodyTest3() {
+  CtorBodyTest3();
+}
+
+#ifdef PEDANTIC
+class CtorBodyTest4 {
+  int a; // expected-note{{uninitialized field 'this->a'}}
+  int b; // expected-note{{uninitialized field 'this->b'}}
+
+public:
+  CtorBodyTest4() {}
+};
+
+void fCtorBodyTest4() {
+  CtorBodyTest4(); // expected-warning{{2 uninitialized fields}}
+}
+#else
+class CtorBodyTest4 {
+  int a;
+  int b;
+
+public:
+  CtorBodyTest4() {}
+};
+
+void fCtorBodyTest4() {
+  CtorBodyTest4();
+}
+#endif
+
+//===--===//
+// Constructor delegation test.
+//===--===//
+
+class CtorDelegationTest1 {
+  int a;
+  int b;
+
+public:
+  CtorDelegationTest1(int)
+  : a(9) {
+// leaves 'b' unintialized, but we'll never check this function
+  }
+
+  CtorDelegationTest1()
+  : CtorDelegationTest1(int{}) { // Initializing 'a'
+b = 10;
+ 

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ accepted this revision.
NoQ added a comment.

Ok, let's land this one and see how it goes! I'm looking forward to seeing the 
follow-up patches.




Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:359-372
+  // Checking bases.
+  const auto *CXXRD = dyn_cast(RD);
+  if (!CXXRD)
+return ContainsUninitField;
+
+  for (const CXXBaseSpecifier BaseSpec : CXXRD->bases()) {
+const auto *Base = BaseSpec.getType()->getAsCXXRecordDecl();

Szelethus wrote:
> Szelethus wrote:
> > NoQ wrote:
> > > Szelethus wrote:
> > > > NoQ wrote:
> > > > > Are there many warnings that will be caused by this but won't be 
> > > > > caused by conducting the check for the constructor-within-constructor 
> > > > > that's currently disabled? Not sure - is it even syntactically 
> > > > > possible to not initialize the base class?
> > > > I'm not a 100% sure what you mean. Can you clarify?
> > > > >Not sure - is it even syntactically possible to not initialize the 
> > > > >base class?
> > > > If I understand the question correctly, no, as far as I know.
> > > You have a check for `isCalledByConstructor()` in order to skip base 
> > > class constructors but then you check them manually. That's a lot of 
> > > code, but it seems that the same result could have been achieved by 
> > > simply skipping the descent into the base class.
> > > 
> > > Same question for class-type fields, actually.
> > >Same question for class-type fields, actually.
> > 
> > My problem with that approach is that the same uninitialized field could've 
> > been emitted multiple times in different warnings. From an earlier 
> > conversation (assume that its run in pedantic mode):
> > 
> > >For this code snippet:
> > >
> > > ```
> > >struct A {
> > >   struct B {
> > >  int x, y;
> > >  
> > >  B() {}
> > >   } b;
> > >   int w;
> > >
> > >   A() {
> > >  b = B();
> > >   }
> > >};
> > >```
> > >the warning message after a call to `A::A()` would be "3 uninitialized 
> > >fields[...]", and for `B::B()` inside `A`s constructor would be "2 
> > >uninitialized fields[...]", so it wouldn't be filtered out.
> > 
> > In my opinion, if `A` contains a field of type `B`, it should be the 
> > responsibility of `A`'s constructors to initialize those fields properly.
> > >You have a check for isCalledByConstructor() in order to skip base class 
> > >constructors but then you check them manually. That's a lot of code, but 
> > >it seems that the same result could have been achieved by simply skipping 
> > >the descent into the base class.
> > I also believe that a constructor "should be held responsible" for the 
> > initialization of the inherited data members. However, in the case of base 
> > classes, uniqueing isn't an issue, as in this case:
> > ```
> > struct B {
> >   int a;
> >   B() {}
> > };
> > 
> > struct D : public B {
> >   int b;
> >   D() {}
> > };
> > ```
> > a call to `D::D()` would not check the inherited member (`a`), if I didn't 
> > implement it explicitly. That also means that if `isCalledByConstructor` 
> > was refactored to not filter out base classes, we would get two warning 
> > each with a single note, reporting `b` and `a` respectively. (I haven't 
> > actually checked it, but its a reasonable assumption)
> > 
> > Actually, I'm not 100% sure which approach is better: a warning for each 
> > base, or one warning for the object, bases included. I personally think one 
> > warning per object results in the best user experience.
> > 
> > I like both ideas, but I think if we'd come to the conclusion that a 
> > warning per base should be the way to go, it should definitely be 
> > implemented in a followup patch, as `isCalledByConstructor` is already in 
> > line for some major refactoring.
> > 
> > What do you think? :)
> >[...]but it seems that the same result could have been achieved by simply 
> >skipping the descent into the base class.
> I can't edit inline comments sadly, but I'd just like to point out that it 
> wouldn't achieve the same thing, as highlighted above. But, both solution 
> would be okay.
I still don't fully understand the reasoning behind who's responsible for 
initializing the field, i.e., the exact rule that you're enforcing seems to 
cause warnings to depend heavily on meta-information such as where does the 
analysis start, which is confusing. Because we're trying to enforce a coding 
guideline here, i believe that the guideline itself should be transparent and 
easy to explain. In particular, it should be obvious to the user which 
constructor is responsible for initializing a field, and by obvious i mean it 
should be obvious both from the guideline itself and from the analyzer's 
warning, and both of them should be the same "obvious".


https://reviews.llvm.org/D45532



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus updated this revision to Diff 150899.
Szelethus added a comment.

Polite ping :)

This diff fixed a minor issue in FieldChainInfo's constructor.


https://reviews.llvm.org/D45532

Files:
  include/clang/StaticAnalyzer/Checkers/Checkers.td
  lib/StaticAnalyzer/Checkers/CMakeLists.txt
  lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
  test/Analysis/Inputs/system-header-simulator-for-cxx-uninitialized-object.h
  test/Analysis/cxx-uninitialized-object-inheritance.cpp
  test/Analysis/cxx-uninitialized-object-ptr-ref.cpp
  test/Analysis/cxx-uninitialized-object.cpp

Index: test/Analysis/cxx-uninitialized-object.cpp
===
--- /dev/null
+++ test/Analysis/cxx-uninitialized-object.cpp
@@ -0,0 +1,1058 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.cplusplus.UninitializedObject -analyzer-config alpha.cplusplus.UninitializedObject:Pedantic=true -std=c++11 -DPEDANTIC -verify %s
+
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.cplusplus.UninitializedObject -std=c++11 -verify %s
+
+//===--===//
+// Default constructor test.
+//===--===//
+
+class CompilerGeneratedConstructorTest {
+  int a, b, c, d, e, f, g, h, i, j;
+
+public:
+  CompilerGeneratedConstructorTest() = default;
+};
+
+void fCompilerGeneratedConstructorTest() {
+  CompilerGeneratedConstructorTest();
+}
+
+#ifdef PEDANTIC
+class DefaultConstructorTest {
+  int a; // expected-note{{uninitialized field 'this->a'}}
+
+public:
+  DefaultConstructorTest();
+};
+
+DefaultConstructorTest::DefaultConstructorTest() = default;
+
+void fDefaultConstructorTest() {
+  DefaultConstructorTest(); // expected-warning{{1 uninitialized field}}
+}
+#else
+class DefaultConstructorTest {
+  int a;
+
+public:
+  DefaultConstructorTest();
+};
+
+DefaultConstructorTest::DefaultConstructorTest() = default;
+
+void fDefaultConstructorTest() {
+  DefaultConstructorTest();
+}
+#endif // PEDANTIC
+
+//===--===//
+// Initializer list test.
+//===--===//
+
+class InitListTest1 {
+  int a;
+  int b;
+
+public:
+  InitListTest1()
+  : a(1),
+b(2) {
+// All good!
+  }
+};
+
+void fInitListTest1() {
+  InitListTest1();
+}
+
+class InitListTest2 {
+  int a;
+  int b; // expected-note{{uninitialized field 'this->b'}}
+
+public:
+  InitListTest2()
+  : a(3) {} // expected-warning{{1 uninitialized field}}
+};
+
+void fInitListTest2() {
+  InitListTest2();
+}
+
+class InitListTest3 {
+  int a; // expected-note{{uninitialized field 'this->a'}}
+  int b;
+
+public:
+  InitListTest3()
+  : b(4) {} // expected-warning{{1 uninitialized field}}
+};
+
+void fInitListTest3() {
+  InitListTest3();
+}
+
+//===--===//
+// Constructor body test.
+//===--===//
+
+class CtorBodyTest1 {
+  int a, b;
+
+public:
+  CtorBodyTest1() {
+a = 5;
+b = 6;
+// All good!
+  }
+};
+
+void fCtorBodyTest1() {
+  CtorBodyTest1();
+}
+
+class CtorBodyTest2 {
+  int a;
+  int b; // expected-note{{uninitialized field 'this->b'}}
+
+public:
+  CtorBodyTest2() {
+a = 7; // expected-warning{{1 uninitialized field}}
+  }
+};
+
+void fCtorBodyTest2() {
+  CtorBodyTest2();
+}
+
+class CtorBodyTest3 {
+  int a; // expected-note{{uninitialized field 'this->a'}}
+  int b;
+
+public:
+  CtorBodyTest3() {
+b = 8; // expected-warning{{1 uninitialized field}}
+  }
+};
+
+void fCtorBodyTest3() {
+  CtorBodyTest3();
+}
+
+#ifdef PEDANTIC
+class CtorBodyTest4 {
+  int a; // expected-note{{uninitialized field 'this->a'}}
+  int b; // expected-note{{uninitialized field 'this->b'}}
+
+public:
+  CtorBodyTest4() {}
+};
+
+void fCtorBodyTest4() {
+  CtorBodyTest4(); // expected-warning{{2 uninitialized fields}}
+}
+#else
+class CtorBodyTest4 {
+  int a;
+  int b;
+
+public:
+  CtorBodyTest4() {}
+};
+
+void fCtorBodyTest4() {
+  CtorBodyTest4();
+}
+#endif
+
+//===--===//
+// Constructor delegation test.
+//===--===//
+
+class CtorDelegationTest1 {
+  int a;
+  int b;
+
+public:
+  CtorDelegationTest1(int)
+  : a(9) {
+// leaves 'b' unintialized, but we'll never check this function
+  }
+
+  CtorDelegationTest1()
+  : CtorDelegationTest1(int{}) { // Initializing 'a'
+b = 10;
+// All good!
+  }
+};
+
+void fCtorDelegationTest1() {
+  CtorDelegationTest1();
+}
+
+class CtorDelegationTest2 {
+  int a; // expected-note{{uninitialized field 'this->a'}}
+  int b;
+
+public:
+  CtorDelegationTest2(int)
+  : b(11) {
+// leaves 'a' unintialized, but 

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

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus marked 5 inline comments as done.
Szelethus added a comment.

In https://reviews.llvm.org/D45532#1116992, @george.karpenkov wrote:

> @Szelethus I personally really like this idea, and I think it would be a 
> great checker.
>  Could you perform more evaluation on other projects?


Thanks for the encouraging words!

I have checked some other projects:

- Xerces: I only found one uninit field (shockingly), and I found that it was 
probably unintentional.
- grpc: This project depends on numerous third-party libraries, so it has a 
variety of coding and implementation styles. The checker emitted 5 warnings, 
and I think it's very likely that at least 3 of those were unintentional (4, if 
we count the missing `=default;`).
- CppCheck: No warnings at all :)

The checker didn't crash during the analysis of any of the projects.

After some thinking, I decided to mark the comments on base classes done. I 
believe that the current implementation results in the best user experience, 
but even if we were come to the conclusion that it is not, we could resume the 
conversation on this when I submit a patch to fix related issues within 
`isCalledByConstructor`.


https://reviews.llvm.org/D45532



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:223-225
+  ExplodedNode *Node = Context.generateNonFatalErrorNode(Context.getState());
+  if (!Node)
+return;

Szelethus wrote:
> NoQ wrote:
> > Szelethus wrote:
> > > NoQ wrote:
> > > > I suspect that a fatal error is better here. We don't want the user to 
> > > > receive duplicate report from other checkers that catch uninitialized 
> > > > values; just one report is enough.
> > > I think that would be a bad idea. For example, this checker shouts so 
> > > loudly while checking the LLVM project, it would practically halt the 
> > > analysis of the code, reducing the coverage, which means that checkers 
> > > other then uninit value checkers would "suffer" from it.
> > > 
> > > However, I also think that having multiple uninit reports for the same 
> > > object might be good, especially with this checker, as it would be very 
> > > easy to see where the problem originated from.
> > > 
> > > What do you think?
> > Well, i guess that's the reason to not use the checker on LLVM. Regardless 
> > of fatal/nonfatal warnings, enabling this checker on LLVM regularly would 
> > be a bad idea because it's unlikely that anybody will be able to fix all 
> > the false positives to make it usable. And for other projects that don't 
> > demonstrate many false positives, this shouldn't be a problem.
> > 
> > In order to indicate where the problem originates from, we have our bug 
> > reporter visitors that try their best to add such info directly to the 
> > report. In particular, @george.karpenkov's recent `NoStoreFuncVisitor` 
> > highlights functions in which a variable was //not// initialized but was 
> > probably expected to be. Not sure if it highlights constructors in its 
> > current shape, but that's definitely a better way to give this piece of 
> > information to the user because it doesn't make the user look for a 
> > different report to understand the current report.
> LLVM is a special project in the fact that almost every part of it is so 
> performance critical, that leaving many fields uninit matters. However, I 
> would believe that in most projects, only a smaller portion of the code would 
> be like that.
> 
> Suppose that we have a project that also defines a set of ADTs, like an 
> `std::list`-like container. If that container had a field that would be left 
> uninit after a ctor call, analysis on every execution path would be halted 
> which would use an object like that.
> 
> My point is, as long as there is no way to tell the analyzer (or the checker) 
> to ignore certain constructor calls, I think it would be best not to generate 
> a fatal error.
> 
> >Regardless of fatal/nonfatal warnings, enabling this checker on LLVM 
> >regularly would be a bad idea because it's unlikely that anybody will be 
> >able to fix all the false positives to make it usable. And for other 
> >projects that don't demonstrate many false positives, this shouldn't be a 
> >problem.
> I wouldn't necessarily call them false positives. This checker doesn't look 
> for bugs, and all reports I checked were correct in the fact that those 
> fields really were left uninit. They just don't cause any trouble (just yet!).
I think of this check as a tool to support a specific programming model where 
every field needs to be initialized by the constructor. This programming model 
might be followed by some parts of the projects while a 3rd party library in 
the same project or some other files might not follow this model. Right now 
there is no easy way to turn off some checks for a set of files, and there is 
no way to turn off a set of checks for some headers. For this reason, I think 
it is not a good idea to make these errors fatal, as 3rd party headers might 
reduce the coverage of the analysis on a project in a way that the user cannot 
control.

If we are afraid of having multiple reports from this check we could turn off 
the check for that particular path, for example, we could have a bool stored in 
the GDM for each path whether this check is already reported an error or not 
and we can check that before emitting warnings. 


https://reviews.llvm.org/D45532



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus updated this revision to Diff 149290.
Szelethus added a comment.

- Rebased to 1cefbc5593d2f017ae56a853b0723a31865aa602 (revision 333276)
- Fixed some typos
- Added tests `CyclicPointerTest` and `CyclicVoidPointerTest` to highlight an 
issue to be fixed in a later patch


https://reviews.llvm.org/D45532

Files:
  include/clang/StaticAnalyzer/Checkers/Checkers.td
  lib/StaticAnalyzer/Checkers/CMakeLists.txt
  lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
  test/Analysis/Inputs/system-header-simulator-for-cxx-uninitialized-object.h
  test/Analysis/cxx-uninitialized-object-inheritance.cpp
  test/Analysis/cxx-uninitialized-object-ptr-ref.cpp
  test/Analysis/cxx-uninitialized-object.cpp

Index: test/Analysis/cxx-uninitialized-object.cpp
===
--- /dev/null
+++ test/Analysis/cxx-uninitialized-object.cpp
@@ -0,0 +1,1058 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.cplusplus.UninitializedObject -analyzer-config alpha.cplusplus.UninitializedObject:Pedantic=true -std=c++11 -DPEDANTIC -verify %s
+
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.cplusplus.UninitializedObject -std=c++11 -verify %s
+
+//===--===//
+// Default constructor test.
+//===--===//
+
+class CompilerGeneratedConstructorTest {
+  int a, b, c, d, e, f, g, h, i, j;
+
+public:
+  CompilerGeneratedConstructorTest() = default;
+};
+
+void fCompilerGeneratedConstructorTest() {
+  CompilerGeneratedConstructorTest();
+}
+
+#ifdef PEDANTIC
+class DefaultConstructorTest {
+  int a; // expected-note{{uninitialized field 'this->a'}}
+
+public:
+  DefaultConstructorTest();
+};
+
+DefaultConstructorTest::DefaultConstructorTest() = default;
+
+void fDefaultConstructorTest() {
+  DefaultConstructorTest(); // expected-warning{{1 uninitialized field}}
+}
+#else
+class DefaultConstructorTest {
+  int a;
+
+public:
+  DefaultConstructorTest();
+};
+
+DefaultConstructorTest::DefaultConstructorTest() = default;
+
+void fDefaultConstructorTest() {
+  DefaultConstructorTest();
+}
+#endif // PEDANTIC
+
+//===--===//
+// Initializer list test.
+//===--===//
+
+class InitListTest1 {
+  int a;
+  int b;
+
+public:
+  InitListTest1()
+  : a(1),
+b(2) {
+// All good!
+  }
+};
+
+void fInitListTest1() {
+  InitListTest1();
+}
+
+class InitListTest2 {
+  int a;
+  int b; // expected-note{{uninitialized field 'this->b'}}
+
+public:
+  InitListTest2()
+  : a(3) {} // expected-warning{{1 uninitialized field}}
+};
+
+void fInitListTest2() {
+  InitListTest2();
+}
+
+class InitListTest3 {
+  int a; // expected-note{{uninitialized field 'this->a'}}
+  int b;
+
+public:
+  InitListTest3()
+  : b(4) {} // expected-warning{{1 uninitialized field}}
+};
+
+void fInitListTest3() {
+  InitListTest3();
+}
+
+//===--===//
+// Constructor body test.
+//===--===//
+
+class CtorBodyTest1 {
+  int a, b;
+
+public:
+  CtorBodyTest1() {
+a = 5;
+b = 6;
+// All good!
+  }
+};
+
+void fCtorBodyTest1() {
+  CtorBodyTest1();
+}
+
+class CtorBodyTest2 {
+  int a;
+  int b; // expected-note{{uninitialized field 'this->b'}}
+
+public:
+  CtorBodyTest2() {
+a = 7; // expected-warning{{1 uninitialized field}}
+  }
+};
+
+void fCtorBodyTest2() {
+  CtorBodyTest2();
+}
+
+class CtorBodyTest3 {
+  int a; // expected-note{{uninitialized field 'this->a'}}
+  int b;
+
+public:
+  CtorBodyTest3() {
+b = 8; // expected-warning{{1 uninitialized field}}
+  }
+};
+
+void fCtorBodyTest3() {
+  CtorBodyTest3();
+}
+
+#ifdef PEDANTIC
+class CtorBodyTest4 {
+  int a; // expected-note{{uninitialized field 'this->a'}}
+  int b; // expected-note{{uninitialized field 'this->b'}}
+
+public:
+  CtorBodyTest4() {}
+};
+
+void fCtorBodyTest4() {
+  CtorBodyTest4(); // expected-warning{{2 uninitialized fields}}
+}
+#else
+class CtorBodyTest4 {
+  int a;
+  int b;
+
+public:
+  CtorBodyTest4() {}
+};
+
+void fCtorBodyTest4() {
+  CtorBodyTest4();
+}
+#endif
+
+//===--===//
+// Constructor delegation test.
+//===--===//
+
+class CtorDelegationTest1 {
+  int a;
+  int b;
+
+public:
+  CtorDelegationTest1(int)
+  : a(9) {
+// leaves 'b' unintialized, but we'll never check this function
+  }
+
+  CtorDelegationTest1()
+  : CtorDelegationTest1(int{}) { // Initializing 'a'
+b = 10;
+// All good!
+  }
+};
+
+void fCtorDelegationTest1() {
+  CtorDelegationTest1();
+}
+
+class CtorDelegationTest2 {
+  int a; // 

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

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov added a comment.

@Szelethus I personally really like this idea, and I think it would be a great 
checker.
Could you perform more evaluation on other projects?


https://reviews.llvm.org/D45532



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus marked 4 inline comments as done.
Szelethus added a comment.

I decided to mark the inline comments in `isPointerOrReferenceUninit` regarding 
the dereferencing loop termination done for now. I left several TODO's in the 
function to be fixed later, with many of them depending on one another, so 
fixing them all or many at once would probably be the best solution.

I left two conversations "not done" (fatal/nonfatal errors and base classes), 
but if I may, do you have any other objections?




Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:223-225
+  ExplodedNode *Node = Context.generateNonFatalErrorNode(Context.getState());
+  if (!Node)
+return;

NoQ wrote:
> Szelethus wrote:
> > NoQ wrote:
> > > I suspect that a fatal error is better here. We don't want the user to 
> > > receive duplicate report from other checkers that catch uninitialized 
> > > values; just one report is enough.
> > I think that would be a bad idea. For example, this checker shouts so 
> > loudly while checking the LLVM project, it would practically halt the 
> > analysis of the code, reducing the coverage, which means that checkers 
> > other then uninit value checkers would "suffer" from it.
> > 
> > However, I also think that having multiple uninit reports for the same 
> > object might be good, especially with this checker, as it would be very 
> > easy to see where the problem originated from.
> > 
> > What do you think?
> Well, i guess that's the reason to not use the checker on LLVM. Regardless of 
> fatal/nonfatal warnings, enabling this checker on LLVM regularly would be a 
> bad idea because it's unlikely that anybody will be able to fix all the false 
> positives to make it usable. And for other projects that don't demonstrate 
> many false positives, this shouldn't be a problem.
> 
> In order to indicate where the problem originates from, we have our bug 
> reporter visitors that try their best to add such info directly to the 
> report. In particular, @george.karpenkov's recent `NoStoreFuncVisitor` 
> highlights functions in which a variable was //not// initialized but was 
> probably expected to be. Not sure if it highlights constructors in its 
> current shape, but that's definitely a better way to give this piece of 
> information to the user because it doesn't make the user look for a different 
> report to understand the current report.
LLVM is a special project in the fact that almost every part of it is so 
performance critical, that leaving many fields uninit matters. However, I would 
believe that in most projects, only a smaller portion of the code would be like 
that.

Suppose that we have a project that also defines a set of ADTs, like an 
`std::list`-like container. If that container had a field that would be left 
uninit after a ctor call, analysis on every execution path would be halted 
which would use an object like that.

My point is, as long as there is no way to tell the analyzer (or the checker) 
to ignore certain constructor calls, I think it would be best not to generate a 
fatal error.

>Regardless of fatal/nonfatal warnings, enabling this checker on LLVM regularly 
>would be a bad idea because it's unlikely that anybody will be able to fix all 
>the false positives to make it usable. And for other projects that don't 
>demonstrate many false positives, this shouldn't be a problem.
I wouldn't necessarily call them false positives. This checker doesn't look for 
bugs, and all reports I checked were correct in the fact that those fields 
really were left uninit. They just don't cause any trouble (just yet!).



Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:359-372
+  // Checking bases.
+  const auto *CXXRD = dyn_cast(RD);
+  if (!CXXRD)
+return ContainsUninitField;
+
+  for (const CXXBaseSpecifier BaseSpec : CXXRD->bases()) {
+const auto *Base = BaseSpec.getType()->getAsCXXRecordDecl();

Szelethus wrote:
> NoQ wrote:
> > Szelethus wrote:
> > > NoQ wrote:
> > > > Are there many warnings that will be caused by this but won't be caused 
> > > > by conducting the check for the constructor-within-constructor that's 
> > > > currently disabled? Not sure - is it even syntactically possible to not 
> > > > initialize the base class?
> > > I'm not a 100% sure what you mean. Can you clarify?
> > > >Not sure - is it even syntactically possible to not initialize the base 
> > > >class?
> > > If I understand the question correctly, no, as far as I know.
> > You have a check for `isCalledByConstructor()` in order to skip base class 
> > constructors but then you check them manually. That's a lot of code, but it 
> > seems that the same result could have been achieved by simply skipping the 
> > descent into the base class.
> > 
> > Same question for class-type fields, actually.
> >Same question for class-type fields, actually.
> 
> My problem with that approach is that the same 

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

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus marked 9 inline comments as done.
Szelethus added a comment.

Thanks again for taking a look :)




Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:359-372
+  // Checking bases.
+  const auto *CXXRD = dyn_cast(RD);
+  if (!CXXRD)
+return ContainsUninitField;
+
+  for (const CXXBaseSpecifier BaseSpec : CXXRD->bases()) {
+const auto *Base = BaseSpec.getType()->getAsCXXRecordDecl();

NoQ wrote:
> Szelethus wrote:
> > NoQ wrote:
> > > Are there many warnings that will be caused by this but won't be caused 
> > > by conducting the check for the constructor-within-constructor that's 
> > > currently disabled? Not sure - is it even syntactically possible to not 
> > > initialize the base class?
> > I'm not a 100% sure what you mean. Can you clarify?
> > >Not sure - is it even syntactically possible to not initialize the base 
> > >class?
> > If I understand the question correctly, no, as far as I know.
> You have a check for `isCalledByConstructor()` in order to skip base class 
> constructors but then you check them manually. That's a lot of code, but it 
> seems that the same result could have been achieved by simply skipping the 
> descent into the base class.
> 
> Same question for class-type fields, actually.
>Same question for class-type fields, actually.

My problem with that approach is that the same uninitialized field could've 
been emitted multiple times in different warnings. From an earlier conversation 
(assume that its run in pedantic mode):

>For this code snippet:
>
> ```
>struct A {
>   struct B {
>  int x, y;
>  
>  B() {}
>   } b;
>   int w;
>
>   A() {
>  b = B();
>   }
>};
>```
>the warning message after a call to `A::A()` would be "3 uninitialized 
>fields[...]", and for `B::B()` inside `A`s constructor would be "2 
>uninitialized fields[...]", so it wouldn't be filtered out.

In my opinion, if `A` contains a field of type `B`, it should be the 
responsibility of `A`'s constructors to initialize those fields properly.
>You have a check for isCalledByConstructor() in order to skip base class 
>constructors but then you check them manually. That's a lot of code, but it 
>seems that the same result could have been achieved by simply skipping the 
>descent into the base class.
I also believe that a constructor "should be held responsible" for the 
initialization of the inherited data members. However, in the case of base 
classes, uniqueing isn't an issue, as in this case:
```
struct B {
  int a;
  B() {}
};

struct D : public B {
  int b;
  D() {}
};
```
a call to `D::D()` would not check the inherited member (`a`), if I didn't 
implement it explicitly. That also means that if `isCalledByConstructor` was 
refactored to not filter out base classes, we would get two warning each with a 
single note, reporting `b` and `a` respectively. (I haven't actually checked 
it, but its a reasonable assumption)

Actually, I'm not 100% sure which approach is better: a warning for each base, 
or one warning for the object, bases included. I personally think one warning 
per object results in the best user experience.

I like both ideas, but I think if we'd come to the conclusion that a warning 
per base should be the way to go, it should definitely be implemented in a 
followup patch, as `isCalledByConstructor` is already in line for some major 
refactoring.

What do you think? :)



Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:405-406
+  SVal DerefdV = StoreMgr.getBinding(S, V.castAs());
+  while (Optional L = DerefdV.getAs())
+DerefdV = StoreMgr.getBinding(S, *L);
+

NoQ wrote:
> Szelethus wrote:
> > NoQ wrote:
> > > Is this loop guaranteed to terminate? Is something like `void *v; v = 
> > > ` possible?
> > > 
> > > I think this loop should unwrap the AST type on every iteration and 
> > > dereference the pointer accordingly.
> > > 
> > > In general, i believe that every symbolic pointer dereference should be 
> > > made with awareness of the type of the object we're trying to retrieve. 
> > > Which is an optional argument for `State->getSVal(R, ...)`, but it seems 
> > > that it should be made mandatory because in a lot of cases omitting it 
> > > causes problems.
> > > Is this loop guaranteed to terminate? Is something like void *v; v =  
> > > possible?
> > I looked this up, and I am confident that it is not possible for a pointer 
> > to point to itself. Only a `void**` object may point to a `void*`. The loop 
> > terminates even if you do something evil like `v = 
> > reinterpret_cast();` (I have tried this with `int`s).
> > 
> > >I think this loop should unwrap the AST type on every iteration and 
> > >dereference the pointer accordingly.
> > >
> > >In general, i believe that every symbolic pointer dereference should be 
> > >made with awareness of the type of the object we're trying to retrieve. 
> > >Which is an optional argument for 

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Ok! Putting any remaining work into follow-up patches would be great indeed. 
Let's land this into `alpha.cplusplus` for now because i still haven't made up 
my mind for how to organize the proposed `bugprone` category (sorry!).




Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:223-225
+  ExplodedNode *Node = Context.generateNonFatalErrorNode(Context.getState());
+  if (!Node)
+return;

Szelethus wrote:
> NoQ wrote:
> > I suspect that a fatal error is better here. We don't want the user to 
> > receive duplicate report from other checkers that catch uninitialized 
> > values; just one report is enough.
> I think that would be a bad idea. For example, this checker shouts so loudly 
> while checking the LLVM project, it would practically halt the analysis of 
> the code, reducing the coverage, which means that checkers other then uninit 
> value checkers would "suffer" from it.
> 
> However, I also think that having multiple uninit reports for the same object 
> might be good, especially with this checker, as it would be very easy to see 
> where the problem originated from.
> 
> What do you think?
Well, i guess that's the reason to not use the checker on LLVM. Regardless of 
fatal/nonfatal warnings, enabling this checker on LLVM regularly would be a bad 
idea because it's unlikely that anybody will be able to fix all the false 
positives to make it usable. And for other projects that don't demonstrate many 
false positives, this shouldn't be a problem.

In order to indicate where the problem originates from, we have our bug 
reporter visitors that try their best to add such info directly to the report. 
In particular, @george.karpenkov's recent `NoStoreFuncVisitor` highlights 
functions in which a variable was //not// initialized but was probably expected 
to be. Not sure if it highlights constructors in its current shape, but that's 
definitely a better way to give this piece of information to the user because 
it doesn't make the user look for a different report to understand the current 
report.



Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:258-260
+Report->addNote(NoteBuf,
+PathDiagnosticLocation::create(FieldChain.getEndOfChain(),
+   
Context.getSourceManager()));

Szelethus wrote:
> NoQ wrote:
> > NoQ wrote:
> > > Szelethus wrote:
> > > > NoQ wrote:
> > > > > Aha, ok, got it, so you're putting the warning at the end of the 
> > > > > constructor and squeezing all uninitialized fields into a single 
> > > > > warning by presenting them as notes.
> > > > > 
> > > > > Because for now notes aren't supported everywhere (and aren't always 
> > > > > supported really well), i guess it'd be necessary to at least provide 
> > > > > an option to make note-less reports before the checker is enabled by 
> > > > > default everywhere. In this case notes contain critical pieces of 
> > > > > information and as such cannot be really discarded.
> > > > > 
> > > > > I'm not sure that notes are providing a better user experience here 
> > > > > than simply saying that "field x.y->z is never initialized". With 
> > > > > notes, the user will have to scroll around (at least in scan-build) 
> > > > > to find which field is uninitialized.
> > > > > 
> > > > > Notes will probably also not decrease the number of reports too much 
> > > > > because in most cases there will be just one uninitialized field. 
> > > > > Though i agree that when there is more than one report, the user will 
> > > > > be likely to deal with all fields at once rather than separately.
> > > > > 
> > > > > So it's a bit wonky.
> > > > > 
> > > > > We might want to enable one mode or another in the checker depending 
> > > > > on the analyzer output flag. Probably in the driver 
> > > > > (`RenderAnalyzerOptions()`).
> > > > > 
> > > > > It'd be a good idea to land the checker into alpha first and fix this 
> > > > > in a follow-up patch because this review is already overweight.
> > > > > [...]i guess it'd be necessary to at least provide an option to make 
> > > > > note-less reports before the checker is enabled by default 
> > > > > everywhere. In this case notes contain critical pieces of information 
> > > > > and as such cannot be really discarded.
> > > > This can already be achieved with `-analyzer-config 
> > > > notes-as-events=true`. There no good reason however not to make an 
> > > > additional flag that would emit warnings instead of notes.
> > > > > I'm not sure that notes are providing a better user experience here 
> > > > > than simply saying that "field x.y->z is never initialized". With 
> > > > > notes, the user will have to scroll around (at least in scan-build) 
> > > > > to find which field is uninitialized.
> > > > This checker had a previous implementation that emitted a warning for 
> > > > each uninitialized 

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

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus marked 28 inline comments as done.
Szelethus added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:181-182
+/// Returns whether FD can be (transitively) dereferenced to a void pointer 
type
+/// (void*, void**, ...). The type of the region behind a void pointer isn't
+/// known, and thus FD can not be analyzed.
+bool isVoidPointer(const FieldDecl *FD);

NoQ wrote:
> Type of an object pointed to by a void pointer is something that our 
> path-sensitive engine is sometimes able to provide. It is not uncommon that a 
> void pointer points to a concrete variable on the current path, and we may 
> know it in the analyzer. I'm not sure this sort of check is necessary (i.e. 
> require more information).
> 
> You may also want to have a look at `getDynamicTypeInfo()` which is sometimes 
> capable of retrieving the dynamic type of the object pointed to by a pointer 
> or a reference - i.e. even if it's different from the pointee type of the 
> pointer.
I took a look at it, but I found that it'd change the implementation of 
`isPointerOrReferenceUninit` so much, I'd be a lot more comfortable if I could 
implement it in a smaller followup patch. I placed some `TODO`s in the code 
about this.



Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:223-225
+  ExplodedNode *Node = Context.generateNonFatalErrorNode(Context.getState());
+  if (!Node)
+return;

NoQ wrote:
> I suspect that a fatal error is better here. We don't want the user to 
> receive duplicate report from other checkers that catch uninitialized values; 
> just one report is enough.
I think that would be a bad idea. For example, this checker shouts so loudly 
while checking the LLVM project, it would practically halt the analysis of the 
code, reducing the coverage, which means that checkers other then uninit value 
checkers would "suffer" from it.

However, I also think that having multiple uninit reports for the same object 
might be good, especially with this checker, as it would be very easy to see 
where the problem originated from.

What do you think?



Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:405-406
+  SVal DerefdV = StoreMgr.getBinding(S, V.castAs());
+  while (Optional L = DerefdV.getAs())
+DerefdV = StoreMgr.getBinding(S, *L);
+

NoQ wrote:
> Is this loop guaranteed to terminate? Is something like `void *v; v = ` 
> possible?
> 
> I think this loop should unwrap the AST type on every iteration and 
> dereference the pointer accordingly.
> 
> In general, i believe that every symbolic pointer dereference should be made 
> with awareness of the type of the object we're trying to retrieve. Which is 
> an optional argument for `State->getSVal(R, ...)`, but it seems that it 
> should be made mandatory because in a lot of cases omitting it causes 
> problems.
> Is this loop guaranteed to terminate? Is something like void *v; v =  
> possible?
I looked this up, and I am confident that it is not possible for a pointer to 
point to itself. Only a `void**` object may point to a `void*`. The loop 
terminates even if you do something evil like `v = 
reinterpret_cast();` (I have tried this with `int`s).

>I think this loop should unwrap the AST type on every iteration and 
>dereference the pointer accordingly.
>
>In general, i believe that every symbolic pointer dereference should be made 
>with awareness of the type of the object we're trying to retrieve. Which is an 
>optional argument for State->getSVal(R, ...), but it seems that it should be 
>made mandatory because in a lot of cases omitting it causes problems.

I invested some serious effort into `getDynamicType`, but I think it'll require 
more time and some serious refactoring of this (`isPointerOrReferenceUninit`) 
function, so I'd post it separately.



Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:432
+  if (isUnionUninit(R)) {
+LocalChain.dereference();
+return addFieldToUninits(std::move(LocalChain));

NoQ wrote:
> Needs a comment on why the `IsDereferenced` flag on this path.
Added a comment at the line where I dereference the field that after that point 
we'll check the pointee.



Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:559-560
+  while (LC) {
+if (isa(LC->getDecl()))
+  return true;
+

NoQ wrote:
> This doesn't check that the constructor on the parent stack frame is anyhow 
> related to the current constructor, so it may introduce false negatives. For 
> instance, a class may lazy-initialize a singleton but never store a reference 
> to it within itself (because, well, it's a singleton, you can always obtain 
> the reference to it). In this case we'll never find the bug in the 
> constructor of the singleton.
Added a TODO and a test 

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

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus updated this revision to Diff 147485.
Szelethus added a comment.

In this diff, I preferred placing `TODO`s in places where the logic of the 
checker would have changed too much. While I didn't have a strict rule for this 
in mind, I tried changing as little as possible in order to implement the fixes 
in smaller followup patches.

Now with that being said, I still did make numerous changes to address the 
inline comments, so here is a complete list of them:

- I placed the checker back in `alpha.cplusplus`
- While the main logic of the checker is unchanged, I did make some small 
additions: it turned out (thanks to @NoQ) that objects I wrongly referred to as 
fundamental were in fact of `BuiltinType` or `EnumeralType`. In the checker I 
defined these types as "primitive", as neither clang or the C++ standard 
defines a type called "primitive"
- A new node is added to the directed tree: objects of `MemberPointerType`. I 
updated the documentation, and added a function to `FindUninitializedFields` 
but left the implementation to be done in a followup patch
- String concatenations are all done with streams
- `FieldChainInfo` now uses `llvm::ImmutableList`
- `FieldChainInfo` is now completely immutable
- `FindUninitializedFields` no longer checks lazily, and has only a single 
`getUninitFields` public non-special member function.
- `FindUninitializedFields` now has a `ProgramStateRef` field instead of a 
bunch of managers (this added a dependency https://reviews.llvm.org/D46891)
- Added asserts and `llvm_unreachable` at numerous locations
- More comments for easier reading

For the test files:

- Changed all non-member function names to eliminate numbering
- Split `cxx-uninitialized-object.cpp` up, and placed pointer/reference tests 
in a different file
- Added tests for
  - `loc::ConcreteInt`
  - Member pointers
  - `BuiltinType` and `EnumeralType`
  - Constructorcall within a constructor, without the two constructors being in 
relation, to highlight a false negative
  - C++11 member initializers.

I'll be the first to admit that I'm not 100% sure I implemented the change to 
`llvm::ImmutableList` perfectly, so I guess maybe it's worth taking a deeper 
look at (but it certainly works!). Shall I tie its lifetime to the checker 
object's lifetime?
Of course, I rechecked the entire LLVM/Clang project and I'm happy to report 
that no crashes occured, and it emitted almost the same amount of warning 
(almost, because I used a newer version of clang).


https://reviews.llvm.org/D45532

Files:
  include/clang/StaticAnalyzer/Checkers/Checkers.td
  lib/StaticAnalyzer/Checkers/CMakeLists.txt
  lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
  test/Analysis/Inputs/system-header-simulator-for-cxx-uninitialized-object.h
  test/Analysis/cxx-uninitialized-object-inheritance.cpp
  test/Analysis/cxx-uninitialized-object-ptr-ref.cpp
  test/Analysis/cxx-uninitialized-object.cpp

Index: test/Analysis/cxx-uninitialized-object.cpp
===
--- /dev/null
+++ test/Analysis/cxx-uninitialized-object.cpp
@@ -0,0 +1,1058 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.cplusplus.UninitializedObject -analyzer-config alpha.cplusplus.UninitializedObject:Pedantic=true -std=c++11 -DPEDANTIC -verify %s
+
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.cplusplus.UninitializedObject -std=c++11 -verify %s
+
+//===--===//
+// Default constructor test.
+//===--===//
+
+class CompilerGeneratedConstructorTest {
+  int a, b, c, d, e, f, g, h, i, j;
+
+public:
+  CompilerGeneratedConstructorTest() = default;
+};
+
+void fCompilerGeneratedConstructorTest() {
+  CompilerGeneratedConstructorTest();
+}
+
+#ifdef PEDANTIC
+class DefaultConstructorTest {
+  int a; // expected-note{{uninitialized field 'this->a'}}
+
+public:
+  DefaultConstructorTest();
+};
+
+DefaultConstructorTest::DefaultConstructorTest() = default;
+
+void fDefaultConstructorTest() {
+  DefaultConstructorTest(); // expected-warning{{1 uninitialized field}}
+}
+#else
+class DefaultConstructorTest {
+  int a;
+
+public:
+  DefaultConstructorTest();
+};
+
+DefaultConstructorTest::DefaultConstructorTest() = default;
+
+void fDefaultConstructorTest() {
+  DefaultConstructorTest();
+}
+#endif // PEDANTIC
+
+//===--===//
+// Initializer list test.
+//===--===//
+
+class InitListTest1 {
+  int a;
+  int b;
+
+public:
+  InitListTest1()
+  : a(1),
+b(2) {
+// All good!
+  }
+};
+
+void fInitListTest1() {
+  InitListTest1();
+}
+
+class InitListTest2 {
+  int a;
+  int b; // expected-note{{uninitialized field 'this->b'}}
+
+public:
+  InitListTest2()
+  : a(3) {} // expected-warning{{1 uninitialized field}}
+};

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

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus added a comment.

I'm afraid it'll be even more time before I post a new diff. There are some 
things that `ProgramState` is lacking, so I'll get that fixed first in a 
different pull request first.




Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:258-260
+Report->addNote(NoteBuf,
+PathDiagnosticLocation::create(FieldChain.getEndOfChain(),
+   
Context.getSourceManager()));

NoQ wrote:
> NoQ wrote:
> > Szelethus wrote:
> > > NoQ wrote:
> > > > Aha, ok, got it, so you're putting the warning at the end of the 
> > > > constructor and squeezing all uninitialized fields into a single 
> > > > warning by presenting them as notes.
> > > > 
> > > > Because for now notes aren't supported everywhere (and aren't always 
> > > > supported really well), i guess it'd be necessary to at least provide 
> > > > an option to make note-less reports before the checker is enabled by 
> > > > default everywhere. In this case notes contain critical pieces of 
> > > > information and as such cannot be really discarded.
> > > > 
> > > > I'm not sure that notes are providing a better user experience here 
> > > > than simply saying that "field x.y->z is never initialized". With 
> > > > notes, the user will have to scroll around (at least in scan-build) to 
> > > > find which field is uninitialized.
> > > > 
> > > > Notes will probably also not decrease the number of reports too much 
> > > > because in most cases there will be just one uninitialized field. 
> > > > Though i agree that when there is more than one report, the user will 
> > > > be likely to deal with all fields at once rather than separately.
> > > > 
> > > > So it's a bit wonky.
> > > > 
> > > > We might want to enable one mode or another in the checker depending on 
> > > > the analyzer output flag. Probably in the driver 
> > > > (`RenderAnalyzerOptions()`).
> > > > 
> > > > It'd be a good idea to land the checker into alpha first and fix this 
> > > > in a follow-up patch because this review is already overweight.
> > > > [...]i guess it'd be necessary to at least provide an option to make 
> > > > note-less reports before the checker is enabled by default everywhere. 
> > > > In this case notes contain critical pieces of information and as such 
> > > > cannot be really discarded.
> > > This can already be achieved with `-analyzer-config 
> > > notes-as-events=true`. There no good reason however not to make an 
> > > additional flag that would emit warnings instead of notes.
> > > > I'm not sure that notes are providing a better user experience here 
> > > > than simply saying that "field x.y->z is never initialized". With 
> > > > notes, the user will have to scroll around (at least in scan-build) to 
> > > > find which field is uninitialized.
> > > This checker had a previous implementation that emitted a warning for 
> > > each uninitialized field, instead of squeezing them in a single warning 
> > > per constructor call. One of the major reasons behind rewriting it was 
> > > that it emitted so many of them that it was difficult to differentiate 
> > > which warning belonged to which constructor call. Also, in the case of 
> > > the LLVM/Clang project, the warnings from this checker alone would be in 
> > > the thousands.
> > > > Notes will probably also not decrease the number of reports too much 
> > > > because in most cases there will be just one uninitialized field.
> > > While this is true to some extent, it's not uncommon that a single 
> > > constructor call leaves 7 uninitialized -- in the LLVM/Clang project I 
> > > found one that had over 30!
> > > Since its practically impossible to determine whether this was a 
> > > performance enhancement or just poor programming, the few cases where it 
> > > is intentional, an object would emit many more warnings would make  make 
> > > majority of the findings (where an object truly had only 1 or 2 fields 
> > > uninit) a lot harder to spot in my opinion.
> > > 
> > > In general, I think one warning per constructor call makes the best user 
> > > experience, but a temporary solution should be implemented until there's 
> > > better support for notes.
> > > 
> > > I agree, this should be a topic of a follow-up patch.
> > > 
> > > This can already be achieved with `-analyzer-config notes-as-events=true`.
> > 
> > Yep. But the resulting user experience seems pretty bad to me. It was a 
> > good quick proof-of-concept trick to test things, but the fact that notes 
> > aren't path pieces is way too apparent in case of this checker. So i guess 
> > this flag was a bad idea.
> > 
> > > Also, in the case of the LLVM/Clang project, the warnings from this 
> > > checker alone would be in the thousands.
> > 
> > This makes me a bit worried, i wonder what's the reason why does the 
> > checker shout so loudly on a codebase that doesn't seem to have actual 
> > uninitialized 

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:258-260
+Report->addNote(NoteBuf,
+PathDiagnosticLocation::create(FieldChain.getEndOfChain(),
+   
Context.getSourceManager()));

NoQ wrote:
> Szelethus wrote:
> > NoQ wrote:
> > > Aha, ok, got it, so you're putting the warning at the end of the 
> > > constructor and squeezing all uninitialized fields into a single warning 
> > > by presenting them as notes.
> > > 
> > > Because for now notes aren't supported everywhere (and aren't always 
> > > supported really well), i guess it'd be necessary to at least provide an 
> > > option to make note-less reports before the checker is enabled by default 
> > > everywhere. In this case notes contain critical pieces of information and 
> > > as such cannot be really discarded.
> > > 
> > > I'm not sure that notes are providing a better user experience here than 
> > > simply saying that "field x.y->z is never initialized". With notes, the 
> > > user will have to scroll around (at least in scan-build) to find which 
> > > field is uninitialized.
> > > 
> > > Notes will probably also not decrease the number of reports too much 
> > > because in most cases there will be just one uninitialized field. Though 
> > > i agree that when there is more than one report, the user will be likely 
> > > to deal with all fields at once rather than separately.
> > > 
> > > So it's a bit wonky.
> > > 
> > > We might want to enable one mode or another in the checker depending on 
> > > the analyzer output flag. Probably in the driver 
> > > (`RenderAnalyzerOptions()`).
> > > 
> > > It'd be a good idea to land the checker into alpha first and fix this in 
> > > a follow-up patch because this review is already overweight.
> > > [...]i guess it'd be necessary to at least provide an option to make 
> > > note-less reports before the checker is enabled by default everywhere. In 
> > > this case notes contain critical pieces of information and as such cannot 
> > > be really discarded.
> > This can already be achieved with `-analyzer-config notes-as-events=true`. 
> > There no good reason however not to make an additional flag that would emit 
> > warnings instead of notes.
> > > I'm not sure that notes are providing a better user experience here than 
> > > simply saying that "field x.y->z is never initialized". With notes, the 
> > > user will have to scroll around (at least in scan-build) to find which 
> > > field is uninitialized.
> > This checker had a previous implementation that emitted a warning for each 
> > uninitialized field, instead of squeezing them in a single warning per 
> > constructor call. One of the major reasons behind rewriting it was that it 
> > emitted so many of them that it was difficult to differentiate which 
> > warning belonged to which constructor call. Also, in the case of the 
> > LLVM/Clang project, the warnings from this checker alone would be in the 
> > thousands.
> > > Notes will probably also not decrease the number of reports too much 
> > > because in most cases there will be just one uninitialized field.
> > While this is true to some extent, it's not uncommon that a single 
> > constructor call leaves 7 uninitialized -- in the LLVM/Clang project I 
> > found one that had over 30!
> > Since its practically impossible to determine whether this was a 
> > performance enhancement or just poor programming, the few cases where it is 
> > intentional, an object would emit many more warnings would make  make 
> > majority of the findings (where an object truly had only 1 or 2 fields 
> > uninit) a lot harder to spot in my opinion.
> > 
> > In general, I think one warning per constructor call makes the best user 
> > experience, but a temporary solution should be implemented until there's 
> > better support for notes.
> > 
> > I agree, this should be a topic of a follow-up patch.
> > 
> > This can already be achieved with `-analyzer-config notes-as-events=true`.
> 
> Yep. But the resulting user experience seems pretty bad to me. It was a good 
> quick proof-of-concept trick to test things, but the fact that notes aren't 
> path pieces is way too apparent in case of this checker. So i guess this flag 
> was a bad idea.
> 
> > Also, in the case of the LLVM/Clang project, the warnings from this checker 
> > alone would be in the thousands.
> 
> This makes me a bit worried, i wonder what's the reason why does the checker 
> shout so loudly on a codebase that doesn't seem to have actual uninitialized 
> value bugs all over the place.
> 
> Are any of these duplicates found in the same constructor for the same field 
> in different translation units? Suppose we discard the duplicates (if any) 
> and warn exactly once (across the whole LLVM) for every uninitialized field 
> (which is probably more than once per constructor). Then I wonder:
> 1. How many 

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:258-260
+Report->addNote(NoteBuf,
+PathDiagnosticLocation::create(FieldChain.getEndOfChain(),
+   
Context.getSourceManager()));

Szelethus wrote:
> NoQ wrote:
> > Aha, ok, got it, so you're putting the warning at the end of the 
> > constructor and squeezing all uninitialized fields into a single warning by 
> > presenting them as notes.
> > 
> > Because for now notes aren't supported everywhere (and aren't always 
> > supported really well), i guess it'd be necessary to at least provide an 
> > option to make note-less reports before the checker is enabled by default 
> > everywhere. In this case notes contain critical pieces of information and 
> > as such cannot be really discarded.
> > 
> > I'm not sure that notes are providing a better user experience here than 
> > simply saying that "field x.y->z is never initialized". With notes, the 
> > user will have to scroll around (at least in scan-build) to find which 
> > field is uninitialized.
> > 
> > Notes will probably also not decrease the number of reports too much 
> > because in most cases there will be just one uninitialized field. Though i 
> > agree that when there is more than one report, the user will be likely to 
> > deal with all fields at once rather than separately.
> > 
> > So it's a bit wonky.
> > 
> > We might want to enable one mode or another in the checker depending on the 
> > analyzer output flag. Probably in the driver (`RenderAnalyzerOptions()`).
> > 
> > It'd be a good idea to land the checker into alpha first and fix this in a 
> > follow-up patch because this review is already overweight.
> > [...]i guess it'd be necessary to at least provide an option to make 
> > note-less reports before the checker is enabled by default everywhere. In 
> > this case notes contain critical pieces of information and as such cannot 
> > be really discarded.
> This can already be achieved with `-analyzer-config notes-as-events=true`. 
> There no good reason however not to make an additional flag that would emit 
> warnings instead of notes.
> > I'm not sure that notes are providing a better user experience here than 
> > simply saying that "field x.y->z is never initialized". With notes, the 
> > user will have to scroll around (at least in scan-build) to find which 
> > field is uninitialized.
> This checker had a previous implementation that emitted a warning for each 
> uninitialized field, instead of squeezing them in a single warning per 
> constructor call. One of the major reasons behind rewriting it was that it 
> emitted so many of them that it was difficult to differentiate which warning 
> belonged to which constructor call. Also, in the case of the LLVM/Clang 
> project, the warnings from this checker alone would be in the thousands.
> > Notes will probably also not decrease the number of reports too much 
> > because in most cases there will be just one uninitialized field.
> While this is true to some extent, it's not uncommon that a single 
> constructor call leaves 7 uninitialized -- in the LLVM/Clang project I found 
> one that had over 30!
> Since its practically impossible to determine whether this was a performance 
> enhancement or just poor programming, the few cases where it is intentional, 
> an object would emit many more warnings would make  make majority of the 
> findings (where an object truly had only 1 or 2 fields uninit) a lot harder 
> to spot in my opinion.
> 
> In general, I think one warning per constructor call makes the best user 
> experience, but a temporary solution should be implemented until there's 
> better support for notes.
> 
> I agree, this should be a topic of a follow-up patch.
> 
> This can already be achieved with `-analyzer-config notes-as-events=true`.

Yep. But the resulting user experience seems pretty bad to me. It was a good 
quick proof-of-concept trick to test things, but the fact that notes aren't 
path pieces is way too apparent in case of this checker. So i guess this flag 
was a bad idea.

> Also, in the case of the LLVM/Clang project, the warnings from this checker 
> alone would be in the thousands.

This makes me a bit worried, i wonder what's the reason why does the checker 
shout so loudly on a codebase that doesn't seem to have actual uninitialized 
value bugs all over the place.

Are any of these duplicates found in the same constructor for the same field in 
different translation units? Suppose we discard the duplicates (if any) and 
warn exactly once (across the whole LLVM) for every uninitialized field (which 
is probably more than once per constructor). Then I wonder:
1. How many uninitialize fields would we be able to find this way?
2. How many of such fields were intentionally left uninitialized?
3. How many were found by deep inspection of the heap through field 

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

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus added a comment.

Thanks @NoQ  for taking the time to review my code!

I'm sorry that the patch became this long. It was an error on my part, and I 
completely get that the checker now takes an uncomfortably long time to read 
and understand. This is my first "bigger" project in the CSA, I learned a lot 
so far, and I'll definitely review how I should've handled developing and 
uploading it here better, so next time I will do my best to make my 
contributions a lot more reviewer-friendly.

> It's very unlikely that you have a single idea that takes 500 non-trivial 
> lines of code to express.

That's true. Since this was the first time I wrote anything the StaticAnalyzer 
project, I often found myself almost completely rewriting everything. Looking 
back, I still could've uploaded the code in parts, so notes taken, will do 
better next time!

It'll be some time before I sort everything out you commented on, just wanted 
to leave this here.




Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:258-260
+Report->addNote(NoteBuf,
+PathDiagnosticLocation::create(FieldChain.getEndOfChain(),
+   
Context.getSourceManager()));

NoQ wrote:
> Aha, ok, got it, so you're putting the warning at the end of the constructor 
> and squeezing all uninitialized fields into a single warning by presenting 
> them as notes.
> 
> Because for now notes aren't supported everywhere (and aren't always 
> supported really well), i guess it'd be necessary to at least provide an 
> option to make note-less reports before the checker is enabled by default 
> everywhere. In this case notes contain critical pieces of information and as 
> such cannot be really discarded.
> 
> I'm not sure that notes are providing a better user experience here than 
> simply saying that "field x.y->z is never initialized". With notes, the user 
> will have to scroll around (at least in scan-build) to find which field is 
> uninitialized.
> 
> Notes will probably also not decrease the number of reports too much because 
> in most cases there will be just one uninitialized field. Though i agree that 
> when there is more than one report, the user will be likely to deal with all 
> fields at once rather than separately.
> 
> So it's a bit wonky.
> 
> We might want to enable one mode or another in the checker depending on the 
> analyzer output flag. Probably in the driver (`RenderAnalyzerOptions()`).
> 
> It'd be a good idea to land the checker into alpha first and fix this in a 
> follow-up patch because this review is already overweight.
> [...]i guess it'd be necessary to at least provide an option to make 
> note-less reports before the checker is enabled by default everywhere. In 
> this case notes contain critical pieces of information and as such cannot be 
> really discarded.
This can already be achieved with `-analyzer-config notes-as-events=true`. 
There no good reason however not to make an additional flag that would emit 
warnings instead of notes.
> I'm not sure that notes are providing a better user experience here than 
> simply saying that "field x.y->z is never initialized". With notes, the user 
> will have to scroll around (at least in scan-build) to find which field is 
> uninitialized.
This checker had a previous implementation that emitted a warning for each 
uninitialized field, instead of squeezing them in a single warning per 
constructor call. One of the major reasons behind rewriting it was that it 
emitted so many of them that it was difficult to differentiate which warning 
belonged to which constructor call. Also, in the case of the LLVM/Clang 
project, the warnings from this checker alone would be in the thousands.
> Notes will probably also not decrease the number of reports too much because 
> in most cases there will be just one uninitialized field.
While this is true to some extent, it's not uncommon that a single constructor 
call leaves 7 uninitialized -- in the LLVM/Clang project I found one that had 
over 30!
Since its practically impossible to determine whether this was a performance 
enhancement or just poor programming, the few cases where it is intentional, an 
object would emit many more warnings would make  make majority of the findings 
(where an object truly had only 1 or 2 fields uninit) a lot harder to spot in 
my opinion.

In general, I think one warning per constructor call makes the best user 
experience, but a temporary solution should be implemented until there's better 
support for notes.

I agree, this should be a topic of a follow-up patch.




Comment at: test/Analysis/cxx-uninitialized-object.cpp:1412
+  struct RecordType;
+  // no-crash
+  RecordType *recptr; // expected-note{{uninitialized pointer 'this->recptr}}

whisperity wrote:
> What is this comment symbolising? Is this actually something the test 
> infrastructure picks up?
`// 

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Had a look at the actual code, came up with some comments, most are fairly 
minor, so good job! You did a good job explaining the overall idea, but a lot 
of specifics were difficult to understand, so i made a few comments on how to 
make the code a bit easier to read.

> I know, and I tried to look for ways to split this checker into smaller 
> logical chunks, but I didn't find a satisfying solution.

What i would have done:

1. A checker that straightforwardly iterates through immediate fields of the 
current object and reports uninitialized fields.
2. Add base classes support.
3. Heuristic for skipping sub-constructors could use a separate review.
4. Start skipping arrays and unions.
5. Add simple pointer dereference support - introduce chains of fields.
6. More heuristics - the one for symbolic regions, the one for system header 
fields, etc.

etc.

I'm asking for this because it's, like, actually difficult and painful to 
understand formal information in large chunks. One-solution-at-a-time would 
have been so much easier. It is really a bad idea to start by presenting a 
working solution; the great idea is to present a small non-working solution 
with a reasonable idea behind it, and then extend it "in place" as much as it 
seems necessary. It is very comfortable when parts of the patch with different 
"status" (main ideas, corner-case fixes, heuristics, refactoring) stay in 
separate patches. Alpha checkers are essentially branches: because our svn 
doesn't have branches, we have alpha checkers and off-by-default flags. Even 
before step 1, you should be totally fine with committing an empty checker with 
a single empty callback and a header comment - even on such "step 0." we would 
be able to discuss if your approach to the checker seems right or might have 
inevitable issues, and it could have saved a lot of time.

Essentially every part of the solution that you implement, if you think it 
might deserve a separate discussion, also deserves a separate patch. It is 
reasonable to split the work into logical chunks before you start it. It's very 
unlikely that you have a single idea that takes 500 non-trivial lines of code 
to express. Splitting up ideas so that they were easy to grasp is an invaluable 
effort. I had very positive experience with that recently, both as a coder and 
as a reviewer, so i encourage that a lot.




Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:47
+class FieldChainInfo {
+  using FieldChain = llvm::SmallVector;
+

At a glance it looks like a great use case for an immutable list. It's easy to 
use the immutable list as a stack, and it's also O(1) to copy/move and take 
snapshots of it.

Note that you copy 10 pointers every time you copy //or move// a `SmallVector`. 
Small vectors don't make much sense as fields of actively used data structures 
- their main purpose is to live on the stack as local variables.



Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:51-53
+  /// If this is a fieldchain whose last element is an uninitialized region of 
a
+  /// pointer type, this variable will store whether the pointer itself or the
+  /// pointee is uninitialized.

Please move this doc to the getter/setter function. That's where people expect 
to find it when they're reading code.



Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:58-59
+  FieldChainInfo() = default;
+  /// Delegates to the copy ctor and adds FR to Chain. Note that Chain cannot 
be
+  /// modified in a FieldChainInfo object after its construction.
+  FieldChainInfo(const FieldChainInfo , const FieldRegion *FR);

Do we really need the other field (`IsDereferenced`) to be mutable? Or maybe we 
can keep the whole object immutable? I.e., instead of `dereference()` we'll get 
a constructor that constructs a `FieldChainInfo` with the same chain but with 
the dereferenced flag set.



Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:86-89
+  StoreManager 
+  MemRegionManager 
+  SourceManager 
+  Store S;

All of these can be replaced with a single `ProgramStateRef` object.



Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:93
+  const bool IsPedantic;
+  bool IsChecked = false;
+  bool IsAnyFieldInitialized = false;

This field is worth documenting.



Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:156-157
+  //
+  // We'll traverse each node of the above graph with the appropiate one of
+  // these methods:
+  bool isUnionUninit(const TypedValueRegion *R);

Please explain what these methods do in a more direct manner. Something like 
"This method returns true if an uninitialized field was found within the 
region. It should only be used with regions of union-type 

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

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

Two minor comments.

With regards to the function naming, the problem with incremental counts is 
that they get out of sync, like they did with `fXpY` and such, and if someone 
wants to keep the count incremental down the file, adding any new test will 
result in an unnecessarily large patch. Perhaps you could use `void T_()` for 
the test that calls `T::T()`?




Comment at: test/Analysis/cxx-uninitialized-object.cpp:660
+}
+#endif // PEDANTIC
+class MultiPointerTest3 {

A newline between `#endif` and the next token is missing here.



Comment at: test/Analysis/cxx-uninitialized-object.cpp:1412
+  struct RecordType;
+  // no-crash
+  RecordType *recptr; // expected-note{{uninitialized pointer 'this->recptr}}

What is this comment symbolising? Is this actually something the test 
infrastructure picks up?


https://reviews.llvm.org/D45532



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus updated this revision to Diff 10.
Szelethus added a comment.

Forgot to rename the system header simulator file. Sorry about the spam :)


https://reviews.llvm.org/D45532

Files:
  include/clang/StaticAnalyzer/Checkers/Checkers.td
  lib/StaticAnalyzer/Checkers/CMakeLists.txt
  lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
  test/Analysis/Inputs/system-header-simulator-for-cxx-uninitialized-object.h
  test/Analysis/cxx-uninitialized-object-inheritance.cpp
  test/Analysis/cxx-uninitialized-object.cpp

Index: test/Analysis/cxx-uninitialized-object.cpp
===
--- /dev/null
+++ test/Analysis/cxx-uninitialized-object.cpp
@@ -0,0 +1,1449 @@
+//RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.uninitialized.UninitializedObject -analyzer-config cplusplus.uninitialized.UninitializedObject:Pedantic=true -std=c++11 -DPEDANTIC -verify %s
+
+//RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.uninitialized.UninitializedObject -std=c++11 -verify %s
+
+//===--===//
+// Default constructor test.
+//===--===//
+
+class CompilerGeneratedConstructorTest {
+  int a, b, c, d, e, f, g, h, i, j;
+
+public:
+  CompilerGeneratedConstructorTest() = default;
+};
+
+void f000() {
+  CompilerGeneratedConstructorTest();
+}
+
+#ifdef PEDANTIC
+class DefaultConstructorTest {
+  int a; // expected-note{{uninitialized field 'this->a'}}
+
+public:
+  DefaultConstructorTest();
+};
+
+DefaultConstructorTest::DefaultConstructorTest() = default;
+
+void f00() {
+  DefaultConstructorTest(); // expected-warning{{1 uninitialized field}}
+}
+#else
+class DefaultConstructorTest {
+  int a;
+
+public:
+  DefaultConstructorTest();
+};
+
+DefaultConstructorTest::DefaultConstructorTest() = default;
+
+void f00() {
+  DefaultConstructorTest();
+}
+#endif // PEDANTIC
+
+//===--===//
+// Initializer list test.
+//===--===//
+
+class InitListTest1 {
+  int a;
+  int b;
+
+public:
+  InitListTest1()
+  : a(1),
+b(2) {
+// All good!
+  }
+};
+
+void f01() {
+  InitListTest1();
+}
+
+class InitListTest2 {
+  int a;
+  int b; // expected-note{{uninitialized field 'this->b'}}
+
+public:
+  InitListTest2()
+  : a(3) {} // expected-warning{{1 uninitialized field}}
+};
+
+void f02() {
+  InitListTest2();
+}
+
+class InitListTest3 {
+  int a; // expected-note{{uninitialized field 'this->a'}}
+  int b;
+
+public:
+  InitListTest3()
+  : b(4) {} // expected-warning{{1 uninitialized field}}
+};
+
+void f03() {
+  InitListTest3();
+}
+
+//===--===//
+// Constructor body test.
+//===--===//
+
+class CtorBodyTest1 {
+  int a, b;
+
+public:
+  CtorBodyTest1() {
+a = 5;
+b = 6;
+// All good!
+  }
+};
+
+void f04() {
+  CtorBodyTest1();
+}
+
+class CtorBodyTest2 {
+  int a;
+  int b; // expected-note{{uninitialized field 'this->b'}}
+
+public:
+  CtorBodyTest2() {
+a = 7; // expected-warning{{1 uninitialized field}}
+  }
+};
+
+void f05() {
+  CtorBodyTest2();
+}
+
+class CtorBodyTest3 {
+  int a; // expected-note{{uninitialized field 'this->a'}}
+  int b;
+
+public:
+  CtorBodyTest3() {
+b = 8; // expected-warning{{1 uninitialized field}}
+  }
+};
+
+void f06() {
+  CtorBodyTest3();
+}
+
+#ifdef PEDANTIC
+class CtorBodyTest4 {
+  int a; // expected-note{{uninitialized field 'this->a'}}
+  int b; // expected-note{{uninitialized field 'this->b'}}
+
+public:
+  CtorBodyTest4() {}
+};
+
+void f07() {
+  CtorBodyTest4(); // expected-warning{{2 uninitialized fields}}
+}
+#else
+class CtorBodyTest4 {
+  int a;
+  int b;
+
+public:
+  CtorBodyTest4() {}
+};
+
+void f07() {
+  CtorBodyTest4();
+}
+#endif
+
+//===--===//
+// Constructor delegation test.
+//===--===//
+
+class CtorDelegationTest1 {
+  int a;
+  int b;
+
+public:
+  CtorDelegationTest1(int)
+  : a(9) {
+// leaves 'b' unintialized, but we'll never check this function
+  }
+
+  CtorDelegationTest1()
+  : CtorDelegationTest1(int{}) { // Initializing 'a'
+b = 10;
+// All good!
+  }
+};
+
+void f08() {
+  CtorDelegationTest1();
+}
+
+class CtorDelegationTest2 {
+  int a; // expected-note{{uninitialized field 'this->a'}}
+  int b;
+
+public:
+  CtorDelegationTest2(int)
+  : b(11) {
+// leaves 'a' unintialized, but we'll never check this function
+  }
+
+  CtorDelegationTest2()
+  : CtorDelegationTest2(int{}) { // expected-warning{{1 uninitialized field}}
+  }
+};
+
+void f09() {
+  CtorDelegationTest2();
+}
+

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

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus marked an inline comment as done.
Szelethus added inline comments.



Comment at: test/Analysis/cxx-uninitialized-object.cpp:526
+
+void f23p5() {
+  void *vptr = malloc(sizeof(int));

I haven't marked @a.sidorin's comment as done, but it disappeared because I 
renamed the file, so here it is:
> Could you please explain what is the logic of test naming?
To which I replied:
> The test files follow the strict structure that for each test case we'll 
> define a structure type and then call its constructor(s) in a function right 
> after it (functions are used to avoid zero initializations).
>
>To be honest, there is no real logic behind the naming of the functions, it is 
>only to keep the ODR. I used numbers in an increasing order, however I later 
>added there cases, so in between `f23` and `f24` I used `f23p5` and so on.
>
>I figured that the strict structure of the test files would avoid confusion. 
>Do you find it distracting?


https://reviews.llvm.org/D45532



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus updated this revision to Diff 144439.
Szelethus marked an inline comment as done.
Szelethus added a comment.

Renamed the checker to `cplusplus.uninitialized.UninitializedObject`.

I've read your comments regarding the category this should be in thoroughly, 
and should the clang-tidy category naming not be an issue, `cplusplus.bugprone` 
would've probably been the best option. However, since it is a concern, I did 
find many checkers in the core in the `uninitialized` subcategory, so I thought 
I'll place it in `cplusplus.uninitialized` for now. This could be a little too 
specific though, as I don't think many other checkers could be implemented that 
would reside in there.

What do you guys think?


https://reviews.llvm.org/D45532

Files:
  include/clang/StaticAnalyzer/Checkers/Checkers.td
  lib/StaticAnalyzer/Checkers/CMakeLists.txt
  lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
  test/Analysis/Inputs/system-header-simulator-ctor-uninitialized-member.h
  test/Analysis/cxx-uninitialized-object-inheritance.cpp
  test/Analysis/cxx-uninitialized-object.cpp

Index: test/Analysis/cxx-uninitialized-object.cpp
===
--- /dev/null
+++ test/Analysis/cxx-uninitialized-object.cpp
@@ -0,0 +1,1449 @@
+//RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.uninitialized.UninitializedObject -analyzer-config cplusplus.uninitialized.UninitializedObject:Pedantic=true -std=c++11 -DPEDANTIC -verify %s
+
+//RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.uninitialized.UninitializedObject -std=c++11 -verify %s
+
+//===--===//
+// Default constructor test.
+//===--===//
+
+class CompilerGeneratedConstructorTest {
+  int a, b, c, d, e, f, g, h, i, j;
+
+public:
+  CompilerGeneratedConstructorTest() = default;
+};
+
+void f000() {
+  CompilerGeneratedConstructorTest();
+}
+
+#ifdef PEDANTIC
+class DefaultConstructorTest {
+  int a; // expected-note{{uninitialized field 'this->a'}}
+
+public:
+  DefaultConstructorTest();
+};
+
+DefaultConstructorTest::DefaultConstructorTest() = default;
+
+void f00() {
+  DefaultConstructorTest(); // expected-warning{{1 uninitialized field}}
+}
+#else
+class DefaultConstructorTest {
+  int a;
+
+public:
+  DefaultConstructorTest();
+};
+
+DefaultConstructorTest::DefaultConstructorTest() = default;
+
+void f00() {
+  DefaultConstructorTest();
+}
+#endif // PEDANTIC
+
+//===--===//
+// Initializer list test.
+//===--===//
+
+class InitListTest1 {
+  int a;
+  int b;
+
+public:
+  InitListTest1()
+  : a(1),
+b(2) {
+// All good!
+  }
+};
+
+void f01() {
+  InitListTest1();
+}
+
+class InitListTest2 {
+  int a;
+  int b; // expected-note{{uninitialized field 'this->b'}}
+
+public:
+  InitListTest2()
+  : a(3) {} // expected-warning{{1 uninitialized field}}
+};
+
+void f02() {
+  InitListTest2();
+}
+
+class InitListTest3 {
+  int a; // expected-note{{uninitialized field 'this->a'}}
+  int b;
+
+public:
+  InitListTest3()
+  : b(4) {} // expected-warning{{1 uninitialized field}}
+};
+
+void f03() {
+  InitListTest3();
+}
+
+//===--===//
+// Constructor body test.
+//===--===//
+
+class CtorBodyTest1 {
+  int a, b;
+
+public:
+  CtorBodyTest1() {
+a = 5;
+b = 6;
+// All good!
+  }
+};
+
+void f04() {
+  CtorBodyTest1();
+}
+
+class CtorBodyTest2 {
+  int a;
+  int b; // expected-note{{uninitialized field 'this->b'}}
+
+public:
+  CtorBodyTest2() {
+a = 7; // expected-warning{{1 uninitialized field}}
+  }
+};
+
+void f05() {
+  CtorBodyTest2();
+}
+
+class CtorBodyTest3 {
+  int a; // expected-note{{uninitialized field 'this->a'}}
+  int b;
+
+public:
+  CtorBodyTest3() {
+b = 8; // expected-warning{{1 uninitialized field}}
+  }
+};
+
+void f06() {
+  CtorBodyTest3();
+}
+
+#ifdef PEDANTIC
+class CtorBodyTest4 {
+  int a; // expected-note{{uninitialized field 'this->a'}}
+  int b; // expected-note{{uninitialized field 'this->b'}}
+
+public:
+  CtorBodyTest4() {}
+};
+
+void f07() {
+  CtorBodyTest4(); // expected-warning{{2 uninitialized fields}}
+}
+#else
+class CtorBodyTest4 {
+  int a;
+  int b;
+
+public:
+  CtorBodyTest4() {}
+};
+
+void f07() {
+  CtorBodyTest4();
+}
+#endif
+
+//===--===//
+// Constructor delegation test.
+//===--===//
+
+class CtorDelegationTest1 {
+  int a;
+  int b;
+
+public:
+  CtorDelegationTest1(int)
+  : a(9) {
+// leaves 'b' unintialized, but we'll never check this function
+  }
+
+  

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

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus marked 3 inline comments as done.
Szelethus added a comment.

By the way, thank you all for taking the time to review my code!




Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:306
+
+  const RecordDecl *RD =
+  R->getValueType()->getAs()->getDecl()->getDefinition();

a.sidorin wrote:
> What will happen if we analyze a member which is a pointer to a structure 
> type and the structure is of incomplete type?
For that field `isPointerOrReferenceUninit` will be called. If it happens not 
to be null, unknown or undef, the CSA core will construct a symbolic region for 
it, and the checker always assumes that symbolic regions are initialized, 
meaning that we'll never call `isNonUnionUninit` for a field of incomplete type.

However, I did not cover this with tests, thanks for pointing it out!



Comment at: test/Analysis/ctor-uninitialized-member.cpp:554
+
+void f23p15() {
+  void *vptr = malloc(sizeof(int));

a.sidorin wrote:
> Could you please explain what is the logic of test naming?
The test files follow the strict structure that for each test case we'll define 
a structure type and then call its constructor(s) in a function right after it 
(functions are used to avoid zero initializations).

To be honest, there is no real logic behind the naming of the functions, it is 
only to keep the ODR. I used numbers in an increasing order, however I later 
added there cases, so in between `f23` and `f24` I used `f23p5` and so on.

I figured that the strict structure of the test files would avoid confusion. Do 
you find it distracting?


https://reviews.llvm.org/D45532



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus updated this revision to Diff 144119.
Szelethus added a comment.

Fixes according to inline comments.


https://reviews.llvm.org/D45532

Files:
  include/clang/StaticAnalyzer/Checkers/Checkers.td
  lib/StaticAnalyzer/Checkers/CMakeLists.txt
  lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp
  test/Analysis/Inputs/system-header-simulator-ctor-uninitialized-member.h
  test/Analysis/ctor-uninitialized-member-inheritance.cpp
  test/Analysis/ctor-uninitialized-member.cpp

Index: test/Analysis/ctor-uninitialized-member.cpp
===
--- /dev/null
+++ test/Analysis/ctor-uninitialized-member.cpp
@@ -0,0 +1,1449 @@
+//RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.cplusplus.CtorUninitializedMember -analyzer-config alpha.cplusplus.CtorUninitializedMember:Pedantic=true -std=c++11 -DPEDANTIC -verify %s
+
+//RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.cplusplus.CtorUninitializedMember -std=c++11 -verify %s
+
+//===--===//
+// Default constructor test.
+//===--===//
+
+class CompilerGeneratedConstructorTest {
+  int a, b, c, d, e, f, g, h, i, j;
+
+public:
+  CompilerGeneratedConstructorTest() = default;
+};
+
+void f000() {
+  CompilerGeneratedConstructorTest();
+}
+
+#ifdef PEDANTIC
+class DefaultConstructorTest {
+  int a; // expected-note{{uninitialized field 'this->a'}}
+
+public:
+  DefaultConstructorTest();
+};
+
+DefaultConstructorTest::DefaultConstructorTest() = default;
+
+void f00() {
+  DefaultConstructorTest(); // expected-warning{{1 uninitialized field}}
+}
+#else
+class DefaultConstructorTest {
+  int a;
+
+public:
+  DefaultConstructorTest();
+};
+
+DefaultConstructorTest::DefaultConstructorTest() = default;
+
+void f00() {
+  DefaultConstructorTest();
+}
+#endif // PEDANTIC
+
+//===--===//
+// Initializer list test.
+//===--===//
+
+class InitListTest1 {
+  int a;
+  int b;
+
+public:
+  InitListTest1()
+  : a(1),
+b(2) {
+// All good!
+  }
+};
+
+void f01() {
+  InitListTest1();
+}
+
+class InitListTest2 {
+  int a;
+  int b; // expected-note{{uninitialized field 'this->b'}}
+
+public:
+  InitListTest2()
+  : a(3) {} // expected-warning{{1 uninitialized field}}
+};
+
+void f02() {
+  InitListTest2();
+}
+
+class InitListTest3 {
+  int a; // expected-note{{uninitialized field 'this->a'}}
+  int b;
+
+public:
+  InitListTest3()
+  : b(4) {} // expected-warning{{1 uninitialized field}}
+};
+
+void f03() {
+  InitListTest3();
+}
+
+//===--===//
+// Constructor body test.
+//===--===//
+
+class CtorBodyTest1 {
+  int a, b;
+
+public:
+  CtorBodyTest1() {
+a = 5;
+b = 6;
+// All good!
+  }
+};
+
+void f04() {
+  CtorBodyTest1();
+}
+
+class CtorBodyTest2 {
+  int a;
+  int b; // expected-note{{uninitialized field 'this->b'}}
+
+public:
+  CtorBodyTest2() {
+a = 7; // expected-warning{{1 uninitialized field}}
+  }
+};
+
+void f05() {
+  CtorBodyTest2();
+}
+
+class CtorBodyTest3 {
+  int a; // expected-note{{uninitialized field 'this->a'}}
+  int b;
+
+public:
+  CtorBodyTest3() {
+b = 8; // expected-warning{{1 uninitialized field}}
+  }
+};
+
+void f06() {
+  CtorBodyTest3();
+}
+
+#ifdef PEDANTIC
+class CtorBodyTest4 {
+  int a; // expected-note{{uninitialized field 'this->a'}}
+  int b; // expected-note{{uninitialized field 'this->b'}}
+
+public:
+  CtorBodyTest4() {}
+};
+
+void f07() {
+  CtorBodyTest4(); // expected-warning{{2 uninitialized fields}}
+}
+#else
+class CtorBodyTest4 {
+  int a;
+  int b;
+
+public:
+  CtorBodyTest4() {}
+};
+
+void f07() {
+  CtorBodyTest4();
+}
+#endif
+
+//===--===//
+// Constructor delegation test.
+//===--===//
+
+class CtorDelegationTest1 {
+  int a;
+  int b;
+
+public:
+  CtorDelegationTest1(int)
+  : a(9) {
+// leaves 'b' unintialized, but we'll never check this function
+  }
+
+  CtorDelegationTest1()
+  : CtorDelegationTest1(int{}) { // Initializing 'a'
+b = 10;
+// All good!
+  }
+};
+
+void f08() {
+  CtorDelegationTest1();
+}
+
+class CtorDelegationTest2 {
+  int a; // expected-note{{uninitialized field 'this->a'}}
+  int b;
+
+public:
+  CtorDelegationTest2(int)
+  : b(11) {
+// leaves 'a' unintialized, but we'll never check this function
+  }
+
+  CtorDelegationTest2()
+  : CtorDelegationTest2(int{}) { // expected-warning{{1 uninitialized field}}
+  }
+};
+
+void f09() {
+  CtorDelegationTest2();
+}
+

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

[Aleksei Sidorin via Phabricator via cfe-commits]

a.sidorin added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:72
+  const FieldDecl *getEndOfChain() const { return Chain.back()->getDecl(); }
+  template  void toString(SmallString ) const;
+  friend struct FieldChainInfoComparator;

This can be rewritten like `void toString(SmallVectorImpl ) const;



Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:306
+
+  const RecordDecl *RD =
+  R->getValueType()->getAs()->getDecl()->getDefinition();

What will happen if we analyze a member which is a pointer to a structure type 
and the structure is of incomplete type?



Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:534
+
+  while (T.getTypePtrOrNull()) {
+if (T->isVoidPointerType())

while (!T.isNull())



Comment at: test/Analysis/ctor-uninitialized-member.cpp:554
+
+void f23p15() {
+  void *vptr = malloc(sizeof(int));

Could you please explain what is the logic of test naming?


https://reviews.llvm.org/D45532



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus updated this revision to Diff 143875.
Szelethus added a comment.

In this diff I

- added a `Pedantic` flag that is set to false by default to filter out results 
from objects that don't have a single field initialized,
- made it so that fields that are declared in system headers are now ignored,
- refactored `isFullyInitialized` to `hasUnintializedFields` (it returned true 
when there were in fact uninit fields),
- fixed everything mentioned in inline comments aside from the naming and the 
category,
- added TODOs for `FieldChainInfo::toString`, I decided to fix those in a later 
patch to keep the diff just a little bit smaller,
- added many more test cases, including tests for the `Pedantic` flag
- added support for arrays. Granted, they worked wonderfully with the checker 
before, but there was nothing mentioned about them in the code.

If you like how I implemented the `Pedantic` flag, then I think only the naming 
and choosing the correct category is left.

I also rechecked the entire LLVM/Clang project before the system header fix and 
after the fix with `Pedantic` set to true and set to false. Here are my 
findings:

How many reports did the checker emit?

- Without fields in system headers being ignored: 208 (only functional had some 
fields uninitialized)
- With fields in system headers being ignored and `Pedantic` set to true: 181
- With fields in system headers being ignored and `Pedantic` set to false: 150

Most of these are intentional, as a very large portion of the project is 
performance critical. I did however find some constructors with the checker 
that would benefit from having the rest of their fields initialized.
I also found some constructors that didn't use `= default` for no good reason.


https://reviews.llvm.org/D45532

Files:
  include/clang/StaticAnalyzer/Checkers/Checkers.td
  lib/StaticAnalyzer/Checkers/CMakeLists.txt
  lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp
  test/Analysis/Inputs/system-header-simulator-ctor-uninitialized-member.h
  test/Analysis/ctor-uninitialized-member-inheritance.cpp
  test/Analysis/ctor-uninitialized-member.cpp

Index: test/Analysis/ctor-uninitialized-member.cpp
===
--- /dev/null
+++ test/Analysis/ctor-uninitialized-member.cpp
@@ -0,0 +1,1404 @@
+//RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.cplusplus.CtorUninitializedMember -analyzer-config alpha.cplusplus.CtorUninitializedMember:Pedantic=true -std=c++11 -DPEDANTIC -verify %s
+
+//RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.cplusplus.CtorUninitializedMember -std=c++11 -verify %s
+
+//===--===//
+// Default constructor test.
+//===--===//
+
+class CompilerGeneratedConstructorTest {
+  int a, b, c, d, e, f, g, h, i, j;
+
+public:
+  CompilerGeneratedConstructorTest() = default;
+};
+
+void f000() {
+  CompilerGeneratedConstructorTest();
+}
+
+#ifdef PEDANTIC
+class DefaultConstructorTest {
+  int a; // expected-note{{uninitialized field 'this->a'}}
+
+public:
+  DefaultConstructorTest();
+};
+
+DefaultConstructorTest::DefaultConstructorTest() = default;
+
+void f00() {
+  DefaultConstructorTest(); // expected-warning{{1 uninitialized field}}
+}
+#else
+class DefaultConstructorTest {
+  int a;
+
+public:
+  DefaultConstructorTest();
+};
+
+DefaultConstructorTest::DefaultConstructorTest() = default;
+
+void f00() {
+  DefaultConstructorTest();
+}
+#endif // PEDANTIC
+
+//===--===//
+// Initializer list test.
+//===--===//
+
+class InitListTest1 {
+  int a;
+  int b;
+
+public:
+  InitListTest1()
+  : a(1),
+b(2) {
+// All good!
+  }
+};
+
+void f01() {
+  InitListTest1();
+}
+
+class InitListTest2 {
+  int a;
+  int b; // expected-note{{uninitialized field 'this->b'}}
+
+public:
+  InitListTest2()
+  : a(3) {} // expected-warning{{1 uninitialized field}}
+};
+
+void f02() {
+  InitListTest2();
+}
+
+class InitListTest3 {
+  int a; // expected-note{{uninitialized field 'this->a'}}
+  int b;
+
+public:
+  InitListTest3()
+  : b(4) {} // expected-warning{{1 uninitialized field}}
+};
+
+void f03() {
+  InitListTest3();
+}
+
+//===--===//
+// Constructor body test.
+//===--===//
+
+class CtorBodyTest1 {
+  int a, b;
+
+public:
+  CtorBodyTest1() {
+a = 5;
+b = 6;
+// All good!
+  }
+};
+
+void f04() {
+  CtorBodyTest1();
+}
+
+class CtorBodyTest2 {
+  int a;
+  int b; // expected-note{{uninitialized field 'this->b'}}
+
+public:
+  CtorBodyTest2() {
+a = 7; // expected-warning{{1 uninitialized field}}
+  }
+};
+
+void f05() {
+  CtorBodyTest2();
+}
+
+class 

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

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus added a comment.

I'd also like to point out that as I mentioned before, the checker's name 
itself is misleading (it is a leftover from an earlier implementation of  this 
checker). Here are just some ideas I came up with:

- UninitializedObjectChecker
- UninitializedFieldsChecker
- UninitializedFieldsAfterConstructionChecker
- UninitializedMembersChecker
- UninitializedMembersAfterConstructionChecker

Of these I like the first the most, but I'm open for anything, if you have an 
idea for it.

In https://reviews.llvm.org/D45532#1075789, @NoQ wrote:

> Guys, what do you think about a checker that warns on uninitialized fields 
> only when at least one field is initialized? I'd be much more confident about 
> turning such check on by default. We can still keep a `pedantic` version.


Sounds good! I just finished implementing it along with a few minor (like some 
TODOs and fixes according to inline comments) and not-so-minor (like ignoring 
fields from system headers)  changes. I'll update the diff and post results on 
it once I finish checking the LLVM/Clang project. I feel very confident about 
the upcoming version.


https://reviews.llvm.org/D45532



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Guys, what do you think about a checker that warns on uninitialized fields only 
when at least one field is initialized? I'd be much more confident about 
turning such check on by default. We can still keep a `pedantic` version.

> I can say with confidence that CodeChecker does not break if the same 
> category name is used by two different analyzers. Does the same stand for 
> XCode / Scan-Build?

XCode and Scan-Build only support one analyzer, so it won't be a problem (at 
least not until more analyzers get supported).

But in general if we do overlapping names, we must be super sure that they mean 
the same thing in both analyzers, so that we didn't end up with a user 
interface in which one of the relatively common task would be to enable 
clang-tidy "bugprone" checkers and disable static analyzer "bugprone" checkers 
but the user would have to list all checkers by name in order to accomplish 
that because they all stay in the same package.


https://reviews.llvm.org/D45532



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

@george.karpenkov @NoQ `bugprone.` as a category sounds nice. It also nicely 
corresponds to the Clang-Tidy `bugprone-` category. It would not be nice to 
further fragment the "top levels" of checker categories.

I can say with confidence that CodeChecker does not break if the same category 
name is used by two different analyzers. Does the same stand for XCode / 
Scan-Build? In this case, introducing the `bugprone` category with the same 
principle that is behind Tidy's one is a good step.




Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:162
+/// We can't know the type of the region that a void pointer points to, so FD
+/// can't be analyzed if this function return true for it.
+bool isVoidPointer(const FieldDecl *FD);

george.karpenkov wrote:
> "returns"
Actually, this explanation is superfluous. I believe

Returns if FD can be (transitively) dereferenced to a void* (void**, ...). 
The type of the region behind a void pointer isn't known, and thus FD can not 
be analyzed.

should suffice?



Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:212
+
+  std::string WarningMsg = std::to_string(UninitFields.size()) +
+   " uninitialized field" +

george.karpenkov wrote:
> nitpicking: llvm::Twine is normally used for such constructs
I have also suggested the usage of Twine for this line (it's just the diff that 
got out of sync with the line numbers!), but I don't recall what @Szelethus' 
concern was about them. In case we have move semantics enabled, this line will 
compile into using the moving concatenator.


https://reviews.llvm.org/D45532



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov added a comment.

`antipatterns` or `security` could be another potential category name.


https://reviews.llvm.org/D45532



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov added a comment.

The code looks good apart from a few minor nits. I think I would prefer a new 
category created for this checker instead of using `alpha`; let's see what @NoQ 
has to say.




Comment at: include/clang/StaticAnalyzer/Checkers/Checkers.td:305
+ HelpText<"Reports uninitialized members in constructors">,
+ DescFile<"CtorUninitializedMemberChecker.cpp">;
+

`alpha` might not be a right place for this checker.
The  meaning of `alpha` seems to be "this checker is currently too immature to 
be used by default because it generates too many FPs".

This is not the case for the uninitialized-fields checker, as it finds non-bugs 
by design, and each project might want to decide whether using such checker is 
a right fit.
I would suggest introducing a new category here, e.g. `bugprone` (other 
suggestions: `lint`?)
@NoQ any objections?



Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:162
+/// We can't know the type of the region that a void pointer points to, so FD
+/// can't be analyzed if this function return true for it.
+bool isVoidPointer(const FieldDecl *FD);

"returns"



Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:212
+
+  std::string WarningMsg = std::to_string(UninitFields.size()) +
+   " uninitialized field" +

nitpicking: llvm::Twine is normally used for such constructs



Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:448
+bool isVoidPointer(const FieldDecl *FD) {
+  QualType T = FD->getType();
+

Might be a naive question, but what about a chain intermixing references and 
pointers?
Wouldn't it be simpler to write

```
while (T)
  if (T->isVoidPointerType())
return true;
  T = T->getPointeeType();
```

?



Comment at: test/Analysis/ctor-uninitialized-member.cpp:817
+UnionWithRecord(){};
+  } u; // xpected-note{{uninitialized field 'this->u'}}
+

What happened to `e`? Here and in all notes below.


https://reviews.llvm.org/D45532



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus updated this revision to Diff 142760.
Szelethus added a comment.

In https://reviews.llvm.org/D45532#1068700, @Szelethus wrote:

> Also, I managed to cause a crash with the class `linked_ptr_internal` from 
> google's boringssl when I analyzed the grpc project. I'll look deeper into 
> this, but I have a strong suspicion that the error lies within the CSA core.


I was very much wrong on that, the checker didn't handle cyclic references. I 
fixed that and some other crashes I encountered when checking the LLVM project.

To summarize, in this diff I

- added some more test cases,
- fixed crashes,
- removed unnecessary headers,
- fixed some typos in the documentation.


https://reviews.llvm.org/D45532

Files:
  include/clang/StaticAnalyzer/Checkers/Checkers.td
  lib/StaticAnalyzer/Checkers/CMakeLists.txt
  lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp
  test/Analysis/ctor-uninitialized-member-inheritance.cpp
  test/Analysis/ctor-uninitialized-member.cpp

Index: test/Analysis/ctor-uninitialized-member.cpp
===
--- /dev/null
+++ test/Analysis/ctor-uninitialized-member.cpp
@@ -0,0 +1,1122 @@
+//RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.cplusplus.CtorUninitializedMember -std=c++11 -verify %s
+
+//===--===//
+// Default constructor test.
+//===--===//
+
+class CompilerGeneratedConstructorTest {
+  int a, b, c, d, e, f, g, h, i, j;
+
+public:
+  CompilerGeneratedConstructorTest() = default;
+};
+
+void f000() {
+  CompilerGeneratedConstructorTest();
+}
+
+class DefaultConstructorTest {
+  int a; // expected-note{{uninitialized field 'this->a'}}
+
+public:
+  DefaultConstructorTest();
+};
+
+DefaultConstructorTest::DefaultConstructorTest() = default;
+
+void f00() {
+  DefaultConstructorTest(); // expected-warning{{1 uninitialized field}}
+}
+
+//===--===//
+// Initializer list test.
+//===--===//
+
+class InitListTest1 {
+  int a;
+  int b;
+
+public:
+  InitListTest1()
+  : a(1),
+b(2) {
+// All good!
+  }
+};
+
+void f01() {
+  InitListTest1();
+}
+
+class InitListTest2 {
+  int a;
+  int b; // expected-note{{uninitialized field 'this->b'}}
+
+public:
+  InitListTest2()
+  : a(3) {} // expected-warning{{1 uninitialized field}}
+};
+
+void f02() {
+  InitListTest2();
+}
+
+class InitListTest3 {
+  int a; // expected-note{{uninitialized field 'this->a'}}
+  int b;
+
+public:
+  InitListTest3()
+  : b(4) {} // expected-warning{{1 uninitialized field}}
+};
+
+void f03() {
+  InitListTest3();
+}
+
+//===--===//
+// Constructor body test.
+//===--===//
+
+class CtorBodyTest1 {
+  int a, b;
+
+public:
+  CtorBodyTest1() {
+a = 5;
+b = 6;
+// All good!
+  }
+};
+
+void f04() {
+  CtorBodyTest1();
+}
+
+class CtorBodyTest2 {
+  int a;
+  int b; // expected-note{{uninitialized field 'this->b'}}
+
+public:
+  CtorBodyTest2() {
+a = 7; // expected-warning{{1 uninitialized field}}
+  }
+};
+
+void f05() {
+  CtorBodyTest2();
+}
+
+class CtorBodyTest3 {
+  int a; // expected-note{{uninitialized field 'this->a'}}
+  int b;
+
+public:
+  CtorBodyTest3() {
+b = 8; // expected-warning{{1 uninitialized field}}
+  }
+};
+
+void f06() {
+  CtorBodyTest3();
+}
+
+class CtorBodyTest4 {
+  int a; // expected-note{{uninitialized field 'this->a'}}
+  int b; // expected-note{{uninitialized field 'this->b'}}
+
+public:
+  CtorBodyTest4() {}
+};
+
+void f07() {
+  CtorBodyTest4(); // expected-warning{{2 uninitialized fields}}
+}
+
+//===--===//
+// Constructor delegation test.
+//===--===//
+
+class CtorDelegationTest1 {
+  int a;
+  int b;
+
+public:
+  CtorDelegationTest1(int)
+  : a(9) {
+// leaves 'b' unintialized, but we'll never check this function
+  }
+
+  CtorDelegationTest1()
+  : CtorDelegationTest1(int{}) { // Initializing 'a'
+b = 10;
+// All good!
+  }
+};
+
+void f08() {
+  CtorDelegationTest1();
+}
+
+class CtorDelegationTest2 {
+  int a; // expected-note{{uninitialized field 'this->a'}}
+  int b;
+
+public:
+  CtorDelegationTest2(int)
+  : b(11) {
+// leaves 'a' unintialized, but we'll never check this function
+  }
+
+  CtorDelegationTest2()
+  : CtorDelegationTest2(int{}) { // expected-warning{{1 uninitialized field}}
+  }
+};
+
+void f09() {
+  CtorDelegationTest2();
+}
+
+//===--===//
+// Tests for classes containing records.

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

[Henry Wong via Phabricator via cfe-commits]

MTC added a comment.

In https://reviews.llvm.org/D45532#1069683, @whisperity wrote:

> There is something that came up in my mind:
>
> Consider a construct like this:
>
>   class A
>   {
> A()
> {
>   memset(X, 0, 10 * sizeof(int));
> }
>  
> int X[10];
>   };
>
>
> I think it's worthy for a test case if this `X` is considered unitialised at 
> the end of the constructor or not. (And if not, a ticket, or a fix for SA in 
> a subpatch.)


Just for a reminding, there is a very primitive patch to model `memset()`, see 
D44934 . And if we think that `memset` is a 
way of initialization, we may need to distinguish between POD objects and 
non-POD Objects.


https://reviews.llvm.org/D45532



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Whisperity via Phabricator via cfe-commits]

whisperity requested changes to this revision.
whisperity added a comment.
This revision now requires changes to proceed.

There is something that came up in my mind:

Consider a construct like this:

  class A
  {
A()
{
  memset(X, 0, 10 * sizeof(int));
}
  
int X[10];
  };

I think it's worthy for a test case if this `X` is considered unitialised at 
the end of the constructor or not. (And if not, a ticket, or a fix for SA in a 
subpatch.)


https://reviews.llvm.org/D45532



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

In https://reviews.llvm.org/D45532#1068700, @Szelethus wrote:

> In https://reviews.llvm.org/D45532#1068647, @dkrupp wrote:
>
> > This bug report also mentions assignment operator. But for that a warning 
> > may be not so useful. In that case the members of the assigned to object 
> > should have some initialized value already which the programmer may not 
> > want to overwrite in the assignment operator.
>
>
> I believe there's a checker for that already, but I'm really not sure whether 
> `UndefinedAssignmentChecker` covers all such cases.


Indeed, there are two different targets for analysis here:

- The `rhs` of the assignment contains an uninitialised value, that is 
copied/moved into the `this` of the assignment. --> This //should// be checked 
by `UndefinedAssignmentChecker`.
- Not every field is initialised by the assignment operator. This one, I 
believe, is hard to check and prove, and also feels superfluous. As @NoQ wrote 
earlier, the amount of reports this checker emits in itself is, at least 
expected to be, huge, and while it can be a valid programming approach that a 
ctor must initialise all, tracking in the CSA on an `operator=` that the 
`this`-side had something uninitialised but it was also not initialised by the 
assignment? Hard, and also not very useful, as far as I see.



In https://reviews.llvm.org/D45532#1064652, @NoQ wrote:

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


Where I can vision the usefulness of this checker **the most** is code that is 
evolving. There are many communities and codebases where coding standards and 
practices aren't laid out in a well-formed manner like LLVM has. There are also 
projects which are traditionally C code that has evolved into C++-ish code. 
When moving into C++, people start to realise they can write things like 
constructors, so they write them, but then leave out some members, and we can 
only guess (and **perhaps** make use of other checkers related to dereference 
or read of undefineds) what needs to be initialised, what is used without 
initialisation.

This checker is more close to something like a `bugprone-` Tidy matcher from 
the user's perspective, but proper analysis requires it to be executed in the 
SA.

A valid constriction of what this checker can find could be members that can 
"seemingly" be only be set in the constructor, there are no write operations or 
public exposure on them.

I have reviewed some findings of the checker, consider the following very 
trivial example:

  #define HEAD_ELEMENT_SPECIFIER 0xDEADF00D // some magic yadda
  
  struct linked_list;
  
  struct linked_list
  {
  int elem;
  linked_list* next;
  };
  
  class some_information_chain_class
  {
public:
  some_information_chain_class() : iList(new linked_list())
  {
iList->elem = HEAD_ELEMENT_SPECIFIER;
  }
  
private:
  linked_list* iList;
  };

In this case, the "next" is in many cases remain as a garbage value. Of course, 
the checker cannot know, if, for example, there is also a `count` field and we 
don't rely on `next == nullptr` to signify the end of the list. But this can 
very well be a mistake from the programmer's end.

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

I'm not entirely sure what you mean by "changing the behaviour of the program". 
As far as I can see, this checker only reads information from the SA, so it 
should not mess up any preconception set or state potentially used by other 
checkers.

---

In https://reviews.llvm.org/D45532#1065716, @Szelethus wrote:

> I didn't implement anything explicitly to suppress warnings, so if there is 
> nothing in the CSA by default for it, I'm afraid this isn't possible (just 
> yet).


So far the only thing I can think of is coming up with new command-line 
arguments that can fine-tune the behaviour of the checker - but in this case, 
you can just as well just toggle the whole thing to be disabled.

An improvement heuristic (implemented in a later patch, toggleable with a 
command-line option perhaps?), as discussed above, would be checking //only// 
for members for whom there isn't any "setter" capability anywhere in the type.


https://reviews.llvm.org/D45532



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus updated this revision to Diff 142624.
Szelethus added a comment.

> Would be interesting to extend this checker (maybe in an upcoming patch) to 
> report on uninitialized members not only in constructors, but also copy 
> constructors and move constructors.

Added 3 new test cases. to cover them. Interestingly, move constructors don't 
emit any warnings - the core can only assert that the fields after a move 
construction are valid (returns `true` for Val::isValid()`).
Came to think of it, I'm not 100% confident in the checkers name. It could be 
misleading, as this checker doesn't check constructors, but rather objects 
after construction. The end of a constructor call is only the point at which we 
know that analysis can be done.

> This bug report also mentions assignment operator. But for that a warning may 
> be not so useful. In that case the members of the assigned to object should 
> have some initialized value already which the programmer may not want to 
> overwrite in the assignment operator.

I believe there's a checker for that already, but I'm really not sure whether 
`UndefinedAssignmentChecker` covers all such cases.

Also, I managed to cause a crash with the class `linked_ptr_internal` from 
google's boringssl when I analyzed the grpc project. I'll look deeper into 
this, but I have a strong suspicion that the error lies within the CSA core.


https://reviews.llvm.org/D45532

Files:
  include/clang/StaticAnalyzer/Checkers/Checkers.td
  lib/StaticAnalyzer/Checkers/CMakeLists.txt
  lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp
  test/Analysis/ctor-uninitialized-member-inheritance.cpp
  test/Analysis/ctor-uninitialized-member.cpp

Index: test/Analysis/ctor-uninitialized-member.cpp
===
--- /dev/null
+++ test/Analysis/ctor-uninitialized-member.cpp
@@ -0,0 +1,960 @@
+//RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.cplusplus.CtorUninitializedMember -std=c++11 -verify %s
+
+//===--===//
+// Default constructor test.
+//===--===//
+
+class CompilerGeneratedConstructorTest {
+  int a, b, c, d, e, f, g, h, i, j;
+
+public:
+  CompilerGeneratedConstructorTest() = default;
+};
+
+void f000() {
+  CompilerGeneratedConstructorTest();
+}
+
+class DefaultConstructorTest {
+  int a; // expected-note{{uninitialized field 'this->a'}}
+
+public:
+  DefaultConstructorTest();
+};
+
+DefaultConstructorTest::DefaultConstructorTest() = default;
+
+void f00() {
+  DefaultConstructorTest(); // expected-warning{{1 uninitialized field}}
+}
+
+//===--===//
+// Initializer list test.
+//===--===//
+
+class InitListTest1 {
+  int a;
+  int b;
+
+public:
+  InitListTest1()
+  : a(1),
+b(2) {
+// All good!
+  }
+};
+
+void f01() {
+  InitListTest1();
+}
+
+class InitListTest2 {
+  int a;
+  int b; // expected-note{{uninitialized field 'this->b'}}
+
+public:
+  InitListTest2()
+  : a(3) {} // expected-warning{{1 uninitialized field}}
+};
+
+void f02() {
+  InitListTest2();
+}
+
+class InitListTest3 {
+  int a; // expected-note{{uninitialized field 'this->a'}}
+  int b;
+
+public:
+  InitListTest3()
+  : b(4) {} // expected-warning{{1 uninitialized field}}
+};
+
+void f03() {
+  InitListTest3();
+}
+
+//===--===//
+// Constructor body test.
+//===--===//
+
+class CtorBodyTest1 {
+  int a, b;
+
+public:
+  CtorBodyTest1() {
+a = 5;
+b = 6;
+// All good!
+  }
+};
+
+void f04() {
+  CtorBodyTest1();
+}
+
+class CtorBodyTest2 {
+  int a;
+  int b; // expected-note{{uninitialized field 'this->b'}}
+
+public:
+  CtorBodyTest2() {
+a = 7; // expected-warning{{1 uninitialized field}}
+  }
+};
+
+void f05() {
+  CtorBodyTest2();
+}
+
+class CtorBodyTest3 {
+  int a; // expected-note{{uninitialized field 'this->a'}}
+  int b;
+
+public:
+  CtorBodyTest3() {
+b = 8; // expected-warning{{1 uninitialized field}}
+  }
+};
+
+void f06() {
+  CtorBodyTest3();
+}
+
+class CtorBodyTest4 {
+  int a; // expected-note{{uninitialized field 'this->a'}}
+  int b; // expected-note{{uninitialized field 'this->b'}}
+
+public:
+  CtorBodyTest4() {}
+};
+
+void f07() {
+  CtorBodyTest4(); // expected-warning{{2 uninitialized fields}}
+}
+
+//===--===//
+// Constructor delegation test.
+//===--===//
+
+class CtorDelegationTest1 {
+  int a;
+  int b;
+
+public:
+  CtorDelegationTest1(int)
+  : a(9) {
+// leaves 'b' unintialized, but we'll never check this function
+  }
+
+  

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

[Daniel Krupp via Phabricator via cfe-commits]

dkrupp added a comment.

Would be interesting to extend this checker (maybe in an upcoming patch) to 
report on uninitialized members not only in constructors, but also copy 
constructors and move constructors.

See related https://bugs.llvm.org/show_bug.cgi?id=37086

This bug report also mentions assignment operator. But for that a warning may 
be not so useful. In that case the members of the assigned to object should 
have some initialized value already which the programmer may not want to 
overwrite in the assignment operator.


https://reviews.llvm.org/D45532



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus marked 24 inline comments as done.
Szelethus added a comment.

Note that there was a comment made about the test files being too long. I still 
haven't split them, as I didn't find a good "splitting point". Is this okay, or 
shall I try to split these into numerous smaller ones?


https://reviews.llvm.org/D45532



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus updated this revision to Diff 142581.
Szelethus marked 2 inline comments as done.
Szelethus edited the summary of this revision.
Szelethus added a comment.

Among many other things:

- The checker class is now on top of the file.
- Reviewed all comments, fixed typos, tried to make the general idea more 
understandable.
- Removed all (at least, all I could find) unnecessary functions and function 
arguments.
- Removed support for unions entirely.


https://reviews.llvm.org/D45532

Files:
  include/clang/StaticAnalyzer/Checkers/Checkers.td
  lib/StaticAnalyzer/Checkers/CMakeLists.txt
  lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp
  test/Analysis/ctor-uninitialized-member-inheritance.cpp
  test/Analysis/ctor-uninitialized-member.cpp

Index: test/Analysis/ctor-uninitialized-member.cpp
===
--- /dev/null
+++ test/Analysis/ctor-uninitialized-member.cpp
@@ -0,0 +1,899 @@
+//RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.cplusplus.CtorUninitializedMember -std=c++11 -verify %s
+
+//===--===//
+// Default constructor test.
+//===--===//
+
+class DefaultConstructorTest {
+  int a, b, c, d, e, f, g, h, i, j;
+
+public:
+  DefaultConstructorTest() = default;
+};
+
+void f00() {
+  DefaultConstructorTest();
+}
+
+//===--===//
+// Initializer list test.
+//===--===//
+
+class InitListTest1 {
+  int a;
+  int b;
+
+public:
+  InitListTest1()
+  : a(1),
+b(2) {
+// All good!
+  }
+};
+
+void f01() {
+  InitListTest1();
+}
+
+class InitListTest2 {
+  int a;
+  int b; // expected-note{{uninitialized field 'this->b'}}
+
+public:
+  InitListTest2()
+  : a(3) {} // expected-warning{{1 uninitialized field}}
+};
+
+void f02() {
+  InitListTest2();
+}
+
+class InitListTest3 {
+  int a; // expected-note{{uninitialized field 'this->a'}}
+  int b;
+
+public:
+  InitListTest3()
+  : b(4) {} // expected-warning{{1 uninitialized field}}
+};
+
+void f03() {
+  InitListTest3();
+}
+
+//===--===//
+// Constructor body test.
+//===--===//
+
+class CtorBodyTest1 {
+  int a, b;
+
+public:
+  CtorBodyTest1() {
+a = 5;
+b = 6;
+// All good!
+  }
+};
+
+void f04() {
+  CtorBodyTest1();
+}
+
+class CtorBodyTest2 {
+  int a;
+  int b; // expected-note{{uninitialized field 'this->b'}}
+
+public:
+  CtorBodyTest2() {
+a = 7; // expected-warning{{1 uninitialized field}}
+  }
+};
+
+void f05() {
+  CtorBodyTest2();
+}
+
+class CtorBodyTest3 {
+  int a; // expected-note{{uninitialized field 'this->a'}}
+  int b;
+
+public:
+  CtorBodyTest3() {
+b = 8; // expected-warning{{1 uninitialized field}}
+  }
+};
+
+void f06() {
+  CtorBodyTest3();
+}
+
+class CtorBodyTest4 {
+  int a; // expected-note{{uninitialized field 'this->a'}}
+  int b; // expected-note{{uninitialized field 'this->b'}}
+
+public:
+  CtorBodyTest4() {}
+};
+
+void f07() {
+  CtorBodyTest4(); // expected-warning{{2 uninitialized fields}}
+}
+
+//===--===//
+// Constructor delegation test.
+//===--===//
+
+class CtorDelegationTest1 {
+  int a;
+  int b;
+
+public:
+  CtorDelegationTest1(int)
+  : a(9) {
+// leaves 'b' unintialized, but we'll never check this function
+  }
+
+  CtorDelegationTest1()
+  : CtorDelegationTest1(int{}) { // Initializing 'a'
+b = 10;
+// All good!
+  }
+};
+
+void f08() {
+  CtorDelegationTest1();
+}
+
+class CtorDelegationTest2 {
+  int a; // expected-note{{uninitialized field 'this->a'}}
+  int b;
+
+public:
+  CtorDelegationTest2(int)
+  : b(11) {
+// leaves 'a' unintialized, but we'll never check this function
+  }
+
+  CtorDelegationTest2()
+  : CtorDelegationTest2(int{}) { // expected-warning{{1 uninitialized field}}
+  }
+};
+
+void f09() {
+  CtorDelegationTest2();
+}
+
+//===--===//
+// Tests for classes containing records.
+//===--===//
+
+class ContainsRecordTest1 {
+  struct RecordType {
+int x;
+int y;
+  } rec;
+  int c, d;
+
+public:
+  ContainsRecordTest1()
+  : rec({12, 13}),
+c(14),
+d(15) {
+// All good!
+  }
+};
+
+void f10() {
+  ContainsRecordTest1();
+}
+
+class ContainsRecordTest2 {
+  struct RecordType {
+int x;
+int y; // expected-note{{uninitialized field 'this->rec.y'}}
+  } rec;
+  int c, d;
+
+public:
+  ContainsRecordTest2()
+  : c(16),
+d(17) 

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

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus marked 6 inline comments as done.
Szelethus added a comment.

I'm about to update the diff, I changed a quite a lot of stuff, so I'm not sure 
that I'd be able to respond to these inline comments.




Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:31
+/// every other element is a field, and the element that precedes it is the
+/// object that contains it.
+class FieldChainInfo {

xazax.hun wrote:
> As far as I understand, for every operation, the only relevant contribution 
> of the non-last elements in the chain is the diagnostic message.
> I wonder if you would build a string instead of a FieldRegion chain and only 
> store the last FieldRegion would make this more explicit in the code. 
I experimented with a number of implementations along this idea, but I just 
couldn't get it to work. Here are my findings:
* `Twine`-s aren't meant to be copied, so using them didn't work out
* `StringRef`s for some reason get invalidated by the time it'd make a call to 
`FieldChainInfo::getAsString()`
* I didn't like the idea of storing the string because it'd not only impact 
performance greatly, but it'd also make the code a lot harder to understand, as 
opposed to making it more explicit

I did however try to make the implementation more easy to understand.



Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:35
+
+  FieldChain Chain;
+  // If this is a fieldchain whose last element is an uninitialized region of a

xazax.hun wrote:
> I was wondering, do you need the chain at all? I think a field region might 
> be sufficient. The enclosing object of the field should be accessible by 
> querying the super region of the field region.
I tried it, but that approach made it impossible to include pointers and 
references in the fieldchain.



Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:218
+
+  std::string WarningMsg = std::to_string(UninitFields.size()) +
+   " uninitialized field" +

whisperity wrote:
> Maybe one could use a Twine or a string builder to avoid all these 
> unneccessary string instantiations? Or `std::string::append()`?
Doesn't move semantics take care of that? I'm not too sure on this one.


https://reviews.llvm.org/D45532



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:35
+
+  FieldChain Chain;
+  // If this is a fieldchain whose last element is an uninitialized region of a

I was wondering, do you need the chain at all? I think a field region might be 
sufficient. The enclosing object of the field should be accessible by querying 
the super region of the field region.


https://reviews.llvm.org/D45532



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Henry Wong via Phabricator via cfe-commits]

MTC added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:10
+//
+//  This file defines a checker that checks cunstructors for possibly
+//  uninitialized fields.

typo :)  `cunstructors` -> `constructors`



Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:54
+  StringRef getAsString(SmallString<200> ) const;
+  friend class FieldChainInfoComparator;
+};

When I apply this patch, hit a compiler warning. Maybe `friend struct` is 
better.
```
warning: 'FieldChainInfoComparator' defined as a struct here but previously 
declared as a class [-Wmismatched-tags]
```


https://reviews.llvm.org/D45532



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:187
+
+  if (isCalledByConstructor(Context))
+return;

xazax.hun wrote:
> Szelethus wrote:
> > whisperity wrote:
> > > I think somewhere there should be a bit of reasoning why exactly these 
> > > cases are ignored by the checker.
> > At this function's declaration I have some comments describing what this 
> > does and why it does it. Did you find it insufficient?
> I am a bit surprised why these errors are not deduplicated by the analyzer. 
> Maybe it would worth some investigation?
Copied from `BugReport`s constructor documentation  for uniqueing:
>The reports that have the same report location, description, bug type, and 
>ranges are uniqued - only one of the equivalent reports will be presented to 
>the user. [...]
For this code snippet:
```
struct A {
   struct B {
  int x, y;
  
  B() {}
   } b;
   int w;

   A() {
  b = B();
   }
};
```
the warning message after a call to `A::A()` would be "3 uninitialized 
fields[...]", and for `B::B()` inside `A`s constructor would be "2 
uninitialized fields[...]", so it wouldn't be filtered out.



Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:292
+  if (LocalChain)
+LocalChain->push_back(FR);
+  if (isNonUnionUninit(FR, LocalChain))

xazax.hun wrote:
> Don't you need to pop somewhere?
Nice catch! This isn't covered by tests, and would most probably cause an 
incorrect note message. I'll be sure to fix it.


https://reviews.llvm.org/D45532



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:31
+/// every other element is a field, and the element that precedes it is the
+/// object that contains it.
+class FieldChainInfo {

As far as I understand, for every operation, the only relevant contribution of 
the non-last elements in the chain is the diagnostic message.
I wonder if you would build a string instead of a FieldRegion chain and only 
store the last FieldRegion would make this more explicit in the code. 



Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:187
+
+  if (isCalledByConstructor(Context))
+return;

Szelethus wrote:
> whisperity wrote:
> > I think somewhere there should be a bit of reasoning why exactly these 
> > cases are ignored by the checker.
> At this function's declaration I have some comments describing what this does 
> and why it does it. Did you find it insufficient?
I am a bit surprised why these errors are not deduplicated by the analyzer. 
Maybe it would worth some investigation?



Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:272
+// If Chain's value is None, that means that this function was called to
+// determine whether a union has any initialized fields. In this case we're not
+// collecting fields, we'd only like to know  whether the value contained in

I find the documentation and the name of the function below misleading. 
`isNonUnionUninit` tells me this function is not supposed to be called for 
unions but the documentation suggests otherwise.



Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:283
+  bool ContainsUninitField = false;
+  Optional LocalChain = Chain;
+

Why do you copy here explicitly instead of taking the `Chain` argument by value?



Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:292
+  if (LocalChain)
+LocalChain->push_back(FR);
+  if (isNonUnionUninit(FR, LocalChain))

Don't you need to pop somewhere?


https://reviews.llvm.org/D45532



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus added inline comments.



Comment at: test/Analysis/ctor-uninitialized-member-inheritance.cpp:24
+  : NonPolymorphicLeft1(int{}) {
+y = 420;
+z = 420;

Szelethus wrote:
> whisperity wrote:
> > The literal `420` is repeated //everywhere// in this file. I think this 
> > (the same value appearing over and over again) will make debugging bad if 
> > something goes haywire and one has to look at memory dumps, 
> > control-flow-graphs, etc.
> Would you say that I should rather use a different number for each test case?
I mean, a different number for each variable within a test case.


https://reviews.llvm.org/D45532



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus added a comment.

Thank you for all your comments so far! I'll probably only be able to update 
the diff tomorrow (with me being in the GMT + 1 timezone).

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

I know, and I tried to look for ways to split this checker into smaller logical 
chunks, but I didn't find a satisfying solution. I'll be sure to review the 
comments I have in the code so it's as easy as possible to understand what I 
did and why I did it. Also, I'd be delighted to help any way I can to guide you 
through the code! :)

> 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. [...] 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?

I didn't implement anything explicitly to suppress warnings, so if there is 
nothing in the CSA by default for it, I'm afraid this isn't possible (just yet).

My checker is mainly a tool to enforce the rule that every field should be 
initialized in a C++ object. While I see that this approach could result  
reduced performance, I think it's fine to say that those users who find this 
important (by 'this' I mean not initializing every field) should not enable 
this checker.

Nevertheless, I'd be happy to know what you think of this.

> Did you find any such intentionally uninitialized fields with your checker? 
> Were there many of those?

I ran the checker on some projects, but it's little difficult to analyze larger 
ones as this checker emits very important information in notes, and those are 
not yet part of the plist output. But it's coming: 
https://reviews.llvm.org/D45407! I'll be sure to answer these questions as soon 
as I can.




Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:187
+
+  if (isCalledByConstructor(Context))
+return;

whisperity wrote:
> I think somewhere there should be a bit of reasoning why exactly these cases 
> are ignored by the checker.
At this function's declaration I have some comments describing what this does 
and why it does it. Did you find it insufficient?



Comment at: test/Analysis/ctor-uninitialized-member-inheritance.cpp:24
+  : NonPolymorphicLeft1(int{}) {
+y = 420;
+z = 420;

whisperity wrote:
> The literal `420` is repeated //everywhere// in this file. I think this (the 
> same value appearing over and over again) will make debugging bad if 
> something goes haywire and one has to look at memory dumps, 
> control-flow-graphs, etc.
Would you say that I should rather use a different number for each test case?


https://reviews.llvm.org/D45532



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Whisperity via Phabricator via cfe-commits]

whisperity requested changes to this revision.
whisperity added subscribers: gsd, dkrupp, o.gyorgy.
whisperity added a comment.
This revision now requires changes to proceed.

Sorry, one comment has gone missing meanwhile, I'm still getting used to this 
interface and hit //Submit// early.




Comment at: test/Analysis/ctor-uninitialized-member-inheritance.cpp:24
+  : NonPolymorphicLeft1(int{}) {
+y = 420;
+z = 420;

The literal `420` is repeated //everywhere// in this file. I think this (the 
same value appearing over and over again) will make debugging bad if something 
goes haywire and one has to look at memory dumps, control-flow-graphs, etc.


https://reviews.llvm.org/D45532



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

@NoQ Do you reckon these tests files are too long? Perhaps the one about this 
inheritance, that inheritance, diamond inheritance, etc. could be split into 
multiple files.




Comment at: include/clang/StaticAnalyzer/Checkers/Checkers.td:317
 
+def CtorUninitializedMemberChecker: Checker<"CtorUninitializedMember">,
+ HelpText<"Reports uninitialized members in constructors">,

This always pokes me in the eye, but shouldn't this file be sorted 
alphabetically?



Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:103
+  // - pointer/reference
+  // - fundamental object (int, double, etc.)
+  //   * the parent of each node is the object that contains it

I believe `fundamental object` should be rephrased as `of fundamental type` (as 
in: object that is of fundamental type), as the standard talks about 
//fundamental types//.



Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:187
+
+  if (isCalledByConstructor(Context))
+return;

I think somewhere there should be a bit of reasoning why exactly these cases 
are ignored by the checker.



Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:218
+
+  std::string WarningMsg = std::to_string(UninitFields.size()) +
+   " uninitialized field" +

Maybe one could use a Twine or a string builder to avoid all these unneccessary 
string instantiations? Or `std::string::append()`?



Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:316
+
+// At this point the field is a fundamental type.
+if (isFundamentalUninit(V)) {

(See, you use //fundamental type// here.)



Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:340
+
+// TODO As of writing this checker, there is very little support for unions in
+// the CSA. This function relies on a nonexisting implementation by assuming as

Please put `:` after `TODO`.



Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:342
+// the CSA. This function relies on a nonexisting implementation by assuming as
+// little about it as possible.
+bool FindUninitializedFields::isUnionUninit(const TypedValueRegion *R) {

What does `it` refer to in this sentence?



Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:421
+if (T->isUnionType()) {
+  // TODO does control ever reach here?
+  if (isUnionUninit(RT)) {

Please insert a `:` after `TODO` here too.



Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:475
+  if (Chain.back()->getDecl()->getType()->isPointerType())
+return OS.str().drop_back(2);
+  return OS.str().drop_back(1);

Maybe this could be made more explicit (as opposed to a comment) by using 
[[http://llvm.org/doxygen/classllvm_1_1StringRef.html#a3f45a78650a72626cdf9210de0c6d701|`StringRef::rtrim(StringRef)`]],
 like this:

return OS.str().rtrim('.').rtrim("->");

How does this code behave if the `Chain` is empty and thus `OS` contains no 
string whatsoever? `drop_back` asserts if you want to drop more elements than 
the length of the string.



Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:498
+Context.getStackFrame());
+  // getting 'this'
+  SVal This = Context.getState()->getSVal(ThisLoc);

Comment isn't formatted as full sentence.



Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:501
+
+  // getting '*this'
+  SVal Object = Context.getState()->getSVal(This.castAs());

Neither here.



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
+

NoQ wrote:
> I managed to find the thread, but this link doesn't work for me.
Confirmed, this link is a 404.


https://reviews.llvm.org/D45532



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In https://reviews.llvm.org/D45532#1064685, @Szelethus wrote:

> In https://reviews.llvm.org/D45532#1064670, @Szelethus wrote:
>
> > I wasn't too clear on this one: unknown fields are regarded as 
> > uninitialized, what I wrote was a temporary change in the code so I could 
> > check whether it would work.
>
>
> Woops, I meant that unknown fields are regarded az initialized.


Aha, yep, that makes a lot more sense =)


https://reviews.llvm.org/D45532



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus added a comment.

In https://reviews.llvm.org/D45532#1064670, @Szelethus wrote:

> I wasn't too clear on this one: unknown fields are regarded as uninitialized, 
> what I wrote was a temporary change in the code so I could check whether it 
> would work.


Woops, I meant that unknown fields are regarded az initialized.


https://reviews.llvm.org/D45532



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Ok, thanks, looking forward to it!




Comment at: test/Analysis/ctor-uninitialized-member.cpp:869
+void f44() {
+  ContainsUnionWithSimpleUnionTest2(); // xpected-warning{{1 uninitialized 
field}}
+}

Szelethus wrote:
> NoQ wrote:
> > Hmm, shouldn't it say "expected"? Do these tests actually work?
> Since unions are not yet supported by the CSA, this is only what would be 
> expected from this checker to find in the future. I purposely wrote 'xpected' 
> so tests wouldn't break.
Aha, okay, got it. The tradition is to say "// no-warning" and then put a FIXME 
nearby saying that it should warn, how it should warn, probably why it should 
warn, if it's non-obvious.


https://reviews.llvm.org/D45532



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus added a comment.

In https://reviews.llvm.org/D45532#1064652, @NoQ wrote:

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


I wasn't too clear on this one: unknown fields are regarded as uninitialized, 
what I wrote was a temporary change in the code so I could check whether it 
would work.

I agree with you regarding unions, and will remove the code that relies on non 
existing implementation.


https://reviews.llvm.org/D45532



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus added inline comments.



Comment at: test/Analysis/ctor-uninitialized-member.cpp:869
+void f44() {
+  ContainsUnionWithSimpleUnionTest2(); // xpected-warning{{1 uninitialized 
field}}
+}

NoQ wrote:
> Hmm, shouldn't it say "expected"? Do these tests actually work?
Since unions are not yet supported by the CSA, this is only what would be 
expected from this checker to find in the future. I purposely wrote 'xpected' 
so tests wouldn't break.


https://reviews.llvm.org/D45532



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus updated this revision to Diff 142053.
Szelethus added a comment.

Reuploaded the diff with full context.


https://reviews.llvm.org/D45532

Files:
  include/clang/StaticAnalyzer/Checkers/Checkers.td
  lib/StaticAnalyzer/Checkers/CMakeLists.txt
  lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp
  test/Analysis/ctor-uninitialized-member-inheritance.cpp
  test/Analysis/ctor-uninitialized-member.cpp

Index: test/Analysis/ctor-uninitialized-member.cpp
===
--- /dev/null
+++ test/Analysis/ctor-uninitialized-member.cpp
@@ -0,0 +1,870 @@
+//RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.cplusplus.CtorUninitializedMember -std=c++11 -verify %s
+
+//===--===//
+// Default constructor test.
+//===--===//
+
+class DefaultConstructorTest {
+  int a, b, c, d, e, f, g, h, i, j;
+
+public:
+  DefaultConstructorTest() = default;
+};
+
+void f00() {
+  DefaultConstructorTest();
+}
+
+//===--===//
+// Initializer list test.
+//===--===//
+
+class InitListTest1 {
+  int a;
+  int b;
+
+public:
+  InitListTest1()
+  : a(420),
+b(420) {
+// All good!
+  }
+};
+
+void f01() {
+  InitListTest1();
+}
+
+class InitListTest2 {
+  int a;
+  int b; // expected-note{{uninitialized field 'this->b'}}
+
+public:
+  InitListTest2()
+  : a(420) {} // expected-warning{{1 uninitialized field}}
+};
+
+void f02() {
+  InitListTest2();
+}
+
+class InitListTest3 {
+  int a; // expected-note{{uninitialized field 'this->a'}}
+  int b;
+
+public:
+  InitListTest3()
+  : b(420) {} // expected-warning{{1 uninitialized field}}
+};
+
+void f03() {
+  InitListTest3();
+}
+
+//===--===//
+// Constructor body test.
+//===--===//
+
+class CtorBodyTest1 {
+  int a, b;
+
+public:
+  CtorBodyTest1() {
+a = 420;
+b = 420;
+// All good!
+  }
+};
+
+void f04() {
+  CtorBodyTest1();
+}
+
+class CtorBodyTest2 {
+  int a;
+  int b; // expected-note{{uninitialized field 'this->b'}}
+
+public:
+  CtorBodyTest2() {
+a = 420; // expected-warning{{1 uninitialized field}}
+  }
+};
+
+void f05() {
+  CtorBodyTest2();
+}
+
+class CtorBodyTest3 {
+  int a; // expected-note{{uninitialized field 'this->a'}}
+  int b;
+
+public:
+  CtorBodyTest3() {
+b = 420; // expected-warning{{1 uninitialized field}}
+  }
+};
+
+void f06() {
+  CtorBodyTest3();
+}
+
+class CtorBodyTest4 {
+  int a; // expected-note{{uninitialized field 'this->a'}}
+  int b; // expected-note{{uninitialized field 'this->b'}}
+
+public:
+  CtorBodyTest4() {}
+};
+
+void f07() {
+  CtorBodyTest4(); // expected-warning{{2 uninitialized fields}}
+}
+
+//===--===//
+// Constructor delegation test.
+//===--===//
+
+class CtorDelegationTest1 {
+  int a;
+  int b;
+
+public:
+  CtorDelegationTest1(int)
+  : a(420) {
+// leaves 'b' unintialized, but we'll never check this function
+  }
+
+  CtorDelegationTest1()
+  : CtorDelegationTest1(int{}) { // Initializing 'a'
+b = 420;
+// All good!
+  }
+};
+
+void f08() {
+  CtorDelegationTest1();
+}
+
+class CtorDelegationTest2 {
+  int a; // expected-note{{uninitialized field 'this->a'}}
+  int b;
+
+public:
+  CtorDelegationTest2(int)
+  : b(420) {
+// leaves 'a' unintialized, but we'll never check this function
+  }
+
+  CtorDelegationTest2()
+  : CtorDelegationTest2(int{}) { // expected-warning{{1 uninitialized field}}
+  }
+};
+
+void f09() {
+  CtorDelegationTest2();
+}
+
+//===--===//
+// Tests for classes containing records.
+//===--===//
+
+class ContainsRecordTest1 {
+  struct RecordType {
+int x;
+int y;
+  } rec;
+  int c, d;
+
+public:
+  ContainsRecordTest1()
+  : rec({420, 420}),
+c(420),
+d(420) {
+// All good!
+  }
+};
+
+void f10() {
+  ContainsRecordTest1();
+}
+
+class ContainsRecordTest2 {
+  struct RecordType {
+int x;
+int y; // expected-note{{uninitialized field 'this->rec.y'}}
+  } rec;
+  int c, d;
+
+public:
+  ContainsRecordTest2()
+  : c(420),
+d(420) {
+rec.x = 420; // expected-warning{{1 uninitialized field}}
+  }
+};
+
+void f11() {
+  ContainsRecordTest2();
+}
+
+class ContainsRecordTest3 {
+  struct RecordType {
+int x; // expected-note{{uninitialized field 'this->rec.x'}}
+int y; // expected-note{{uninitialized field 'this->rec.y'}}
+  } rec;
+  

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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Please always upload all patches with full context (`-U9`)


Repository:
  rC Clang

https://reviews.llvm.org/D45532



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus created this revision.
Szelethus added reviewers: NoQ, xazax.hun, dkrupp, whisperity.
Herald added subscribers: cfe-commits, a.sidorin, rnkovacs, szepet, mgorny.
Herald added a reviewer: george.karpenkov.

This checker checks at the end of a constructor call whether all fields of the 
object were initialized.

Note that a significant portion of the code is for handling unions, for which 
there is very little support in the CSA. In most cases, all fields of a union 
is regarded as unknown. I checked these cases by regarding unknown fields as 
uninitialized.

The test files also contain checks for heap allocated objects, and heap regions 
are mostly conjured in the CSA, so the tests for them and unions should work 
theoretically, but its not 100%.


Repository:
  rC Clang

https://reviews.llvm.org/D45532

Files:
  include/clang/StaticAnalyzer/Checkers/Checkers.td
  lib/StaticAnalyzer/Checkers/CMakeLists.txt
  lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp
  test/Analysis/ctor-uninitialized-member-inheritance.cpp
  test/Analysis/ctor-uninitialized-member.cpp

Index: test/Analysis/ctor-uninitialized-member.cpp
===
--- /dev/null
+++ test/Analysis/ctor-uninitialized-member.cpp
@@ -0,0 +1,870 @@
+//RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.cplusplus.CtorUninitializedMember -std=c++11 -verify %s
+
+//===--===//
+// Default constructor test.
+//===--===//
+
+class DefaultConstructorTest {
+  int a, b, c, d, e, f, g, h, i, j;
+
+public:
+  DefaultConstructorTest() = default;
+};
+
+void f00() {
+  DefaultConstructorTest();
+}
+
+//===--===//
+// Initializer list test.
+//===--===//
+
+class InitListTest1 {
+  int a;
+  int b;
+
+public:
+  InitListTest1()
+  : a(420),
+b(420) {
+// All good!
+  }
+};
+
+void f01() {
+  InitListTest1();
+}
+
+class InitListTest2 {
+  int a;
+  int b; // expected-note{{uninitialized field 'this->b'}}
+
+public:
+  InitListTest2()
+  : a(420) {} // expected-warning{{1 uninitialized field}}
+};
+
+void f02() {
+  InitListTest2();
+}
+
+class InitListTest3 {
+  int a; // expected-note{{uninitialized field 'this->a'}}
+  int b;
+
+public:
+  InitListTest3()
+  : b(420) {} // expected-warning{{1 uninitialized field}}
+};
+
+void f03() {
+  InitListTest3();
+}
+
+//===--===//
+// Constructor body test.
+//===--===//
+
+class CtorBodyTest1 {
+  int a, b;
+
+public:
+  CtorBodyTest1() {
+a = 420;
+b = 420;
+// All good!
+  }
+};
+
+void f04() {
+  CtorBodyTest1();
+}
+
+class CtorBodyTest2 {
+  int a;
+  int b; // expected-note{{uninitialized field 'this->b'}}
+
+public:
+  CtorBodyTest2() {
+a = 420; // expected-warning{{1 uninitialized field}}
+  }
+};
+
+void f05() {
+  CtorBodyTest2();
+}
+
+class CtorBodyTest3 {
+  int a; // expected-note{{uninitialized field 'this->a'}}
+  int b;
+
+public:
+  CtorBodyTest3() {
+b = 420; // expected-warning{{1 uninitialized field}}
+  }
+};
+
+void f06() {
+  CtorBodyTest3();
+}
+
+class CtorBodyTest4 {
+  int a; // expected-note{{uninitialized field 'this->a'}}
+  int b; // expected-note{{uninitialized field 'this->b'}}
+
+public:
+  CtorBodyTest4() {}
+};
+
+void f07() {
+  CtorBodyTest4(); // expected-warning{{2 uninitialized fields}}
+}
+
+//===--===//
+// Constructor delegation test.
+//===--===//
+
+class CtorDelegationTest1 {
+  int a;
+  int b;
+
+public:
+  CtorDelegationTest1(int)
+  : a(420) {
+// leaves 'b' unintialized, but we'll never check this function
+  }
+
+  CtorDelegationTest1()
+  : CtorDelegationTest1(int{}) { // Initializing 'a'
+b = 420;
+// All good!
+  }
+};
+
+void f08() {
+  CtorDelegationTest1();
+}
+
+class CtorDelegationTest2 {
+  int a; // expected-note{{uninitialized field 'this->a'}}
+  int b;
+
+public:
+  CtorDelegationTest2(int)
+  : b(420) {
+// leaves 'a' unintialized, but we'll never check this function
+  }
+
+  CtorDelegationTest2()
+  : CtorDelegationTest2(int{}) { // expected-warning{{1 uninitialized field}}
+  }
+};
+
+void f09() {
+  CtorDelegationTest2();
+}
+
+//===--===//
+// Tests for classes containing records.
+//===--===//
+
+class ContainsRecordTest1 {
+  struct RecordType {
+int x;
+int y;
+  } rec;
+  int c, d;
+
+public:
+  ContainsRecordTest1()
+