This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] Make ObjCDeallocChecker path sensitive.
ClosedPublic

Authored by dcoughlin on Feb 22 2016, 8:53 AM.

Details

Summary

This patch converts the ObjCDeallocChecker to be path sensitive. The primary motivation for this change is to prevent false positives when -dealloc calls helper invalidation methods to release instance variables, but it additionally improves precision when -dealloc contains control flow. It also reduces the need for pattern matching. The check for missing -dealloc methods remains AST-based.

Diff Detail

Repository
rL LLVM

Event Timeline

dcoughlin updated this revision to Diff 48693.Feb 22 2016, 8:53 AM
dcoughlin retitled this revision from to [analyzer] Make ObjCDeallocChecker path sensitive..
dcoughlin updated this object.
dcoughlin added a reviewer: zaks.anna.
dcoughlin added a subscriber: ddkilzer.
dcoughlin updated this revision to Diff 48750.Feb 22 2016, 4:02 PM
dcoughlin added a subscriber: cfe-commits.

Provide diff with context. Apologies for leaving that out.

zaks.anna added inline comments.Feb 22 2016, 5:01 PM
lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp
89 ↗(On Diff #48693)

Let's not use these comments blocks. We should use oxygen style comments.

161 ↗(On Diff #48693)

Use doxygen throughout. Please, mention that this is an AST check.

184 ↗(On Diff #48693)

Looks like we do not use this for the other checker. Is that the correct behavior? Seems like it.

Maybe we should reflect this in the method name.

198 ↗(On Diff #48693)

We usually capitalize this.

249 ↗(On Diff #48693)

I do not understand this.

261 ↗(On Diff #48693)

I think it's worth adding "If we are in -dealloc or" ...

298 ↗(On Diff #48693)

Add: "We have to release before a call to [super dealloc]."

383 ↗(On Diff #48693)

Hopefully, this ensures that all symbols added in the function begin call are removed! The logic is slightly different than where we were adding these:

getContainingObjCImpl(LCtx)->property_impls()
418 ↗(On Diff #48693)

This might be too long.. Maybe we could remove "instance variable"?

526 ↗(On Diff #48693)

BugType names should be capitalized.

721 ↗(On Diff #48693)

We might need an annotation for this as well.

dcoughlin updated this revision to Diff 48995.Feb 24 2016, 5:44 PM
dcoughlin marked 8 inline comments as done.

This update addresses Anna's review comments. The big change is that the program state now maps instance symbols to sets of initial ivar symbols that must be released. (Rather than just storing the set, as before). With this approach we don't need to play the trick of explicitly binding the initial field symbols in the store to get notified that they escape when the instance escapes. Instead, we can look up the instance in the map when it escapes and remove the entire set.

I've also fixed a leak where we weren't removing values properly from the set that must be released and fixed a false positive so we no longer warn about a missing release when the field is known to be nil.

lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp
184 ↗(On Diff #48750)

I've changed the method name to 'classHasSeparateTeardown()'.

249 ↗(On Diff #48750)

I've removed this. It is not needed now that the program state is a map from instance symbols to sets of initial field values that must be released.

383 ↗(On Diff #48750)

I've added an assertion to catch when these aren't being removed. (You were right, they weren't always.)

zaks.anna accepted this revision.Feb 24 2016, 9:42 PM
zaks.anna edited edge metadata.
This revision is now accepted and ready to land.Feb 24 2016, 9:42 PM
ddkilzer added inline comments.Feb 25 2016, 8:19 AM
lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp
804 ↗(On Diff #48995)

I think you meant "/param InstanceValOut" here.

ddkilzer added inline comments.Feb 25 2016, 8:21 AM
lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp
804 ↗(On Diff #48995)

And I meant "\param InstanceValOut". 😐

dcoughlin added inline comments.Feb 25 2016, 9:50 AM
lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp
804 ↗(On Diff #48995)

Thanks! Will fix.

This revision was automatically updated to reflect the committed changes.