[analyzer][UninitializedObjectChecker] Refactoring p6.: Move dereferencing to a function

[Szelethus]

Summary

Now that it has it's own file, it makes little sense for isPointerOrReferenceUninit to be this large, so I moved dereferencing to a separate function.

[Szelethus]

Fixed a comment.

[george.karpenkov]

lib/StaticAnalyzer/Checkers/UninitializedPointee.cpp

78

In general, using return values is better than out-parameters.
Could you instead return Optional<std::pair<SVal, QualType>> ?

[NoQ]

lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp

223 ↗

(On Diff #161209)

Hmm, i still have concerns about things like int *x = (int *)&x;. Why not just check the type to terminate the loop? Type hierarchy is guaranteed to be finite.

[NoQ]

Which shouldn't prevent us from moving code around.

[Szelethus]

lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp

223 ↗

(On Diff #161209)

There actually is a testcase for that -- it would create a nonloc::LocAsInteger, not a loc::MemRegionVal.

I'll add a TODO to revisit this loop condition (again :) ).

[NoQ]

lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp

223 ↗

(On Diff #161209)

Ok, let's try with one more asterisk:

1 void test() {
2   int **x = (int **)&x;
3   int *y = *x;
4   int z = *y;
5 }

Here's what i get in the Store:

(x,0,direct) : &element{x,0 S64b,int *}
(y,0,direct) : &element{x,0 S64b,int *}
(z,0,direct) : &element{x,0 S64b,int *}

[Szelethus]

lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp

223 ↗

(On Diff #161209)

Sounds fun, I'll see how the checker behaves to these when I'm in the office.

[Szelethus]

lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp

223 ↗

(On Diff #161209)

Yup, you were correct, it ends up in an infinite loop. I'll add the testcase for it before commiting.

lib/StaticAnalyzer/Checkers/UninitializedPointee.cpp

 
 // 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.
 static bool isVoidPointer(QualType T);
 
+/// Dereferences V, and stores the dereferences value in it, the dynamic type
+/// of V in DynT, and whether it needs to be casted back in NeedsCastBack.
+///
+/// V must be loc::MemRegionVal.
+///
+/// If for whatever reason dereferencing fails, returns with false.
+static bool dereference(ProgramStateRef State, SVal &V, QualType &DynT);
+
 //===----------------------------------------------------------------------===//
 //                   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(
     const FieldRegion *FR, FieldChainInfo LocalChain) {
bool FindUninitializedFields::isPointerOrReferenceUninit(
 
   if (!CheckPointeeInitialization) {
     IsAnyFieldInitialized = true;
     return false;
   }
 
   assert(V.getAs<loc::MemRegionVal>() &&
          "At this point V must be loc::MemRegionVal!");
-  auto L = V.castAs<loc::MemRegionVal>();
-
-  // We can't reason about symbolic regions, assume its initialized.
-  // Note that this also avoids a potential infinite recursion, because
-  // constructors for list-like classes are checked without being called, and
-  // the Static Analyzer will construct a symbolic region for Node *next; or
-  // similar code snippets.
-  if (L.getRegion()->getSymbolicBase()) {
-    IsAnyFieldInitialized = true;
-    return false;
-  }
-
-  DynamicTypeInfo DynTInfo = getDynamicTypeInfo(State, L.getRegion());
-  if (!DynTInfo.isValid()) {
-    IsAnyFieldInitialized = true;
-    return false;
-  }
 
-  QualType DynT = DynTInfo.getType();
-
-  if (isVoidPointer(DynT)) {
-    IsAnyFieldInitialized = true;
-    return false;
-  }
+  QualType DynT;
 
   // At this point the pointer itself is initialized and points to a valid
   // location, we'll now check the pointee.
-  SVal DerefdV = State->getSVal(V.castAs<Loc>(), DynT);
-
-  // If DerefdV is still a pointer value, we'll dereference it again (e.g.:
-  // int** -> int*).
-  while (auto Tmp = DerefdV.getAs<loc::MemRegionVal>()) {
-    if (Tmp->getRegion()->getSymbolicBase()) {
-      IsAnyFieldInitialized = true;
-      return false;
-    }
-
-    DynTInfo = getDynamicTypeInfo(State, Tmp->getRegion());
-    if (!DynTInfo.isValid()) {
-      IsAnyFieldInitialized = true;
-      return false;
-    }
-
-    DynT = DynTInfo.getType();
-    if (isVoidPointer(DynT)) {
-      IsAnyFieldInitialized = true;
-      return false;
-    }
+  if (!dereference(State, V, DynT)) {
 
-    DerefdV = State->getSVal(*Tmp, DynT);
-  }
-
   // If FR is a pointer pointing to a non-primitive type.
   if (Optional<nonloc::LazyCompoundVal> RecordV =
-          DerefdV.getAs<nonloc::LazyCompoundVal>()) {
+          V.getAs<nonloc::LazyCompoundVal>()) {
 
     const TypedValueRegion *R = RecordV->getRegion();
 
     if (DynT->getPointeeType()->isStructureOrClassType())
       return isNonUnionUninit(R, LocalChain.add(LocField(FR)));
 
     if (DynT->getPointeeType()->isUnionType()) {
       if (isUnionUninit(R)) {
if (Optional<nonloc::LazyCompoundVal> RecordV =
     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(V))
     return addFieldToUninits(LocalChain.add(LocField(FR)));
 
   IsAnyFieldInitialized = true;
   return false;
 }
 
 //===----------------------------------------------------------------------===//
 //                           Utility functions.
 //===----------------------------------------------------------------------===//
 
 static bool isVoidPointer(QualType T) {
   while (!T.isNull()) {
     if (T->isVoidPointerType())
       return true;
     T = T->getPointeeType();
   }
   return false;
 }
+
+static bool dereference(ProgramStateRef State, SVal &V, QualType &DynT) {
+  // If V is multiple pointer value, we'll dereference it again (e.g.: int** ->
+  // int*).
+  while (auto Tmp = V.getAs<loc::MemRegionVal>()) {
+    // We can't reason about symbolic regions, assume its initialized.
+    // Note that this also avoids a potential infinite recursion, because
+    // constructors for list-like classes are checked without being called, and
+    // the Static Analyzer will construct a symbolic region for Node *next; or
+    // similar code snippets.
+    if (Tmp->getRegion()->getSymbolicBase()) {
+      return false;
+    }
+
+    DynamicTypeInfo DynTInfo = getDynamicTypeInfo(State, Tmp->getRegion());
+    if (!DynTInfo.isValid()) {
+      return false;
+    }
+
+    DynT = DynTInfo.getType();
+
+    if (isVoidPointer(DynT)) {
+      return false;
+    }
+
+    V = State->getSVal(*Tmp, DynT);
+  }
+  return true;
+}