Author: Aaron Puchert Date: 2021-06-29T23:56:52+02:00 New Revision: f664e2ec371f61b69e11147d7f9e045083335917
URL: https://github.com/llvm/llvm-project/commit/f664e2ec371f61b69e11147d7f9e045083335917 DIFF: https://github.com/llvm/llvm-project/commit/f664e2ec371f61b69e11147d7f9e045083335917.diff LOG: Thread safety analysis: Always warn when dropping locks on back edges We allow branches to join where one holds a managed lock but the other doesn't, but we can't do so for back edges: because there we can't drop them from the lockset, as we have already analyzed the loop with the larger lockset. So we can't allow dropping managed locks on back edges. We move the managed() check from handleRemovalFromIntersection up to intersectAndWarn, where we additionally check if we're on a back edge if we're removing from the first lock set (the entry set of the next block) but not if we're removing from the second lock set (the exit set of the previous block). Now that the order of arguments matters, I had to swap them in one invocation, which also causes some minor differences in the tests. Reviewed By: delesley Differential Revision: https://reviews.llvm.org/D104261 Added: Modified: clang/lib/Analysis/ThreadSafety.cpp clang/test/SemaCXX/warn-thread-safety-analysis.cpp Removed: ################################################################################ diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp index 3eb1b640e7290..b09de2bd71f24 100644 --- a/clang/lib/Analysis/ThreadSafety.cpp +++ b/clang/lib/Analysis/ThreadSafety.cpp @@ -865,7 +865,7 @@ class LockableFactEntry : public FactEntry { handleRemovalFromIntersection(const FactSet &FSet, FactManager &FactMan, SourceLocation JoinLoc, LockErrorKind LEK, ThreadSafetyHandler &Handler) const override { - if (!managed() && !asserted() && !negative() && !isUniversal()) { + if (!asserted() && !negative() && !isUniversal()) { Handler.handleMutexHeldEndOfScope("mutex", toString(), loc(), JoinLoc, LEK); } @@ -2239,7 +2239,7 @@ void ThreadSafetyAnalyzer::intersectAndWarn(FactSet &FSet1, if (Iter1 != FSet1.end()) { if (join(FactMan[*Iter1], LDat2) && LEK1 == LEK_LockedSomePredecessors) *Iter1 = Fact; - } else { + } else if (!LDat2.managed()) { LDat2.handleRemovalFromIntersection(FSet2, FactMan, JoinLoc, LEK1, Handler); } @@ -2251,8 +2251,9 @@ void ThreadSafetyAnalyzer::intersectAndWarn(FactSet &FSet1, const FactEntry *LDat2 = FSet2.findLock(FactMan, *LDat1); if (!LDat2) { - LDat1->handleRemovalFromIntersection(FSet1Orig, FactMan, JoinLoc, LEK2, - Handler); + if (!LDat1->managed() || LEK2 == LEK_LockedSomeLoopIterations) + LDat1->handleRemovalFromIntersection(FSet1Orig, FactMan, JoinLoc, LEK2, + Handler); if (LEK2 == LEK_LockedSomePredecessors) FSet1.removeLock(FactMan, *LDat1); } @@ -2528,7 +2529,7 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) { CFGBlock *FirstLoopBlock = *SI; CFGBlockInfo *PreLoop = &BlockInfo[FirstLoopBlock->getBlockID()]; CFGBlockInfo *LoopEnd = &BlockInfo[CurrBlockID]; - intersectAndWarn(LoopEnd->ExitSet, PreLoop->EntrySet, PreLoop->EntryLoc, + intersectAndWarn(PreLoop->EntrySet, LoopEnd->ExitSet, PreLoop->EntryLoc, LEK_LockedSomeLoopIterations); } } diff --git a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp index 8e8bb6f45dde4..e9d41da80517c 100644 --- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp +++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp @@ -636,11 +636,11 @@ void shared_fun_0() { void shared_fun_1() { sls_mu.ReaderLock(); // \ - // expected-warning {{mutex 'sls_mu' is acquired exclusively and shared in the same scope}} + // expected-note {{the other acquisition of mutex 'sls_mu' is here}} do { sls_mu.Unlock(); sls_mu.Lock(); // \ - // expected-note {{the other acquisition of mutex 'sls_mu' is here}} + // expected-warning {{mutex 'sls_mu' is acquired exclusively and shared in the same scope}} } while (getBool()); sls_mu.Unlock(); } @@ -695,11 +695,11 @@ void shared_fun_11() { void shared_bad_0() { sls_mu.Lock(); // \ - // expected-warning {{mutex 'sls_mu' is acquired exclusively and shared in the same scope}} + // expected-note {{the other acquisition of mutex 'sls_mu' is here}} do { sls_mu.Unlock(); sls_mu.ReaderLock(); // \ - // expected-note {{the other acquisition of mutex 'sls_mu' is here}} + // expected-warning {{mutex 'sls_mu' is acquired exclusively and shared in the same scope}} } while (getBool()); sls_mu.Unlock(); } @@ -2773,6 +2773,45 @@ void unlockJoin() { x = 2; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}} } +void loopAcquire() { + RelockableMutexLock scope(&mu, DeferTraits{}); + for (unsigned i = 1; i < 10; ++i) + scope.Lock(); // We could catch this double lock with negative capabilities. +} + +void loopRelease() { + 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) { // 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(); + } +} + +void loopAcquireContinue() { + RelockableMutexLock scope(&mu, DeferTraits{}); + for (unsigned i = 1; i < 10; ++i) { + x = 1; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}} + if (i == 5) { + scope.Lock(); + continue; + } + } +} + +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) { + 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}} + } + } +} + void exclusiveSharedJoin() { RelockableMutexLock scope(&mu, DeferTraits{}); if (b) _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits