Szelethus marked 28 inline comments as done.
Szelethus added inline comments.
================
Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:181-182
+/// 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);
----------------
NoQ wrote:
> Type of an object pointed to by a void pointer is something that our
> path-sensitive engine is sometimes able to provide. It is not uncommon that a
> void pointer points to a concrete variable on the current path, and we may
> know it in the analyzer. I'm not sure this sort of check is necessary (i.e.
> require more information).
>
> You may also want to have a look at `getDynamicTypeInfo()` which is sometimes
> capable of retrieving the dynamic type of the object pointed to by a pointer
> or a reference - i.e. even if it's different from the pointee type of the
> pointer.
I took a look at it, but I found that it'd change the implementation of
`isPointerOrReferenceUninit` so much, I'd be a lot more comfortable if I could
implement it in a smaller followup patch. I placed some `TODO`s in the code
about this.
================
Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:223-225
+ ExplodedNode *Node = Context.generateNonFatalErrorNode(Context.getState());
+ if (!Node)
+ return;
----------------
NoQ wrote:
> I suspect that a fatal error is better here. We don't want the user to
> receive duplicate report from other checkers that catch uninitialized values;
> just one report is enough.
I think that would be a bad idea. For example, this checker shouts so loudly
while checking the LLVM project, it would practically halt the analysis of the
code, reducing the coverage, which means that checkers other then uninit value
checkers would "suffer" from it.
However, I also think that having multiple uninit reports for the same object
might be good, especially with this checker, as it would be very easy to see
where the problem originated from.
What do you think?
================
Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:405-406
+ SVal DerefdV = StoreMgr.getBinding(S, V.castAs<Loc>());
+ while (Optional<Loc> L = DerefdV.getAs<Loc>())
+ DerefdV = StoreMgr.getBinding(S, *L);
+
----------------
NoQ wrote:
> Is this loop guaranteed to terminate? Is something like `void *v; v = &v;`
> possible?
>
> I think this loop should unwrap the AST type on every iteration and
> dereference the pointer accordingly.
>
> In general, i believe that every symbolic pointer dereference should be made
> with awareness of the type of the object we're trying to retrieve. Which is
> an optional argument for `State->getSVal(R, ...)`, but it seems that it
> should be made mandatory because in a lot of cases omitting it causes
> problems.
> Is this loop guaranteed to terminate? Is something like void *v; v = &v;
> possible?
I looked this up, and I am confident that it is not possible for a pointer to
point to itself. Only a `void**` object may point to a `void*`. The loop
terminates even if you do something evil like `v =
reinterpret_cast<void*>(&v);` (I have tried this with `int`s).
>I think this loop should unwrap the AST type on every iteration and
>dereference the pointer accordingly.
>
>In general, i believe that every symbolic pointer dereference should be made
>with awareness of the type of the object we're trying to retrieve. Which is an
>optional argument for State->getSVal(R, ...), but it seems that it should be
>made mandatory because in a lot of cases omitting it causes problems.
I invested some serious effort into `getDynamicType`, but I think it'll require
more time and some serious refactoring of this (`isPointerOrReferenceUninit`)
function, so I'd post it separately.
================
Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:432
+ if (isUnionUninit(R)) {
+ LocalChain.dereference();
+ return addFieldToUninits(std::move(LocalChain));
----------------
NoQ wrote:
> Needs a comment on why the `IsDereferenced` flag on this path.
Added a comment at the line where I dereference the field that after that point
we'll check the pointee.
================
Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:559-560
+ while (LC) {
+ if (isa<CXXConstructorDecl>(LC->getDecl()))
+ return true;
+
----------------
NoQ wrote:
> This doesn't check that the constructor on the parent stack frame is anyhow
> related to the current constructor, so it may introduce false negatives. For
> instance, a class may lazy-initialize a singleton but never store a reference
> to it within itself (because, well, it's a singleton, you can always obtain
> the reference to it). In this case we'll never find the bug in the
> constructor of the singleton.
Added a TODO and a test case to highlight this issue, and I'm planning to fix
it in a followup patch.
https://reviews.llvm.org/D45532
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
