[analyzer][UninitializedObjectChecker] Support for MemberPointerTypes

[Szelethus]

Accidently added the pointer MemberPointer objects twice, fixed that.

[Szelethus]

Polite ping :)

[george.karpenkov]

Requesting changes or clarification on uninitialized references.

lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp

392

I am confused here, how can references be uninitialized?

393

Same question here (though that's for a different commit) -- I'm confused on how a reference can be uninitialized after the constructor call has finished.

[NoQ]

Looks good! One minor comment.

lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp

488–491

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.

524

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.

[NoQ]

lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp

392

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.

[Szelethus]

lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp

392

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.

488–491

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 :)

[Szelethus]

lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp

488–491

Also note that there are tests in cxx-uninitialized-object-ptr-ref.cpp for void*, void*&, void**, and void**&&.

[NoQ]

lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp

392

Aha, oh, i see, you just reordered the ifs and it seemed as if you're suddenly doing new stuff with references, sorry><

488–491

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.

[Szelethus]

MemberPointerTypes are regarded as primitive types from now. I agree with @NoQ, it makes so much more sense this way :)

[NoQ]

Yay less code.

[Szelethus]

Thank you! ^-^

Rebased to rC336901.

lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp

   //     - a non-union record
   //     - a pointer/reference
   //     - an array
-  //     - of a member pointer type
-  //     - of a primitive type, which we'll define as either a BuiltinType or
-  //       EnumeralType.
+  //     - of a primitive type, which we'll define later in a helper function.
   //   * the parent of each node is the object that contains it
-  //   * every leaf is an array, a primitive object, a member pointer, a nullptr
-  //     or an undefined pointer.
+  //   * every leaf is an array, a primitive object, a nullptr or an undefined
+  //   pointer.
   //
   // Example:
   //

   //
   // From this we'll construct a vector of fieldchains, where each fieldchain
   // represents an uninitialized field. An uninitialized field may be a
-  // primitive object, a member pointer, a pointer, a pointee or a union without
-  // a single initialized field.
+  // primitive object, a pointer, a pointee or a union without a single
+  // initialized field.
   // In the above example, for the default constructor call we'll end up with
   // these fieldchains:
   //

   bool isPointerOrReferenceUninit(const FieldRegion *FR,
                                   FieldChainInfo LocalChain);
 
-  /// This method checks a region of MemberPointerType, and returns true if the
-  /// the pointer is uninitialized.
-  bool isMemberPointerUninit(const FieldRegion *FR, FieldChainInfo LocalChain);
-
   /// This method returns true if the value of a primitive object is
   /// uninitialized.
   bool isPrimitiveUninit(const SVal &V);

 /// known, and thus FD can not be analyzed.
 bool isVoidPointer(const FieldDecl *FD);
 
-/// Returns true if T is a primitive type. We'll call a type primitive if it's
-/// either a BuiltinType or an EnumeralType.
+/// Returns true if T is a primitive type. We defined this type so that for
+/// objects that we'd only like analyze as much as checking whether their
+/// value is undefined or not, such as ints and doubles, can be analyzed with
+/// ease. This also helps ensuring that every special field type is handled
+/// correctly.
 bool isPrimitiveType(const QualType &T) {
-  return T->isBuiltinType() || T->isEnumeralType();
+  return T->isBuiltinType() || T->isEnumeralType() || T->isMemberPointerType();
 }
 
 /// Constructs a note message for a given FieldChainInfo object.

       continue;
     }
 
-    if (T->isMemberPointerType()) {
-      if (isMemberPointerUninit(FR, LocalChain))
-        ContainsUninitField = true;
-      continue;
-    }
-
-    // If this is a pointer or reference type.
     if (T->isPointerType() || T->isReferenceType()) {
       if (isPointerOrReferenceUninit(FR, LocalChain))
         ContainsUninitField = true;

   return false;
 }
 
-bool FindUninitializedFields::isMemberPointerUninit(const FieldRegion *FR,
-                                                    FieldChainInfo LocalChain) {
-  assert(FR->getDecl()->getType()->isMemberPointerType() &&
-         "This function only checks regions that hold MemberPointerTypes!");
-  // TODO: Implement support for MemberPointerTypes.
-  return false;
-}
-
 bool FindUninitializedFields::isPrimitiveUninit(const SVal &V) {
   if (V.isUndef())
     return true;

test/Analysis/cxx-uninitialized-object-ptr-ref.cpp

 
 #ifdef PEDANTIC
 struct PointerToMemberFunctionTest1 {
-  // TODO: we'd expect the note {{uninitialized field 'this->f'}}
-  void (UsefulFunctions::*f)(void); // no-note
+  void (UsefulFunctions::*f)(void); // expected-note{{uninitialized field 'this->f'}}
   PointerToMemberFunctionTest1() {}
 };
 
 void fPointerToMemberFunctionTest1() {
-  // TODO: we'd expect the warning {{1 uninitialized field}}
-  PointerToMemberFunctionTest1(); // no-warning
+  PointerToMemberFunctionTest1(); // expected-warning{{1 uninitialized field}}
 }
 
 struct PointerToMemberFunctionTest2 {

 }
 
 struct PointerToMemberDataTest1 {
-  // TODO: we'd expect the note {{uninitialized field 'this->f'}}
-  int UsefulFunctions::*d; // no-note
+  int UsefulFunctions::*d; // expected-note{{uninitialized field 'this->d'}}
   PointerToMemberDataTest1() {}
 };
 
 void fPointerToMemberDataTest1() {
-  // TODO: we'd expect the warning {{1 uninitialized field}}
-  PointerToMemberDataTest1(); // no-warning
+  PointerToMemberDataTest1(); // expected-warning{{1 uninitialized field}}
 }
 
 struct PointerToMemberDataTest2 {