[analyzer][UninitializedObjectChecker] Support for nonloc::LocAsInteger

[george.karpenkov]

I think a checker for uninitialized values left after a constructor call is very valuable.

It is uncertain whether it is valuable to check that all pointers point to initialized objects for a number of reasons:

• As we have seen, it increases the number of false positives
• It significantly increases the complexity of the checker
• Projects would be more reluctant to try the checker due to the first point

Could we actually go in an opposite direction and try to separate the pointer-chasing into perhaps a separate checker?

[Szelethus]

I just added a new patch as you wrote that comment (D49438)! Separating this functionality to a separate checker sounds great. I didn't actually check it, but I bet that around ~30-40% is at least indirectly in relation with pointer/reference handling, and it is getting a little out of hand.

I'll definitely explore this option too. Thanks for the idea! :)

[george.karpenkov]

I'm fine with this if pointer-chasing is not on by default.

[NoQ]

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

26

This one also suffers from D49228#1176136

[Szelethus]

Re-implemented with the refactored checker.

[Szelethus]

Rebased to D50509.

[Szelethus]

Rebased to D51057.

[NoQ]

Let's see what it finds, i guess.

lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp

private:
 
   /// This method 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.
 };
 
 } // end of anonymous namespace
 
 // Static variable instantionations.
 
 static FieldChainInfo::FieldChain::Factory ChainFactory;
 static FieldChainInfo::CastBackList::Factory CastBackFactory;

 /// Constructs a note message for a given FieldChainInfo object.
 static void printNoteMessage(llvm::raw_ostream &Out,
                              const FieldChainInfo &Chain);
 
 /// Returns with Field's name. This is a helper function to get the correct name
 /// even if Field is a captured lambda variable.
 static StringRef getVariableName(const FieldDecl *Field);
 
+/// Returns with "reinterpret_cast" if Field is an integer (meaning that it is a
+/// nonloc::LocAsInteger), or with "static_cast" otherwise.
+///
+/// This function doesn't check whether Field really needs to be casted back.
+static StringRef getCastTypeAsString(const FieldDecl *Field);
+
 //===----------------------------------------------------------------------===//
 //                  Methods for UninitializedObjectChecker.
 //===----------------------------------------------------------------------===//
 
 void UninitializedObjectChecker::checkEndFunction(
     CheckerContext &Context) const {
 
   const auto *CtorDecl = dyn_cast_or_null<CXXConstructorDecl>(
if (T->isUnionType()) {
       continue;
     }
 
     if (T->isArrayType()) {
       IsAnyFieldInitialized = true;
       continue;
     }
 
-    if (T->isPointerType() || T->isReferenceType()) {
+    SVal V = State->getSVal(FieldVal);
+
+    if (T->isPointerType() || T->isReferenceType() ||
+        V.getAs<nonloc::LocAsInteger>()) {
       if (isPointerOrReferenceUninit(FR, LocalChain))
         ContainsUninitField = true;
       continue;
     }
 
     if (isPrimitiveType(T)) {
-      SVal V = State->getSVal(FieldVal);
-
       if (isPrimitiveUninit(V)) {
         if (addFieldToUninits({LocalChain, FR}))
           ContainsUninitField = true;
       }
       continue;
     }
 
     llvm_unreachable("All cases are handled!");
bool FindUninitializedFields::isUnionUninit(const TypedValueRegion *R) {
   return false;
 }
 
 // Note that pointers/references don't contain fields themselves, so in this
 // function we won't add anything to LocalChain.
 bool FindUninitializedFields::isPointerOrReferenceUninit(
     const FieldRegion *FR, FieldChainInfo LocalChain) {
 
-  assert((FR->getDecl()->getType()->isPointerType() ||
-          FR->getDecl()->getType()->isReferenceType()) &&
-         "This method only checks pointer/reference objects!");
-
   SVal V = State->getSVal(FR);
 
+  assert((FR->getDecl()->getType()->isPointerType() ||
+          FR->getDecl()->getType()->isReferenceType() ||
+          V.getAs<nonloc::LocAsInteger>()) &&
+         "This method only checks pointer/reference or LocAsInteger objects!");
+
+  // If the static type of the field is a void pointer or is a
+  // nonloc::LocAsInteger, we need to cast it back to the dynamic type before
+  // dereferencing.
+  bool NeedsCastBack = false;
+
+  if (auto LocAsInt = V.getAs<nonloc::LocAsInteger>()) {
+    NeedsCastBack = true;
+    V = LocAsInt->getLoc();
+  }
+
   if (V.isUnknown() || V.getAs<loc::ConcreteInt>()) {
     IsAnyFieldInitialized = true;
     return false;
   }
 
   if (V.isUndef()) {
     return addFieldToUninits({LocalChain, FR});
   }
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());
+  NeedsCastBack = (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 (NeedsCastBack)
-        return isNonUnionUninit(R, {LocalChain, FR, /*IsDereferenced */false, DynT});
+        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});
FieldChainInfo::FieldChainInfo(const FieldChainInfo &Other,
     : 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();
+
+  const QualType &T = (*Chain.begin())->getDecl()->getType();
+
+  // If the field is a nonloc::LocAsInteger, we'll also return true.
+  if (const CastBacksListImpl *C = CastBacks.getInternalPointer()) {
+    if (Chain.getInternalPointer() == C->getHead().second && T->isIntegerType())
+      return true;
+  }
+
+  return T->isPointerType();
 }
 
 bool FieldChainInfo::isDereferenced() const {
   assert(isPointer() && "Only pointers may or may not be dereferenced!");
   return IsDereferenced;
 }
 
 const FieldDecl *FieldChainInfo::getEndOfChain() const {
void FieldChainInfo::print(llvm::raw_ostream &Out) const {
   // 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) << ')';
+    Out << getCastTypeAsString(Field) << "<" << C->getHead().first.getAsString()
+        << ">(" << getVariableName(Field) << ')';
   } else {
     printTail(Out, L->getTail(), C);
     Out << getVariableName(Field);
   }
 }
 
 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);
 
   const FieldDecl *Field = L->getHead()->getDecl();
-  
   if (C && L == C->getHead().second)
-    Out << "static_cast<" << C->getHead().first.getAsString() << ">("
-        << getVariableName(Field) << ')';
+    Out << getCastTypeAsString(Field) << "<" << C->getHead().first.getAsString()
+        << ">(" << getVariableName(Field) << ')';
   else
     Out << getVariableName(Field);
   Out << (Field->getType()->isPointerType() ? "->" : ".");
 }
 
 //===----------------------------------------------------------------------===//
 //                           Utility functions.
 //===----------------------------------------------------------------------===//
if (CXXParent && CXXParent->isLambda()) {
     assert(CXXParent->captures_begin());
     auto It = CXXParent->captures_begin() + Field->getFieldIndex();
     return It->getCapturedVar()->getName();
   }
 
   return Field->getName();
 }
 
+static StringRef getCastTypeAsString(const FieldDecl *Field) {
+  return Field->getType()->isIntegerType() ? "reinterpret_cast" : "static_cast";
+}
+
 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-ptr-ref.cpp

struct ConcreteIntLocTest {
   ConcreteIntLocTest() : ptr(reinterpret_cast<int *>(0xDEADBEEF)) {}
 };
 
 void fConcreteIntLocTest() {
   ConcreteIntLocTest();
 }
 
 //===----------------------------------------------------------------------===//
+// nonloc::LocAsInteger tests.
+//===----------------------------------------------------------------------===//
+
+using intptr_t = long;
+
+struct LocAsIntegerTest {
+  intptr_t ptr; // expected-note{{uninitialized pointee 'this->reinterpret_cast<char *>(ptr)'}}
+  int dontGetFilteredByNonPedanticMode = 0;
+
+  LocAsIntegerTest(void *ptr) : ptr(reinterpret_cast<intptr_t>(ptr)) {} // expected-warning{{1 uninitialized field}}
+};
+
+void fLocAsIntegerTest() {
+  char c;
+  LocAsIntegerTest t(&c);
+}
+
+//===----------------------------------------------------------------------===//
 // Null pointer tests.
 //===----------------------------------------------------------------------===//
 
 class NullPtrTest {
   struct RecordType {
     int x;
     int y;
   };
struct RecordType {
     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}}
-  } 
+  }
 };
 
 void fNestedNonVoidDynTypedVoidPointerTest() {
   NestedNonVoidDynTypedVoidPointerTest::RecordType a;
   char c;
   NestedNonVoidDynTypedVoidPointerTest tmp(&a, &c);
 }