xazax.hun created this revision.
xazax.hun added reviewers: zaks.anna, dcoughlin, krememek, jordan_rose.
xazax.hun added a subscriber: cfe-commits.
With this patch a nullability violation no longer generates a sink node, so it
does not cause any coverage loss for other checkers.
>From now on, it also do not warns, when a nullability precondition is
>violated. These changes are related, because in order to be able to remove the
>sinks (without generating duplicate reports for the same symbol), there should
>be a mechanism to turn off this checker for a path.
http://reviews.llvm.org/D12445
Files:
lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
test/Analysis/nullability.mm
Index: test/Analysis/nullability.mm
===================================================================
--- test/Analysis/nullability.mm
+++ test/Analysis/nullability.mm
@@ -179,3 +179,44 @@
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);
+}
Index: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
===================================================================
--- lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
+++ lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
@@ -161,6 +161,11 @@
const MemRegion *Region;
};
+ // This overload of report bug reports the bugs conditionally. When a
+ // nullability precondition is violated it will not report any bugs at all.
+ void reportBug(ErrorKind Error, ExplodedNode *N, const MemRegion *Region,
+ CheckerContext &C, const Stmt *ValueExpr = nullptr) const;
+
void reportBug(ErrorKind Error, ExplodedNode *N, const MemRegion *Region,
BugReporter &BR, const Stmt *ValueExpr = nullptr) const {
if (!BT)
@@ -220,6 +225,13 @@
REGISTER_MAP_WITH_PROGRAMSTATE(NullabilityMap, const MemRegion *,
NullabilityState)
+// If the nullability preconditions of a function is violated, we should not
+// report that, a postcondition is violated. For this reason once a precondition
+// is not met on a path, this checker will be esentially turned of for the rest
+// of the analysis. We do not want to generate a sink node however, so this
+// check does not reduce the coverage.
+REGISTER_TRAIT_WITH_PROGRAMSTATE(PreconditionViolated, bool)
+
enum class NullConstraint { IsNull, IsNotNull, Unknown };
static NullConstraint getNullConstraint(DefinedOrUnknownSVal Val,
@@ -302,6 +314,68 @@
return Nullability::Unspecified;
}
+template <typename ParamVarDeclRange>
+static ProgramStateRef
+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 State->set<PreconditionViolated>(true);
+ }
+ }
+ return State;
+}
+
+static ProgramStateRef
+checkPreconditionViolation(ProgramStateRef State,
+ const LocationContext *LocCtxt) {
+ const Decl *D = LocCtxt->getDecl();
+ if (!D)
+ return State;
+
+ if (const auto *BlockD = dyn_cast<BlockDecl>(D)) {
+ return checkParamsForPreconditionViolation(BlockD->parameters(), State,
+ LocCtxt);
+ }
+
+ if (const auto *FuncDecl = dyn_cast<FunctionDecl>(D)) {
+ return checkParamsForPreconditionViolation(FuncDecl->parameters(), State,
+ LocCtxt);
+ }
+ return State;
+}
+
+void NullabilityChecker::reportBug(ErrorKind Error, ExplodedNode *N,
+ const MemRegion *Region, CheckerContext &C,
+ const Stmt *ValueExpr) const {
+ ProgramStateRef OriginalState = N->getState();
+ ProgramStateRef NewState =
+ checkPreconditionViolation(OriginalState, C.getLocationContext());
+ if (NewState != OriginalState) {
+ C.addTransition(NewState, N);
+ return;
+ }
+
+ reportBug(Error, N, Region, C.getBugReporter(), ValueExpr);
+}
+
/// Cleaning up the program state.
void NullabilityChecker::checkDeadSymbols(SymbolReaper &SR,
CheckerContext &C) const {
@@ -314,12 +388,29 @@
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 (!State->get<PreconditionViolated>()) {
+ ProgramStateRef NewState =
+ checkPreconditionViolation(State, C.getLocationContext());
+ if (NewState != State) {
+ NewState = NewState->set<PreconditionViolated>(true);
+ C.addTransition(NewState);
+ 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 +426,8 @@
if (Filter.CheckNullableDereferenced &&
TrackedNullability->getValue() == Nullability::Nullable) {
BugReporter &BR = *Event.BR;
+ // Do not suppress errors on defensive code paths, because dereferencing
+ // null is alwazs an error.
if (Event.IsDirectDereference)
reportBug(ErrorKind::NullableDereferenced, Event.SinkNode, Region, BR);
else
@@ -358,6 +451,9 @@
return;
ProgramStateRef State = C.getState();
+ if (State->get<PreconditionViolated>())
+ return;
+
auto RetSVal =
State->getSVal(S, C.getLocationContext()).getAs<DefinedOrUnknownSVal>();
if (!RetSVal)
@@ -379,8 +475,7 @@
StaticNullability == Nullability::Nonnull) {
static CheckerProgramPointTag Tag(this, "NullReturnedFromNonnull");
ExplodedNode *N = C.addTransition(State, C.getPredecessor(), &Tag);
- reportBug(ErrorKind::NilReturnedToNonnull, N, nullptr, C.getBugReporter(),
- S);
+ reportBug(ErrorKind::NilReturnedToNonnull, N, nullptr, C, S);
return;
}
@@ -398,8 +493,7 @@
StaticNullability == Nullability::Nonnull) {
static CheckerProgramPointTag Tag(this, "NullableReturnedFromNonnull");
ExplodedNode *N = C.addTransition(State, C.getPredecessor(), &Tag);
- reportBug(ErrorKind::NullableReturnedToNonnull, N, Region,
- C.getBugReporter());
+ reportBug(ErrorKind::NullableReturnedToNonnull, N, Region, C);
}
return;
}
@@ -418,6 +512,9 @@
return;
ProgramStateRef State = C.getState();
+ if (State->get<PreconditionViolated>())
+ return;
+
ProgramStateRef OrigState = State;
unsigned Idx = 0;
@@ -445,10 +542,9 @@
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);
+ State = State->set<PreconditionViolated>(true);
+ ExplodedNode *N = C.addTransition(State);
+ reportBug(ErrorKind::NilPassedToNonnull, N, nullptr, C, ArgExpr);
return;
}
@@ -466,18 +562,16 @@
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);
+ State = State->set<PreconditionViolated>(true);
+ ExplodedNode *N = C.addTransition(State);
+ reportBug(ErrorKind::NullablePassedToNonnull, N, Region, C, ArgExpr);
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);
+ State = State->set<PreconditionViolated>(true);
+ ExplodedNode *N = C.addTransition(State);
+ reportBug(ErrorKind::NullableDereferenced, N, Region, C, ArgExpr);
return;
}
continue;
@@ -507,10 +601,13 @@
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 +673,14 @@
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,14 +788,17 @@
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
// change the nullability.
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 +847,14 @@
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;
@@ -762,8 +868,7 @@
LocNullability == Nullability::Nonnull) {
static CheckerProgramPointTag Tag(this, "NullPassedToNonnull");
ExplodedNode *N = C.addTransition(State, C.getPredecessor(), &Tag);
- reportBug(ErrorKind::NilAssignedToNonnull, N, nullptr, C.getBugReporter(),
- S);
+ reportBug(ErrorKind::NilAssignedToNonnull, N, nullptr, C, S);
return;
}
// Intentionally missing case: '0' is bound to a reference. It is handled by
@@ -784,8 +889,7 @@
LocNullability == Nullability::Nonnull) {
static CheckerProgramPointTag Tag(this, "NullablePassedToNonnull");
ExplodedNode *N = C.addTransition(State, C.getPredecessor(), &Tag);
- reportBug(ErrorKind::NullableAssignedToNonnull, N, ValueRegion,
- C.getBugReporter());
+ reportBug(ErrorKind::NullableAssignedToNonnull, N, ValueRegion, C);
}
return;
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
