[analyzer] Improved diagnostic pruning for calls initializing values.
This heuristic addresses the case when a pointer (or ref) is passed
to a function, which initializes the variable (or sets it to something
other than '0'). On the branch where the inlined function does not
set the value, we report use of undefined value (or NULL pointer
dereference). The access happens in the caller and the path
through the callee would get pruned away with regular path pruning. To
solve this issue, we previously disabled diagnostic pruning completely
on undefined and null pointer dereference checks, which entailed very
verbose diagnostics in most cases. Furthermore, not all of the
undef value checks had the diagnostic pruning disabled.
This patch implements the following heuristic: if we pass a pointer (or
ref) to the region (on which the error is reported) into a function and
it's value is either undef or 'NULL' (and is a pointer), do not prune
the function.
git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@162863 91177308-0d34-0410-b5e6-96231b3b80d8
diff --git a/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp b/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp
index cb0a3ce..00128de 100644
--- a/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp
+++ b/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp
@@ -192,7 +192,6 @@
new BugReport(*BT_undef, BT_undef->getDescription(), N);
bugreporter::trackNullOrUndefValue(N, bugreporter::GetDerefExpr(N),
*report);
- report->disablePathPruning();
C.EmitReport(report);
}
return;
diff --git a/lib/StaticAnalyzer/Checkers/ReturnUndefChecker.cpp b/lib/StaticAnalyzer/Checkers/ReturnUndefChecker.cpp
index 4abda42..7e95199 100644
--- a/lib/StaticAnalyzer/Checkers/ReturnUndefChecker.cpp
+++ b/lib/StaticAnalyzer/Checkers/ReturnUndefChecker.cpp
@@ -53,7 +53,6 @@
BugReport *report =
new BugReport(*BT, BT->getDescription(), N);
- report->disablePathPruning();
report->addRange(RetE->getSourceRange());
bugreporter::trackNullOrUndefValue(N, RetE, *report);
diff --git a/lib/StaticAnalyzer/Checkers/UndefBranchChecker.cpp b/lib/StaticAnalyzer/Checkers/UndefBranchChecker.cpp
index 8a97b05..a50152e 100644
--- a/lib/StaticAnalyzer/Checkers/UndefBranchChecker.cpp
+++ b/lib/StaticAnalyzer/Checkers/UndefBranchChecker.cpp
@@ -101,7 +101,6 @@
BugReport *R = new BugReport(*BT, BT->getDescription(), N);
bugreporter::trackNullOrUndefValue(N, Ex, *R);
R->addRange(Ex->getSourceRange());
- R->disablePathPruning();
Ctx.EmitReport(R);
}
diff --git a/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp b/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
index 605f677..bbfff0c 100644
--- a/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
+++ b/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
@@ -81,7 +81,6 @@
else
bugreporter::trackNullOrUndefValue(N, B, *report);
- report->disablePathPruning();
C.EmitReport(report);
}
}
diff --git a/lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp b/lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp
index 8ae75ea..021364b 100644
--- a/lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp
+++ b/lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp
@@ -80,7 +80,6 @@
R->addRange(ex->getSourceRange());
bugreporter::trackNullOrUndefValue(N, ex, *R);
}
- R->disablePathPruning();
C.EmitReport(R);
}
diff --git a/lib/StaticAnalyzer/Core/BugReporter.cpp b/lib/StaticAnalyzer/Core/BugReporter.cpp
index 571baec..1866a27 100644
--- a/lib/StaticAnalyzer/Core/BugReporter.cpp
+++ b/lib/StaticAnalyzer/Core/BugReporter.cpp
@@ -121,7 +121,7 @@
/// Recursively scan through a path and prune out calls and macros pieces
/// that aren't needed. Return true if afterwards the path contains
/// "interesting stuff" which means it should be pruned from the parent path.
-static bool RemoveUneededCalls(PathPieces &pieces) {
+bool BugReporter::RemoveUneededCalls(PathPieces &pieces, BugReport *R) {
bool containsSomethingInteresting = false;
const unsigned N = pieces.size();
@@ -134,16 +134,22 @@
switch (piece->getKind()) {
case PathDiagnosticPiece::Call: {
PathDiagnosticCallPiece *call = cast<PathDiagnosticCallPiece>(piece);
+ // Check if the location context is interesting.
+ assert(LocationContextMap.count(call));
+ if (R->isInteresting(LocationContextMap[call])) {
+ containsSomethingInteresting = true;
+ break;
+ }
// Recursively clean out the subclass. Keep this call around if
// it contains any informative diagnostics.
- if (!RemoveUneededCalls(call->path))
+ if (!RemoveUneededCalls(call->path, R))
continue;
containsSomethingInteresting = true;
break;
}
case PathDiagnosticPiece::Macro: {
PathDiagnosticMacroPiece *macro = cast<PathDiagnosticMacroPiece>(piece);
- if (!RemoveUneededCalls(macro->subPieces))
+ if (!RemoveUneededCalls(macro->subPieces, R))
continue;
containsSomethingInteresting = true;
break;
@@ -430,55 +436,60 @@
NextNode = GetPredecessorNode(N);
ProgramPoint P = N->getLocation();
-
- if (const CallExitEnd *CE = dyn_cast<CallExitEnd>(&P)) {
- PathDiagnosticCallPiece *C =
- PathDiagnosticCallPiece::construct(N, *CE, SMgr);
- PD.getActivePath().push_front(C);
- PD.pushActivePath(&C->path);
- CallStack.push_back(StackDiagPair(C, N));
- continue;
- }
-
- if (const CallEnter *CE = dyn_cast<CallEnter>(&P)) {
- // Flush all locations, and pop the active path.
- bool VisitedEntireCall = PD.isWithinCall();
- PD.popActivePath();
- // Either we just added a bunch of stuff to the top-level path, or
- // we have a previous CallExitEnd. If the former, it means that the
- // path terminated within a function call. We must then take the
- // current contents of the active path and place it within
- // a new PathDiagnosticCallPiece.
- PathDiagnosticCallPiece *C;
- if (VisitedEntireCall) {
- C = cast<PathDiagnosticCallPiece>(PD.getActivePath().front());
- } else {
- const Decl *Caller = CE->getLocationContext()->getDecl();
- C = PathDiagnosticCallPiece::construct(PD.getActivePath(), Caller);
+ do {
+ if (const CallExitEnd *CE = dyn_cast<CallExitEnd>(&P)) {
+ PathDiagnosticCallPiece *C =
+ PathDiagnosticCallPiece::construct(N, *CE, SMgr);
+ GRBugReporter& BR = PDB.getBugReporter();
+ BR.addCallPieceLocationContextPair(C, CE->getCalleeContext());
+ PD.getActivePath().push_front(C);
+ PD.pushActivePath(&C->path);
+ CallStack.push_back(StackDiagPair(C, N));
+ break;
}
- C->setCallee(*CE, SMgr);
- if (!CallStack.empty()) {
- assert(CallStack.back().first == C);
- CallStack.pop_back();
+ if (const CallEnter *CE = dyn_cast<CallEnter>(&P)) {
+ // Flush all locations, and pop the active path.
+ bool VisitedEntireCall = PD.isWithinCall();
+ PD.popActivePath();
+
+ // Either we just added a bunch of stuff to the top-level path, or
+ // we have a previous CallExitEnd. If the former, it means that the
+ // path terminated within a function call. We must then take the
+ // current contents of the active path and place it within
+ // a new PathDiagnosticCallPiece.
+ PathDiagnosticCallPiece *C;
+ if (VisitedEntireCall) {
+ C = cast<PathDiagnosticCallPiece>(PD.getActivePath().front());
+ } else {
+ const Decl *Caller = CE->getLocationContext()->getDecl();
+ C = PathDiagnosticCallPiece::construct(PD.getActivePath(), Caller);
+ GRBugReporter& BR = PDB.getBugReporter();
+ BR.addCallPieceLocationContextPair(C, CE->getCalleeContext());
+ }
+
+ C->setCallee(*CE, SMgr);
+ if (!CallStack.empty()) {
+ assert(CallStack.back().first == C);
+ CallStack.pop_back();
+ }
+ break;
}
- continue;
- }
- if (const BlockEdge *BE = dyn_cast<BlockEdge>(&P)) {
- const CFGBlock *Src = BE->getSrc();
- const CFGBlock *Dst = BE->getDst();
- const Stmt *T = Src->getTerminator();
+ if (const BlockEdge *BE = dyn_cast<BlockEdge>(&P)) {
+ const CFGBlock *Src = BE->getSrc();
+ const CFGBlock *Dst = BE->getDst();
+ const Stmt *T = Src->getTerminator();
- if (!T)
- continue;
+ if (!T)
+ break;
- PathDiagnosticLocation Start =
- PathDiagnosticLocation::createBegin(T, SMgr,
- N->getLocationContext());
+ PathDiagnosticLocation Start =
+ PathDiagnosticLocation::createBegin(T, SMgr,
+ N->getLocationContext());
- switch (T->getStmtClass()) {
+ switch (T->getStmtClass()) {
default:
break;
@@ -487,16 +498,16 @@
const Stmt *S = GetNextStmt(N);
if (!S)
- continue;
+ break;
std::string sbuf;
llvm::raw_string_ostream os(sbuf);
const PathDiagnosticLocation &End = PDB.getEnclosingStmtLocation(S);
os << "Control jumps to line "
- << End.asLocation().getExpansionLineNumber();
- PD.getActivePath().push_front(new PathDiagnosticControlFlowPiece(Start, End,
- os.str()));
+ << End.asLocation().getExpansionLineNumber();
+ PD.getActivePath().push_front(new PathDiagnosticControlFlowPiece(
+ Start, End, os.str()));
break;
}
@@ -509,52 +520,52 @@
PathDiagnosticLocation End(S, SMgr, LC);
switch (S->getStmtClass()) {
- default:
- os << "No cases match in the switch statement. "
- "Control jumps to line "
- << End.asLocation().getExpansionLineNumber();
- break;
- case Stmt::DefaultStmtClass:
- os << "Control jumps to the 'default' case at line "
- << End.asLocation().getExpansionLineNumber();
- break;
+ default:
+ os << "No cases match in the switch statement. "
+ "Control jumps to line "
+ << End.asLocation().getExpansionLineNumber();
+ break;
+ case Stmt::DefaultStmtClass:
+ os << "Control jumps to the 'default' case at line "
+ << End.asLocation().getExpansionLineNumber();
+ break;
- case Stmt::CaseStmtClass: {
- os << "Control jumps to 'case ";
- const CaseStmt *Case = cast<CaseStmt>(S);
- const Expr *LHS = Case->getLHS()->IgnoreParenCasts();
+ case Stmt::CaseStmtClass: {
+ os << "Control jumps to 'case ";
+ const CaseStmt *Case = cast<CaseStmt>(S);
+ const Expr *LHS = Case->getLHS()->IgnoreParenCasts();
- // Determine if it is an enum.
- bool GetRawInt = true;
+ // Determine if it is an enum.
+ bool GetRawInt = true;
- if (const DeclRefExpr *DR = dyn_cast<DeclRefExpr>(LHS)) {
- // FIXME: Maybe this should be an assertion. Are there cases
- // were it is not an EnumConstantDecl?
- const EnumConstantDecl *D =
+ if (const DeclRefExpr *DR = dyn_cast<DeclRefExpr>(LHS)) {
+ // FIXME: Maybe this should be an assertion. Are there cases
+ // were it is not an EnumConstantDecl?
+ const EnumConstantDecl *D =
dyn_cast<EnumConstantDecl>(DR->getDecl());
- if (D) {
- GetRawInt = false;
- os << *D;
- }
+ if (D) {
+ GetRawInt = false;
+ os << *D;
}
-
- if (GetRawInt)
- os << LHS->EvaluateKnownConstInt(PDB.getASTContext());
-
- os << ":' at line "
- << End.asLocation().getExpansionLineNumber();
- break;
}
+
+ if (GetRawInt)
+ os << LHS->EvaluateKnownConstInt(PDB.getASTContext());
+
+ os << ":' at line "
+ << End.asLocation().getExpansionLineNumber();
+ break;
}
- PD.getActivePath().push_front(new PathDiagnosticControlFlowPiece(Start, End,
- os.str()));
+ }
+ PD.getActivePath().push_front(new PathDiagnosticControlFlowPiece(
+ Start, End, os.str()));
}
else {
os << "'Default' branch taken. ";
const PathDiagnosticLocation &End = PDB.ExecutionContinues(os, N);
- PD.getActivePath().push_front(new PathDiagnosticControlFlowPiece(Start, End,
- os.str()));
+ PD.getActivePath().push_front(new PathDiagnosticControlFlowPiece(
+ Start, End, os.str()));
}
break;
@@ -565,12 +576,12 @@
std::string sbuf;
llvm::raw_string_ostream os(sbuf);
PathDiagnosticLocation End = PDB.ExecutionContinues(os, N);
- PD.getActivePath().push_front(new PathDiagnosticControlFlowPiece(Start, End,
- os.str()));
+ PD.getActivePath().push_front(new PathDiagnosticControlFlowPiece(
+ Start, End, os.str()));
break;
}
- // Determine control-flow for ternary '?'.
+ // Determine control-flow for ternary '?'.
case Stmt::BinaryConditionalOperatorClass:
case Stmt::ConditionalOperatorClass: {
std::string sbuf;
@@ -587,12 +598,12 @@
if (const Stmt *S = End.asStmt())
End = PDB.getEnclosingStmtLocation(S);
- PD.getActivePath().push_front(new PathDiagnosticControlFlowPiece(Start, End,
- os.str()));
+ PD.getActivePath().push_front(new PathDiagnosticControlFlowPiece(
+ Start, End, os.str()));
break;
}
- // Determine control-flow for short-circuited '&&' and '||'.
+ // Determine control-flow for short-circuited '&&' and '||'.
case Stmt::BinaryOperatorClass: {
if (!PDB.supportsLogicalOpControlFlow())
break;
@@ -609,16 +620,16 @@
os << "false";
PathDiagnosticLocation End(B->getLHS(), SMgr, LC);
PathDiagnosticLocation Start =
- PathDiagnosticLocation::createOperatorLoc(B, SMgr);
- PD.getActivePath().push_front(new PathDiagnosticControlFlowPiece(Start, End,
- os.str()));
+ PathDiagnosticLocation::createOperatorLoc(B, SMgr);
+ PD.getActivePath().push_front(new PathDiagnosticControlFlowPiece(
+ Start, End, os.str()));
}
else {
os << "true";
PathDiagnosticLocation Start(B->getLHS(), SMgr, LC);
PathDiagnosticLocation End = PDB.ExecutionContinues(N);
- PD.getActivePath().push_front(new PathDiagnosticControlFlowPiece(Start, End,
- os.str()));
+ PD.getActivePath().push_front(new PathDiagnosticControlFlowPiece(
+ Start, End, os.str()));
}
}
else {
@@ -629,16 +640,16 @@
os << "false";
PathDiagnosticLocation Start(B->getLHS(), SMgr, LC);
PathDiagnosticLocation End = PDB.ExecutionContinues(N);
- PD.getActivePath().push_front(new PathDiagnosticControlFlowPiece(Start, End,
- os.str()));
+ PD.getActivePath().push_front(new PathDiagnosticControlFlowPiece(
+ Start, End, os.str()));
}
else {
os << "true";
PathDiagnosticLocation End(B->getLHS(), SMgr, LC);
PathDiagnosticLocation Start =
- PathDiagnosticLocation::createOperatorLoc(B, SMgr);
- PD.getActivePath().push_front(new PathDiagnosticControlFlowPiece(Start, End,
- os.str()));
+ PathDiagnosticLocation::createOperatorLoc(B, SMgr);
+ PD.getActivePath().push_front(new PathDiagnosticControlFlowPiece(
+ Start, End, os.str()));
}
}
@@ -656,8 +667,8 @@
if (const Stmt *S = End.asStmt())
End = PDB.getEnclosingStmtLocation(S);
- PD.getActivePath().push_front(new PathDiagnosticControlFlowPiece(Start, End,
- os.str()));
+ PD.getActivePath().push_front(new PathDiagnosticControlFlowPiece(
+ Start, End, os.str()));
}
else {
PathDiagnosticLocation End = PDB.ExecutionContinues(N);
@@ -665,8 +676,8 @@
if (const Stmt *S = End.asStmt())
End = PDB.getEnclosingStmtLocation(S);
- PD.getActivePath().push_front(new PathDiagnosticControlFlowPiece(Start, End,
- "Loop condition is false. Exiting loop"));
+ PD.getActivePath().push_front(new PathDiagnosticControlFlowPiece(
+ Start, End, "Loop condition is false. Exiting loop"));
}
break;
@@ -683,16 +694,16 @@
if (const Stmt *S = End.asStmt())
End = PDB.getEnclosingStmtLocation(S);
- PD.getActivePath().push_front(new PathDiagnosticControlFlowPiece(Start, End,
- os.str()));
+ PD.getActivePath().push_front(new PathDiagnosticControlFlowPiece(
+ Start, End, os.str()));
}
else {
PathDiagnosticLocation End = PDB.ExecutionContinues(N);
if (const Stmt *S = End.asStmt())
End = PDB.getEnclosingStmtLocation(S);
- PD.getActivePath().push_front(new PathDiagnosticControlFlowPiece(Start, End,
- "Loop condition is true. Entering loop body"));
+ PD.getActivePath().push_front(new PathDiagnosticControlFlowPiece(
+ Start, End, "Loop condition is true. Entering loop body"));
}
break;
@@ -705,16 +716,17 @@
End = PDB.getEnclosingStmtLocation(S);
if (*(Src->succ_begin()+1) == Dst)
- PD.getActivePath().push_front(new PathDiagnosticControlFlowPiece(Start, End,
- "Taking false branch"));
+ PD.getActivePath().push_front(new PathDiagnosticControlFlowPiece(
+ Start, End, "Taking false branch"));
else
- PD.getActivePath().push_front(new PathDiagnosticControlFlowPiece(Start, End,
- "Taking true branch"));
+ PD.getActivePath().push_front(new PathDiagnosticControlFlowPiece(
+ Start, End, "Taking true branch"));
break;
}
+ }
}
- }
+ } while(0);
if (NextNode) {
// Add diagnostic pieces from custom visitors.
@@ -1166,6 +1178,8 @@
PathDiagnosticCallPiece *C =
PathDiagnosticCallPiece::construct(N, *CE, SM);
+ GRBugReporter& BR = PDB.getBugReporter();
+ BR.addCallPieceLocationContextPair(C, CE->getCalleeContext());
EB.addEdge(C->callReturn, true);
EB.flushLocations();
@@ -1202,6 +1216,8 @@
} else {
const Decl *Caller = CE->getLocationContext()->getDecl();
C = PathDiagnosticCallPiece::construct(PD.getActivePath(), Caller);
+ GRBugReporter& BR = PDB.getBugReporter();
+ BR.addCallPieceLocationContextPair(C, CE->getCalleeContext());
}
C->setCallee(*CE, SM);
@@ -1414,6 +1430,12 @@
markInteresting(V.getAsSymbol());
}
+void BugReport::markInteresting(const LocationContext *LC) {
+ if (!LC)
+ return;
+ InterestingLocationContexts.insert(LC);
+}
+
bool BugReport::isInteresting(SVal V) {
return isInteresting(V.getAsRegion()) || isInteresting(V.getAsSymbol());
}
@@ -1438,6 +1460,12 @@
return false;
}
+bool BugReport::isInteresting(const LocationContext *LC) {
+ if (!LC)
+ return false;
+ return InterestingLocationContexts.count(LC);
+}
+
void BugReport::lazyInitializeInterestingSets() {
if (interestingSymbols.empty()) {
interestingSymbols.push_back(new Symbols());
@@ -1911,7 +1939,7 @@
// Finally, prune the diagnostic path of uninteresting stuff.
if (R->shouldPrunePath()) {
- bool hasSomethingInteresting = RemoveUneededCalls(PD.getMutablePieces());
+ bool hasSomethingInteresting = RemoveUneededCalls(PD.getMutablePieces(), R);
assert(hasSomethingInteresting);
(void) hasSomethingInteresting;
}
diff --git a/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp b/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
index a1b7e30..521b727 100644
--- a/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ b/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -17,6 +17,7 @@
#include "clang/AST/ExprObjC.h"
#include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
#include "clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h"
@@ -478,6 +479,7 @@
SVal V = state->getRawSVal(loc::MemRegionVal(R));
report.markInteresting(R);
report.markInteresting(V);
+ report.addVisitor(new UndefOrNullArgVisitor(R));
// If the contents are symbolic, find out when they became null.
if (V.getAsLocSymbol()) {
@@ -503,6 +505,8 @@
// Is it a symbolic value?
if (loc::MemRegionVal *L = dyn_cast<loc::MemRegionVal>(&V)) {
const MemRegion *Base = L->getRegion()->getBaseRegion();
+ report.addVisitor(new UndefOrNullArgVisitor(Base));
+
if (isa<SymbolicRegion>(Base)) {
report.markInteresting(Base);
report.addVisitor(new TrackConstraintBRVisitor(loc::MemRegionVal(Base),
@@ -950,3 +954,54 @@
return event;
}
+PathDiagnosticPiece *
+UndefOrNullArgVisitor::VisitNode(const ExplodedNode *N,
+ const ExplodedNode *PrevN,
+ BugReporterContext &BRC,
+ BugReport &BR) {
+
+ ProgramStateRef State = N->getState();
+ ProgramPoint ProgLoc = N->getLocation();
+
+ // We are only interested in visiting CallEnter nodes.
+ CallEnter *CEnter = dyn_cast<CallEnter>(&ProgLoc);
+ if (!CEnter)
+ return 0;
+
+ // Check if one of the arguments is the region the visitor is tracking.
+ CallEventManager &CEMgr = BRC.getStateManager().getCallEventManager();
+ CallEventRef<> Call = CEMgr.getCaller(CEnter->getCalleeContext(), State);
+ unsigned Idx = 0;
+ for (CallEvent::param_iterator I = Call->param_begin(),
+ E = Call->param_end(); I != E; ++I, ++Idx) {
+ const MemRegion *ArgReg = Call->getArgSVal(Idx).getAsRegion();
+
+ // Are we tracking the argument?
+ if ( !ArgReg || ArgReg != R)
+ return 0;
+
+ // Check the function parameter type.
+ const ParmVarDecl *ParamDecl = *I;
+ assert(ParamDecl && "Formal parameter has no decl?");
+ QualType T = ParamDecl->getType();
+
+ if (!(T->isAnyPointerType() || T->isReferenceType())) {
+ // Function can only change the value passed in by address.
+ return 0;
+ }
+
+ // If it is a const pointer value, the function does not intend to
+ // change the value.
+ if (T->getPointeeType().isConstQualified())
+ return 0;
+
+ // Mark the call site (LocationContext) as interesting if the value of the
+ // argument is undefined or '0'/'NULL'.
+ SVal BoundVal = State->getSVal(ArgReg);
+ if (BoundVal.isUndef() || BoundVal.isZeroConstant()) {
+ BR.markInteresting(CEnter->getCalleeContext());
+ return 0;
+ }
+ }
+ return 0;
+}