[analyzer] Another RetainCountChecker cleanup

This is not NFC strictly speaking, since it unifies CleanupAttr handling,
so that out parameters now also understand it.

Differential Revision: https://reviews.llvm.org/D56759

llvm-svn: 351394

clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp

@@ -529,12 +529,24 @@ void RetainCountChecker::processSummaryOfInlined(const RetainSummary &Summ,
   C.addTransition(state);
 }
 
+/// A value escapes in these possible cases:
+///
+/// - binding to something that is not a memory region.
+/// - binding to a memregion that does not have stack storage
+/// - binding to a variable that has a destructor attached using CleanupAttr
+///
+/// We do not currently model what happens when a symbol is
+/// assigned to a struct field, so be conservative here and let the symbol go.
+/// FIXME: This could definitely be improved upon.
 static bool shouldEscapeRegion(const MemRegion *R) {
+  const auto *VR = dyn_cast<VarRegion>(R);
+  if (!R->hasStackStorage() || !VR)
+    return true;
 
-  // We do not currently model what happens when a symbol is
-  // assigned to a struct field, so be conservative here and let the symbol
-  // go. TODO: This could definitely be improved upon.
-  return !R->hasStackStorage() || !isa<VarRegion>(R);
+  const VarDecl *VD = VR->getDecl();
+  if (!VD->hasAttr<CleanupAttr>())
+    return false; // CleanupAttr attaches destructors, which cause escaping.
+  return true;
 }
 
 static SmallVector<ProgramStateRef, 2>
@@ -1145,39 +1157,15 @@ ExplodedNode * RetainCountChecker::checkReturnWithRetEffect(const ReturnStmt *S,
 
 void RetainCountChecker::checkBind(SVal loc, SVal val, const Stmt *S,
                                    CheckerContext &C) const {
-  // Are we storing to something that causes the value to "escape"?
-  bool escapes = true;
-
-  // A value escapes in three possible cases (this may change):
-  //
-  // (1) we are binding to something that is not a memory region.
-  // (2) we are binding to a memregion that does not have stack storage
   ProgramStateRef state = C.getState();
+  const MemRegion *MR = loc.getAsRegion();
 
-  if (auto regionLoc = loc.getAs<loc::MemRegionVal>()) {
-    escapes = shouldEscapeRegion(regionLoc->getRegion());
-  }
-
-  // If we are storing the value into an auto function scope variable annotated
-  // with (__attribute__((cleanup))), stop tracking the value to avoid leak
-  // false positives.
-  if (const auto *LVR = dyn_cast_or_null<VarRegion>(loc.getAsRegion())) {
-    const VarDecl *VD = LVR->getDecl();
-    if (VD->hasAttr<CleanupAttr>()) {
-      escapes = true;
-    }
-  }
-
-  // If our store can represent the binding and we aren't storing to something
-  // that doesn't have local storage then just return and have the simulation
-  // state continue as is.
-  if (!escapes)
-      return;
-
-  // Otherwise, find all symbols referenced by 'val' that we are tracking
+  // Find all symbols referenced by 'val' that we are tracking
   // and stop tracking them.
-  state = state->scanReachableSymbols<StopTrackingCallback>(val).getState();
-  C.addTransition(state);
+  if (MR && shouldEscapeRegion(MR)) {
+    state = state->scanReachableSymbols<StopTrackingCallback>(val).getState();
+    C.addTransition(state);
+  }
 }
 
 ProgramStateRef RetainCountChecker::evalAssume(ProgramStateRef state,
@@ -1196,14 +1184,14 @@ ProgramStateRef RetainCountChecker::evalAssume(ProgramStateRef state,
 
   bool changed = false;
   RefBindingsTy::Factory &RefBFactory = state->get_context<RefBindings>();
+  ConstraintManager &CMgr = state->getConstraintManager();
 
-  for (RefBindingsTy::iterator I = B.begin(), E = B.end(); I != E; ++I) {
+  for (auto &I : B) {
     // Check if the symbol is null stop tracking the symbol.
-    ConstraintManager &CMgr = state->getConstraintManager();
-    ConditionTruthVal AllocFailed = CMgr.isNull(state, I.getKey());
+    ConditionTruthVal AllocFailed = CMgr.isNull(state, I.first);
     if (AllocFailed.isConstrainedTrue()) {
       changed = true;
-      B = RefBFactory.remove(B, I.getKey());
+      B = RefBFactory.remove(B, I.first);
     }
   }
 
@@ -1424,9 +1412,9 @@ void RetainCountChecker::checkEndFunction(const ReturnStmt *RS,
     return;
   }
 
-  for (RefBindingsTy::iterator I = B.begin(), E = B.end(); I != E; ++I) {
+  for (auto &I : B) {
     state = handleAutoreleaseCounts(state, Pred, /*Tag=*/nullptr, Ctx,
-                                    I->first, I->second);
+                                    I.first, I.second);
     if (!state)
       return;
   }
@@ -1441,8 +1429,8 @@ void RetainCountChecker::checkEndFunction(const ReturnStmt *RS,
   B = state->get<RefBindings>();
   SmallVector<SymbolRef, 10> Leaked;
 
-  for (RefBindingsTy::iterator I = B.begin(), E = B.end(); I != E; ++I)
-    state = handleSymbolDeath(state, I->first, I->second, Leaked);
+  for (auto &I : B)
+    state = handleSymbolDeath(state, I.first, I.second, Leaked);
 
   processLeaks(state, Leaked, Ctx, Pred);
 }
@@ -1503,9 +1491,9 @@ void RetainCountChecker::printState(raw_ostream &Out, ProgramStateRef State,
 
   Out << Sep << NL;
 
-  for (RefBindingsTy::iterator I = B.begin(), E = B.end(); I != E; ++I) {
-    Out << I->first << " : ";
-    I->second.print(Out);
+  for (auto &I : B) {
+    Out << I.first << " : ";
+    I.second.print(Out);
     Out << NL;
   }
 }

clang/test/Analysis/osobject-retain-release.cpp

@@ -263,6 +263,13 @@ void use_out_param_leak_osreturn() {
 } // expected-warning{{Potential leak of an object stored into 'obj'}}
   // expected-note@-1{{Object leaked: object allocated and stored into 'obj' is not referenced later in this execution path and has a retain count of +1}}
 
+void cleanup(OSObject **obj);
+
+void test_cleanup_escaping() {
+  __attribute__((cleanup(cleanup))) OSObject *obj;
+  always_write_into_out_param(&obj); // no-warning, the value has escaped.
+}
+
 struct StructWithField {
   OSObject *obj;