From fd374bb3dd7c3f9b11973df235d7ed66f49f1326 Mon Sep 17 00:00:00 2001 From: DeLesley Hutchins Date: Mon, 8 Apr 2013 20:11:11 +0000 Subject: [PATCH] Thread safety analysis: turn on checking within lock and unlock functions. These checks are enabled with the -Wthread-safety-beta flag. llvm-svn: 179046 --- clang/lib/Analysis/ThreadSafety.cpp | 58 +++++++++++--- clang/lib/Sema/AnalysisBasedWarnings.cpp | 8 +- clang/test/SemaCXX/warn-thread-safety-analysis.cpp | 90 +++++++++++++++++++--- 3 files changed, 134 insertions(+), 22 deletions(-) diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp index 4fe342d..479d9a3 100644 --- a/clang/lib/Analysis/ThreadSafety.cpp +++ b/clang/lib/Analysis/ThreadSafety.cpp @@ -2279,6 +2279,10 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) { // Fill in source locations for all CFGBlocks. findBlockLocations(CFGraph, SortedGraph, BlockInfo); + MutexIDList ExclusiveLocksAcquired; + MutexIDList SharedLocksAcquired; + MutexIDList LocksReleased; + // Add locks from exclusive_locks_required and shared_locks_required // to initial lockset. Also turn off checking for lock and unlock functions. // FIXME: is there a more intelligent way to check lock/unlock functions? @@ -2300,15 +2304,30 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) { } else if (SharedLocksRequiredAttr *A = dyn_cast(Attr)) { getMutexIDs(SharedLocksToAdd, A, (Expr*) 0, D); - } else if (isa(Attr)) { - // Don't try to check unlock functions for now - return; - } else if (isa(Attr)) { - // Don't try to check lock functions for now - return; - } else if (isa(Attr)) { - // Don't try to check lock functions for now - return; + } else if (UnlockFunctionAttr *A = dyn_cast(Attr)) { + if (!Handler.issueBetaWarnings()) + return; + // UNLOCK_FUNCTION() is used to hide the underlying lock implementation. + // We must ignore such methods. + if (A->args_size() == 0) + return; + // FIXME -- deal with exclusive vs. shared unlock functions? + getMutexIDs(ExclusiveLocksToAdd, A, (Expr*) 0, D); + getMutexIDs(LocksReleased, A, (Expr*) 0, D); + } else if (ExclusiveLockFunctionAttr *A + = dyn_cast(Attr)) { + if (!Handler.issueBetaWarnings()) + return; + if (A->args_size() == 0) + return; + getMutexIDs(ExclusiveLocksAcquired, A, (Expr*) 0, D); + } else if (SharedLockFunctionAttr *A + = dyn_cast(Attr)) { + if (!Handler.issueBetaWarnings()) + return; + if (A->args_size() == 0) + return; + getMutexIDs(SharedLocksAcquired, A, (Expr*) 0, D); } else if (isa(Attr)) { // Don't try to check trylock functions for now return; @@ -2491,8 +2510,27 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) { if (!Final->Reachable) return; + // By default, we expect all locks held on entry to be held on exit. + FactSet ExpectedExitSet = Initial->EntrySet; + + // Adjust the expected exit set by adding or removing locks, as declared + // by *-LOCK_FUNCTION and UNLOCK_FUNCTION. The intersect below will then + // issue the appropriate warning. + // FIXME: the location here is not quite right. + for (unsigned i=0,n=ExclusiveLocksAcquired.size(); igetLocation(), LK_Exclusive)); + } + for (unsigned i=0,n=SharedLocksAcquired.size(); igetLocation(), LK_Shared)); + } + for (unsigned i=0,n=LocksReleased.size(); iEntrySet, Final->ExitSet, + intersectAndWarn(ExpectedExitSet, Final->ExitSet, Final->ExitLoc, LEK_LockedAtEndOfFunction, LEK_NotLockedAtEndOfFunction, diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp index 00d3c47..1295339 100644 --- a/clang/lib/Sema/AnalysisBasedWarnings.cpp +++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp @@ -1333,8 +1333,12 @@ class ThreadSafetyReporter : public clang::thread_safety::ThreadSafetyHandler { LocEndOfScope = FunEndLocation; PartialDiagnosticAt Warning(LocEndOfScope, S.PDiag(DiagID) << LockName); - PartialDiagnosticAt Note(LocLocked, S.PDiag(diag::note_locked_here)); - Warnings.push_back(DelayedDiag(Warning, OptionalNotes(1, Note))); + if (LocLocked.isValid()) { + PartialDiagnosticAt Note(LocLocked, S.PDiag(diag::note_locked_here)); + Warnings.push_back(DelayedDiag(Warning, OptionalNotes(1, Note))); + return; + } + Warnings.push_back(DelayedDiag(Warning, OptionalNotes())); } diff --git a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp index 3f41124d..bc4b40e 100644 --- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp +++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp @@ -1475,8 +1475,8 @@ namespace substitution_test { public: Mutex mu; - void lockData() __attribute__((exclusive_lock_function(mu))) { } - void unlockData() __attribute__((unlock_function(mu))) { } + void lockData() __attribute__((exclusive_lock_function(mu))); + void unlockData() __attribute__((unlock_function(mu))); void doSomething() __attribute__((exclusive_locks_required(mu))) { } }; @@ -1484,8 +1484,8 @@ namespace substitution_test { class DataLocker { public: - void lockData (MyData *d) __attribute__((exclusive_lock_function(d->mu))) { } - void unlockData(MyData *d) __attribute__((unlock_function(d->mu))) { } + void lockData (MyData *d) __attribute__((exclusive_lock_function(d->mu))); + void unlockData(MyData *d) __attribute__((unlock_function(d->mu))); }; @@ -2858,7 +2858,7 @@ void Foo::lock1() EXCLUSIVE_LOCK_FUNCTION(mu1_) { } void Foo::slock1() SHARED_LOCK_FUNCTION(mu1_) { - mu1_.Lock(); + mu1_.ReaderLock(); } void Foo::lock3() EXCLUSIVE_LOCK_FUNCTION(mu1_, mu2_, mu3_) { @@ -3640,8 +3640,8 @@ class Foo { LOCKS_EXCLUDED(mu2_); void lock() EXCLUSIVE_LOCK_FUNCTION(mu1_) EXCLUSIVE_LOCK_FUNCTION(mu2_); - void readerlock() EXCLUSIVE_LOCK_FUNCTION(mu1_) - EXCLUSIVE_LOCK_FUNCTION(mu2_); + void readerlock() SHARED_LOCK_FUNCTION(mu1_) + SHARED_LOCK_FUNCTION(mu2_); void unlock() UNLOCK_FUNCTION(mu1_) UNLOCK_FUNCTION(mu2_); bool trylock() EXCLUSIVE_TRYLOCK_FUNCTION(true, mu1_) @@ -3663,9 +3663,9 @@ void Foo::foo2() { } void Foo::foo3() { } -void Foo::lock() { } -void Foo::readerlock() { } -void Foo::unlock() { } +void Foo::lock() { mu1_.Lock(); mu2_.Lock(); } +void Foo::readerlock() { mu1_.ReaderLock(); mu2_.ReaderLock(); } +void Foo::unlock() { mu1_.Unlock(); mu2_.Unlock(); } bool Foo::trylock() { return true; } bool Foo::readertrylock() { return true; } @@ -3915,3 +3915,73 @@ void bar2() EXCLUSIVE_LOCKS_REQUIRED(getMutex1(), getMutex2()); } // end namespace UnevaluatedContextTest + +namespace LockUnlockFunctionTest { + +// Check built-in lock functions +class LOCKABLE MyLockable { +public: + void lock() EXCLUSIVE_LOCK_FUNCTION() { mu_.Lock(); } + void readerLock() SHARED_LOCK_FUNCTION() { mu_.ReaderLock(); } + void unlock() UNLOCK_FUNCTION() { mu_.Unlock(); } + +private: + Mutex mu_; +}; + + +class Foo { +public: + // Correct lock/unlock functions + void lock() EXCLUSIVE_LOCK_FUNCTION(mu_) { + mu_.Lock(); + } + + void readerLock() SHARED_LOCK_FUNCTION(mu_) { + mu_.ReaderLock(); + } + + void unlock() UNLOCK_FUNCTION(mu_) { + mu_.Unlock(); + } + + // Check failure to lock. + void lockBad() EXCLUSIVE_LOCK_FUNCTION(mu_) { // expected-note {{mutex acquired here}} + mu2_.Lock(); + mu2_.Unlock(); + } // expected-warning {{expecting mutex 'mu_' to be locked at the end of function}} + + void readerLockBad() SHARED_LOCK_FUNCTION(mu_) { // expected-note {{mutex acquired here}} + mu2_.Lock(); + mu2_.Unlock(); + } // expected-warning {{expecting mutex 'mu_' to be locked at the end of function}} + + void unlockBad() UNLOCK_FUNCTION(mu_) { // expected-note {{mutex acquired here}} + mu2_.Lock(); + mu2_.Unlock(); + } // expected-warning {{mutex 'mu_' is still locked at the end of function}} + + // Check locking the wrong thing. + void lockBad2() EXCLUSIVE_LOCK_FUNCTION(mu_) { // expected-note {{mutex acquired here}} + mu2_.Lock(); // expected-note {{mutex acquired here}} + } // expected-warning {{expecting mutex 'mu_' to be locked at the end of function}} \ + // expected-warning {{mutex 'mu2_' is still locked at the end of function}} + + + void readerLockBad2() SHARED_LOCK_FUNCTION(mu_) { // expected-note {{mutex acquired here}} + mu2_.ReaderLock(); // expected-note {{mutex acquired here}} + } // expected-warning {{expecting mutex 'mu_' to be locked at the end of function}} \ + // expected-warning {{mutex 'mu2_' is still locked at the end of function}} + + + void unlockBad2() UNLOCK_FUNCTION(mu_) { // expected-note {{mutex acquired here}} + mu2_.Unlock(); // expected-warning {{unlocking 'mu2_' that was not locked}} + } // expected-warning {{mutex 'mu_' is still locked at the end of function}} + +private: + Mutex mu_; + Mutex mu2_; +}; + +} // end namespace LockUnlockFunctionTest + -- 2.7.4