From: Ted Kremenek Date: Wed, 2 May 2012 00:31:29 +0000 (+0000) Subject: Refine analyzer diagnostics by adding an expression "cone-of-influence" to reverse... X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=11abcecc8c919673237cf37384290a1ef1943976;p=clang Refine analyzer diagnostics by adding an expression "cone-of-influence" to reverse track interesting values through interesting expressions. This allows us to map from interesting values in a caller to interesting values in a caller, thus recovering some precision in diagnostics lost from IPA. Fixes git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@155971 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/lib/StaticAnalyzer/Core/BugReporter.cpp b/lib/StaticAnalyzer/Core/BugReporter.cpp index 4330d2ed47..c774818edf 100644 --- a/lib/StaticAnalyzer/Core/BugReporter.cpp +++ b/lib/StaticAnalyzer/Core/BugReporter.cpp @@ -1051,6 +1051,79 @@ void EdgeBuilder::addContext(const Stmt *S) { CLocs.push_back(L); } +// Cone-of-influence: support the reverse propagation of "interesting" symbols +// and values by tracing interesting calculations backwards through evaluated +// expressions along a path. This is probably overly complicated, but the idea +// is that if an expression computed an "interesting" value, the child +// expressions are are also likely to be "interesting" as well (which then +// propagates to the values they in turn compute). This reverse propagation +// is needed to track interesting correlations across function call boundaries, +// where formal arguments bind to actual arguments, etc. This is also needed +// because the constraint solver sometimes simplifies certain symbolic values +// into constants when appropriate, and this complicates reasoning about +// interesting values. +typedef llvm::DenseSet InterestingExprs; + +static void reversePropagateIntererstingSymbols(BugReport &R, + InterestingExprs &IE, + const ProgramState *State, + const Expr *Ex, + const LocationContext *LCtx) { + SVal V = State->getSVal(Ex, LCtx); + if (!(R.isInteresting(V) || IE.count(Ex))) + return; + + switch (Ex->getStmtClass()) { + default: + if (!isa(Ex)) + break; + // Fall through. + case Stmt::BinaryOperatorClass: + case Stmt::UnaryOperatorClass: { + for (Stmt::const_child_iterator CI = Ex->child_begin(), + CE = Ex->child_end(); + CI != CE; ++CI) { + if (const Expr *child = dyn_cast_or_null(*CI)) { + IE.insert(child); + SVal ChildV = State->getSVal(child, LCtx); + R.markInteresting(ChildV); + } + break; + } + } + } + + R.markInteresting(V); +} + +static void reversePropagateInterestingSymbols(BugReport &R, + InterestingExprs &IE, + const ProgramState *State, + const LocationContext *CalleeCtx, + const LocationContext *CallerCtx) +{ + // FIXME: Handle CXXConstructExpr. + // FIXME: Handle calls to blocks. + const StackFrameContext *Callee = CalleeCtx->getCurrentStackFrame(); + const Stmt *CallSite = Callee->getCallSite(); + if (const CallExpr *CE = dyn_cast(CallSite)) { + if (const FunctionDecl *FD = dyn_cast(CalleeCtx->getDecl())) { + FunctionDecl::param_const_iterator PI = FD->param_begin(), + PE = FD->param_end(); + CallExpr::const_arg_iterator AI = CE->arg_begin(), AE = CE->arg_end(); + for (; AI != AE && PI != PE; ++AI, ++PI) { + if (const Expr *ArgE = *AI) { + if (const ParmVarDecl *PD = *PI) { + Loc LV = State->getLValue(PD, CalleeCtx); + if (R.isInteresting(LV) || R.isInteresting(State->getRawSVal(LV))) + IE.insert(ArgE); + } + } + } + } + } +} + static void GenerateExtensivePathDiagnostic(PathDiagnostic& PD, PathDiagnosticBuilder &PDB, const ExplodedNode *N, @@ -1058,6 +1131,7 @@ static void GenerateExtensivePathDiagnostic(PathDiagnostic& PD, EdgeBuilder EB(PD, PDB); const SourceManager& SM = PDB.getSourceManager(); StackDiagVector CallStack; + InterestingExprs IE; const ExplodedNode *NextNode = N->pred_empty() ? NULL : *(N->pred_begin()); while (NextNode) { @@ -1066,6 +1140,13 @@ static void GenerateExtensivePathDiagnostic(PathDiagnostic& PD, ProgramPoint P = N->getLocation(); do { + if (const PostStmt *PS = dyn_cast(&P)) { + if (const Expr *Ex = PS->getStmtAs()) + reversePropagateIntererstingSymbols(*PDB.getBugReport(), IE, + N->getState().getPtr(), Ex, + N->getLocationContext()); + } + if (const CallExitEnd *CE = dyn_cast(&P)) { const StackFrameContext *LCtx = CE->getLocationContext()->getCurrentStackFrame(); @@ -1127,7 +1208,19 @@ static void GenerateExtensivePathDiagnostic(PathDiagnostic& PD, PDB.LC = N->getLocationContext(); // Block edges. - if (const BlockEdge *BE = dyn_cast(&P)) { + if (const BlockEdge *BE = dyn_cast(&P)) { + // Does this represent entering a call? If so, look at propagating + // interesting symbols across call boundaries. + if (NextNode) { + const LocationContext *CallerCtx = NextNode->getLocationContext(); + const LocationContext *CalleeCtx = PDB.LC; + if (CallerCtx != CalleeCtx) { + reversePropagateInterestingSymbols(*PDB.getBugReport(), IE, + N->getState().getPtr(), + CalleeCtx, CallerCtx); + } + } + const CFGBlock &Blk = *BE->getSrc(); const Stmt *Term = Blk.getTerminator(); diff --git a/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp b/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp index 6532486851..4afc874f20 100644 --- a/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp +++ b/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp @@ -34,15 +34,23 @@ const Stmt *bugreporter::GetDerefExpr(const ExplodedNode *N) { // a[0], p->f, *p const Stmt *S = N->getLocationAs()->getStmt(); - if (const UnaryOperator *U = dyn_cast(S)) { - if (U->getOpcode() == UO_Deref) - return U->getSubExpr()->IgnoreParenCasts(); - } - else if (const MemberExpr *ME = dyn_cast(S)) { - return ME->getBase()->IgnoreParenCasts(); - } - else if (const ArraySubscriptExpr *AE = dyn_cast(S)) { - return AE->getBase(); + while (true) { + if (const BinaryOperator *B = dyn_cast(S)) { + assert(B->isAssignmentOp()); + S = B->getLHS()->IgnoreParenCasts(); + continue; + } + else if (const UnaryOperator *U = dyn_cast(S)) { + if (U->getOpcode() == UO_Deref) + return U->getSubExpr()->IgnoreParenCasts(); + } + else if (const MemberExpr *ME = dyn_cast(S)) { + return ME->getBase()->IgnoreParenCasts(); + } + else if (const ArraySubscriptExpr *AE = dyn_cast(S)) { + return AE->getBase(); + } + break; } return NULL; @@ -320,7 +328,7 @@ bugreporter::getTrackNullOrUndefValueVisitor(const ExplodedNode *N, StateMgr.getRegionManager().getVarRegion(VD, N->getLocationContext()); // What did we load? - SVal V = state->getSVal(loc::MemRegionVal(R)); + SVal V = state->getRawSVal(loc::MemRegionVal(R)); report->markInteresting(R); report->markInteresting(V); return new FindLastStoreBRVisitor(V, R); diff --git a/test/Analysis/inline-plist.c b/test/Analysis/inline-plist.c index 549082dc9c..0510e8f21d 100644 --- a/test/Analysis/inline-plist.c +++ b/test/Analysis/inline-plist.c @@ -23,6 +23,19 @@ void test_has_bug() { has_bug(0); } +void triggers_bug(int *p) { + *p = 0xDEADBEEF; +} + +// This function triggers a bug by calling triggers_bug(). The diagnostics +// should show when p is assumed to be null. +void bar(int *p) { + if (!!p) + return; + + if (p == 0) + triggers_bug(p); +} // CHECK: // CHECK: @@ -364,6 +377,259 @@ void test_has_bug() { // CHECK: file0 // CHECK: // CHECK: +// CHECK: +// CHECK: path +// CHECK: +// CHECK: +// CHECK: kindcontrol +// CHECK: edges +// CHECK: +// CHECK: +// CHECK: start +// CHECK: +// CHECK: +// CHECK: line33 +// CHECK: col3 +// CHECK: file0 +// CHECK: +// CHECK: +// CHECK: line33 +// CHECK: col3 +// CHECK: file0 +// CHECK: +// CHECK: +// CHECK: end +// CHECK: +// CHECK: +// CHECK: line33 +// CHECK: col8 +// CHECK: file0 +// CHECK: +// CHECK: +// CHECK: line33 +// CHECK: col9 +// CHECK: file0 +// CHECK: +// CHECK: +// CHECK: +// CHECK: +// CHECK: +// CHECK: +// CHECK: kindevent +// CHECK: location +// CHECK: +// CHECK: line33 +// CHECK: col8 +// CHECK: file0 +// CHECK: +// CHECK: ranges +// CHECK: +// CHECK: +// CHECK: +// CHECK: line33 +// CHECK: col8 +// CHECK: file0 +// CHECK: +// CHECK: +// CHECK: line33 +// CHECK: col9 +// CHECK: file0 +// CHECK: +// CHECK: +// CHECK: +// CHECK: depth0 +// CHECK: extended_message +// CHECK: Assuming 'p' is null +// CHECK: message +// CHECK: Assuming 'p' is null +// CHECK: +// CHECK: +// CHECK: kindcontrol +// CHECK: edges +// CHECK: +// CHECK: +// CHECK: start +// CHECK: +// CHECK: +// CHECK: line33 +// CHECK: col8 +// CHECK: file0 +// CHECK: +// CHECK: +// CHECK: line33 +// CHECK: col9 +// CHECK: file0 +// CHECK: +// CHECK: +// CHECK: end +// CHECK: +// CHECK: +// CHECK: line36 +// CHECK: col3 +// CHECK: file0 +// CHECK: +// CHECK: +// CHECK: line36 +// CHECK: col3 +// CHECK: file0 +// CHECK: +// CHECK: +// CHECK: +// CHECK: +// CHECK: +// CHECK: +// CHECK: kindcontrol +// CHECK: edges +// CHECK: +// CHECK: +// CHECK: start +// CHECK: +// CHECK: +// CHECK: line36 +// CHECK: col3 +// CHECK: file0 +// CHECK: +// CHECK: +// CHECK: line36 +// CHECK: col3 +// CHECK: file0 +// CHECK: +// CHECK: +// CHECK: end +// CHECK: +// CHECK: +// CHECK: line37 +// CHECK: col5 +// CHECK: file0 +// CHECK: +// CHECK: +// CHECK: line37 +// CHECK: col5 +// CHECK: file0 +// CHECK: +// CHECK: +// CHECK: +// CHECK: +// CHECK: +// CHECK: +// CHECK: kindevent +// CHECK: location +// CHECK: +// CHECK: line37 +// CHECK: col5 +// CHECK: file0 +// CHECK: +// CHECK: ranges +// CHECK: +// CHECK: +// CHECK: +// CHECK: line37 +// CHECK: col5 +// CHECK: file0 +// CHECK: +// CHECK: +// CHECK: line37 +// CHECK: col19 +// CHECK: file0 +// CHECK: +// CHECK: +// CHECK: +// CHECK: depth0 +// CHECK: extended_message +// CHECK: Calling 'triggers_bug' +// CHECK: message +// CHECK: Calling 'triggers_bug' +// CHECK: +// CHECK: +// CHECK: kindevent +// CHECK: location +// CHECK: +// CHECK: line26 +// CHECK: col1 +// CHECK: file0 +// CHECK: +// CHECK: depth1 +// CHECK: extended_message +// CHECK: Entered call from 'bar' +// CHECK: message +// CHECK: Entered call from 'bar' +// CHECK: +// CHECK: +// CHECK: kindcontrol +// CHECK: edges +// CHECK: +// CHECK: +// CHECK: start +// CHECK: +// CHECK: +// CHECK: line26 +// CHECK: col1 +// CHECK: file0 +// CHECK: +// CHECK: +// CHECK: line26 +// CHECK: col1 +// CHECK: file0 +// CHECK: +// CHECK: +// CHECK: end +// CHECK: +// CHECK: +// CHECK: line27 +// CHECK: col3 +// CHECK: file0 +// CHECK: +// CHECK: +// CHECK: line27 +// CHECK: col3 +// CHECK: file0 +// CHECK: +// CHECK: +// CHECK: +// CHECK: +// CHECK: +// CHECK: +// CHECK: kindevent +// CHECK: location +// CHECK: +// CHECK: line27 +// CHECK: col3 +// CHECK: file0 +// CHECK: +// CHECK: ranges +// CHECK: +// CHECK: +// CHECK: +// CHECK: line27 +// CHECK: col4 +// CHECK: file0 +// CHECK: +// CHECK: +// CHECK: line27 +// CHECK: col4 +// CHECK: file0 +// CHECK: +// CHECK: +// CHECK: +// CHECK: depth1 +// CHECK: extended_message +// CHECK: Dereference of null pointer (loaded from variable 'p') +// CHECK: message +// CHECK: Dereference of null pointer (loaded from variable 'p') +// CHECK: +// CHECK: +// CHECK: descriptionDereference of null pointer (loaded from variable 'p') +// CHECK: categoryLogic error +// CHECK: typeDereference of null pointer +// CHECK: issue_context_kindfunction +// CHECK: issue_contexttriggers_bug +// CHECK: location +// CHECK: +// CHECK: line27 +// CHECK: col3 +// CHECK: file0 +// CHECK: +// CHECK: // CHECK: // CHECK: // CHECK: diff --git a/test/Analysis/malloc-plist.c b/test/Analysis/malloc-plist.c index 440be1d5f5..8a36ab3085 100644 --- a/test/Analysis/malloc-plist.c +++ b/test/Analysis/malloc-plist.c @@ -4054,40 +4054,6 @@ void use_function_with_leak7() { //CHECK: //CHECK: //CHECK: line142 -//CHECK: col5 -//CHECK: file0 -//CHECK: -//CHECK: -//CHECK: line142 -//CHECK: col5 -//CHECK: file0 -//CHECK: -//CHECK: -//CHECK: -//CHECK: -//CHECK: -//CHECK: -//CHECK: kindcontrol -//CHECK: edges -//CHECK: -//CHECK: -//CHECK: start -//CHECK: -//CHECK: -//CHECK: line142 -//CHECK: col5 -//CHECK: file0 -//CHECK: -//CHECK: -//CHECK: line142 -//CHECK: col5 -//CHECK: file0 -//CHECK: -//CHECK: -//CHECK: end -//CHECK: -//CHECK: -//CHECK: line142 //CHECK: col12 //CHECK: file0 //CHECK: