From 3f0ec5209726641782468bd4c7597e79dda78b15 Mon Sep 17 00:00:00 2001 From: DeLesley Hutchins Date: Mon, 10 Sep 2012 19:58:23 +0000 Subject: [PATCH] Thread-safety analysis: differentiate between two forms of analysis; a precise analysis that may give false positives because it is confused by aliasing, and a less precise analysis that has fewer false positives, but may have false negatives. The more precise warnings are enabled by -Wthread-safety-precise. An additional note clarify the warnings in the precise case. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@163537 91177308-0d34-0410-b5e6-96231b3b80d8 --- .../clang/Analysis/Analyses/ThreadSafety.h | 3 +- include/clang/Basic/DiagnosticGroups.td | 9 ++- include/clang/Basic/DiagnosticSemaKinds.td | 37 ++++++++---- lib/Analysis/ThreadSafety.cpp | 42 ++++++++++++- lib/Sema/AnalysisBasedWarnings.cpp | 48 ++++++++++----- test/SemaCXX/warn-thread-safety-analysis.cpp | 59 +++++++++++++------ 6 files changed, 148 insertions(+), 50 deletions(-) diff --git a/include/clang/Analysis/Analyses/ThreadSafety.h b/include/clang/Analysis/Analyses/ThreadSafety.h index 742cc04344..ef6b821e39 100644 --- a/include/clang/Analysis/Analyses/ThreadSafety.h +++ b/include/clang/Analysis/Analyses/ThreadSafety.h @@ -132,7 +132,8 @@ public: /// \param Loc -- The location of the protected operation. virtual void handleMutexNotHeld(const NamedDecl *D, ProtectedOperationKind POK, Name LockName, - LockKind LK, SourceLocation Loc) {} + LockKind LK, SourceLocation Loc, + Name *PossibleMatch=0) {} /// Warn when a function is called while an excluded mutex is locked. For /// example, the mutex may be locked inside the function. diff --git a/include/clang/Basic/DiagnosticGroups.td b/include/clang/Basic/DiagnosticGroups.td index de90f73ccb..7923db01f2 100644 --- a/include/clang/Basic/DiagnosticGroups.td +++ b/include/clang/Basic/DiagnosticGroups.td @@ -386,9 +386,12 @@ def Most : DiagGroup<"most", [ // Thread Safety warnings def ThreadSafetyAttributes : DiagGroup<"thread-safety-attributes">; -def ThreadSafetyAnalysis : DiagGroup<"thread-safety-analysis">; -def ThreadSafety : DiagGroup<"thread-safety", - [ThreadSafetyAttributes, ThreadSafetyAnalysis]>; +def ThreadSafetyAnalysis : DiagGroup<"thread-safety-analysis">; +def ThreadSafetyPrecise : DiagGroup<"thread-safety-precise">; +def ThreadSafety : DiagGroup<"thread-safety", + [ThreadSafetyAttributes, + ThreadSafetyAnalysis, + ThreadSafetyPrecise]>; // Note that putting warnings in -Wall will not disable them by default. If a // warning should be active _only_ when -Wall is passed in, mark it as diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td index 16afa972c5..e85f8359fe 100644 --- a/include/clang/Basic/DiagnosticSemaKinds.td +++ b/include/clang/Basic/DiagnosticSemaKinds.td @@ -1866,14 +1866,6 @@ def warn_lock_exclusive_and_shared : Warning< InGroup, DefaultIgnore; def note_lock_exclusive_and_shared : Note< "the other lock of mutex '%0' is here">; -def warn_variable_requires_lock : Warning< - "%select{reading|writing}2 variable '%0' requires locking " - "%select{'%1'|'%1' exclusively}2">, - InGroup, DefaultIgnore; -def warn_var_deref_requires_lock : Warning< - "%select{reading|writing}2 the value pointed to by '%0' requires locking " - "%select{'%1'|'%1' exclusively}2">, - InGroup, DefaultIgnore; def warn_variable_requires_any_lock : Warning< "%select{reading|writing}1 variable '%0' requires locking " "%select{any mutex|any mutex exclusively}1">, @@ -1882,9 +1874,6 @@ def warn_var_deref_requires_any_lock : Warning< "%select{reading|writing}1 the value pointed to by '%0' requires locking " "%select{any mutex|any mutex exclusively}1">, InGroup, DefaultIgnore; -def warn_fun_requires_lock : Warning< - "calling function '%0' requires %select{shared|exclusive}2 lock on '%1'">, - InGroup, DefaultIgnore; def warn_fun_excludes_mutex : Warning< "cannot call function '%0' while mutex '%1' is locked">, InGroup, DefaultIgnore; @@ -1892,6 +1881,32 @@ def warn_cannot_resolve_lock : Warning< "cannot resolve lock expression">, InGroup, DefaultIgnore; +// Imprecise thread safety warnings +def warn_variable_requires_lock : Warning< + "%select{reading|writing}2 variable '%0' requires locking " + "%select{'%1'|'%1' exclusively}2">, + InGroup, DefaultIgnore; +def warn_var_deref_requires_lock : Warning< + "%select{reading|writing}2 the value pointed to by '%0' requires locking " + "%select{'%1'|'%1' exclusively}2">, + InGroup, DefaultIgnore; +def warn_fun_requires_lock : Warning< + "calling function '%0' requires %select{shared|exclusive}2 lock on '%1'">, + InGroup, DefaultIgnore; + +// Precise thread safety warnings +def warn_variable_requires_lock_precise : Warning< + "%select{reading|writing}2 variable '%0' requires locking " + "%select{'%1'|'%1' exclusively}2">, + InGroup, DefaultIgnore; +def warn_var_deref_requires_lock_precise : Warning< + "%select{reading|writing}2 the value pointed to by '%0' requires locking " + "%select{'%1'|'%1' exclusively}2">, + InGroup, DefaultIgnore; +def warn_fun_requires_lock_precise : Warning< + "calling function '%0' requires %select{shared|exclusive}2 lock on '%1'">, + InGroup, DefaultIgnore; +def note_found_mutex_near_match : Note<"found near match '%0'">; def warn_impcast_vector_scalar : Warning< "implicit conversion turns vector to scalar: %0 to %1">, diff --git a/lib/Analysis/ThreadSafety.cpp b/lib/Analysis/ThreadSafety.cpp index 9c5fb1a145..8875943480 100644 --- a/lib/Analysis/ThreadSafety.cpp +++ b/lib/Analysis/ThreadSafety.cpp @@ -575,6 +575,15 @@ public: return false; } + // A partial match between a.mu and b.mu returns true a and b have the same + // type (and thus mu refers to the same mutex declaration), regardless of + // whether a and b are different objects or not. + bool partiallyMatches(const SExpr &Other) const { + if (NodeVec[0].kind() == EOP_Dot) + return NodeVec[0].matches(Other.NodeVec[0]); + return false; + } + /// \brief Pretty print a lock expression for use in error messages. std::string toString(unsigned i = 0) const { assert(isValid()); @@ -820,7 +829,7 @@ public: return false; } - LockData* findLock(FactManager& FM, const SExpr& M) const { + LockData* findLock(FactManager &FM, const SExpr &M) const { for (const_iterator I = begin(), E = end(); I != E; ++I) { const SExpr &Exp = FM[*I].MutID; if (Exp.matches(M)) @@ -829,7 +838,7 @@ public: return 0; } - LockData* findLockUniv(FactManager& FM, const SExpr& M) const { + LockData* findLockUniv(FactManager &FM, const SExpr &M) const { for (const_iterator I = begin(), E = end(); I != E; ++I) { const SExpr &Exp = FM[*I].MutID; if (Exp.matches(M) || Exp.isUniversal()) @@ -837,6 +846,14 @@ public: } return 0; } + + FactEntry* findPartialMatch(FactManager &FM, const SExpr &M) const { + for (const_iterator I=begin(), E=end(); I != E; ++I) { + const SExpr& Exp = FM[*I].MutID; + if (Exp.partiallyMatches(M)) return &FM[*I]; + } + return 0; + } }; @@ -1742,7 +1759,26 @@ void BuildLockset::warnIfMutexNotHeld(const NamedDecl *D, Expr *Exp, } LockData* LDat = FSet.findLockUniv(Analyzer->FactMan, Mutex); - if (!LDat || !LDat->isAtLeast(LK)) + bool NoError = true; + if (!LDat) { + // No exact match found. Look for a partial match. + FactEntry* FEntry = FSet.findPartialMatch(Analyzer->FactMan, Mutex); + if (FEntry) { + // Warn that there's no precise match. + LDat = &FEntry->LDat; + std::string PartMatchStr = FEntry->MutID.toString(); + StringRef PartMatchName(PartMatchStr); + Analyzer->Handler.handleMutexNotHeld(D, POK, Mutex.toString(), LK, + Exp->getExprLoc(), &PartMatchName); + } else { + // Warn that there's no match at all. + Analyzer->Handler.handleMutexNotHeld(D, POK, Mutex.toString(), LK, + Exp->getExprLoc()); + } + NoError = false; + } + // Make sure the mutex we found is the right kind. + if (NoError && LDat && !LDat->isAtLeast(LK)) Analyzer->Handler.handleMutexNotHeld(D, POK, Mutex.toString(), LK, Exp->getExprLoc()); } diff --git a/lib/Sema/AnalysisBasedWarnings.cpp b/lib/Sema/AnalysisBasedWarnings.cpp index de6710fbce..7455c75c2e 100644 --- a/lib/Sema/AnalysisBasedWarnings.cpp +++ b/lib/Sema/AnalysisBasedWarnings.cpp @@ -1089,22 +1089,42 @@ class ThreadSafetyReporter : public clang::thread_safety::ThreadSafetyHandler { } void handleMutexNotHeld(const NamedDecl *D, ProtectedOperationKind POK, - Name LockName, LockKind LK, SourceLocation Loc) { + Name LockName, LockKind LK, SourceLocation Loc, + Name *PossibleMatch) { unsigned DiagID = 0; - switch (POK) { - case POK_VarAccess: - DiagID = diag::warn_variable_requires_lock; - break; - case POK_VarDereference: - DiagID = diag::warn_var_deref_requires_lock; - break; - case POK_FunctionCall: - DiagID = diag::warn_fun_requires_lock; - break; + if (PossibleMatch) { + switch (POK) { + case POK_VarAccess: + DiagID = diag::warn_variable_requires_lock_precise; + break; + case POK_VarDereference: + DiagID = diag::warn_var_deref_requires_lock_precise; + break; + case POK_FunctionCall: + DiagID = diag::warn_fun_requires_lock_precise; + break; + } + PartialDiagnosticAt Warning(Loc, S.PDiag(DiagID) + << D->getName() << LockName << LK); + PartialDiagnosticAt Note(Loc, S.PDiag(diag::note_found_mutex_near_match) + << *PossibleMatch); + Warnings.push_back(DelayedDiag(Warning, OptionalNotes(1, Note))); + } else { + switch (POK) { + case POK_VarAccess: + DiagID = diag::warn_variable_requires_lock; + break; + case POK_VarDereference: + DiagID = diag::warn_var_deref_requires_lock; + break; + case POK_FunctionCall: + DiagID = diag::warn_fun_requires_lock; + break; + } + PartialDiagnosticAt Warning(Loc, S.PDiag(DiagID) + << D->getName() << LockName << LK); + Warnings.push_back(DelayedDiag(Warning, OptionalNotes())); } - PartialDiagnosticAt Warning(Loc, S.PDiag(DiagID) - << D->getName() << LockName << LK); - Warnings.push_back(DelayedDiag(Warning, OptionalNotes())); } void handleFunExcludesLock(Name FunName, Name LockName, SourceLocation Loc) { diff --git a/test/SemaCXX/warn-thread-safety-analysis.cpp b/test/SemaCXX/warn-thread-safety-analysis.cpp index 446c9099f1..7637b77405 100644 --- a/test/SemaCXX/warn-thread-safety-analysis.cpp +++ b/test/SemaCXX/warn-thread-safety-analysis.cpp @@ -533,7 +533,8 @@ void late_bad_0() { LateFoo fooB; fooA.mu.Lock(); fooB.a = 5; // \ - // expected-warning{{writing variable 'a' requires locking 'fooB.mu' exclusively}} + // expected-warning{{writing variable 'a' requires locking 'fooB.mu' exclusively}} \ + // expected-note{{found near match 'fooA.mu'}} fooA.mu.Unlock(); } @@ -553,7 +554,8 @@ void late_bad_2() { LateBar BarA; BarA.FooPointer->mu.Lock(); BarA.Foo.a = 2; // \ - // expected-warning{{writing variable 'a' requires locking 'BarA.Foo.mu' exclusively}} + // expected-warning{{writing variable 'a' requires locking 'BarA.Foo.mu' exclusively}} \ + // expected-note{{found near match 'BarA.FooPointer->mu'}} BarA.FooPointer->mu.Unlock(); } @@ -561,7 +563,8 @@ void late_bad_3() { LateBar BarA; BarA.Foo.mu.Lock(); BarA.FooPointer->a = 2; // \ - // expected-warning{{writing variable 'a' requires locking 'BarA.FooPointer->mu' exclusively}} + // expected-warning{{writing variable 'a' requires locking 'BarA.FooPointer->mu' exclusively}} \ + // expected-note{{found near match 'BarA.Foo.mu'}} BarA.Foo.mu.Unlock(); } @@ -569,7 +572,8 @@ void late_bad_4() { LateBar BarA; BarA.Foo.mu.Lock(); BarA.Foo2.a = 2; // \ - // expected-warning{{writing variable 'a' requires locking 'BarA.Foo2.mu' exclusively}} + // expected-warning{{writing variable 'a' requires locking 'BarA.Foo2.mu' exclusively}} \ + // expected-note{{found near match 'BarA.Foo.mu'}} BarA.Foo.mu.Unlock(); } @@ -1237,7 +1241,9 @@ void func() { b1->MyLock(); b1->a_ = 5; - b2->a_ = 3; // expected-warning {{writing variable 'a_' requires locking 'b2->mu1_' exclusively}} + b2->a_ = 3; // \ + // expected-warning {{writing variable 'a_' requires locking 'b2->mu1_' exclusively}} \ + // expected-note {{found near match 'b1->mu1_'}} b2->MyLock(); b2->MyUnlock(); b1->MyUnlock(); @@ -1267,11 +1273,13 @@ int func(int i) int x; b3->mu1_.Lock(); res = b1.a_ + b3->b_; // expected-warning {{reading variable 'a_' requires locking 'b1.mu1_'}} \ - // expected-warning {{writing variable 'res' requires locking 'mu' exclusively}} + // expected-warning {{writing variable 'res' requires locking 'mu' exclusively}} \ + // expected-note {{found near match 'b3->mu1_'}} *p = i; // expected-warning {{reading variable 'p' requires locking 'mu'}} \ // expected-warning {{writing the value pointed to by 'p' requires locking 'mu' exclusively}} b1.a_ = res + b3->b_; // expected-warning {{reading variable 'res' requires locking 'mu'}} \ - // expected-warning {{writing variable 'a_' requires locking 'b1.mu1_' exclusively}} + // expected-warning {{writing variable 'a_' requires locking 'b1.mu1_' exclusively}} \ + // expected-note {{found near match 'b3->mu1_'}} b3->b_ = *b1.q; // expected-warning {{reading the value pointed to by 'q' requires locking 'mu'}} b3->mu1_.Unlock(); b1.b_ = res; // expected-warning {{reading variable 'res' requires locking 'mu'}} @@ -1296,8 +1304,12 @@ class Foo { child->Func(new_foo); // There shouldn't be any warning here as the // acquired lock is not in child. - child->bar(7); // expected-warning {{calling function 'bar' requires exclusive lock on 'child->lock_'}} - child->a_ = 5; // expected-warning {{writing variable 'a_' requires locking 'child->lock_' exclusively}} + child->bar(7); // \ + // expected-warning {{calling function 'bar' requires exclusive lock on 'child->lock_'}} \ + // expected-note {{found near match 'lock_'}} + child->a_ = 5; // \ + // expected-warning {{writing variable 'a_' requires locking 'child->lock_' exclusively}} \ + // expected-note {{found near match 'lock_'}} lock_.Unlock(); } @@ -1495,7 +1507,8 @@ namespace substitution_test { DataLocker dlr; dlr.lockData(d1); foo(d2); // \ - // expected-warning {{calling function 'foo' requires exclusive lock on 'd2->mu'}} + // expected-warning {{calling function 'foo' requires exclusive lock on 'd2->mu'}} \ + // expected-note {{found near match 'd1->mu'}} dlr.unlockData(d1); } }; @@ -1833,7 +1846,8 @@ void test() { f1.mu_.Unlock(); bt.barTD(&f1); // \ - // expected-warning {{calling function 'barTD' requires exclusive lock on 'f1.mu_'}} + // expected-warning {{calling function 'barTD' requires exclusive lock on 'f1.mu_'}} \ + // expected-note {{found near match 'bt.fooBase.mu_'}} bt.fooBase.mu_.Unlock(); bt.fooBaseT.mu_.Unlock(); @@ -2239,27 +2253,32 @@ void test() { bar.getFoo().mu_.Lock(); bar.getFooey().a = 0; // \ - // expected-warning {{writing variable 'a' requires locking 'bar.getFooey().mu_' exclusively}} + // expected-warning {{writing variable 'a' requires locking 'bar.getFooey().mu_' exclusively}} \ + // expected-note {{found near match 'bar.getFoo().mu_'}} bar.getFoo().mu_.Unlock(); bar.getFoo2(a).mu_.Lock(); bar.getFoo2(b).a = 0; // \ - // expected-warning {{writing variable 'a' requires locking 'bar.getFoo2(b).mu_' exclusively}} + // expected-warning {{writing variable 'a' requires locking 'bar.getFoo2(b).mu_' exclusively}} \ + // expected-note {{found near match 'bar.getFoo2(a).mu_'}} bar.getFoo2(a).mu_.Unlock(); bar.getFoo3(a, b).mu_.Lock(); bar.getFoo3(a, c).a = 0; // \ - // expected-warning {{writing variable 'a' requires locking 'bar.getFoo3(a,c).mu_' exclusively}} + // expected-warning {{writing variable 'a' requires locking 'bar.getFoo3(a,c).mu_' exclusively}} \ + // expected-note {{'bar.getFoo3(a,b).mu_'}} bar.getFoo3(a, b).mu_.Unlock(); getBarFoo(bar, a).mu_.Lock(); getBarFoo(bar, b).a = 0; // \ - // expected-warning {{writing variable 'a' requires locking 'getBarFoo(bar,b).mu_' exclusively}} + // expected-warning {{writing variable 'a' requires locking 'getBarFoo(bar,b).mu_' exclusively}} \ + // expected-note {{'getBarFoo(bar,a).mu_'}} getBarFoo(bar, a).mu_.Unlock(); (a > 0 ? fooArray[1] : fooArray[b]).mu_.Lock(); (a > 0 ? fooArray[b] : fooArray[c]).a = 0; // \ - // expected-warning {{writing variable 'a' requires locking '((a#_)#_#fooArray[b]).mu_' exclusively}} + // expected-warning {{writing variable 'a' requires locking '((a#_)#_#fooArray[b]).mu_' exclusively}} \ + // expected-note {{'((a#_)#_#fooArray[_]).mu_'}} (a > 0 ? fooArray[1] : fooArray[b]).mu_.Unlock(); } @@ -2318,7 +2337,9 @@ void test1(Foo* f1, Foo* f2) { f1->a = 0; f1->foo(); - f1->foo2(f2); // expected-warning {{calling function 'foo2' requires exclusive lock on 'f2->mu_'}} + f1->foo2(f2); // \ + // expected-warning {{calling function 'foo2' requires exclusive lock on 'f2->mu_'}} \ + // expected-note {{found near match 'f1->mu_'}} Foo::getMu(f2)->Lock(); f1->foo2(f2); @@ -2358,7 +2379,9 @@ void test2(Bar* b1, Bar* b2) { b1->b = 0; b1->bar(); - b1->bar2(b2); // expected-warning {{calling function 'bar2' requires exclusive lock on 'b2->mu_'}} + b1->bar2(b2); // \ + // expected-warning {{calling function 'bar2' requires exclusive lock on 'b2->mu_'}} \ + // // expected-note {{found near match 'b1->mu_'}} b2->getMu()->Lock(); b1->bar2(b2); -- 2.40.0