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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
