Suppressing Reference Counting Diagnostics for Functions Containing 'rc_ownership_trusted_implementation' Annotate Attribute

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

Initially, I built the ISL codebase with scan-build only to find a lot of false positives being raised by it. Majority of the false positives were due to the RetainCountChecker analyzing the bodies of the functions which perform reference counting.
Hence, to prevent such false positives, Dr. Devin Coughlin gave me a task to suppress them by adding some "trusted" annotation to such reference counting ISL functions.

The attached diff aims to do that by not allowing the RetainCountChecker to analyze a function's body if it has 'rc_ownership_trusted_implementation' annotate attribute.

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.

Let me know your thoughts on the same.

[dcoughlin]

This looks good! You will need to add tests though. I would suggest adding them to "test/Analysis/retain-release-inline.m"

lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp

1898

Naming-wise, I think it is probably better to describe the high-level semantics of this function rather than how it is intended to be used. I would suggest something like "isTrustedReferenceCountImplementation" for this function instead.

Also, let's break with tradition and add a doxygen style comment describing what the function does.

[malhar1995]

Changed function name from 'isAnnotatedToSkipDiagnostics' to 'isTrustedReferenceCountImplementation'.
Added some test-cases to test/Analysis/retain-release-inline.m.
Applied clang-format to the changed code.

[dcoughlin]

Thanks for the tests.

Have you tried this on the ISL codebase to make sure it is suppressing the diagnostics in related to reference counting implementation that you expect?

I think it would be good to add some tests that reflect the reference counting implementation patterns in ISL that you want to make sure not to warn on. Can you simplify those patterns to their essence and add them as tests?

test/Analysis/retain-release-inline.m

303

What is the purpose of this test? Does it test new functionality added by your patch?

312

Can you simplify this test so that it directly tests the new functionality you added? In general we try to avoid duplicating unnecessary parts of other tests. This slows down running the tests and also adds a maintenance burden when we need to change the test.

313

I think it would be better to use C here rather than Objective-C. Can you change the test to use new prototypes for the specific purpose of this test. One that is annotated "attribute((cf_returns_retained))" (this is similar to isl_give) and one with a parameter that is that is annotated "attribute__((cf_consumed))".

The reason it doesn't make sense to use Objective-C here because we want this new annotation to be used for C frameworks with their own reference counting implementation -- and Objective-C's is built into the language.

[malhar1995]

Added relevant test-cases to verify the added functionality.

[dcoughlin]

Thanks for the tests.

Have you tried this on the ISL codebase to make sure it is suppressing the diagnostics in related to reference counting implementation that you expect?

I think it would be good to add some tests that reflect the reference counting implementation patterns in ISL that you want to make sure not to warn on. Can you simplify those patterns to their essence and add them as tests?

[malhar1995]

In D34937#799504, @dcoughlin wrote:

Thanks for the tests.

Have you tried this on the ISL codebase to make sure it is suppressing the diagnostics in related to reference counting implementation that you expect?

I think it would be good to add some tests that reflect the reference counting implementation patterns in ISL that you want to make sure not to warn on. Can you simplify those patterns to their essence and add them as tests?

Although, I have not tried running the new RetainCountChecker on the entire ISL codebase, I ran it on small chunks of it and it seemed to be working as expected. Also, the test-cases that I have added in my latest patch are indeed representative of the reference counting implementation in ISL. Ideally, in case of ISL, we don't want the analyzer to analyze the bodies of functions of the type obj_free(), obj_cow(), etc. as they result in a large number of 'leak' false positives but adding 'rc_ownership_trusted_implementation' annotate attribute to just these functions will not do the trick as when warnings are raised, these functions are no longer present on the call-stack.

Also, I just realized that I made a mistake with one of the two test-cases that I added. I'll fix that ASAP.

[malhar1995]

Corrected one of the two test-cases added in the last-diff.

[malhar1995]

Suppresses false positives involving functions which perform reference counting.
Added relevant test-cases to test/Analysis/retain-release-inline.m

[malhar1995]

Checked for "trusted" annotation on the function declaration corresponding to the function definition.

[dcoughlin]

Looks good to me, other than a nit on indentation and a request for a little bit more info in a comment!

lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp

3403

Can you add a little bit more comment here to describe what the checker will do if the function has the attribute? Some like "... if so, we won't inline it."

3418

Nit: indentation is off here. It should be two spaces.

[malhar1995]

In D34937#811500, @dcoughlin wrote:

Looks good to me, other than a nit on indentation and a request for a little bit more info in a comment!

Currently, I have added support for generic annotations for isl_take and isl_give and I have also added a generic diagnostic note when the function returns an object which is neither Core-Foundation nor Objective-C.
For emitting a generic diagnostic note, I have added an object kind to the enum ObjKind in ObjCRetainCount.h.

Now, for adding the aforementioned support, I have made some changes to the patch which I last submitted.

So, should I submit another patch on the same revision after modifying the title, summary, etc. or should I create another revision for that?

[malhar1995]

Addressed comments.
Changed the function from isTrustedReferenceCountImplementation() to hasRCAnnotation() (I'm unable to come up with a better name) as this will be useful when I add support to RetainCountChecker to check for generic annotations corresponding to isl_give(cf_returns_retained) and isl_take(cf_consumed).

[dcoughlin]

lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp

3419

I'd like you to keep a call to "isTrustedReferenceCountImplementation()". The name of the function indicates what it means (rather than how it does it), which will be important when/if we change how annotations are implemented (for example, away from using the 'annotation' attribute to a custom attribute).

Using common code to check for annotations makes a lot sense though -- can you call hasRCAnnotation() from inside isTrustedReferenceCountImplementation()?

[dcoughlin]

So, should I submit another patch on the same revision after modifying the title, summary, etc. or should I create another revision for that?

You should create another revision. You can mark it as dependent on this one in Phabricator.

[malhar1995]

Added isTrustedReferenceCountAnnotation() again.

[dcoughlin]

LGTM! One thing I should have noted in a prior review is that the helper functions should be declared 'static' so that we don't have a public symbol for them. Making them static will prevent a linker error if some other part of clang declares a function with the same signature. This is unlikely, but we try to keep things clean.

I will update the two functions to be static and commit.

[dcoughlin]

Committed in r308416.

lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp

 static bool isSynthesizedAccessor(const StackFrameContext *SFC) {
   auto Method = dyn_cast_or_null<ObjCMethodDecl>(SFC->getDecl());
   if (!Method || !Method->isPropertyAccessor())
     return false;
 
   return SFC->getAnalysisDeclContext()->isBodyAutosynthesized();
 }
 
+/// Returns true if any declaration on the call-stack contains
+/// 'rc_ownership_trusted_implementation' annotate attribute.
+bool isTrustedReferenceCountImplementation(const LocationContext *LCtx) {
+  while (LCtx) {
+    if (const StackFrameContext *SFC = dyn_cast<StackFrameContext>(LCtx)) {
+      const Decl *D = SFC->getDecl();
+      for (const auto *Ann : D->specific_attrs<AnnotateAttr>()) {
+        if (Ann->getAnnotation() == "rc_ownership_trusted_implementation") {
+          return true;
+        }
+      }
+    }
+    LCtx = LCtx->getParent();
+  }
+  return false;
+}
+
 std::shared_ptr<PathDiagnosticPiece>
 CFRefReportVisitor::VisitNode(const ExplodedNode *N, const ExplodedNode *PrevN,
                               BugReporterContext &BRC, BugReport &BR) {
   // FIXME: We will eventually need to handle non-statement-based events
   // (__attribute__((cleanup))).
   if (!N->getLocation().getAs<StmtPoint>())
     return nullptr;
 
switch (ErrorKind) {
     case RefVal::ErrorDeallocNotOwned:
       if (!deallocNotOwned)
         deallocNotOwned.reset(new DeallocNotOwned(this));
       BT = deallocNotOwned.get();
       break;
   }
 
   assert(BT);
+  if (!isTrustedReferenceCountImplementation(N->getLocationContext())) {
     auto report = std::unique_ptr<BugReport>(
-      new CFRefReport(*BT, C.getASTContext().getLangOpts(), C.isObjCGCEnabled(),
-                      SummaryLog, N, Sym));
+        new CFRefReport(*BT, C.getASTContext().getLangOpts(),
+                        C.isObjCGCEnabled(), SummaryLog, N, Sym));
     report->addRange(ErrorRange);
     C.emitReport(std::move(report));
   }
+}
 
 //===----------------------------------------------------------------------===//
 // Handle the return values of retain-count-related functions.
 //===----------------------------------------------------------------------===//
 
 bool RetainCountChecker::evalCall(const CallExpr *CE, CheckerContext &C) const {
   // Get the callee. We're only interested in simple C functions.
   ProgramStateRef state = C.getState();
bool RetainCountChecker::evalCall(const CallExpr *CE, CheckerContext &C) const {
 
   // Get the name of the function.
   StringRef FName = II->getName();
   FName = FName.substr(FName.find_first_not_of('_'));
 
   // See if it's one of the specific functions we know how to eval.
   bool canEval = false;
 
   QualType ResultTy = CE->getCallReturnType(C.getASTContext());
   if (ResultTy->isObjCIdType()) {
     // Handle: id NSMakeCollectable(CFTypeRef)
     canEval = II->isStr("NSMakeCollectable");
   } else if (ResultTy->isPointerType()) {
     // Handle: (CF|CG|CV)Retain
     //         CFAutorelease
     //         CFMakeCollectable
     // It's okay to be a little sloppy here (CGMakeCollectable doesn't exist).
     if (cocoa::isRefType(ResultTy, "CF", FName) ||
         cocoa::isRefType(ResultTy, "CG", FName) ||
         cocoa::isRefType(ResultTy, "CV", FName)) {
       canEval = isRetain(FD, FName) || isAutorelease(FD, FName) ||
                 isMakeCollectable(FD, FName);
     }
   }
 
   if (!canEval)
     return false;
 
   // Bind the return value.
   const LocationContext *LCtx = C.getLocationContext();
   SVal RetVal = state->getSVal(CE->getArg(0), LCtx);
   if (RetVal.isUnknown()) {
     // If the receiver is unknown, conjure a return value.
if (RE.getKind() != RetEffect::NoRet) {
         // Generate an error node.
         state = setRefBinding(state, Sym, X);
 
         static CheckerProgramPointTag ReturnOwnLeakTag(this, "ReturnsOwnLeak");
         ExplodedNode *N = C.addTransition(state, Pred, &ReturnOwnLeakTag);
         if (N) {
           const LangOptions &LOpts = C.getASTContext().getLangOpts();
           bool GCEnabled = C.isObjCGCEnabled();
+          if (!isTrustedReferenceCountImplementation(N->getLocationContext()))
             C.emitReport(std::unique_ptr<BugReport>(new CFRefLeakReport(
                 *getLeakAtReturnBug(LOpts, GCEnabled), LOpts, GCEnabled,
                 SummaryLog, N, Sym, C, IncludeAllocationLine)));
         }
       }
     }
   } else if (X.isReturnedNotOwned()) {
     if (RE.isOwned()) {
       if (X.getIvarAccessHistory() ==
             RefVal::IvarAccessHistory::AccessedDirectly) {
         // Assume the method was trying to transfer a +1 reference from a
         // strong ivar to the caller.
         state = setRefBinding(state, Sym,
                               X.releaseViaIvar() ^ RefVal::ReturnedOwned);
       } else {
         // Trying to return a not owned object to a caller expecting an
         // owned object.
         state = setRefBinding(state, Sym, X ^ RefVal::ErrorReturnedNotOwned);
 
         static CheckerProgramPointTag
             ReturnNotOwnedTag(this, "ReturnNotOwnedForOwned");
 
         ExplodedNode *N = C.addTransition(state, Pred, &ReturnNotOwnedTag);
         if (N) {
           if (!returnNotOwnedForOwned)
             returnNotOwnedForOwned.reset(new ReturnedNotOwnedForOwned(this));
 
+          if (!isTrustedReferenceCountImplementation(N->getLocationContext()))
             C.emitReport(std::unique_ptr<BugReport>(new CFRefReport(
                 *returnNotOwnedForOwned, C.getASTContext().getLangOpts(),
                 C.isObjCGCEnabled(), SummaryLog, N, Sym)));
         }
       }
     }
   }
 }
 
 //===----------------------------------------------------------------------===//
 // Check various ways a symbol can be invalidated.
if (N) {
     else
       os << "but ";
     os << "has a +" << V.getCount() << " retain count";
 
     if (!overAutorelease)
       overAutorelease.reset(new OverAutorelease(this));
 
     const LangOptions &LOpts = Ctx.getASTContext().getLangOpts();
+    if (!isTrustedReferenceCountImplementation(N->getLocationContext()))
       Ctx.emitReport(std::unique_ptr<BugReport>(
           new CFRefReport(*overAutorelease, LOpts, /* GCEnabled = */ false,
                           SummaryLog, N, Sym, os.str())));
   }
 
   return nullptr;
 }
 
 ProgramStateRef
 RetainCountChecker::handleSymbolDeath(ProgramStateRef state,
                                       SymbolRef sid, RefVal V,
for (SmallVectorImpl<SymbolRef>::iterator
          I = Leaked.begin(), E = Leaked.end(); I != E; ++I) {
 
       const LangOptions &LOpts = Ctx.getASTContext().getLangOpts();
       bool GCEnabled = Ctx.isObjCGCEnabled();
       CFRefBug *BT = Pred ? getLeakWithinFunctionBug(LOpts, GCEnabled)
                           : getLeakAtReturnBug(LOpts, GCEnabled);
       assert(BT && "BugType not initialized.");
 
+      if (!isTrustedReferenceCountImplementation(N->getLocationContext()))
         Ctx.emitReport(std::unique_ptr<BugReport>(
             new CFRefLeakReport(*BT, LOpts, GCEnabled, SummaryLog, N, *I, Ctx,
                                 IncludeAllocationLine)));
     }
   }
 
   return N;
 }
 
 void RetainCountChecker::checkEndFunction(CheckerContext &Ctx) const {
   ProgramStateRef state = Ctx.getState();

test/Analysis/retain-release-inline.m

 // RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin10 -analyzer-checker=core,osx.coreFoundation.CFRetainRelease,osx.cocoa.ClassRelease,osx.cocoa.RetainCount -fblocks -verify %s
 
 //===----------------------------------------------------------------------===//
 // The following code is reduced using delta-debugging from Mac OS X headers:
 //
 // #include <Cocoa/Cocoa.h>
 // #include <CoreFoundation/CoreFoundation.h>
 // #include <DiskArbitration/DiskArbitration.h>
 // #include <QuartzCore/QuartzCore.h>
 // #include <Quartz/Quartz.h>
 // #include <IOKit/IOKitLib.h>
 //
 // It includes the basic definitions for the test cases below.
 //===----------------------------------------------------------------------===//
-
+#define NULL 0
 typedef unsigned int __darwin_natural_t;
 typedef unsigned long uintptr_t;
 typedef unsigned int uint32_t;
 typedef unsigned long long uint64_t;
 typedef unsigned int UInt32;
 typedef signed long CFIndex;
 typedef CFIndex CFByteOrder;
 typedef struct {
enum {
     NSSymbolStringEncoding = 6,
     NSNonLossyASCIIStringEncoding = 7,
 };
 typedef struct __CFString * CFMutableStringRef;
 typedef NSUInteger NSStringEncoding;
 
 extern CFStringRef CFStringCreateWithCStringNoCopy(CFAllocatorRef alloc, const char *cStr, CFStringEncoding encoding, CFAllocatorRef contentsDeallocator);
 
+typedef struct {
+  int ref;
+} isl_basic_map;
+
 //===----------------------------------------------------------------------===//
 // Test cases.
 //===----------------------------------------------------------------------===//
 
 void foo(id x) {
   [x retain];
 }
 
 void bar(id x) {
   [x release];
 }
 
 void test() {
   NSString *s = [[NSString alloc] init]; // expected-warning {{Potential leak}}
   foo(s);
   foo(s);
   bar(s);
 }
+
 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_copy(isl_basic_map *bmap);
+__attribute__((cf_returns_retained)) isl_basic_map *isl_basic_map_cow(__attribute__((cf_consumed)) isl_basic_map *bmap);
+isl_basic_map *isl_basic_map_decrement_reference_count(__attribute__((cf_consumed)) isl_basic_map *bmap);
+void free(void *);
+
+__attribute__((annotate("rc_ownership_trusted_implementation"))) __attribute__((cf_returns_retained)) isl_basic_map *test_use_after_release_with_trusted_implementation_annotate_attribute(__attribute__((cf_consumed)) isl_basic_map *bmap) {
+  // After this call, 'temp' has a +1 reference count.
+  isl_basic_map *temp = isl_basic_map_copy(bmap);
+  // After this call, 'bmap' has a +1 reference count.
+  bmap = isl_basic_map_cow(bmap);
+  // After this call, 'bmap' has a +0 reference count.
+  isl_basic_map_decrement_reference_count(bmap);
+  isl_basic_map_decrement_reference_count(bmap); // no-warning {{A 'use-after-release' warning is not raised here as this function has 'rc_ownership_trusted_implementation' annotate attribute}}
+  return temp;
+}
+
+isl_basic_map *isl_basic_map_free(__attribute__((cf_consumed)) isl_basic_map *bmap) {
+  if (!bmap)
+    return NULL;
+
+  if (--bmap->ref > 0)
+    return NULL;
+
+  free(bmap);
+  return NULL;
+}
+
+__attribute__((cf_returns_retained)) isl_basic_map *test_leak_with_trusted_implementation_annotate_attribute(__attribute__((cf_consumed)) isl_basic_map *bmap) {
+  // After this call, 'temp' has a +1 reference count.
+  isl_basic_map *temp = isl_basic_map_copy(bmap);
+  // After this call, 'bmap' has a +1 reference count.
+  bmap = isl_basic_map_cow(bmap); // no-warning {{A 'leak' warning is not raised here as 'foo_bar' (annotated with 'rc_ownership_trusted_implementation') is on the call-stack}}
+  // After this call, assuming the predicate of the second if branch to be true, 'bmap' has a +1 reference count.
+  isl_basic_map_free(bmap);
+  return temp;
+}
+
+__attribute__((annotate("rc_ownership_trusted_implementation"))) __attribute__((cf_returns_retained)) isl_basic_map *foo_bar(__attribute__((cf_consumed)) isl_basic_map *bmap) {
+  bmap = test_leak_with_trusted_implementation_annotate_attribute(bmap);
+  return bmap;
+}
+
 //===----------------------------------------------------------------------===//
 // Test returning retained and not-retained values.
 //===----------------------------------------------------------------------===//
 
 // On return (intraprocedural), assume CF objects are leaked.
 CFStringRef test_return_ratained_CF(char *bytes) {
   CFStringRef str;
   return CFStringCreateWithCStringNoCopy(0, bytes, NSNEXTSTEPStringEncoding, 0); // expected-warning {{leak}}