From eb0ea5f40a48c488cb8973490c41713eb202904d Mon Sep 17 00:00:00 2001 From: DeLesley Hutchins Date: Thu, 14 Aug 2014 21:40:15 +0000 Subject: [PATCH] Thread safety analysis: add -Wthread-safety-verbose flag, which adds additional notes that are helpful when compiling statistics on thread safety warnings. llvm-svn: 215677 --- .../clang/Analysis/Analyses/ThreadSafety.h | 10 +++ clang/include/clang/Basic/DiagnosticGroups.td | 1 + .../clang/Basic/DiagnosticSemaKinds.td | 10 ++- clang/lib/Analysis/ThreadSafety.cpp | 5 ++ clang/lib/Sema/AnalysisBasedWarnings.cpp | 72 +++++++++++++--- .../SemaCXX/warn-thread-safety-verbose.cpp | 86 +++++++++++++++++++ 6 files changed, 170 insertions(+), 14 deletions(-) create mode 100644 clang/test/SemaCXX/warn-thread-safety-verbose.cpp diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafety.h b/clang/include/clang/Analysis/Analyses/ThreadSafety.h index 08ba97545391..38600912dae8 100644 --- a/clang/include/clang/Analysis/Analyses/ThreadSafety.h +++ b/clang/include/clang/Analysis/Analyses/ThreadSafety.h @@ -181,6 +181,16 @@ public: virtual void handleFunExcludesLock(StringRef Kind, Name FunName, Name LockName, SourceLocation Loc) {} + /// Called by the analysis when starting analysis of a function. + /// Used to issue suggestions for changes to annotations. + virtual void enterFunction(const FunctionDecl *FD) {} + + /// Called by the analysis when finishing analysis of a function. + virtual void leaveFunction(const FunctionDecl *FD) {} + + /// Return the number of errors found within this function so far. + virtual int numErrors() { return 0; } + bool issueBetaWarnings() { return IssueBetaWarnings; } void setIssueBetaWarnings(bool b) { IssueBetaWarnings = b; } diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td index c737270cae3f..13981a545dad 100644 --- a/clang/include/clang/Basic/DiagnosticGroups.td +++ b/clang/include/clang/Basic/DiagnosticGroups.td @@ -596,6 +596,7 @@ def ThreadSafety : DiagGroup<"thread-safety", ThreadSafetyAnalysis, ThreadSafetyPrecise, ThreadSafetyNegative]>; +def ThreadSafetyVerbose : DiagGroup<"thread-safety-verbose">; 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 ca37b076fe56..a1b2bc6389c7 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -2349,9 +2349,15 @@ def warn_fun_requires_lock_precise : InGroup, DefaultIgnore; def note_found_mutex_near_match : Note<"found near match '%0'">; +// Verbose thread safety warnings +def warn_thread_safety_verbose : Warning<"Thread safety verbose warning.">, + InGroup, DefaultIgnore; +def note_thread_warning_in_fun : Note<"Thread warning in function '%0'">; +def note_guarded_by_declared_here : Note<"Guarded_by declared here.">; + // Dummy warning that will trigger "beta" warnings from the analysis if enabled. -def warn_thread_safety_beta : Warning< - "Thread safety beta warning.">, InGroup, DefaultIgnore; +def warn_thread_safety_beta : Warning<"Thread safety beta warning.">, + InGroup, DefaultIgnore; // Consumed warnings def warn_use_in_invalid_state : Warning< diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp index c49e6e7049bf..469e79be7242 100644 --- a/clang/lib/Analysis/ThreadSafety.cpp +++ b/clang/lib/Analysis/ThreadSafety.cpp @@ -1810,6 +1810,7 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) { CFG *CFGraph = walker.getGraph(); const NamedDecl *D = walker.getDecl(); + const FunctionDecl *CurrentFunction = dyn_cast(D); CurrentMethod = dyn_cast(D); if (D->hasAttr()) @@ -1824,6 +1825,8 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) { if (isa(D)) return; // Don't check inside destructors. + Handler.enterFunction(CurrentFunction); + BlockInfo.resize(CFGraph->getNumBlockIDs(), CFGBlockInfo::getEmptyBlockInfo(LocalVarMap)); @@ -2079,6 +2082,8 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) { LEK_LockedAtEndOfFunction, LEK_NotLockedAtEndOfFunction, false); + + Handler.leaveFunction(CurrentFunction); } diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp index 59d13de8309a..93ee16b4c52d 100644 --- a/clang/lib/Sema/AnalysisBasedWarnings.cpp +++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp @@ -1451,6 +1451,30 @@ class ThreadSafetyReporter : public clang::threadSafety::ThreadSafetyHandler { DiagList Warnings; SourceLocation FunLocation, FunEndLocation; + const FunctionDecl *CurrentFunction; + bool Verbose; + + OptionalNotes getNotes() { + if (Verbose && CurrentFunction) { + PartialDiagnosticAt FNote(CurrentFunction->getBody()->getLocStart(), + S.PDiag(diag::note_thread_warning_in_fun) + << CurrentFunction->getNameAsString()); + return OptionalNotes(1, FNote); + } + else return OptionalNotes(); + } + + OptionalNotes getNotes(const PartialDiagnosticAt &Note) { + OptionalNotes ONS(1, Note); + if (Verbose && CurrentFunction) { + PartialDiagnosticAt FNote(CurrentFunction->getBody()->getLocStart(), + S.PDiag(diag::note_thread_warning_in_fun) + << CurrentFunction->getNameAsString()); + ONS.push_back(FNote); + } + return ONS; + } + // Helper functions void warnLockMismatch(unsigned DiagID, StringRef Kind, Name LockName, SourceLocation Loc) { @@ -1459,12 +1483,15 @@ class ThreadSafetyReporter : public clang::threadSafety::ThreadSafetyHandler { if (!Loc.isValid()) Loc = FunLocation; PartialDiagnosticAt Warning(Loc, S.PDiag(DiagID) << Kind << LockName); - Warnings.push_back(DelayedDiag(Warning, OptionalNotes())); + Warnings.push_back(DelayedDiag(Warning, getNotes())); } public: ThreadSafetyReporter(Sema &S, SourceLocation FL, SourceLocation FEL) - : S(S), FunLocation(FL), FunEndLocation(FEL) {} + : S(S), FunLocation(FL), FunEndLocation(FEL), + CurrentFunction(nullptr), Verbose(false) {} + + void setVerbose(bool b) { Verbose = b; } /// \brief Emit all buffered diagnostics in order of sourcelocation. /// We need to output diagnostics produced while iterating through @@ -1482,12 +1509,14 @@ class ThreadSafetyReporter : public clang::threadSafety::ThreadSafetyHandler { void handleInvalidLockExp(StringRef Kind, SourceLocation Loc) override { PartialDiagnosticAt Warning(Loc, S.PDiag(diag::warn_cannot_resolve_lock) << Loc); - Warnings.push_back(DelayedDiag(Warning, OptionalNotes())); + Warnings.push_back(DelayedDiag(Warning, getNotes())); } + void handleUnmatchedUnlock(StringRef Kind, Name LockName, SourceLocation Loc) override { warnLockMismatch(diag::warn_unlock_but_no_lock, Kind, LockName, Loc); } + void handleIncorrectUnlockKind(StringRef Kind, Name LockName, LockKind Expected, LockKind Received, SourceLocation Loc) override { @@ -1496,8 +1525,9 @@ class ThreadSafetyReporter : public clang::threadSafety::ThreadSafetyHandler { PartialDiagnosticAt Warning(Loc, S.PDiag(diag::warn_unlock_kind_mismatch) << Kind << LockName << Received << Expected); - Warnings.push_back(DelayedDiag(Warning, OptionalNotes())); + Warnings.push_back(DelayedDiag(Warning, getNotes())); } + void handleDoubleLock(StringRef Kind, Name LockName, SourceLocation Loc) override { warnLockMismatch(diag::warn_double_lock, Kind, LockName, Loc); } @@ -1529,10 +1559,10 @@ class ThreadSafetyReporter : public clang::threadSafety::ThreadSafetyHandler { if (LocLocked.isValid()) { PartialDiagnosticAt Note(LocLocked, S.PDiag(diag::note_locked_here) << Kind); - Warnings.push_back(DelayedDiag(Warning, OptionalNotes(1, Note))); + Warnings.push_back(DelayedDiag(Warning, getNotes(Note))); return; } - Warnings.push_back(DelayedDiag(Warning, OptionalNotes())); + Warnings.push_back(DelayedDiag(Warning, getNotes())); } void handleExclusiveAndShared(StringRef Kind, Name LockName, @@ -1543,7 +1573,7 @@ class ThreadSafetyReporter : public clang::threadSafety::ThreadSafetyHandler { << Kind << LockName); PartialDiagnosticAt Note(Loc2, S.PDiag(diag::note_lock_exclusive_and_shared) << Kind << LockName); - Warnings.push_back(DelayedDiag(Warning, OptionalNotes(1, Note))); + Warnings.push_back(DelayedDiag(Warning, getNotes(Note))); } void handleNoMutexHeld(StringRef Kind, const NamedDecl *D, @@ -1556,7 +1586,7 @@ class ThreadSafetyReporter : public clang::threadSafety::ThreadSafetyHandler { diag::warn_var_deref_requires_any_lock; PartialDiagnosticAt Warning(Loc, S.PDiag(DiagID) << D->getNameAsString() << getLockKindFromAccessKind(AK)); - Warnings.push_back(DelayedDiag(Warning, OptionalNotes())); + Warnings.push_back(DelayedDiag(Warning, getNotes())); } void handleMutexNotHeld(StringRef Kind, const NamedDecl *D, @@ -1581,7 +1611,7 @@ class ThreadSafetyReporter : public clang::threadSafety::ThreadSafetyHandler { << LockName << LK); PartialDiagnosticAt Note(Loc, S.PDiag(diag::note_found_mutex_near_match) << *PossibleMatch); - Warnings.push_back(DelayedDiag(Warning, OptionalNotes(1, Note))); + Warnings.push_back(DelayedDiag(Warning, getNotes(Note))); } else { switch (POK) { case POK_VarAccess: @@ -1597,7 +1627,15 @@ class ThreadSafetyReporter : public clang::threadSafety::ThreadSafetyHandler { PartialDiagnosticAt Warning(Loc, S.PDiag(DiagID) << Kind << D->getNameAsString() << LockName << LK); - Warnings.push_back(DelayedDiag(Warning, OptionalNotes())); + if (Verbose && POK == POK_VarAccess) { + PartialDiagnosticAt Note(D->getLocation(), + S.PDiag(diag::note_guarded_by_declared_here) << + D->getNameAsString()); + Warnings.push_back(DelayedDiag(Warning, getNotes(Note))); + } + else { + Warnings.push_back(DelayedDiag(Warning, getNotes())); + } } } @@ -1606,7 +1644,7 @@ class ThreadSafetyReporter : public clang::threadSafety::ThreadSafetyHandler { PartialDiagnosticAt Warning(Loc, S.PDiag(diag::warn_acquire_requires_negative_cap) << Kind << LockName << Neg); - Warnings.push_back(DelayedDiag(Warning, OptionalNotes())); + Warnings.push_back(DelayedDiag(Warning, getNotes())); } @@ -1614,7 +1652,15 @@ class ThreadSafetyReporter : public clang::threadSafety::ThreadSafetyHandler { SourceLocation Loc) override { PartialDiagnosticAt Warning(Loc, S.PDiag(diag::warn_fun_excludes_mutex) << Kind << FunName << LockName); - Warnings.push_back(DelayedDiag(Warning, OptionalNotes())); + Warnings.push_back(DelayedDiag(Warning, getNotes())); + } + + void enterFunction(const FunctionDecl* FD) override { + CurrentFunction = FD; + } + + void leaveFunction(const FunctionDecl* FD) override { + CurrentFunction = 0; } }; @@ -1908,6 +1954,8 @@ AnalysisBasedWarnings::IssueWarnings(sema::AnalysisBasedWarnings::Policy P, threadSafety::ThreadSafetyReporter Reporter(S, FL, FEL); if (!Diags.isIgnored(diag::warn_thread_safety_beta, D->getLocStart())) Reporter.setIssueBetaWarnings(true); + if (!Diags.isIgnored(diag::warn_thread_safety_verbose, D->getLocStart())) + Reporter.setVerbose(true); threadSafety::runThreadSafetyAnalysis(AC, Reporter); Reporter.emitDiagnostics(); diff --git a/clang/test/SemaCXX/warn-thread-safety-verbose.cpp b/clang/test/SemaCXX/warn-thread-safety-verbose.cpp new file mode 100644 index 000000000000..4e19f30bf2c3 --- /dev/null +++ b/clang/test/SemaCXX/warn-thread-safety-verbose.cpp @@ -0,0 +1,86 @@ +// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 -Wthread-safety -Wthread-safety-beta -Wthread-safety-verbose -Wno-thread-safety-negative -fcxx-exceptions %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(); +}; + + +class Test { + Mutex mu; + int a GUARDED_BY(mu); // expected-note3 {{Guarded_by declared here.}} + + void foo1() EXCLUSIVE_LOCKS_REQUIRED(mu); + void foo2() SHARED_LOCKS_REQUIRED(mu); + void foo3() LOCKS_EXCLUDED(mu); + + void test1() { // expected-note {{Thread warning in function 'test1'}} + a = 0; // expected-warning {{writing variable 'a' requires holding mutex 'mu' exclusively}} + } + + void test2() { // expected-note {{Thread warning in function 'test2'}} + int b = a; // expected-warning {{reading variable 'a' requires holding mutex 'mu'}} + } + + void test3() { // expected-note {{Thread warning in function 'test3'}} + foo1(); // expected-warning {{calling function 'foo1' requires holding mutex 'mu' exclusively}} + } + + void test4() { // expected-note {{Thread warning in function 'test4'}} + foo2(); // expected-warning {{calling function 'foo2' requires holding mutex 'mu'}} + } + + void test5() { // expected-note {{Thread warning in function 'test5'}} + mu.ReaderLock(); + foo1(); // expected-warning {{calling function 'foo1' requires holding mutex 'mu' exclusively}} + mu.Unlock(); + } + + void test6() { // expected-note {{Thread warning in function 'test6'}} + mu.ReaderLock(); + a = 0; // expected-warning {{writing variable 'a' requires holding mutex 'mu' exclusively}} + mu.Unlock(); + } + + void test7() { // expected-note {{Thread warning in function 'test7'}} + mu.Lock(); + foo3(); // expected-warning {{cannot call function 'foo3' while mutex 'mu' is held}} + mu.Unlock(); + } +}; +