[analyzer] Insert notes in RetainCountChecker where our dynamic cast modeling assumes 'null' output

[george.karpenkov]

Summary

rdar://47397214

[NoQ]

clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.h

266–267 ↗

(On Diff #182656)

I think you can put tags themselves here (as non-static members) and compare them by pointers later.

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

514 ↗

(On Diff #182656)

I think that there should be a note when the cast succeeds.

Another thing to think about: should we add these pieces on *all* casts? Wouldn't it lead to displaying too much inlined diagnostics? Should we somehow mark these as prunable? Should we display them only when interesting objects are casted?

[george.karpenkov]

clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.h

266–267 ↗

(On Diff #182656)

That would be better - but then I need access to the checker in the visitor.
I guess I could just cast it =/

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

514 ↗

(On Diff #182656)

I think that there should be a note when the cast succeeds.

Not sure, usually readers assume that the cast succeeds when reading the code.

Another thing to think about: should we add these pieces on *all* casts?

I haven't seen too much diagnostics from this in practice (by manually looking at reports).

Should we display them only when interesting objects are casted?

Again not sure =/ Even if the object is not interesting failed casts can lead us to enter unexpected branches.

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

class RetainCountChecker
   RefCountBug releaseNotOwned{this, RefCountBug::ReleaseNotOwned};
   RefCountBug deallocNotOwned{this, RefCountBug::DeallocNotOwned};
   RefCountBug freeNotOwned{this, RefCountBug::FreeNotOwned};
   RefCountBug overAutorelease{this, RefCountBug::OverAutorelease};
   RefCountBug returnNotOwnedForOwned{this, RefCountBug::ReturnNotOwnedForOwned};
   RefCountBug leakWithinFunction{this, RefCountBug::LeakWithinFunction};
   RefCountBug leakAtReturn{this, RefCountBug::LeakAtReturn};
 
+  CheckerProgramPointTag DeallocSentTag{this, "DeallocSent"};
+  CheckerProgramPointTag CastFailTag{this, "DynamicCastFail"};
+
   mutable std::unique_ptr<RetainSummaryManager> Summaries;
 public:
-  static constexpr const char *DeallocTagDescription = "DeallocSent";
 
   /// Track Objective-C and CoreFoundation objects.
   bool TrackObjCAndCFObjects = false;
 
   /// Track sublcasses of OSObject.
   bool TrackOSObjects = false;
 
   RetainCountChecker() {};
handleAutoreleaseCounts(ProgramStateRef state, ExplodedNode *Pred,
                           RefVal V,
                           const ReturnStmt *S=nullptr) const;
 
   ExplodedNode *processLeaks(ProgramStateRef state,
                              SmallVectorImpl<SymbolRef> &Leaked,
                              CheckerContext &Ctx,
                              ExplodedNode *Pred = nullptr) const;
 
+  const CheckerProgramPointTag &getDeallocSentTag() const {
+    return DeallocSentTag;
+  }
+
+  const CheckerProgramPointTag &getCastFailTag() const {
+    return CastFailTag;
+  }
+
 private:
   /// Perform the necessary checks and state adjustments at the end of the
   /// function.
   /// \p S Return statement, may be null.
   ExplodedNode * processReturn(const ReturnStmt *S, CheckerContext &C) const;
 };
 
 //===----------------------------------------------------------------------===//

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

void RetainCountChecker::checkSummary(const RetainSummary &Summ,
 
   // Evaluate the effect of the arguments.
   RefVal::Kind hasErr = (RefVal::Kind) 0;
   SourceRange ErrorRange;
   SymbolRef ErrorSym = nullptr;
 
   // Helper tag for providing diagnostics: indicate whether dealloc was sent
   // at this location.
-  static CheckerProgramPointTag DeallocSentTag(this, DeallocTagDescription);
   bool DeallocSent = false;
 
   for (unsigned idx = 0, e = CallOrMsg.getNumArgs(); idx != e; ++idx) {
     SVal V = CallOrMsg.getArgSVal(idx);
 
     ArgEffect Effect = Summ.getArg(idx);
     if (SymbolRef Sym = V.getAsLocSymbol()) {
       if (const RefVal *T = getRefBinding(state, Sym)) {
if (BSmr == BehaviorSummary::Identity ||
 
     if (BSmr == BehaviorSummary::IdentityOrZero) {
       // Add a branch where the output is zero.
       ProgramStateRef NullOutputState = C.getState();
 
       // Assume that output is zero on the other branch.
       NullOutputState = NullOutputState->BindExpr(
           CE, LCtx, C.getSValBuilder().makeNull(), /*Invalidate=*/false);
-
-      C.addTransition(NullOutputState);
+      C.addTransition(NullOutputState, &CastFailTag);
 
       // And on the original branch assume that both input and
       // output are non-zero.
       if (auto L = RetVal.getAs<DefinedOrUnknownSVal>())
         state = state->assume(*L, /*Assumption=*/true);
 
     }
   }

cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.h

enum RefCountBugType {
     FreeNotOwned,
     OverAutorelease,
     ReturnNotOwnedForOwned,
     LeakWithinFunction,
     LeakAtReturn,
   };
   RefCountBug(const CheckerBase *checker, RefCountBugType BT);
   StringRef getDescription() const;
+
   RefCountBugType getBugType() const {
     return BT;
   }
 
+  const CheckerBase *getChecker() const {
+    return Checker;
+  }
+
 private:
   RefCountBugType BT;
+  const CheckerBase *Checker;
   static StringRef bugTypeToName(RefCountBugType BT);
 };
 
 class RefCountReport : public BugReport {
 protected:
   SymbolRef Sym;
   bool isLeak = false;
 

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

case LeakAtReturn:
     return "";
   }
   llvm_unreachable("Unknown RefCountBugType");
 }
 
 RefCountBug::RefCountBug(const CheckerBase *Checker, RefCountBugType BT)
     : BugType(Checker, bugTypeToName(BT), categories::MemoryRefCount,
               /*SupressOnSink=*/BT == LeakWithinFunction || BT == LeakAtReturn),
-      BT(BT) {}
+      BT(BT), Checker(Checker) {}
 
 static bool isNumericLiteralExpression(const Expr *E) {
   // FIXME: This set of cases was copied from SemaExprObjC.
   return isa<IntegerLiteral>(E) ||
          isa<CharacterLiteral>(E) ||
          isa<FloatingLiteral>(E) ||
          isa<ObjCBoolLiteralExpr>(E) ||
          isa<CXXBoolLiteralExpr>(E);
annotateConsumedSummaryMismatch(const ExplodedNode *N,
   return std::make_shared<PathDiagnosticEventPiece>(L, os.str());
 }
 
 std::shared_ptr<PathDiagnosticPiece>
 RefCountReportVisitor::VisitNode(const ExplodedNode *N,
                               BugReporterContext &BRC, BugReport &BR) {
 
   const auto &BT = static_cast<const RefCountBug&>(BR.getBugType());
+  const auto *Checker =
+      static_cast<const RetainCountChecker *>(BT.getChecker());
 
   bool IsFreeUnowned = BT.getBugType() == RefCountBug::FreeNotOwned ||
                        BT.getBugType() == RefCountBug::DeallocNotOwned;
 
   const SourceManager &SM = BRC.getSourceManager();
   CallEventManager &CEMgr = BRC.getStateManager().getCallEventManager();
   if (auto CE = N->getLocationAs<CallExitBegin>())
     if (auto PD = annotateConsumedSummaryMismatch(N, *CE, SM, CEMgr))
if (!PrevT) {
     return std::make_shared<PathDiagnosticEventPiece>(Pos, os.str());
   }
 
   // Gather up the effects that were performed on the object at this
   // program point
   bool DeallocSent = false;
 
   const ProgramPointTag *Tag = N->getLocation().getTag();
-  if (Tag && Tag->getTagDescription().contains(
-                 RetainCountChecker::DeallocTagDescription)) {
+
+  if (Tag == &Checker->getCastFailTag()) {
+    os << "Assuming dynamic cast returns null due to type mismatch";
+  }
+
+  if (Tag == &Checker->getDeallocSentTag()) {
     // We only have summaries attached to nodes after evaluating CallExpr and
     // ObjCMessageExprs.
     const Stmt *S = N->getLocation().castAs<StmtPoint>().getStmt();
 
     if (const CallExpr *CE = dyn_cast<CallExpr>(S)) {
       // Iterate through the parameter expressions and see if the symbol
       // was ever passed as an argument.
       unsigned i = 0;

cfe/trunk/test/Analysis/osobject-retain-release.cpp

if (arr) {
     // The fact that dynamic cast has failed should not imply that
     // the input object was null.
     return obj->foo(); // no-warning
   }
 }
 
 void check_dynamic_cast_null_branch(OSObject *obj) {
   OSArray *arr1 = OSArray::withCapacity(10); // expected-note{{Call to method 'OSArray::withCapacity' returns an OSObject}}
-  OSArray *arr = OSDynamicCast(OSArray, obj);
+  OSArray *arr = OSDynamicCast(OSArray, obj); // expected-note{{Assuming dynamic cast returns null due to type mismatch}}
   if (!arr) // expected-note{{Taking true branch}}
     return; // expected-warning{{Potential leak of an object stored into 'arr1'}}
             // expected-note@-1{{Object leaked}}
   arr1->release();
 }
 
 void check_dynamic_cast_null_check() {
   OSArray *arr = OSDynamicCast(OSArray, OSObject::generateObject(1)); // expected-note{{Call to method 'OSObject::generateObject' returns an OSObject}}
     // expected-warning@-1{{Potential leak of an object}}
     // expected-note@-2{{Object leaked}}
+    // expected-note@-3{{Assuming dynamic cast returns null due to type mismatch}}
   if (!arr)
     return;
   arr->release();
 }
 
 void use_after_release() {
   OSArray *arr = OSArray::withCapacity(10); // expected-note{{Call to method 'OSArray::withCapacity' returns an OSObject of type 'OSArray' with a +1 retain count}}
   arr->release(); // expected-note{{Object released}}