[analyzer] New option to not suppress null return paths if an argument is null.

Our one basic suppression heuristic is to assume that functions do not
usually return NULL. However, when one of the arguments is NULL it is
suddenly much more likely that NULL is a valid return value. In this case,
we don't suppress the report here, but we do attach /another/ visitor to
go find out if this NULL argument also comes from an inlined function's
error path.

This new behavior, controlled by the 'avoid-suppressing-null-argument-paths'
analyzer-config option, is turned off by default. Turning it on produced
two false positives and no new true positives when running over LLVM/Clang.

This is one of the possible refinements to our suppression heuristics.
<rdar://problem/12350829>

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@166941 91177308-0d34-0410-b5e6-96231b3b80d8

3 files changed, 109 insertions, 17 deletions

diff --git a/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp b/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
index 32073d726d..da88589c86 100644
--- a/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
+++ b/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
@@ -102,6 +102,12 @@ bool AnalyzerOptions::shouldPruneNullReturnPaths() {
                           /* Default = */ true);
 }
 
+bool AnalyzerOptions::shouldAvoidSuppressingNullArgumentPaths() {
+  return getBooleanOption(AvoidSuppressingNullArgumentPaths,
+                          "avoid-suppressing-null-argument-paths",
+                          /* Default = */ false);
+}
+
 int AnalyzerOptions::getOptionAsInteger(StringRef Name, int DefaultVal) {
   llvm::SmallString<10> StrBuf;
   llvm::raw_svector_ostream OS(StrBuf);
diff --git a/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp b/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
index dace7f3c8b..328e8a650d 100644
--- a/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ b/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -135,10 +135,15 @@ namespace {
 /// interesting value comes from an inlined function call.
 class ReturnVisitor : public BugReporterVisitorImpl<ReturnVisitor> {
   const StackFrameContext *StackFrame;
-  bool Satisfied;
+  enum {
+    Initial,
+    MaybeSuppress,
+    Satisfied
+  } Mode;
+
 public:
   ReturnVisitor(const StackFrameContext *Frame)
-    : StackFrame(Frame), Satisfied(false) {}
+    : StackFrame(Frame), Mode(Initial) {}
 
   static void *getTag() {
     static int Tag = 0;
@@ -190,13 +195,16 @@ public:
     }
   }
 
-  PathDiagnosticPiece *VisitNode(const ExplodedNode *N,
-                                 const ExplodedNode *PrevN,
-                                 BugReporterContext &BRC,
-                                 BugReport &BR) {
-    if (Satisfied)
-      return 0;
+  /// Returns true if any counter-suppression heuristics are enabled for
+  /// ReturnVisitor.
+  static bool hasCounterSuppression(AnalyzerOptions &Options) {
+    return Options.shouldAvoidSuppressingNullArgumentPaths();
+  }
 
+  PathDiagnosticPiece *visitNodeInitial(const ExplodedNode *N,
+                                        const ExplodedNode *PrevN,
+                                        BugReporterContext &BRC,
+                                        BugReport &BR) {
     // Only print a message at the interesting return statement.
     if (N->getLocationContext() != StackFrame)
       return 0;
@@ -217,7 +225,7 @@ public:
       return 0;
 
     // Don't print any more notes after this one.
-    Satisfied = true;
+    Mode = Satisfied;
 
     const Expr *RetE = Ret->getRetValue();
     assert(RetE && "Tracking a return value for a void function");
@@ -243,8 +251,13 @@ public:
       // report invalid. We still want to emit a path note, however, in case
       // the report is resurrected as valid later on.
       ExprEngine &Eng = BRC.getBugReporter().getEngine();
-      if (Eng.getAnalysisManager().options.shouldPruneNullReturnPaths())
-        BR.markInvalid(ReturnVisitor::getTag(), StackFrame);
+      AnalyzerOptions &Options = Eng.getAnalysisManager().options;
+      if (Options.shouldPruneNullReturnPaths()) {
+        if (hasCounterSuppression(Options))
+          Mode = MaybeSuppress;
+        else
+          BR.markInvalid(ReturnVisitor::getTag(), StackFrame);
+      }
 
       if (RetE->getType()->isObjCObjectPointerType())
         Out << "Returning nil";
@@ -262,6 +275,74 @@ public:
     PathDiagnosticLocation L(Ret, BRC.getSourceManager(), StackFrame);
     return new PathDiagnosticEventPiece(L, Out.str());
   }
+
+  PathDiagnosticPiece *visitNodeMaybeSuppress(const ExplodedNode *N,
+                                              const ExplodedNode *PrevN,
+                                              BugReporterContext &BRC,
+                                              BugReport &BR) {
+    // Are we at the entry node for this call?
+    const CallEnter *CE = N->getLocationAs<CallEnter>();
+    if (!CE)
+      return 0;
+
+    if (CE->getCalleeContext() != StackFrame)
+      return 0;
+
+    Mode = Satisfied;
+
+    ExprEngine &Eng = BRC.getBugReporter().getEngine();
+    AnalyzerOptions &Options = Eng.getAnalysisManager().options;
+    if (Options.shouldAvoidSuppressingNullArgumentPaths()) {
+      // Don't automatically suppress a report if one of the arguments is
+      // known to be a null pointer. Instead, start tracking /that/ null
+      // value back to its origin.
+      ProgramStateManager &StateMgr = BRC.getStateManager();
+      CallEventManager &CallMgr = StateMgr.getCallEventManager();
+
+      ProgramStateRef State = N->getState();
+      CallEventRef<> Call = CallMgr.getCaller(StackFrame, State);
+      for (unsigned I = 0, E = Call->getNumArgs(); I != E; ++I) {
+        SVal ArgV = Call->getArgSVal(I);
+        if (!isa<Loc>(ArgV))
+          continue;
+
+        const Expr *ArgE = Call->getArgExpr(I);
+        if (!ArgE)
+          continue;
+
+        // Is it possible for this argument to be non-null?
+        if (State->assume(cast<Loc>(ArgV), true))
+          continue;
+
+        if (bugreporter::trackNullOrUndefValue(N, ArgE, BR, /*IsArg=*/true))
+          return 0;
+
+        // If we /can't/ track the null pointer, we should err on the side of
+        // false negatives, and continue towards marking this report invalid.
+        // (We will still look at the other arguments, though.)
+      }
+    }
+
+    // There is no reason not to suppress this report; go ahead and do it.
+    BR.markInvalid(ReturnVisitor::getTag(), StackFrame);
+    return 0;
+  }
+
+  PathDiagnosticPiece *VisitNode(const ExplodedNode *N,
+                                 const ExplodedNode *PrevN,
+                                 BugReporterContext &BRC,
+                                 BugReport &BR) {
+    switch (Mode) {
+    case Initial:
+      return visitNodeInitial(N, PrevN, BRC, BR);
+    case MaybeSuppress:
+      return visitNodeMaybeSuppress(N, PrevN, BRC, BR);
+    case Satisfied:
+      return 0;
+    }
+
+    llvm_unreachable("Invalid visit mode!");
+  }
 };
 } // end anonymous namespace
 
@@ -525,10 +606,10 @@ TrackConstraintBRVisitor::VisitNode(const ExplodedNode *N,
   return NULL;
 }
 
-void bugreporter::trackNullOrUndefValue(const ExplodedNode *N, const Stmt *S,
+bool bugreporter::trackNullOrUndefValue(const ExplodedNode *N, const Stmt *S,
                                         BugReport &report, bool IsArg) {
   if (!S || !N)
-    return;
+    return false;
 
   if (const OpaqueValueExpr *OVE = dyn_cast<OpaqueValueExpr>(S))
     S = OVE->getSourceExpr();
@@ -550,7 +631,7 @@ void bugreporter::trackNullOrUndefValue(const ExplodedNode *N, const Stmt *S,
     } while (N);
 
     if (!N)
-      return;
+      return false;
   }
   
   ProgramStateRef state = N->getState();
@@ -600,7 +681,7 @@ void bugreporter::trackNullOrUndefValue(const ExplodedNode *N, const Stmt *S,
         }
 
         report.addVisitor(new FindLastStoreBRVisitor(V, R));
-        return;
+        return true;
       }
     }
   }
@@ -634,6 +715,8 @@ void bugreporter::trackNullOrUndefValue(const ExplodedNode *N, const Stmt *S,
       S = E->IgnoreParenCasts();
     ReturnVisitor::addVisitorIfNecessary(N, S, report);
   }
+
+  return true;
 }
 
 BugReporterVisitor *
diff --git a/lib/StaticAnalyzer/Core/PathDiagnostic.cpp b/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
index 258ad253bd..0f48d1e1c7 100644
--- a/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
+++ b/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
@@ -586,8 +586,8 @@ PathDiagnosticLocation
     const CFGBlock *BSrc = BE->getSrc();
     S = BSrc->getTerminatorCondition();
   }
-  else if (const PostStmt *PS = dyn_cast<PostStmt>(&P)) {
-    S = PS->getStmt();
+  else if (const StmtPoint *SP = dyn_cast<StmtPoint>(&P)) {
+    S = SP->getStmt();
   }
   else if (const PostImplicitCall *PIE = dyn_cast<PostImplicitCall>(&P)) {
     return PathDiagnosticLocation(PIE->getLocation(), SMng);
@@ -602,6 +602,9 @@ PathDiagnosticLocation
                                 CEE->getLocationContext(),
                                 SMng);
   }
+  else {
+    llvm_unreachable("Unexpected ProgramPoint");
+  }
 
   return PathDiagnosticLocation(S, SMng, P.getLocationContext());
 }