Page MenuHomePhabricator

[analyzer] Clean up the program state map of DanglingInternalBufferChecker

Authored by rnkovacs on May 26 2018, 11:54 AM.



Symbols are cleaned up from the program state map when they go out of scope.
Memory regions are cleaned up when the corresponding object is destroyed, and
additionally in checkDeadSymbols in case destructor modeling was incomplete.

Diff Detail


Event Timeline

rnkovacs created this revision.May 26 2018, 11:54 AM
This revision is now accepted and ready to land.May 26 2018, 12:01 PM
NoQ added a comment.May 27 2018, 11:22 PM

Yep, great stuff!

I guess, please add a comment on incomplete destructor support in the CFG and/or analyzer core that may cause the region to be never cleaned up. Or perform an extra cleanup pass - not sure if it's worth it, but it should be easy and safe.

94 ↗(On Diff #148733)

It's kinda weird that something that's not a const MemRegion * is called "Region".

I'd rather use a neutral term like "Item".

rnkovacs updated this revision to Diff 150625.Jun 9 2018, 7:26 AM
rnkovacs marked an inline comment as done.
rnkovacs edited the summary of this revision. (Show Details)

Fixed naming and added an extra pass for regions left behind by incomplete destructors.

NoQ added inline comments.Jun 9 2018, 11:30 AM
92–94 ↗(On Diff #150625)

This optimization is in fact incorrect due to an old bug that i didn't yet get to fixing: D18860. The proposed patch would most likely remove the check anyway, because the set of dead symbols is not well-defined. So i think we shouldn't add it.

98 ↗(On Diff #150625)

For the same reason (D18860), it should be more correct to use !isLive(). Otherwise you may miss some symbols.

100–103 ↗(On Diff #150625)

I mildly advocate for braces here because that's as many as three lines to wrap.

rnkovacs updated this revision to Diff 150637.Jun 9 2018, 1:57 PM
rnkovacs marked 3 inline comments as done.

Addressed comments.

NoQ accepted this revision.Jun 9 2018, 2:08 PM

Great, thanks!

This revision was automatically updated to reflect the committed changes.