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

[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.

llvm-svn: 259221
This commit is contained in:
Anna Zaks 2016-01-29 18:43:15 +00:00
parent 6ea637af35
commit ad9e7ea6d7
6 changed files with 114 additions and 57 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
View File

@ -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;

4
clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp
View File

@ -230,7 +230,7 @@ void DereferenceChecker::checkLocation(SVal l, bool isLoad, const Stmt* 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 L, SVal V, const Stmt *S,
if (ExplodedNode *N = C.generateSink(StNull, C.getPredecessor())) {
ImplicitNullDerefEvent event = {V, /*isLoad=*/true, N,
&C.getBugReporter(),
/*IsDirectDereference=*/false};
/*IsDirectDereference=*/true};
dispatchEvent(event);
}
}

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

@ -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(ProgramStateRef State, ExplodedNode *N,
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::reportBugIfPreconditionHolds(
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(ImplicitNullDerefEvent Event) const {
// 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(const ReturnStmt *S,
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(const ReturnStmt *S,
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(const CallEvent &Call,
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(const CallEvent &Call,
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 L, SVal V, const Stmt *S,
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 L, SVal V, const Stmt *S,
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;

7
clang/lib/StaticAnalyzer/Core/CheckerContext.cpp
View File

@ -35,6 +35,13 @@ StringRef CheckerContext::getCalleeName(const FunctionDecl *FunDecl) const {
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) {

56
clang/test/Analysis/nullability.mm
View File

@ -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 *_Nonnull nonnull, Dummy *_Nullable nullable) {
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 testDirectCastNilToNonnull() {
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 testDefensiveInlineChecks(Dummy * p) {
- (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 {

8
clang/test/Analysis/nullability_nullonly.mm
View File

@ -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 returnsNilObjCInstanceDirectlyWithSuppressingCast() {
@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 {