[analyzer][UninitializedObjectChecker] Updated comments

[Szelethus]

Summary

Some of the comments are incorrect, imprecise, or simply nonexistent. Since I have a better grasp on how the analyzer works, it makes sense to update most of them in a single swoop.

I tried not to flood the code with comments too much, this amount feels just right to me.

[Szelethus]

lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp

377–397

Fixed in rC339653, but apparently I forgot to remove this.

[Szelethus]

Polite ping ^-^

[NoQ]

Comments always welcome!

lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp

363–364

Ugh, why is it not const?

lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObject.h

 #ifndef LLVM_CLANG_STATICANALYZER_UNINITIALIZEDOBJECT_H
 #define LLVM_CLANG_STATICANALYZER_UNINITIALIZEDOBJECT_H
 
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 
 namespace clang {
 namespace ento {
 
-/// Represent a single field. This is only an interface to abstract away special
-/// cases like pointers/references.
+/// A lightweight polymorphic wrapper around FieldRegion *. We'll use this
+/// interface to store addinitional information about fields. As described
+/// later, a list of these objects (i.e. "fieldchain") will be constructed and
+/// used for printing note messages should an uninitialized value be found.
 class FieldNode {
 protected:
   const FieldRegion *FR;
 
+  /// FieldNodes are never meant to be created on the heap, see
+  /// FindUninitializedFields::addFieldToUninits().
   /* non-virtual */ ~FieldNode() = default;
 
 public:
   FieldNode(const FieldRegion *FR) : FR(FR) {}
 
+  // We'll delete all of these special member functions to force the users of
+  // this interface to only store references to FieldNode objects in containers.
   FieldNode() = delete;
   FieldNode(const FieldNode &) = delete;
   FieldNode(FieldNode &&) = delete;
   FieldNode &operator=(const FieldNode &) = delete;
   FieldNode &operator=(const FieldNode &&) = delete;
 
-  /// Profile - Used to profile the contents of this object for inclusion in a
-  /// FoldingSet.
   void Profile(llvm::FoldingSetNodeID &ID) const { ID.AddPointer(this); }
 
-  // Helper method for uniqueing.
+  /// Helper method for uniqueing.
   bool isSameRegion(const FieldRegion *OtherFR) const {
-    // Special FieldNode descendants may wrap nullpointers -- we wouldn't like
-    // to unique these objects.
+    // Special FieldNode descendants may wrap nullpointers (for example if they
+    // describe a special relationship between two elements of the fieldchain)
+    // -- we wouldn't like to unique these objects.
     if (FR == nullptr)
       return false;
 
     return FR == OtherFR;
   }
 
   const FieldRegion *getRegion() const { return FR; }
   const FieldDecl *getDecl() const {
     assert(FR);
     return FR->getDecl();
   }
 
-  // When a fieldchain is printed (a list of FieldNode objects), it will have
-  // the following format:
-  // <note message>'<prefix>this-><node><separator><node><separator>...<node>'
+  // When a fieldchain is printed, it will have the following format (without
+  // newline, indices are in order of insertion, from 1 to n):
+  //
+  // <note_message_n>'<prefix_n><prefix_n-1>...<prefix_1>
+  //       this-><node_1><separator_1><node_2><separator_2>...<node_n>'
 
-  /// If this is the last element of the fieldchain, this method will be called.
+  /// If this is the last element of the fieldchain, this method will print the
+  /// note message associated with it.
   /// The note message should state something like "uninitialized field" or
   /// "uninitialized pointee" etc.
   virtual void printNoteMsg(llvm::raw_ostream &Out) const = 0;
 
-  /// Print any prefixes before the fieldchain.
+  /// Print any prefixes before the fieldchain. Could contain casts, etc.
   virtual void printPrefix(llvm::raw_ostream &Out) const = 0;
 
-  /// Print the node. Should contain the name of the field stored in getRegion.
+  /// Print the node. Should contain the name of the field stored in FR.
   virtual void printNode(llvm::raw_ostream &Out) const = 0;
 
   /// Print the separator. For example, fields may be separated with '.' or
   /// "->".
   virtual void printSeparator(llvm::raw_ostream &Out) const = 0;
 
   virtual bool isBase() const { return false; }
 };
 
 /// Returns with Field's name. This is a helper function to get the correct name
 /// even if Field is a captured lambda variable.
 StringRef getVariableName(const FieldDecl *Field);
 
-/// 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.
+/// Represents a field chain. A field chain is a list 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 (essentially a wrapper around an
-/// ImmutableList), and new elements can only be added by creating new
-/// FieldChainInfo objects through add().
+/// ImmutableList), new FieldChainInfo objects may be created by member
+/// functions such as add() and replaceHead().
 class FieldChainInfo {
 public:
   using FieldChainImpl = llvm::ImmutableListImpl<const FieldNode &>;
   using FieldChain = llvm::ImmutableList<const FieldNode &>;
 
 private:
   FieldChain::Factory &ChainFactory;
   FieldChain Chain;
 
   FieldChainInfo(FieldChain::Factory &F, FieldChain NewChain)
       : FieldChainInfo(F) {
     Chain = NewChain;
   }
 
 public:
   FieldChainInfo() = delete;
   FieldChainInfo(FieldChain::Factory &F) : ChainFactory(F) {}
   FieldChainInfo(const FieldChainInfo &Other) = default;
 
+  /// Constructs a new FieldChainInfo object with \p FN appended.
   template <class FieldNodeT> FieldChainInfo add(const FieldNodeT &FN);
+
+  /// Constructs a new FieldChainInfo object with \p FN as the new head of the
+  /// list.
   template <class FieldNodeT> FieldChainInfo replaceHead(const FieldNodeT &FN);
 
   bool contains(const FieldRegion *FR) const;
   bool isEmpty() const { return Chain.isEmpty(); }
 
   const FieldRegion *getUninitRegion() const;
   const FieldNode &getHead() { return Chain.getHead(); }
+
   void printNoteMsg(llvm::raw_ostream &Out) const;
 };
 
 using UninitFieldMap = std::map<const FieldRegion *, llvm::SmallString<50>>;
 
 /// Searches for and stores uninitialized fields in a non-union object.
 class FindUninitializedFields {
   ProgramStateRef State;
public:
   /// uninitialized fields in R.
   FindUninitializedFields(ProgramStateRef State,
                           const TypedValueRegion *const R,
                           bool CheckPointeeInitialization);
 
   const UninitFieldMap &getUninitFields() { return UninitFields; }
 
   /// Returns whether the analyzed region contains at least one initialized
-  /// field.
+  /// field. Note that this includes subfields as well, not just direct ones,
+  /// and will return false if an uninitialized pointee is found with
+  /// CheckPointeeInitialization enabled.
   bool isAnyFieldInitialized() { return IsAnyFieldInitialized; }
 
 private:
-  // For the purposes of this checker, we'll regard the object under checking as
-  // a directed tree, where
+  // For the purposes of this checker, we'll regard the analyzed region as a
+  // directed tree, where
   //   * the root is the object under checking
   //   * every node is an object that is
   //     - a union
   //     - a non-union record
-  //     - a pointer/reference
+  //     - dereferencable (see isDereferencableType())
   //     - an array
-  //     - of a primitive type, which we'll define later in a helper function.
+  //     - of a primitive type (see isPrimitiveType())
   //   * the parent of each node is the object that contains it
   //   * every leaf is an array, a primitive object, a nullptr or an undefined
   //   pointer.
   //
   // Example:
   //
   //   struct A {
   //      struct B {
private:
   //
   //   this->b.x
   //   this->iptr (pointee uninit)
   //   this->bptr (pointer uninit)
   //
   // We'll traverse each node of the above graph with the appropiate one of
   // these methods:
 
-  /// This method checks a region of a union object, and returns true if no
-  /// field is initialized within the region.
+  /// Checks the region of a union object, and returns true if no field is
+  /// initialized within the region.
   bool isUnionUninit(const TypedValueRegion *R);
 
-  /// This method checks a region of a non-union object, and returns true if
-  /// an uninitialized field is found within the region.
+  /// Checks a region of a non-union object, and returns true if an
+  /// uninitialized field is found within the region.
   bool isNonUnionUninit(const TypedValueRegion *R, FieldChainInfo LocalChain);
 
-  /// This method checks a region of a pointer or reference object, and returns
-  /// true if the ptr/ref object itself or any field within the pointee's region
-  /// is uninitialized.
-  bool isPointerOrReferenceUninit(const FieldRegion *FR,
-                                  FieldChainInfo LocalChain);
-
-  /// This method returns true if the value of a primitive object is
+  /// Checks a region of a pointer or reference object, and returns true if the
+  /// ptr/ref object itself or any field within the pointee's region is
   /// uninitialized.
+  bool isDereferencableUninit(const FieldRegion *FR, FieldChainInfo LocalChain);
+
+  /// Returns true if the value of a primitive object is uninitialized.
   bool isPrimitiveUninit(const SVal &V);
 
   // Note that we don't have a method for arrays -- the elements of an array are
   // often left uninitialized intentionally even when it is of a C++ record
   // type, so we'll assume that an array is always initialized.
   // TODO: Add a support for nonloc::LocAsInteger.
 
   /// Processes LocalChain and attempts to insert it into UninitFields. Returns
   /// true on success.
   ///
   /// Since this class analyzes regions with recursion, we'll only store
   /// references to temporary FieldNode objects created on the stack. This means
   /// that after analyzing a leaf of the directed tree described above, the
   /// elements LocalChain references will be destructed, so we can't store it
   /// directly.
   bool addFieldToUninits(FieldChainInfo LocalChain);
 };
 
-/// Returns true if T is a primitive type. We defined this type so that for
-/// objects that we'd only like analyze as much as checking whether their
-/// value is undefined or not, such as ints and doubles, can be analyzed with
-/// ease. This also helps ensuring that every special field type is handled
-/// correctly.
+/// Returns true if T is a primitive type. An object of a primitive type only
+/// needs to be analyzed as much as checking whether their value is undefined.
 inline bool isPrimitiveType(const QualType &T) {
   return T->isBuiltinType() || T->isEnumeralType() ||
          T->isMemberPointerType() || T->isBlockPointerType();
 }
 
 inline bool isDereferencableType(const QualType &T) {
   return T->isAnyPointerType() || T->isReferenceType();
 }

lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp

public:
   }
 
   virtual void printSeparator(llvm::raw_ostream &Out) const override {
     Out << '.';
   }
 };
 
 /// Represents that the FieldNode that comes after this is declared in a base
-/// of the previous FieldNode.
+/// of the previous FieldNode. As such, this descendant doesn't wrap a
+/// FieldRegion, and is purely a tool to describe a relation between two other
+/// FieldRegion wrapping descendants.
 class BaseClass final : public FieldNode {
   const QualType BaseClassT;
 
 public:
   BaseClass(const QualType &T) : FieldNode(nullptr), BaseClassT(T) {
     assert(!T.isNull());
     assert(T->getAsCXXRecordDecl());
   }
for (const FieldDecl *I : RD->fields()) {
     }
 
     if (T->isArrayType()) {
       IsAnyFieldInitialized = true;
       continue;
     }
 
     if (isDereferencableType(T)) {
-      if (isPointerOrReferenceUninit(FR, LocalChain))
+      if (isDereferencableUninit(FR, LocalChain))
         ContainsUninitField = true;
       continue;
     }
 
     if (isPrimitiveType(T)) {
       SVal V = State->getSVal(FieldVal);
 
       if (isPrimitiveUninit(V)) {
         if (addFieldToUninits(LocalChain.add(RegularField(FR))))
           ContainsUninitField = true;
       }
       continue;
     }
 
     llvm_unreachable("All cases are handled!");
   }
 
-  // Checking bases.
+  // Checking bases. The checker will regard inherited data members as direct
+  // fields.
   const auto *CXXRD = dyn_cast<CXXRecordDecl>(RD);
   if (!CXXRD)
     return ContainsUninitField;
 
   for (const CXXBaseSpecifier &BaseSpec : CXXRD->bases()) {
     const auto *BaseRegion = State->getLValue(BaseSpec, R)
                                  .castAs<loc::MemRegionVal>()
                                  .getRegionAs<TypedValueRegion>();

 }
 
 //===----------------------------------------------------------------------===//
 //                       Methods for FieldChainInfo.
 //===----------------------------------------------------------------------===//
 
 const FieldRegion *FieldChainInfo::getUninitRegion() const {
   assert(!Chain.isEmpty() && "Empty fieldchain!");
+
+  // ImmutableList::getHead() isn't a const method, hence the not too nice
+  // implementation.
   return (*Chain.begin()).getRegion();
 }
 
 bool FieldChainInfo::contains(const FieldRegion *FR) const {
   for (const FieldNode &Node : Chain) {
     if (Node.isSameRegion(FR))
       return true;
   }
   return false;
 }
 
 /// 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`.
+/// the chain is to be printed by `FieldChainInfo::print`.
 static void printTail(llvm::raw_ostream &Out,
                       const FieldChainInfo::FieldChainImpl *L);
 
-// TODO: This function constructs an incorrect string if a void pointer is a
-// part of the chain:
-//
-//   struct B { int x; }
-//
-//   struct A {
-//     void *vptr;
-//     A(void* vptr) : vptr(vptr) {}
-//   };
-//
-//   void f() {
-//     B b;
-//     A a(&b);
-//   }
-//
-// The note message will be "uninitialized field 'this->vptr->x'", even though
-// void pointers can't be dereferenced. This should be changed to "uninitialized
-// field 'static_cast<B*>(this->vptr)->x'".
-//
-// TODO: This function constructs an incorrect fieldchain string in the
-// following case:
+// FIXME: This function constructs an incorrect string in the following case:
 //
 //   struct Base { int x; };
 //   struct D1 : Base {}; struct D2 : Base {};
 //
 //   struct MostDerived : D1, D2 {
 //     MostDerived() {}
 //   }
 //
if (CXXParent && CXXParent->isLambda()) {
     return It->getCapturedVar()->getName();
   }
 
   return Field->getName();
 }
 
 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);
 }

lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp

-//===----- UninitializedPointer.cpp ------------------------------*- C++ -*-==//
+//===----- UninitializedPointee.cpp ------------------------------*- C++ -*-==//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
virtual void printSeparator(llvm::raw_ostream &Out) const override {
     Out << "->";
   }
 };
 
 } // end of anonymous namespace
 
 // Utility function declarations.
 
-/// Returns whether T 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.
+/// Returns whether \p T can be (transitively) dereferenced to a void pointer
+/// type (void*, void**, ...).
 static bool isVoidPointer(QualType T);
 
 using DereferenceInfo = std::pair<const TypedValueRegion *, bool>;
 
 /// Dereferences \p FR and returns with the pointee's region, and whether it
 /// needs to be casted back to it's location type. If for whatever reason
 /// dereferencing fails, returns with None.
 static llvm::Optional<DereferenceInfo> dereference(ProgramStateRef State,
                                                    const FieldRegion *FR);
 
 //===----------------------------------------------------------------------===//
 //                   Methods for FindUninitializedFields.
 //===----------------------------------------------------------------------===//
 
-// Note that pointers/references don't contain fields themselves, so in this
-// function we won't add anything to LocalChain.
-bool FindUninitializedFields::isPointerOrReferenceUninit(
+bool FindUninitializedFields::isDereferencableUninit(
     const FieldRegion *FR, FieldChainInfo LocalChain) {
 
   assert(isDereferencableType(FR->getDecl()->getType()) &&
          "This method only checks dereferencable objects!");
 
   SVal V = State->getSVal(FR);
 
   if (V.isUnknown() || V.getAs<loc::ConcreteInt>()) {
static llvm::Optional<DereferenceInfo> dereference(ProgramStateRef State,
   VisitedRegions.insert(R);
 
   // We acquire the dynamic type of R,
   QualType DynT = R->getLocationType();
 
   while (const MemRegion *Tmp = State->getSVal(R, DynT).getAsRegion()) {
 
     R = Tmp->getAs<TypedValueRegion>();
-
     if (!R)
       return None;
 
     // We found a cyclic pointer, like int *ptr = (int *)&ptr.
-    // TODO: Report these fields too.
+    // TODO: Should we report these fields too?
     if (!VisitedRegions.insert(R).second)
       return None;
 
     DynT = R->getLocationType();
     // In order to ensure that this loop terminates, we're also checking the
     // dynamic type of R, since type hierarchy is finite.
     if (isDereferencableType(DynT->getPointeeType()))
       break;