Author: zaks Date: Fri Jan 29 12:43:15 2016 New Revision: 259221 URL: http://llvm.org/viewvc/llvm-project?rev=259221&view=rev Log: [analyzer] Improve Nullability checker diagnostics
- Include the position of the argument on which the nullability is violated - Differentiate between a 'method' and a 'function' in the message wording - Test for the error message text in the tests - Fix a bug with setting 'IsDirectDereference' which resulted in regular dereferences assumed to have call context. Modified: cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h cfe/trunk/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp cfe/trunk/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp cfe/trunk/lib/StaticAnalyzer/Core/CheckerContext.cpp cfe/trunk/test/Analysis/nullability.mm cfe/trunk/test/Analysis/nullability_nullonly.mm Modified: cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h?rev=259221&r1=259220&r2=259221&view=diff ============================================================================== --- cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h (original) +++ cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h Fri Jan 29 12:43:15 2016 @@ -263,6 +263,10 @@ public: Eng.getBugReporter().emitReport(std::move(R)); } + /// \brief Returns the word that should be used to refer to the declaration + /// in the report. + StringRef getDeclDescription(const Decl *D); + /// \brief Get the declaration of the called function (path-sensitive). const FunctionDecl *getCalleeDecl(const CallExpr *CE) const; Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp?rev=259221&r1=259220&r2=259221&view=diff ============================================================================== --- cfe/trunk/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp (original) +++ cfe/trunk/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp Fri Jan 29 12:43:15 2016 @@ -230,7 +230,7 @@ void DereferenceChecker::checkLocation(S // dereference. if (ExplodedNode *N = C.generateSink(nullState, C.getPredecessor())) { ImplicitNullDerefEvent event = {l, isLoad, N, &C.getBugReporter(), - /*IsDirectDereference=*/false}; + /*IsDirectDereference=*/true}; dispatchEvent(event); } } @@ -272,7 +272,7 @@ void DereferenceChecker::checkBind(SVal if (ExplodedNode *N = C.generateSink(StNull, C.getPredecessor())) { ImplicitNullDerefEvent event = {V, /*isLoad=*/true, N, &C.getBugReporter(), - /*IsDirectDereference=*/false}; + /*IsDirectDereference=*/true}; dispatchEvent(event); } } Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp?rev=259221&r1=259220&r2=259221&view=diff ============================================================================== --- cfe/trunk/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp (original) +++ cfe/trunk/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp Fri Jan 29 12:43:15 2016 @@ -26,13 +26,16 @@ //===----------------------------------------------------------------------===// #include "ClangSACheckers.h" -#include "llvm/Support/Path.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/CheckerContext.h" #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h" +#include "llvm/ADT/StringExtras.h" +#include "llvm/Support/Path.h" + using namespace clang; using namespace ento; @@ -89,18 +92,6 @@ enum class ErrorKind : int { NullablePassedToNonnull }; -const char *const ErrorMessages[] = { - "Null is assigned to a pointer which is expected to have non-null value", - "Null passed to a callee that requires a non-null argument", - "Null is returned from a function that is expected to return a non-null " - "value", - "Nullable pointer is assigned to a pointer which is expected to have " - "non-null value", - "Nullable pointer is returned from a function that is expected to return a " - "non-null value", - "Nullable pointer is dereferenced", - "Nullable pointer is passed to a callee that requires a non-null argument"}; - class NullabilityChecker : public Checker<check::Bind, check::PreCall, check::PreStmt<ReturnStmt>, check::PostCall, check::PostStmt<ExplicitCastExpr>, @@ -169,17 +160,19 @@ private: /// /// 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, + void reportBugIfPreconditionHolds(StringRef Msg, 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 { + void reportBug(StringRef Msg, ErrorKind Error, ExplodedNode *N, + const MemRegion *Region, BugReporter &BR, + const Stmt *ValueExpr = nullptr) const { if (!BT) BT.reset(new BugType(this, "Nullability", "Memory error")); - const char *Msg = ErrorMessages[static_cast<int>(Error)]; - std::unique_ptr<BugReport> R(new BugReport(*BT, Msg, N)); + + auto R = llvm::make_unique<BugReport>(*BT, Msg, N); if (Region) { R->markInteresting(Region); R->addVisitor(llvm::make_unique<NullabilityBugVisitor>(Region)); @@ -384,7 +377,7 @@ static bool checkPreconditionViolation(P return false; } -void NullabilityChecker::reportBugIfPreconditionHolds( +void NullabilityChecker::reportBugIfPreconditionHolds(StringRef Msg, ErrorKind Error, ExplodedNode *N, const MemRegion *Region, CheckerContext &C, const Stmt *ValueExpr, bool SuppressPath) const { ProgramStateRef OriginalState = N->getState(); @@ -396,7 +389,7 @@ void NullabilityChecker::reportBugIfPrec N = C.addTransition(OriginalState, N); } - reportBug(Error, N, Region, C.getBugReporter(), ValueExpr); + reportBug(Msg, Error, N, Region, C.getBugReporter(), ValueExpr); } /// Cleaning up the program state. @@ -450,9 +443,13 @@ void NullabilityChecker::checkEvent(Impl // 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 - reportBug(ErrorKind::NullablePassedToNonnull, Event.SinkNode, Region, BR); + reportBug("Nullable pointer is dereferenced", + ErrorKind::NullableDereferenced, Event.SinkNode, Region, BR); + else { + reportBug("Nullable pointer is passed to a callee that requires a " + "non-null", ErrorKind::NullablePassedToNonnull, + Event.SinkNode, Region, BR); + } } } @@ -537,7 +534,14 @@ void NullabilityChecker::checkPreStmt(co ExplodedNode *N = C.generateErrorNode(State, &Tag); if (!N) return; - reportBugIfPreconditionHolds(ErrorKind::NilReturnedToNonnull, N, nullptr, C, + + SmallString<256> SBuf; + llvm::raw_svector_ostream OS(SBuf); + 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); return; } @@ -556,7 +560,14 @@ void NullabilityChecker::checkPreStmt(co RequiredNullability == Nullability::Nonnull) { static CheckerProgramPointTag Tag(this, "NullableReturnedFromNonnull"); ExplodedNode *N = C.addTransition(State, C.getPredecessor(), &Tag); - reportBugIfPreconditionHolds(ErrorKind::NullableReturnedToNonnull, N, + + SmallString<256> SBuf; + llvm::raw_svector_ostream OS(SBuf); + 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); } return; @@ -605,14 +616,21 @@ void NullabilityChecker::checkPreCall(co Nullability ArgExprTypeLevelNullability = getNullabilityAnnotation(ArgExpr->getType()); + unsigned ParamIdx = Param->getFunctionScopeIndex() + 1; + if (Filter.CheckNullPassedToNonnull && Nullness == NullConstraint::IsNull && ArgExprTypeLevelNullability != Nullability::Nonnull && RequiredNullability == Nullability::Nonnull) { ExplodedNode *N = C.generateErrorNode(State); if (!N) return; - reportBugIfPreconditionHolds(ErrorKind::NilPassedToNonnull, N, nullptr, C, - ArgExpr); + SmallString<256> SBuf; + 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); return; } @@ -631,14 +649,20 @@ void NullabilityChecker::checkPreCall(co if (Filter.CheckNullablePassedToNonnull && RequiredNullability == Nullability::Nonnull) { ExplodedNode *N = C.addTransition(State); - reportBugIfPreconditionHolds(ErrorKind::NullablePassedToNonnull, N, + SmallString<256> SBuf; + 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); return; } if (Filter.CheckNullableDereferenced && Param->getType()->isReferenceType()) { ExplodedNode *N = C.addTransition(State); - reportBugIfPreconditionHolds(ErrorKind::NullableDereferenced, N, Region, + reportBugIfPreconditionHolds("Nullable pointer is dereferenced", + ErrorKind::NullableDereferenced, N, Region, C, ArgExpr, /*SuppressPath=*/true); return; } @@ -1007,7 +1031,9 @@ void NullabilityChecker::checkBind(SVal if (!ValueExpr) ValueExpr = S; - reportBugIfPreconditionHolds(ErrorKind::NilAssignedToNonnull, N, nullptr, C, + reportBugIfPreconditionHolds("Null is assigned to a pointer which is " + "expected to have non-null value", + ErrorKind::NilAssignedToNonnull, N, nullptr, C, ValueExpr); return; } @@ -1029,7 +1055,9 @@ void NullabilityChecker::checkBind(SVal LocNullability == Nullability::Nonnull) { static CheckerProgramPointTag Tag(this, "NullablePassedToNonnull"); ExplodedNode *N = C.addTransition(State, C.getPredecessor(), &Tag); - reportBugIfPreconditionHolds(ErrorKind::NullableAssignedToNonnull, N, + reportBugIfPreconditionHolds("Nullable pointer is assigned to a pointer " + "which is expected to have non-null value", + ErrorKind::NullableAssignedToNonnull, N, ValueRegion, C); } return; Modified: cfe/trunk/lib/StaticAnalyzer/Core/CheckerContext.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/CheckerContext.cpp?rev=259221&r1=259220&r2=259221&view=diff ============================================================================== --- cfe/trunk/lib/StaticAnalyzer/Core/CheckerContext.cpp (original) +++ cfe/trunk/lib/StaticAnalyzer/Core/CheckerContext.cpp Fri Jan 29 12:43:15 2016 @@ -35,6 +35,13 @@ StringRef CheckerContext::getCalleeName( return funI->getName(); } +StringRef CheckerContext::getDeclDescription(const Decl *D) { + if (isa<ObjCMethodDecl>(D) || isa<CXXMethodDecl>(D)) + return "method"; + if (isa<BlockDecl>(D)) + return "anonymous block"; + return "function"; +} bool CheckerContext::isCLibraryFunction(const FunctionDecl *FD, StringRef Name) { Modified: cfe/trunk/test/Analysis/nullability.mm URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/nullability.mm?rev=259221&r1=259220&r2=259221&view=diff ============================================================================== --- cfe/trunk/test/Analysis/nullability.mm (original) +++ cfe/trunk/test/Analysis/nullability.mm Fri Jan 29 12:43:15 2016 @@ -1,5 +1,4 @@ -// RUN: %clang_cc1 -fobjc-arc -analyze -analyzer-checker=core,nullability -verify %s -// RUN: %clang_cc1 -analyze -analyzer-checker=core,nullability -verify %s +// RUN: %clang_cc1 -fblocks -analyze -analyzer-checker=core,nullability -verify %s #define nil 0 #define BOOL int @@ -72,16 +71,16 @@ void testBasicRules() { // Make every dereference a different path to avoid sinks after errors. switch (getRandom()) { case 0: { - Dummy &r = *p; // expected-warning {{}} + Dummy &r = *p; // expected-warning {{Nullable pointer is dereferenced}} } break; case 1: { - int b = p->val; // expected-warning {{}} + int b = p->val; // expected-warning {{Nullable pointer is dereferenced}} } break; case 2: { - int stuff = *ptr; // expected-warning {{}} + int stuff = *ptr; // expected-warning {{Nullable pointer is dereferenced}} } break; case 3: - takesNonnull(p); // expected-warning {{}} + takesNonnull(p); // expected-warning {{Nullable pointer is passed to a callee that requires a non-null 1st parameter}} break; case 4: { Dummy d; @@ -103,11 +102,11 @@ void testBasicRules() { Dummy *q = 0; if (getRandom()) { takesNullable(q); - takesNonnull(q); // expected-warning {{}} + takesNonnull(q); // expected-warning {{Null passed to a callee that requires a non-null 1st parameter}} } Dummy a; Dummy *_Nonnull nonnull = &a; - nonnull = q; // expected-warning {{}} + nonnull = q; // expected-warning {{Null is assigned to a pointer which is expected to have non-null value}} q = &a; takesNullable(q); takesNonnull(q); @@ -120,14 +119,14 @@ void testArgumentTracking(Dummy *_Nonnul Dummy *p = nullable; Dummy *q = nonnull; switch(getRandom()) { - case 1: nonnull = p; break; // expected-warning {{}} + case 1: nonnull = p; break; // expected-warning {{Nullable pointer is assigned to a pointer which is expected to have non-null value}} case 2: p = 0; break; case 3: q = p; break; case 4: testMultiParamChecking(nonnull, nullable, nonnull); break; case 5: testMultiParamChecking(nonnull, nonnull, nonnull); break; - case 6: testMultiParamChecking(nonnull, nullable, nullable); break; // expected-warning {{}} - case 7: testMultiParamChecking(nullable, nullable, nonnull); // expected-warning {{}} - case 8: testMultiParamChecking(nullable, nullable, nullable); // expected-warning {{}} + case 6: testMultiParamChecking(nonnull, nullable, nullable); break; // expected-warning {{Nullable pointer is passed to a callee that requires a non-null 3rd parameter}} + case 7: testMultiParamChecking(nullable, nullable, nonnull); // expected-warning {{Nullable pointer is passed to a callee that requires a non-null 1st parameter}} + case 8: testMultiParamChecking(nullable, nullable, nullable); // expected-warning {{Nullable pointer is passed to a callee that requires a non-null 1st parameter}} case 9: testMultiParamChecking((Dummy *_Nonnull)0, nullable, nonnull); break; } } @@ -161,20 +160,20 @@ void testObjCMessageResultNullability() break; case 3: shouldBeNullable = [eraseNullab(getNullableTestObject()) returnsNullable]; - [o takesNonnull:shouldBeNullable]; // expected-warning {{}} + [o takesNonnull:shouldBeNullable]; // expected-warning {{Nullable pointer is passed to a callee that requires a non-null 1st parameter}} break; case 4: shouldBeNullable = [eraseNullab(getNonnullTestObject()) returnsNullable]; - [o takesNonnull:shouldBeNullable]; // expected-warning {{}} + [o takesNonnull:shouldBeNullable]; // expected-warning {{Nullable pointer is passed to a callee that requires a non-null 1st parameter}} break; case 5: shouldBeNullable = [eraseNullab(getUnspecifiedTestObject()) returnsNullable]; - [o takesNonnull:shouldBeNullable]; // expected-warning {{}} + [o takesNonnull:shouldBeNullable]; // expected-warning {{Nullable pointer is passed to a callee that requires a non-null 1st parameter}} break; case 6: shouldBeNullable = [eraseNullab(getNullableTestObject()) returnsNullable]; - [o takesNonnull:shouldBeNullable]; // expected-warning {{}} + [o takesNonnull:shouldBeNullable]; // expected-warning {{Nullable pointer is passed to a callee that requires a non-null 1st parameter}} break; case 7: { int *shouldBeNonnull = [eraseNullab(getNonnullTestObject()) returnsNonnull]; @@ -203,7 +202,18 @@ Dummy * _Nonnull testDirectCastNilToNonn void testIndirectCastNilToNonnullAndPass() { Dummy *p = (Dummy * _Nonnull)0; // FIXME: Ideally the cast above would suppress this warning. - takesNonnull(p); // expected-warning {{Null passed to a callee that requires a non-null argument}} + takesNonnull(p); // expected-warning {{Null passed to a callee that requires a non-null 1st parameter}} +} + +void testIndirectNilPassToNonnull() { + Dummy *p = 0; + takesNonnull(p); // expected-warning {{Null passed to a callee that requires a non-null 1st parameter}} +} + +void testConditionalNilPassToNonnull(Dummy *p) { + if (!p) { + takesNonnull(p); // expected-warning {{Null passed to a callee that requires a non-null 1st parameter}} + } } Dummy * _Nonnull testIndirectCastNilToNonnullAndReturn() { @@ -220,7 +230,7 @@ void testInvalidPropagation() { void onlyReportFirstPreconditionViolationOnPath() { Dummy *p = returnsNullable(); - takesNonnull(p); // expected-warning {{}} + takesNonnull(p); // expected-warning {{Nullable pointer is passed to a callee that requires a non-null 1st parameter}} takesNonnull(p); // No warning. // The first warning was not a sink. The analysis expected to continue. int i = 0; @@ -269,6 +279,14 @@ void inlinedUnspecified(Dummy *p) { if (p) return; } +void testNilReturnWithBlock(Dummy *p) { + p = 0; + Dummy *_Nonnull (^myblock)(void) = ^Dummy *_Nonnull(void) { + return p; // TODO: We should warn in blocks. + }; + myblock(); +} + Dummy *_Nonnull testDefensiveInlineChecks(Dummy * p) { switch (getRandom()) { case 1: inlinedNullable(p); break; @@ -310,7 +328,7 @@ Dummy *_Nonnull testDefensiveInlineCheck - (TestObject * _Nonnull)testReturnsNullableInNonnullIndirectly { TestObject *local = getNullableTestObject(); - return local; // expected-warning {{Nullable pointer is returned from a function that is expected to return a non-null value}} + return local; // expected-warning {{Nullable pointer is returned from a method that is expected to return a non-null value}} } - (TestObject * _Nonnull)testReturnsCastSuppressedNullableInNonnullIndirectly { Modified: cfe/trunk/test/Analysis/nullability_nullonly.mm URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/nullability_nullonly.mm?rev=259221&r1=259220&r2=259221&view=diff ============================================================================== --- cfe/trunk/test/Analysis/nullability_nullonly.mm (original) +++ cfe/trunk/test/Analysis/nullability_nullonly.mm Fri Jan 29 12:43:15 2016 @@ -31,18 +31,18 @@ void testBasicRules() { Dummy *q = 0; if (getRandom()) { takesNullable(q); - takesNonnull(q); // expected-warning {{}} + takesNonnull(q); // expected-warning {{Null passed to a callee that requires a non-null 1st parameter}} } } Dummy *_Nonnull testNullReturn() { Dummy *p = 0; - return p; // expected-warning {{}} + return p; // expected-warning {{Null is returned from a function that is expected to return a non-null value}} } void onlyReportFirstPreconditionViolationOnPath() { Dummy *p = 0; - takesNonnull(p); // expected-warning {{}} + takesNonnull(p); // expected-warning {{Null passed to a callee that requires a non-null 1st parameter}} takesNonnull(p); // No warning. // Passing null to nonnull is a sink. Stop the analysis. int i = 0; @@ -143,7 +143,7 @@ TestObject * _Nonnull returnsNilObjCInst @implementation SomeClass (MethodReturn) - (SomeClass * _Nonnull)testReturnsNilInNonnull { SomeClass *local = nil; - return local; // expected-warning {{Null is returned from a function that is expected to return a non-null value}} + return local; // expected-warning {{Null is returned from a method that is expected to return a non-null value}} } - (SomeClass * _Nonnull)testReturnsCastSuppressedNilInNonnull { _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits