Page MenuHomePhabricator

Add Support for Generic Reference Counting Annotations in RetainCountChecker

Authored by malhar1995 on Jul 19 2017, 1:25 AM.



As part of my Google Summer of Code project, I am working on adding support for Integer Set Library (ISL) annotations to the current RetainCountChecker.

Note about ISL:
ISL has annotations isl_give and isl_take which are analogous to cf_returns_retained and cf_consumed but in case of ISL, annotations precede datatypes of function parameters.

So far, to build the ISL codebase using scan-build, I was #define-ing isl_give and isl_take to attribute((cf_returns_retained)) and attribute((cf_consumed)) respectively which resulted in the analyzer treating ISL objects as Core Foundation objects while performing reference counting.

This patch aims to add support for generic annotations (attribute(annotate("give"))) and attribute(annotate("take")))) in RetainCountChecker hence enabling the RetainCountChecker to perform reference counting for any codebase written in C.

Let me know your comments on the same.

Diff Detail


Event Timeline

malhar1995 created this revision.Jul 19 2017, 1:25 AM
dcoughlin edited edge metadata.Jul 19 2017, 7:31 AM

This looks pretty good. There are some minor comments in line.

One thing that is missing: tests for the new diagnostic text. Can you add new tests that specifically test for the new diagnostic text with // expected-warning {{ ... }}}. These should probably go in retain-release.m.

150 ↗(On Diff #107259)

I would suggest calling this just "Generalized" to avoid confusion with generics and because it might not be C (it could, for example, be C++).

1343 ↗(On Diff #107259)

I would suggest having the annotation be "rc_ownership_returns_retained". This is consistent with the CF and NS attribute names. I wouldn't be too concerned about this being verbose since users of the annotation in ISL will still use "__give".

1368 ↗(On Diff #107259)

I would suggest having the annotation be "rc_ownership_consumed".

NoQ added inline comments.Jul 19 2017, 7:51 AM
2012 ↗(On Diff #107259)

I don't think this can happen. Symbols always have a type, see isValidTypeForSymbol(). These branches can be removed from the surrounding code as well.

Addressed all comments except the one where I had to remove the test to see if the return type was null while emitting diagnostics.

malhar1995 marked 3 inline comments as done.Jul 19 2017, 11:36 AM
malhar1995 added inline comments.Jul 20 2017, 2:26 AM
2012 ↗(On Diff #107259)

So, you're saying Sym->getType() can never be NULL?
If so, do you want me to insert assert(isValidTypeForSymbol(Sym->getType())) instead?

NoQ added inline comments.Jul 20 2017, 2:29 AM
2012 ↗(On Diff #107259)

This assertion is already there in Sym's constructor (of the whole SymExpr class), and symbols are also immutable, so it's already verified and you can just rely on it and remove the branch in your if (and probably nearby ifs as well).

Removed the checks to see if the symbol type is NULL while printing diagnostics as they are unnecessary.

dcoughlin accepted this revision.Jul 25 2017, 9:38 AM

Sorry for the delay! This looks good to me.

We have a really embarrassing FIXMELATER from 2012 (!!!) that disabled the plist tests for diagnostics. This means we're not getting testing for the diagnostic text itself. Let's not block this patch on fixing that, though.

This revision is now accepted and ready to land.Jul 25 2017, 9:38 AM

I will commit.

This revision was automatically updated to reflect the committed changes.