From 3efd0495a0813896e1559e532c5d9e581dd48c0e Mon Sep 17 00:00:00 2001 From: DeLesley Hutchins Date: Mon, 4 Aug 2014 22:13:06 +0000 Subject: [PATCH] Thread Safety Analysis: add a -Wthread-safety-negative flag that warns whenever a mutex is acquired, but corresponding mutex is not provably not-held. This is based on the earlier negative requirements patch. llvm-svn: 214789 --- .../include/clang/Analysis/Analyses/ThreadSafety.h | 8 ++ clang/include/clang/Basic/DiagnosticGroups.td | 4 +- clang/include/clang/Basic/DiagnosticSemaKinds.td | 5 + clang/lib/Analysis/ThreadSafety.cpp | 102 +++++++++++++------- clang/lib/Sema/AnalysisBasedWarnings.cpp | 9 ++ clang/test/Sema/warn-thread-safety-analysis.c | 6 +- clang/test/SemaCXX/warn-thread-safety-analysis.cpp | 29 +++++- clang/test/SemaCXX/warn-thread-safety-negative.cpp | 104 +++++++++++++++++++++ 8 files changed, 228 insertions(+), 39 deletions(-) create mode 100644 clang/test/SemaCXX/warn-thread-safety-negative.cpp diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafety.h b/clang/include/clang/Analysis/Analyses/ThreadSafety.h index bc22c4d..153e83e 100644 --- a/clang/include/clang/Analysis/Analyses/ThreadSafety.h +++ b/clang/include/clang/Analysis/Analyses/ThreadSafety.h @@ -161,6 +161,14 @@ public: LockKind LK, SourceLocation Loc, Name *PossibleMatch = nullptr) {} + /// Warn when acquiring a lock that the negative capability is not held. + /// \param Kind -- the capability's name parameter (role, mutex, etc). + /// \param LockName -- A StringRef name for the lock expression, to be printed + /// in the error message. + /// \param Loc -- The location of the protected operation. + virtual void handleNegativeNotHeld(StringRef Kind, Name LockName, Name Neg, + SourceLocation Loc) {} + /// Warn when a function is called while an excluded mutex is locked. For /// example, the mutex may be locked inside the function. /// \param Kind -- the capability's name parameter (role, mutex, etc). diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td index 6d77707..8beff3e 100644 --- a/clang/include/clang/Basic/DiagnosticGroups.td +++ b/clang/include/clang/Basic/DiagnosticGroups.td @@ -589,10 +589,12 @@ def Most : DiagGroup<"most", [ def ThreadSafetyAttributes : DiagGroup<"thread-safety-attributes">; def ThreadSafetyAnalysis : DiagGroup<"thread-safety-analysis">; def ThreadSafetyPrecise : DiagGroup<"thread-safety-precise">; +def ThreadSafetyNegative : DiagGroup<"thread-safety-negative">; def ThreadSafety : DiagGroup<"thread-safety", [ThreadSafetyAttributes, ThreadSafetyAnalysis, - ThreadSafetyPrecise]>; + ThreadSafetyPrecise, + ThreadSafetyNegative]>; def ThreadSafetyBeta : DiagGroup<"thread-safety-beta">; // Uniqueness Analysis warnings diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td index 2414531..e4cdbd4 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -2317,6 +2317,11 @@ def warn_cannot_resolve_lock : Warning< "cannot resolve lock expression">, InGroup, DefaultIgnore; +// Thread safety warnings negative capabilities +def warn_acquire_requires_negative_cap : Warning< + "acquiring %0 '%1' requires negative capability '%2'">, + InGroup, DefaultIgnore; + // Imprecise thread safety warnings def warn_variable_requires_lock : Warning< "%select{reading|writing}3 variable '%1' requires holding %0 " diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp index e43297a..d233b1b 100644 --- a/clang/lib/Analysis/ThreadSafety.cpp +++ b/clang/lib/Analysis/ThreadSafety.cpp @@ -173,6 +173,17 @@ public: bool isEmpty() const { return FactIDs.size() == 0; } + // Return true if the set contains only negative facts + bool isEmpty(FactManager &FactMan) const { + for (FactID FID : *this) { + if (!FactMan[FID].negative()) + return false; + } + return true; + } + + void addLockByID(FactID ID) { FactIDs.push_back(ID); } + FactID addLock(FactManager& FM, const FactEntry &Entry) { FactID F = FM.newFact(Entry); FactIDs.push_back(F); @@ -765,7 +776,10 @@ public: ThreadSafetyAnalyzer(ThreadSafetyHandler &H) : Arena(&Bpa), SxBuilder(Arena), Handler(H) {} - void addLock(FactSet &FSet, const FactEntry &Entry, StringRef DiagKind); + bool inCurrentScope(const CapabilityExpr &CapE); + + void addLock(FactSet &FSet, const FactEntry &Entry, StringRef DiagKind, + bool ReqAttr = false); void removeLock(FactSet &FSet, const CapabilityExpr &CapE, SourceLocation UnlockLoc, bool FullyRemove, LockKind Kind, StringRef DiagKind); @@ -879,20 +893,47 @@ ClassifyDiagnostic(const AttrTy *A) { return "mutex"; } + +inline bool ThreadSafetyAnalyzer::inCurrentScope(const CapabilityExpr &CapE) { + if (!CurrentMethod) + return false; + if (auto *P = dyn_cast_or_null(CapE.sexpr())) { + auto *VD = P->clangDecl(); + if (VD) + return VD->getDeclContext() == CurrentMethod->getDeclContext(); + } + return false; +} + + /// \brief Add a new lock to the lockset, warning if the lock is already there. -/// \param Mutex -- the Mutex expression for the lock -/// \param LDat -- the LockData for the lock +/// \param Mutex -- the Mutex expression for the lock +/// \param LDat -- the LockData for the lock +/// \param ReqAttr -- true if this is part of an initial Requires attribute. void ThreadSafetyAnalyzer::addLock(FactSet &FSet, const FactEntry &Entry, - StringRef DiagKind) { + StringRef DiagKind, bool ReqAttr) { if (Entry.shouldIgnore()) return; - + if (!ReqAttr && !Entry.negative()) { + // look for the negative capability, and remove it from the fact set. + CapabilityExpr NegC = !Entry; + FactEntry *Nen = FSet.findLock(FactMan, NegC); + if (Nen) { + FSet.removeLock(FactMan, NegC); + } + else { + if (inCurrentScope(Entry) && !Entry.asserted()) + Handler.handleNegativeNotHeld(DiagKind, Entry.toString(), + NegC.toString(), Entry.loc()); + } + } // FIXME: deal with acquired before/after annotations. // FIXME: Don't always warn when we have support for reentrant locks. - if (!Entry.asserted() && FSet.findLock(FactMan, Entry)) { - Handler.handleDoubleLock(DiagKind, Entry.toString(), Entry.loc()); + if (FSet.findLock(FactMan, Entry)) { + if (!Entry.asserted()) + Handler.handleDoubleLock(DiagKind, Entry.toString(), Entry.loc()); } else { FSet.addLock(FactMan, Entry); } @@ -920,18 +961,22 @@ void ThreadSafetyAnalyzer::removeLock(FactSet &FSet, const CapabilityExpr &Cp, if (ReceivedKind != LK_Generic && LDat->kind() != ReceivedKind) { Handler.handleIncorrectUnlockKind(DiagKind, Cp.toString(), LDat->kind(), ReceivedKind, UnlockLoc); - return; } if (LDat->underlying()) { + assert(!Cp.negative() && "Managing object cannot be negative."); CapabilityExpr UnderCp(LDat->underlying(), false); + FactEntry UnderEntry(!UnderCp, LK_Exclusive, UnlockLoc); // This is scoped lockable object, which manages the real mutex. if (FullyRemove) { // We're destroying the managing object. // Remove the underlying mutex if it exists; but don't warn. - if (FSet.findLock(FactMan, UnderCp)) + if (FSet.findLock(FactMan, UnderCp)) { FSet.removeLock(FactMan, UnderCp); + FSet.addLock(FactMan, UnderEntry); + } + FSet.removeLock(FactMan, Cp); } else { // We're releasing the underlying mutex, but not destroying the // managing object. Warn on dual release. @@ -939,10 +984,16 @@ void ThreadSafetyAnalyzer::removeLock(FactSet &FSet, const CapabilityExpr &Cp, Handler.handleUnmatchedUnlock(DiagKind, UnderCp.toString(), UnlockLoc); } FSet.removeLock(FactMan, UnderCp); - return; + FSet.addLock(FactMan, UnderEntry); } + return; } + // else !LDat->underlying() + FSet.removeLock(FactMan, Cp); + if (!Cp.negative()) { + FSet.addLock(FactMan, FactEntry(!Cp, LK_Exclusive, UnlockLoc)); + } } @@ -1164,9 +1215,7 @@ class BuildLockset : public StmtVisitor { LocalVariableMap::Context LVarCtx; unsigned CtxIndex; - // Helper functions - bool inCurrentScope(const CapabilityExpr &CapE); - + // helper functions void warnIfMutexNotHeld(const NamedDecl *D, const Expr *Exp, AccessKind AK, Expr *MutexExp, ProtectedOperationKind POK, StringRef DiagKind); @@ -1196,18 +1245,6 @@ public: }; -inline bool BuildLockset::inCurrentScope(const CapabilityExpr &CapE) { - if (!Analyzer->CurrentMethod) - return false; - if (auto *P = dyn_cast_or_null(CapE.sexpr())) { - auto *VD = P->clangDecl(); - if (VD) - return VD->getDeclContext() == Analyzer->CurrentMethod->getDeclContext(); - } - return false; -} - - /// \brief Warn if the LSet does not contain a lock sufficient to protect access /// of at least the passed in AccessKind. void BuildLockset::warnIfMutexNotHeld(const NamedDecl *D, const Expr *Exp, @@ -1235,7 +1272,7 @@ void BuildLockset::warnIfMutexNotHeld(const NamedDecl *D, const Expr *Exp, // If this does not refer to a negative capability in the same class, // then stop here. - if (!inCurrentScope(Cp)) + if (!Analyzer->inCurrentScope(Cp)) return; // Otherwise the negative requirement must be propagated to the caller. @@ -1326,9 +1363,10 @@ void BuildLockset::checkAccess(const Expr *Exp, AccessKind AK) { if (!D || !D->hasAttrs()) return; - if (D->hasAttr() && FSet.isEmpty()) + if (D->hasAttr() && FSet.isEmpty(Analyzer->FactMan)) { Analyzer->Handler.handleNoMutexHeld("mutex", D, POK_VarAccess, AK, Exp->getExprLoc()); + } for (const auto *I : D->specific_attrs()) warnIfMutexNotHeld(D, Exp, AK, I->getArg(), POK_VarAccess, @@ -1359,7 +1397,7 @@ void BuildLockset::checkPtAccess(const Expr *Exp, AccessKind AK) { if (!D || !D->hasAttrs()) return; - if (D->hasAttr() && FSet.isEmpty()) + if (D->hasAttr() && FSet.isEmpty(Analyzer->FactMan)) Analyzer->Handler.handleNoMutexHeld("mutex", D, POK_VarDereference, AK, Exp->getExprLoc()); @@ -1692,7 +1730,7 @@ void ThreadSafetyAnalyzer::intersectAndWarn(FactSet &FSet1, } } else if (!LDat2->managed() && !LDat2->asserted() && - !LDat2->isUniversal()) { + !LDat2->negative() && !LDat2->isUniversal()) { Handler.handleMutexHeldEndOfScope("mutex", LDat2->toString(), LDat2->loc(), JoinLoc, LEK1); } @@ -1717,7 +1755,7 @@ void ThreadSafetyAnalyzer::intersectAndWarn(FactSet &FSet1, } } else if (!LDat1->managed() && !LDat1->asserted() && - !LDat1->isUniversal()) { + !LDat1->negative() && !LDat1->isUniversal()) { Handler.handleMutexHeldEndOfScope("mutex", LDat1->toString(), LDat1->loc(), JoinLoc, LEK2); } @@ -1844,10 +1882,10 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) { // FIXME -- Loc can be wrong here. for (const auto &Mu : ExclusiveLocksToAdd) addLock(InitialLockset, FactEntry(Mu, LK_Exclusive, Loc), - CapDiagKind); + CapDiagKind, true); for (const auto &Mu : SharedLocksToAdd) addLock(InitialLockset, FactEntry(Mu, LK_Shared, Loc), - CapDiagKind); + CapDiagKind, true); } for (const auto *CurrBlock : *SortedGraph) { diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp index d1d4c27..59d13de 100644 --- a/clang/lib/Sema/AnalysisBasedWarnings.cpp +++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp @@ -1601,6 +1601,15 @@ class ThreadSafetyReporter : public clang::threadSafety::ThreadSafetyHandler { } } + virtual void handleNegativeNotHeld(StringRef Kind, Name LockName, Name Neg, + SourceLocation Loc) { + PartialDiagnosticAt Warning(Loc, + S.PDiag(diag::warn_acquire_requires_negative_cap) + << Kind << LockName << Neg); + Warnings.push_back(DelayedDiag(Warning, OptionalNotes())); + } + + void handleFunExcludesLock(StringRef Kind, Name FunName, Name LockName, SourceLocation Loc) override { PartialDiagnosticAt Warning(Loc, S.PDiag(diag::warn_fun_excludes_mutex) diff --git a/clang/test/Sema/warn-thread-safety-analysis.c b/clang/test/Sema/warn-thread-safety-analysis.c index 6d41e40..55e6e70 100644 --- a/clang/test/Sema/warn-thread-safety-analysis.c +++ b/clang/test/Sema/warn-thread-safety-analysis.c @@ -117,12 +117,12 @@ int main() { mutex_unlock(foo_.mu_); mutex_exclusive_lock(&mu1); - mutex_shared_unlock(&mu1); // expected-warning {{releasing mutex 'mu1' using shared access, expected exclusive access}} - mutex_exclusive_unlock(&mu1); + mutex_shared_unlock(&mu1); // expected-warning {{releasing mutex 'mu1' using shared access, expected exclusive access}} + mutex_exclusive_unlock(&mu1); // expected-warning {{releasing mutex 'mu1' that was not held}} mutex_shared_lock(&mu1); mutex_exclusive_unlock(&mu1); // expected-warning {{releasing mutex 'mu1' using exclusive access, expected shared access}} - mutex_shared_unlock(&mu1); + mutex_shared_unlock(&mu1); // expected-warning {{releasing mutex 'mu1' that was not held}} return 0; } diff --git a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp index be3040e..61e92db 100644 --- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp +++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp @@ -1,4 +1,4 @@ -// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 -Wthread-safety -Wthread-safety-beta -fcxx-exceptions %s +// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 -Wthread-safety -Wthread-safety-beta -Wno-thread-safety-negative -fcxx-exceptions %s // FIXME: should also run %clang_cc1 -fsyntax-only -verify -Wthread-safety -std=c++11 -Wc++98-compat %s // FIXME: should also run %clang_cc1 -fsyntax-only -verify -Wthread-safety %s @@ -4549,7 +4549,11 @@ public: } void bar() { - baz(); // expected-warning {{calling function 'baz' requires holding '!mu'}} + bar2(); // expected-warning {{calling function 'bar2' requires holding '!mu'}} + } + + void bar2() EXCLUSIVE_LOCKS_REQUIRED(!mu) { + baz(); } void baz() EXCLUSIVE_LOCKS_REQUIRED(!mu) { @@ -4620,5 +4624,24 @@ int main(void) { return 0; } -} +} // end namespace NegativeThreadRoles + + +namespace AssertSharedExclusive { + +void doSomething(); + +class Foo { + Mutex mu; + int a GUARDED_BY(mu); + + void test() SHARED_LOCKS_REQUIRED(mu) { + mu.AssertHeld(); + if (a > 0) + doSomething(); + } +}; + +} // end namespace AssertSharedExclusive + diff --git a/clang/test/SemaCXX/warn-thread-safety-negative.cpp b/clang/test/SemaCXX/warn-thread-safety-negative.cpp new file mode 100644 index 0000000..2a725a1 --- /dev/null +++ b/clang/test/SemaCXX/warn-thread-safety-negative.cpp @@ -0,0 +1,104 @@ +// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 -Wthread-safety -Wthread-safety-beta -fcxx-exceptions %s + +// FIXME: should also run %clang_cc1 -fsyntax-only -verify -Wthread-safety -std=c++11 -Wc++98-compat %s +// FIXME: should also run %clang_cc1 -fsyntax-only -verify -Wthread-safety %s + +#define LOCKABLE __attribute__ ((lockable)) +#define SCOPED_LOCKABLE __attribute__ ((scoped_lockable)) +#define GUARDED_BY(x) __attribute__ ((guarded_by(x))) +#define GUARDED_VAR __attribute__ ((guarded_var)) +#define PT_GUARDED_BY(x) __attribute__ ((pt_guarded_by(x))) +#define PT_GUARDED_VAR __attribute__ ((pt_guarded_var)) +#define ACQUIRED_AFTER(...) __attribute__ ((acquired_after(__VA_ARGS__))) +#define ACQUIRED_BEFORE(...) __attribute__ ((acquired_before(__VA_ARGS__))) +#define EXCLUSIVE_LOCK_FUNCTION(...) __attribute__ ((exclusive_lock_function(__VA_ARGS__))) +#define SHARED_LOCK_FUNCTION(...) __attribute__ ((shared_lock_function(__VA_ARGS__))) +#define ASSERT_EXCLUSIVE_LOCK(...) __attribute__ ((assert_exclusive_lock(__VA_ARGS__))) +#define ASSERT_SHARED_LOCK(...) __attribute__ ((assert_shared_lock(__VA_ARGS__))) +#define EXCLUSIVE_TRYLOCK_FUNCTION(...) __attribute__ ((exclusive_trylock_function(__VA_ARGS__))) +#define SHARED_TRYLOCK_FUNCTION(...) __attribute__ ((shared_trylock_function(__VA_ARGS__))) +#define UNLOCK_FUNCTION(...) __attribute__ ((unlock_function(__VA_ARGS__))) +#define LOCK_RETURNED(x) __attribute__ ((lock_returned(x))) +#define LOCKS_EXCLUDED(...) __attribute__ ((locks_excluded(__VA_ARGS__))) +#define EXCLUSIVE_LOCKS_REQUIRED(...) \ + __attribute__ ((exclusive_locks_required(__VA_ARGS__))) +#define SHARED_LOCKS_REQUIRED(...) \ + __attribute__ ((shared_locks_required(__VA_ARGS__))) +#define NO_THREAD_SAFETY_ANALYSIS __attribute__ ((no_thread_safety_analysis)) + + +class __attribute__((lockable)) Mutex { + public: + void Lock() __attribute__((exclusive_lock_function)); + void ReaderLock() __attribute__((shared_lock_function)); + void Unlock() __attribute__((unlock_function)); + bool TryLock() __attribute__((exclusive_trylock_function(true))); + bool ReaderTryLock() __attribute__((shared_trylock_function(true))); + void LockWhen(const int &cond) __attribute__((exclusive_lock_function)); + + // for negative capabilities + const Mutex& operator!() const { return *this; } + + void AssertHeld() ASSERT_EXCLUSIVE_LOCK(); + void AssertReaderHeld() ASSERT_SHARED_LOCK(); +}; + + +namespace SimpleTest { + +class Bar { + Mutex mu; + int a GUARDED_BY(mu); + +public: + void baz() EXCLUSIVE_LOCKS_REQUIRED(!mu) { + mu.Lock(); + a = 0; + mu.Unlock(); + } +}; + + +class Foo { + Mutex mu; + int a GUARDED_BY(mu); + +public: + void foo() { + mu.Lock(); // expected-warning {{acquiring mutex 'mu' requires negative capability '!mu'}} + baz(); // expected-warning {{cannot call function 'baz' while mutex 'mu' is held}} + bar(); + mu.Unlock(); + } + + void bar() { + baz(); // expected-warning {{calling function 'baz' requires holding '!mu'}} + } + + void baz() EXCLUSIVE_LOCKS_REQUIRED(!mu) { + mu.Lock(); + a = 0; + mu.Unlock(); + } + + void test() { + Bar b; + b.baz(); // no warning -- in different class. + } + + void test2() { + mu.Lock(); // expected-warning {{acquiring mutex 'mu' requires negative capability '!mu'}} + a = 0; + mu.Unlock(); + baz(); // no warning -- !mu in set. + } + + void test3() EXCLUSIVE_LOCKS_REQUIRED(!mu) { + mu.Lock(); + a = 0; + mu.Unlock(); + baz(); // no warning -- !mu in set. + } +}; + +} // end namespace SimpleTest -- 2.7.4