Improve NULL-checking for CFRetain/CFRelease. We now remember that the argument was non-NULL, and we report where the null assumption came from (like AttrNonNullChecker already did).

llvm-svn: 107633

clang/lib/Checker/BasicObjCFoundationChecks.cpp

@@ -415,59 +415,72 @@ clang::CreateAuditCFNumberCreate(ASTContext& Ctx, BugReporter& BR) {
 }
 
 //===----------------------------------------------------------------------===//
-// CFRetain/CFRelease auditing for null arguments.
+// CFRetain/CFRelease checking for null arguments.
 //===----------------------------------------------------------------------===//
 
 namespace {
-class AuditCFRetainRelease : public GRSimpleAPICheck {
+class CFRetainReleaseChecker : public CheckerVisitor<CFRetainReleaseChecker> {
   APIMisuse *BT;
-
-  // FIXME: Either this should be refactored into GRSimpleAPICheck, or
-  //   it should always be passed with a call to Audit.  The latter
-  //   approach makes this class more stateless.
-  ASTContext& Ctx;
   IdentifierInfo *Retain, *Release;
-  BugReporter& BR;
 
 public:
-  AuditCFRetainRelease(ASTContext& ctx, BugReporter& br)
-  : BT(0), Ctx(ctx),
-    Retain(&Ctx.Idents.get("CFRetain")), Release(&Ctx.Idents.get("CFRelease")),
-    BR(br){}
+  CFRetainReleaseChecker(ASTContext& Ctx): BT(NULL),
+    Retain(&Ctx.Idents.get("CFRetain")), Release(&Ctx.Idents.get("CFRelease"))
+    {}
 
-  ~AuditCFRetainRelease() {}
+  static void *getTag() { static int x = 0; return &x; }
 
-  bool Audit(ExplodedNode* N, GRStateManager&);
+  void PreVisitCallExpr(CheckerContext& C, const CallExpr* CE);
 };
 } // end anonymous namespace
 
 
-bool AuditCFRetainRelease::Audit(ExplodedNode* N, GRStateManager&) {
-  const CallExpr* CE = cast<CallExpr>(cast<PostStmt>(N->getLocation()).getStmt());
-
+void CFRetainReleaseChecker::PreVisitCallExpr(CheckerContext& C,
+                                              const CallExpr* CE) {
   // If the CallExpr doesn't have exactly 1 argument just give up checking.
   if (CE->getNumArgs() != 1)
-    return false;
+    return;
 
-  // Check if we called CFRetain/CFRelease.
-  const GRState* state = N->getState();
+  // Get the function declaration of the callee.
+  const GRState* state = C.getState();
   SVal X = state->getSVal(CE->getCallee());
   const FunctionDecl* FD = X.getAsFunctionDecl();
 
   if (!FD)
-    return false;
+    return;
 
+  // Check if we called CFRetain/CFRelease.
   const IdentifierInfo *FuncII = FD->getIdentifier();
   if (!(FuncII == Retain || FuncII == Release))
-    return false;
+    return;
+
+  // FIXME: The rest of this just checks that the argument is non-null.
+  // It should probably be refactored and combined with AttrNonNullChecker.
+
+  // Get the argument's value.
+  const Expr *Arg = CE->getArg(0);
+  SVal ArgVal = state->getSVal(Arg);
+  DefinedSVal *DefArgVal = dyn_cast<DefinedSVal>(&ArgVal);
+  if (!DefArgVal)
+    return;
+
+  // Get a NULL value.
+  ValueManager &ValMgr = C.getValueManager();
+  DefinedSVal Zero = cast<DefinedSVal>(ValMgr.makeZeroVal(Arg->getType()));
+
+  // Make an expression asserting that they're equal.
+  SValuator &SVator = ValMgr.getSValuator();
+  DefinedOrUnknownSVal ArgIsNull = SVator.EvalEQ(state, Zero, *DefArgVal);
+
+  // Are they equal?
+  const GRState *stateTrue, *stateFalse;
+  llvm::tie(stateTrue, stateFalse) = state->Assume(ArgIsNull);
+
+  if (stateTrue && !stateFalse) {
+    ExplodedNode *N = C.GenerateSink(stateTrue);
+    if (!N)
+      return;
 
-  // Finally, check if the argument is NULL.
-  // FIXME: We should be able to bifurcate the state here, as a successful
-  // check will result in the value not being NULL afterwards.
-  // FIXME: Need a way to register vistors for the BugReporter.  Would like
-  // to benefit from the same diagnostics that regular null dereference
-  // reporting has.
-  if (state->getStateManager().isEqual(state, CE->getArg(0), 0)) {
     if (!BT)
       BT = new APIMisuse("null passed to CFRetain/CFRelease");
 
@@ -475,19 +488,16 @@ bool AuditCFRetainRelease::Audit(ExplodedNode* N, GRStateManager&) {
                             ? "Null pointer argument in call to CFRetain"
                             : "Null pointer argument in call to CFRelease";
 
-    RangedBugReport *report = new RangedBugReport(*BT, description, N);
-    report->addRange(CE->getArg(0)->getSourceRange());
-    BR.EmitReport(report);
-    return true;
+    EnhancedBugReport *report = new EnhancedBugReport(*BT, description, N);
+    report->addRange(Arg->getSourceRange());
+    report->addVisitorCreator(bugreporter::registerTrackNullOrUndefValue, Arg);
+
+    C.EmitReport(report);
+    return;
   }
 
-  return false;
-}
-
-
-GRSimpleAPICheck*
-clang::CreateAuditCFRetainRelease(ASTContext& Ctx, BugReporter& BR) {
-  return new AuditCFRetainRelease(Ctx, BR);
+  // From here on, we know the argument is non-null.
+  C.addTransition(stateFalse);
 }
 
 //===----------------------------------------------------------------------===//
@@ -569,9 +579,10 @@ void clang::RegisterAppleChecks(GRExprEngine& Eng, const Decl &D) {
   Eng.AddCheck(CreateBasicObjCFoundationChecks(Ctx, BR),
                Stmt::ObjCMessageExprClass);
   Eng.AddCheck(CreateAuditCFNumberCreate(Ctx, BR), Stmt::CallExprClass);
-  Eng.AddCheck(CreateAuditCFRetainRelease(Ctx, BR), Stmt::CallExprClass);
 
   RegisterNSErrorChecks(BR, Eng, D);
   RegisterNSAutoreleasePoolChecks(Eng);
+
+  Eng.registerCheck(new CFRetainReleaseChecker(Ctx));
   Eng.registerCheck(new ClassReleaseChecker(Ctx));
 }

clang/lib/Checker/BasicObjCFoundationChecks.h

@@ -30,9 +30,6 @@ GRSimpleAPICheck *CreateBasicObjCFoundationChecks(ASTContext& Ctx,
 GRSimpleAPICheck *CreateAuditCFNumberCreate(ASTContext& Ctx,
                                             BugReporter& BR);
 
-GRSimpleAPICheck *CreateAuditCFRetainRelease(ASTContext& Ctx,
-                                             BugReporter& BR);
-
 void RegisterNSErrorChecks(BugReporter& BR, GRExprEngine &Eng, const Decl &D);
 void RegisterNSAutoreleasePoolChecks(GRExprEngine &Eng);
 

clang/test/Analysis/retain-release.m

@@ -468,6 +468,20 @@ void f16(int x, CFTypeRef p) {
   }
 }
 
+// Test that an object is non-null after being CFRetained/CFReleased.
+void f17(int x, CFTypeRef p) {
+	if (x) {
+		CFRelease(p);
+		if (!p)
+			CFRelease(0); // no-warning
+	}
+	else {
+		CFRetain(p);
+		if (!p)
+			CFRetain(0); // no-warning
+	}
+}
+
 // Test basic tracking of ivars associated with 'self'.  For the retain/release
 // checker we currently do not want to flag leaks associated with stores
 // of tracked objects to ivars.