[PATCH] D51057: [analyzer][UninitializedObjectChecker] Fixed dereferencing

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Yup, this looks great and that's exactly how i imagined it.



================
Comment at: lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObject.h:261-265
+inline bool isLocType(const QualType &T) {
+  return T->isAnyPointerType() || T->isReferenceType() ||
+         T->isBlockPointerType();
+}
+
----------------
We have a fancy static `Loc::isLocType()`.


================
Comment at: lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp:126-127
   if (V.isUndef()) {
+    assert(!FR->getDecl()->getType()->isReferenceType() &&
+           "References must be initialized!");
     return addFieldToUninits(
----------------
Good catch.

It might still be possible to initialize a reference with an already-undefined pointer if core checkers are turned off, but we don't support turning them off, so i guess it's fine.


================
Comment at: lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp:177
 
-  if (isPrimitiveUninit(V)) {
+  const SVal &PointeeV = State->getSVal(loc::MemRegionVal(R));
+  if (isPrimitiveUninit(PointeeV)) {
----------------
Just `SVal`. And you should be able to pass `R` directly into `getSVal`.


================
Comment at: lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp:210
   assert(V.getAs<loc::MemRegionVal>() && "V must be loc::MemRegionVal!");
+  auto Tmp = V.getAs<loc::MemRegionVal>();
+
----------------
We usually just do `.getAsRegion()`.

And then later `dyn_cast` it.


================
Comment at: lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp:212-216
+  // 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.
----------------
Ok, i have a new concern that i didn't think about before.

There's nothing special about symbolic regions. You can make a linked list entirely of concrete regions:

```
struct List {
  List *next;
  List(): next(this) {}
};

void foo() {
  List list;
}
```

In this case the finite-ness of the type system won't save us.

I guess we could also memoize base regions that we visit :/ This is guaranteed to terminate because all of our concrete regions are based either on AST nodes (eg. global variables) or on certain events that happened previously during analysis (eg. local variables or temporaries, as they depend on the stack frame which must have been previously entered). I don't immediately see a better solution.


================
Comment at: lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp:223
+  const auto *R = Tmp->getRegionAs<TypedValueRegion>();
+  assert(R);
+
----------------
We might have eliminated symbolic regions but we may still have concrete function pointers (which are `TypedRegion`s that aren't `TypedValueRegion`s, it's pretty weird), and i guess we might even encounter an `AllocaRegion` (which is completely untyped). I guess we should not try to dereference them either.


================
Comment at: lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp:240-244
+    if (Tmp->getRegion()->getSymbolicBase())
       return None;
-    }
 
-    V = State->getSVal(*Tmp, DynT);
+    DynT = DynT->getPointeeType();
+    R = Tmp->getRegionAs<TypedValueRegion>();
----------------
This code seems to be duplicated with the "0th iteration" before the loop. I guess you can put everything into the loop.


Repository:
  rC Clang

https://reviews.llvm.org/D51057