r246818 - [Static Analyzer] Remove sinks from nullability checks.
Gabor Horvath via cfe-commits
cfe-commits at lists.llvm.org
Thu Sep 3 16:16:21 PDT 2015
Author: xazax
Date: Thu Sep 3 18:16:21 2015
New Revision: 246818
URL: http://llvm.org/viewvc/llvm-project?rev=246818&view=rev
Log:
[Static Analyzer] Remove sinks from nullability checks.
Differential Revision: http://reviews.llvm.org/D12445
Modified:
cfe/trunk/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
cfe/trunk/test/Analysis/nullability.mm
Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp?rev=246818&r1=246817&r2=246818&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp Thu Sep 3 18:16:21 2015
@@ -161,6 +161,16 @@ private:
const MemRegion *Region;
};
+ /// When any of the nonnull arguments of the analyzed function is null, do not
+ /// report anything and turn off the check.
+ ///
+ /// When \p SuppressPath is set to true, no more bugs will be reported on this
+ /// path by this checker.
+ void reportBugIfPreconditionHolds(ErrorKind Error, ExplodedNode *N,
+ const MemRegion *Region, CheckerContext &C,
+ const Stmt *ValueExpr = nullptr,
+ bool SuppressPath = false) const;
+
void reportBug(ErrorKind Error, ExplodedNode *N, const MemRegion *Region,
BugReporter &BR, const Stmt *ValueExpr = nullptr) const {
if (!BT)
@@ -220,6 +230,13 @@ bool operator==(NullabilityState Lhs, Nu
REGISTER_MAP_WITH_PROGRAMSTATE(NullabilityMap, const MemRegion *,
NullabilityState)
+// If the nullability precondition of a function is violated, we should not
+// report nullability related issues on that path. For this reason once a
+// precondition is not met on a path, this checker will be esentially turned off
+// for the rest of the analysis. We do not want to generate a sink node however,
+// so this checker would not lead to reduced coverage.
+REGISTER_TRAIT_WITH_PROGRAMSTATE(PreconditionViolated, bool)
+
enum class NullConstraint { IsNull, IsNotNull, Unknown };
static NullConstraint getNullConstraint(DefinedOrUnknownSVal Val,
@@ -302,6 +319,82 @@ static Nullability getNullabilityAnnotat
return Nullability::Unspecified;
}
+template <typename ParamVarDeclRange>
+static bool
+checkParamsForPreconditionViolation(const ParamVarDeclRange &Params,
+ ProgramStateRef State,
+ const LocationContext *LocCtxt) {
+ for (const auto *ParamDecl : Params) {
+ if (ParamDecl->isParameterPack())
+ break;
+
+ if (getNullabilityAnnotation(ParamDecl->getType()) != Nullability::Nonnull)
+ continue;
+
+ auto RegVal = State->getLValue(ParamDecl, LocCtxt)
+ .template getAs<loc::MemRegionVal>();
+ if (!RegVal)
+ continue;
+
+ auto ParamValue = State->getSVal(RegVal->getRegion())
+ .template getAs<DefinedOrUnknownSVal>();
+ if (!ParamValue)
+ continue;
+
+ if (getNullConstraint(*ParamValue, State) == NullConstraint::IsNull) {
+ return true;
+ }
+ }
+ return false;
+}
+
+static bool checkPreconditionViolation(ProgramStateRef State, ExplodedNode *N,
+ CheckerContext &C) {
+ if (State->get<PreconditionViolated>())
+ return true;
+
+ const LocationContext *LocCtxt = C.getLocationContext();
+ const Decl *D = LocCtxt->getDecl();
+ if (!D)
+ return false;
+
+ if (const auto *BlockD = dyn_cast<BlockDecl>(D)) {
+ if (checkParamsForPreconditionViolation(BlockD->parameters(), State,
+ LocCtxt)) {
+ if (!N->isSink())
+ C.addTransition(State->set<PreconditionViolated>(true), N);
+ return true;
+ }
+ return false;
+ }
+
+ if (const auto *FuncDecl = dyn_cast<FunctionDecl>(D)) {
+ if (checkParamsForPreconditionViolation(FuncDecl->parameters(), State,
+ LocCtxt)) {
+ if (!N->isSink())
+ C.addTransition(State->set<PreconditionViolated>(true), N);
+ return true;
+ }
+ return false;
+ }
+ return false;
+}
+
+void NullabilityChecker::reportBugIfPreconditionHolds(
+ ErrorKind Error, ExplodedNode *N, const MemRegion *Region,
+ CheckerContext &C, const Stmt *ValueExpr, bool SuppressPath) const {
+ ProgramStateRef OriginalState = N->getState();
+
+ if (checkPreconditionViolation(OriginalState, N, C))
+ return;
+ if (SuppressPath) {
+ OriginalState = OriginalState->set<PreconditionViolated>(true);
+ N = C.addTransition(OriginalState, N);
+ }
+
+ reportBug(Error, N, Region, C.getBugReporter(), ValueExpr);
+}
+
/// Cleaning up the program state.
void NullabilityChecker::checkDeadSymbols(SymbolReaper &SR,
CheckerContext &C) const {
@@ -314,12 +407,22 @@ void NullabilityChecker::checkDeadSymbol
State = State->remove<NullabilityMap>(I->first);
}
}
+ // When one of the nonnull arguments are constrained to be null, nullability
+ // preconditions are violated. It is not enough to check this only when we
+ // actually report an error, because at that time interesting symbols might be
+ // reaped.
+ if (checkPreconditionViolation(State, C.getPredecessor(), C))
+ return;
+ C.addTransition(State);
}
/// This callback triggers when a pointer is dereferenced and the analyzer does
/// not know anything about the value of that pointer. When that pointer is
/// nullable, this code emits a warning.
void NullabilityChecker::checkEvent(ImplicitNullDerefEvent Event) const {
+ if (Event.SinkNode->getState()->get<PreconditionViolated>())
+ return;
+
const MemRegion *Region =
getTrackRegion(Event.Location, /*CheckSuperregion=*/true);
if (!Region)
@@ -335,6 +438,8 @@ void NullabilityChecker::checkEvent(Impl
if (Filter.CheckNullableDereferenced &&
TrackedNullability->getValue() == Nullability::Nullable) {
BugReporter &BR = *Event.BR;
+ // Do not suppress errors on defensive code paths, because dereferencing
+ // a nullable pointer is always an error.
if (Event.IsDirectDereference)
reportBug(ErrorKind::NullableDereferenced, Event.SinkNode, Region, BR);
else
@@ -358,6 +463,9 @@ void NullabilityChecker::checkPreStmt(co
return;
ProgramStateRef State = C.getState();
+ if (State->get<PreconditionViolated>())
+ return;
+
auto RetSVal =
State->getSVal(S, C.getLocationContext()).getAs<DefinedOrUnknownSVal>();
if (!RetSVal)
@@ -378,9 +486,9 @@ void NullabilityChecker::checkPreStmt(co
Nullness == NullConstraint::IsNull &&
StaticNullability == Nullability::Nonnull) {
static CheckerProgramPointTag Tag(this, "NullReturnedFromNonnull");
- ExplodedNode *N = C.addTransition(State, C.getPredecessor(), &Tag);
- reportBug(ErrorKind::NilReturnedToNonnull, N, nullptr, C.getBugReporter(),
- S);
+ ExplodedNode *N = C.generateSink(State, C.getPredecessor(), &Tag);
+ reportBugIfPreconditionHolds(ErrorKind::NilReturnedToNonnull, N, nullptr, C,
+ RetExpr);
return;
}
@@ -398,8 +506,8 @@ void NullabilityChecker::checkPreStmt(co
StaticNullability == Nullability::Nonnull) {
static CheckerProgramPointTag Tag(this, "NullableReturnedFromNonnull");
ExplodedNode *N = C.addTransition(State, C.getPredecessor(), &Tag);
- reportBug(ErrorKind::NullableReturnedToNonnull, N, Region,
- C.getBugReporter());
+ reportBugIfPreconditionHolds(ErrorKind::NullableReturnedToNonnull, N,
+ Region, C);
}
return;
}
@@ -418,6 +526,9 @@ void NullabilityChecker::checkPreCall(co
return;
ProgramStateRef State = C.getState();
+ if (State->get<PreconditionViolated>())
+ return;
+
ProgramStateRef OrigState = State;
unsigned Idx = 0;
@@ -445,10 +556,9 @@ void NullabilityChecker::checkPreCall(co
if (Filter.CheckNullPassedToNonnull && Nullness == NullConstraint::IsNull &&
ArgStaticNullability != Nullability::Nonnull &&
ParamNullability == Nullability::Nonnull) {
- static CheckerProgramPointTag Tag(this, "NullPassedToNonnull");
- ExplodedNode *N = C.generateSink(State, C.getPredecessor(), &Tag);
- reportBug(ErrorKind::NilPassedToNonnull, N, nullptr, C.getBugReporter(),
- ArgExpr);
+ ExplodedNode *N = C.generateSink(State);
+ reportBugIfPreconditionHolds(ErrorKind::NilPassedToNonnull, N, nullptr, C,
+ ArgExpr);
return;
}
@@ -466,18 +576,16 @@ void NullabilityChecker::checkPreCall(co
if (Filter.CheckNullablePassedToNonnull &&
ParamNullability == Nullability::Nonnull) {
- static CheckerProgramPointTag Tag(this, "NullablePassedToNonnull");
- ExplodedNode *N = C.generateSink(State, C.getPredecessor(), &Tag);
- reportBug(ErrorKind::NullablePassedToNonnull, N, Region,
- C.getBugReporter(), ArgExpr);
+ ExplodedNode *N = C.addTransition(State);
+ reportBugIfPreconditionHolds(ErrorKind::NullablePassedToNonnull, N,
+ Region, C, ArgExpr, /*SuppressPath=*/true);
return;
}
if (Filter.CheckNullableDereferenced &&
Param->getType()->isReferenceType()) {
- static CheckerProgramPointTag Tag(this, "NullableDereferenced");
- ExplodedNode *N = C.generateSink(State, C.getPredecessor(), &Tag);
- reportBug(ErrorKind::NullableDereferenced, N, Region,
- C.getBugReporter(), ArgExpr);
+ ExplodedNode *N = C.addTransition(State);
+ reportBugIfPreconditionHolds(ErrorKind::NullableDereferenced, N, Region,
+ C, ArgExpr, /*SuppressPath=*/true);
return;
}
continue;
@@ -507,10 +615,13 @@ void NullabilityChecker::checkPostCall(c
QualType ReturnType = FuncType->getReturnType();
if (!ReturnType->isAnyPointerType())
return;
+ ProgramStateRef State = C.getState();
+ if (State->get<PreconditionViolated>())
+ return;
+
const MemRegion *Region = getTrackRegion(Call.getReturnValue());
if (!Region)
return;
- ProgramStateRef State = C.getState();
// CG headers are misannotated. Do not warn for symbols that are the results
// of CG calls.
@@ -576,11 +687,14 @@ void NullabilityChecker::checkPostObjCMe
if (!RetType->isAnyPointerType())
return;
+ ProgramStateRef State = C.getState();
+ if (State->get<PreconditionViolated>())
+ return;
+
const MemRegion *ReturnRegion = getTrackRegion(M.getReturnValue());
if (!ReturnRegion)
return;
- ProgramStateRef State = C.getState();
auto Interface = Decl->getClassInterface();
auto Name = Interface ? Interface->getName() : "";
// In order to reduce the noise in the diagnostics generated by this checker,
@@ -688,6 +802,10 @@ void NullabilityChecker::checkPostStmt(c
if (!DestType->isAnyPointerType())
return;
+ ProgramStateRef State = C.getState();
+ if (State->get<PreconditionViolated>())
+ return;
+
Nullability DestNullability = getNullabilityAnnotation(DestType);
// No explicit nullability in the destination type, so this cast does not
@@ -695,7 +813,6 @@ void NullabilityChecker::checkPostStmt(c
if (DestNullability == Nullability::Unspecified)
return;
- ProgramStateRef State = C.getState();
auto RegionSVal =
State->getSVal(CE, C.getLocationContext()).getAs<DefinedOrUnknownSVal>();
const MemRegion *Region = getTrackRegion(*RegionSVal);
@@ -744,11 +861,14 @@ void NullabilityChecker::checkBind(SVal
if (!LocType->isAnyPointerType())
return;
+ ProgramStateRef State = C.getState();
+ if (State->get<PreconditionViolated>())
+ return;
+
auto ValDefOrUnknown = V.getAs<DefinedOrUnknownSVal>();
if (!ValDefOrUnknown)
return;
- ProgramStateRef State = C.getState();
NullConstraint RhsNullness = getNullConstraint(*ValDefOrUnknown, State);
Nullability ValNullability = Nullability::Unspecified;
@@ -761,9 +881,9 @@ void NullabilityChecker::checkBind(SVal
ValNullability != Nullability::Nonnull &&
LocNullability == Nullability::Nonnull) {
static CheckerProgramPointTag Tag(this, "NullPassedToNonnull");
- ExplodedNode *N = C.addTransition(State, C.getPredecessor(), &Tag);
- reportBug(ErrorKind::NilAssignedToNonnull, N, nullptr, C.getBugReporter(),
- S);
+ ExplodedNode *N = C.generateSink(State, C.getPredecessor(), &Tag);
+ reportBugIfPreconditionHolds(ErrorKind::NilAssignedToNonnull, N, nullptr, C,
+ S);
return;
}
// Intentionally missing case: '0' is bound to a reference. It is handled by
@@ -784,8 +904,8 @@ void NullabilityChecker::checkBind(SVal
LocNullability == Nullability::Nonnull) {
static CheckerProgramPointTag Tag(this, "NullablePassedToNonnull");
ExplodedNode *N = C.addTransition(State, C.getPredecessor(), &Tag);
- reportBug(ErrorKind::NullableAssignedToNonnull, N, ValueRegion,
- C.getBugReporter());
+ reportBugIfPreconditionHolds(ErrorKind::NullableAssignedToNonnull, N,
+ ValueRegion, C);
}
return;
}
Modified: cfe/trunk/test/Analysis/nullability.mm
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/nullability.mm?rev=246818&r1=246817&r2=246818&view=diff
==============================================================================
--- cfe/trunk/test/Analysis/nullability.mm (original)
+++ cfe/trunk/test/Analysis/nullability.mm Thu Sep 3 18:16:21 2015
@@ -179,3 +179,65 @@ void testInvalidPropagation() {
takesNullable(p);
takesNonnull(p);
}
+
+void onlyReportFirstPreconditionViolationOnPath() {
+ Dummy *p = returnsNullable();
+ takesNonnull(p); // expected-warning {{}}
+ takesNonnull(p); // No warning.
+ // The first warning was not a sink. The analysis expected to continue.
+ int i = 0;
+ i = 5 / i; // expected-warning {{Division by zero}}
+ (void)i;
+}
+
+Dummy *_Nonnull doNotWarnWhenPreconditionIsViolatedInTopFunc(
+ Dummy *_Nonnull p) {
+ if (!p) {
+ Dummy *ret =
+ 0; // avoid compiler warning (which is not generated by the analyzer)
+ if (getRandom())
+ return ret; // no warning
+ else
+ return p; // no warning
+ } else {
+ return p;
+ }
+}
+
+Dummy *_Nonnull doNotWarnWhenPreconditionIsViolated(Dummy *_Nonnull p) {
+ if (!p) {
+ Dummy *ret =
+ 0; // avoid compiler warning (which is not generated by the analyzer)
+ if (getRandom())
+ return ret; // no warning
+ else
+ return p; // no warning
+ } else {
+ return p;
+ }
+}
+
+void testPreconditionViolationInInlinedFunction(Dummy *p) {
+ doNotWarnWhenPreconditionIsViolated(p);
+}
+
+void inlinedNullable(Dummy *_Nullable p) {
+ if (p) return;
+}
+void inlinedNonnull(Dummy *_Nonnull p) {
+ if (p) return;
+}
+void inlinedUnspecified(Dummy *p) {
+ if (p) return;
+}
+
+Dummy *_Nonnull testDefensiveInlineChecks(Dummy * p) {
+ switch (getRandom()) {
+ case 1: inlinedNullable(p); break;
+ case 2: inlinedNonnull(p); break;
+ case 3: inlinedUnspecified(p); break;
+ }
+ if (getRandom())
+ takesNonnull(p);
+ return p;
+}
More information about the cfe-commits
mailing list