[cfe-commits] r52555 - in /cfe/trunk: include/clang/Analysis/LocalCheckers.h include/clang/Analysis/PathSensitive/BugReporter.h include/clang/Analysis/PathSensitive/GRCoreEngine.h include/clang/Analysis/PathSensitive/GRExprEngine.h lib/Analysis/BugReporter.cpp lib/Analysis/DeadStores.cpp lib/Analysis/GRCoreEngine.cpp lib/Analysis/GRExprEngine.cpp test/Analysis/dead-stores.c
Ted Kremenek
kremenek at apple.com
Fri Jun 20 14:45:26 PDT 2008
Author: kremenek
Date: Fri Jun 20 16:45:25 2008
New Revision: 52555
URL: http://llvm.org/viewvc/llvm-project?rev=52555&view=rev
Log:
Modified the dead stores checker to...
1) Check if a dead store appears as a subexpression. For such cases, we emit
a verbose diagnostic so that users aren't confused. This addresses:
<rdar://problem/5968508> checker gives misleading report for dead store in loop
2) Don't emit a dead store warning when assigning a null value to a pointer.
This is a common form of defensive programming. We may wish to make
this an option to the the checker one day.
This addresses the feature request in the following email:
http://lists.cs.uiuc.edu/pipermail/cfe-dev/2008-June/001978.html
Modified:
cfe/trunk/include/clang/Analysis/LocalCheckers.h
cfe/trunk/include/clang/Analysis/PathSensitive/BugReporter.h
cfe/trunk/include/clang/Analysis/PathSensitive/GRCoreEngine.h
cfe/trunk/include/clang/Analysis/PathSensitive/GRExprEngine.h
cfe/trunk/lib/Analysis/BugReporter.cpp
cfe/trunk/lib/Analysis/DeadStores.cpp
cfe/trunk/lib/Analysis/GRCoreEngine.cpp
cfe/trunk/lib/Analysis/GRExprEngine.cpp
cfe/trunk/test/Analysis/dead-stores.c
Modified: cfe/trunk/include/clang/Analysis/LocalCheckers.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Analysis/LocalCheckers.h?rev=52555&r1=52554&r2=52555&view=diff
==============================================================================
--- cfe/trunk/include/clang/Analysis/LocalCheckers.h (original)
+++ cfe/trunk/include/clang/Analysis/LocalCheckers.h Fri Jun 20 16:45:25 2008
@@ -25,8 +25,10 @@
class GRTransferFuncs;
class BugType;
class LangOptions;
+class ParentMap;
-void CheckDeadStores(CFG& cfg, ASTContext &Ctx, Diagnostic &Diags);
+void CheckDeadStores(CFG& cfg, ASTContext &Ctx, ParentMap& Parents,
+ Diagnostic &Diags);
void CheckUninitializedValues(CFG& cfg, ASTContext& Ctx, Diagnostic& Diags,
bool FullUninitTaint=false);
Modified: cfe/trunk/include/clang/Analysis/PathSensitive/BugReporter.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Analysis/PathSensitive/BugReporter.h?rev=52555&r1=52554&r2=52555&view=diff
==============================================================================
--- cfe/trunk/include/clang/Analysis/PathSensitive/BugReporter.h (original)
+++ cfe/trunk/include/clang/Analysis/PathSensitive/BugReporter.h Fri Jun 20 16:45:25 2008
@@ -34,6 +34,7 @@
class ValueState;
class Stmt;
class BugReport;
+class ParentMap;
class BugType {
public:
@@ -156,6 +157,8 @@
CFG& getCFG() { return getGraph().getCFG(); }
+ ParentMap& getParentMap();
+
void EmitWarning(BugReport& R);
void GeneratePathDiagnostic(PathDiagnostic& PD, BugReport& R);
Modified: cfe/trunk/include/clang/Analysis/PathSensitive/GRCoreEngine.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Analysis/PathSensitive/GRCoreEngine.h?rev=52555&r1=52554&r2=52555&view=diff
==============================================================================
--- cfe/trunk/include/clang/Analysis/PathSensitive/GRCoreEngine.h (original)
+++ cfe/trunk/include/clang/Analysis/PathSensitive/GRCoreEngine.h Fri Jun 20 16:45:25 2008
@@ -47,19 +47,10 @@
friend class GRIndirectGotoNodeBuilderImpl;
friend class GRSwitchNodeBuilderImpl;
friend class GREndPathNodeBuilderImpl;
-
- typedef llvm::DenseMap<Stmt*,Stmt*> ParentMapTy;
/// G - The simulation graph. Each node is a (location,state) pair.
llvm::OwningPtr<ExplodedGraphImpl> G;
-
- /// ParentMap - A lazily populated map from a Stmt* to its parent Stmt*.
- void* ParentMap;
-
- /// CurrentBlkExpr - The current Block-level expression being processed.
- /// This is used when lazily populating ParentMap.
- Stmt* CurrentBlkExpr;
-
+
/// WList - A set of queued nodes that need to be processed by the
/// worklist algorithm. It is up to the implementation of WList to decide
/// the order that nodes are processed.
Modified: cfe/trunk/include/clang/Analysis/PathSensitive/GRExprEngine.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Analysis/PathSensitive/GRExprEngine.h?rev=52555&r1=52554&r2=52555&view=diff
==============================================================================
--- cfe/trunk/include/clang/Analysis/PathSensitive/GRExprEngine.h (original)
+++ cfe/trunk/include/clang/Analysis/PathSensitive/GRExprEngine.h Fri Jun 20 16:45:25 2008
@@ -28,6 +28,7 @@
class BugType;
class PathDiagnosticClient;
class Diagnostic;
+ class ParentMap;
class GRExprEngine {
@@ -51,6 +52,9 @@
/// G - the simulation graph.
GraphTy& G;
+ /// Parents - a lazily created map from Stmt* to parents.
+ ParentMap* Parents;
+
/// Liveness - live-variables information the ValueDecl* and block-level
/// Expr* in the CFG. Used to prune out dead state.
LiveVariables Liveness;
@@ -213,6 +217,10 @@
GraphTy& getGraph() { return G; }
const GraphTy& getGraph() const { return G; }
+
+ /// getParentMap - Return a map from Stmt* to parents for the function/method
+ /// body being analyzed. This map is lazily constructed as needed.
+ ParentMap& getParentMap();
typedef BugTypeSet::iterator bug_type_iterator;
typedef BugTypeSet::const_iterator const_bug_type_iterator;
Modified: cfe/trunk/lib/Analysis/BugReporter.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/BugReporter.cpp?rev=52555&r1=52554&r2=52555&view=diff
==============================================================================
--- cfe/trunk/lib/Analysis/BugReporter.cpp (original)
+++ cfe/trunk/lib/Analysis/BugReporter.cpp Fri Jun 20 16:45:25 2008
@@ -39,6 +39,10 @@
return Eng.getStateManager();
}
+ParentMap& BugReporter::getParentMap() {
+ return Eng.getParentMap();
+}
+
static inline Stmt* GetStmt(const ProgramPoint& P) {
if (const PostStmt* PS = dyn_cast<PostStmt>(&P)) {
return PS->getStmt();
Modified: cfe/trunk/lib/Analysis/DeadStores.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/DeadStores.cpp?rev=52555&r1=52554&r2=52555&view=diff
==============================================================================
--- cfe/trunk/lib/Analysis/DeadStores.cpp (original)
+++ cfe/trunk/lib/Analysis/DeadStores.cpp Fri Jun 20 16:45:25 2008
@@ -19,28 +19,49 @@
#include "clang/Analysis/PathSensitive/GRExprEngine.h"
#include "clang/Basic/Diagnostic.h"
#include "clang/AST/ASTContext.h"
+#include "clang/AST/ParentMap.h"
#include "llvm/Support/Compiler.h"
using namespace clang;
namespace {
-
+
class VISIBILITY_HIDDEN DeadStoreObs : public LiveVariables::ObserverTy {
ASTContext &Ctx;
Diagnostic &Diags;
DiagnosticClient &Client;
+ ParentMap& Parents;
+
public:
- DeadStoreObs(ASTContext &ctx, Diagnostic &diags, DiagnosticClient &client)
- : Ctx(ctx), Diags(diags), Client(client) {}
+ DeadStoreObs(ASTContext &ctx, Diagnostic &diags, DiagnosticClient &client,
+ ParentMap& parents)
+ : Ctx(ctx), Diags(diags), Client(client), Parents(parents) {}
virtual ~DeadStoreObs() {}
- unsigned GetDiag(VarDecl* VD) {
- std::string msg = "value stored to '" + std::string(VD->getName()) +
- "' is never used";
+ unsigned GetDiag(VarDecl* VD, bool inEnclosing = false) {
+ std::string name(VD->getName());
+
+ std::string msg = inEnclosing
+ ? "Although the value stored to '" + name +
+ "' is used in the enclosing expression, the value is never actually read"
+ " from '" + name + "'"
+ : "Value stored to '" + name + "' is never read";
- return Diags.getCustomDiagID(Diagnostic::Warning, msg.c_str());
-
+ return Diags.getCustomDiagID(Diagnostic::Warning, msg.c_str());
+ }
+
+ void CheckVarDecl(VarDecl* VD, Expr* Ex, Expr* Val,
+ bool hasEnclosing,
+ const LiveVariables::AnalysisDataTy& AD,
+ const LiveVariables::ValTy& Live) {
+
+ if (VD->hasLocalStorage() && !Live(VD, AD)) {
+ SourceRange R = Val->getSourceRange();
+ Diags.Report(&Client,
+ Ctx.getFullLoc(Ex->getSourceRange().getBegin()),
+ GetDiag(VD, hasEnclosing), 0, 0, &R, 1);
+ }
}
void CheckDeclRef(DeclRefExpr* DR, Expr* Val,
@@ -48,12 +69,7 @@
const LiveVariables::ValTy& Live) {
if (VarDecl* VD = dyn_cast<VarDecl>(DR->getDecl()))
- if (VD->hasLocalStorage() && !Live(VD, AD)) {
- SourceRange R = Val->getSourceRange();
- Diags.Report(&Client,
- Ctx.getFullLoc(DR->getSourceRange().getBegin()),
- GetDiag(VD), 0, 0, &R, 1);
- }
+ CheckVarDecl(VD, DR, Val, false, AD, Live);
}
virtual void ObserveStmt(Stmt* S,
@@ -68,7 +84,22 @@
if (!B->isAssignmentOp()) return; // Skip non-assignments.
if (DeclRefExpr* DR = dyn_cast<DeclRefExpr>(B->getLHS()))
- CheckDeclRef(DR, B->getRHS(), AD, Live);
+ if (VarDecl *VD = dyn_cast<VarDecl>(DR->getDecl())) {
+
+ // Special case: check for assigning null to a pointer. This
+ // is a common form of defensive programming.
+ // FIXME: Make this optional?
+
+ Expr* Val = B->getRHS();
+ llvm::APSInt Result(Ctx.getTypeSize(Val->getType()));
+
+ if (VD->getType()->isPointerType() &&
+ Val->IgnoreParenCasts()->isIntegerConstantExpr(Result, Ctx, 0))
+ if (Result == 0)
+ return;
+
+ CheckVarDecl(VD, DR, Val, Parents.isSubExpr(B), AD, Live);
+ }
}
else if (UnaryOperator* U = dyn_cast<UnaryOperator>(S)) {
if (!U->isIncrementOp())
@@ -116,10 +147,11 @@
// Driver function to invoke the Dead-Stores checker on a CFG.
//===----------------------------------------------------------------------===//
-void clang::CheckDeadStores(CFG& cfg, ASTContext &Ctx, Diagnostic &Diags) {
+void clang::CheckDeadStores(CFG& cfg, ASTContext &Ctx,
+ ParentMap& Parents, Diagnostic &Diags) {
LiveVariables L(cfg);
L.runOnCFG(cfg);
- DeadStoreObs A(Ctx, Diags, Diags.getClient());
+ DeadStoreObs A(Ctx, Diags, Diags.getClient(), Parents);
L.runOnAllBlocks(cfg, &A);
}
@@ -197,9 +229,10 @@
// Run the dead store checker and collect the diagnostics.
DiagCollector C(*this);
- DeadStoreObs A(BR.getContext(), BR.getDiagnostic(), C);
- GRExprEngine& Eng = BR.getEngine();
- Eng.getLiveness().runOnAllBlocks(Eng.getCFG(), &A);
+ DeadStoreObs A(BR.getContext(), BR.getDiagnostic(), C, BR.getParentMap());
+
+ GRExprEngine& Eng = BR.getEngine();
+ Eng.getLiveness().runOnAllBlocks(BR.getCFG(), &A);
// Emit the bug reports.
Modified: cfe/trunk/lib/Analysis/GRCoreEngine.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/GRCoreEngine.cpp?rev=52555&r1=52554&r2=52555&view=diff
==============================================================================
--- cfe/trunk/lib/Analysis/GRCoreEngine.cpp (original)
+++ cfe/trunk/lib/Analysis/GRCoreEngine.cpp Fri Jun 20 16:45:25 2008
@@ -253,19 +253,6 @@
}
}
-typedef llvm::DenseMap<Stmt*,Stmt*> ParentMapTy;
-/// PopulateParentMap - Recurse the AST starting at 'Parent' and add the
-/// mappings between child and parent to ParentMap.
-static void PopulateParentMap(Stmt* Parent, ParentMapTy& M) {
- for (Stmt::child_iterator I=Parent->child_begin(),
- E=Parent->child_end(); I!=E; ++I) {
-
- assert (M.find(*I) == M.end());
- M[*I] = Parent;
- PopulateParentMap(*I, M);
- }
-}
-
/// GenerateNode - Utility method to generate nodes, hook up successors,
/// and add nodes to the worklist.
void GRCoreEngineImpl::GenerateNode(const ProgramPoint& Loc, void* State,
Modified: cfe/trunk/lib/Analysis/GRExprEngine.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/GRExprEngine.cpp?rev=52555&r1=52554&r2=52555&view=diff
==============================================================================
--- cfe/trunk/lib/Analysis/GRExprEngine.cpp (original)
+++ cfe/trunk/lib/Analysis/GRExprEngine.cpp Fri Jun 20 16:45:25 2008
@@ -13,6 +13,7 @@
//
//===----------------------------------------------------------------------===//
+#include "clang/AST/ParentMap.h"
#include "clang/Analysis/PathSensitive/GRExprEngine.h"
#include "clang/Analysis/PathSensitive/BugReporter.h"
#include "clang/Basic/SourceManager.h"
@@ -41,6 +42,7 @@
GRExprEngine::GRExprEngine(CFG& cfg, Decl& CD, ASTContext& Ctx)
: CoreEngine(cfg, CD, Ctx, *this),
G(CoreEngine.getGraph()),
+ Parents(0),
Liveness(G.getCFG()),
Builder(NULL),
StateMgr(G.getContext(), G.getAllocator()),
@@ -56,7 +58,7 @@
Liveness.runOnAllBlocks(G.getCFG(), NULL, true);
}
-GRExprEngine::~GRExprEngine() {
+GRExprEngine::~GRExprEngine() {
for (BugTypeSet::iterator I = BugTypes.begin(), E = BugTypes.end(); I!=E; ++I)
delete *I;
@@ -69,6 +71,17 @@
delete *I;
delete [] NSExceptionInstanceRaiseSelectors;
+
+ delete Parents;
+}
+
+ParentMap& GRExprEngine::getParentMap() {
+ if (!Parents) {
+ Stmt* Body = getGraph().getCodeDecl().getCodeBody();
+ Parents = new ParentMap(Body);
+ }
+
+ return *Parents;
}
//===----------------------------------------------------------------------===//
Modified: cfe/trunk/test/Analysis/dead-stores.c
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/dead-stores.c?rev=52555&r1=52554&r2=52555&view=diff
==============================================================================
--- cfe/trunk/test/Analysis/dead-stores.c (original)
+++ cfe/trunk/test/Analysis/dead-stores.c Fri Jun 20 16:45:25 2008
@@ -3,12 +3,12 @@
void f1() {
int k, y;
int abc=1;
- long idx=abc+3*5; // expected-warning {{never used}}
+ long idx=abc+3*5; // expected-warning {{never read}}
}
void f2(void *b) {
char *c = (char*)b; // no-warning
- char *d = b+1; // expected-warning {{never used}}
+ char *d = b+1; // expected-warning {{never read}}
printf("%s", c);
}
@@ -27,19 +27,32 @@
if (k)
f1();
- k = 2; // expected-warning {{never used}}
+ k = 2; // expected-warning {{never read}}
}
void f5() {
int x = 4; // no-warning
- int *p = &x; // expected-warning{{never used}}
+ int *p = &x; // expected-warning{{never read}}
}
int f6() {
int x = 4;
- ++x; // expected-warning{{never used}}
+ ++x; // expected-warning{{never read}}
return 1;
}
+
+int f7(int *p) {
+ // This is allowed for defensive programming.
+ p = 0; // no-warning
+ return 1;
+}
+
+int f8(int *p) {
+ if (p = baz()) // expected-warning{{Although the value}}
+ return 1;
+ return 0;
+}
+
More information about the cfe-commits
mailing list