[analyzer] Address Jordan’s review of r178309 - do not register an extra visitor for nil receiver
We can check if the receiver is nil in the node that corresponds to the StmtPoint of the message send.
At that point, the receiver is guaranteed to be live. We will find at least one unreclaimed node due to
my previous commit (look for StmtPoint instead of PostStmt) and the fact that the nil receiver nodes are tagged.
+ a couple of extra tests.
git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@178381 91177308-0d34-0410-b5e6-96231b3b80d8
diff --git a/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h b/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h
index 6eb5f25..2e5f207 100644
--- a/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h
+++ b/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h
@@ -160,25 +160,11 @@
/// \brief Prints path notes when a message is sent to a nil receiver.
class NilReceiverBRVisitor
: public BugReporterVisitorImpl<NilReceiverBRVisitor> {
-
- /// \brief The reciever to track. If null, all receivers should be tracked.
- const Expr *TrackedReceiver;
-
- /// If the visitor is tracking the value directly responsible for the
- /// bug, we are going to employ false positive suppression.
- bool EnableNullFPSuppression;
-
public:
- NilReceiverBRVisitor(const Expr *InTrackedReceiver = 0,
- bool InEnableNullFPSuppression = false):
- TrackedReceiver(InTrackedReceiver),
- EnableNullFPSuppression(InEnableNullFPSuppression) {}
void Profile(llvm::FoldingSetNodeID &ID) const {
static int x = 0;
ID.AddPointer(&x);
- ID.AddPointer(TrackedReceiver);
- ID.AddBoolean(EnableNullFPSuppression);
}
PathDiagnosticPiece *VisitNode(const ExplodedNode *N,
@@ -186,7 +172,9 @@
BugReporterContext &BRC,
BugReport &BR);
- static const Expr *getReceiver(const Stmt *S);
+ /// If the statement is a message send expression with nil receiver, returns
+ /// the receiver expression. Returns NULL otherwise.
+ static const Expr *getNilReceiver(const Stmt *S, const ExplodedNode *N);
};
/// Visitor that tries to report interesting diagnostics from conditions.
diff --git a/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp b/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
index 11218c0..241388d 100644
--- a/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ b/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -824,12 +824,6 @@
S = Ex;
}
- // The message send could be null if the receiver is null.
- if (const Expr *Receiver = NilReceiverBRVisitor::getReceiver(S)) {
- report.addVisitor(new NilReceiverBRVisitor(Receiver,
- EnableNullFPSuppression));
- }
-
const Expr *Inner = 0;
if (const Expr *Ex = dyn_cast<Expr>(S)) {
Ex = Ex->IgnoreParenCasts();
@@ -862,7 +856,14 @@
ProgramStateRef state = N->getState();
- // See if the expression we're interested refers to a variable.
+ // The message send could be nil due to the receiver being nil.
+ // At this point in the path, the receiver should be live since we are at the
+ // message send expr. If it is nil, start tracking it.
+ if (const Expr *Receiver = NilReceiverBRVisitor::getNilReceiver(S, N))
+ trackNullOrUndefValue(N, Receiver, report, IsArg, EnableNullFPSuppression);
+
+
+ // See if the expression we're interested refers to a variable.
// If so, we can track both its contents and constraints on its value.
if (Inner && ExplodedGraph::isInterestingLValueExpr(Inner)) {
const MemRegion *R = 0;
@@ -985,11 +986,18 @@
return true;
}
-const Expr *NilReceiverBRVisitor::getReceiver(const Stmt *S) {
+const Expr *NilReceiverBRVisitor::getNilReceiver(const Stmt *S,
+ const ExplodedNode *N) {
const ObjCMessageExpr *ME = dyn_cast<ObjCMessageExpr>(S);
if (!ME)
return 0;
- return ME->getInstanceReceiver();
+ if (const Expr *Receiver = ME->getInstanceReceiver()) {
+ ProgramStateRef state = N->getState();
+ SVal V = state->getSVal(Receiver, N->getLocationContext());
+ if (state->isNull(V).isConstrainedTrue())
+ return Receiver;
+ }
+ return 0;
}
PathDiagnosticPiece *NilReceiverBRVisitor::VisitNode(const ExplodedNode *N,
@@ -1000,24 +1008,15 @@
if (!P)
return 0;
- const Expr *Receiver = getReceiver(P->getStmt());
+ const Expr *Receiver = getNilReceiver(P->getStmt(), N);
if (!Receiver)
return 0;
- // Are we tracking a different reciever?
- if (TrackedReceiver && TrackedReceiver != Receiver)
- return 0;
-
- ProgramStateRef state = N->getState();
- SVal V = state->getSVal(Receiver, N->getLocationContext());
- if (!state->isNull(V).isConstrainedTrue())
- return 0;
-
// The receiver was nil, and hence the method was skipped.
// Register a BugReporterVisitor to issue a message telling us how
// the receiver was null.
bugreporter::trackNullOrUndefValue(N, Receiver, BR, /*IsArg*/ false,
- EnableNullFPSuppression);
+ /*EnableNullFPSuppression*/ false);
// Issue a message saying that the method was skipped.
PathDiagnosticLocation L(Receiver, BRC.getSourceManager(),
N->getLocationContext());
diff --git a/test/Analysis/inlining/inline-defensive-checks.m b/test/Analysis/inlining/inline-defensive-checks.m
index a757af3..0404ee6 100644
--- a/test/Analysis/inlining/inline-defensive-checks.m
+++ b/test/Analysis/inlining/inline-defensive-checks.m
@@ -76,6 +76,22 @@
return mem->x; // expected-warning {{Access to instance variable 'x' results in a dereference of a null pointer}}
}
+int suppressNilReceiverRetNullCond(Foo* fPtr) {
+ unsigned zero = 0;
+ fPtr = retInputOrNil(fPtr);
+ // On a path where fPtr is nzil, mem should be nil.
+ Foo *mem = [fPtr getFooPtr];
+ return mem->x;
+}
+
+int suppressNilReceiverRetNullCondCast(id fPtr) {
+ unsigned zero = 0;
+ fPtr = retInputOrNil(fPtr);
+ // On a path where fPtr is nzil, mem should be nil.
+ Foo *mem = ((id)([(Foo*)(fPtr) getFooPtr]));
+ return mem->x;
+}
+
int dontSuppressNilReceiverRetNullCond(Foo* fPtr) {
unsigned zero = 0;
fPtr = retInputOrNil(fPtr);
