[analyzer] ReverseNull: New checker to warn for pointer value conditions, if the pointer value is unconditionally non-null

[Szelethus]

Summary

Loosely tied to EXP34-C.

Suppose we do a pointer operation, such as dereferencing, or passing it to a function whose parameter is attributed with non null (like _Nonnull or clang::nonnull). Then, we write an if branch whose condition is this very pointer.

void tp1(int *p) {
  *p = 5;
  // expected-note@-1{{Pointer assumed non-null here}}
  // expected-note@-2{{Consider moving the condition here}}
  if (p)
    // expected-note@-1{{Pointer is unconditionally non-null here}}
    // expected-warning@-2{{Pointer is unconditionally non-null here [alpha.core.ReverseNull]}}
    return;
}

This is a sign of code smell: We either inteded to place the condition before the operation that already expects the pointer to be non-null, or there are some other high level issues in our code. The patch adds a new checker to find and warn for similar cases.

One of the fundamental ideas here is that this is an interprocedural checker. We want to ensure that the derefence (or similar operation) unconditionally leads to the condition without interleaving assignments. This sounds a lot like a dataflow analysis! Indeed, it would be great if we had a dataflow framework already in the bag, and although there is a work on it (a lot of patches can be seen on @ymandel's profile: https://reviews.llvm.org/people/revisions/17749/), its not quite there yet.

Another thought could be, that this checker is (somewhat) redundant with DeadCodeChecker, because this checker also finds dead code, but restricted to pointers. This wasn't a deterrent in developing this alpha checker, because if an 'else' branch is not present, the dead code will not be found (despite the sode smell being there), and restricting ourselves to pointer analysis takes a lot of weight off from our constraint solvers.

I've ran the checker on a lot of projects, and the results so far look promising, and I'm looking forward to publishing them ASAP.

[steakhal]

In addition to the excellent summary, I'd like to highlight that this is intended to catch only the cases where the use of the constrained pointer is in the very same stack frame where it was constrained. This leads to a really nice property: local reasoning, which greatly reduces the number of false-positives.

This should be emphasized in the doc segment of the checker as well.

[steakhal]

Looks great. I'm loving it!
BTW what is the semantics of [p retain] in ObjC? Can p be null in that context? Or does it count as a dereferences, hence it constraints the pointer?

Why did you touch the AnalysisOrderChecker, should we separate those changes?

Additionally, why do you think it needs to be in the alpha.core package instead of the core?
What sort of false-positives have you seen in the wild justifying that classification?

clang/docs/analyzer/checkers.rst

1703

clang/lib/StaticAnalyzer/Checkers/NullPtrInterferenceChecker.cpp

196–198

I think additionally to this, you should also check for the type of the Condition expression. It's gotta be a pointer.

211–216

IMO we should catch these as well:

void store(int *q, int value) {
  *q = value;
}

void top(int *p) {
  store(p, 5);
  if (!p)
    return;
}

In this case, the stack-frame in which the pointer gets constrained is not the same as where the redundant check resides - rather a child frame of that.

clang/test/Analysis/null-pointer-interference.c

18

"before this expression"?
The term here is not well defined IMO.

[xazax.hun]

clang/lib/StaticAnalyzer/Checkers/NullPtrInterferenceChecker.cpp

166

Consider the following code snippet:

void f(int *p, bool b)
{
  if (b) {
    *p = 4;
  }
  if (p) {
   ...
  }
}

I suspect that we would get a warning for the code above. I think warning on the code above might be reasonable (the values of b and p might be correlated but in some cases the analyzer has no way to know this, probably some assertions could make the code clearer in that case).

My problem is with the wording of the error message.
The warning Pointer is unconditionally non-null here on the null check is not true for the code above.

[xazax.hun]

clang/lib/StaticAnalyzer/Checkers/NullPtrInterferenceChecker.cpp

166

Also, if the check would warn for the code snippet above, the note "suggest moving the condition here" would also be incorrect.

[Szelethus]

clang/lib/StaticAnalyzer/Checkers/NullPtrInterferenceChecker.cpp

166

What if we demand that the the CFGBlock of the dereference must dominate the CFGBlock of the condition point?

[xazax.hun]

clang/lib/StaticAnalyzer/Checkers/NullPtrInterferenceChecker.cpp

166

I think it makes sense to warn both when the dereference dominates the null check, and when the null check post-dominates the dereference. We just want to give different error messages in those cases.

[NoQ]

This check checks must-properties/all-paths properties. This has to be a data flow / CFG-based warning. I don't think there's a way around.

clang/lib/StaticAnalyzer/Checkers/NullPtrInterferenceChecker.cpp

166

What if we demand that the the CFGBlock of the dereference must dominate the CFGBlock of the condition point?

*p = 4;
if (b) {
  p = bar();
}
if (p) {
  ...
}

[NoQ]

In D120992#3359864, @steakhal wrote:

BTW what is the semantics of [p retain] in ObjC? Can p be null in that context? Or does it count as a dereferences, hence it constraints the pointer?

In Objective-C "methods" (messages) can be "called on" (sent to) null pointers. The well-defined result is that nothing happens and null is returned. If the method returns an integer, a zero is returned. If the method returns a structure by value, a zero-filled structure is returned. In Objective-C++, if the method returns a C++ object by value, a zero-filled object is returned (without calling the constructor; in particular, the object may be ill-formed from the start).

[xazax.hun]

clang/lib/StaticAnalyzer/Checkers/NullPtrInterferenceChecker.cpp

166

Yup, this is a nice example. I cannot think of an easy way around this using symbolic execution.

[steakhal]

clang/lib/StaticAnalyzer/Checkers/NullPtrInterferenceChecker.cpp

166

Ah, I see. However, empirically, the checker showed really promising results.

[Szelethus]

In D120992#3364804, @NoQ wrote:

This check checks must-properties/all-paths properties. This has to be a data flow / CFG-based warning. I don't think there's a way around.

Oof. I concede on that. Do you think there is any point in turning this into an optin checker? Or the philosophy should be that this is just way too wild?

[NoQ]

I guess there's the usual direction that I occasionally suggest: develop a way to verify that all possible paths were explored during symbolic execution (CoreEngine::hasWorkRemaining() on steroids), then do most of the work in checkEndAnalysis.

We also already have alpha.core.TestAfterDivZero that implements a somewhat similar check-after-use strategy.

Do you think there is any point in turning this into an optin checker? Or the philosophy should be that this is just way too wild?

We made such trade-offs in the past, the most obvious one being our strategy to always split paths on every if-statement and never merge. In particular, our core null dereference checker suffers a lot from this, and it's a major point of criticism we regularly receive from our users.

What I'm trying to say here is, I honestly think re-doing it as CFG-based should be relatively easy. We couldn't make any progress at all without those state splits but in this case it's much less of a fundamental issue so I think it's not worth it to hard-commit to a flawed solution from the start.

In fact you already have one of the building blocks:

What if we demand that the the CFGBlock of the dereference must dominate the CFGBlock of the condition point?

I mean, yes, this is a good strategy, you could implement that and then remove symbolic execution. Another building block could be "there aren't any writes into (or escapes of) that local variable between the use and the check". So it looks like this is entirely about statements of certain kind appearing in certain order in the CFG.

Another bonus from CFG-based checkers is that they can be turned into compiler warnings - which may affect a lot more users. I actually don't know why dead stores checker isn't a compiler warning.

[xazax.hun]

In D120992#3368118, @NoQ wrote:

I guess there's the usual direction that I occasionally suggest: develop a way to verify that all possible paths were explored during symbolic execution (CoreEngine::hasWorkRemaining() on steroids), then do most of the work in checkEndAnalysis.

I agree that having infrastructure for that would be useful. And I also agree that it might be overkill for this particular warning.

In fact you already have one of the building blocks:

The dataflow analysis framework from google is also starting to shape up quite nicely. It already has a lightweight modeling of the memory that could be used to check if the pointer was changed between two program points.

I actually don't know why dead stores checker isn't a compiler warning.

My guess would be simply no-one submitted a patch doing that.

[Szelethus]

Very well :) Let's abandon this in its current state, I share this sentiment:

In D120992#3368118, @NoQ wrote:

What I'm trying to say here is, I honestly think re-doing it as CFG-based should be relatively easy. We couldn't make any progress at all without those state splits but in this case it's much less of a fundamental issue so I think it's not worth it to hard-commit to a flawed solution from the start.

clang/docs/analyzer/checkers.rst

 
 .. code-block:: c
 
  void test() {
    int x, y;
    int d = &y - &x; // warn
  }
 
+alpha.core.ReverseNull (C)
+"""""""""""""""""""""""""
+Checks whether a dereferenced (and as such, assumed to be non-null) pointer is
+present in a condition.
+
+.. code-block:: c
+
+  int *get();
+  void top() {
+    int *p = get();
+    int x = *p; // note: pointer assumed non-null here
+    if (p) // warn: pointer already constrained nonnull
+      return;
+  }
+
 .. _alpha-core-SizeofPtr:
 
 alpha.core.SizeofPtr (C)
 """"""""""""""""""""""""
 Warn about unintended use of ``sizeof()`` on pointer expressions.
 
 .. code-block:: c
 

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

def PthreadLockBase : Checker<"PthreadLockBase">,
   Documentation<NotDocumented>,
   Hidden;
 
 def C11LockChecker : Checker<"C11Lock">,
   HelpText<"Simple lock -> unlock checker">,
   Dependencies<[PthreadLockBase]>,
   Documentation<HasAlphaDocumentation>;
 
+def ReverseNullChecker : Checker<"ReverseNull">,
+  HelpText<"Checks whether a pointer that is already known to null or non-null "
+           "is present in a condition.">,
+  Documentation<HasAlphaDocumentation>;
+
 } // end "alpha.core"
 
 //===----------------------------------------------------------------------===//
 // Nullability checkers.
 //===----------------------------------------------------------------------===//
 
 let ParentPackage = Nullability in {
 
CmdLineOption<Boolean,
                   Hide>,
     CmdLineOption<Boolean,
                   "EvalCall",
                   "",
                   "false",
                   Released,
                   Hide>,
     CmdLineOption<Boolean,
+                  "EvalAssume",
+                  "",
+                  "false",
+                  Released,
+                  Hide>,
+    CmdLineOption<Boolean,
                   "PreCall",
                   "",
                   "false",
                   Released,
                   Hide>,
     CmdLineOption<Boolean,
                   "PostCall",
                   "",

clang/lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp

: public Checker<
           check::PreStmt<ArraySubscriptExpr>,
           check::PostStmt<ArraySubscriptExpr>, check::PreStmt<CXXNewExpr>,
           check::PostStmt<CXXNewExpr>, check::PreStmt<CXXDeleteExpr>,
           check::PostStmt<CXXDeleteExpr>, check::PreStmt<CXXConstructExpr>,
           check::PostStmt<CXXConstructExpr>, check::PreStmt<OffsetOfExpr>,
           check::PostStmt<OffsetOfExpr>, check::PreCall, check::PostCall,
           check::EndFunction, check::EndAnalysis, check::NewAllocator,
           check::Bind, check::PointerEscape, check::RegionChanges,
-          check::LiveSymbols, eval::Call> {
+          check::LiveSymbols, eval::Call, eval::Assume> {
 
   bool isCallbackEnabled(const AnalyzerOptions &Opts,
                          StringRef CallbackName) const {
     return Opts.getCheckerBooleanOption(this, "*") ||
            Opts.getCheckerBooleanOption(this, CallbackName);
   }
 
   bool isCallbackEnabled(CheckerContext &C, StringRef CallbackName) const {
public:
   ProgramStateRef checkPointerEscape(ProgramStateRef State,
                                      const InvalidatedSymbols &Escaped,
                                      const CallEvent *Call,
                                      PointerEscapeKind Kind) const {
     if (isCallbackEnabled(State, "PointerEscape"))
       llvm::errs() << "PointerEscape\n";
     return State;
   }
+
+  ProgramStateRef evalAssume(ProgramStateRef State, SVal Cond,
+                             bool Assumption) const {
+    if (isCallbackEnabled(State, "EvalAssume"))
+      llvm::errs() << "EvalAssume\n";
+    return State;
+  }
 };
 } // end anonymous namespace
 
 //===----------------------------------------------------------------------===//
 // Registration.
 //===----------------------------------------------------------------------===//
 
 void ento::registerAnalysisOrderChecker(CheckerManager &mgr) {
   mgr.registerChecker<AnalysisOrderChecker>();
 }
 
 bool ento::shouldRegisterAnalysisOrderChecker(const CheckerManager &mgr) {
   return true;
 }

clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt

add_clang_library(clangStaticAnalyzerCheckers
   MPI-Checker/MPIChecker.cpp
   MPI-Checker/MPIFunctionClassifier.cpp
   NSAutoreleasePoolChecker.cpp
   NSErrorChecker.cpp
   NoReturnFunctionChecker.cpp
   NonNullParamChecker.cpp
   NonnullGlobalConstantsChecker.cpp
   NullabilityChecker.cpp
+  NullPtrInterferenceChecker.cpp
   NumberObjectConversionChecker.cpp
   ObjCAtSyncChecker.cpp
   ObjCAutoreleaseWriteChecker.cpp
   ObjCContainersASTChecker.cpp
   ObjCContainersChecker.cpp
   ObjCMissingSuperCallChecker.cpp
   ObjCPropertyChecker.cpp
   ObjCSelfInitChecker.cpp

clang/lib/StaticAnalyzer/Checkers/NullPtrInterferenceChecker.cpp

+//===--- ReverseNullChecker.cpp ----------------------------- -*- C++ -*---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// TODO
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/AST/Stmt.h"
+#include "clang/AST/StmtObjC.h"
+#include "clang/Analysis/AnalysisDeclContext.h"
+#include "clang/Analysis/PathDiagnostic.h"
+#include "clang/Analysis/ProgramPoint.h"
+#include "clang/Basic/AttrKinds.h"
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState_Fwd.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/SymExpr.h"
+#include "llvm/Support/ErrorHandling.h"
+#include <memory>
+
+using namespace clang;
+using namespace ento;
+
+REGISTER_MAP_WITH_PROGRAMSTATE(NonNullConstrainedPtrs, const SymbolRef,
+                               const LocationContext *)
+
+static bool isConstrainedNonNull(ProgramStateRef State, const MemRegion *MR) {
+  return State->isNonNull(loc::MemRegionVal(MR)).isConstrainedTrue();
+}
+
+static bool isNodeBeforeNonNullConstraint(const ExplodedNode *N,
+                                          const MemRegion *MR) {
+  assert(N);
+  assert(N->getFirstPred());
+  return !isConstrainedNonNull(N->getFirstPred()->getState(), MR) &&
+         isConstrainedNonNull(N->getState(), MR);
+}
+
+/// Leave a note about where the constraint to null/non-null was imposed.
+class ReverseNullVisitor : public BugReporterVisitor {
+  /// The memory region that was a part of a condition where its value was
+  /// already known.
+  const MemRegion *MR;
+  bool IsSatisfied = false;
+
+public:
+  ReverseNullVisitor(const MemRegion *MR) : MR(MR) {}
+
+  void Profile(llvm::FoldingSetNodeID &ID) const override {
+    static int x = 0;
+    ID.AddPointer(&x);
+  }
+
+  static void *getTag() {
+    static int Tag = 0;
+    return static_cast<void *>(&Tag);
+  }
+
+  PathDiagnosticPieceRef VisitNode(const ExplodedNode *N,
+                                   BugReporterContext &BC,
+                                   PathSensitiveBugReport &R) override {
+    if (IsSatisfied)
+      return nullptr;
+
+    if (!isNodeBeforeNonNullConstraint(N, MR))
+      return nullptr;
+
+    IsSatisfied = true;
+
+    return std::make_shared<PathDiagnosticEventPiece>(
+        PathDiagnosticLocation{N->getNextStmtForDiagnostics(),
+                               BC.getSourceManager(), N->getLocationContext()},
+        "Pointer assumed non-null here");
+  }
+
+  /// The checker must have made sure that this node actually exists. If we did
+  /// not find it, we must've messed up the visitor. Are we sure that we used
+  /// the same machinery to find the exploded here AND in the checker?
+  virtual void finalizeVisitor(BugReporterContext &BRC,
+                               const ExplodedNode *EndPathNode,
+                               PathSensitiveBugReport &BR) override {
+    assert(IsSatisfied &&
+           "Failed to find the ExplodedNode where the constaint was imposed on "
+           "the condition whose value was known!");
+  }
+};
+
+/// Climb up the ExplodedGraph, and find the ExplodedNode where \p MR was
+/// constrained to be non-null. Ideally, this would be done with a
+/// BugReporterVisitor, but the ExplodedGraph it has access to is NOT the same
+/// as the ExplodedGraph that is constructed during analysis
+/// (see https://reviews.llvm.org/D65379). In fact, it is a linear graph, and
+/// we're specifically interested in branches, hence the unusual approach.
+static const ExplodedNode *getNBeforeNonNullConstraint(const ExplodedNode *N,
+                                                       const MemRegion *MR) {
+
+  assert(!N->getFirstSucc() &&
+         "This should be a leaf of the ExplodedGraph (at this point in the "
+         "analysis)!");
+
+  assert(isConstrainedNonNull(N->getState(), MR) &&
+         "This pointer should be non-null!");
+
+  N = N->getFirstPred();
+
+  // Look for the node where the constraint was imposed.
+  while (N->getFirstPred() && !isNodeBeforeNonNullConstraint(N, MR))
+    N = N->getFirstPred();
+
+  // We failed to find the node. This can happen with ObjC self, which may not
+  // have a node where its non-null, even if it can be non-null.
+  if (!N)
+    return nullptr;
+
+  const ExplodedNode *BeforeNonNullConstraintN = N->getFirstPred();
+  if (!BeforeNonNullConstraintN)
+    return nullptr;
+
+  assert(!isConstrainedNonNull(BeforeNonNullConstraintN->getState(), MR) &&
+         "This is supposed to be the node where the constraint is not yet "
+         "imposed!");
+
+  return BeforeNonNullConstraintN;
+}
+
+/// Check whether the point where the condition was contrained to be
+/// null/non-null was a state split to analyze (at least) two new paths of
+/// execution, or only one. For example, here, p is null on one path, and
+/// non-null on another where the constraints are imposed:
+///
+///   int *p = get();
+///   if (p) // state split, this ExplodedNode will have two children
+///     // p assumed non-null
+///   else
+///     // p assumed non-null
+///   //...
+///   if (p) // Don't warn here, this is a totally valid condition
+///     // ...
+///
+/// But here, on all paths immediately after p's dereference, p is non-null:
+///
+///   int *p = get();
+///   *p = 5; // This ExplodedNode will have a single child
+///   if (p) // Warn here!
+///     // ...
+///
+/// Mind that the node unconditionally leads down a single path, if all but one
+/// child is a sink node.
+static bool unconditionallyLeadsHere(const ExplodedNode *N) {
+  size_t NonSinkNodeCount = llvm::count_if(
+      N->succs(), [](const ExplodedNode *N) { return !N->isSink(); });
+
+  assert(NonSinkNodeCount != 0 &&
+         "At least one node should be non-sinking, because one leads to the "
+         "path of execution the checker currently analyses!");
+
+  // TODO: Actually assert whether this child leads to the node where the currnt
+  // analysis is.
+  return NonSinkNodeCount == 1;
+}
+
+namespace {
+
+class ReverseNullChecker : public Checker<check::BranchCondition> {
+  BugType BT;
+
+public:
+  ReverseNullChecker()
+      : BT(this, "Pointer is unconditionally non-null here", "Reverse null") {}
+
+  void checkBranchCondition(const Stmt *Condition, CheckerContext &Ctx) const {
+    if (isa<ObjCForCollectionStmt>(Condition))
+      return;
+
+    auto Cond = Ctx.getSVal(Condition).getAs<DefinedOrUnknownSVal>();
+    if (!Cond)
+      return;
+
+    const MemRegion *MR = Cond->getAsRegion();
+    if (!MR)
+      return;
+
+    // TODO: Altough not as interesting, we could also check the null case.
+    if (isConstrainedNonNull(Ctx.getState(), MR)) {
+
+      const ExplodedNode *NBeforeConstraint =
+          getNBeforeNonNullConstraint(Ctx.getPredecessor(), MR);
+      if (!NBeforeConstraint)
+        return;
+
+      if (!unconditionallyLeadsHere(NBeforeConstraint))
+        return;
+
+      // We want to be sure that the constraint and the condition are in the
+      // same stackframe. Caller and callee functions' pre/post conditions may
+      // not apply to the caller stackframe. A similar issue is discussed here:
+      // https://discourse.llvm.org/t/static-analyzer-query-why-is-suppress-null-return-paths-enabled-by-default/
+      if (NBeforeConstraint->getLocationContext() != Ctx.getLocationContext())
+        return;
+
+      const ExplodedNode *N = Ctx.generateNonFatalErrorNode(Ctx.getState());
+      if (!N)
+        return;
+
+      std::unique_ptr<PathSensitiveBugReport> R(
+          std::make_unique<PathSensitiveBugReport>(BT, BT.getDescription(), N));
+      R->addNote(
+          "Consider moving the condition here",
+          PathDiagnosticLocation{NBeforeConstraint->getNextStmtForDiagnostics(),
+                                 Ctx.getSourceManager(),
+                                 NBeforeConstraint->getLocationContext()});
+      R->addVisitor<ReverseNullVisitor>(MR);
+      bugreporter::trackExpressionValue(N, cast<Expr>(Condition), *R);
+      Ctx.emitReport(std::move(R));
+    }
+  }
+};
+
+} // namespace
+
+void clang::ento::registerReverseNullChecker(CheckerManager &Mgr) {
+  Mgr.registerChecker<ReverseNullChecker>();
+}
+
+bool clang::ento::shouldRegisterReverseNullChecker(const CheckerManager &mgr) {
+  return true;
+}

clang/test/Analysis/NSString.m

-// RUN: %clang_analyze_cc1 -triple i386-apple-darwin10 -analyzer-checker=core,osx.cocoa.NilArg,osx.cocoa.RetainCount,alpha.core -analyzer-store=region -verify -Wno-objc-root-class %s
-// RUN: %clang_analyze_cc1 -triple i386-apple-darwin10 -analyzer-checker=core,osx.cocoa.NilArg,osx.cocoa.RetainCount,alpha.core -analyzer-store=region -analyzer-config mode=shallow -verify -Wno-objc-root-class %s
-// RUN: %clang_analyze_cc1 -DTEST_64 -triple x86_64-apple-darwin10 -analyzer-checker=core,osx.cocoa.NilArg,osx.cocoa.RetainCount,alpha.core -analyzer-store=region -verify -Wno-objc-root-class %s
-// RUN: %clang_analyze_cc1 -DOSATOMIC_USE_INLINED -triple i386-apple-darwin10 -analyzer-checker=core,osx.cocoa.NilArg,osx.cocoa.RetainCount,alpha.core -analyzer-store=region -verify -Wno-objc-root-class %s
+// RUN: %clang_analyze_cc1 -verify %s\
+// RUN:   -triple i386-apple-darwin10 -Wno-objc-root-class \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=osx.cocoa.NilArg \
+// RUN:   -analyzer-checker=osx.cocoa.RetainCount \
+// RUN:   -analyzer-checker=alpha.core
+
+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN:   -triple i386-apple-darwin10 -Wno-objc-root-class \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=osx.cocoa.NilArg \
+// RUN:   -analyzer-checker=osx.cocoa.RetainCount \
+// RUN:   -analyzer-checker=alpha.core \
+// RUN:   -analyzer-config mode=shallow
+
+// RUN: %clang_analyze_cc1 -verify %s -DTEST_64 \
+// RUN:   -triple x86_64-apple-darwin10 -Wno-objc-root-class \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=osx.cocoa.NilArg \
+// RUN:   -analyzer-checker=osx.cocoa.RetainCount \
+// RUN:   -analyzer-checker=alpha.core
+
+// RUN: %clang_analyze_cc1 -verify %s -DOSATOMIC_USE_INLINED \
+// RUN:   -triple i386-apple-darwin10 -Wno-objc-root-class \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=osx.cocoa.NilArg \
+// RUN:   -analyzer-checker=osx.cocoa.RetainCount \
+// RUN:   -analyzer-checker=alpha.core
 
 //===----------------------------------------------------------------------===//
 // The following code is reduced using delta-debugging from
 // Foundation.h (Mac OS X).
 //
 // It includes the basic definitions for the test cases below.
 // Not directly including Foundation.h directly makes this test case 
 // both svelte and portable to non-Mac platforms.
NSString* f10(void) {
   return s; // no-warning
 }
 
 // Test case for regression reported in <rdar://problem/6452745>.
 // Essentially 's' should not be considered allocated on the false branch.
 // This exercises the 'EvalAssume' logic in GRTransferFuncs (CFRefCount.cpp).
 NSString* f11(CFDictionaryRef dict, const char* key) {
   NSString* s = (NSString*) CFDictionaryGetValue(dict, key);
-  [s retain];
+  [s retain]; // expected-note{{Consider moving the condition here}}
   if (s) {
+    // expected-warning@-1{{Pointer is unconditionally non-null here [alpha.core.ReverseNull]}}
     [s release];
   }
   return 0;
 }
 
 // Test case for passing a tracked object by-reference to a function we
 // don't understand.
 void unknown_function_f12(NSString** s);

clang/test/Analysis/analyzer-config.c

 // CHECK-NEXT: ctu-index-name = externalDefMap.txt
 // CHECK-NEXT: ctu-invocation-list = invocations.yaml
 // CHECK-NEXT: deadcode.DeadStores:ShowFixIts = false
 // CHECK-NEXT: deadcode.DeadStores:WarnForDeadNestedAssignments = true
 // CHECK-NEXT: debug.AnalysisOrder:* = false
 // CHECK-NEXT: debug.AnalysisOrder:Bind = false
 // CHECK-NEXT: debug.AnalysisOrder:EndAnalysis = false
 // CHECK-NEXT: debug.AnalysisOrder:EndFunction = false
+// CHECK-NEXT: debug.AnalysisOrder:EvalAssume = false
 // CHECK-NEXT: debug.AnalysisOrder:EvalCall = false
 // CHECK-NEXT: debug.AnalysisOrder:LiveSymbols = false
 // CHECK-NEXT: debug.AnalysisOrder:NewAllocator = false
 // CHECK-NEXT: debug.AnalysisOrder:PointerEscape = false
 // CHECK-NEXT: debug.AnalysisOrder:PostCall = false
 // CHECK-NEXT: debug.AnalysisOrder:PostStmtArraySubscriptExpr = false
 // CHECK-NEXT: debug.AnalysisOrder:PostStmtCXXConstructExpr = false
 // CHECK-NEXT: debug.AnalysisOrder:PostStmtCXXDeleteExpr = false

clang/test/Analysis/null-pointer-interference.c

+// RUN: %clang_analyze_cc1 -verify %s -analyzer-output=text\
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=apiModeling \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -analyzer-checker=alpha.core.ReverseNull \
+// RUN:   -analyzer-config apiModeling.StdCLibraryFunctions:ModelPOSIX=true \
+// RUN:   -analyzer-config apiModeling.StdCLibraryFunctions:DisplayLoadedSummaries=true \
+// RUN:   2>&1 | FileCheck %s
+
+void clang_analyzer_warnIfReached();
+//===----------------------------------------------------------------------===//
+// True positive test cases.
+//===----------------------------------------------------------------------===//
+
+void tp1(int *p) {
+  *p = 5;
+  // expected-note@-1{{Pointer assumed non-null here}}
+  // expected-note@-2{{Consider moving the condition here}}
+  if (p)
+    // expected-note@-1{{Pointer is unconditionally non-null here}}
+    // expected-warning@-2{{Pointer is unconditionally non-null here [alpha.core.ReverseNull]}}
+    return;
+}
+
+//===----------------------------------------------------------------------===//
+
+void tp2(int *p) {
+  *p = 5;
+  // expected-note@-1{{Pointer assumed non-null here}}
+  // expected-note@-2{{Consider moving the condition here}}
+  int *x = p;
+  // expected-note@-1{{'x' initialized here}}
+  if (x)
+    // expected-note@-1{{Pointer is unconditionally non-null here}}
+    // expected-warning@-2{{Pointer is unconditionally non-null here [alpha.core.ReverseNull]}}
+    return;
+}
+
+//===----------------------------------------------------------------------===//
+
+char *getenv(const char *name);
+long a64l(const char *str64);
+// CHECK: Loaded summary for: char *getenv(const char *name)
+// CHECK: Loaded summary for: long a64l(const char *str64)
+
+void tp3_posix_nonnull_constraint(const char *p) {
+  a64l(p);
+  // expected-note@-1{{Pointer assumed non-null here}}
+  // expected-note@-2{{Consider moving the condition here}}
+  if (p)
+    // expected-note@-1{{Pointer is unconditionally non-null here}}
+    // expected-warning@-2{{Pointer is unconditionally non-null here [alpha.core.ReverseNull]}}
+    return;
+}
+
+//===----------------------------------------------------------------------===//
+
+void tp4_nonnull_constraint(const char *p) {
+  getenv(p);
+  // expected-note@-1{{Pointer assumed non-null here}}
+  // expected-note@-2{{Consider moving the condition here}}
+  if (p)
+    // expected-note@-1{{Pointer is unconditionally non-null here}}
+    // expected-warning@-2{{Pointer is unconditionally non-null here [alpha.core.ReverseNull]}}
+    return;
+}
+
+//===----------------------------------------------------------------------===//
+// True negative test cases.
+//===----------------------------------------------------------------------===//
+
+void tn1_constrain_arg(int *p) {
+  *p = 5;
+}
+
+void tn1_constraint_in_nested_stackframe(int *p) {
+  tn1_constrain_arg(p);
+  if (p)
+    return;
+}
+
+//===----------------------------------------------------------------------===//
+
+void tn2_constraint_in_parent_stackframe(int *p) {
+  if (p)
+    return;
+}
+
+void tn2_caller(int *p) {
+  *p = 5;
+  tn2_constraint_in_parent_stackframe(p);
+}

clang/test/Analysis/null-pointer-interference.m

+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN:   -triple i386-apple-darwin10 -Wno-objc-root-class -fblocks \
+// RUN:   -analyzer-output=text \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=osx.cocoa.NilArg \
+// RUN:   -analyzer-checker=osx.cocoa.RetainCount \
+// RUN:   -analyzer-checker=alpha.core.ReverseNull
+
+#include "Inputs/system-header-simulator-objc.h"
+
+typedef const struct __CFDictionary * CFDictionaryRef;
+const void *CFDictionaryGetValue(CFDictionaryRef theDict, const void *key);
+
+// From NSString.m:
+NSString* f11(CFDictionaryRef dict, const char* key) {
+  NSString* s = (NSString*) CFDictionaryGetValue(dict, key);
+  // expected-note@-1{{'s' initialized here}}
+  [s retain];
+  // expected-note@-1{{Pointer assumed non-null here}}
+  // expected-note@-2{{Consider moving the condition here}}
+  if (s) {
+    // expected-warning@-1{{Pointer is unconditionally non-null here [alpha.core.ReverseNull]}}
+    // expected-note@-2{{Pointer is unconditionally non-null here}}
+    [s release];
+  }
+  return 0;
+}