From: DeLesley Hutchins Date: Fri, 22 Jun 2012 17:07:28 +0000 (+0000) Subject: Thread safety analysis: fixes a bug in which locksets are not handled X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=0da4414f3d30c34fafb81b13b2cec3680c0bc9e1;p=clang Thread safety analysis: fixes a bug in which locksets are not handled properly if there is a join point in the control flow graph that involves a trylock. Also changes the source locations of some warnings to be more consistent. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@159008 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/lib/Analysis/ThreadSafety.cpp b/lib/Analysis/ThreadSafety.cpp index 80bebb6144..075324da5f 100644 --- a/lib/Analysis/ThreadSafety.cpp +++ b/lib/Analysis/ThreadSafety.cpp @@ -898,13 +898,13 @@ public: Expr *BrE, bool Neg); const CallExpr* getTrylockCallExpr(const Stmt *Cond, LocalVarContext C, bool &Negate); - Lockset handleTrylock(const Lockset &LSet, - const CFGBlock* PredBlock, - const CFGBlock *CurrBlock); - Lockset intersectAndWarn(const CFGBlockInfo &Block1, CFGBlockSide Side1, - const CFGBlockInfo &Block2, CFGBlockSide Side2, - LockErrorKind LEK); + Lockset getEdgeLockset(const Lockset &ExitSet, + const CFGBlock* PredBlock, + const CFGBlock *CurrBlock); + + Lockset intersectAndWarn(const Lockset &LSet1, const Lockset &LSet2, + SourceLocation JoinLoc, LockErrorKind LEK); void runAnalysis(AnalysisDeclContext &AC); }; @@ -1106,11 +1106,15 @@ const CallExpr* ThreadSafetyAnalyzer::getTrylockCallExpr(const Stmt *Cond, } -/// \brief Process a conditional branch from a previous block to the current -/// block, looking for trylock calls. -Lockset ThreadSafetyAnalyzer::handleTrylock(const Lockset &LSet, - const CFGBlock *PredBlock, - const CFGBlock *CurrBlock) { +/// \brief Find the lockset that holds on the edge between PredBlock +/// and CurrBlock. The edge set is the exit set of PredBlock (passed +/// as the ExitSet parameter) plus any trylocks, which are conditionally held. +Lockset ThreadSafetyAnalyzer::getEdgeLockset(const Lockset &ExitSet, + const CFGBlock *PredBlock, + const CFGBlock *CurrBlock) { + if (!PredBlock->getTerminatorCondition()) + return ExitSet; + bool Negate = false; const Stmt *Cond = PredBlock->getTerminatorCondition(); const CFGBlockInfo *PredBlockInfo = &BlockInfo[PredBlock->getBlockID()]; @@ -1119,13 +1123,13 @@ Lockset ThreadSafetyAnalyzer::handleTrylock(const Lockset &LSet, CallExpr *Exp = const_cast( getTrylockCallExpr(Cond, LVarCtx, Negate)); if (!Exp) - return LSet; + return ExitSet; NamedDecl *FunDecl = dyn_cast_or_null(Exp->getCalleeDecl()); if(!FunDecl || !FunDecl->hasAttrs()) - return LSet; + return ExitSet; - Lockset Result = LSet; + Lockset Result = ExitSet; // If the condition is a call to a Trylock function, then grab the attributes AttrVec &ArgAttrs = FunDecl->getAttrs(); @@ -1458,6 +1462,7 @@ void BuildLockset::VisitDeclStmt(DeclStmt *S) { } + /// \brief Compute the intersection of two locksets and issue warnings for any /// locks in the symmetric difference. /// @@ -1466,15 +1471,17 @@ void BuildLockset::VisitDeclStmt(DeclStmt *S) { /// 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. -Lockset ThreadSafetyAnalyzer::intersectAndWarn(const CFGBlockInfo &Block1, - CFGBlockSide Side1, - const CFGBlockInfo &Block2, - CFGBlockSide Side2, +/// +/// \param LSet1 The first lockset. +/// \param LSet2 The second lockset. +/// \param JoinLoc The location of the join point for error reporting +/// \param LEK The error message to report. +Lockset ThreadSafetyAnalyzer::intersectAndWarn(const Lockset &LSet1, + const Lockset &LSet2, + SourceLocation JoinLoc, LockErrorKind LEK) { - Lockset LSet1 = Block1.getSet(Side1); - Lockset LSet2 = Block2.getSet(Side2); - 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(); @@ -1490,7 +1497,7 @@ Lockset ThreadSafetyAnalyzer::intersectAndWarn(const CFGBlockInfo &Block1, } else { Handler.handleMutexHeldEndOfScope(LSet2Mutex.getName(), LSet2LockData.AcquireLoc, - Block1.getLocation(Side1), LEK); + JoinLoc, LEK); } } @@ -1500,7 +1507,7 @@ Lockset ThreadSafetyAnalyzer::intersectAndWarn(const CFGBlockInfo &Block1, const LockData &MissingLock = I.getData(); Handler.handleMutexHeldEndOfScope(Mutex.getName(), MissingLock.AcquireLoc, - Block2.getLocation(Side2), LEK); + JoinLoc, LEK); Intersection = LocksetFactory.remove(Intersection, Mutex); } } @@ -1518,6 +1525,8 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) { if (!CFGraph) return; const NamedDecl *D = dyn_cast_or_null(AC.getDecl()); + // AC.dumpCFG(true); + if (!D) return; // Ignore anonymous functions for now. if (D->getAttr()) @@ -1631,14 +1640,16 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) { int PrevBlockID = (*PI)->getBlockID(); CFGBlockInfo *PrevBlockInfo = &BlockInfo[PrevBlockID]; + Lockset PrevLockset = + getEdgeLockset(PrevBlockInfo->ExitSet, *PI, CurrBlock); if (!LocksetInitialized) { - CurrBlockInfo->EntrySet = PrevBlockInfo->ExitSet; + CurrBlockInfo->EntrySet = PrevLockset; LocksetInitialized = true; } else { CurrBlockInfo->EntrySet = - intersectAndWarn(*CurrBlockInfo, CBS_Entry, - *PrevBlockInfo, CBS_Exit, + intersectAndWarn(CurrBlockInfo->EntrySet, PrevLockset, + CurrBlockInfo->EntryLoc, LEK_LockedSomePredecessors); } } @@ -1663,24 +1674,17 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) { const Stmt *Terminator = PrevBlock->getTerminator(); bool IsLoop = Terminator && isa(Terminator); + Lockset PrevLockset = + getEdgeLockset(PrevBlockInfo->ExitSet, PrevBlock, CurrBlock); + // Do not update EntrySet. - intersectAndWarn(*CurrBlockInfo, CBS_Entry, *PrevBlockInfo, CBS_Exit, + intersectAndWarn(CurrBlockInfo->EntrySet, PrevLockset, + PrevBlockInfo->ExitLoc, IsLoop ? LEK_LockedSomeLoopIterations : LEK_LockedSomePredecessors); } } - // If the previous block ended in a trylock, then grab any extra mutexes - // from the trylock. - for (CFGBlock::const_pred_iterator PI = CurrBlock->pred_begin(), - PE = CurrBlock->pred_end(); PI != PE; ++PI) { - // If the predecessor ended in a branch, then process any trylocks. - if ((*PI)->getTerminatorCondition()) { - CurrBlockInfo->EntrySet = handleTrylock(CurrBlockInfo->EntrySet, - *PI, CurrBlock); - } - } - BuildLockset LocksetBuilder(this, *CurrBlockInfo); // Visit all the statements in the basic block. @@ -1725,18 +1729,20 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) { continue; CFGBlock *FirstLoopBlock = *SI; - CFGBlockInfo &PreLoop = BlockInfo[FirstLoopBlock->getBlockID()]; - CFGBlockInfo &LoopEnd = BlockInfo[CurrBlockID]; - intersectAndWarn(LoopEnd, CBS_Exit, PreLoop, CBS_Entry, + CFGBlockInfo *PreLoop = &BlockInfo[FirstLoopBlock->getBlockID()]; + CFGBlockInfo *LoopEnd = &BlockInfo[CurrBlockID]; + intersectAndWarn(LoopEnd->ExitSet, PreLoop->EntrySet, + PreLoop->EntryLoc, LEK_LockedSomeLoopIterations); } } - CFGBlockInfo &Initial = BlockInfo[CFGraph->getEntry().getBlockID()]; - CFGBlockInfo &Final = BlockInfo[CFGraph->getExit().getBlockID()]; + CFGBlockInfo *Initial = &BlockInfo[CFGraph->getEntry().getBlockID()]; + CFGBlockInfo *Final = &BlockInfo[CFGraph->getExit().getBlockID()]; // FIXME: Should we call this function for all blocks which exit the function? - intersectAndWarn(Initial, CBS_Entry, Final, CBS_Exit, + intersectAndWarn(Initial->EntrySet, Final->ExitSet, + Final->ExitLoc, LEK_LockedAtEndOfFunction); } diff --git a/test/SemaCXX/warn-thread-safety-analysis.cpp b/test/SemaCXX/warn-thread-safety-analysis.cpp index 47d844d003..4755daa6e3 100644 --- a/test/SemaCXX/warn-thread-safety-analysis.cpp +++ b/test/SemaCXX/warn-thread-safety-analysis.cpp @@ -178,14 +178,11 @@ void sls_fun_bad_3() { void sls_fun_bad_4() { if (getBool()) - sls_mu.Lock(); // \ - expected-warning{{mutex 'sls_mu2' is not locked on every path through here}} \ - expected-note{{mutex acquired here}} - + sls_mu.Lock(); // expected-note{{mutex acquired here}} else - sls_mu2.Lock(); // \ - expected-note{{mutex acquired here}} -} // expected-warning{{mutex 'sls_mu' is not locked on every path through here}} + sls_mu2.Lock(); // expected-note{{mutex acquired here}} +} // expected-warning{{mutex 'sls_mu' is not locked on every path through here}} \ + // expected-warning{{mutex 'sls_mu2' is not locked on every path through here}} void sls_fun_bad_5() { sls_mu.Lock(); // expected-note {{mutex acquired here}} @@ -226,15 +223,14 @@ void sls_fun_bad_7() { void sls_fun_bad_8() { sls_mu.Lock(); // expected-note{{mutex acquired here}} - // FIXME: TERRIBLE SOURCE LOCATION! - do { // expected-warning{{expecting mutex 'sls_mu' to be locked at start of each loop}} - sls_mu.Unlock(); + do { + sls_mu.Unlock(); // expected-warning{{expecting mutex 'sls_mu' to be locked at start of each loop}} } while (getBool()); } void sls_fun_bad_9() { do { - sls_mu.Lock(); // \ + sls_mu.Lock(); // \ // expected-warning{{expecting mutex 'sls_mu' to be locked at start of each loop}} \ // expected-note{{mutex acquired here}} } while (getBool()); @@ -242,15 +238,15 @@ void sls_fun_bad_9() { } void sls_fun_bad_10() { - sls_mu.Lock(); // expected-note 2{{mutex acquired here}} - while(getBool()) { - sls_mu.Unlock(); // expected-warning{{expecting mutex 'sls_mu' to be locked at start of each loop}} + sls_mu.Lock(); // expected-note 2{{mutex acquired here}} + while(getBool()) { // expected-warning{{expecting mutex 'sls_mu' to be locked at start of each loop}} + sls_mu.Unlock(); } } // expected-warning{{mutex 'sls_mu' is still locked at the end of function}} void sls_fun_bad_11() { while (getBool()) { // \ - expected-warning{{expecting mutex 'sls_mu' to be locked at start of each loop}} + expected-warning{{expecting mutex 'sls_mu' to be locked at start of each loop}} sls_mu.Lock(); // expected-note {{mutex acquired here}} } sls_mu.Unlock(); // \ @@ -2239,6 +2235,26 @@ void test() { } -} // end namespace +} // end namespace MoreLockExpressions + + +namespace TrylockJoinPoint { + +class Foo { + Mutex mu; + bool c; + + void foo() { + if (c) { + if (!mu.TryLock()) + return; + } else { + mu.Lock(); + } + mu.Unlock(); + } +}; + +} // end namespace TrylockJoinPoint