Hi,
On Mon, 2014-06-02 at 15:54 -0700, Jordan Rose wrote:
> Good idea! Rather than add another state, though, it might be better to
> use a custom tag for the initialization transition. That way we don't
> need to have "S->isInitialized() || S->isUnlocked()" anywhere. What do
> you think?
Do you mean to add another variable that is true if the unlocked mutex
just been initialized? I tried that but that made the code look a bit
kludgy. The ugly part simply moved from one place to another.
Initialized is only a special case of unlocked, so if isUnlocked()
returns true for both cases the rest of the code turned out rather
clean.
I've attached a new version of the patch with that approach and a note
that the order we check the state in VisitNode now is important.
> I don't think we have one yet, but feel free to augment the existing
> StackHintGeneratorForSymbol to support both. I imagine most of the
> logic is the same.
Ok. I'll see if I have some time later on for doing that.
Daniel
Index: PthreadLockChecker.cpp
===================================================================
--- PthreadLockChecker.cpp (revision 210096)
+++ PthreadLockChecker.cpp (working copy)
@@ -26,7 +26,7 @@ using namespace ento;
namespace {
struct LockState {
- enum Kind { Destroyed, Locked, Unlocked } K;
+ enum Kind { Destroyed, Locked, Unlocked, Initialized } K;
private:
LockState(Kind K) : K(K) {}
@@ -35,14 +35,16 @@ struct LockState {
static LockState getLocked(void) { return LockState(Locked); }
static LockState getUnlocked(void) { return LockState(Unlocked); }
static LockState getDestroyed(void) { return LockState(Destroyed); }
+ static LockState getInitialized(void) { return LockState(Initialized); }
bool operator==(const LockState &X) const {
return K == X.K;
}
bool isLocked() const { return K == Locked; }
- bool isUnlocked() const { return K == Unlocked; }
+ bool isUnlocked() const { return K == Unlocked || K == Initialized; }
bool isDestroyed() const { return K == Destroyed; }
+ bool isInitialized() const { return K == Initialized; }
void Profile(llvm::FoldingSetNodeID &ID) const {
ID.AddInteger(K);
@@ -69,7 +71,52 @@ class PthreadLockChecker : public Checker< check::
void ReleaseLock(CheckerContext &C, const CallExpr *CE, SVal lock) const;
void DestroyLock(CheckerContext &C, const CallExpr *CE, SVal Lock) const;
void InitLock(CheckerContext &C, const CallExpr *CE, SVal Lock) const;
- void reportUseDestroyedBug(CheckerContext &C, const CallExpr *CE) const;
+ void reportUseDestroyedBug(CheckerContext &C, const CallExpr *CE,
+ const MemRegion *LockR) const;
+private:
+ class PthreadBugVisitor : public BugReporterVisitorImpl<PthreadBugVisitor> {
+ protected:
+ const MemRegion *LockR;
+
+ public:
+ PthreadBugVisitor(const MemRegion *MR) : LockR(MR) {}
+ virtual ~PthreadBugVisitor() {}
+
+ void Profile(llvm::FoldingSetNodeID &ID) const override {
+ static int X = 0;
+ ID.AddPointer(&X);
+ ID.AddPointer(LockR);
+ }
+
+ inline bool isInitialized(const LockState *S, const LockState *SPrev,
+ const Stmt *Stmt) {
+ return (Stmt && isa<CallExpr>(Stmt) && (S && S->isInitialized()) &&
+ (!SPrev || !SPrev->isInitialized()));
+ }
+
+ inline bool isLocked(const LockState *S, const LockState *SPrev,
+ const Stmt *Stmt) {
+ return (Stmt && isa<CallExpr>(Stmt) && (S && S->isLocked()) &&
+ (!SPrev || !SPrev->isLocked()));
+ }
+
+ inline bool isUnlocked(const LockState *S, const LockState *SPrev,
+ const Stmt *Stmt) {
+ return (Stmt && isa<CallExpr>(Stmt) && (S && S->isUnlocked()) &&
+ (!SPrev || !SPrev->isUnlocked()));
+ }
+
+ inline bool isDestroyed(const LockState *S, const LockState *SPrev,
+ const Stmt *Stmt) {
+ return (Stmt && isa<CallExpr>(Stmt) && (S && S->isDestroyed()) &&
+ (!SPrev || !SPrev->isDestroyed()));
+ }
+
+ PathDiagnosticPiece *VisitNode(const ExplodedNode *N,
+ const ExplodedNode *PrevN,
+ BugReporterContext &BRC,
+ BugReport &BR) override;
+ };
};
} // end anonymous namespace
@@ -149,10 +196,12 @@ void PthreadLockChecker::AcquireLock(CheckerContex
"This lock has already been acquired",
N);
report->addRange(CE->getArg(0)->getSourceRange());
+ report->markInteresting(lockR);
+ report->addVisitor(new PthreadBugVisitor(lockR));
C.emitReport(report);
return;
} else if (LState->isDestroyed()) {
- reportUseDestroyedBug(C, CE);
+ reportUseDestroyedBug(C, CE, lockR);
return;
}
}
@@ -212,10 +261,12 @@ void PthreadLockChecker::ReleaseLock(CheckerContex
"This lock has already been unlocked",
N);
Report->addRange(CE->getArg(0)->getSourceRange());
+ Report->markInteresting(lockR);
+ Report->addVisitor(new PthreadBugVisitor(lockR));
C.emitReport(Report);
return;
} else if (LState->isDestroyed()) {
- reportUseDestroyedBug(C, CE);
+ reportUseDestroyedBug(C, CE, lockR);
return;
}
}
@@ -238,6 +289,8 @@ void PthreadLockChecker::ReleaseLock(CheckerContex
"reversal",
N);
report->addRange(CE->getArg(0)->getSourceRange());
+ report->markInteresting(lockR);
+ report->addVisitor(new PthreadBugVisitor(lockR));
C.emitReport(report);
return;
}
@@ -281,6 +334,8 @@ void PthreadLockChecker::DestroyLock(CheckerContex
return;
BugReport *Report = new BugReport(*BT_destroylock, Message, N);
Report->addRange(CE->getArg(0)->getSourceRange());
+ Report->markInteresting(LockR);
+ Report->addVisitor(new PthreadBugVisitor(LockR));
C.emitReport(Report);
}
@@ -295,7 +350,7 @@ void PthreadLockChecker::InitLock(CheckerContext &
const struct LockState *LState = State->get<LockMap>(LockR);
if (!LState || LState->isDestroyed()) {
- State = State->set<LockMap>(LockR, LockState::getUnlocked());
+ State = State->set<LockMap>(LockR, LockState::getInitialized());
C.addTransition(State);
return;
}
@@ -316,11 +371,14 @@ void PthreadLockChecker::InitLock(CheckerContext &
return;
BugReport *Report = new BugReport(*BT_initlock, Message, N);
Report->addRange(CE->getArg(0)->getSourceRange());
+ Report->markInteresting(LockR);
+ Report->addVisitor(new PthreadBugVisitor(LockR));
C.emitReport(Report);
}
void PthreadLockChecker::reportUseDestroyedBug(CheckerContext &C,
- const CallExpr *CE) const {
+ const CallExpr *CE,
+ const MemRegion *LockR) const {
if (!BT_destroylock)
BT_destroylock.reset(new BugType(this, "Use destroyed lock",
"Lock checker"));
@@ -331,9 +389,61 @@ void PthreadLockChecker::reportUseDestroyedBug(Che
"This lock has already been destroyed",
N);
Report->addRange(CE->getArg(0)->getSourceRange());
+ Report->markInteresting(LockR);
+ Report->addVisitor(new PthreadBugVisitor(LockR));
C.emitReport(Report);
}
void ento::registerPthreadLockChecker(CheckerManager &mgr) {
mgr.registerChecker<PthreadLockChecker>();
}
+
+PathDiagnosticPiece *
+PthreadLockChecker::PthreadBugVisitor::VisitNode(const ExplodedNode *N,
+ const ExplodedNode *PrevN,
+ BugReporterContext &BRC,
+ BugReport &BR) {
+ ProgramStateRef State = N->getState();
+ ProgramStateRef StatePrev = PrevN->getState();
+
+ const LockState *LS = State->get<LockMap>(LockR);
+ const LockState *LSPrev = StatePrev->get<LockMap>(LockR);
+ if (!LS)
+ return nullptr;
+
+ const Stmt *S = nullptr;
+ const char *Msg = nullptr;
+
+ // Retrieve the associated statement.
+ ProgramPoint ProgLoc = N->getLocation();
+ if (Optional<StmtPoint> SP = ProgLoc.getAs<StmtPoint>()) {
+ S = SP->getStmt();
+ } else if (Optional<CallExitEnd> Exit = ProgLoc.getAs<CallExitEnd>()) {
+ S = Exit->getCalleeContext()->getCallSite();
+ } else if (Optional<BlockEdge> Edge = ProgLoc.getAs<BlockEdge>()) {
+ // If an assumption was made on a branch, it should be caught
+ // here by looking at the state transition.
+ S = Edge->getSrc()->getTerminator();
+ }
+
+ if (!S)
+ return nullptr;
+
+ // We must check isInitialized before isUnlocked
+ if (isInitialized(LS, LSPrev, S)) {
+ Msg = "Lock is initialized";
+ } else if (isLocked(LS, LSPrev, S)) {
+ Msg = "Lock is locked";
+ } else if (isUnlocked(LS, LSPrev, S)) {
+ Msg = "Lock is unlocked";
+ } else if (isDestroyed(LS, LSPrev, S)) {
+ Msg = "Lock is destroyed";
+ }
+
+ if (!Msg)
+ return nullptr;
+
+ PathDiagnosticLocation Pos(S, BRC.getSourceManager(),
+ N->getLocationContext());
+ return new PathDiagnosticEventPiece(Pos, Msg, false);
+}
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
