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.
We were modifying precisely when intersecting the lock sets of multiple
predecessors without back edge. That's no coincidence: we can't modify
on back edges, it doesn't make sense to modify at the end of a function,
and otherwise we always want to intersect on forward edges, because we
can build a new lock set for those.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D101755
Files:
clang/lib/Analysis/ThreadSafety.cpp
Index: clang/lib/Analysis/ThreadSafety.cpp
===================================================================
--- clang/lib/Analysis/ThreadSafety.cpp
+++ clang/lib/Analysis/ThreadSafety.cpp
@@ -1051,14 +1051,12 @@
const CFGBlock *CurrBlock);
void intersectAndWarn(FactSet &FSet1, const FactSet &FSet2,
- SourceLocation JoinLoc,
- LockErrorKind LEK1, LockErrorKind LEK2,
- bool Modify=true);
+ SourceLocation JoinLoc, LockErrorKind LEK1,
+ LockErrorKind LEK2);
void intersectAndWarn(FactSet &FSet1, const FactSet &FSet2,
- SourceLocation JoinLoc, LockErrorKind LEK1,
- bool Modify=true) {
- intersectAndWarn(FSet1, FSet2, JoinLoc, LEK1, LEK1, Modify);
+ SourceLocation JoinLoc, LockErrorKind LEK1) {
+ intersectAndWarn(FSet1, FSet2, JoinLoc, LEK1, LEK1);
}
void runAnalysis(AnalysisDeclContext &AC);
@@ -2206,8 +2204,7 @@
const FactSet &FSet2,
SourceLocation JoinLoc,
LockErrorKind LEK1,
- LockErrorKind LEK2,
- bool Modify) {
+ LockErrorKind LEK2) {
FactSet FSet1Orig = FSet1;
// Find locks in FSet2 that conflict or are not in FSet1, and warn.
@@ -2220,11 +2217,13 @@
if (LDat1.kind() != LDat2.kind()) {
Handler.handleExclusiveAndShared("mutex", LDat2.toString(),
LDat2.loc(),
LDat1.loc());
- if (Modify && LDat1.kind() != LK_Exclusive) {
+ if (LEK1 == LEK_LockedSomePredecessors &&
+ LDat1.kind() != LK_Exclusive) {
// Take the exclusive lock, which is the one in FSet2.
*Iter1 = Fact;
}
- } else if (Modify && LDat1.asserted() && !LDat2.asserted()) {
+ } else if (LEK1 == LEK_LockedSomePredecessors && LDat1.asserted() &&
+ !LDat2.asserted()) {
// The non-asserted lock in FSet2 is the one we want to track.
*Iter1 = Fact;
}
@@ -2242,7 +2241,7 @@
if (!LDat2) {
LDat1->handleRemovalFromIntersection(FSet1Orig, FactMan, JoinLoc, LEK2,
Handler);
- if (Modify)
+ if (LEK2 == LEK_LockedSomePredecessors)
FSet1.removeLock(FactMan, *LDat1);
}
}
@@ -2469,8 +2468,7 @@
// Do not update EntrySet.
intersectAndWarn(
CurrBlockInfo->EntrySet, PrevLockset, PrevBlockInfo->ExitLoc,
- IsLoop ? LEK_LockedSomeLoopIterations : LEK_LockedSomePredecessors,
- !IsLoop);
+ IsLoop ? LEK_LockedSomeLoopIterations :
LEK_LockedSomePredecessors);
}
}
@@ -2518,10 +2516,8 @@
CFGBlock *FirstLoopBlock = *SI;
CFGBlockInfo *PreLoop = &BlockInfo[FirstLoopBlock->getBlockID()];
CFGBlockInfo *LoopEnd = &BlockInfo[CurrBlockID];
- intersectAndWarn(LoopEnd->ExitSet, PreLoop->EntrySet,
- PreLoop->EntryLoc,
- LEK_LockedSomeLoopIterations,
- false);
+ intersectAndWarn(LoopEnd->ExitSet, PreLoop->EntrySet, PreLoop->EntryLoc,
+ LEK_LockedSomeLoopIterations);
}
}
@@ -2549,11 +2545,8 @@
ExpectedExitSet.removeLock(FactMan, Lock);
// FIXME: Should we call this function for all blocks which exit the
function?
- intersectAndWarn(ExpectedExitSet, Final->ExitSet,
- Final->ExitLoc,
- LEK_LockedAtEndOfFunction,
- LEK_NotLockedAtEndOfFunction,
- false);
+ intersectAndWarn(ExpectedExitSet, Final->ExitSet, Final->ExitLoc,
+ LEK_LockedAtEndOfFunction, LEK_NotLockedAtEndOfFunction);
Handler.leaveFunction(CurrentFunction);
}
Index: clang/lib/Analysis/ThreadSafety.cpp
===================================================================
--- clang/lib/Analysis/ThreadSafety.cpp
+++ clang/lib/Analysis/ThreadSafety.cpp
@@ -1051,14 +1051,12 @@
const CFGBlock *CurrBlock);
void intersectAndWarn(FactSet &FSet1, const FactSet &FSet2,
- SourceLocation JoinLoc,
- LockErrorKind LEK1, LockErrorKind LEK2,
- bool Modify=true);
+ SourceLocation JoinLoc, LockErrorKind LEK1,
+ LockErrorKind LEK2);
void intersectAndWarn(FactSet &FSet1, const FactSet &FSet2,
- SourceLocation JoinLoc, LockErrorKind LEK1,
- bool Modify=true) {
- intersectAndWarn(FSet1, FSet2, JoinLoc, LEK1, LEK1, Modify);
+ SourceLocation JoinLoc, LockErrorKind LEK1) {
+ intersectAndWarn(FSet1, FSet2, JoinLoc, LEK1, LEK1);
}
void runAnalysis(AnalysisDeclContext &AC);
@@ -2206,8 +2204,7 @@
const FactSet &FSet2,
SourceLocation JoinLoc,
LockErrorKind LEK1,
- LockErrorKind LEK2,
- bool Modify) {
+ LockErrorKind LEK2) {
FactSet FSet1Orig = FSet1;
// Find locks in FSet2 that conflict or are not in FSet1, and warn.
@@ -2220,11 +2217,13 @@
if (LDat1.kind() != LDat2.kind()) {
Handler.handleExclusiveAndShared("mutex", LDat2.toString(), LDat2.loc(),
LDat1.loc());
- if (Modify && LDat1.kind() != LK_Exclusive) {
+ if (LEK1 == LEK_LockedSomePredecessors &&
+ LDat1.kind() != LK_Exclusive) {
// Take the exclusive lock, which is the one in FSet2.
*Iter1 = Fact;
}
- } else if (Modify && LDat1.asserted() && !LDat2.asserted()) {
+ } else if (LEK1 == LEK_LockedSomePredecessors && LDat1.asserted() &&
+ !LDat2.asserted()) {
// The non-asserted lock in FSet2 is the one we want to track.
*Iter1 = Fact;
}
@@ -2242,7 +2241,7 @@
if (!LDat2) {
LDat1->handleRemovalFromIntersection(FSet1Orig, FactMan, JoinLoc, LEK2,
Handler);
- if (Modify)
+ if (LEK2 == LEK_LockedSomePredecessors)
FSet1.removeLock(FactMan, *LDat1);
}
}
@@ -2469,8 +2468,7 @@
// Do not update EntrySet.
intersectAndWarn(
CurrBlockInfo->EntrySet, PrevLockset, PrevBlockInfo->ExitLoc,
- IsLoop ? LEK_LockedSomeLoopIterations : LEK_LockedSomePredecessors,
- !IsLoop);
+ IsLoop ? LEK_LockedSomeLoopIterations : LEK_LockedSomePredecessors);
}
}
@@ -2518,10 +2516,8 @@
CFGBlock *FirstLoopBlock = *SI;
CFGBlockInfo *PreLoop = &BlockInfo[FirstLoopBlock->getBlockID()];
CFGBlockInfo *LoopEnd = &BlockInfo[CurrBlockID];
- intersectAndWarn(LoopEnd->ExitSet, PreLoop->EntrySet,
- PreLoop->EntryLoc,
- LEK_LockedSomeLoopIterations,
- false);
+ intersectAndWarn(LoopEnd->ExitSet, PreLoop->EntrySet, PreLoop->EntryLoc,
+ LEK_LockedSomeLoopIterations);
}
}
@@ -2549,11 +2545,8 @@
ExpectedExitSet.removeLock(FactMan, Lock);
// FIXME: Should we call this function for all blocks which exit the function?
- intersectAndWarn(ExpectedExitSet, Final->ExitSet,
- Final->ExitLoc,
- LEK_LockedAtEndOfFunction,
- LEK_NotLockedAtEndOfFunction,
- false);
+ intersectAndWarn(ExpectedExitSet, Final->ExitSet, Final->ExitLoc,
+ LEK_LockedAtEndOfFunction, LEK_NotLockedAtEndOfFunction);
Handler.leaveFunction(CurrentFunction);
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
