Hm. Thanks to the fact that one callback may end up indirectly invoking
another, this is not so simple. On the one hand, it does let us track the
response for any callback. On the other, ResetCallbackFlag is a kludge.
Holding off on committing for now.
On Wed, 4 Aug 2010 17:10:17 -0700, Ted Kremenek <[email protected]>
wrote:
> That's not a bad idea at all. In fact I really like it. The value can
> still get lazily set, but just stored with the Checker object.
>
> On Aug 4, 2010, at 10:51 AM, Jordy Rose wrote:
>
>> This is not a general solution, though. Alternately, we could just
stick
>> the flag in Checker rather than CheckerContext.
Index: include/clang/Checker/PathSensitive/Checker.h
===================================================================
--- include/clang/Checker/PathSensitive/Checker.h (revision 110344)
+++ include/clang/Checker/PathSensitive/Checker.h (working copy)
@@ -37,20 +37,16 @@
const Stmt *statement;
const unsigned size;
public:
- bool *respondsToCallback;
-public:
CheckerContext(ExplodedNodeSet &dst, GRStmtNodeBuilder &builder,
GRExprEngine &eng, ExplodedNode *pred,
const void *tag, ProgramPoint::Kind K,
- bool *respondsToCB = 0,
const Stmt *stmt = 0, const GRState *st = 0)
: Dst(dst), B(builder), Eng(eng), Pred(pred),
OldSink(B.BuildSinks),
OldTag(B.Tag, tag),
OldPointKind(B.PointKind, K),
OldHasGen(B.HasGeneratedNode),
- ST(st), statement(stmt), size(Dst.size()),
- respondsToCallback(respondsToCB) {}
+ ST(st), statement(stmt), size(Dst.size()) {}
~CheckerContext();
@@ -184,6 +180,8 @@
};
class Checker {
+protected:
+ bool respondsToCallback;
private:
friend class GRExprEngine;
@@ -192,11 +190,10 @@
GRStmtNodeBuilder &Builder,
GRExprEngine &Eng,
const Stmt *S,
- ExplodedNode *Pred, void *tag, bool isPrevisit,
- bool& respondsToCallback) {
+ ExplodedNode *Pred, void *tag, bool isPrevisit) {
CheckerContext C(Dst, Builder, Eng, Pred, tag,
isPrevisit ? ProgramPoint::PreStmtKind :
- ProgramPoint::PostStmtKind, &respondsToCallback, S);
+ ProgramPoint::PostStmtKind, S);
if (isPrevisit)
_PreVisit(C, S);
else
@@ -207,7 +204,7 @@
GRExprEngine &Eng, const ObjCMessageExpr *ME,
ExplodedNode *Pred, const GRState *state, void *tag) {
CheckerContext C(Dst, Builder, Eng, Pred, tag, ProgramPoint::PostStmtKind,
- 0, ME, state);
+ ME, state);
return EvalNilReceiver(C, ME);
}
@@ -215,7 +212,7 @@
GRExprEngine &Eng, const CallExpr *CE,
ExplodedNode *Pred, void *tag) {
CheckerContext C(Dst, Builder, Eng, Pred, tag, ProgramPoint::PostStmtKind,
- 0, CE);
+ CE);
return EvalCallExpr(C, CE);
}
@@ -228,7 +225,7 @@
bool isPrevisit) {
CheckerContext C(Dst, Builder, Eng, Pred, tag,
isPrevisit ? ProgramPoint::PreStmtKind :
- ProgramPoint::PostStmtKind, 0, StoreE);
+ ProgramPoint::PostStmtKind, StoreE);
assert(isPrevisit && "Only previsit supported for now.");
PreVisitBind(C, AssignE, StoreE, location, val);
}
@@ -243,7 +240,7 @@
void *tag, bool isLoad) {
CheckerContext C(Dst, Builder, Eng, Pred, tag,
isLoad ? ProgramPoint::PreLoadKind :
- ProgramPoint::PreStoreKind, 0, S, state);
+ ProgramPoint::PreStoreKind, S, state);
VisitLocation(C, S, location);
}
@@ -251,14 +248,44 @@
GRExprEngine &Eng, const Stmt *S, ExplodedNode *Pred,
SymbolReaper &SymReaper, void *tag) {
CheckerContext C(Dst, Builder, Eng, Pred, tag,
- ProgramPoint::PostPurgeDeadSymbolsKind, 0, S);
+ ProgramPoint::PostPurgeDeadSymbolsKind, S);
EvalDeadSymbols(C, SymReaper);
}
public:
+ Checker() : respondsToCallback(true) {}
virtual ~Checker();
- virtual void _PreVisit(CheckerContext &C, const Stmt *S) {}
- virtual void _PostVisit(CheckerContext &C, const Stmt *S) {}
+
+ /// ResetCallbackFlag - Called by clients intending to check whether this
+ /// checker actually responds to a certain callback. This is necessary
+ /// because some callbacks may cause others to be invoked. Clients should
+ /// follow this pattern:
+ /// SaveAndRestore<bool> OldFlag = checker->ResetCallbackFlag();
+ /// checker->SomeCallback();
+ /// if (checker->RespondedToCallback())
+ /// responses.insert(checker)
+ SaveAndRestore<bool> ResetCallbackFlag() {
+ // This looks like it's incorrect, since a non-optimizing compiler will
+ // use a temporary and a copy here, then destroy the copy and reset the
+ // value of respondsToCallback. However, the only case where
+ // respondsToCallback is 'false' is just after calling an ignored
+ // callback. By the end of the block in which a client calls that callback,
+ // the value of respondsToCallback will have been reset to 'true'.
+ return SaveAndRestore<bool>(respondsToCallback, true);
+ }
+
+ /// RespondedToCallback - Returns whether or not the checker responded to its
+ /// last callback. See SaveCallbackFlag.
+ bool RespondedToCallback() { return respondsToCallback; }
+
+ virtual void _PreVisit(CheckerContext &C, const Stmt *S) {
+ respondsToCallback = false;
+ }
+
+ virtual void _PostVisit(CheckerContext &C, const Stmt *S) {
+ respondsToCallback = false;
+ }
+
virtual void VisitLocation(CheckerContext &C, const Stmt *S, SVal location) {}
virtual void PreVisitBind(CheckerContext &C, const Stmt *AssignE,
const Stmt *StoreE, SVal location, SVal val) {}
@@ -279,8 +306,8 @@
}
virtual const GRState *EvalAssume(const GRState *state, SVal Cond,
- bool Assumption, bool *respondsToCallback) {
- *respondsToCallback = false;
+ bool Assumption) {
+ respondsToCallback = false;
return state;
}
Index: include/clang/Checker/PathSensitive/CheckerVisitor.h
===================================================================
--- include/clang/Checker/PathSensitive/CheckerVisitor.h (revision 110344)
+++ include/clang/Checker/PathSensitive/CheckerVisitor.h (working copy)
@@ -80,11 +80,11 @@
}
void PreVisitStmt(CheckerContext &C, const Stmt *S) {
- *C.respondsToCallback = false;
+ respondsToCallback = false;
}
void PostVisitStmt(CheckerContext &C, const Stmt *S) {
- *C.respondsToCallback = false;
+ respondsToCallback = false;
}
void PreVisitCastExpr(CheckerContext &C, const CastExpr *E) {
Index: lib/Checker/MallocChecker.cpp
===================================================================
--- lib/Checker/MallocChecker.cpp (revision 110344)
+++ lib/Checker/MallocChecker.cpp (working copy)
@@ -75,8 +75,7 @@
void EvalDeadSymbols(CheckerContext &C, SymbolReaper &SymReaper);
void EvalEndPath(GREndPathNodeBuilder &B, void *tag, GRExprEngine &Eng);
void PreVisitReturnStmt(CheckerContext &C, const ReturnStmt *S);
- const GRState *EvalAssume(const GRState *state, SVal Cond, bool Assumption,
- bool *respondsToCallback);
+ const GRState *EvalAssume(const GRState *state, SVal Cond, bool Assumption);
void VisitLocation(CheckerContext &C, const Stmt *S, SVal l);
virtual void PreVisitBind(CheckerContext &C, const Stmt *AssignE,
const Stmt *StoreE, SVal location,
@@ -630,8 +629,7 @@
}
const GRState *MallocChecker::EvalAssume(const GRState *state, SVal Cond,
- bool Assumption,
- bool * /* respondsToCallback */) {
+ bool Assumption) {
// If a symblic region is assumed to NULL, set its state to AllocateFailed.
// FIXME: should also check symbols assumed to non-null.
Index: lib/Checker/GRExprEngine.cpp
===================================================================
--- lib/Checker/GRExprEngine.cpp (revision 110344)
+++ lib/Checker/GRExprEngine.cpp (working copy)
@@ -215,13 +215,13 @@
}
void *tag = I->first;
Checker *checker = I->second;
- bool respondsToCallback = true;
+ SaveAndRestore<bool> OldFlag = checker->ResetCallbackFlag();
for (ExplodedNodeSet::iterator NI = PrevSet->begin(), NE = PrevSet->end();
NI != NE; ++NI) {
checker->GR_Visit(*CurrSet, *Builder, *this, S, *NI, tag,
- Kind == PreVisitStmtCallback, respondsToCallback);
+ Kind == PreVisitStmtCallback);
}
@@ -229,9 +229,9 @@
if (NewCO.get()) {
++checkersEvaluated;
- if (respondsToCallback)
+ if (checker->RespondedToCallback())
NewCO->push_back(*I);
- }
+ }
}
// If we built NewCO, check if we called all the checkers. This is important
@@ -534,12 +534,11 @@
return NULL;
Checker *C = I->second;
- bool respondsToCallback = true;
+ SaveAndRestore<bool> OldFlag = C->ResetCallbackFlag();
+ state = C->EvalAssume(state, cond, assumption);
- state = C->EvalAssume(state, cond, assumption, &respondsToCallback);
-
// Check if we're building the cache of checkers that care about Assumes.
- if (NewCO.get() && respondsToCallback)
+ if (NewCO.get() && C->RespondedToCallback())
NewCO->push_back(*I);
}
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
