aaronpuchert created this revision.
aaronpuchert added reviewers: aaron.ballman, delesley.
aaronpuchert requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
Previously in D104261 <https://reviews.llvm.org/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.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D106713
Files:
clang/lib/Analysis/ThreadSafety.cpp
clang/test/SemaCXX/warn-thread-safety-analysis.cpp
Index: clang/test/SemaCXX/warn-thread-safety-analysis.cpp
===================================================================
--- clang/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ clang/test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -2789,6 +2789,25 @@
}
}
+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 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)
Index: clang/lib/Analysis/ThreadSafety.cpp
===================================================================
--- clang/lib/Analysis/ThreadSafety.cpp
+++ clang/lib/Analysis/ThreadSafety.cpp
@@ -1050,7 +1050,7 @@
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 @@
}
}
-/// 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 @@
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,
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
