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

[malhar1995]

Summary

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.

[malhar1995]

lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp

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]

lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp

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]

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?

lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp

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.

test/Analysis/retain-release-inline.m

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]

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.

[dcoughlin]

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!

lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp

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.

[malhar1995]

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

[dcoughlin]

Thanks Malhar! Committed in r311063.

cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp

bool isSimple() const {
     return Args.isEmpty();
   }
 
 private:
   ArgEffects getArgEffects() const { return Args; }
   ArgEffect getDefaultArgEffect() const { return DefaultArgEffect; }
 
   friend class RetainSummaryManager;
+  friend class RetainCountChecker;
 };
 } // end anonymous namespace
 
 //===----------------------------------------------------------------------===//
 // Data structures for constructing summaries.
 //===----------------------------------------------------------------------===//
 
 namespace {

 }
 
 /// Returns true if the function declaration 'FD' contains
 /// 'rc_ownership_trusted_implementation' annotate attribute.
 static bool isTrustedReferenceCountImplementation(const FunctionDecl *FD) {
   return hasRCAnnotation(FD, "rc_ownership_trusted_implementation");
 }
 
+static bool isGeneralizedObjectRef(QualType Ty) {
+  if (Ty.getAsString().substr(0, 4) == "isl_")
+    return true;
+  else
+    return false;
+}
+
 //===----------------------------------------------------------------------===//
 // Summary creation for Selectors.
 //===----------------------------------------------------------------------===//
 
 Optional<RetEffect>
 RetainSummaryManager::getRetEffectFromAnnotations(QualType RetTy,
                                                   const Decl *D) {
   if (cocoa::isCocoaObjectRef(RetTy)) {
llvm::iterator_range<ranges_iterator> getRanges() override {
       if (!BugTy.isLeak())
         return BugReport::getRanges();
       return llvm::make_range(ranges_iterator(), ranges_iterator());
     }
   };
 
   class CFRefLeakReport : public CFRefReport {
     const MemRegion* AllocBinding;
+    const Stmt *AllocStmt;
+
+    // Finds the function declaration where a leak warning for the parameter 'sym' should be raised.
+    void deriveParamLocation(CheckerContext &Ctx, SymbolRef sym);
+    // Finds the location where a leak warning for 'sym' should be raised.
+    void deriveAllocLocation(CheckerContext &Ctx, SymbolRef sym);
+    // Produces description of a leak warning which is printed on the console.
+    void createDescription(CheckerContext &Ctx, bool GCEnabled, bool IncludeAllocationLine);
+
   public:
     CFRefLeakReport(CFRefBug &D, const LangOptions &LOpts, bool GCEnabled,
                     const SummaryLogTy &Log, ExplodedNode *n, SymbolRef sym,
                     CheckerContext &Ctx,
                     bool IncludeAllocationLine);
 
     PathDiagnosticLocation getLocation(const SourceManager &SM) const override {
       assert(Location.isValid());
CFRefLeakReportVisitor::getEndPath(BugReporterContext &BRC,
   }
   else
     os << " is not referenced later in this execution path and has a retain "
           "count of +" << RV->getCount();
 
   return llvm::make_unique<PathDiagnosticEventPiece>(L, os.str());
 }
 
-CFRefLeakReport::CFRefLeakReport(CFRefBug &D, const LangOptions &LOpts,
-                                 bool GCEnabled, const SummaryLogTy &Log,
-                                 ExplodedNode *n, SymbolRef sym,
-                                 CheckerContext &Ctx,
-                                 bool IncludeAllocationLine)
-  : CFRefReport(D, LOpts, GCEnabled, Log, n, sym, false) {
+void CFRefLeakReport::deriveParamLocation(CheckerContext &Ctx, SymbolRef sym) {
+  const SourceManager& SMgr = Ctx.getSourceManager();
+
+  if (!sym->getOriginRegion())
+    return;
 
+  auto *Region = dyn_cast<DeclRegion>(sym->getOriginRegion());
+  if (Region) {
+    const Decl *PDecl = Region->getDecl();
+    if (PDecl && isa<ParmVarDecl>(PDecl)) {
+      PathDiagnosticLocation ParamLocation = PathDiagnosticLocation::create(PDecl, SMgr);
+      Location = ParamLocation;
+      UniqueingLocation = ParamLocation;
+      UniqueingDecl = Ctx.getLocationContext()->getDecl();
+    }
+  }
+}
+
+void CFRefLeakReport::deriveAllocLocation(CheckerContext &Ctx,SymbolRef sym) {
   // Most bug reports are cached at the location where they occurred.
   // With leaks, we want to unique them by the location where they were
   // allocated, and only report a single path.  To do this, we need to find
   // the allocation site of a piece of tracked memory, which we do via a
   // call to GetAllocationSite.  This will walk the ExplodedGraph backwards.
   // Note that this is *not* the trimmed graph; we are guaranteed, however,
   // that all ancestor nodes that represent the allocation site have the
   // same SourceLocation.
   const ExplodedNode *AllocNode = nullptr;
 
   const SourceManager& SMgr = Ctx.getSourceManager();
 
   AllocationInfo AllocI =
     GetAllocationSite(Ctx.getStateManager(), getErrorNode(), sym);
 
   AllocNode = AllocI.N;
   AllocBinding = AllocI.R;
   markInteresting(AllocI.InterestingMethodContext);
 
   // Get the SourceLocation for the allocation site.
   // FIXME: This will crash the analyzer if an allocation comes from an
   // implicit call (ex: a destructor call).
   // (Currently there are no such allocations in Cocoa, though.)
-  const Stmt *AllocStmt = PathDiagnosticLocation::getStmt(AllocNode);
-  assert(AllocStmt && "Cannot find allocation statement");
+  AllocStmt = PathDiagnosticLocation::getStmt(AllocNode);
+
+  if (!AllocStmt) {
+    AllocBinding = nullptr;
+    return;
+  }
 
   PathDiagnosticLocation AllocLocation =
     PathDiagnosticLocation::createBegin(AllocStmt, SMgr,
                                         AllocNode->getLocationContext());
   Location = AllocLocation;
 
   // Set uniqieing info, which will be used for unique the bug reports. The
   // leaks should be uniqued on the allocation site.
   UniqueingLocation = AllocLocation;
   UniqueingDecl = AllocNode->getLocationContext()->getDecl();
+}
 
-  // Fill in the description of the bug.
+void CFRefLeakReport::createDescription(CheckerContext &Ctx, bool GCEnabled, bool IncludeAllocationLine) {
+  assert(Location.isValid() && UniqueingDecl && UniqueingLocation.isValid());
   Description.clear();
   llvm::raw_string_ostream os(Description);
   os << "Potential leak ";
   if (GCEnabled)
     os << "(when using garbage collection) ";
   os << "of an object";
 
   if (AllocBinding) {
     os << " stored into '" << AllocBinding->getString() << '\'';
     if (IncludeAllocationLine) {
       FullSourceLoc SL(AllocStmt->getLocStart(), Ctx.getSourceManager());
       os << " (allocated on line " << SL.getSpellingLineNumber() << ")";
     }
   }
+}
+
+CFRefLeakReport::CFRefLeakReport(CFRefBug &D, const LangOptions &LOpts,
+                                 bool GCEnabled, const SummaryLogTy &Log,
+                                 ExplodedNode *n, SymbolRef sym,
+                                 CheckerContext &Ctx,
+                                 bool IncludeAllocationLine)
+  : CFRefReport(D, LOpts, GCEnabled, Log, n, sym, false) {
+
+  deriveAllocLocation(Ctx, sym);
+  if (!AllocBinding)
+    deriveParamLocation(Ctx, sym);
+
+  createDescription(Ctx, GCEnabled, IncludeAllocationLine);
 
   addVisitor(llvm::make_unique<CFRefLeakReportVisitor>(sym, GCEnabled, Log));
 }
 
 //===----------------------------------------------------------------------===//
 // Main checker logic.
 //===----------------------------------------------------------------------===//
 
 namespace {
 class RetainCountChecker
   : public Checker< check::Bind,
                     check::DeadSymbols,
                     check::EndAnalysis,
+                    check::BeginFunction,
                     check::EndFunction,
                     check::PostStmt<BlockExpr>,
                     check::PostStmt<CastExpr>,
                     check::PostStmt<ObjCArrayLiteral>,
                     check::PostStmt<ObjCDictionaryLiteral>,
                     check::PostStmt<ObjCBoxedExpr>,
                     check::PostStmt<ObjCIvarRefExpr>,
                     check::PostCall,
checkRegionChanges(ProgramStateRef state,
                      const CallEvent *Call) const;
 
   void checkPreStmt(const ReturnStmt *S, CheckerContext &C) const;
   void checkReturnWithRetEffect(const ReturnStmt *S, CheckerContext &C,
                                 ExplodedNode *Pred, RetEffect RE, RefVal X,
                                 SymbolRef Sym, ProgramStateRef state) const;
 
   void checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &C) const;
+  void checkBeginFunction(CheckerContext &C) const;
   void checkEndFunction(CheckerContext &C) const;
 
   ProgramStateRef updateSymbol(ProgramStateRef state, SymbolRef sym,
                                RefVal V, ArgEffect E, RefVal::Kind &hasErr,
                                CheckerContext &C) const;
 
   void processNonLeakError(ProgramStateRef St, SourceRange ErrorRange,
                            RefVal::Kind ErrorKind, SymbolRef Sym,
for (SmallVectorImpl<SymbolRef>::iterator
           new CFRefLeakReport(*BT, LOpts, GCEnabled, SummaryLog, N, *I, Ctx,
                               IncludeAllocationLine)));
     }
   }
 
   return N;
 }
 
+void RetainCountChecker::checkBeginFunction(CheckerContext &Ctx) const {
+  if (!Ctx.inTopFrame())
+    return;
+
+  const LocationContext *LCtx = Ctx.getLocationContext();
+  const FunctionDecl *FD = dyn_cast<FunctionDecl>(LCtx->getDecl());
+
+  if (!FD || isTrustedReferenceCountImplementation(FD))
+    return;
+
+  ProgramStateRef state = Ctx.getState();
+
+  const RetainSummary *FunctionSummary = getSummaryManager(Ctx).getFunctionSummary(FD);
+  ArgEffects CalleeSideArgEffects = FunctionSummary->getArgEffects();
+
+  for (unsigned idx = 0, e = FD->getNumParams(); idx != e; ++idx) {
+    const ParmVarDecl *Param = FD->getParamDecl(idx);
+    SymbolRef Sym = state->getSVal(state->getRegion(Param, LCtx)).getAsSymbol();
+
+    QualType Ty = Param->getType();
+    const ArgEffect *AE = CalleeSideArgEffects.lookup(idx);
+    if (AE && *AE == DecRef && isGeneralizedObjectRef(Ty))
+      state = setRefBinding(state, Sym, RefVal::makeOwned(RetEffect::ObjKind::Generalized, Ty));
+    else if (isGeneralizedObjectRef(Ty))
+      state = setRefBinding(state, Sym, RefVal::makeNotOwned(RetEffect::ObjKind::Generalized, Ty));
+  }
+
+  Ctx.addTransition(state);
+}
+
 void RetainCountChecker::checkEndFunction(CheckerContext &Ctx) const {
   ProgramStateRef state = Ctx.getState();
   RefBindingsTy B = state->get<RefBindings>();
   ExplodedNode *Pred = Ctx.getPredecessor();
 
   // Don't process anything within synthesized bodies.
   const LocationContext *LCtx = Pred->getLocationContext();
   if (LCtx->getAnalysisDeclContext()->isBodyAutosynthesized()) {

cfe/trunk/test/Analysis/retain-release-inline.m

void test_neg() {
   bar(s);
   bar(s);
   bar(s);
 }
 
 __attribute__((annotate("rc_ownership_returns_retained"))) isl_basic_map *isl_basic_map_cow(__attribute__((annotate("rc_ownership_consumed"))) isl_basic_map *bmap);
 void free(void *);
 
+void callee_side_parameter_checking_leak(__attribute__((annotate("rc_ownership_consumed"))) isl_basic_map *bmap) { // expected-warning {{Potential leak of an object}}
+}
+
 // 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__((annotate("rc_ownership_consumed"))) isl_basic_map *bmap) {
   if (!bmap)
     return NULL;

 
 void test_leak_with_trusted_implementation_annotate_attribute(__attribute__((annotate("rc_ownership_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);
 }
 
+void callee_side_parameter_checking_incorrect_rc_decrement(isl_basic_map *bmap) {
+  isl_basic_map_free(bmap); // expected-warning {{Incorrect decrement of the reference count}}
+}
+
+__attribute__((annotate("rc_ownership_returns_retained"))) isl_basic_map *callee_side_parameter_checking_return_notowned_object(isl_basic_map *bmap) {
+  return bmap; // expected-warning {{Object with a +0 retain count returned to caller where a +1 (owning) retain count is expected}}
+}
+
+__attribute__((annotate("rc_ownership_returns_retained"))) isl_basic_map *callee_side_parameter_checking_assign_consumed_parameter_leak_return(__attribute__((annotate("rc_ownership_consumed"))) isl_basic_map *bmap1, __attribute__((annotate("rc_ownership_consumed"))) isl_basic_map *bmap2) { // expected-warning {{Potential leak of an object}}
+  bmap1 = bmap2;
+  isl_basic_map_free(bmap2);
+  return bmap1;
+}
+
+__attribute__((annotate("rc_ownership_returns_retained"))) isl_basic_map *callee_side_parameter_checking_assign_consumed_parameter_leak(__attribute__((annotate("rc_ownership_consumed"))) isl_basic_map *bmap1, __attribute__((annotate("rc_ownership_consumed"))) isl_basic_map *bmap2) { // expected-warning {{Potential leak of an object}}
+  bmap1 = bmap2;
+  isl_basic_map_free(bmap1);
+  return bmap2;
+}
+
+__attribute__((annotate("rc_ownership_returns_retained"))) isl_basic_map *error_path_leak(__attribute__((annotate("rc_ownership_consumed"))) isl_basic_map *bmap1, __attribute__((annotate("rc_ownership_consumed"))) isl_basic_map *bmap2) { // expected-warning {{Potential leak of an object}}
+  bmap1 = isl_basic_map_cow(bmap1);
+  if (!bmap1 || !bmap2)
+    goto error;
+
+  isl_basic_map_free(bmap2);
+  return bmap1;
+error:
+  return isl_basic_map_free(bmap1);
+}
+
 //===----------------------------------------------------------------------===//
 // 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}}