Add Support for Generic Reference Counting Annotations in RetainCountChecker

[malhar1995]

Summary

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.

[dcoughlin]

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.

include/clang/StaticAnalyzer/Checkers/ObjCRetainCount.h

150

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++).

lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp

1343

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

I would suggest having the annotation be "rc_ownership_consumed".

[NoQ]

lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp

2012

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.

[malhar1995]

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]

lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp

2012

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

[NoQ]

lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp

2012

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).

[malhar1995]

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

[malhar1995]

@dcoughlin Ping.

[dcoughlin]

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.

[dcoughlin]

I will commit.

include/clang/StaticAnalyzer/Checkers/ObjCRetainCount.h

public:
   /// Determines the object kind of a tracked object.
   enum ObjKind {
     /// Indicates that the tracked object is a CF object.  This is
     /// important between GC and non-GC code.
     CF,
     /// Indicates that the tracked object is an Objective-C object.
     ObjC,
     /// Indicates that the tracked object could be a CF or Objective-C object.
-    AnyObj
+    AnyObj,
+    /// Indicates that the tracked object is a generic C object.
+    GenericC
   };
-  
 private:
   Kind K;
   ObjKind O;
   
   RetEffect(Kind k, ObjKind o = AnyObj) : K(k), O(o) {}
   
 public:
   Kind getKind() const { return K; }

lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp

if (D->hasAttr<NSReturnsNotRetainedAttr>() ||
       return RetEffect::MakeNotOwned(RetEffect::ObjC);
 
   } else if (!RetTy->isPointerType()) {
     return None;
   }
 
   if (D->hasAttr<CFReturnsRetainedAttr>())
     return RetEffect::MakeOwned(RetEffect::CF);
+  else if (hasRCAnnotation(D, "give"))
+    return RetEffect::MakeOwned(RetEffect::GenericC);
 
   if (D->hasAttr<CFReturnsNotRetainedAttr>())
     return RetEffect::MakeNotOwned(RetEffect::CF);
 
   return None;
 }
 
 void
 RetainSummaryManager::updateSummaryFromAnnotations(const RetainSummary *&Summ,
                                                    const FunctionDecl *FD) {
   if (!FD)
     return;
 
   assert(Summ && "Must have a summary to add annotations to.");
   RetainSummaryTemplate Template(Summ, *this);
 
   // Effects on the parameters.
   unsigned parm_idx = 0;
   for (FunctionDecl::param_const_iterator pi = FD->param_begin(),
          pe = FD->param_end(); pi != pe; ++pi, ++parm_idx) {
     const ParmVarDecl *pd = *pi;
     if (pd->hasAttr<NSConsumedAttr>())
       Template->addArg(AF, parm_idx, DecRefMsg);
-    else if (pd->hasAttr<CFConsumedAttr>())
+    else if (pd->hasAttr<CFConsumedAttr>() || hasRCAnnotation(pd, "take"))
       Template->addArg(AF, parm_idx, DecRef);
-    else if (pd->hasAttr<CFReturnsRetainedAttr>()) {
+    else if (pd->hasAttr<CFReturnsRetainedAttr>() ||
+             hasRCAnnotation(pd, "give")) {
       QualType PointeeTy = pd->getType()->getPointeeType();
       if (!PointeeTy.isNull())
         if (coreFoundation::isCFObjectRef(PointeeTy))
           Template->addArg(AF, parm_idx, RetainedOutParameter);
     } else if (pd->hasAttr<CFReturnsNotRetainedAttr>()) {
       QualType PointeeTy = pd->getType()->getPointeeType();
       if (!PointeeTy.isNull())
         if (coreFoundation::isCFObjectRef(PointeeTy))
else {
 
       if (CurrV.getObjKind() == RetEffect::CF) {
         if (Sym->getType().isNull()) {
           os << " returns a Core Foundation object with a ";
         } else {
           os << " returns a Core Foundation object of type "
              << Sym->getType().getAsString() << " with a ";
         }
+      } else if (CurrV.getObjKind() == RetEffect::GenericC) {
+        if (Sym->getType().isNull()) {
+          os << " returns an object with a ";
+        } else {
+          os << " returns an object of type " << Sym->getType().getAsString()
+             << " with a ";
         }
-      else {
+      } else {
         assert (CurrV.getObjKind() == RetEffect::ObjC);
         QualType T = Sym->getType();
         if (T.isNull() || !isa<ObjCObjectPointerType>(T)) {
           os << " returns an Objective-C object with a ";
         } else {
           const ObjCObjectPointerType *PT = cast<ObjCObjectPointerType>(T);
           os << " returns an instance of "
              << PT->getPointeeType().getAsString() << " with a ";

test/Analysis/retain-release-inline.m

void test_neg() {
   NSString *s = [[NSString alloc] init]; // no-warning  
   foo(s);
   foo(s);
   bar(s);
   bar(s);
   bar(s);
 }
 
-__attribute__((cf_returns_retained)) isl_basic_map *isl_basic_map_cow(__attribute__((cf_consumed)) isl_basic_map *bmap);
+__attribute__((annotate("give"))) isl_basic_map *isl_basic_map_cow(__attribute__((annotate("take"))) isl_basic_map *bmap);
 void free(void *);
 
 // As 'isl_basic_map_free' is annotated with 'rc_ownership_trusted_implementation', RetainCountChecker trusts its
 // implementation and doesn't analyze its body. If the annotation 'rc_ownership_trusted_implementation' is removed,
 // a leak warning is raised by RetainCountChecker as the analyzer is unable to detect a decrement in the reference
 // count of 'bmap' along the path in 'isl_basic_map_free' assuming the predicate of the second 'if' branch to be
 // true or assuming both the predicates in the function to be false.
-__attribute__((annotate("rc_ownership_trusted_implementation"))) isl_basic_map *isl_basic_map_free(__attribute__((cf_consumed)) isl_basic_map *bmap) {
+__attribute__((annotate("rc_ownership_trusted_implementation"))) isl_basic_map *isl_basic_map_free(__attribute__((annotate("take"))) isl_basic_map *bmap) {
   if (!bmap)
     return NULL;
 
   if (--bmap->ref > 0)
     return NULL;
 
   free(bmap);
   return NULL;
 }
 
 // As 'isl_basic_map_copy' is annotated with 'rc_ownership_trusted_implementation', RetainCountChecker trusts its
 // implementation and doesn't analyze its body. If that annotation is removed, a 'use-after-release' warning might
 // be raised by RetainCountChecker as the pointer which is passed as an argument to this function and the pointer
 // which is returned from the function point to the same memory location.
-__attribute__((annotate("rc_ownership_trusted_implementation"))) __attribute__((cf_returns_retained)) isl_basic_map *isl_basic_map_copy(isl_basic_map *bmap) {
+__attribute__((annotate("rc_ownership_trusted_implementation"))) __attribute__((annotate("give"))) isl_basic_map *isl_basic_map_copy(isl_basic_map *bmap) {
   if (!bmap)
     return NULL;
 
   bmap->ref++;
   return bmap;
 }
 
-void test_use_after_release_with_trusted_implementation_annotate_attribute(__attribute__((cf_consumed)) isl_basic_map *bmap) {
+void test_use_after_release_with_trusted_implementation_annotate_attribute(__attribute__((annotate("take"))) isl_basic_map *bmap) {
   // After this call, 'bmap' has a +1 reference count.
   bmap = isl_basic_map_cow(bmap);
   // After the call to 'isl_basic_map_copy', 'bmap' has a +1 reference count.
   isl_basic_map *temp = isl_basic_map_cow(isl_basic_map_copy(bmap));
   // After this call, 'bmap' has a +0 reference count.
   isl_basic_map *temp2 = isl_basic_map_cow(bmap); // no-warning
   isl_basic_map_free(temp2);
   isl_basic_map_free(temp);
 }
 
-void test_leak_with_trusted_implementation_annotate_attribute(__attribute__((cf_consumed)) isl_basic_map *bmap) {
+void test_leak_with_trusted_implementation_annotate_attribute(__attribute__((annotate("take"))) isl_basic_map *bmap) {
   // After this call, 'bmap' has a +1 reference count.
   bmap = isl_basic_map_cow(bmap); // no-warning
   // After this call, 'bmap' has a +0 reference count.
   isl_basic_map_free(bmap);
 }
 
 //===----------------------------------------------------------------------===//
 // Test returning retained and not-retained values.