diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafety.h b/clang/include/clang/Analysis/Analyses/ThreadSafety.h index 5def3dd3dfd1..417451dbf056 100644 --- a/clang/include/clang/Analysis/Analyses/ThreadSafety.h +++ b/clang/include/clang/Analysis/Analyses/ThreadSafety.h @@ -39,7 +39,8 @@ enum ProtectedOperationKind { /// mutex. enum LockKind { LK_Shared, ///< Shared/reader lock of a mutex. - LK_Exclusive ///< Exclusive/writer lock of a mutex. + LK_Exclusive, ///< Exclusive/writer lock of a mutex. + LK_Generic ///< Can be either Shared or Exclusive }; /// This enum distinguishes between different ways to access (read or write) a @@ -82,6 +83,18 @@ public: /// \param Loc -- The SourceLocation of the Unlock virtual void handleUnmatchedUnlock(Name LockName, SourceLocation Loc) {} + /// Warn about an unlock function call that attempts to unlock a lock with + /// the incorrect lock kind. For instance, a shared lock being unlocked + /// exclusively, or vice versa. + /// \param LockName -- A StringRef name for the lock expression, to be printed + /// in the error message. + /// \param Expected -- the kind of lock expected. + /// \param Received -- the kind of lock received. + /// \param Loc -- The SourceLocation of the Unlock. + virtual void handleIncorrectUnlockKind(Name LockName, LockKind Expected, + LockKind Received, + SourceLocation Loc) {} + /// Warn about lock function calls for locks which are already held. /// \param LockName -- A StringRef name for the lock expression, to be printed /// in the error message. diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td index c567cf29ebee..3eb5d4e0f3fd 100644 --- a/clang/include/clang/Basic/Attr.td +++ b/clang/include/clang/Basic/Attr.td @@ -1388,7 +1388,8 @@ def ReleaseCapability : InheritableAttr { CXX11<"clang", "release_shared_capability">]>, Accessor<"isGeneric", [GNU<"release_generic_capability">, - CXX11<"clang", "release_generic_capability">]>]; + CXX11<"clang", "release_generic_capability">, + GNU<"unlock_function">]>]; let Documentation = [ReleaseCapabilityDocs]; } diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td index 5e4593051265..179c605aeb6f 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -2201,6 +2201,10 @@ def err_attribute_argument_out_of_range : Error< def warn_unlock_but_no_lock : Warning< "unlocking '%0' that was not locked">, InGroup, DefaultIgnore; +def warn_unlock_kind_mismatch : Warning< + "unlocking '%0' using %select{shared|exclusive}1 access, expected " + "%select{shared|exclusive}2 access">, + InGroup, DefaultIgnore; def warn_double_lock : Warning< "locking '%0' that is already locked">, InGroup, DefaultIgnore; diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp index cde19b531e04..5f7ce2d59281 100644 --- a/clang/lib/Analysis/ThreadSafety.cpp +++ b/clang/lib/Analysis/ThreadSafety.cpp @@ -1430,7 +1430,7 @@ public: void addLock(FactSet &FSet, const SExpr &Mutex, const LockData &LDat); void removeLock(FactSet &FSet, const SExpr &Mutex, - SourceLocation UnlockLoc, bool FullyRemove=false); + SourceLocation UnlockLoc, bool FullyRemove, LockKind Kind); template void getMutexIDs(MutexIDList &Mtxs, AttrType *Attr, Expr *Exp, @@ -1486,10 +1486,9 @@ void ThreadSafetyAnalyzer::addLock(FactSet &FSet, const SExpr &Mutex, /// \brief Remove a lock from the lockset, warning if the lock is not there. /// \param Mutex The lock expression corresponding to the lock to be removed /// \param UnlockLoc The source location of the unlock (only used in error msg) -void ThreadSafetyAnalyzer::removeLock(FactSet &FSet, - const SExpr &Mutex, +void ThreadSafetyAnalyzer::removeLock(FactSet &FSet, const SExpr &Mutex, SourceLocation UnlockLoc, - bool FullyRemove) { + bool FullyRemove, LockKind ReceivedKind) { if (Mutex.shouldIgnore()) return; @@ -1499,6 +1498,14 @@ void ThreadSafetyAnalyzer::removeLock(FactSet &FSet, return; } + // Generic lock removal doesn't care about lock kind mismatches, but + // otherwise diagnose when the lock kinds are mismatched. + if (ReceivedKind != LK_Generic && LDat->LKind != ReceivedKind) { + Handler.handleIncorrectUnlockKind(Mutex.toString(), LDat->LKind, + ReceivedKind, UnlockLoc); + return; + } + if (LDat->UnderlyingMutex.isValid()) { // This is scoped lockable object, which manages the real mutex. if (FullyRemove) { @@ -1926,9 +1933,8 @@ void BuildLockset::checkPtAccess(const Expr *Exp, AccessKind AK) { void BuildLockset::handleCall(Expr *Exp, const NamedDecl *D, VarDecl *VD) { SourceLocation Loc = Exp->getExprLoc(); const AttrVec &ArgAttrs = D->getAttrs(); - MutexIDList ExclusiveLocksToAdd; - MutexIDList SharedLocksToAdd; - MutexIDList LocksToRemove; + MutexIDList ExclusiveLocksToAdd, SharedLocksToAdd; + MutexIDList ExclusiveLocksToRemove, SharedLocksToRemove, GenericLocksToRemove; for(unsigned i = 0; i < ArgAttrs.size(); ++i) { Attr *At = const_cast(ArgAttrs[i]); @@ -1973,7 +1979,12 @@ void BuildLockset::handleCall(Expr *Exp, const NamedDecl *D, VarDecl *VD) { // mutexes from the lockset, and flag a warning if they are not there. case attr::ReleaseCapability: { auto *A = cast(At); - Analyzer->getMutexIDs(LocksToRemove, A, Exp, D, VD); + if (A->isGeneric()) + Analyzer->getMutexIDs(GenericLocksToRemove, A, Exp, D, VD); + else if (A->isShared()) + Analyzer->getMutexIDs(SharedLocksToRemove, A, Exp, D, VD); + else + Analyzer->getMutexIDs(ExclusiveLocksToRemove, A, Exp, D, VD); break; } @@ -2014,14 +2025,10 @@ void BuildLockset::handleCall(Expr *Exp, const NamedDecl *D, VarDecl *VD) { } // Add locks. - for (unsigned i=0,n=ExclusiveLocksToAdd.size(); iaddLock(FSet, ExclusiveLocksToAdd[i], - LockData(Loc, LK_Exclusive, isScopedVar)); - } - for (unsigned i=0,n=SharedLocksToAdd.size(); iaddLock(FSet, SharedLocksToAdd[i], - LockData(Loc, LK_Shared, isScopedVar)); - } + for (const auto &M : ExclusiveLocksToAdd) + Analyzer->addLock(FSet, M, LockData(Loc, LK_Exclusive, isScopedVar)); + for (const auto &M : SharedLocksToAdd) + Analyzer->addLock(FSet, M, LockData(Loc, LK_Shared, isScopedVar)); // Add the managing object as a dummy mutex, mapped to the underlying mutex. // FIXME -- this doesn't work if we acquire multiple locks. @@ -2030,22 +2037,21 @@ void BuildLockset::handleCall(Expr *Exp, const NamedDecl *D, VarDecl *VD) { DeclRefExpr DRE(VD, false, VD->getType(), VK_LValue, VD->getLocation()); SExpr SMutex(&DRE, 0, 0); - for (unsigned i=0,n=ExclusiveLocksToAdd.size(); iaddLock(FSet, SMutex, LockData(MLoc, LK_Exclusive, - ExclusiveLocksToAdd[i])); - } - for (unsigned i=0,n=SharedLocksToAdd.size(); iaddLock(FSet, SMutex, LockData(MLoc, LK_Shared, - SharedLocksToAdd[i])); - } + for (const auto &M : ExclusiveLocksToAdd) + Analyzer->addLock(FSet, SMutex, LockData(MLoc, LK_Exclusive, M)); + for (const auto &M : SharedLocksToAdd) + Analyzer->addLock(FSet, SMutex, LockData(MLoc, LK_Shared, M)); } // Remove locks. // FIXME -- should only fully remove if the attribute refers to 'this'. bool Dtor = isa(D); - for (unsigned i=0,n=LocksToRemove.size(); iremoveLock(FSet, LocksToRemove[i], Loc, Dtor); - } + for (const auto &M : ExclusiveLocksToRemove) + Analyzer->removeLock(FSet, M, Loc, Dtor, LK_Exclusive); + for (const auto &M : SharedLocksToRemove) + Analyzer->removeLock(FSet, M, Loc, Dtor, LK_Shared); + for (const auto &M : GenericLocksToRemove) + Analyzer->removeLock(FSet, M, Loc, Dtor, LK_Generic); } diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp index ecf6d51ef1c8..efaf966562cf 100644 --- a/clang/lib/Sema/AnalysisBasedWarnings.cpp +++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp @@ -1452,7 +1452,15 @@ class ThreadSafetyReporter : public clang::thread_safety::ThreadSafetyHandler { void handleUnmatchedUnlock(Name LockName, SourceLocation Loc) override { warnLockMismatch(diag::warn_unlock_but_no_lock, LockName, Loc); } - + void handleIncorrectUnlockKind(Name LockName, LockKind Expected, + LockKind Received, + SourceLocation Loc) override { + if (Loc.isInvalid()) + Loc = FunLocation; + PartialDiagnosticAt Warning(Loc, S.PDiag(diag::warn_unlock_kind_mismatch) + << LockName << Received << Expected); + Warnings.push_back(DelayedDiag(Warning, OptionalNotes())); + } void handleDoubleLock(Name LockName, SourceLocation Loc) override { warnLockMismatch(diag::warn_double_lock, LockName, Loc); } diff --git a/clang/test/Sema/warn-thread-safety-analysis.c b/clang/test/Sema/warn-thread-safety-analysis.c index 1796f74caab3..62460f99fa61 100644 --- a/clang/test/Sema/warn-thread-safety-analysis.c +++ b/clang/test/Sema/warn-thread-safety-analysis.c @@ -35,6 +35,8 @@ struct Foo { void mutex_exclusive_lock(struct Mutex *mu) EXCLUSIVE_LOCK_FUNCTION(mu); void mutex_shared_lock(struct Mutex *mu) SHARED_LOCK_FUNCTION(mu); void mutex_unlock(struct Mutex *mu) UNLOCK_FUNCTION(mu); +void mutex_shared_unlock(struct Mutex *mu) __attribute__((release_shared_capability(mu))); +void mutex_exclusive_unlock(struct Mutex *mu) __attribute__((release_capability(mu))); // Define global variables. struct Mutex mu1; @@ -114,5 +116,13 @@ int main() { (void)(*d_ == 1); mutex_unlock(foo_.mu_); + mutex_exclusive_lock(&mu1); + mutex_shared_unlock(&mu1); // expected-warning {{unlocking 'mu1' using shared access, expected exclusive access}} + mutex_exclusive_unlock(&mu1); + + mutex_shared_lock(&mu1); + mutex_exclusive_unlock(&mu1); // expected-warning {{unlocking 'mu1' using exclusive access, expected shared access}} + mutex_shared_unlock(&mu1); + return 0; }