[cfe-commits] [PATCH] Thread-safety analysis: add support for scoped_lockable attribute
Richard Smith
richard at metafoo.co.uk
Fri Oct 21 16:18:31 PDT 2011
Hi Delesley,
On Fri, October 21, 2011 23:28, Delesley Hutchins wrote:
> This patch adds support for the scoped_lockable attribute, which is
> used to create wrapper objects that acquire a mutex on construction, and
> automatically release the mutex when they leave scope. The patch allows one
> mutex to map to another, and removes the second when the first is released.
>
> http://codereview.appspot.com/5314051/
Comments inline:
diff --git a/lib/Analysis/ThreadSafety.cpp b/lib/Analysis/ThreadSafety.cpp
index 7b9a693..059a131 100644
--- a/lib/Analysis/ThreadSafety.cpp
+++ b/lib/Analysis/ThreadSafety.cpp
@@ -42,6 +42,21 @@ ThreadSafetyHandler::~ThreadSafetyHandler() {}
namespace {
+/// \brief If the declaration has an attribute of the given kind, then
+/// return a pointer to the attribute, else return 0.
+Attr* getDeclAttribute(const NamedDecl* D, attr::Kind AK) {
+ if (!D || !D->hasAttrs())
+ return false;
+
+ const AttrVec &Attrs = D->getAttrs();
+ for(unsigned i = 0; i < Attrs.size(); ++i) {
+ Attr *Attr = Attrs[i];
+ if (Attr->getKind() == AK)
+ return Attr;
+ }
+ return 0;
+}
You can use Decl::getAttr<AttrType>() for this.
+
/// \brief Implements a set of CFGBlocks using a BitVector.
///
/// This class contains a minimal interface, primarily dictated by the SetType
@@ -248,6 +263,10 @@ class MutexID {
}
public:
+ explicit MutexID(int dummy) {
Can you use something more explicit than an int? Elsewhere in clang, we use
things like "struct EmptyShell { };" for such constructors.
+ DeclSeq.clear();
+ }
+
/// \param MutexExp The original mutex expression within an attribute
/// \param DeclExp An expression involving the Decl on which the attribute
/// occurs.
@@ -325,9 +344,13 @@ struct LockData {
///
/// FIXME: add support for re-entrant locking and lock up/downgrading
LockKind LKind;
+ MutexID TrueMutex; // for ScopedLockable objects
UnderlyingMutex or InnerMutex might be clearer. Unless this is standard
terminology for a scoped mutex?
LockData(SourceLocation AcquireLoc, LockKind LKind)
- : AcquireLoc(AcquireLoc), LKind(LKind) {}
+ : AcquireLoc(AcquireLoc), LKind(LKind), TrueMutex(0) {}
+
+ LockData(SourceLocation AcquireLoc, LockKind LKind, const MutexID &Mu)
+ : AcquireLoc(AcquireLoc), LKind(LKind), TrueMutex(Mu) {}
bool operator==(const LockData &other) const {
return AcquireLoc == other.AcquireLoc && LKind == other.LKind;
@@ -361,11 +384,12 @@ class BuildLockset : public StmtVisitor<BuildLockset> {
Lockset::Factory &LocksetFactory;
// Helper functions
- void removeLock(SourceLocation UnlockLoc, MutexID &Mutex);
- void addLock(SourceLocation LockLoc, MutexID &Mutex, LockKind LK);
+ void addLock(MutexID &Mutex, const LockData &LDat);
+ void removeLock(MutexID &Mutex, SourceLocation UnlockLoc);
template <class AttrType>
- void addLocksToSet(LockKind LK, AttrType *Attr, Expr *Exp, NamedDecl *D);
+ void addLocksToSet(LockKind LK, AttrType *Attr,
+ Expr *Exp, NamedDecl *D, VarDecl *VD = 0);
void removeLocksFromSet(UnlockFunctionAttr *Attr,
Expr *Exp, NamedDecl* FunDecl);
@@ -374,7 +398,7 @@ class BuildLockset : public StmtVisitor<BuildLockset> {
Expr *MutexExp, ProtectedOperationKind POK);
void checkAccess(Expr *Exp, AccessKind AK);
void checkDereference(Expr *Exp, AccessKind AK);
- void handleCall(Expr *Exp, NamedDecl *D);
+ void handleCall(Expr *Exp, NamedDecl *D, VarDecl *VD = 0);
/// \brief Returns true if the lockset contains a lock, regardless of whether
/// the lock is held exclusively or shared.
@@ -418,51 +442,61 @@ public:
void VisitCastExpr(CastExpr *CE);
void VisitCXXMemberCallExpr(CXXMemberCallExpr *Exp);
void VisitCXXConstructExpr(CXXConstructExpr *Exp);
+ void VisitDeclStmt(DeclStmt *S);
};
/// \brief Add a new lock to the lockset, warning if the lock is already there.
-/// \param LockLoc The source location of the acquire
-/// \param LockExp The lock expression corresponding to the lock to be added
-void BuildLockset::addLock(SourceLocation LockLoc, MutexID &Mutex,
- LockKind LK) {
- // FIXME: deal with acquired before/after annotations. We can write a first
- // pass that does the transitive lookup lazily, and refine afterwards.
- LockData NewLock(LockLoc, LK);
-
+/// \param Mutex -- the Mutex expression for the lock
+/// \param LDat -- the LockData for the lock
+void BuildLockset::addLock(MutexID &Mutex, const LockData& LDat) {
+ // FIXME: deal with acquired before/after annotations.
// FIXME: Don't always warn when we have support for reentrant locks.
if (locksetContains(Mutex))
- Handler.handleDoubleLock(Mutex.getName(), LockLoc);
+ Handler.handleDoubleLock(Mutex.getName(), LDat.AcquireLoc);
else
- LSet = LocksetFactory.add(LSet, Mutex, NewLock);
+ LSet = LocksetFactory.add(LSet, Mutex, LDat);
}
/// \brief Remove a lock from the lockset, warning if the lock is not there.
/// \param LockExp The lock expression corresponding to the lock to be removed
/// \param UnlockLoc The source location of the unlock (only used in error msg)
-void BuildLockset::removeLock(SourceLocation UnlockLoc, MutexID &Mutex) {
- Lockset NewLSet = LocksetFactory.remove(LSet, Mutex);
- if(NewLSet == LSet)
+void BuildLockset::removeLock(MutexID &Mutex, SourceLocation UnlockLoc) {
+ const LockData *LDat = LSet.lookup(Mutex);
+ if(!LDat)
Handler.handleUnmatchedUnlock(Mutex.getName(), UnlockLoc);
- else
- LSet = NewLSet;
+ else {
+ // For scoped-lockable vars, remove the mutex associated with this var.
+ if (LDat->TrueMutex.isValid())
+ LSet = LocksetFactory.remove(LSet, LDat->TrueMutex);
If the inner mutex has been explicitly unlocked since the scoped lock was
acquired, it looks like that won't be caught. Would a recursive call to
removeLock work here?
+ LSet = LocksetFactory.remove(LSet, Mutex);
+ }
}
/// \brief This function, parameterized by an attribute type, is used to add a
/// set of locks specified as attribute arguments to the lockset.
template <typename AttrType>
void BuildLockset::addLocksToSet(LockKind LK, AttrType *Attr,
- Expr *Exp, NamedDecl* FunDecl) {
+ Expr *Exp, NamedDecl* FunDecl, VarDecl *VD) {
typedef typename AttrType::args_iterator iterator_type;
SourceLocation ExpLocation = Exp->getExprLoc();
+ // Figure out if we're calling the constructor of scoped lockable class
+ bool isScopedVar = false;
+ if (VD) {
+ if (CXXConstructorDecl *CD = dyn_cast<CXXConstructorDecl>(FunDecl)) {
+ if (getDeclAttribute(CD->getParent(), attr::ScopedLockable))
+ isScopedVar = true;
+ }
+ }
+
if (Attr->args_size() == 0) {
// The mutex held is the "this" object.
MutexID Mutex(0, Exp, FunDecl);
if (!Mutex.isValid())
MutexID::warnInvalidLock(Handler, 0, Exp, FunDecl);
else
- addLock(ExpLocation, Mutex, LK);
+ addLock(Mutex, LockData(ExpLocation, LK));
return;
}
@@ -470,8 +504,15 @@ void BuildLockset::addLocksToSet(LockKind LK, AttrType
*Attr,
MutexID Mutex(*I, Exp, FunDecl);
if (!Mutex.isValid())
MutexID::warnInvalidLock(Handler, *I, Exp, FunDecl);
- else
- addLock(ExpLocation, Mutex, LK);
+ else {
+ addLock(Mutex, LockData(ExpLocation, LK));
+ if (isScopedVar) {
+ // For scoped lockable vars, map this var to its true mutex.
+ DeclRefExpr DRE(VD, VD->getType(), VK_LValue, VD->getLocation());
+ MutexID SMutex(&DRE, 0, 0);
+ addLock(SMutex, LockData(VD->getLocation(), LK, Mutex));
It would be great if addLock and removeLock were symmetric (either both
locking/unlocking the inner mutex or neither doing so).
+ }
+ }
}
}
@@ -488,7 +529,7 @@ void BuildLockset::removeLocksFromSet(UnlockFunctionAttr
*Attr,
if (!Mu.isValid())
MutexID::warnInvalidLock(Handler, 0, Exp, FunDecl);
else
- removeLock(ExpLocation, Mu);
+ removeLock(Mu, ExpLocation);
return;
}
@@ -498,7 +539,7 @@ void BuildLockset::removeLocksFromSet(UnlockFunctionAttr
*Attr,
if (!Mutex.isValid())
MutexID::warnInvalidLock(Handler, *I, Exp, FunDecl);
else
- removeLock(ExpLocation, Mutex);
+ removeLock(Mutex, ExpLocation);
}
}
@@ -584,7 +625,7 @@ void BuildLockset::checkAccess(Expr *Exp, AccessKind AK) {
///
/// FIXME: Do not flag an error for member variables accessed in constructors/
/// destructors
-void BuildLockset::handleCall(Expr *Exp, NamedDecl *D) {
+void BuildLockset::handleCall(Expr *Exp, NamedDecl *D, VarDecl *VD) {
AttrVec &ArgAttrs = D->getAttrs();
for(unsigned i = 0; i < ArgAttrs.size(); ++i) {
Attr *Attr = ArgAttrs[i];
@@ -593,7 +634,7 @@ void BuildLockset::handleCall(Expr *Exp, NamedDecl *D) {
// to our lockset with kind exclusive.
case attr::ExclusiveLockFunction: {
ExclusiveLockFunctionAttr *A = cast<ExclusiveLockFunctionAttr>(Attr);
- addLocksToSet(LK_Exclusive, A, Exp, D);
+ addLocksToSet(LK_Exclusive, A, Exp, D, VD);
break;
}
@@ -601,7 +642,7 @@ void BuildLockset::handleCall(Expr *Exp, NamedDecl *D) {
// to our lockset with kind shared.
case attr::SharedLockFunction: {
SharedLockFunctionAttr *A = cast<SharedLockFunctionAttr>(Attr);
- addLocksToSet(LK_Shared, A, Exp, D);
+ addLocksToSet(LK_Shared, A, Exp, D, VD);
break;
}
@@ -703,10 +744,23 @@ void BuildLockset::VisitCXXMemberCallExpr(CXXMemberCallExpr
*Exp) {
}
void BuildLockset::VisitCXXConstructExpr(CXXConstructExpr *Exp) {
- NamedDecl *D = cast<NamedDecl>(Exp->getConstructor());
- if(!D || !D->hasAttrs())
- return;
- handleCall(Exp, D);
+ // FIXME -- only handles constructors in DeclStmt below.
+}
+
+void BuildLockset::VisitDeclStmt(DeclStmt *S) {
+ DeclGroupRef DGrp = S->getDeclGroup();
+ for (DeclGroupRef::iterator I = DGrp.begin(), E = DGrp.end(); I != E; ++I) {
+ Decl *D = *I;
+ if (VarDecl *VD = dyn_cast_or_null<VarDecl>(D)) {
+ Expr *E = VD->getInit();
+ if (CXXConstructExpr *CE = dyn_cast_or_null<CXXConstructExpr>(E)) {
+ NamedDecl *CtorD = dyn_cast_or_null<NamedDecl>(CE->getConstructor());
+ if (!CtorD || !CtorD->hasAttrs())
+ return;
+ handleCall(CE, CtorD, VD);
+ }
+ }
+ }
}
diff --git a/lib/Sema/AnalysisBasedWarnings.cpp
b/lib/Sema/AnalysisBasedWarnings.cpp
index bd34dec..2eefdeb 100644
--- a/lib/Sema/AnalysisBasedWarnings.cpp
+++ b/lib/Sema/AnalysisBasedWarnings.cpp
@@ -845,7 +845,7 @@
AnalysisBasedWarnings::IssueWarnings(sema::AnalysisBasedWarnings::Policy P,
// prototyping, but we need a way for analyses to say what expressions they
// expect to always be CFGElements and then fill in the BuildOptions
// appropriately. This is essentially a layering violation.
- if (P.enableCheckUnreachable) {
+ if (P.enableCheckUnreachable || P.enableThreadSafetyAnalysis) {
// Unreachable code analysis requires a linearized CFG.
Please update this comment, too.
AC.getCFGBuildOptions().setAllAlwaysAdd();
}
diff --git a/test/SemaCXX/warn-thread-safety-analysis.cpp
b/test/SemaCXX/warn-thread-safety-analysis.cpp
index 7adead7..7026a19 100644
--- a/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ b/test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -36,6 +36,18 @@ class __attribute__((lockable)) Mutex {
void LockWhen(const int &cond) __attribute__((exclusive_lock_function));
};
+class __attribute__((scoped_lockable)) MutexLock {
+ public:
+ MutexLock(Mutex *mu) __attribute__((exclusive_lock_function(mu)));
+ ~MutexLock() __attribute__((unlock_function));
+};
+
+class __attribute__((scoped_lockable)) ReaderMutexLock {
+ public:
+ ReaderMutexLock(Mutex *mu) __attribute__((exclusive_lock_function(mu)));
+ ~ReaderMutexLock() __attribute__((unlock_function));
+};
+
Mutex sls_mu;
@@ -1510,3 +1522,46 @@ void foo() {
} // end namespace invalid_lock_expression_test
+namespace test_scoped_lockable {
+
+struct TestScopedLockable {
+ Mutex mu1;
+ Mutex mu2;
+ int a __attribute__((guarded_by(mu1)));
+ int b __attribute__((guarded_by(mu2)));
+
+ bool getBool();
+
+ void foo1() {
+ MutexLock mulock(&mu1);
+ a = 5;
+ }
+
+ void foo2() {
+ ReaderMutexLock mulock1(&mu1);
+ if (getBool()) {
+ MutexLock mulock2a(&mu2);
+ b = a + 1;
+ }
+ else {
+ MutexLock mulock2b(&mu2);
+ b = a + 2;
+ }
+ }
+
+ void foo3() {
+ MutexLock mulock_a(&mu1);
+ MutexLock mulock_b(&mu1); // \
+ // expected-warning {{locking 'mu1' that is already locked}}
+ }
+
+ void foo4() {
+ MutexLock mulock1(&mu1), mulock2(&mu2);
+ a = b+1;
+ b = a+1;
+ }
+};
+
+} // end namespace test_scoped_lockable
+
+
More information about the cfe-commits
mailing list