Thread safety: This patch deals with previously unhandled cases when building lock expressions. We now resolve this expressions, avoid crashing when encountering cast expressions, and have a diagnostic for unresolved lock expressions
http://codereview.appspot.com/4968075/ Cheers, Caitlin
From 5e0cb9755df699f95a0e2e2fd289641a4349ef96 Mon Sep 17 00:00:00 2001 From: Caitlin Sadowski <[email protected]> Date: Wed, 7 Sep 2011 11:54:44 -0700 Subject: [PATCH] Thread safety: This patch deals with previously unhandled cases when building lock expressions. We now resolve this expressions, avoid crashing when encountering cast expressions, and have a diagnostic for unresolved lock expressions --- include/clang/Analysis/Analyses/ThreadSafety.h | 1 + include/clang/Basic/DiagnosticSemaKinds.td | 3 + lib/Analysis/ThreadSafety.cpp | 58 ++++++++++++------- lib/Sema/AnalysisBasedWarnings.cpp | 4 + test/SemaCXX/warn-thread-safety-analysis.cpp | 74 ++++++++++++++++++++++++ 5 files changed, 119 insertions(+), 21 deletions(-) diff --git a/include/clang/Analysis/Analyses/ThreadSafety.h b/include/clang/Analysis/Analyses/ThreadSafety.h index 2f19973..36795f6 100644 --- a/include/clang/Analysis/Analyses/ThreadSafety.h +++ b/include/clang/Analysis/Analyses/ThreadSafety.h @@ -47,6 +47,7 @@ public: typedef llvm::StringRef Name; ThreadSafetyHandler() {} virtual ~ThreadSafetyHandler() {} + virtual void handleBogusLockExp(SourceLocation Loc) {} virtual void handleUnmatchedUnlock(Name LockName, SourceLocation Loc) {} virtual void handleDoubleLock(Name LockName, SourceLocation Loc) {} virtual void handleMutexHeldEndOfScope(Name LockName, SourceLocation Loc){} diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td index 0dd2099..2979c90 100644 --- a/include/clang/Basic/DiagnosticSemaKinds.td +++ b/include/clang/Basic/DiagnosticSemaKinds.td @@ -1424,6 +1424,9 @@ def warn_fun_requires_lock : Warning< def warn_fun_excludes_mutex : Warning< "cannot call function '%0' while holding mutex '%1'">, InGroup<ThreadSafety>, DefaultIgnore; +def warn_cannot_resolve_lock : Warning< + "cannot resolve lock expression">, + InGroup<ThreadSafety>, DefaultIgnore; def warn_impcast_vector_scalar : Warning< diff --git a/lib/Analysis/ThreadSafety.cpp b/lib/Analysis/ThreadSafety.cpp index b0d1d30..b3bbc36 100644 --- a/lib/Analysis/ThreadSafety.cpp +++ b/lib/Analysis/ThreadSafety.cpp @@ -37,6 +37,15 @@ 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. /// @@ -146,28 +155,32 @@ 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) { + void buildMutexID(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()); + buildMutexID(ME->getBase(), Parent); } else if (isa<CXXThisExpr>(Exp)) { - return; - } else { - // FIXME: add diagnostic - llvm::report_fatal_error("Expected lock expression!"); - } + if (!Parent) + return; + buildMutexID(Parent, 0); + } else if (CastExpr *CE = dyn_cast<CastExpr>(Exp)) + buildMutexID(CE->getSubExpr(), Parent); + else + Handler.handleBogusLockExp(Exp->getExprLoc()); } public: - MutexID(Expr *LExpr) { - buildMutexID(LExpr); + MutexID(ThreadSafetyHandler &Handler, Expr *LExpr, Expr *ParentExpr) + : Handler(Handler) { + buildMutexID(LExpr, ParentExpr); assert(!DeclSeq.empty()); } @@ -252,8 +265,9 @@ class BuildLockset : public StmtVisitor<BuildLockset> { Lockset::Factory &LocksetFactory; // Helper functions - void removeLock(SourceLocation UnlockLoc, Expr *LockExp); - void addLock(SourceLocation LockLoc, Expr *LockExp, LockKind LK); + void removeLock(SourceLocation UnlockLoc, Expr *LockExp, Expr *Parent); + void addLock(SourceLocation LockLoc, Expr *LockExp, Expr *Parent, + LockKind LK); const ValueDecl *getValueDecl(Expr *Exp); void warnIfMutexNotHeld (const NamedDecl *D, Expr *Exp, AccessKind AK, Expr *MutexExp, ProtectedOperationKind POK); @@ -307,10 +321,10 @@ 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, +void BuildLockset::addLock(SourceLocation LockLoc, Expr *LockExp, Expr *Parent, LockKind LK) { // FIXME: deal with acquired before/after annotations - MutexID Mutex(LockExp); + MutexID Mutex(Handler, LockExp, Parent); LockData NewLock(LockLoc, LK); // FIXME: Don't always warn when we have support for reentrant locks. @@ -322,8 +336,9 @@ void BuildLockset::addLock(SourceLocation LockLoc, Expr *LockExp, /// \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) { - MutexID Mutex(LockExp); +void BuildLockset::removeLock(SourceLocation UnlockLoc, Expr *LockExp, + Expr *Parent) { + MutexID Mutex(Handler, LockExp, Parent); Lockset NewLSet = LocksetFactory.remove(LSet, Mutex); if(NewLSet == LSet) @@ -349,7 +364,8 @@ void BuildLockset::warnIfMutexNotHeld(const NamedDecl *D, Expr *Exp, AccessKind AK, Expr *MutexExp, ProtectedOperationKind POK) { LockKind LK = getLockKindFromAccessKind(AK); - MutexID Mutex(MutexExp); + Expr *Parent = getParent(Exp); + MutexID Mutex(Handler, MutexExp, Parent); if (!locksetContainsAtLeast(Mutex, LK)) Handler.handleMutexNotHeld(D, POK, Mutex.getName(), LK, Exp->getExprLoc()); } @@ -451,13 +467,13 @@ void BuildLockset::addLocksToSet(LockKind LK, Attr *Attr, if (SpecificAttr->args_size() == 0) { // The mutex held is the "this" object. - addLock(ExpLocation, Parent, LK); + addLock(ExpLocation, Parent, Parent, LK); return; } for (iterator_type I = SpecificAttr->args_begin(), E = SpecificAttr->args_end(); I != E; ++I) - addLock(ExpLocation, *I, LK); + addLock(ExpLocation, *I, Parent, LK); } /// \brief When visiting CXXMemberCallExprs we need to examine the attributes on @@ -501,13 +517,13 @@ 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); + removeLock(ExpLocation, Parent, Parent); break; } for (UnlockFunctionAttr::args_iterator I = UFAttr->args_begin(), E = UFAttr->args_end(); I != E; ++I) - removeLock(ExpLocation, *I); + removeLock(ExpLocation, *I, Parent); break; } @@ -540,7 +556,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(*I); + MutexID Mutex(Handler, *I, Parent); if (locksetContains(Mutex)) Handler.handleFunExcludesLock(D->getName(), Mutex.getName(), ExpLocation); diff --git a/lib/Sema/AnalysisBasedWarnings.cpp b/lib/Sema/AnalysisBasedWarnings.cpp index 405ba9e..1e1636e 100644 --- a/lib/Sema/AnalysisBasedWarnings.cpp +++ b/lib/Sema/AnalysisBasedWarnings.cpp @@ -648,6 +648,10 @@ class ThreadSafetyReporter : public clang::thread_safety::ThreadSafetyHandler { S.Diag(I->first, I->second); } + void handleBogusLockExp(SourceLocation Loc) { + PartialDiagnostic Warning = S.PDiag(diag::warn_cannot_resolve_lock) << Loc; + Warnings.push_back(DelayedDiag(Loc, Warning)); + } void handleUnmatchedUnlock(Name LockName, SourceLocation Loc) { warnLockMismatch(diag::warn_unlock_but_no_lock, LockName, Loc); } diff --git a/test/SemaCXX/warn-thread-safety-analysis.cpp b/test/SemaCXX/warn-thread-safety-analysis.cpp index 9cecea7..85de918 100644 --- a/test/SemaCXX/warn-thread-safety-analysis.cpp +++ b/test/SemaCXX/warn-thread-safety-analysis.cpp @@ -425,6 +425,80 @@ public: Mutex mu; }; +class LateBar { + public: + int a_ __attribute__((guarded_by(mu1_))); + int b_; + int *q __attribute__((pt_guarded_by(mu))); + Mutex mu1_; + Mutex mu; + LateFoo Foo; + LateFoo Foo2; + LateFoo *FooPointer; +}; + +LateBar b1, *b3; + +void late_0() { + LateFoo FooA; + LateFoo FooB; + FooA.mu.Lock(); + FooA.a = 5; + FooA.mu.Unlock(); +} + +void late_1() { + LateBar BarA; + BarA.FooPointer->mu.Lock(); + BarA.FooPointer->a = 2; + BarA.FooPointer->mu.Unlock(); +} + +void late_bad_0() { + LateFoo fooA; + LateFoo fooB; + fooA.mu.Lock(); + fooB.a = 5; // \ + // expected-warning{{writing variable 'a' requires lock on 'mu' to be held exclusively}} + fooA.mu.Unlock(); +} + +void late_bad_1() { + Mutex mu; + mu.Lock(); + b1.mu1_.Lock(); + int res = b1.a_ + b3->b_; + b3->b_ = *b1.q; // \ + // expected-warning{{reading the value pointed to by 'q' requires lock on 'mu' to be held}} + b1.mu1_.Unlock(); + b1.b_ = res; + mu.Unlock(); +} + +void late_bad_2() { + LateBar BarA; + BarA.FooPointer->mu.Lock(); + BarA.Foo.a = 2; // \ + // expected-warning{{writing variable 'a' requires lock on 'mu' to be held exclusively}} + BarA.FooPointer->mu.Unlock(); +} + +void late_bad_3() { + LateBar BarA; + BarA.Foo.mu.Lock(); + BarA.FooPointer->a = 2; // \ + // expected-warning{{writing variable 'a' requires lock on 'mu' to be held exclusively}} + BarA.Foo.mu.Unlock(); +} + +void late_bad_4() { + LateBar BarA; + BarA.Foo.mu.Lock(); + BarA.Foo2.a = 2; // \ + // expected-warning{{writing variable 'a' requires lock on 'mu' to be held exclusively}} + BarA.Foo.mu.Unlock(); +} + //-----------------------------------------------// // Extra warnings for shared vs. exclusive locks // ----------------------------------------------// -- 1.7.3.1
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
