[analyzer][UninitializedObjectChecker] Void pointer objects are casted back to their dynmic type in note message

[Szelethus]

Rebased to the latest trunk.

[george.karpenkov]

Cf. my comments to https://reviews.llvm.org/D49437: while this change looks great, is it possible to separate the pointer chasing from the rest of the checker?

[NoQ]

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

290

Shouldn't this rather say something like static_cast<int *>(this->vptr)? That's the normal syntax to do this sort of stuff, i guess.

[whisperity]

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

290

Not just "normal syntax", this syntax of the output does not compile.

foo.cpp:6:13: error: expected unqualified-id
      this->static_cast<int*>(vptr) = new int();

[Szelethus]

The solution now relies on the refactored version of the checker. Note that it modifies roughly 75% less code ;).

lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp

 /// 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 FieldChainImpl = llvm::ImmutableListImpl<const FieldRegion *>;
+
+  // If the fieldchain has void pointers or nonloc::LocAsInteger objects,
+  // they have to be casted back in order to dereference them.
+  //
+  //   struct B {
+  //     void *ptr;
+  //     B(void *ptr) : ptr(ptr) {}
+  //   };
+  //   struct A { int x; } a; // say this isn't zero initialized
+  //
+  //   B b(&a); // warning
+  //
+  // For cases like this, we can't report that 'this->ptr->a' is uninitialized,
+  // since void pointers can't be dereferenced, but rather the note message
+  // should say 'static_cast<A *>(this->ptr)->a' is uninitialized.
+  //
+  // We'll store which elements of the fieldchain needs to be casted back,
+  // and the type it needs to be casted to.
+  using CastBack = const std::pair<QualType, const FieldChainImpl *>;
+  using CastBacksListImpl = llvm::ImmutableListImpl<CastBack>;
+
+public:
+  // We'll expose these types for a nicer looking factory instantionation.
   using FieldChain = llvm::ImmutableList<const FieldRegion *>;
+  using CastBackList = llvm::ImmutableList<CastBack>;
 
+private:
   FieldChain Chain;
+  CastBackList CastBacks;
 
   const bool IsDereferenced = false;
 
 public:
   FieldChainInfo() = default;
 
   FieldChainInfo(const FieldChainInfo &Other, const bool IsDereferenced)
-      : Chain(Other.Chain), IsDereferenced(IsDereferenced) {}
+      : Chain(Other.Chain), CastBacks(Other.CastBacks),
+        IsDereferenced(IsDereferenced) {}
 
   FieldChainInfo(const FieldChainInfo &Other, const FieldRegion *FR,
                  const bool IsDereferenced = false);
 
+  FieldChainInfo(const FieldChainInfo &Other, const FieldRegion *FR,
+                 const bool IsDereferenced, QualType CastBackType);
+
   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 &Out) 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 &Out,
-                        const llvm::ImmutableListImpl<const FieldRegion *> *L);
+  static void printTail(llvm::raw_ostream &Out, const FieldChainImpl *L,
+                        const CastBacksListImpl *C);
   friend struct FieldChainInfoComparator;
 };
 
 struct FieldChainInfoComparator {
   bool operator()(const FieldChainInfo &lhs, const FieldChainInfo &rhs) {
     assert(!lhs.Chain.isEmpty() && !rhs.Chain.isEmpty() &&
            "Attempted to store an empty fieldchain!");
     return *lhs.Chain.begin() < *rhs.Chain.begin();
private:
   // type, so we'll assume that an array is always initialized.
   // TODO: Add a support for nonloc::LocAsInteger.
 };
 
 } // end of anonymous namespace
 
 // Static variable instantionations.
 
-static llvm::ImmutableListFactory<const FieldRegion *> Factory;
+static FieldChainInfo::FieldChain::Factory ChainFactory;
+static FieldChainInfo::CastBackList::Factory CastBackFactory;
 
 // Utility function declarations.
 
 /// Returns the object that was constructed by CtorDecl, or None if that isn't
 /// possible.
 static Optional<nonloc::LazyCompoundVal>
 getObjectVal(const CXXConstructorDecl *CtorDecl, CheckerContext &Context);
 
bool FindUninitializedFields::isPointerOrReferenceUninit(
 
   DynamicTypeInfo DynTInfo = getDynamicTypeInfo(State, L.getRegion());
   if (!DynTInfo.isValid()) {
     IsAnyFieldInitialized = true;
     return false;
   }
 
   QualType DynT = DynTInfo.getType();
+  // If the static type of the field is a void pointer, we need to cast it back
+  // to the dynamic type before dereferencing.
+  bool NeedsCastBack = isVoidPointer(FR->getDecl()->getType());
 
   if (isVoidPointer(DynT)) {
     IsAnyFieldInitialized = true;
     return false;
   }
 
   // At this point the pointer itself is initialized and points to a valid
   // location, we'll now check the pointee.
bool FindUninitializedFields::isPointerOrReferenceUninit(
   }
 
   // If FR is a pointer pointing to a non-primitive type.
   if (Optional<nonloc::LazyCompoundVal> RecordV =
           DerefdV.getAs<nonloc::LazyCompoundVal>()) {
 
     const TypedValueRegion *R = RecordV->getRegion();
 
-    if (DynT->getPointeeType()->isStructureOrClassType())
+    if (DynT->getPointeeType()->isStructureOrClassType()) {
+      if (NeedsCastBack)
+        return isNonUnionUninit(
+            R, {LocalChain, FR, /*IsDereferenced */ false, DynT});
       return isNonUnionUninit(R, {LocalChain, FR});
+    }
 
     if (DynT->getPointeeType()->isUnionType()) {
       if (isUnionUninit(R)) {
+        if (NeedsCastBack)
+          return addFieldToUninits(
+              {LocalChain, FR, /*IsDereferenced*/ true, DynT});
         return addFieldToUninits({LocalChain, FR, /*IsDereferenced*/ true});
       } else {
         IsAnyFieldInitialized = true;
         return false;
       }
     }
 
     if (DynT->getPointeeType()->isArrayType()) {
       IsAnyFieldInitialized = true;
       return false;
     }
 
     llvm_unreachable("All cases are handled!");
   }
 
   assert((isPrimitiveType(DynT->getPointeeType()) || DynT->isPointerType() ||
           DynT->isReferenceType()) &&
          "At this point FR must either have a primitive dynamic type, or it "
          "must be a null, undefined, unknown or concrete pointer!");
 
-  if (isPrimitiveUninit(DerefdV))
+  if (isPrimitiveUninit(DerefdV)) {
+    if (NeedsCastBack)
+      return addFieldToUninits({LocalChain, FR, /*IsDereferenced*/ true, DynT});
     return addFieldToUninits({LocalChain, FR, /*IsDereferenced*/ true});
+  }
 
   IsAnyFieldInitialized = true;
   return false;
 }
 
 bool FindUninitializedFields::isPrimitiveUninit(const SVal &V) {
   if (V.isUndef())
     return true;
 
   IsAnyFieldInitialized = true;
   return false;
 }
 
 //===----------------------------------------------------------------------===//
 //                       Methods for FieldChainInfo.
 //===----------------------------------------------------------------------===//
 
 FieldChainInfo::FieldChainInfo(const FieldChainInfo &Other,
                                const FieldRegion *FR, const bool IsDereferenced)
     : FieldChainInfo(Other, IsDereferenced) {
   assert(!contains(FR) && "Can't add a field that is already a part of the "
                           "fieldchain! Is this a cyclic reference?");
-  Chain = Factory.add(FR, Other.Chain);
+  Chain = ChainFactory.add(FR, Other.Chain);
+}
+
+FieldChainInfo::FieldChainInfo(const FieldChainInfo &Other,
+                               const FieldRegion *FR, const bool IsDereferenced,
+                               QualType CastBackType)
+    : FieldChainInfo(Other, FR, IsDereferenced) {
+  CastBacks = CastBackFactory.add(
+      std::make_pair(CastBackType, Chain.getInternalPointer()),
+      Other.CastBacks);
 }
 
 bool FieldChainInfo::isPointer() const {
   assert(!Chain.isEmpty() && "Empty fieldchain!");
   return (*Chain.begin())->getDecl()->getType()->isPointerType();
 }
 
 bool FieldChainInfo::isDereferenced() const {
   assert(isPointer() && "Only pointers may or may not be dereferenced!");
   return IsDereferenced;
 }
 
 const FieldDecl *FieldChainInfo::getEndOfChain() const {
   assert(!Chain.isEmpty() && "Empty fieldchain!");
   return (*Chain.begin())->getDecl();
 }
 
-// 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:
 //
 //   struct Base { int x; };
 //   struct D1 : Base {}; struct D2 : Base {};
 //
 //   struct MostDerived : D1, D2 {
 //     MostDerived() {}
 //   }
 //
 // A call to MostDerived::MostDerived() will cause two notes that say
 // "uninitialized field 'this->x'", but we can't refer to 'x' directly,
 // we need an explicit namespace resolution whether the uninit field was
 // 'D1::x' or 'D2::x'.
 void FieldChainInfo::print(llvm::raw_ostream &Out) const {
   if (Chain.isEmpty())
     return;
 
-  const llvm::ImmutableListImpl<const FieldRegion *> *L =
-      Chain.getInternalPointer();
-  printTail(Out, L->getTail());
-  Out << getVariableName(L->getHead()->getDecl());
+  const FieldChainImpl *L = Chain.getInternalPointer();
+  const CastBacksListImpl *C = CastBacks.getInternalPointer();
+  const FieldDecl *Field = L->getHead()->getDecl();
+
+  // During the construction of a FieldChainInfo object, the CastBacks field
+  // will be filled in order with pointers to certain elements of the FieldChain
+  // that needs to be casted to another type.
+  // If L is the first element stored in CastBacks, we'll pass the rest of the
+  // CastBacks list to printTail. If not, we know that the first element of it
+  // will refer to another field in the fieldchain, unless no field needs to be
+  // casted back.
+  if (C && L == C->getHead().second) {
+    printTail(Out, L->getTail(), C->getTail());
+    Out << "static_cast<" << C->getHead().first.getAsString() << ">("
+        << getVariableName(Field) << ')';
+  } else {
+    printTail(Out, L->getTail(), C);
+    Out << getVariableName(Field);
+  }
 }
 
-void FieldChainInfo::printTail(
-    llvm::raw_ostream &Out,
-    const llvm::ImmutableListImpl<const FieldRegion *> *L) {
-  if (!L)
+void FieldChainInfo::printTail(llvm::raw_ostream &Out, const FieldChainImpl *L,
+                               const CastBacksListImpl *C) {
+  if (!L) {
+    assert(!C && "Entire fieldchain printed but some elements of CastBacks is "
+                 "unused!");
     return;
+  }
+
+  if (C && L == C->getHead().second)
+    printTail(Out, L->getTail(), C->getTail());
+  else
+    printTail(Out, L->getTail(), C);
 
-  printTail(Out, L->getTail());
   const FieldDecl *Field = L->getHead()->getDecl();
+  if (C && L == C->getHead().second)
+    Out << "static_cast<" << C->getHead().first.getAsString() << ">("
+        << getVariableName(Field) << ')';
+  else
     Out << getVariableName(Field);
   Out << (Field->getType()->isPointerType() ? "->" : ".");
 }
 
 //===----------------------------------------------------------------------===//
 //                           Utility functions.
 //===----------------------------------------------------------------------===//
 
 static bool isVoidPointer(QualType T) {

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

struct CyclicVoidPointerTest {
   CyclicVoidPointerTest() : vptr(&vptr) {}
 };
 
 void fCyclicVoidPointerTest() {
   CyclicVoidPointerTest();
 }
 
 struct IntDynTypedVoidPointerTest1 {
-  void *vptr; // expected-note{{uninitialized pointee 'this->vptr'}}
+  void *vptr; // expected-note{{uninitialized pointee 'this->static_cast<int *>(vptr)'}}
   int dontGetFilteredByNonPedanticMode = 0;
 
   IntDynTypedVoidPointerTest1(void *vptr) : vptr(vptr) {} // expected-warning{{1 uninitialized field}}
 };
 
 void fIntDynTypedVoidPointerTest1() {
   int a;
   IntDynTypedVoidPointerTest1 tmp(&a);
 }
 
 struct RecordDynTypedVoidPointerTest {
   struct RecordType {
-    int x; // expected-note{{uninitialized field 'this->vptr->x'}}
-    int y; // expected-note{{uninitialized field 'this->vptr->y'}}
+    int x; // expected-note{{uninitialized field 'this->static_cast<struct RecordDynTypedVoidPointerTest::RecordType *>(vptr)->x'}}
+    int y; // expected-note{{uninitialized field 'this->static_cast<struct RecordDynTypedVoidPointerTest::RecordType *>(vptr)->y'}}
   };
 
   void *vptr;
   int dontGetFilteredByNonPedanticMode = 0;
 
   RecordDynTypedVoidPointerTest(void *vptr) : vptr(vptr) {} // expected-warning{{2 uninitialized fields}}
 };
 
 void fRecordDynTypedVoidPointerTest() {
   RecordDynTypedVoidPointerTest::RecordType a;
   RecordDynTypedVoidPointerTest tmp(&a);
 }
 
 struct NestedNonVoidDynTypedVoidPointerTest {
   struct RecordType {
-    int x;      // expected-note{{uninitialized field 'this->vptr->x'}}
-    int y;      // expected-note{{uninitialized field 'this->vptr->y'}}
-    void *vptr; // expected-note{{uninitialized pointee 'this->vptr->vptr'}}
+    int x;      // expected-note{{uninitialized field 'this->static_cast<struct NestedNonVoidDynTypedVoidPointerTest::RecordType *>(vptr)->x'}}
+    int y;      // expected-note{{uninitialized field 'this->static_cast<struct NestedNonVoidDynTypedVoidPointerTest::RecordType *>(vptr)->y'}}
+    void *vptr; // expected-note{{uninitialized pointee 'this->static_cast<struct NestedNonVoidDynTypedVoidPointerTest::RecordType *>(vptr)->static_cast<char *>(vptr)'}}
   };
 
   void *vptr;
   int dontGetFilteredByNonPedanticMode = 0;
 
   NestedNonVoidDynTypedVoidPointerTest(void *vptr, void *c) : vptr(vptr) {
     static_cast<RecordType *>(vptr)->vptr = c; // expected-warning{{3 uninitialized fields}}
   }