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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ 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))
----------------
Szelethus wrote:
> 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`.
Aha, oh, i see, you just reordered the `if`s and it seemed as if you're suddenly doing new stuff with references, sorry><


================
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:
> 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**&&`.
Aha, yeah, i see, got it, yeah.

I think the way this should work is you take the type of the field `FR`, unwrap it to the pointee type as long as it is a pointer type, and dereference the pointer with that unwrapped type every time you unwrap it.

This would terminate because you'll eventually get to the leaf type. Values loaded from the store would also match types that the program would see if it tried to dereference the pointer that many times.


https://reviews.llvm.org/D48325