[cfe-commits] [Patch] Thread-safety analysis: more refactoring
Richard Smith
richard at metafoo.co.uk
Thu Oct 20 19:51:44 PDT 2011
On Wed, October 19, 2011 23:58, Delesley Hutchins wrote:
> This patch performs two additional refactorings. The first refactors
> lockset removal into a separate function, to be consistent with lockset
> addition. The second refactors the handling of invalid lock expressions into
> a separate routine, in preparation for better handling of such messages in
> more complex cases, such as attributes on destructors. There is no major
> change in functionality.
>
> http://codereview.appspot.com/5293049/
Some comments below.
--- a/lib/Analysis/ThreadSafety.cpp
+++ b/lib/Analysis/ThreadSafety.cpp
@@ -161,6 +161,11 @@ class MutexID {
/// Recursive function that bottoms out when the final DeclRefExpr is reached.
void buildMutexID(Expr *Exp, Expr *Parent, int NumArgs,
const NamedDecl **FunArgDecls, Expr **FunArgs) {
+ if (!Exp) {
+ DeclSeq.clear();
+ return;
+ }
Can this happen? If this is a crash fix, there should be a testcase, otherwise
this should be an assert.
+
if (DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(Exp)) {
if (FunArgDecls) {
// Substitute call arguments for references to function parameters
@@ -202,11 +207,13 @@ class MutexID {
Expr **FunArgs = 0;
SmallVector<const NamedDecl*, 8> FunArgDecls;
+ // If we are processing a raw attribute expression...
if (DeclExp == 0) {
buildMutexID(MutexExp, 0, 0, 0, 0);
return;
}
+ // Get Parent and FunArgs from DeclExp
It'd be nice if this comment explained why we're doing this. As an outsider to
this code, it wasn't immediately obvious to me that we want to extract these
for substitution.
if (MemberExpr *ME = dyn_cast<MemberExpr>(DeclExp)) {
Parent = ME->getBase();
} else if (CXXMemberCallExpr *CE = dyn_cast<CXXMemberCallExpr>(DeclExp)) {
@@ -250,6 +257,18 @@ public:
return !DeclSeq.empty();
}
+ /// Issue a warning about an invalid lock expression
+ static void warnInvalidLock(ThreadSafetyHandler &Handler, Expr* MutexExp,
+ Expr *DeclExp, const NamedDecl* D) {
+ SourceLocation Loc;
+ if (DeclExp)
+ Loc = DeclExp->getExprLoc();
+
+ // FIXME: add a note about the attribute location in MutexExp or D
+ if (Loc.isValid())
+ Handler.handleInvalidLockExp(Loc);
+ }
+
bool operator==(const MutexID &other) const {
return DeclSeq == other.DeclSeq;
}
@@ -329,6 +348,8 @@ typedef llvm::ImmutableMap<MutexID, LockData> Lockset;
/// output error messages related to missing locks.
/// FIXME: In future, we may be able to not inherit from a visitor.
class BuildLockset : public StmtVisitor<BuildLockset> {
+ friend class ThreadSafetyAnalyzer;
+
ThreadSafetyHandler &Handler;
Lockset LSet;
Lockset::Factory &LocksetFactory;
@@ -339,6 +360,8 @@ class BuildLockset : public StmtVisitor<BuildLockset> {
template <class AttrType>
void addLocksToSet(LockKind LK, AttrType *Attr, Expr *Exp, NamedDecl *D);
+ void removeLocksFromSet(UnlockFunctionAttr *Attr,
+ Expr *Exp, NamedDecl* FunDecl);
const ValueDecl *getValueDecl(Expr *Exp);
void warnIfMutexNotHeld (const NamedDecl *D, Expr *Exp, AccessKind AK,
@@ -401,9 +424,10 @@ void BuildLockset::addLock(SourceLocation LockLoc,
MutexID &Mutex,
LockData NewLock(LockLoc, LK);
// FIXME: Don't always warn when we have support for reentrant locks.
- if (locksetContains(Mutex))
+ if (locksetContains(Mutex) && LockLoc.isValid())
What's the meaning of an invalid LockLoc? A test or an assert would elucidate.
Handler.handleDoubleLock(Mutex.getName(), LockLoc);
- LSet = LocksetFactory.add(LSet, Mutex, NewLock);
+ else
+ LSet = LocksetFactory.add(LSet, Mutex, NewLock);
}
/// \brief Remove a lock from the lockset, warning if the lock is not there.
@@ -411,10 +435,10 @@ void BuildLockset::addLock(SourceLocation LockLoc,
MutexID &Mutex,
/// \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)
+ if(NewLSet == LSet && UnlockLoc.isValid())
Handler.handleUnmatchedUnlock(Mutex.getName(), UnlockLoc);
-
- LSet = NewLSet;
+ else
+ LSet = NewLSet;
}
/// \brief This function, parameterized by an attribute type, is used to add a
@@ -428,10 +452,9 @@ void BuildLockset::addLocksToSet(LockKind LK, AttrType
*Attr,
if (Attr->args_size() == 0) {
// The mutex held is the "this" object.
-
MutexID Mutex(0, Exp, FunDecl);
if (!Mutex.isValid())
- Handler.handleInvalidLockExp(Exp->getExprLoc());
+ MutexID::warnInvalidLock(Handler, 0, Exp, FunDecl);
else
addLock(ExpLocation, Mutex, LK);
return;
@@ -440,12 +463,39 @@ void BuildLockset::addLocksToSet(LockKind LK, AttrType
*Attr,
for (iterator_type I=Attr->args_begin(), E=Attr->args_end(); I != E; ++I) {
MutexID Mutex(*I, Exp, FunDecl);
if (!Mutex.isValid())
- Handler.handleInvalidLockExp(Exp->getExprLoc());
+ MutexID::warnInvalidLock(Handler, *I, Exp, FunDecl);
else
addLock(ExpLocation, Mutex, LK);
}
}
+/// \brief this function adds a set of locks specified as attribute arguments
+/// to the lockset.
This comment needs updating.
+void BuildLockset::removeLocksFromSet(UnlockFunctionAttr *Attr,
+ Expr *Exp, NamedDecl* FunDecl) {
+ SourceLocation ExpLocation;
+ if (Exp) ExpLocation = Exp->getExprLoc();
+
+ if (Attr->args_size() == 0) {
+ // The mutex held is the "this" object.
+ MutexID Mu(0, Exp, FunDecl);
+ if (!Mu.isValid())
+ MutexID::warnInvalidLock(Handler, 0, Exp, FunDecl);
+ else
+ removeLock(ExpLocation, Mu);
+ return;
+ }
+
+ for (UnlockFunctionAttr::args_iterator I = Attr->args_begin(),
+ E = Attr->args_end(); I != E; ++I) {
+ MutexID Mutex(*I, Exp, FunDecl);
+ if (!Mutex.isValid())
+ MutexID::warnInvalidLock(Handler, *I, Exp, FunDecl);
+ else
+ removeLock(ExpLocation, Mutex);
+ }
+}
+
/// \brief Gets the value decl pointer from DeclRefExprs or MemberExprs
const ValueDecl *BuildLockset::getValueDecl(Expr *Exp) {
if (const DeclRefExpr *DR = dyn_cast<DeclRefExpr>(Exp))
@@ -466,7 +516,7 @@ void BuildLockset::warnIfMutexNotHeld(const NamedDecl *D,
Expr *Exp,
MutexID Mutex(MutexExp, Exp, D);
if (!Mutex.isValid())
- Handler.handleInvalidLockExp(MutexExp->getExprLoc());
+ MutexID::warnInvalidLock(Handler, MutexExp, Exp, D);
else if (!locksetContainsAtLeast(Mutex, LK))
Handler.handleMutexNotHeld(D, POK, Mutex.getName(), LK, Exp->getExprLoc());
}
@@ -529,8 +579,6 @@ 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) {
- SourceLocation ExpLocation = Exp->getExprLoc();
-
AttrVec &ArgAttrs = D->getAttrs();
for(unsigned i = 0; i < ArgAttrs.size(); ++i) {
Attr *Attr = ArgAttrs[i];
@@ -555,24 +603,7 @@ void BuildLockset::handleCall(Expr *Exp, NamedDecl *D) {
// mutexes from the lockset, and flag a warning if they are not there.
case attr::UnlockFunction: {
UnlockFunctionAttr *UFAttr = cast<UnlockFunctionAttr>(Attr);
-
- if (UFAttr->args_size() == 0) { // The lock held is the "this" object.
- MutexID Mu(0, Exp, D);
- if (!Mu.isValid())
- Handler.handleInvalidLockExp(Exp->getExprLoc());
- else
- removeLock(ExpLocation, Mu);
- break;
- }
-
- for (UnlockFunctionAttr::args_iterator I = UFAttr->args_begin(),
- E = UFAttr->args_end(); I != E; ++I) {
- MutexID Mutex(*I, Exp, D);
- if (!Mutex.isValid())
- Handler.handleInvalidLockExp(Exp->getExprLoc());
- else
- removeLock(ExpLocation, Mutex);
- }
+ removeLocksFromSet(UFAttr, Exp, D);
break;
}
@@ -601,10 +632,10 @@ void BuildLockset::handleCall(Expr *Exp, NamedDecl *D) {
E = LEAttr->args_end(); I != E; ++I) {
MutexID Mutex(*I, Exp, D);
if (!Mutex.isValid())
- Handler.handleInvalidLockExp((*I)->getExprLoc());
+ MutexID::warnInvalidLock(Handler, *I, Exp, D);
else if (locksetContains(Mutex))
Handler.handleFunExcludesLock(D->getName(), Mutex.getName(),
- ExpLocation);
+ Exp->getExprLoc());
}
break;
}
@@ -737,7 +768,7 @@ Lockset ThreadSafetyAnalyzer::addLock(Lockset &LSet, Expr
*MutexExp,
LockKind LK, SourceLocation Loc) {
MutexID Mutex(MutexExp, 0, D);
if (!Mutex.isValid()) {
- Handler.handleInvalidLockExp(MutexExp->getExprLoc());
+ MutexID::warnInvalidLock(Handler, MutexExp, 0, D);
return LSet;
}
LockData NewLock(Loc, LK);
More information about the cfe-commits
mailing list