This is an archive of the discontinued LLVM Phabricator instance.

[analyzer][UninitializedObjectChecker] Support for MemberPointerTypes
ClosedPublic

Authored by Szelethus on Jun 19 2018, 9:05 AM.

Diff Detail

Event Timeline

Szelethus created this revision.Jun 19 2018, 9:05 AM

Accidently added the pointer MemberPointer objects twice, fixed that.

Polite ping :)

george.karpenkov requested changes to this revision.Jun 26 2018, 5:59 PM

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.

This revision now requires changes to proceed.Jun 26 2018, 5:59 PM
NoQ accepted this revision.Jun 26 2018, 6:00 PM

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 added inline comments.Jun 26 2018, 6:01 PM
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 added inline comments.Jun 27 2018, 4:25 AM
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 added inline comments.Jun 27 2018, 7:09 AM
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 added inline comments.Jun 27 2018, 10:27 AM
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 updated this revision to Diff 154398.Jul 6 2018, 6:45 AM

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

Szelethus marked 10 inline comments as done.Jul 6 2018, 6:46 AM
NoQ accepted this revision.Jul 12 2018, 12:24 PM

Yay less code.

Szelethus updated this revision to Diff 155356.Jul 13 2018, 5:24 AM

Thank you! ^-^

Rebased to rC336901.

This revision was not accepted when it landed; it landed in state Needs Review.Jul 13 2018, 5:26 AM
This revision was automatically updated to reflect the committed changes.