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

1913

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

3399

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

3422

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

3418

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

 
 const RetainSummary *
 RetainSummaryManager::getCFSummaryGetRule(const FunctionDecl *FD) {
   assert (ScratchArgs.isEmpty());
   return getPersistentSummary(RetEffect::MakeNotOwned(RetEffect::CF),
                               DoNothing, DoNothing);
 }
 
+/// Returns true if the declaration 'D' is annotated with 'rcAnnotation'.
+bool hasRCAnnotation(const Decl *D, StringRef rcAnnotation) {
+  for (const auto *Ann : D->specific_attrs<AnnotateAttr>()) {
+    if (Ann->getAnnotation() == rcAnnotation)
+      return true;
+  }
+  return false;
+}
+
+/// Returns true if the function declaration 'FD' contains
+/// 'rc_ownership_trusted_implementation' annotate attribute.
+bool isTrustedReferenceCountImplementation(const FunctionDecl *FD) {
+  return hasRCAnnotation(FD, "rc_ownership_trusted_implementation");
+}
+
 //===----------------------------------------------------------------------===//
 // Summary creation for Selectors.
 //===----------------------------------------------------------------------===//
 
 Optional<RetEffect>
 RetainSummaryManager::getRetEffectFromAnnotations(QualType RetTy,
                                                   const Decl *D) {
   if (cocoa::isCocoaObjectRef(RetTy)) {
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();
 }
 
 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;
 
   // Check if the type state has changed.
   ProgramStateRef PrevSt = PrevN->getState();
if (CE->getNumArgs() != 1)
     return false;
 
   // 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;
+  // See if the function has 'rc_ownership_trusted_implementation'
+  // annotate attribute. If it does, we will not inline it.
+  bool hasTrustedImplementationAnnotation = 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);
+    } else {
+      if (FD->getDefinition()) {
+        canEval = isTrustedReferenceCountImplementation(FD->getDefinition());
+        hasTrustedImplementationAnnotation = canEval;
+      }
     }
   }
 
   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 (RetVal.isUnknown() ||
+      (hasTrustedImplementationAnnotation && !ResultTy.isNull())) {
+    // If the receiver is unknown or the function has
+    // 'rc_ownership_trusted_implementation' annotate attribute, conjure a
+    // return value.
     SValBuilder &SVB = C.getSValBuilder();
     RetVal = SVB.conjureSymbolVal(nullptr, CE, LCtx, ResultTy, C.blockCount());
   }
   state = state->BindExpr(CE, LCtx, RetVal, false);
 
   // FIXME: This should not be necessary, but otherwise the argument seems to be
   // considered alive during the next statement.
   if (const MemRegion *ArgRegion = RetVal.getAsRegion()) {
     // Save the refcount status of the argument.
     SymbolRef Sym = RetVal.getAsLocSymbol();
     const RefVal *Binding = nullptr;
     if (Sym)
       Binding = getRefBinding(state, Sym);
 
     // Invalidate the argument region.
-    state = state->invalidateRegions(ArgRegion, CE, C.blockCount(), LCtx,
-                                     /*CausesPointerEscape*/ false);
+    state = state->invalidateRegions(
+        ArgRegion, CE, C.blockCount(), LCtx,
+        /*CausesPointerEscape*/ hasTrustedImplementationAnnotation);
 
     // Restore the refcount status of the argument.
     if (Binding)
       state = setRefBinding(state, Sym, *Binding);
   }
 
   C.addTransition(state);
   return true;

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_cow(__attribute__((cf_consumed)) 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) {
+  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) {
+  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) {
+  // 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) {
+  // 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.
 //===----------------------------------------------------------------------===//
 
 // 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}}