[analyzer][UninitializedObjectChecker] Added "NotesAsWarnings" flag

[Szelethus]

Summary

For Plist consumers that don't support notes just yet, this flag can be used to convert them into warnings. For now, I defaulted it to false.

[george.karpenkov]

LGTM provided the nit on passing arguments is fixed.

lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp

698

registerChecker passes through arguments to the constructor. Could we use that instead of mutable fields?

[xazax.hun]

I wonder if this could be done when plist is emitted generally, independent of any checks.

[NoQ]

lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp

698

Really? Yay. I never noticed that. I think currently all checkers that need options use mutable fields for that purpose, and everybody thought it was intended to work this way.

I'm not super worried about fields being mutable because all checker callbacks are const (so you're still protected from mutating them in all of your code), but that'd definitely make the code look better.

[NoQ]

Also, great, and can i has tests?^^

Like a simple code snippet with two // RUN: ... -analyzer-output=text lines and different expected-warnings/notes under #ifs.

[Szelethus]

In D48285#1136059, @NoQ wrote:

Also, great, and can i has tests?^^

Like a simple code snippet with two // RUN: ... -analyzer-output=text lines and different expected-warnings/notes under #ifs.

I intended to, but forgot git add. Whoops. I didn't find a satisfying way to add it to the existing tests without surrounding the entire file with #if, so I added a new test file instead.

[Szelethus]

In D48285#1135480, @xazax.hun wrote:

I wonder if this could be done when plist is emitted generally, independent of any checks.

Well, there's -analyzer-config notes-as-events=true. By the time plist files are generated, every warning is just regarded as an event (as far as I know).

lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp

698

Well, I didn't really find that to be the case. Even if it was, 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.

[NoQ]

Looks great, thanks!

lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp

698

Yeah, well, i guess if you just use this as the last argument of getBooleanOption(), you'll still have CheckName unset during construction.

A slight refactoring might help, but it doesn't currently work "out of the box".

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 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`.
+// This checker has two 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`.
 //
+//   - "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`.
+//
 //===----------------------------------------------------------------------===//
 
 #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:
-  bool IsPedantic; // Will be initialized when registering the checker.
+  // These fields will be initialized when registering the checker.
+  bool IsPedantic;
+  bool ShouldConvertNotesToWarnings;
 
   UninitializedObjectChecker()
       : BT_uninitField(new BuiltinBug(this, "Uninitialized fields")) {}
   void checkEndFunction(CheckerContext &C) const;
 };
 
 llvm::ImmutableListFactory<const FieldRegion *> Factory;
 

 bool isVoidPointer(const FieldDecl *FD);
 
 /// Returns true if T is a primitive type. We'll call a type primitive if it's
 /// either a BuiltinType or an EnumeralType.
 bool isPrimitiveType(const QualType &T) {
   return T->isBuiltinType() || T->isEnumeralType();
 }
 
+/// Constructs a note message for a given FieldChainInfo object.
+void printNoteMessage(llvm::raw_ostream &Out, const FieldChainInfo &Chain);
+
 } // end of anonymous namespace
 
 //===----------------------------------------------------------------------===//
 //                  Methods for UninitializedObjectChecker.
 //===----------------------------------------------------------------------===//
 
 void UninitializedObjectChecker::checkEndFunction(
     CheckerContext &Context) const {
if (!Node)
     return;
 
   PathDiagnosticLocation LocUsedForUniqueing;
   const Stmt *CallSite = Context.getStackFrame()->getCallSite();
   if (CallSite)
     LocUsedForUniqueing = PathDiagnosticLocation::createBegin(
         CallSite, Context.getSourceManager(), Node->getLocationContext());
 
+  // For Plist consumers that don't support notes just yet, we'll convert notes
+  // to warnings.
+  if (ShouldConvertNotesToWarnings) {
+    for (const auto &Chain : UninitFields) {
+      SmallString<100> WarningBuf;
+      llvm::raw_svector_ostream WarningOS(WarningBuf);
+
+      printNoteMessage(WarningOS, Chain);
+
+      auto Report = llvm::make_unique<BugReport>(
+          *BT_uninitField, WarningOS.str(), Node, LocUsedForUniqueing,
+          Node->getLocationContext()->getDecl());
+      Context.emitReport(std::move(Report));
+    }
+    return;
+  }
+
   SmallString<100> WarningBuf;
   llvm::raw_svector_ostream WarningOS(WarningBuf);
   WarningOS << UninitFields.size() << " uninitialized field"
             << (UninitFields.size() == 1 ? "" : "s")
             << " at the end of the constructor call";
 
   auto Report = llvm::make_unique<BugReport>(
       *BT_uninitField, WarningOS.str(), Node, LocUsedForUniqueing,
       Node->getLocationContext()->getDecl());
 
-  // TODO: As of now, one warning is emitted per constructor call, and the
-  // uninitialized fields are listed in notes. Until there's a better support
-  // for notes avaible, a note-less version of this checker should be
-  // implemented.
-  for (const auto &FieldChain : UninitFields) {
+  for (const auto &Chain : UninitFields) {
     SmallString<200> NoteBuf;
     llvm::raw_svector_ostream NoteOS(NoteBuf);
 
-    if (FieldChain.isPointer()) {
-      if (FieldChain.isDereferenced())
-        NoteOS << "uninitialized pointee 'this->";
-      else
-        NoteOS << "uninitialized pointer 'this->";
-    } else
-      NoteOS << "uninitialized field 'this->";
-    FieldChain.print(NoteOS);
-    NoteOS << "'";
+    printNoteMessage(NoteOS, Chain);
 
     Report->addNote(NoteOS.str(),
-                    PathDiagnosticLocation::create(FieldChain.getEndOfChain(),
+                    PathDiagnosticLocation::create(Chain.getEndOfChain(),
                                                    Context.getSourceManager()));
   }
-
   Context.emitReport(std::move(Report));
 }
 
 //===----------------------------------------------------------------------===//
 //                   Methods for FindUninitializedFields.
 //===----------------------------------------------------------------------===//
 
 FindUninitializedFields::FindUninitializedFields(
while (LC) {
     if (isa<CXXConstructorDecl>(LC->getDecl()))
       return true;
 
     LC = LC->getParent();
   }
   return false;
 }
 
+void printNoteMessage(llvm::raw_ostream &Out, const FieldChainInfo &Chain) {
+  if (Chain.isPointer()) {
+    if (Chain.isDereferenced())
+      Out << "uninitialized pointee 'this->";
+    else
+      Out << "uninitialized pointer 'this->";
+  } else
+    Out << "uninitialized field 'this->";
+  Chain.print(Out);
+  Out << "'";
+}
+
 } // end of anonymous namespace
 
 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);
 }

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
+
+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();
+}