[analyzer][UninitializedObjectChecker] Mark uninitialized regions as interesting.

[baloghadamsoftware]

Looks much better than the original one, but why did the warning move to the correct place just because marking the region as interesting?

[Szelethus]

Ping, anything against this? :)

[NoQ]

I don't remember what exactly does markInteresting() do and these tests don't really convince me that it does anything at all. Halp? >.<

[Szelethus]

In D62899#1544630, @NoQ wrote:

I don't remember what exactly does markInteresting() do and these tests don't really convince me that it does anything at all. Halp? >.<

Damnit, forgot my actual test file in the office (did not git add it).

I wanted to just say "Well markInteresting() is about the same as trackExpressionValue()!", and while I suspect that there's some truth to that, honestly, it's explained extremely poorly in the code. As I understand it, interesting symbols and expressions are used in the pruning of the bug path, namely, if a you'd like to prune out B->C->D from bugpath A->B->C->D->E, but C is interesting, no pruning will take place.

I particularly liked your description of BugReporter.cpp as a "military grade portion of spaghetti", because thats pretty much all I could figure out. I'll try harder though, because it seems to be an important part of the bug report construction. @xazax.hun, you seem to be a lot more confident with this, is what I'm saying more or less correct?

[Szelethus]

Added a proper testfile. The only downside of it is that it doesn't test anything. Literally nothing would change if I didn't mark the fields interesting. I'll take this back to the drawing board.

[NoQ]

In D62899#1549715, @Szelethus wrote:

Added a proper testfile. The only downside of it is that it doesn't test anything.

Use creduce!

[Szelethus]

In D62899#1551312, @NoQ wrote:

In D62899#1549715, @Szelethus wrote:

Added a proper testfile. The only downside of it is that it doesn't test anything.

Use creduce!

I would, but I'm not even sure what to look for, really.

Okay, I have a far better understanding of interestingness, so here's the deal: Visitors, like ConditionBRVisitor, will mark its diagnostics non-prunable if it describes an interesting entity. The problem is, for an uninitialized variable, everything that is worth explaining is probably unexplored by the analyzer, leaving only to the note "Declaring 'x' without an initial value", which we don't emit for C++ constructors.

That said, I'd like ReturnVisitor to step up and place the notes it usually does. Even better, I'd like to use trackRegionValue() to add our beloved selection of visitors, so its a real shame it doesn't exist :^) Since we mark tracked values as interesting anyways, that would be a better solution then this.

[NoQ]

In D62899#1634657, @Szelethus wrote:

In D62899#1551312, @NoQ wrote:

In D62899#1549715, @Szelethus wrote:

Added a proper testfile. The only downside of it is that it doesn't test anything.

Use creduce!

I would, but I'm not even sure what to look for, really.

When you already have code and have a real-world example that it works but you don't have a reduced test case, just use "anything changes" as the reduce condition, because you're basically interested in the answer to "why does this patch change anything at all?".

clang/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp

void UninitializedObjectChecker::checkEndFunction(
   auto Report = llvm::make_unique<BugReport>(
       *BT_uninitField, WarningOS.str(), Node, LocUsedForUniqueing,
       Node->getLocationContext()->getDecl());
 
   for (const auto &Pair : UninitFields) {
     Report->addNote(Pair.second,
                     PathDiagnosticLocation::create(Pair.first->getDecl(),
                                                    Context.getSourceManager()));
+    Report->markInteresting(Pair.first);
   }
   Context.emitReport(std::move(Report));
 }
 
 void UninitializedObjectChecker::checkDeadSymbols(SymbolReaper &SR,
                                                   CheckerContext &C) const {
   ProgramStateRef State = C.getState();
   for (const MemRegion *R : State->get<AnalyzedRegions>()) {

clang/test/Analysis/cxx-uninitialized-object-tracking.cpp

+// RUN: %clang_analyze_cc1 -std=c++11 -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=optin.cplusplus.UninitializedObject \
+// RUN:   -analyzer-config \
+// RUN:     optin.cplusplus.UninitializedObject:Pedantic=true \
+// RUN:   -analyzer-output=text
+
+namespace must_have_happened {
+struct S {
+  int x; // expected-note{{uninitialized field 'this->x'}}
+  S(int r) {
+    if (r) // expected-note{{Assuming 'r' is 0}}
+           // expected-note@-1{{Taking false branch}}
+      x = 5;
+  } // expected-warning{{1 uninitialized field}}
+  // expected-note@-1{{1 uninitialized field}}
+};
+
+void f(int r) {
+  S s(r); // expected-note{{Calling constructor for 'S'}}
+}
+} // end of namespace must_have_happened
+
+namespace inlined_category {
+void init(int cond, int &val) {
+  if (cond)
+    val = 0;
+}
+
+struct S {
+  int x; // expected-note{{uninitialized field 'this->x'}}
+  S(int r) {
+    init(r, x);
+  } // expected-warning{{1 uninitialized field}}
+  // expected-note@-1{{1 uninitialized field}}
+};
+
+void f(int r) {
+  S s(r); // expected-note{{Calling constructor for 'S'}}
+}
+} // end of namespace inlined_category
+
+namespace not_inlined_category {
+void init(int cond, int &val) {
+  if (cond)
+    val = 0;
+}
+
+struct S {
+  int x; // expected-note{{uninitialized field 'this->x'}}
+  S(int r) {
+    if (r) // expected-note{{Assuming 'r' is 0}}
+           // expected-note@-1{{Taking false branch}}
+      init(r, x);
+  } // expected-warning{{1 uninitialized field}}
+  // expected-note@-1{{1 uninitialized field}}
+};
+
+void f(int r) {
+  S s(r); // expected-note{{Calling constructor for 'S'}}
+}
+} // end of namespace not_inlined_category

clang/test/Analysis/cxx-uninitialized-object-unguarded-access.cpp

 // RUN: %clang_analyze_cc1 -analyzer-checker=core,optin.cplusplus.UninitializedObject \
 // RUN:   -analyzer-config optin.cplusplus.UninitializedObject:Pedantic=true -DPEDANTIC \
 // RUN:   -analyzer-config optin.cplusplus.UninitializedObject:IgnoreGuardedFields=true \
+// RUN:   -analyzer-output=text \
 // RUN:   -std=c++11 -verify  %s
 
 //===----------------------------------------------------------------------===//
 // Helper functions for tests.
 //===----------------------------------------------------------------------===//
 
 [[noreturn]] void halt();
 
public:
   };
 
 private:
   int Volume, Area; // expected-note {{uninitialized field 'this->Volume'}}
   Kind K;
 
 public:
   UnguardedFieldThroughMethodTest(Kind K) : K(K) {
-    switch (K) {
+    switch (K) { // expected-note{{Control jumps to 'case A:'  at line}}
     case V:
       Volume = 0;
       break;
     case A:
-      Area = 0; // expected-warning {{1 uninitialized field}}
-      break;
-    }
+      Area = 0;
+      break; // expected-note{{Execution jumps to the end of the function}}
     }
+  } // expected-warning{{1 uninitialized field}}
+  // expected-note@-1{{1 uninitialized field}}
 
   void operator-() {
     assert(K == Kind::A);
     (void)Area;
   }
 
   void operator+() {
     (void)Volume;
   }
 };
 
 void fUnguardedFieldThroughMethodTest() {
   UnguardedFieldThroughMethodTest T1(UnguardedFieldThroughMethodTest::Kind::A);
+  // expected-note@-1{{Calling constructor for 'UnguardedFieldThroughMethodTest'}}
 }
 
 class UnguardedPublicFieldsTest {
 public:
   enum Kind {
     V,
     A
   };
 
 public:
   // Note that fields are public.
   int Volume, Area; // expected-note {{uninitialized field 'this->Volume'}}
   Kind K;
 
 public:
   UnguardedPublicFieldsTest(Kind K) : K(K) {
-    switch (K) {
+    switch (K) { // expected-note{{Control jumps to 'case A:'  at line}}
     case V:
       Volume = 0;
       break;
     case A:
-      Area = 0; // expected-warning {{1 uninitialized field}}
-      break;
-    }
+      Area = 0;
+      break; // expected-note{{Execution jumps to the end of the function}}
     }
+  } // expected-warning {{1 uninitialized field}}
+  // expected-note@-1{{1 uninitialized field}}
 
   void operator-() {
     assert(K == Kind::A);
     (void)Area;
   }
 
   void operator+() {
     assert(K == Kind::V);
     (void)Volume;
   }
 };
 
 void fUnguardedPublicFieldsTest() {
   UnguardedPublicFieldsTest T1(UnguardedPublicFieldsTest::Kind::A);
+  // expected-note@-1{{Calling constructor for 'UnguardedPublicFieldsTest'}}
 }
 
 //===----------------------------------------------------------------------===//
 // Highlights of some false negatives due to syntactic checking.
 //===----------------------------------------------------------------------===//
 
 class UnguardedFalseNegativeTest1 {
 public: