Page MenuHomePhabricator

Add Support for Reference Counting of Parameters on the Callee Side in RetainCountChecker

Authored by malhar1995 on Aug 7 2017, 9:49 PM.



Current RetainCountChecker performs reference counting of function arguments/parameters only on the caller side and not on the callee side.

This patch aims to add support for reference counting on the callee-side for objects of 'Generalized' ObjKind.

This patch is still a work in progress.

Diff Detail


Event Timeline

malhar1995 created this revision.Aug 7 2017, 9:49 PM
malhar1995 added inline comments.Aug 7 2017, 10:02 PM
2521–2523 ↗(On Diff #110132)

I'm not sure what difference it will make if I change the ordering of the invocations of deriveAllocLocation and 'deriveParamLocation`.

2683 ↗(On Diff #110132)

This might be used in the future in case callee side parameter checking is added for Core Foundation and Objective-C objects.

3960–3971 ↗(On Diff #110132)

Getting function summary and checking for ArgEffects doesn't seem like an option to me as currently there is no way to differentiate between Core Foundation and Generalized objects just by looking at their ArgEffects.

However, this loop results in check-clang-analysis failure in retain-release.m on lines 1140 and 2237.

dcoughlin added inline comments.Aug 7 2017, 10:26 PM
3960–3971 ↗(On Diff #110132)

You can differentiate based on the Type of the parameter and use ArgEffects to determine what the initial ref binding should be. I'd like there to be a single point of truth about what it means for a parameter to annotated.

dcoughlin edited edge metadata.Aug 8 2017, 8:01 PM

Nice work! This looks good, with some nits and testing comments inline. Have you run this on real code? What kind of false positives do you see?

1853 ↗(On Diff #110132)

Let's dispense with tradition and add some doxygen comments describing what these methods do.

2442 ↗(On Diff #110132)

LLVM style says to use auto * here rather than repeating the name of the type twice.

2479 ↗(On Diff #110132)

If you don't want this assertion, you should remove it rather than comment it out.

2524 ↗(On Diff #110132)

I'm worried that in the future it would be possible to reach createDescription without the Location, UniqueingLocation, and UniqueingDecl being filled in. Can you please add an assertion for this in createDescription().

2683 ↗(On Diff #110132)

Let's not add this if it is not used in the code.. You should either use this to control the checking for your generalized case or remove it.

305 ↗(On Diff #110132)

Please include the entire diagnostic text in the expected-warning{{}}. This will guarantee that future changes to the checker don't change it unexpectedly.

306 ↗(On Diff #110132)

I'd like to see some more tests. For example, you should get a warning if you set the parameter to something else; also if you return it.

Do you have a test checking for that you don't warn when a consumed parameter is correctly released?

malhar1995 updated this revision to Diff 110514.Aug 9 2017, 8:07 PM
malhar1995 marked 7 inline comments as done.

Addressed comments.

P.S: I'm yet to test this callee-side parameter checking functionality on the actual ISL codebase. I'll do that ASAP.

Thanks! This looks good to me, with the note about factoring out the duplicated logic addressed, Would you factor out that logic into a static function and update phabricator summary to be a commit message. Then I will commit!

3971 ↗(On Diff #110514)

Can you factor out the logic that looks whether the type name starts with "isl_" to a static function that types a type and is called something 'isGeneralizedObjectRef"? This will avoid duplicating the logic twice here and will also give us a single place to update when we add an attribute indicating that a type is reference counted.

This patch adds the functionality of performing reference counting on the callee side for Integer Set Library (ISL) to Clang Static Analyzer's RetainCountChecker.

Reference counting on the callee side can be extensively used to perform debugging within a function (For example: Finding leaks on error paths).

This revision was automatically updated to reflect the committed changes.

Thanks Malhar! Committed in r311063.