[analyzer] Another RetainCountChecker cleanup

[george.karpenkov]

Summary

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

[NoQ]

This is not NFC strictly speaking

Test? (:

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

1162–1166 ↗

(On Diff #181931)

Slightly shorter:

const MemRegion *MR = loc.getAsRegion();
if (!MR)
  return;

if (shouldEscapeRegion(MR)) { ... }

It should be equivalent because the only other case in which getAsRegion() succeeds is when the value is nonloc::LocAsInteger but it's something you can bind *to*.

[george.karpenkov]

Added test.

[NoQ]

Thx (:

lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp

void RetainCountChecker::processSummaryOfInlined(const RetainSummary &Summ,
   if (SymbolRef Sym = CallOrMsg.getReturnValue().getAsSymbol()) {
     if (RE.getKind() == RetEffect::NoRetHard)
       state = removeRefBinding(state, Sym);
   }
 
   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>
 updateOutParameters(ProgramStateRef State, const RetainSummary &Summ,
                     const CallEvent &CE) {
 
   SVal L = CE.getReturnValue();
 

 }
 
 //===----------------------------------------------------------------------===//
 // Check various ways a symbol can be invalidated.
 //===----------------------------------------------------------------------===//
 
 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.
+  if (MR && shouldEscapeRegion(MR)) {
     state = state->scanReachableSymbols<StopTrackingCallback>(val).getState();
     C.addTransition(state);
   }
+}
 
 ProgramStateRef RetainCountChecker::evalAssume(ProgramStateRef state,
                                                SVal Cond,
                                                bool Assumption) const {
   // FIXME: We may add to the interface of evalAssume the list of symbols
   //  whose assumptions have changed.  For now we just iterate through the
   //  bindings and check if any of the tracked symbols are NULL.  This isn't
   //  too bad since the number of symbols we will track in practice are
   //  probably small and evalAssume is only called at branches and a few
   //  other places.
   RefBindingsTy B = state->get<RefBindings>();
 
   if (B.isEmpty())
     return 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);
     }
   }
 
   if (changed)
     state = state->set<RefBindings>(B);
 
   return state;
 }
void RetainCountChecker::checkEndFunction(const ReturnStmt *RS,
 
   // Don't process anything within synthesized bodies.
   const LocationContext *LCtx = Pred->getLocationContext();
   if (LCtx->getAnalysisDeclContext()->isBodyAutosynthesized()) {
     assert(!LCtx->inTopFrame());
     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;
   }
 
   // If the current LocationContext has a parent, don't check for leaks.
   // We will do that later.
   // FIXME: we should instead check for imbalances of the retain/releases,
   // and suggest annotations.
   if (LCtx->getParent())
     return;
 
   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);
 }
 
 void RetainCountChecker::checkDeadSymbols(SymbolReaper &SymReaper,
                                           CheckerContext &C) const {
   ExplodedNode *Pred = C.getPredecessor();
 
void RetainCountChecker::printState(raw_ostream &Out, ProgramStateRef State,
 
   RefBindingsTy B = State->get<RefBindings>();
 
   if (B.isEmpty())
     return;
 
   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;
   }
 }
 
 //===----------------------------------------------------------------------===//
 // Checker registration.
 //===----------------------------------------------------------------------===//
 

test/Analysis/osobject-retain-release.cpp

 }
 
 void use_out_param_leak_osreturn() {
   OSObject *obj;
   write_into_out_param_on_os_success(&obj); // expected-note-re{{Call to function 'write_into_out_param_on_os_success' writes an OSObject of type 'OSObject' with a +1 retain count into an out parameter 'obj' (assuming the call returns zero){{$}}}}
 } // 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;
 
   void initViaOutParamCall() { // no warning on writing into fields
     always_write_into_out_param(&obj);
   }
 
 };