[PATCH] D102025: Thread safety analysis: Factor out function for merging locks (NFC)
This revision was automatically updated to reflect the committed changes.
Closed by commit rG3d64677c2807: Thread safety analysis: Factor out function
for merging locks (NFC) (authored by aaronpuchert).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D102025/new/
https://reviews.llvm.org/D102025
Files:
clang/lib/Analysis/ThreadSafety.cpp
Index: clang/lib/Analysis/ThreadSafety.cpp
===
--- clang/lib/Analysis/ThreadSafety.cpp
+++ clang/lib/Analysis/ThreadSafety.cpp
@@ -1050,6 +1050,8 @@
const CFGBlock* PredBlock,
const CFGBlock *CurrBlock);
+ bool join(const FactEntry , const FactEntry );
+
void intersectAndWarn(FactSet , const FactSet ,
SourceLocation JoinLoc, LockErrorKind LEK1,
LockErrorKind LEK2);
@@ -2186,6 +2188,21 @@
}
}
+/// Given two facts merging on a join point, decide whether to warn and which
+/// one to keep.
+///
+/// \return false if we should keep \p A, true if we should keep \p B.
+bool ThreadSafetyAnalyzer::join(const FactEntry , const FactEntry ) {
+ if (A.kind() != B.kind()) {
+Handler.handleExclusiveAndShared("mutex", B.toString(), B.loc(), A.loc());
+// Take the exclusive capability to reduce further warnings.
+return B.kind() == LK_Exclusive;
+ } else {
+// The non-asserted capability is the one we want to track.
+return A.asserted() && !B.asserted();
+ }
+}
+
/// Compute the intersection of two locksets and issue warnings for any
/// locks in the symmetric difference.
///
@@ -2213,20 +2230,8 @@
FactSet::iterator Iter1 = FSet1.findLockIter(FactMan, LDat2);
if (Iter1 != FSet1.end()) {
- const FactEntry = FactMan[*Iter1];
- if (LDat1.kind() != LDat2.kind()) {
-Handler.handleExclusiveAndShared("mutex", LDat2.toString(),
LDat2.loc(),
- LDat1.loc());
-if (LEK1 == LEK_LockedSomePredecessors &&
-LDat1.kind() != LK_Exclusive) {
- // Take the exclusive lock, which is the one in FSet2.
- *Iter1 = Fact;
-}
- } else if (LEK1 == LEK_LockedSomePredecessors && LDat1.asserted() &&
- !LDat2.asserted()) {
-// The non-asserted lock in FSet2 is the one we want to track.
+ if (join(FactMan[*Iter1], LDat2) && LEK1 == LEK_LockedSomePredecessors)
*Iter1 = Fact;
- }
} else {
LDat2.handleRemovalFromIntersection(FSet2, FactMan, JoinLoc, LEK1,
Handler);
Index: clang/lib/Analysis/ThreadSafety.cpp
===
--- clang/lib/Analysis/ThreadSafety.cpp
+++ clang/lib/Analysis/ThreadSafety.cpp
@@ -1050,6 +1050,8 @@
const CFGBlock* PredBlock,
const CFGBlock *CurrBlock);
+ bool join(const FactEntry , const FactEntry );
+
void intersectAndWarn(FactSet , const FactSet ,
SourceLocation JoinLoc, LockErrorKind LEK1,
LockErrorKind LEK2);
@@ -2186,6 +2188,21 @@
}
}
+/// Given two facts merging on a join point, decide whether to warn and which
+/// one to keep.
+///
+/// \return false if we should keep \p A, true if we should keep \p B.
+bool ThreadSafetyAnalyzer::join(const FactEntry , const FactEntry ) {
+ if (A.kind() != B.kind()) {
+Handler.handleExclusiveAndShared("mutex", B.toString(), B.loc(), A.loc());
+// Take the exclusive capability to reduce further warnings.
+return B.kind() == LK_Exclusive;
+ } else {
+// The non-asserted capability is the one we want to track.
+return A.asserted() && !B.asserted();
+ }
+}
+
/// Compute the intersection of two locksets and issue warnings for any
/// locks in the symmetric difference.
///
@@ -2213,20 +2230,8 @@
FactSet::iterator Iter1 = FSet1.findLockIter(FactMan, LDat2);
if (Iter1 != FSet1.end()) {
- const FactEntry = FactMan[*Iter1];
- if (LDat1.kind() != LDat2.kind()) {
-Handler.handleExclusiveAndShared("mutex", LDat2.toString(), LDat2.loc(),
- LDat1.loc());
-if (LEK1 == LEK_LockedSomePredecessors &&
-LDat1.kind() != LK_Exclusive) {
- // Take the exclusive lock, which is the one in FSet2.
- *Iter1 = Fact;
-}
- } else if (LEK1 == LEK_LockedSomePredecessors && LDat1.asserted() &&
- !LDat2.asserted()) {
-// The non-asserted lock in FSet2 is the one we want to track.
+ if (join(FactMan[*Iter1], LDat2) && LEK1 == LEK_LockedSomePredecessors)
*Iter1 = Fact;
- }
} else {
LDat2.handleRemovalFromIntersection(FSet2, FactMan, JoinLoc, LEK1,
Handler);
___
cfe-commits mailing list
[PATCH] D102025: Thread safety analysis: Factor out function for merging locks (NFC)
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM!
Comment at: clang/lib/Analysis/ThreadSafety.cpp:2199
+// Take the exclusive capability to reduce further warnings.
+return B.kind() == LK_Exclusive;
+ } else {
aaronpuchert wrote:
> aaron.ballman wrote:
> > The old code was looking at `LDat1.kind() != LK_Exclusive` -- any reason
> > this isn't `A.kind() != LK_Exclusive` as well?
> Given that both should be equivalent (knowing `A.kind() != B.kind()` and that
> there are just two kinds), I thought this condition fits better to the
> comment: we select the lock that's exclusive, instead of not selecting the
> lock that's not exclusive.
>
> But I don't have a problem with `A.kind() != LK_Exclusive` if that sounds
> more natural to you.
Oo, derp. I was forgetting that there were only two kinds. This looks fine
to me, thanks!
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D102025/new/
https://reviews.llvm.org/D102025
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D102025: Thread safety analysis: Factor out function for merging locks (NFC)
aaronpuchert added inline comments.
Comment at: clang/lib/Analysis/ThreadSafety.cpp:2199
+// Take the exclusive capability to reduce further warnings.
+return B.kind() == LK_Exclusive;
+ } else {
aaron.ballman wrote:
> The old code was looking at `LDat1.kind() != LK_Exclusive` -- any reason this
> isn't `A.kind() != LK_Exclusive` as well?
Given that both should be equivalent (knowing `A.kind() != B.kind()` and that
there are just two kinds), I thought this condition fits better to the comment:
we select the lock that's exclusive, instead of not selecting the lock that's
not exclusive.
But I don't have a problem with `A.kind() != LK_Exclusive` if that sounds more
natural to you.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D102025/new/
https://reviews.llvm.org/D102025
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D102025: Thread safety analysis: Factor out function for merging locks (NFC)
aaron.ballman added inline comments.
Comment at: clang/lib/Analysis/ThreadSafety.cpp:2199
+// Take the exclusive capability to reduce further warnings.
+return B.kind() == LK_Exclusive;
+ } else {
The old code was looking at `LDat1.kind() != LK_Exclusive` -- any reason this
isn't `A.kind() != LK_Exclusive` as well?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D102025/new/
https://reviews.llvm.org/D102025
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D102025: Thread safety analysis: Factor out function for merging locks (NFC)
aaronpuchert created this revision.
aaronpuchert added a reviewer: aaron.ballman.
aaronpuchert requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
It's going to become a bit more complicated, so let's have it separate.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D102025
Files:
clang/lib/Analysis/ThreadSafety.cpp
Index: clang/lib/Analysis/ThreadSafety.cpp
===
--- clang/lib/Analysis/ThreadSafety.cpp
+++ clang/lib/Analysis/ThreadSafety.cpp
@@ -1050,6 +1050,8 @@
const CFGBlock* PredBlock,
const CFGBlock *CurrBlock);
+ bool join(const FactEntry , const FactEntry );
+
void intersectAndWarn(FactSet , const FactSet ,
SourceLocation JoinLoc, LockErrorKind LEK1,
LockErrorKind LEK2);
@@ -2186,6 +2188,21 @@
}
}
+/// Given two facts merging on a join point, decide whether to warn and which
+/// one to keep.
+///
+/// \return false if we should keep \p A, true if we should keep \p B.
+bool ThreadSafetyAnalyzer::join(const FactEntry , const FactEntry ) {
+ if (A.kind() != B.kind()) {
+Handler.handleExclusiveAndShared("mutex", B.toString(), B.loc(), A.loc());
+// Take the exclusive capability to reduce further warnings.
+return B.kind() == LK_Exclusive;
+ } else {
+// The non-asserted capability is the one we want to track.
+return A.asserted() && !B.asserted();
+ }
+}
+
/// Compute the intersection of two locksets and issue warnings for any
/// locks in the symmetric difference.
///
@@ -2213,20 +2230,8 @@
FactSet::iterator Iter1 = FSet1.findLockIter(FactMan, LDat2);
if (Iter1 != FSet1.end()) {
- const FactEntry = FactMan[*Iter1];
- if (LDat1.kind() != LDat2.kind()) {
-Handler.handleExclusiveAndShared("mutex", LDat2.toString(),
LDat2.loc(),
- LDat1.loc());
-if (LEK1 == LEK_LockedSomePredecessors &&
-LDat1.kind() != LK_Exclusive) {
- // Take the exclusive lock, which is the one in FSet2.
- *Iter1 = Fact;
-}
- } else if (LEK1 == LEK_LockedSomePredecessors && LDat1.asserted() &&
- !LDat2.asserted()) {
-// The non-asserted lock in FSet2 is the one we want to track.
+ if (join(FactMan[*Iter1], LDat2) && LEK1 == LEK_LockedSomePredecessors)
*Iter1 = Fact;
- }
} else {
LDat2.handleRemovalFromIntersection(FSet2, FactMan, JoinLoc, LEK1,
Handler);
Index: clang/lib/Analysis/ThreadSafety.cpp
===
--- clang/lib/Analysis/ThreadSafety.cpp
+++ clang/lib/Analysis/ThreadSafety.cpp
@@ -1050,6 +1050,8 @@
const CFGBlock* PredBlock,
const CFGBlock *CurrBlock);
+ bool join(const FactEntry , const FactEntry );
+
void intersectAndWarn(FactSet , const FactSet ,
SourceLocation JoinLoc, LockErrorKind LEK1,
LockErrorKind LEK2);
@@ -2186,6 +2188,21 @@
}
}
+/// Given two facts merging on a join point, decide whether to warn and which
+/// one to keep.
+///
+/// \return false if we should keep \p A, true if we should keep \p B.
+bool ThreadSafetyAnalyzer::join(const FactEntry , const FactEntry ) {
+ if (A.kind() != B.kind()) {
+Handler.handleExclusiveAndShared("mutex", B.toString(), B.loc(), A.loc());
+// Take the exclusive capability to reduce further warnings.
+return B.kind() == LK_Exclusive;
+ } else {
+// The non-asserted capability is the one we want to track.
+return A.asserted() && !B.asserted();
+ }
+}
+
/// Compute the intersection of two locksets and issue warnings for any
/// locks in the symmetric difference.
///
@@ -2213,20 +2230,8 @@
FactSet::iterator Iter1 = FSet1.findLockIter(FactMan, LDat2);
if (Iter1 != FSet1.end()) {
- const FactEntry = FactMan[*Iter1];
- if (LDat1.kind() != LDat2.kind()) {
-Handler.handleExclusiveAndShared("mutex", LDat2.toString(), LDat2.loc(),
- LDat1.loc());
-if (LEK1 == LEK_LockedSomePredecessors &&
-LDat1.kind() != LK_Exclusive) {
- // Take the exclusive lock, which is the one in FSet2.
- *Iter1 = Fact;
-}
- } else if (LEK1 == LEK_LockedSomePredecessors && LDat1.asserted() &&
- !LDat2.asserted()) {
-// The non-asserted lock in FSet2 is the one we want to track.
+ if (join(FactMan[*Iter1], LDat2) && LEK1 == LEK_LockedSomePredecessors)
*Iter1 = Fact;
- }
} else {
LDat2.handleRemovalFromIntersection(FSet2, FactMan, JoinLoc, LEK1,
Handler);
___
cfe-commits mailing
