Updated patch enclosed, and found at: http://codereview.appspot.com/5293049/
-DeLesley On Thu, Oct 20, 2011 at 7:51 PM, Richard Smith <[email protected]> wrote: > 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); > > > -- DeLesley Hutchins | Software Engineer | [email protected] | 505-206-0315
From a2e1e056303b920299fffb441acc60b7d8fbe4b1 Mon Sep 17 00:00:00 2001 From: DeLesley Hutchins <[email protected]> Date: Wed, 19 Oct 2011 15:41:04 -0700 Subject: [PATCH] Thread safety analysis refactoring: invalid lock expressions. --- include/clang/Basic/DiagnosticSemaKinds.td | 2 +- lib/Analysis/ThreadSafety.cpp | 90 +++++++++++++++++--------- lib/Sema/AnalysisBasedWarnings.cpp | 11 +++- test/SemaCXX/warn-thread-safety-analysis.cpp | 36 ++++++++--- 4 files changed, 97 insertions(+), 42 deletions(-) diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td index 38ba954..41581ae 100644 --- a/include/clang/Basic/DiagnosticSemaKinds.td +++ b/include/clang/Basic/DiagnosticSemaKinds.td @@ -1575,7 +1575,7 @@ def warn_fun_excludes_mutex : Warning< "cannot call function '%0' while mutex '%1' is locked">, InGroup<ThreadSafety>, DefaultIgnore; def warn_cannot_resolve_lock : Warning< - "cannot resolve lock expression to a specific lockable object">, + "cannot resolve lock expression">, InGroup<ThreadSafety>, DefaultIgnore; diff --git a/lib/Analysis/ThreadSafety.cpp b/lib/Analysis/ThreadSafety.cpp index 5f03b58..5918f10 100644 --- a/lib/Analysis/ThreadSafety.cpp +++ b/lib/Analysis/ThreadSafety.cpp @@ -164,6 +164,11 @@ class MutexID { /// ensure that the original expression is a valid mutex expression. void buildMutexID(Expr *Exp, Expr *Parent, int NumArgs, const NamedDecl **FunArgDecls, Expr **FunArgs) { + if (!Exp) { + DeclSeq.clear(); + return; + } + if (DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(Exp)) { if (FunArgDecls) { // Substitute call arguments for references to function parameters @@ -204,6 +209,7 @@ class MutexID { Expr **FunArgs = 0; SmallVector<const NamedDecl*, 8> FunArgDecls; + // If we are processing a raw attribute expression, with no substitutions. if (DeclExp == 0) { buildMutexID(MutexExp, 0, 0, 0, 0); return; @@ -254,6 +260,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; } @@ -333,6 +351,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; @@ -343,6 +363,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, @@ -407,7 +429,8 @@ void BuildLockset::addLock(SourceLocation LockLoc, MutexID &Mutex, // FIXME: Don't always warn when we have support for reentrant locks. if (locksetContains(Mutex)) 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. @@ -417,8 +440,8 @@ void BuildLockset::removeLock(SourceLocation UnlockLoc, MutexID &Mutex) { Lockset NewLSet = LocksetFactory.remove(LSet, Mutex); if(NewLSet == LSet) Handler.handleUnmatchedUnlock(Mutex.getName(), UnlockLoc); - - LSet = NewLSet; + else + LSet = NewLSet; } /// \brief This function, parameterized by an attribute type, is used to add a @@ -432,10 +455,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; @@ -444,12 +466,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 removes a set of locks specified as attribute +/// arguments to the lockset. +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)) @@ -470,7 +519,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()); } @@ -533,8 +582,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]; @@ -559,24 +606,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; } @@ -605,10 +635,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; } @@ -741,7 +771,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); diff --git a/lib/Sema/AnalysisBasedWarnings.cpp b/lib/Sema/AnalysisBasedWarnings.cpp index a8ee0b8..bd34dec 100644 --- a/lib/Sema/AnalysisBasedWarnings.cpp +++ b/lib/Sema/AnalysisBasedWarnings.cpp @@ -648,15 +648,21 @@ struct SortDiagBySourceLocation { class ThreadSafetyReporter : public clang::thread_safety::ThreadSafetyHandler { Sema &S; DiagList Warnings; + SourceLocation FunLocation; // Helper functions void warnLockMismatch(unsigned DiagID, Name LockName, SourceLocation Loc) { + // Gracefully handle rare cases when the analysis can't get a more + // precise source location. + if (!Loc.isValid()) + Loc = FunLocation; PartialDiagnostic Warning = S.PDiag(DiagID) << LockName; Warnings.push_back(DelayedDiag(Loc, Warning)); } public: - ThreadSafetyReporter(Sema &S) : S(S) {} + ThreadSafetyReporter(Sema &S, SourceLocation FL) + : S(S), FunLocation(FL) {} /// \brief Emit all buffered diagnostics in order of sourcelocation. /// We need to output diagnostics produced while iterating through @@ -913,7 +919,8 @@ AnalysisBasedWarnings::IssueWarnings(sema::AnalysisBasedWarnings::Policy P, // Check for thread safety violations if (P.enableThreadSafetyAnalysis) { - thread_safety::ThreadSafetyReporter Reporter(S); + SourceLocation FL = AC.getDecl()->getLocation(); + thread_safety::ThreadSafetyReporter Reporter(S, FL); thread_safety::runThreadSafetyAnalysis(AC, Reporter); Reporter.emitDiagnostics(); } diff --git a/test/SemaCXX/warn-thread-safety-analysis.cpp b/test/SemaCXX/warn-thread-safety-analysis.cpp index efa19fd..17b6e83 100644 --- a/test/SemaCXX/warn-thread-safety-analysis.cpp +++ b/test/SemaCXX/warn-thread-safety-analysis.cpp @@ -749,23 +749,22 @@ Mutex UPmu; // FIXME: add support for lock expressions involving arrays. Mutex mua[5]; -int x __attribute__((guarded_by(UPmu = sls_mu))); // \ - // expected-warning{{cannot resolve lock expression to a specific lockable object}} -int y __attribute__((guarded_by(mua[0]))); // \ - // expected-warning{{cannot resolve lock expression to a specific lockable object}} +int x __attribute__((guarded_by(UPmu = sls_mu))); +int y __attribute__((guarded_by(mua[0]))); void testUnparse() { - // no errors, since the lock expressions are not resolved - x = 5; - y = 5; + x = 5; // \ + // expected-warning{{cannot resolve lock expression}} + y = 5; // \ + // expected-warning{{cannot resolve lock expression}} } void testUnparse2() { mua[0].Lock(); // \ - // expected-warning{{cannot resolve lock expression to a specific lockable object}} + // expected-warning{{cannot resolve lock expression}} (&(mua[0]) + 4)->Lock(); // \ - // expected-warning{{cannot resolve lock expression to a specific lockable object}} + // expected-warning{{cannot resolve lock expression}} } @@ -1475,3 +1474,22 @@ namespace substitution_test { }; } // end namespace substituation_test + +namespace invalid_lock_expression_test { + +class LOCKABLE MyLockable { +public: + MyLockable() __attribute__((exclusive_lock_function)) { } + ~MyLockable() __attribute__((unlock_function)) { } +}; + +// create an empty lock expression +void foo() { + MyLockable lock; // \ + // expected-warning {{cannot resolve lock expression}} +} + +} // end namespace invalid_lock_expression_test + + + -- 1.7.3.1
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
