[cfe-commits] r146174 - in /cfe/trunk: lib/Analysis/ThreadSafety.cpp lib/Sema/AnalysisBasedWarnings.cpp test/SemaCXX/warn-thread-safety-analysis.cpp
David Blaikie
dblaikie at gmail.com
Thu Dec 8 12:32:10 PST 2011
On Thu, Dec 8, 2011 at 12:23 PM, DeLesley Hutchins <delesley at google.com> wrote:
> Author: delesley
> Date: Thu Dec 8 14:23:06 2011
> New Revision: 146174
>
> URL: http://llvm.org/viewvc/llvm-project?rev=146174&view=rev
> Log:
> This patch extends thread safety analysis with support for the scoped_lockable attribute.
>
> Modified:
> cfe/trunk/lib/Analysis/ThreadSafety.cpp
> cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
> cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp
>
> Modified: cfe/trunk/lib/Analysis/ThreadSafety.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/ThreadSafety.cpp?rev=146174&r1=146173&r2=146174&view=diff
> ==============================================================================
> --- cfe/trunk/lib/Analysis/ThreadSafety.cpp (original)
> +++ cfe/trunk/lib/Analysis/ThreadSafety.cpp Thu Dec 8 14:23:06 2011
> @@ -172,6 +172,10 @@
> }
>
> public:
> + explicit MutexID(clang::Decl::EmptyShell e) {
> + DeclSeq.clear();
> + }
> +
> /// \param MutexExp The original mutex expression within an attribute
> /// \param DeclExp An expression involving the Decl on which the attribute
> /// occurs.
> @@ -249,9 +253,14 @@
> ///
> /// FIXME: add support for re-entrant locking and lock up/downgrading
> LockKind LKind;
> + MutexID UnderlyingMutex; // for ScopedLockable objects
>
> LockData(SourceLocation AcquireLoc, LockKind LKind)
> - : AcquireLoc(AcquireLoc), LKind(LKind) {}
> + : AcquireLoc(AcquireLoc), LKind(LKind), UnderlyingMutex(Decl::EmptyShell())
> + {}
> +
> + LockData(SourceLocation AcquireLoc, LockKind LKind, const MutexID &Mu)
> + : AcquireLoc(AcquireLoc), LKind(LKind), UnderlyingMutex(Mu) {}
>
> bool operator==(const LockData &other) const {
> return AcquireLoc == other.AcquireLoc && LKind == other.LKind;
> @@ -285,11 +294,12 @@
> Lockset::Factory &LocksetFactory;
>
> // Helper functions
> - void removeLock(SourceLocation UnlockLoc, MutexID &Mutex);
> - void addLock(SourceLocation LockLoc, MutexID &Mutex, LockKind LK);
> + void addLock(const MutexID &Mutex, const LockData &LDat);
> + void removeLock(const 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);
>
> @@ -298,7 +308,7 @@
> 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.
> @@ -342,51 +352,62 @@
> 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(const 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(const 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->UnderlyingMutex.isValid())
> + removeLock(LDat->UnderlyingMutex, UnlockLoc);
> + 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)) {
> + CXXRecordDecl* PD = CD->getParent();
> + if (PD && PD->getAttr<ScopedLockableAttr>())
> + 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;
> }
>
> @@ -394,8 +415,15 @@
> 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 underlying mutex.
> + DeclRefExpr DRE(VD, VD->getType(), VK_LValue, VD->getLocation());
> + MutexID SMutex(&DRE, 0, 0);
> + addLock(SMutex, LockData(VD->getLocation(), LK, Mutex));
> + }
> + }
> }
> }
>
> @@ -412,7 +440,7 @@
> if (!Mu.isValid())
> MutexID::warnInvalidLock(Handler, 0, Exp, FunDecl);
> else
> - removeLock(ExpLocation, Mu);
> + removeLock(Mu, ExpLocation);
> return;
> }
>
> @@ -422,7 +450,7 @@
> if (!Mutex.isValid())
> MutexID::warnInvalidLock(Handler, *I, Exp, FunDecl);
> else
> - removeLock(ExpLocation, Mutex);
> + removeLock(Mutex, ExpLocation);
> }
> }
>
> @@ -508,7 +536,7 @@
> ///
> /// 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];
> @@ -517,7 +545,7 @@
> // 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;
> }
>
> @@ -525,7 +553,7 @@
> // 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;
> }
>
> @@ -627,10 +655,23 @@
> }
>
> 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);
> + }
> + }
> + }
> }
>
>
>
> Modified: cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp?rev=146174&r1=146173&r2=146174&view=diff
> ==============================================================================
> --- cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp (original)
> +++ cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp Thu Dec 8 14:23:06 2011
> @@ -834,8 +834,8 @@
> // 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) {
> - // Unreachable code analysis requires a linearized CFG.
> + if (P.enableCheckUnreachable || P.enableThreadSafetyAnalysis) {
> + // Unreachable code analysis and thread safety require a linearized CFG.
> AC.getCFGBuildOptions().setAllAlwaysAdd();
> }
> else {
>
> Modified: cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp?rev=146174&r1=146173&r2=146174&view=diff
> ==============================================================================
> --- cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp (original)
> +++ cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp Thu Dec 8 14:23:06 2011
> @@ -36,6 +36,18 @@
> 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;
>
> @@ -1549,3 +1561,47 @@
> template struct W<int>; // expected-note {{here}}
>
> }
> +
> +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}}
> + } // expected-warning {{unlocking 'mu1' that was not locked}}
I assume at some point this could be extended/improved to not provide
that second warning so the user is directed to the relevant issue at
the MutexLock construction?
- David
> +
> + void foo4() {
> + MutexLock mulock1(&mu1), mulock2(&mu2);
> + a = b+1;
> + b = a+1;
> + }
> +};
> +
> +} // end namespace test_scoped_lockable
> +
> +
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
More information about the cfe-commits
mailing list