From 9b889f826ff587e9758c80823419512d502e457d Mon Sep 17 00:00:00 2001 From: Aaron Puchert Date: Sat, 18 Sep 2021 13:46:50 +0200 Subject: [PATCH] Thread safety analysis: Warn when demoting locks on back edges Previously in D104261 we warned about dropping locks from back edges, this is the corresponding change for exclusive/shared joins. If we're entering the loop with an exclusive change, which is then relaxed to a shared lock before we loop back, we have already analyzed the loop body with the stronger exclusive lock and thus might have false positives. There is a minor non-observable change: we modify the exit lock set of a function, but since that isn't used further it doesn't change anything. Reviewed By: aaron.ballman Differential Revision: https://reviews.llvm.org/D106713 --- clang/lib/Analysis/ThreadSafety.cpp | 31 ++++++++-------- clang/test/SemaCXX/warn-thread-safety-analysis.cpp | 42 ++++++++++++++++++++++ 2 files changed, 59 insertions(+), 14 deletions(-) diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp index 5b2c882..41a55f9 100644 --- a/clang/lib/Analysis/ThreadSafety.cpp +++ b/clang/lib/Analysis/ThreadSafety.cpp @@ -1050,7 +1050,7 @@ public: const CFGBlock* PredBlock, const CFGBlock *CurrBlock); - bool join(const FactEntry &a, const FactEntry &b); + bool join(const FactEntry &a, const FactEntry &b, bool CanModify); void intersectAndWarn(FactSet &EntrySet, const FactSet &ExitSet, SourceLocation JoinLoc, LockErrorKind EntryLEK, @@ -2188,25 +2188,28 @@ void BuildLockset::VisitDeclStmt(const DeclStmt *S) { } } -/// Given two facts merging on a join point, decide whether to warn and which -/// one to keep. +/// Given two facts merging on a join point, possibly warn and decide whether to +/// keep or replace. /// -/// \return false if we should keep \p A, true if we should keep \p B. -bool ThreadSafetyAnalyzer::join(const FactEntry &A, const FactEntry &B) { +/// \param CanModify Whether we can replace \p A by \p B. +/// \return false if we should keep \p A, true if we should take \p B. +bool ThreadSafetyAnalyzer::join(const FactEntry &A, const FactEntry &B, + bool CanModify) { if (A.kind() != B.kind()) { // For managed capabilities, the destructor should unlock in the right mode // anyway. For asserted capabilities no unlocking is needed. if ((A.managed() || A.asserted()) && (B.managed() || B.asserted())) { - // The shared capability subsumes the exclusive capability. - return B.kind() == LK_Shared; - } else { - Handler.handleExclusiveAndShared("mutex", B.toString(), B.loc(), A.loc()); - // Take the exclusive capability to reduce further warnings. - return B.kind() == LK_Exclusive; + // The shared capability subsumes the exclusive capability, if possible. + bool ShouldTakeB = B.kind() == LK_Shared; + if (CanModify || !ShouldTakeB) + return ShouldTakeB; } + Handler.handleExclusiveAndShared("mutex", B.toString(), B.loc(), A.loc()); + // Take the exclusive capability to reduce further warnings. + return CanModify && B.kind() == LK_Exclusive; } else { // The non-asserted capability is the one we want to track. - return A.asserted() && !B.asserted(); + return CanModify && A.asserted() && !B.asserted(); } } @@ -2237,8 +2240,8 @@ void ThreadSafetyAnalyzer::intersectAndWarn(FactSet &EntrySet, FactSet::iterator EntryIt = EntrySet.findLockIter(FactMan, ExitFact); if (EntryIt != EntrySet.end()) { - if (join(FactMan[*EntryIt], ExitFact) && - EntryLEK == LEK_LockedSomePredecessors) + if (join(FactMan[*EntryIt], ExitFact, + EntryLEK != LEK_LockedSomeLoopIterations)) *EntryIt = Fact; } else if (!ExitFact.managed()) { ExitFact.handleRemovalFromIntersection(ExitSet, FactMan, JoinLoc, diff --git a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp index e9d41da..125a195 100644 --- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp +++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp @@ -2789,6 +2789,25 @@ void loopRelease() { } } +void loopPromote() { + RelockableMutexLock scope(&mu, SharedTraits{}); + for (unsigned i = 1; i < 10; ++i) { + x = 1; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}} + if (i == 5) + scope.PromoteShared(); + } +} + +void loopDemote() { + RelockableMutexLock scope(&mu, ExclusiveTraits{}); // expected-note {{the other acquisition of mutex 'mu' is 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 exclusive lock. + if (i == 5) + scope.DemoteExclusive(); // expected-warning {{mutex 'mu' is acquired exclusively and shared in the same scope}} + } +} + void loopAcquireContinue() { RelockableMutexLock scope(&mu, DeferTraits{}); for (unsigned i = 1; i < 10; ++i) { @@ -2812,6 +2831,29 @@ void loopReleaseContinue() { } } +void loopPromoteContinue() { + RelockableMutexLock scope(&mu, SharedTraits{}); + for (unsigned i = 1; i < 10; ++i) { + x = 1; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}} + if (i == 5) { + scope.PromoteShared(); + continue; + } + } +} + +void loopDemoteContinue() { + RelockableMutexLock scope(&mu, ExclusiveTraits{}); // expected-note {{the other acquisition of mutex 'mu' is 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 exclusive lock. + if (i == 5) { + scope.DemoteExclusive(); // expected-warning {{mutex 'mu' is acquired exclusively and shared in the same scope}} + continue; + } + } +} + void exclusiveSharedJoin() { RelockableMutexLock scope(&mu, DeferTraits{}); if (b) -- 2.7.4