[PATCH] D48325: [analyzer][UninitializedObjectChecker] Support for MemberPointerTypes

[Artem Dergachev via Phabricator via cfe-commits]

NoQ accepted this revision.
NoQ added a comment.

Looks good! One minor comment.



================
Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:488-491
   // TODO: Dereferencing should be done according to the dynamic type.
   while (Optional<Loc> L = DerefdV.getAs<Loc>()) {
     DerefdV = State->getSVal(*L);
   }
----------------
Ok, this isn't related to that review, but i still don't understand why this loop terminates. We might skip `void *` above, but we don't skip `void *****`, right? And if we go on dereferencing that, we'll eventually get to a `void *` that can point to anything.

A bit more of an intuition dump: I strongly believe that in `getSVal(Loc, T)` the optional argument `T` should be mandatory. Omitting it almost always leads to errors, because there are no types in machine memory, and our `RegionStore` tries to work exactly like machine memory, and `getSVal(Loc)` makes a best-effort guess on what the type of the region is, which in some cases may work reliably (these are exactly the cases where you don't mind passing the type directly), but is usually error-prone in other cases. So whenever i see a `getSVal(Loc)` without a type, i imagine that the code essentially reads raw bytes from the symbolic memory and makes random guesses on what they mean.

So the point is "oh we can do better with dynamic type info", it's "we're doing something completely unreliable here". It's not just about void pointers, it's about the fact that the underlying storage simply has no types at all.


================
Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:524
+    if (T->isMemberPointerType()) {
+      return isMemberPointerUninit(FR, LocalChain);
+    }
----------------
Why not just `isPrimitiveUninit()`? I believe that there shouldn't really be any difference, because member pointer types are kinda primitive. You can't really "dereference" a member pointer, at least not on its own, so i think there's no need to make an extra function here.


https://reviews.llvm.org/D48325