r259221 - [analyzer] Improve Nullability checker diagnostics
Anna Zaks via cfe-commits
cfe-commits at lists.llvm.org
Fri Jan 29 10:43:17 PST 2016
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 {
More information about the cfe-commits
mailing list