This patch updates the thread safety analysis to properly substitute
call arguments for function parameters in mutex expressions. For
example:
void foo(MyData* d) __attribute__((exclusive_locks_required(d->mu))) { ... }
...
foo(mydat); // lock required should be mydat->mu.
The patch is broken into two parts. The first part is a refactoring
patch, which makes it easier to write the second part, which
implements the actual functionality.
http://codereview.appspot.com/5001048/
-DeLesley
--
DeLesley Hutchins | Software Engineer | [email protected] | 505-206-0315
From 7fc2567457584127ef4b91f50d03e1849a4259a5 Mon Sep 17 00:00:00 2001
From: DeLesley Hutchins <[email protected]>
Date: Wed, 14 Sep 2011 09:08:37 -0700
Subject: [PATCH] Substitute for arguments in method calls -- refactoring
---
lib/Analysis/ThreadSafety.cpp | 114 +++++++++++++++++++++++-----------------
1 files changed, 65 insertions(+), 49 deletions(-)
diff --git a/lib/Analysis/ThreadSafety.cpp b/lib/Analysis/ThreadSafety.cpp
index fe9c5de..26861ae 100644
--- a/lib/Analysis/ThreadSafety.cpp
+++ b/lib/Analysis/ThreadSafety.cpp
@@ -36,14 +36,6 @@
using namespace clang;
using namespace thread_safety;
-// Helper functions
-static Expr *getParent(Expr *Exp) {
- if (MemberExpr *ME = dyn_cast<MemberExpr>(Exp))
- return ME->getBase();
- if (CXXMemberCallExpr *CE = dyn_cast<CXXMemberCallExpr>(Exp))
- return CE->getImplicitObjectArgument();
- return 0;
-}
namespace {
/// \brief Implements a set of CFGBlocks using a BitVector.
@@ -154,32 +146,54 @@ public:
/// foo(MyL); // requires lock MyL->Mu to be held
class MutexID {
SmallVector<NamedDecl*, 2> DeclSeq;
- ThreadSafetyHandler &Handler;
/// Build a Decl sequence representing the lock from the given expression.
/// Recursive function that bottoms out when the final DeclRefExpr is reached.
- void buildMutexID(Expr *Exp, Expr *Parent) {
+ void buildMutexID(ThreadSafetyHandler &Handler, Expr *Exp, Expr *Parent) {
if (DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(Exp)) {
NamedDecl *ND = cast<NamedDecl>(DRE->getDecl()->getCanonicalDecl());
DeclSeq.push_back(ND);
} else if (MemberExpr *ME = dyn_cast<MemberExpr>(Exp)) {
NamedDecl *ND = ME->getMemberDecl();
DeclSeq.push_back(ND);
- buildMutexID(ME->getBase(), Parent);
+ buildMutexID(Handler, ME->getBase(), Parent);
} else if (isa<CXXThisExpr>(Exp)) {
if (!Parent)
return;
- buildMutexID(Parent, 0);
+ buildMutexID(Handler, Parent, 0);
} else if (CastExpr *CE = dyn_cast<CastExpr>(Exp))
- buildMutexID(CE->getSubExpr(), Parent);
+ buildMutexID(Handler, CE->getSubExpr(), Parent);
else
Handler.handleInvalidLockExp(Exp->getExprLoc());
}
+ /// \brief Construct a MutexID from an expression.
+ /// \param AttrExp The original mutex expression within an attribute
+ /// \param Exp An expression involving the Decl on which the attribute occurs.
+ /// \param Decl The function to which the lock/unlock attribute is attached.
+ void buildMutexIDFromExp(ThreadSafetyHandler &Handler, Expr* AttrExp,
+ Expr *Exp, const NamedDecl* Dec) {
+ Expr* Parent = 0;
+
+ if (MemberExpr *ME = dyn_cast<MemberExpr>(Exp)) {
+ Parent = ME->getBase();
+ }
+ else if (CXXMemberCallExpr *CE = dyn_cast<CXXMemberCallExpr>(Exp)) {
+ Parent = CE->getImplicitObjectArgument();
+ }
+
+ // If the attribute has no arguments, then assume the argument is "this".
+ if (AttrExp == 0) {
+ buildMutexID(Handler, Parent, 0);
+ return;
+ }
+ buildMutexID(Handler, AttrExp, Parent);
+ }
+
public:
- MutexID(ThreadSafetyHandler &Handler, Expr *LExpr, Expr *ParentExpr)
- : Handler(Handler) {
- buildMutexID(LExpr, ParentExpr);
+ MutexID(ThreadSafetyHandler &Handler,
+ Expr* AttrExp, Expr *Exp, const NamedDecl* D) {
+ buildMutexIDFromExp(Handler, AttrExp, Exp, D);
assert(!DeclSeq.empty());
}
@@ -264,9 +278,8 @@ class BuildLockset : public StmtVisitor<BuildLockset> {
Lockset::Factory &LocksetFactory;
// Helper functions
- void removeLock(SourceLocation UnlockLoc, Expr *LockExp, Expr *Parent);
- void addLock(SourceLocation LockLoc, Expr *LockExp, Expr *Parent,
- LockKind LK);
+ void removeLock(SourceLocation UnlockLoc, MutexID &Mutex);
+ void addLock(SourceLocation LockLoc, MutexID &Mutex, LockKind LK);
const ValueDecl *getValueDecl(Expr *Exp);
void warnIfMutexNotHeld (const NamedDecl *D, Expr *Exp, AccessKind AK,
Expr *MutexExp, ProtectedOperationKind POK);
@@ -274,7 +287,8 @@ class BuildLockset : public StmtVisitor<BuildLockset> {
void checkDereference(Expr *Exp, AccessKind AK);
template <class AttrType>
- void addLocksToSet(LockKind LK, Attr *Attr, CXXMemberCallExpr *Exp);
+ void addLocksToSet(LockKind LK, AttrType *Attr, CXXMemberCallExpr *Exp,
+ NamedDecl* D);
/// \brief Returns true if the lockset contains a lock, regardless of whether
/// the lock is held exclusively or shared.
@@ -321,10 +335,9 @@ public:
/// \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, Expr *LockExp, Expr *Parent,
+void BuildLockset::addLock(SourceLocation LockLoc, MutexID &Mutex,
LockKind LK) {
// FIXME: deal with acquired before/after annotations
- MutexID Mutex(Handler, LockExp, Parent);
LockData NewLock(LockLoc, LK);
// FIXME: Don't always warn when we have support for reentrant locks.
@@ -336,10 +349,7 @@ void BuildLockset::addLock(SourceLocation LockLoc, Expr *LockExp, Expr *Parent,
/// \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, Expr *LockExp,
- Expr *Parent) {
- MutexID Mutex(Handler, LockExp, Parent);
-
+void BuildLockset::removeLock(SourceLocation UnlockLoc, MutexID &Mutex) {
Lockset NewLSet = LocksetFactory.remove(LSet, Mutex);
if(NewLSet == LSet)
Handler.handleUnmatchedUnlock(Mutex.getName(), UnlockLoc);
@@ -359,13 +369,12 @@ const ValueDecl *BuildLockset::getValueDecl(Expr *Exp) {
}
/// \brief Warn if the LSet does not contain a lock sufficient to protect access
-/// of at least the passed in AccessType.
+/// of at least the passed in AccessKind.
void BuildLockset::warnIfMutexNotHeld(const NamedDecl *D, Expr *Exp,
AccessKind AK, Expr *MutexExp,
ProtectedOperationKind POK) {
LockKind LK = getLockKindFromAccessKind(AK);
- Expr *Parent = getParent(Exp);
- MutexID Mutex(Handler, MutexExp, Parent);
+ MutexID Mutex(Handler, MutexExp, Exp, D);
if (!locksetContainsAtLeast(Mutex, LK))
Handler.handleMutexNotHeld(D, POK, Mutex.getName(), LK, Exp->getExprLoc());
}
@@ -458,22 +467,24 @@ void BuildLockset::VisitCastExpr(CastExpr *CE) {
/// \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, Attr *Attr,
- CXXMemberCallExpr *Exp) {
+void BuildLockset::addLocksToSet(LockKind LK, AttrType *Attr,
+ CXXMemberCallExpr *Exp,
+ NamedDecl* FunDecl) {
typedef typename AttrType::args_iterator iterator_type;
+
SourceLocation ExpLocation = Exp->getExprLoc();
- Expr *Parent = Exp->getImplicitObjectArgument();
- AttrType *SpecificAttr = cast<AttrType>(Attr);
- if (SpecificAttr->args_size() == 0) {
+ if (Attr->args_size() == 0) {
// The mutex held is the "this" object.
- addLock(ExpLocation, Parent, Parent, LK);
+ MutexID Mu(Handler, 0, Exp, FunDecl);
+ addLock(ExpLocation, Mu, LK);
return;
}
- for (iterator_type I = SpecificAttr->args_begin(),
- E = SpecificAttr->args_end(); I != E; ++I)
- addLock(ExpLocation, *I, Parent, LK);
+ for (iterator_type I=Attr->args_begin(), E=Attr->args_end(); I != E; ++I) {
+ MutexID Mu(Handler, *I, Exp, FunDecl);
+ addLock(ExpLocation, Mu, LK);
+ }
}
/// \brief When visiting CXXMemberCallExprs we need to examine the attributes on
@@ -487,11 +498,9 @@ void BuildLockset::addLocksToSet(LockKind LK, Attr *Attr,
///
/// FIXME: We need to also visit CallExprs to catch/check global functions.
void BuildLockset::VisitCXXMemberCallExpr(CXXMemberCallExpr *Exp) {
- NamedDecl *D = dyn_cast_or_null<NamedDecl>(Exp->getCalleeDecl());
-
SourceLocation ExpLocation = Exp->getExprLoc();
- Expr *Parent = Exp->getImplicitObjectArgument();
+ NamedDecl *D = dyn_cast_or_null<NamedDecl>(Exp->getCalleeDecl());
if(!D || !D->hasAttrs())
return;
@@ -501,15 +510,19 @@ void BuildLockset::VisitCXXMemberCallExpr(CXXMemberCallExpr *Exp) {
switch (Attr->getKind()) {
// When we encounter an exclusive lock function, we need to add the lock
// to our lockset with kind exclusive.
- case attr::ExclusiveLockFunction:
- addLocksToSet<ExclusiveLockFunctionAttr>(LK_Exclusive, Attr, Exp);
+ case attr::ExclusiveLockFunction: {
+ ExclusiveLockFunctionAttr *A = cast<ExclusiveLockFunctionAttr>(Attr);
+ addLocksToSet(LK_Exclusive, A, Exp, D);
break;
+ }
// When we encounter a shared lock function, we need to add the lock
// to our lockset with kind shared.
- case attr::SharedLockFunction:
- addLocksToSet<SharedLockFunctionAttr>(LK_Shared, Attr, Exp);
+ case attr::SharedLockFunction: {
+ SharedLockFunctionAttr *A = cast<SharedLockFunctionAttr>(Attr);
+ addLocksToSet(LK_Shared, A, Exp, D);
break;
+ }
// When we encounter an unlock function, we need to remove unlocked
// mutexes from the lockset, and flag a warning if they are not there.
@@ -517,13 +530,16 @@ void BuildLockset::VisitCXXMemberCallExpr(CXXMemberCallExpr *Exp) {
UnlockFunctionAttr *UFAttr = cast<UnlockFunctionAttr>(Attr);
if (UFAttr->args_size() == 0) { // The lock held is the "this" object.
- removeLock(ExpLocation, Parent, Parent);
+ MutexID Mu(Handler, 0, Exp, D);
+ removeLock(ExpLocation, Mu);
break;
}
for (UnlockFunctionAttr::args_iterator I = UFAttr->args_begin(),
- E = UFAttr->args_end(); I != E; ++I)
- removeLock(ExpLocation, *I, Parent);
+ E = UFAttr->args_end(); I != E; ++I) {
+ MutexID Mu(Handler, *I, Exp, D);
+ removeLock(ExpLocation, Mu);
+ }
break;
}
@@ -556,7 +572,7 @@ void BuildLockset::VisitCXXMemberCallExpr(CXXMemberCallExpr *Exp) {
LocksExcludedAttr *LEAttr = cast<LocksExcludedAttr>(Attr);
for (LocksExcludedAttr::args_iterator I = LEAttr->args_begin(),
E = LEAttr->args_end(); I != E; ++I) {
- MutexID Mutex(Handler, *I, Parent);
+ MutexID Mutex(Handler, *I, Exp, D);
if (locksetContains(Mutex))
Handler.handleFunExcludesLock(D->getName(), Mutex.getName(),
ExpLocation);
--
1.7.3.1
From 90eef09604a318840c643283ee62bd6975767775 Mon Sep 17 00:00:00 2001
From: DeLesley Hutchins <[email protected]>
Date: Thu, 15 Sep 2011 09:18:17 -0700
Subject: [PATCH] Substitute for arguments in method calls -- functionality
---
lib/Analysis/ThreadSafety.cpp | 43 ++++++++++++++++----
test/SemaCXX/warn-thread-safety-analysis.cpp | 55 ++++++++++++++++++++++++++
2 files changed, 90 insertions(+), 8 deletions(-)
diff --git a/lib/Analysis/ThreadSafety.cpp b/lib/Analysis/ThreadSafety.cpp
index 26861ae..68f8422 100644
--- a/lib/Analysis/ThreadSafety.cpp
+++ b/lib/Analysis/ThreadSafety.cpp
@@ -149,22 +149,36 @@ class MutexID {
/// Build a Decl sequence representing the lock from the given expression.
/// Recursive function that bottoms out when the final DeclRefExpr is reached.
- void buildMutexID(ThreadSafetyHandler &Handler, Expr *Exp, Expr *Parent) {
+ void buildMutexID(ThreadSafetyHandler &Handler, Expr *Exp,
+ Expr *Parent, int NumArgs,
+ const NamedDecl **FunArgDecls, Expr **FunArgs) {
if (DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(Exp)) {
+ if (FunArgDecls) {
+ // Substitute call arguments for references to function parameters
+ for (int i = 0; i < NumArgs; ++i) {
+ if (DRE->getDecl() == FunArgDecls[i]) {
+ buildMutexID(Handler, FunArgs[i], 0, 0, 0, 0);
+ return;
+ }
+ }
+ }
NamedDecl *ND = cast<NamedDecl>(DRE->getDecl()->getCanonicalDecl());
DeclSeq.push_back(ND);
} else if (MemberExpr *ME = dyn_cast<MemberExpr>(Exp)) {
NamedDecl *ND = ME->getMemberDecl();
DeclSeq.push_back(ND);
- buildMutexID(Handler, ME->getBase(), Parent);
+ buildMutexID(Handler, ME->getBase(), Parent,
+ NumArgs, FunArgDecls, FunArgs);
} else if (isa<CXXThisExpr>(Exp)) {
if (!Parent)
return;
- buildMutexID(Handler, Parent, 0);
- } else if (CastExpr *CE = dyn_cast<CastExpr>(Exp))
- buildMutexID(Handler, CE->getSubExpr(), Parent);
- else
+ buildMutexID(Handler, Parent, 0, 0, 0, 0);
+ } else if (CastExpr *CE = dyn_cast<CastExpr>(Exp)) {
+ buildMutexID(Handler, CE->getSubExpr(), Parent,
+ NumArgs, FunArgDecls, FunArgs);
+ } else {
Handler.handleInvalidLockExp(Exp->getExprLoc());
+ }
}
/// \brief Construct a MutexID from an expression.
@@ -174,20 +188,33 @@ class MutexID {
void buildMutexIDFromExp(ThreadSafetyHandler &Handler, Expr* AttrExp,
Expr *Exp, const NamedDecl* Dec) {
Expr* Parent = 0;
+ unsigned NumArgs = 0;
+ Expr **FunArgs = 0;
+ SmallVector<const NamedDecl*, 8> FunArgDecls;
if (MemberExpr *ME = dyn_cast<MemberExpr>(Exp)) {
Parent = ME->getBase();
}
else if (CXXMemberCallExpr *CE = dyn_cast<CXXMemberCallExpr>(Exp)) {
Parent = CE->getImplicitObjectArgument();
+ NumArgs = CE->getNumArgs();
+ FunArgs = CE->getArgs();
}
// If the attribute has no arguments, then assume the argument is "this".
if (AttrExp == 0) {
- buildMutexID(Handler, Parent, 0);
+ buildMutexID(Handler, Parent, 0, 0, 0, 0);
return;
}
- buildMutexID(Handler, AttrExp, Parent);
+
+ // FIXME: handle default arguments
+ if (const FunctionDecl *D = dyn_cast_or_null<FunctionDecl>(Dec)) {
+ for (unsigned i = 0, ni = D->getNumParams(); i < ni && i < NumArgs; ++i) {
+ FunArgDecls.push_back(D->getParamDecl(i));
+ }
+ }
+ buildMutexID(Handler, AttrExp, Parent,
+ NumArgs, &FunArgDecls.front(), FunArgs);
}
public:
diff --git a/test/SemaCXX/warn-thread-safety-analysis.cpp b/test/SemaCXX/warn-thread-safety-analysis.cpp
index 85de918..57c1308 100644
--- a/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ b/test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -701,3 +701,58 @@ void es_bad_7() {
// expected-warning {{cannot call function 'le_fun' while holding mutex 'sls_mu'}}
sls_mu.Unlock();
}
+
+
+namespace substitution_test {
+ class MyData {
+ public:
+ Mutex mu;
+
+ void lockData() __attribute__((exclusive_lock_function(mu))) { }
+ void unlockData() __attribute__((unlock_function(mu))) { }
+
+ void doSomething() __attribute__((exclusive_locks_required(mu))) { }
+ };
+
+
+ class DataLocker {
+ public:
+ void lockData (MyData *d) __attribute__((exclusive_lock_function(d->mu))) { }
+ void unlockData(MyData *d) __attribute__((unlock_function(d->mu))) { }
+ };
+
+
+ class Foo {
+ public:
+ void foo(MyData* d) __attribute__((exclusive_locks_required(d->mu))) { }
+
+ void bar1(MyData* d) {
+ d->lockData();
+ foo(d);
+ d->unlockData();
+ }
+
+ void bar2(MyData* d) {
+ DataLocker dlr;
+ dlr.lockData(d);
+ foo(d);
+ dlr.unlockData(d);
+ }
+
+ void bar3(MyData* d1, MyData* d2) {
+ DataLocker dlr;
+ dlr.lockData(d1); // \
+ // expected-warning {{mutex 'mu' is still held at the end of function 'bar3'}}
+ dlr.unlockData(d2); // \
+ // expected-warning {{unlocking 'mu' that was not locked}}
+ }
+
+ void bar4(MyData* d1, MyData* d2) {
+ DataLocker dlr;
+ dlr.lockData(d1);
+ foo(d2); // \
+ // expected-warning {{calling function 'foo' requires exclusive lock on 'mu'}}
+ dlr.unlockData(d1);
+ }
+ };
+} // end namespace substituation_test
--
1.7.3.1
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
