diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafety.h b/clang/include/clang/Analysis/Analyses/ThreadSafety.h index db1ef8f312d4..a32505624a22 100644 --- a/clang/include/clang/Analysis/Analyses/ThreadSafety.h +++ b/clang/include/clang/Analysis/Analyses/ThreadSafety.h @@ -49,6 +49,20 @@ enum AccessKind { AK_Written /// Writing a variable }; +/// This enum distinguishes between different situations where we warn due to +/// inconsistent locking. +/// \enum SK_LockedSomeLoopIterations -- a mutex is locked for some but not all +/// loop iterations. +/// \enum SK_LockedSomePredecessors -- a mutex is locked in some but not all +/// predecessors of a CFGBlock. +/// \enum SK_LockedAtEndOfFunction -- a mutex is still locked at the end of a +/// function. +enum LockErrorKind { + LEK_LockedSomeLoopIterations, + LEK_LockedSomePredecessors, + LEK_LockedAtEndOfFunction +}; + /// Handler class for thread safety warnings. class ThreadSafetyHandler { public: @@ -73,27 +87,16 @@ public: virtual void handleDoubleLock(Name LockName, SourceLocation Loc) {} /// Warn about situations where a mutex is sometimes held and sometimes not. - /// For example, a mutex is locked on an "if" branch but not the "else" - /// branch. + /// The three situations are: + /// 1. a mutex is locked on an "if" branch but not the "else" branch, + /// 2, or a mutex is only held at the start of some loop iterations, + /// 3. or when a mutex is locked but not unlocked inside a function. /// \param LockName -- A StringRef name for the lock expression, to be printed /// in the error message. - /// \param Loc -- The location of the lock expression where the mutex is - /// locked - virtual void handleMutexHeldEndOfScope(Name LockName, SourceLocation Loc){} - - /// Warn when a mutex is only held at the start of some loop iterations. - /// \param LockName -- A StringRef name for the lock expression, to be printed - /// in the error message. - /// \param Loc -- The Loc of the lock expression. - virtual void handleNoLockLoopEntry(Name LockName, SourceLocation Loc) {} - - /// Warn when a mutex is locked but not unlocked inside a function. - /// \param LockName -- A StringRef name for the lock expression, to be printed - /// in the error message. - /// \param FunName -- The name of the function - /// \param Loc -- The location of the lock expression - virtual void handleNoUnlock(Name LockName, Name FunName, - SourceLocation Loc) {} + /// \param Loc -- The location of the lock expression where the mutex is locked + /// \param LEK -- which of the three above cases we should warn for + virtual void handleMutexHeldEndOfScope(Name LockName, SourceLocation Loc, + LockErrorKind LEK){} /// Warn when a mutex is held exclusively and shared at the same point. For /// example, if a mutex is locked exclusively during an if branch and shared diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td index 332aa69432cd..8bb7db69391d 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -1398,7 +1398,7 @@ def warn_double_lock : Warning< "locking '%0' that is already locked">, InGroup, DefaultIgnore; def warn_no_unlock : Warning< - "mutex '%0' is still held at the end of function '%1'">, + "mutex '%0' is still held at the end of function">, InGroup, DefaultIgnore; // FIXME: improve the error message about locks not in scope def warn_lock_at_end_of_scope : Warning< diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp index 233f58c43221..504a24c0fa9e 100644 --- a/clang/lib/Analysis/ThreadSafety.cpp +++ b/clang/lib/Analysis/ThreadSafety.cpp @@ -598,13 +598,18 @@ void BuildLockset::VisitCXXMemberCallExpr(CXXMemberCallExpr *Exp) { } // end anonymous namespace -/// \brief Flags a warning for each lock that is in LSet2 but not LSet1, or -/// else mutexes that are held shared in one lockset and exclusive in the other. -static Lockset warnIfNotInFirstSetOrNotSameKind(ThreadSafetyHandler &Handler, - const Lockset LSet1, - const Lockset LSet2, - Lockset Intersection, - Lockset::Factory &Fact) { +/// \brief Compute the intersection of two locksets and issue warnings for any +/// locks in the symmetric difference. +/// +/// This function is used at a merge point in the CFG when comparing the lockset +/// of each branch being merged. For example, given the following sequence: +/// A; if () then B; else C; D; we need to check that the lockset after B and C +/// are the same. In the event of a difference, we use the intersection of these +/// two locksets at the start of D. +static Lockset intersectAndWarn(ThreadSafetyHandler &Handler, + const Lockset LSet1, const Lockset LSet2, + Lockset::Factory &Fact, LockErrorKind LEK) { + Lockset Intersection = LSet1; for (Lockset::iterator I = LSet2.begin(), E = LSet2.end(); I != E; ++I) { const MutexID &LSet2Mutex = I.getKey(); const LockData &LSet2LockData = I.getData(); @@ -618,34 +623,16 @@ static Lockset warnIfNotInFirstSetOrNotSameKind(ThreadSafetyHandler &Handler, } } else { Handler.handleMutexHeldEndOfScope(LSet2Mutex.getName(), - LSet2LockData.AcquireLoc); + LSet2LockData.AcquireLoc, LEK); } } - return Intersection; -} - - -/// \brief Compute the intersection of two locksets and issue warnings for any -/// locks in the symmetric difference. -/// -/// This function is used at a merge point in the CFG when comparing the lockset -/// of each branch being merged. For example, given the following sequence: -/// A; if () then B; else C; D; we need to check that the lockset after B and C -/// are the same. In the event of a difference, we use the intersection of these -/// two locksets at the start of D. -static Lockset intersectAndWarn(ThreadSafetyHandler &Handler, - const Lockset LSet1, const Lockset LSet2, - Lockset::Factory &Fact) { - Lockset Intersection = LSet1; - Intersection = warnIfNotInFirstSetOrNotSameKind(Handler, LSet1, LSet2, - Intersection, Fact); for (Lockset::iterator I = LSet1.begin(), E = LSet1.end(); I != E; ++I) { if (!LSet2.contains(I.getKey())) { const MutexID &Mutex = I.getKey(); const LockData &MissingLock = I.getData(); Handler.handleMutexHeldEndOfScope(Mutex.getName(), - MissingLock.AcquireLoc); + MissingLock.AcquireLoc, LEK); Intersection = Fact.remove(Intersection, Mutex); } } @@ -669,34 +656,6 @@ static SourceLocation getFirstStmtLocation(CFGBlock *Block) { return Loc; } -/// \brief Warn about different locksets along backedges of loops. -/// This function is called when we encounter a back edge. At that point, -/// we need to verify that the lockset before taking the backedge is the -/// same as the lockset before entering the loop. -/// -/// \param LoopEntrySet Locks before starting the loop -/// \param LoopReentrySet Locks in the last CFG block of the loop -static void warnBackEdgeUnequalLocksets(ThreadSafetyHandler &Handler, - const Lockset LoopReentrySet, - const Lockset LoopEntrySet, - SourceLocation FirstLocInLoop, - Lockset::Factory &Fact) { - assert(FirstLocInLoop.isValid()); - // Warn for locks held at the start of the loop, but not the end. - for (Lockset::iterator I = LoopEntrySet.begin(), E = LoopEntrySet.end(); - I != E; ++I) { - if (!LoopReentrySet.contains(I.getKey())) { - // We report this error at the location of the first statement in a loop - Handler.handleNoLockLoopEntry(I.getKey().getName(), FirstLocInLoop); - } - } - - // Warn for locks held at the end of the loop, but not at the start. - warnIfNotInFirstSetOrNotSameKind(Handler, LoopEntrySet, LoopReentrySet, - LoopReentrySet, Fact); -} - - namespace clang { namespace thread_safety { /// \brief Check a function's CFG for thread-safety violations. @@ -763,7 +722,8 @@ void runThreadSafetyAnalysis(AnalysisContext &AC, LocksetInitialized = true; } else { Entryset = intersectAndWarn(Handler, Entryset, - ExitLocksets[PrevBlockID], LocksetFactory); + ExitLocksets[PrevBlockID], LocksetFactory, + LEK_LockedSomePredecessors); } } @@ -787,36 +747,17 @@ void runThreadSafetyAnalysis(AnalysisContext &AC, continue; CFGBlock *FirstLoopBlock = *SI; - SourceLocation FirstLoopLocation = getFirstStmtLocation(FirstLoopBlock); - - assert(FirstLoopLocation.isValid()); - - // Fail gracefully in release code. - if (!FirstLoopLocation.isValid()) - continue; - Lockset PreLoop = EntryLocksets[FirstLoopBlock->getBlockID()]; Lockset LoopEnd = ExitLocksets[CurrBlockID]; - warnBackEdgeUnequalLocksets(Handler, LoopEnd, PreLoop, FirstLoopLocation, - LocksetFactory); + intersectAndWarn(Handler, LoopEnd, PreLoop, LocksetFactory, + LEK_LockedSomeLoopIterations); } } + Lockset InitialLockset = EntryLocksets[CFGraph->getEntry().getBlockID()]; Lockset FinalLockset = ExitLocksets[CFGraph->getExit().getBlockID()]; - if (!FinalLockset.isEmpty()) { - for (Lockset::iterator I=FinalLockset.begin(), E=FinalLockset.end(); - I != E; ++I) { - const MutexID &Mutex = I.getKey(); - const LockData &MissingLock = I.getData(); - - std::string FunName = ""; - if (const NamedDecl *ContextDecl = dyn_cast(AC.getDecl())) { - FunName = ContextDecl->getDeclName().getAsString(); - } - - Handler.handleNoUnlock(Mutex.getName(), FunName, MissingLock.AcquireLoc); - } - } + intersectAndWarn(Handler, InitialLockset, FinalLockset, LocksetFactory, + LEK_LockedAtEndOfFunction); } /// \brief Helper function that returns a LockKind required for the given level diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp index bbd8fa38fe79..15800b38c0c2 100644 --- a/clang/lib/Sema/AnalysisBasedWarnings.cpp +++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp @@ -633,20 +633,23 @@ class ThreadSafetyReporter : public clang::thread_safety::ThreadSafetyHandler { warnLockMismatch(diag::warn_double_lock, LockName, Loc); } - void handleMutexHeldEndOfScope(Name LockName, SourceLocation Loc){ - warnLockMismatch(diag::warn_lock_at_end_of_scope, LockName, Loc); + void handleMutexHeldEndOfScope(Name LockName, SourceLocation Loc, + LockErrorKind LEK){ + unsigned DiagID = 0; + switch (LEK) { + case LEK_LockedSomePredecessors: + DiagID = diag::warn_lock_at_end_of_scope; + break; + case LEK_LockedSomeLoopIterations: + DiagID = diag::warn_expecting_lock_held_on_loop; + break; + case LEK_LockedAtEndOfFunction: + DiagID = diag::warn_no_unlock; + break; + } + warnLockMismatch(DiagID, LockName, Loc); } - void handleNoLockLoopEntry(Name LockName, SourceLocation Loc) { - warnLockMismatch(diag::warn_expecting_lock_held_on_loop, LockName, Loc); - } - - void handleNoUnlock(Name LockName, llvm::StringRef FunName, - SourceLocation Loc) { - PartialDiagnostic Warning = - S.PDiag(diag::warn_no_unlock) << LockName << FunName; - Warnings.push_back(DelayedDiag(Loc, Warning)); - } void handleExclusiveAndShared(Name LockName, SourceLocation Loc1, SourceLocation Loc2) { diff --git a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp index 899076bb0e66..cca2f3aa65cf 100644 --- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp +++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp @@ -176,9 +176,9 @@ void sls_fun_bad_6() { } void sls_fun_bad_7() { - sls_mu.Lock(); - while (getBool()) { // \ - // expected-warning{{expecting lock on 'sls_mu' to be held at start of each loop}} + sls_mu.Lock(); // \ + // expected-warning{{expecting lock on 'sls_mu' to be held at start of each loop}} + while (getBool()) { sls_mu.Unlock(); if (getBool()) { if (getBool()) { @@ -192,26 +192,26 @@ void sls_fun_bad_7() { } void sls_fun_bad_8() { - sls_mu.Lock(); + sls_mu.Lock(); // \ + // expected-warning{{expecting lock on 'sls_mu' to be held at start of each loop}} do { - sls_mu.Unlock(); // \ - // expected-warning{{expecting lock on 'sls_mu' to be held at start of each loop}} + sls_mu.Unlock(); } while (getBool()); } void sls_fun_bad_9() { do { sls_mu.Lock(); // \ - // expected-warning{{mutex 'sls_mu' is still held at the end of its scope}} + // expected-warning{{expecting lock on 'sls_mu' to be held at start of each loop}} } while (getBool()); sls_mu.Unlock(); } void sls_fun_bad_10() { sls_mu.Lock(); // \ - // expected-warning{{mutex 'sls_mu' is still held at the end of function 'sls_fun_bad_10'}} - while(getBool()) { // \ - // expected-warning{{expecting lock on 'sls_mu' to be held at start of each loop}} + // expected-warning{{mutex 'sls_mu' is still held at the end of function 'sls_fun_bad_10'}} \ + // expected-warning{{expecting lock on 'sls_mu' to be held at start of each loop}} + while(getBool()) { sls_mu.Unlock(); } } @@ -219,7 +219,7 @@ void sls_fun_bad_10() { void sls_fun_bad_11() { while (getBool()) { sls_mu.Lock(); // \ - // expected-warning{{mutex 'sls_mu' is still held at the end of its scope}} + // expected-warning{{expecting lock on 'sls_mu' to be held at start of each loop}} } sls_mu.Unlock(); // \ // expected-warning{{unlocking 'sls_mu' that was not locked}} @@ -519,11 +519,12 @@ void shared_fun_0() { } void shared_fun_1() { - sls_mu.ReaderLock(); + sls_mu.ReaderLock(); // \ + // expected-warning {{lock 'sls_mu' is exclusive and shared in the same scope}} do { sls_mu.Unlock(); - sls_mu.Lock(); // \ - // expected-warning {{lock 'sls_mu' is exclusive and shared in the same scope}} + sls_mu.Lock(); // \ + // expected-note {{the other lock of mutex 'sls_mu' is here}} } while (getBool()); sls_mu.Unlock(); } @@ -557,11 +558,12 @@ void shared_fun_8() { } void shared_bad_0() { - sls_mu.Lock(); + sls_mu.Lock(); // \ + // expected-warning {{lock 'sls_mu' is exclusive and shared in the same scope}} do { sls_mu.Unlock(); - sls_mu.ReaderLock(); // \ - // expected-warning {{lock 'sls_mu' is exclusive and shared in the same scope}} + sls_mu.ReaderLock(); // \ + // expected-note {{the other lock of mutex 'sls_mu' is here}} } while (getBool()); sls_mu.Unlock(); }