diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp index 41a55f9579bd..d6bb8cf673f7 100644 --- a/clang/lib/Analysis/ThreadSafety.cpp +++ b/clang/lib/Analysis/ThreadSafety.cpp @@ -849,6 +849,11 @@ static void findBlockLocations(CFG *CFGraph, // location. CurrBlockInfo->EntryLoc = CurrBlockInfo->ExitLoc = BlockInfo[(*CurrBlock->pred_begin())->getBlockID()].ExitLoc; + } else if (CurrBlock->succ_size() == 1 && *CurrBlock->succ_begin()) { + // The block is empty, and has a single successor. Use its entry + // location. + CurrBlockInfo->EntryLoc = CurrBlockInfo->ExitLoc = + BlockInfo[(*CurrBlock->succ_begin())->getBlockID()].EntryLoc; } } } @@ -2415,7 +2420,6 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) { // union because the real error is probably that we forgot to unlock M on // all code paths. bool LocksetInitialized = false; - SmallVector SpecialBlocks; for (CFGBlock::const_pred_iterator PI = CurrBlock->pred_begin(), PE = CurrBlock->pred_end(); PI != PE; ++PI) { // if *PI -> CurrBlock is a back edge @@ -2432,17 +2436,6 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) { // Okay, we can reach this block from the entry. CurrBlockInfo->Reachable = true; - // If the previous block ended in a 'continue' or 'break' statement, then - // a difference in locksets is probably due to a bug in that block, rather - // than in some other predecessor. In that case, keep the other - // predecessor's lockset. - if (const Stmt *Terminator = (*PI)->getTerminatorStmt()) { - if (isa(Terminator) || isa(Terminator)) { - SpecialBlocks.push_back(*PI); - continue; - } - } - FactSet PrevLockset; getEdgeLockset(PrevLockset, PrevBlockInfo->ExitSet, *PI, CurrBlock); @@ -2450,9 +2443,14 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) { CurrBlockInfo->EntrySet = PrevLockset; LocksetInitialized = true; } else { - intersectAndWarn(CurrBlockInfo->EntrySet, PrevLockset, - CurrBlockInfo->EntryLoc, - LEK_LockedSomePredecessors); + // Surprisingly 'continue' doesn't always produce back edges, because + // the CFG has empty "transition" blocks where they meet with the end + // of the regular loop body. We still want to diagnose them as loop. + intersectAndWarn( + CurrBlockInfo->EntrySet, PrevLockset, CurrBlockInfo->EntryLoc, + isa_and_nonnull((*PI)->getTerminatorStmt()) + ? LEK_LockedSomeLoopIterations + : LEK_LockedSomePredecessors); } } @@ -2460,35 +2458,6 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) { if (!CurrBlockInfo->Reachable) continue; - // Process continue and break blocks. Assume that the lockset for the - // resulting block is unaffected by any discrepancies in them. - for (const auto *PrevBlock : SpecialBlocks) { - unsigned PrevBlockID = PrevBlock->getBlockID(); - CFGBlockInfo *PrevBlockInfo = &BlockInfo[PrevBlockID]; - - if (!LocksetInitialized) { - CurrBlockInfo->EntrySet = PrevBlockInfo->ExitSet; - LocksetInitialized = true; - } else { - // Determine whether this edge is a loop terminator for diagnostic - // purposes. FIXME: A 'break' statement might be a loop terminator, but - // it might also be part of a switch. Also, a subsequent destructor - // might add to the lockset, in which case the real issue might be a - // double lock on the other path. - const Stmt *Terminator = PrevBlock->getTerminatorStmt(); - bool IsLoop = Terminator && isa(Terminator); - - FactSet PrevLockset; - getEdgeLockset(PrevLockset, PrevBlockInfo->ExitSet, - PrevBlock, CurrBlock); - - // Do not update EntrySet. - intersectAndWarn( - CurrBlockInfo->EntrySet, PrevLockset, PrevBlockInfo->ExitLoc, - IsLoop ? LEK_LockedSomeLoopIterations : LEK_LockedSomePredecessors); - } - } - BuildLockset LocksetBuilder(this, *CurrBlockInfo); // Visit all the statements in the basic block. diff --git a/clang/test/PCH/thread-safety-attrs.cpp b/clang/test/PCH/thread-safety-attrs.cpp index 3e0c081f134f..d33917e51859 100644 --- a/clang/test/PCH/thread-safety-attrs.cpp +++ b/clang/test/PCH/thread-safety-attrs.cpp @@ -254,12 +254,12 @@ void sls_fun_bad_6() { void sls_fun_bad_7() { sls_mu.Lock(); - while (getBool()) { + while (getBool()) { // \ + expected-warning{{expecting mutex 'sls_mu' to be held at start of each loop}} sls_mu.Unlock(); if (getBool()) { if (getBool()) { - continue; // \ - expected-warning{{expecting mutex 'sls_mu' to be held at start of each loop}} + continue; } } sls_mu.Lock(); // expected-note {{mutex acquired here}} @@ -306,13 +306,14 @@ void sls_fun_bad_12() { sls_mu.Unlock(); if (getBool()) { if (getBool()) { - break; // expected-warning{{mutex 'sls_mu' is not held on every path through here}} + break; } } sls_mu.Lock(); } sls_mu.Unlock(); // \ - // expected-warning{{releasing mutex 'sls_mu' that was not held}} + expected-warning{{mutex 'sls_mu' is not held on every path through here}} \ + expected-warning{{releasing mutex 'sls_mu' that was not held}} } #endif diff --git a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp index 125a1958c8ba..a3c07cd27d8f 100644 --- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp +++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp @@ -280,12 +280,12 @@ void sls_fun_bad_6() { void sls_fun_bad_7() { sls_mu.Lock(); - while (getBool()) { + while (getBool()) { // \ + expected-warning{{expecting mutex 'sls_mu' to be held at start of each loop}} sls_mu.Unlock(); if (getBool()) { if (getBool()) { - continue; // \ - expected-warning{{expecting mutex 'sls_mu' to be held at start of each loop}} + continue; } } sls_mu.Lock(); // expected-note {{mutex acquired here}} @@ -332,13 +332,14 @@ void sls_fun_bad_12() { sls_mu.Unlock(); if (getBool()) { if (getBool()) { - break; // expected-warning{{mutex 'sls_mu' is not held on every path through here}} + break; } } sls_mu.Lock(); } sls_mu.Unlock(); // \ - // expected-warning{{releasing mutex 'sls_mu' that was not held}} + expected-warning{{mutex 'sls_mu' is not held on every path through here}} \ + expected-warning{{releasing mutex 'sls_mu' that was not held}} } //-----------------------------------------// @@ -2086,13 +2087,13 @@ namespace GoingNative { mutex m; void test() { m.lock(); - while (foo()) { + while (foo()) { // expected-warning {{expecting mutex 'm' to be held at start of each loop}} m.unlock(); // ... if (bar()) { // ... if (foo()) - continue; // expected-warning {{expecting mutex 'm' to be held at start of each loop}} + continue; //... } // ... @@ -2822,11 +2823,11 @@ void loopAcquireContinue() { void loopReleaseContinue() { RelockableMutexLock scope(&mu, ExclusiveTraits{}); // expected-note {{mutex acquired here}} // We have to warn on this join point despite the lock being managed ... - for (unsigned i = 1; i < 10; ++i) { + for (unsigned i = 1; i < 10; ++i) { // expected-warning {{expecting mutex 'mu' to be held at start of each loop}} x = 1; // ... because we might miss that this doesn't always happen under lock. if (i == 5) { scope.Unlock(); - continue; // expected-warning {{expecting mutex 'mu' to be held at start of each loop}} + continue; } } }