[analyzer][UninitializedObjectChecker] New flag to turn off dereferencing

[Szelethus]

Summary

The idea came from both @george.karpenkov (D45532#1145592) and from bugzilla (https://bugs.llvm.org/show_bug.cgi?id=37965).

Assigning an object with uninitialized value to a pointer/reference could be intentional, especially if one writes a class that just initializes values.

struct Initializer {
  int *a;

  void initialize(/* ... */) {
    a = /* ... */;
  }
  
  Initializer(int *a) : a(a) {}
};

void f() {
  int b;
  Initializer init(&b);
  // ...
  init.initialize(/* ... */);
}

I actually have seen some examples for this in LLVM. While I absolutely agree that a flag like this would be neat, I also think that it should be disabled by default, as for example some objects are only created with malloc/new, and because I didn't find this functionality to be too noisy.

[george.karpenkov]

@Szelethus false positives are a single biggest problem of the analyzer.
By a *huge* margin, most projects would prefer to err on the side of less, more precise, warnings.
Given that currently in my understanding no actual bugs we are sure about were found by the uninitialized object checker,
I think by default we should err on the "less warnings" side.

This solves part of the problem, could we also separate the code somehow?
Let's say I would need to debug UnitializedObjectChecker without understanding the details of how the pointer chasing is done --- it's hard to do that when all the code is intermixed in one file.

[Szelethus]

I left a comment on this issue already: D49437#1165462. But in short, definitely yes! It's been very painful to work around pointer/reference objects, too bad I didn't come up with this idea sooner :)

[george.karpenkov]

lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp

723

registerChecker now passes through all arguments to the constructor.
Could you pass the analyzer options to the constructor,
and populate the fields from there?

728

Could we remove the negation in the option name? CheckPointeeInitialization? Also the value should be false by default if we want the checker to be evaluated on actual projects.

[george.karpenkov]

@Szelethus Hi, do you plan to finish this patch soon-ish? I would like to evaluate it on a few codebases, but the pointer chasing is currently way too fragile / generates too many FPs.

[Szelethus]

Dereferencing is disabled by default. I think there's a great value in this part of the checker, but I do concede to the fact that it still needs to mature even for alpha. I'll probably revisit this once heap regions are better supported.

[Szelethus]

lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp

723

From an earlier conversation (D48285#inline-423489):

[...]getBooleanOption(StringRef, /*DefaultVal*/ bool, const ento::CheckerBase *) depends on the constructed checker, so I don't think I can pull this off.

There actually is a way to get boolean options without having to pass the checker as an argument, but then it wouldn't be a checker specific option:
-analyzer-config Pedantic=true
instead of
-analyzer-config alpha.cplusplus.UninitializedObject:Pedantic=true

I did see your patch D49050, but since CHECKER::name is assigned a value after the checker's construction, I don't think it solves this particular problem.

[Szelethus]

In D49438#1184688, @george.karpenkov wrote:

@Szelethus Hi, do you plan to finish this patch soon-ish? I would like to evaluate it on a few codebases, but the pointer chasing is currently way too fragile / generates too many FPs.

What do you mean under fragile? It certainly needs some refactoring and clarification, but I haven't seen the current version of the checker crash due to pointer chasing.

This might be a little nit-picking, but I don't think the finds are false positive. From what I've seen after looking through hundreds of reports, I am confident that pointer chasing is working as intended. I would however agree that some reports hold little value to the developer, as in some cases pointees are intentionally left uninitialized, so it's better to hide it behind a flag for now.

[Szelethus]

Forgot git add and -U9999.

[george.karpenkov]

Dereferencing is disabled by default

Great! Thank you!

but I haven't seen the current version of the checker crash due to pointer chasing.

@NoQ saw quite a few crashes during evaluation, right?

This might be a little nit-picking, but I don't think the finds are false positive

Well, yeah, it depends on how do you define things. If we define reports as "bugs", then I guess this checker already does not find bugs, but best-practices violations.
Then it can be argued whether "best practice" should be extended to include all the memory reachable through pointers.

[george.karpenkov]

Great! The comment should be updated though.

lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp

46

The comment is outdated and "true" should be switched to "false", right?

[Szelethus]

It looks like I won't be back in office until Monday, so I can't finish this one until then :(

but I haven't seen the current version of the checker crash due to pointer chasing.

@NoQ saw quite a few crashes during evaluation, right?

That doesn't sound any good. I'd be very interested in fixing those. Any plans to make some bugreports on bugzilla?

This might be a little nit-picking, but I don't think the finds are false positive

Well, yeah, it depends on how do you define things. If we define reports as "bugs", then I guess this checker already does not find bugs, but best-practices violations.
Then it can be argued whether "best practice" should be extended to include all the memory reachable through pointers.

Fair point. I still am very confident that pointer chasing should be a thing, but I strongly agree that it lacks good heuristics to make the reports more meaningful.

[Szelethus]

Fixed the comments.

[george.karpenkov]

Great, thanks a lot!

[Szelethus]

I think what pointer chasing should do, is check whether that pointer owns the pointee. In that case, it should be fine to analyze it. Do you mind if I put a TODO around flag's description stating that this should be implemented and pointer chasing should be enabled by default once that's done?

[george.karpenkov]

I think what pointer chasing should do, is check whether that pointer owns the pointee

But ownership is a convention, and it's not always deducible from a codebase.
I think of those as two separate checks, and I think we should only talk about enabling the pointer-chasing after we had established that just checking for uninitialized fields finds lots of valid bugs (and we can only do that after it gets enabled for many projects)

[Szelethus]

In D49438#1189772, @george.karpenkov wrote:

I think what pointer chasing should do, is check whether that pointer owns the pointee

But ownership is a convention, and it's not always deducible from a codebase.

How about the following case:

struct A {
  struct B {
    int b;
  };
  std::unique_ptr<B> ptr;
  A() : ptr(new B) {}
};

A a;

Here, a->ptr->b is clearly uninitialized, and I think it's fine to assume in most cases that no other pointer points to it right after a's construction.

I think of those as two separate checks, and I think we should only talk about enabling the pointer-chasing after we had established that just checking for uninitialized fields finds lots of valid bugs (and we can only do that after it gets enabled for many projects)

I think in the earlier case *this->ptr should be regarded as a regular field, and it could be analyzed without fear of spamming too many reports. Currently the biggest problem is that many objects could contain references to the same object:

struct A { int x; };
struct B {
  A &a;
  B(A &a) : a(a) {}
};
struct C {
  A &a;
  C(A &a) : a(a) {}
};

A a;
B b(a);
C c(a); // a.x reported twice

[Szelethus]

If we ignore references, check whether the pointee was constructed within the constructor, and maybe add some other clever heuristics, I'm very much in favor of enabling pointer chasing by default. But I totally support disabling it for now.

[george.karpenkov]

@Szelethus in any case, could you commit this change for now? I'm very keen on seeing the results on a few of our internal projects, and it's much easier to do that once the change is committed.

[Szelethus]

I'll be back in office tomorrow, about ~13 hours later. But feel free to commit it on my behalf if it's urgent.

[Szelethus]

Added the TODO we were discussing.

lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp

 //===----- 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 two options:
+// This checker has several options:
 //   - "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`.
+//     `-analyzer-config alpha.cplusplus.UninitializedObject:Pedantic=true`.
 //
 //   - "NotesAsWarnings" (boolean). If set to true, the checker will emit a
 //     warning for each uninitalized field, as opposed to emitting one warning
 //     per constructor call, and listing the uninitialized fields that belongs
 //     to it in notes. Defaults to false.
 //
-//  `-analyzer-config alpha.cplusplus.UninitializedObject:NotesAsWarnings=true`.
+//     `-analyzer-config \
+//         alpha.cplusplus.UninitializedObject:NotesAsWarnings=true`.
+//
+//   - "CheckPointeeInitialization" (boolean). If set to false, the checker will
+//     not analyze the pointee of pointer/reference fields, and will only check
+//     whether the object itself is initialized. Defaults to false.
+//
+//     `-analyzer-config \
+//         alpha.cplusplus.UninitializedObject:CheckPointeeInitialization=true`.
+//
+//     TODO: With some clever heuristics, some pointers should be dereferenced
+//     by default. For example, if the pointee is constructed within the
+//     constructor call, it's reasonable to say that no external object
+//     references it, and we wouldn't generate multiple report on the same
+//     pointee.
 //
 //===----------------------------------------------------------------------===//
 
 #include "ClangSACheckers.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 #include <algorithm>
 
 using namespace clang;
 using namespace clang::ento;
 
 namespace {
 
 class UninitializedObjectChecker : public Checker<check::EndFunction> {
   std::unique_ptr<BuiltinBug> BT_uninitField;
 
 public:
   // These fields will be initialized when registering the checker.
   bool IsPedantic;
   bool ShouldConvertNotesToWarnings;
+  bool CheckPointeeInitialization;
 
   UninitializedObjectChecker()
       : BT_uninitField(new BuiltinBug(this, "Uninitialized fields")) {}
   void checkEndFunction(CheckerContext &C) const;
 };
 
 /// 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

 using UninitFieldSet = std::set<FieldChainInfo, FieldChainInfoComparator>;
 
 /// Searches for and stores uninitialized fields in a non-union object.
 class FindUninitializedFields {
   ProgramStateRef State;
   const TypedValueRegion *const ObjectR;
 
   const bool IsPedantic;
+  const bool CheckPointeeInitialization;
+
   bool IsAnyFieldInitialized = false;
 
   UninitFieldSet UninitFields;
 
 public:
   FindUninitializedFields(ProgramStateRef State,
-                          const TypedValueRegion *const R, bool IsPedantic);
+                          const TypedValueRegion *const R, bool IsPedantic,
+                          bool CheckPointeeInitialization);
   const UninitFieldSet &getUninitFields();
 
 private:
   /// Adds a FieldChainInfo object to UninitFields. Return true if an insertion
   /// took place.
   bool addFieldToUninits(FieldChainInfo LocalChain);
 
   // For the purposes of this checker, we'll regard the object under checking as
void UninitializedObjectChecker::checkEndFunction(
   // This avoids essentially the same error being reported multiple times.
   if (isCalledByConstructor(Context))
     return;
 
   Optional<nonloc::LazyCompoundVal> Object = getObjectVal(CtorDecl, Context);
   if (!Object)
     return;
 
-  FindUninitializedFields F(Context.getState(), Object->getRegion(),
-                            IsPedantic);
+  FindUninitializedFields F(Context.getState(), Object->getRegion(), IsPedantic,
+                            CheckPointeeInitialization);
 
   const UninitFieldSet &UninitFields = F.getUninitFields();
 
   if (UninitFields.empty())
     return;
 
   // There are uninitialized fields in the record.
 
void UninitializedObjectChecker::checkEndFunction(
   Context.emitReport(std::move(Report));
 }
 
 //===----------------------------------------------------------------------===//
 //                   Methods for FindUninitializedFields.
 //===----------------------------------------------------------------------===//
 
 FindUninitializedFields::FindUninitializedFields(
-    ProgramStateRef State, const TypedValueRegion *const R, bool IsPedantic)
-    : State(State), ObjectR(R), IsPedantic(IsPedantic) {}
+    ProgramStateRef State, const TypedValueRegion *const R, bool IsPedantic,
+    bool CheckPointeeInitialization)
+    : State(State), ObjectR(R), IsPedantic(IsPedantic),
+      CheckPointeeInitialization(CheckPointeeInitialization) {}
 
 const UninitFieldSet &FindUninitializedFields::getUninitFields() {
   isNonUnionUninit(ObjectR, FieldChainInfo());
 
   if (!IsPedantic && !IsAnyFieldInitialized)
     UninitFields.clear();
 
   return UninitFields;
if (V.isUnknown() || V.isZeroConstant()) {
     IsAnyFieldInitialized = true;
     return false;
   }
 
   if (V.isUndef()) {
     return addFieldToUninits({LocalChain, FR});
   }
 
+  if (!CheckPointeeInitialization) {
+    IsAnyFieldInitialized = true;
+    return false;
+  }
+
   const FieldDecl *FD = FR->getDecl();
 
   // TODO: The dynamic type of a void pointer may be retrieved with
   // `getDynamicTypeInfo`.
   if (isVoidPointer(FD)) {
     IsAnyFieldInitialized = true;
     return false;
   }
else
       Out << "uninitialized pointer 'this->";
   } else
     Out << "uninitialized field 'this->";
   Chain.print(Out);
   Out << "'";
 }
 
 void ento::registerUninitializedObjectChecker(CheckerManager &Mgr) {
   auto Chk = Mgr.registerChecker<UninitializedObjectChecker>();
   Chk->IsPedantic = Mgr.getAnalyzerOptions().getBooleanOption(
       "Pedantic", /*DefaultVal*/ false, Chk);
   Chk->ShouldConvertNotesToWarnings = Mgr.getAnalyzerOptions().getBooleanOption(
       "NotesAsWarnings", /*DefaultVal*/ false, Chk);
+  Chk->CheckPointeeInitialization = Mgr.getAnalyzerOptions().getBooleanOption(
+      "CheckPointeeInitialization", /*DefaultVal*/ false, Chk);
 }

test/Analysis/cxx-uninitialized-object-inheritance.cpp

-// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.cplusplus.UninitializedObject -analyzer-config alpha.cplusplus.UninitializedObject:Pedantic=true -std=c++11 -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.cplusplus.UninitializedObject \
+// RUN: -analyzer-config alpha.cplusplus.UninitializedObject:Pedantic=true -DPEDANTIC \
+// RUN: -analyzer-config alpha.cplusplus.UninitializedObject:CheckPointeeInitialization=true \
+// RUN: -std=c++11 -verify  %s
 
 //===----------------------------------------------------------------------===//
 // Non-polymorphic inheritance tests
 //===----------------------------------------------------------------------===//
 
 class NonPolymorphicLeft1 {
   int x;
 

test/Analysis/cxx-uninitialized-object-no-dereference.cpp

+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.cplusplus.UninitializedObject \
+// RUN:   -std=c++11 -DPEDANTIC -verify %s
+
+class UninitPointerTest {
+  int *ptr; // expected-note{{uninitialized pointer 'this->ptr'}}
+  int dontGetFilteredByNonPedanticMode = 0;
+
+public:
+  UninitPointerTest() {} // expected-warning{{1 uninitialized field}}
+};
+
+void fUninitPointerTest() {
+  UninitPointerTest();
+}
+
+class UninitPointeeTest {
+  int *ptr; // no-note
+  int dontGetFilteredByNonPedanticMode = 0;
+
+public:
+  UninitPointeeTest(int *ptr) : ptr(ptr) {} // no-warning
+};
+
+void fUninitPointeeTest() {
+  int a;
+  UninitPointeeTest t(&a);
+}

test/Analysis/cxx-uninitialized-object-notes-as-warnings.cpp

-// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.cplusplus.UninitializedObject -analyzer-config alpha.cplusplus.UninitializedObject:NotesAsWarnings=true -std=c++11 -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.cplusplus.UninitializedObject \
+// RUN:   -analyzer-config alpha.cplusplus.UninitializedObject:NotesAsWarnings=true \
+// RUN:   -analyzer-config alpha.cplusplus.UninitializedObject:CheckPointeeInitialization=true \
+// RUN:   -std=c++11 -verify %s
 
 class NotesAsWarningsTest {
   int a;
   int b;
   int dontGetFilteredByNonPedanticMode = 0;
 
 public:
   NotesAsWarningsTest() {} // expected-warning{{uninitialized field 'this->a'}}
   // expected-warning@-1{{uninitialized field 'this->b'}}
 };
 
 void fNotesAsWarningsTest() {
   NotesAsWarningsTest();
 }

test/Analysis/cxx-uninitialized-object-ptr-ref.cpp

-// 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
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.cplusplus.UninitializedObject \
+// RUN:   -analyzer-config alpha.cplusplus.UninitializedObject:Pedantic=true -DPEDANTIC \
+// RUN:   -analyzer-config alpha.cplusplus.UninitializedObject:CheckPointeeInitialization=true \
+// RUN:   -std=c++11 -verify  %s
+
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.cplusplus.UninitializedObject \
+// RUN:   -analyzer-config alpha.cplusplus.UninitializedObject:CheckPointeeInitialization=true \
+// RUN:   -std=c++11 -verify  %s
 
 //===----------------------------------------------------------------------===//
 // Concrete location tests.
 //===----------------------------------------------------------------------===//
 
 struct ConcreteIntLocTest {
   int *ptr;
 

test/Analysis/cxx-uninitialized-object.cpp

-// 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
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.cplusplus.UninitializedObject \
+// RUN:   -analyzer-config alpha.cplusplus.UninitializedObject:Pedantic=true -DPEDANTIC \
+// RUN:   -analyzer-config alpha.cplusplus.UninitializedObject:CheckPointeeInitialization=true \
+// RUN:   -std=c++11 -verify  %s
+
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.cplusplus.UninitializedObject \
+// RUN:   -analyzer-config alpha.cplusplus.UninitializedObject:CheckPointeeInitialization=true \
+// RUN:   -std=c++11 -verify  %s
 
 //===----------------------------------------------------------------------===//
 // Default constructor test.
 //===----------------------------------------------------------------------===//
 
 class CompilerGeneratedConstructorTest {
   int a, b, c, d, e, f, g, h, i, j;