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

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus added inline comments.


================
Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:392
 
-    if (T->isMemberPointerType()) {
-      if (isMemberPointerUninit(FR, LocalChain))
+    if (T->isPointerType() || T->isReferenceType()) {
+      if (isPointerOrReferenceUninit(FR, LocalChain))
----------------
NoQ wrote:
> george.karpenkov wrote:
> > I am confused here, how can references be uninitialized?
> Hmm, that's actually a good question. I guess we technically initialize a reference with an undefined pointer, but that should be caught by another checker early because it's an immediate UB.
> 
> In any case, there don't seem to be tests for this.
I guess you could say that naming could be improved upon. I didn't mean that the reference object itself, but rather it's pointee is uninitialized.

```
int a; // say that this isn't zero initialized
int &c = a;
```

There are numerous tests for this both for left value and right value references in `cxx-uninitialized-object-ptr-ref.cpp`.


================
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);
   }
----------------
NoQ wrote:
> 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.
>From the code a couple lines back:

  if (isVoidPointer(FD)) {
    IsAnyFieldInitialized = true;
    return false;
  }

Note that isn't `Type::isVoidPointer`, I'm calling a statically defined function to filter out all void pointer cases.
```
/// Returns whether FD 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.
bool isVoidPointer(const FieldDecl *FD);
```

I see your point, this is a bit dodgy. I'm already working on a fix, and hope to upload it along with other important fixes during the next week. It's high on my priority list :)


https://reviews.llvm.org/D48325