maxjhandsome
/
llvm-project
forked from OSchip/llvm-project
1
0
Fork 0
Code Issues Pull Requests Packages Releases Wiki Activity

[analyzer] Nullability: Don't warn along paths where null returned from non-null. 

Change the nullability checker to not warn along paths where null is returned from
a method with a non-null return type, even when the diagnostic for this return
has been suppressed. This prevents warning from methods with non-null return types
that inline methods that themselves return nil but that suppressed the diagnostic.

Also change the PreconditionViolated state component to be called "InvariantViolated"
because it is set when a post-condition is violated, as well.

rdar://problem/25393539

llvm-svn: 264647
This commit is contained in:
Devin Coughlin 2016-03-28 20:30:25 +00:00
parent 0afa32cd59
commit 77942db0b8
3 changed files with 137 additions and 57 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

134
clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
View File

@ -168,11 +168,11 @@ private:
///
/// When \p SuppressPath is set to true, no more bugs will be reported on this
/// path by this checker.
void reportBugIfPreconditionHolds(StringRef Msg, ErrorKind Error,
ExplodedNode *N, const MemRegion *Region,
CheckerContext &C,
const Stmt *ValueExpr = nullptr,
bool SuppressPath = false) const;
void reportBugIfInvariantHolds(StringRef Msg, ErrorKind Error,
ExplodedNode *N, const MemRegion *Region,
CheckerContext &C,
const Stmt *ValueExpr = nullptr,
bool SuppressPath = false) const;
void reportBug(StringRef Msg, ErrorKind Error, ExplodedNode *N,
const MemRegion *Region, BugReporter &BR,
@ -247,12 +247,31 @@ bool operator==(NullabilityState Lhs, NullabilityState Rhs) {
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)
// We say "the nullability type invariant is violated" when a location with a
// non-null type contains NULL or a function with a non-null return type returns
// NULL. Violations of the nullability type invariant can be detected either
// directly (for example, when NULL is passed as an argument to a nonnull
// parameter) or indirectly (for example, when, inside a function, the
// programmer defensively checks whether a nonnull parameter contains NULL and
// finds that it does).
//
// As a matter of policy, the nullability checker typically warns on direct
// violations of the nullability invariant (although it uses various
// heuristics to suppress warnings in some cases) but will not warn if the
// invariant has already been violated along the path (either directly or
// indirectly). As a practical matter, this prevents the analyzer from
// (1) warning on defensive code paths where a nullability precondition is
// determined to have been violated, (2) warning additional times after an
// initial direct violation has been discovered, and (3) warning after a direct
// violation that has been implicitly or explicitly suppressed (for
// example, with a cast of NULL to _Nonnull). In essence, once an invariant
// violation is detected on a path, this checker will be esentially turned off
// for the rest of the analysis
//
// The analyzer takes this approach (rather than generating a sink node) to
// ensure coverage of defensive paths, which may be important for backwards
// compatibility in codebases that were developed without nullability in mind.
REGISTER_TRAIT_WITH_PROGRAMSTATE(InvariantViolated, bool)
enum class NullConstraint { IsNull, IsNotNull, Unknown };
@ -366,9 +385,9 @@ checkParamsForPreconditionViolation(const ParamVarDeclRange &Params,
return false;
}
static bool checkPreconditionViolation(ProgramStateRef State, ExplodedNode *N,
CheckerContext &C) {
if (State->get<PreconditionViolated>())
static bool checkInvariantViolation(ProgramStateRef State, ExplodedNode *N,
CheckerContext &C) {
if (State->get<InvariantViolated>())
return true;
const LocationContext *LocCtxt = C.getLocationContext();
@ -388,21 +407,21 @@ static bool checkPreconditionViolation(ProgramStateRef State, ExplodedNode *N,
if (checkParamsForPreconditionViolation(Params, State, LocCtxt)) {
if (!N->isSink())
C.addTransition(State->set<PreconditionViolated>(true), N);
C.addTransition(State->set<InvariantViolated>(true), N);
return true;
}
return false;
}
void NullabilityChecker::reportBugIfPreconditionHolds(StringRef Msg,
void NullabilityChecker::reportBugIfInvariantHolds(StringRef Msg,
ErrorKind Error, ExplodedNode *N, const MemRegion *Region,
CheckerContext &C, const Stmt *ValueExpr, bool SuppressPath) const {
ProgramStateRef OriginalState = N->getState();
if (checkPreconditionViolation(OriginalState, N, C))
if (checkInvariantViolation(OriginalState, N, C))
return;
if (SuppressPath) {
OriginalState = OriginalState->set<PreconditionViolated>(true);
OriginalState = OriginalState->set<InvariantViolated>(true);
N = C.addTransition(OriginalState, N);
}
@ -430,7 +449,7 @@ void NullabilityChecker::checkDeadSymbols(SymbolReaper &SR,
// 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))
if (checkInvariantViolation(State, C.getPredecessor(), C))
return;
C.addTransition(State);
}
@ -439,7 +458,7 @@ void NullabilityChecker::checkDeadSymbols(SymbolReaper &SR,
/// 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>())
if (Event.SinkNode->getState()->get<InvariantViolated>())
return;
const MemRegion *Region =
@ -486,10 +505,6 @@ static const Expr *lookThroughImplicitCasts(const Expr *E) {
/// This method check when nullable pointer or null value is returned from a
/// function that has nonnull return type.
///
/// TODO: when nullability preconditons are violated, it is ok to violate the
/// nullability postconditons (i.e.: when one of the nonnull parameters are null
/// this check should not report any nullability related issue).
void NullabilityChecker::checkPreStmt(const ReturnStmt *S,
CheckerContext &C) const {
auto RetExpr = S->getRetValue();
@ -500,7 +515,7 @@ void NullabilityChecker::checkPreStmt(const ReturnStmt *S,
return;
ProgramStateRef State = C.getState();
if (State->get<PreconditionViolated>())
if (State->get<InvariantViolated>())
return;
auto RetSVal =
@ -542,10 +557,11 @@ void NullabilityChecker::checkPreStmt(const ReturnStmt *S,
Nullability RetExprTypeLevelNullability =
getNullabilityAnnotation(lookThroughImplicitCasts(RetExpr)->getType());
bool NullReturnedFromNonNull = (RequiredNullability == Nullability::Nonnull &&
Nullness == NullConstraint::IsNull);
if (Filter.CheckNullReturnedFromNonnull &&
Nullness == NullConstraint::IsNull &&
NullReturnedFromNonNull &&
RetExprTypeLevelNullability != Nullability::Nonnull &&
RequiredNullability == Nullability::Nonnull &&
!InSuppressedMethodFamily) {
static CheckerProgramPointTag Tag(this, "NullReturnedFromNonnull");
ExplodedNode *N = C.generateErrorNode(State, &Tag);
@ -557,9 +573,17 @@ void NullabilityChecker::checkPreStmt(const ReturnStmt *S,
OS << "Null is returned from a " << C.getDeclDescription(D) <<
" that is expected to return a non-null value";
reportBugIfPreconditionHolds(OS.str(),
ErrorKind::NilReturnedToNonnull, N, nullptr, C,
RetExpr);
reportBugIfInvariantHolds(OS.str(),
ErrorKind::NilReturnedToNonnull, N, nullptr, C,
RetExpr);
return;
}
// If null was returned from a non-null function, mark the nullability
// invariant as violated even if the diagnostic was suppressed.
if (NullReturnedFromNonNull) {
State = State->set<InvariantViolated>(true);
C.addTransition(State);
return;
}
@ -583,9 +607,9 @@ void NullabilityChecker::checkPreStmt(const ReturnStmt *S,
OS << "Nullable pointer is returned from a " << C.getDeclDescription(D) <<
" that is expected to return a non-null value";
reportBugIfPreconditionHolds(OS.str(),
ErrorKind::NullableReturnedToNonnull, N,
Region, C);
reportBugIfInvariantHolds(OS.str(),
ErrorKind::NullableReturnedToNonnull, N,
Region, C);
}
return;
}
@ -605,7 +629,7 @@ void NullabilityChecker::checkPreCall(const CallEvent &Call,
return;
ProgramStateRef State = C.getState();
if (State->get<PreconditionViolated>())
if (State->get<InvariantViolated>())
return;
ProgramStateRef OrigState = State;
@ -646,9 +670,9 @@ void NullabilityChecker::checkPreCall(const CallEvent &Call,
llvm::raw_svector_ostream OS(SBuf);
OS << "Null passed to a callee that requires a non-null " << ParamIdx
<< llvm::getOrdinalSuffix(ParamIdx) << " parameter";
reportBugIfPreconditionHolds(OS.str(), ErrorKind::NilPassedToNonnull, N,
nullptr, C,
ArgExpr, /*SuppressPath=*/false);
reportBugIfInvariantHolds(OS.str(), ErrorKind::NilPassedToNonnull, N,
nullptr, C,
ArgExpr, /*SuppressPath=*/false);
return;
}
@ -672,17 +696,17 @@ void NullabilityChecker::checkPreCall(const CallEvent &Call,
llvm::raw_svector_ostream OS(SBuf);
OS << "Nullable pointer is passed to a callee that requires a non-null "
<< ParamIdx << llvm::getOrdinalSuffix(ParamIdx) << " parameter";
reportBugIfPreconditionHolds(OS.str(),
ErrorKind::NullablePassedToNonnull, N,
Region, C, ArgExpr, /*SuppressPath=*/true);
reportBugIfInvariantHolds(OS.str(),
ErrorKind::NullablePassedToNonnull, N,
Region, C, ArgExpr, /*SuppressPath=*/true);
return;
}
if (Filter.CheckNullableDereferenced &&
Param->getType()->isReferenceType()) {
ExplodedNode *N = C.addTransition(State);
reportBugIfPreconditionHolds("Nullable pointer is dereferenced",
ErrorKind::NullableDereferenced, N, Region,
C, ArgExpr, /*SuppressPath=*/true);
reportBugIfInvariantHolds("Nullable pointer is dereferenced",
ErrorKind::NullableDereferenced, N, Region,
C, ArgExpr, /*SuppressPath=*/true);
return;
}
continue;
@ -713,7 +737,7 @@ void NullabilityChecker::checkPostCall(const CallEvent &Call,
if (!ReturnType->isAnyPointerType())
return;
ProgramStateRef State = C.getState();
if (State->get<PreconditionViolated>())
if (State->get<InvariantViolated>())
return;
const MemRegion *Region = getTrackRegion(Call.getReturnValue());
@ -782,7 +806,7 @@ void NullabilityChecker::checkPostObjCMessage(const ObjCMethodCall &M,
return;
ProgramStateRef State = C.getState();
if (State->get<PreconditionViolated>())
if (State->get<InvariantViolated>())
return;
const MemRegion *ReturnRegion = getTrackRegion(M.getReturnValue());
@ -897,7 +921,7 @@ void NullabilityChecker::checkPostStmt(const ExplicitCastExpr *CE,
return;
ProgramStateRef State = C.getState();
if (State->get<PreconditionViolated>())
if (State->get<InvariantViolated>())
return;
Nullability DestNullability = getNullabilityAnnotation(DestType);
@ -1022,7 +1046,7 @@ void NullabilityChecker::checkBind(SVal L, SVal V, const Stmt *S,
return;
ProgramStateRef State = C.getState();
if (State->get<PreconditionViolated>())
if (State->get<InvariantViolated>())
return;
auto ValDefOrUnknown = V.getAs<DefinedOrUnknownSVal>();
@ -1050,10 +1074,10 @@ void NullabilityChecker::checkBind(SVal L, SVal V, const Stmt *S,
if (!ValueExpr)
ValueExpr = S;
reportBugIfPreconditionHolds("Null is assigned to a pointer which is "
"expected to have non-null value",
ErrorKind::NilAssignedToNonnull, N, nullptr, C,
ValueExpr);
reportBugIfInvariantHolds("Null is assigned to a pointer which is "
"expected to have non-null value",
ErrorKind::NilAssignedToNonnull, N, nullptr, C,
ValueExpr);
return;
}
// Intentionally missing case: '0' is bound to a reference. It is handled by
@ -1074,10 +1098,10 @@ void NullabilityChecker::checkBind(SVal L, SVal V, const Stmt *S,
LocNullability == Nullability::Nonnull) {
static CheckerProgramPointTag Tag(this, "NullablePassedToNonnull");
ExplodedNode *N = C.addTransition(State, C.getPredecessor(), &Tag);
reportBugIfPreconditionHolds("Nullable pointer is assigned to a pointer "
"which is expected to have non-null value",
ErrorKind::NullableAssignedToNonnull, N,
ValueRegion, C);
reportBugIfInvariantHolds("Nullable pointer is assigned to a pointer "
"which is expected to have non-null value",
ErrorKind::NullableAssignedToNonnull, N,
ValueRegion, C);
}
return;
}

5
clang/test/Analysis/Inputs/system-header-simulator-for-nullability.h
View File

@ -11,8 +11,9 @@ NS_ASSUME_NONNULL_BEGIN
typedef struct _NSZone NSZone;
@protocol NSObject
+ (id)alloc;
- (id)init;
+ (instancetype)alloc;
- (instancetype)init;
- (instancetype)autorelease;
@end
@protocol NSCopying

55
clang/test/Analysis/nullability-no-arc.mm
View File

@ -5,6 +5,8 @@
@protocol NSObject
+ (id)alloc;
- (id)init;
- (instancetype)autorelease;
- (void)release;
@end
__attribute__((objc_root_class))
@ -43,3 +45,56 @@ void testObjCNonARCNoInitialization(TestObject * _Nonnull p) {
void testObjCNonARCExplicitZeroInitialization() {
TestObject * _Nonnull explicitlyZeroInitialized = nil; // expected-warning {{Null is assigned to a pointer which is expected to have non-null value}}
}
@interface ClassWithInitializers : NSObject
@end
@implementation ClassWithInitializers
- (instancetype _Nonnull)initWithNonnullReturnAndSelfCheckingIdiom {
// This defensive check is a common-enough idiom that we don't want
// to issue a diagnostic for it.
if (self = [super init]) {
}
return self; // no-warning
}
- (instancetype _Nonnull)initWithNonnullReturnAndNilReturnViaLocal {
self = [super init];
// This leaks, but we're not checking for that here.
ClassWithInitializers *other = nil;
// False negative. Once we have more subtle suppression of defensive checks in
// initializers we should warn here.
return other;
}
- (instancetype _Nonnull)initWithPreconditionViolation:(int)p {
self = [super init];
if (p < 0) {
[self release];
return (ClassWithInitializers * _Nonnull)nil;
}
return self;
}
+ (instancetype _Nonnull)factoryCallingInitWithNonnullReturnAndSelfCheckingIdiom {
return [[[self alloc] initWithNonnullReturnAndSelfCheckingIdiom] autorelease]; // no-warning
}
+ (instancetype _Nonnull)factoryCallingInitWithNonnullReturnAndNilReturnViaLocal {
return [[[self alloc] initWithNonnullReturnAndNilReturnViaLocal] autorelease]; // no-warning
}
+ (instancetype _Nonnull)initWithPreconditionViolation:(int) p {
return [[[self alloc] initWithPreconditionViolation:p] autorelease]; // no-warning
}
- (TestObject * _Nonnull) returnsNil {
return (TestObject * _Nonnull)nil;
}
- (TestObject * _Nonnull) inlineOfReturnsNilObjCInstanceDirectlyWithSuppressingCast {
TestObject *o = [self returnsNil];
return o;
}
@end