This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] MisusedMovedObjectChecker: Fix false positive on state-resetting, handling method calls on base-class sub-objects
ClosedPublic

Authored by szepet on Oct 8 2017, 9:32 AM.

Details

Summary
  1. Artem's solution D31538 solves the reset problem, however, the reports should be handled the same way in case of method calls. We should not just report the base class of the object where the method was defined but the whole object.
  1. Fixed false positive which came from not removing the subobjects in case of a state-resetting function. (Just replaced the State->remove(...) call to removeFromState(..) which was defined exactly for that purpose.)
  1. Some minor typos fixed in this patch as well which did not worth a whole new patch in my opinion, so included them here.

Diff Detail

Event Timeline

szepet created this revision.Oct 8 2017, 9:32 AM
szepet edited reviewers, added: NoQ; removed: dergachev.a.Oct 8 2017, 9:36 AM
xazax.hun accepted this revision.Oct 9 2017, 6:38 AM

LGTM! I only found a nit.

lib/StaticAnalyzer/Checkers/MisusedMovedObjectChecker.cpp
419

I think this loop could be moved after the check isMoveSafeMethod to improve the performance.

This revision is now accepted and ready to land.Oct 9 2017, 6:38 AM
NoQ added inline comments.Oct 10 2017, 6:21 AM
lib/StaticAnalyzer/Checkers/MisusedMovedObjectChecker.cpp
380–383

I guess we'd have to check this for null, eg. when this-value is Unknown (seeing crashes on dyn_casting this pointer to CXXBaseObjectRegion in the code you just moved).

xazax.hun added inline comments.Oct 10 2017, 6:22 AM
lib/StaticAnalyzer/Checkers/MisusedMovedObjectChecker.cpp
380–383

Good catch. I can confirm that this can return a null pointer.

szepet updated this revision to Diff 119033.Oct 14 2017, 10:20 AM

Fixed the commented bug (check if ThisRegion is null).

szepet marked 2 inline comments as done.Oct 14 2017, 10:20 AM