This is an archive of the discontinued LLVM Phabricator instance.

[analyzer][WIP] Scan the program state map in the visitor only once in DanglingInternalBufferChecker
Needs ReviewPublic

Authored by rnkovacs on Jul 19 2018, 2:45 PM.

Details

Summary

In order to avoid scanning the map at each node in the bug reporter visitor, the MemRegion representing the container object associated with the tracked pointer symbol is looked up once in the visitor constructor. However, at the point where the visitor is constructed, we no longer find the appropriate MemRegion entry in the map, as it has already been cleaned up.
In this patch, I removed some clean-ups to allow for the look-up, but I suspect this is not the right approach.

Diff Detail

Event Timeline

rnkovacs created this revision.Jul 19 2018, 2:45 PM

Yeah, I would rather have the cleanups and do extra work in the visitor. But lets wait what @NoQ thinks.

NoQ added a comment.Jul 19 2018, 6:41 PM

Oh, this FIXME, i almost forgot about that. Not sure if we should focus on this now because it's kinda premature optimization, especially after @george.karpenkov has fixed a large performance problem that caused VisitNode to be called like ~30 times more often than necessary (D47856), so now our visitors are very fast. But i totally agree that it is aesthetically unpleasant to leave this code in that shape.

Also, yeah, if i recall correctly i had something different in mind that doesn't loosen our program state cleanup. Trying to reconstruct what i was probably thinking, i can imagine one efficient strategy that unfortunately ties the two checkers (your checker and MallocChecker) to each other even more. The strategy is to perform the lookup that finds the region not against the leaf of the path, but against the point in which the pointer is marked as released, because that's one easy-to-identify point in which the container is certainly present and has certainly not been cleaned up. In order to find such point, we can consult MallocBugVisitor::isReleased(), which is currently essentially a static function (even if not marked as such) but it should be modified to accept not RefStates but a symbol and an actual state (i.e. something that's not a MallocChecker's private thing).