[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:488-491
   // TODO: Dereferencing should be done according to the dynamic type.
   while (Optional<Loc> L = DerefdV.getAs<Loc>()) {
     DerefdV = State->getSVal(*L);
   }
----------------
Szelethus wrote:
> 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 :)
Also note that there are tests in `cxx-uninitialized-object-ptr-ref.cpp` for `void*`, `void*&`, `void**`, and `void**&&`.


https://reviews.llvm.org/D48325