[clang] [-Wcompletion-handler] Fix a non-termination issue (PR #78380)
Ziqing Luo via cfe-commits
cfe-commits at lists.llvm.org
Thu Jan 25 14:55:38 PST 2024
https://github.com/ziqingluo-90 updated https://github.com/llvm/llvm-project/pull/78380
>From e7c3a3fc2a4f5d5714044a1c407bfe56f328680d Mon Sep 17 00:00:00 2001
From: Ziqing Luo <ziqing at udel.edu>
Date: Tue, 16 Jan 2024 17:45:01 -0800
Subject: [PATCH 1/4] [-Wcompletion-handler] Fix a non-termination issue
The Called-Once dataflow analysis could never terminate as a
consequence of non-monotonic update on states. States of kind Escape
can override states leading to non-monotonic update.
The fix uses finite state sets instead of single states to represent
CFG block outputs, which grows in uni-direction. To preserve the
behavior of the analyzer, a single state can be extracted from a state
set. The extraction follows the idea that Escape can override error
states within one block but obeys to the partial-order for join.
rdar://119671856
---
clang/lib/Analysis/CalledOnceCheck.cpp | 120 ++++++++++++++++++++++---
clang/test/SemaObjC/warn-called-once.m | 10 +++
2 files changed, 120 insertions(+), 10 deletions(-)
diff --git a/clang/lib/Analysis/CalledOnceCheck.cpp b/clang/lib/Analysis/CalledOnceCheck.cpp
index 04c5f6aa9c7450..3fab93b1d09cdf 100644
--- a/clang/lib/Analysis/CalledOnceCheck.cpp
+++ b/clang/lib/Analysis/CalledOnceCheck.cpp
@@ -163,7 +163,7 @@ class ParameterStatus {
NotVisited = 0x8, /* 1000 */
// We already reported a violation and stopped tracking calls for this
// parameter.
- Reported = 0x15, /* 1111 */
+ Reported = 0xF, /* 1111 */
LLVM_MARK_AS_BITMASK_ENUM(/* LargestValue = */ Reported)
};
@@ -215,6 +215,55 @@ class ParameterStatus {
const Expr *Call = nullptr;
};
+// Represents a set of `ParameterStatus`s collected in a single CFGBlock.
+class ParamStatusSet {
+private:
+ // The kinds of `States` spans from 0x0 to 0x15, so 16 bits are enough:
+ llvm::BitVector Set{16, false};
+ // Following the exisitng idea: if there are multiple calls of interest in one
+ // block, recording one of them is good enough.
+ const Expr *Call = nullptr;
+
+public:
+ ParamStatusSet() {}
+
+ // Add a new `ParameterStatus` to the set. Returns true iff the adding status
+ // was new to the set.
+ bool add(ParameterStatus PS) {
+ assert(PS.getKind() != ParameterStatus::Kind::NotVisited &&
+ "the status cannot be NotVisited after visiting a block");
+ if (Set.test(PS.getKind()))
+ return false;
+ Set.set(PS.getKind());
+ if (PS.seenAnyCalls())
+ Call = &PS.getCall();
+ return true;
+ }
+
+ // Get one `ParameterStatus` to represent the set of them. The idea is to
+ // take a join on them but let ESCAPE overrides error statuses.
+ ParameterStatus summaryStatus() {
+ unsigned Summary = 0x0;
+
+ for (unsigned Idx :
+ llvm::make_range(Set.set_bits_begin(), Set.set_bits_end()))
+ Summary |= Idx;
+ assert(Summary == 0x0 || Summary == 0x1 || Summary == 0x3 ||
+ Summary == 0x5 || Summary == 0x7 ||
+ Summary == 0xF && "expecting a defined value");
+
+ ParameterStatus::Kind SummaryKind = ParameterStatus::Kind(Summary);
+
+ if (SummaryKind > ParameterStatus::Kind::NON_ERROR_STATUS &&
+ Set.test(ParameterStatus::Kind::Escaped)) {
+ SummaryKind = ParameterStatus::Kind::Escaped;
+ }
+ if (ParameterStatus::seenAnyCalls(SummaryKind))
+ return {SummaryKind, Call};
+ return {SummaryKind};
+ }
+};
+
/// State aggregates statuses of all tracked parameters.
class State {
public:
@@ -274,6 +323,53 @@ class State {
private:
ParamSizedVector<ParameterStatus> ParamData;
+
+ friend class CFGBlockOutputState;
+
+ State(ParamSizedVector<ParameterStatus> ParamData) : ParamData(ParamData) {}
+
+ unsigned size() const { return ParamData.size(); }
+};
+
+// A different kind of "state" in addition to `State`. `CFGBlockOutputState`
+// are created for dealing with the non-termination issue due to `State`s are
+// not being updated monotonically at the output of each CFGBlock.
+// A `CFGBlockOutputState` is in fact a finite set of `State`s that
+// grows monotonically. One can extract a `State` from a `CFGBlockOutputState`.
+// Note that the `State`-extraction does NOT guarantee monotone but it
+// respects the existing semantics. Specifically, ESCAPE is "greater than"
+// other error states in a single path but is "less than" them at JOIN.
+//
+// `CFGBlockOutputState` will be used to terminate the fix-point computation.
+class CFGBlockOutputState {
+private:
+ ParamSizedVector<ParamStatusSet> StatusSets;
+
+public:
+ CFGBlockOutputState(unsigned Size) : StatusSets{Size} {};
+
+ // Update this `CFGBlockOutputState` with a newly computed `State`. Return
+ // true iff `CFGBlockOutputState` changed.
+ bool update(const State &NewState) {
+ bool Changed = false;
+
+ for (unsigned Idx = 0; Idx < NewState.size(); ++Idx) {
+ if (StatusSets[Idx].add(NewState.getStatusFor(Idx))) {
+ Changed = true;
+ }
+ }
+ return Changed;
+ }
+
+ // Return a `State` that represents the `CFGBlockOutputState`.
+ State getState() {
+ ParamSizedVector<ParameterStatus> ParamData{StatusSets.size()};
+
+ for (unsigned Idx = 0; Idx < ParamData.size(); ++Idx) {
+ ParamData[Idx] = StatusSets[Idx].summaryStatus();
+ }
+ return State{ParamData};
+ }
};
/// A simple class that finds DeclRefExpr in the given expression.
@@ -639,6 +735,8 @@ class CalledOnceChecker : public ConstStmtVisitor<CalledOnceChecker> {
if (size() != 0) {
States =
CFGSizedVector<State>(FunctionCFG.getNumBlockIDs(), State(size()));
+ CFGBlockOutputStates = CFGSizedVector<CFGBlockOutputState>(
+ FunctionCFG.getNumBlockIDs(), CFGBlockOutputState(size()));
}
}
@@ -1305,17 +1403,17 @@ class CalledOnceChecker : public ConstStmtVisitor<CalledOnceChecker> {
}
/// \}
- /// Assign status to the given basic block.
- ///
- /// Returns true when the stored status changed.
+ /// Update output state for the CFGBlock.
+ /// Returns true when the stored state changed.
bool assignState(const CFGBlock *BB, const State &ToAssign) {
- State &Current = getState(BB);
- if (Current == ToAssign) {
- return false;
- }
+ CFGBlockOutputState &OldOutputState =
+ CFGBlockOutputStates[BB->getBlockID()];
- Current = ToAssign;
- return true;
+ if (OldOutputState.update(ToAssign)) {
+ getState(BB) = OldOutputState.getState();
+ return true;
+ }
+ return false;
}
/// Join all incoming statuses for the given basic block.
@@ -1692,6 +1790,8 @@ class CalledOnceChecker : public ConstStmtVisitor<CalledOnceChecker> {
State CurrentState;
ParamSizedVector<const ParmVarDecl *> TrackedParams;
CFGSizedVector<State> States;
+ // The mapping from each `CFGBlock` to its `CFGBlockOutputState`:
+ CFGSizedVector<CFGBlockOutputState> CFGBlockOutputStates;
};
} // end anonymous namespace
diff --git a/clang/test/SemaObjC/warn-called-once.m b/clang/test/SemaObjC/warn-called-once.m
index dbe8dc1cf1ae17..f23e41e00ee298 100644
--- a/clang/test/SemaObjC/warn-called-once.m
+++ b/clang/test/SemaObjC/warn-called-once.m
@@ -1194,6 +1194,16 @@ - (void)test_escape_after_branch:(int)cond
escape(handler);
}
+- (void)test_termination:(int)cond
+ withCompletion:(void (^)(void))handler {
+ // The code below was able to cause non-termination but should be
+ // fixed now:
+ do {
+ escape(handler);
+ handler(); // expected-warning{{completion handler is called twice}} expected-note{{previous call is here; set to nil to indicate it cannot be called afterwards}}
+ } while (cond);
+}
+
typedef void (^DeferredBlock)(void);
static inline void DefferedCallback(DeferredBlock *inBlock) { (*inBlock)(); }
#define _DEFERCONCAT(a, b) a##b
>From 90852bb41bfbfc2d31bc60eae55635b32cb9faa8 Mon Sep 17 00:00:00 2001
From: Ziqing Luo <ziqing at udel.edu>
Date: Thu, 25 Jan 2024 14:39:04 -0800
Subject: [PATCH 2/4] Revert "[-Wcompletion-handler] Fix a non-termination
issue"
This reverts commit e7c3a3fc2a4f5d5714044a1c407bfe56f328680d.
---
clang/lib/Analysis/CalledOnceCheck.cpp | 120 +++----------------------
clang/test/SemaObjC/warn-called-once.m | 10 ---
2 files changed, 10 insertions(+), 120 deletions(-)
diff --git a/clang/lib/Analysis/CalledOnceCheck.cpp b/clang/lib/Analysis/CalledOnceCheck.cpp
index 3fab93b1d09cdf..04c5f6aa9c7450 100644
--- a/clang/lib/Analysis/CalledOnceCheck.cpp
+++ b/clang/lib/Analysis/CalledOnceCheck.cpp
@@ -163,7 +163,7 @@ class ParameterStatus {
NotVisited = 0x8, /* 1000 */
// We already reported a violation and stopped tracking calls for this
// parameter.
- Reported = 0xF, /* 1111 */
+ Reported = 0x15, /* 1111 */
LLVM_MARK_AS_BITMASK_ENUM(/* LargestValue = */ Reported)
};
@@ -215,55 +215,6 @@ class ParameterStatus {
const Expr *Call = nullptr;
};
-// Represents a set of `ParameterStatus`s collected in a single CFGBlock.
-class ParamStatusSet {
-private:
- // The kinds of `States` spans from 0x0 to 0x15, so 16 bits are enough:
- llvm::BitVector Set{16, false};
- // Following the exisitng idea: if there are multiple calls of interest in one
- // block, recording one of them is good enough.
- const Expr *Call = nullptr;
-
-public:
- ParamStatusSet() {}
-
- // Add a new `ParameterStatus` to the set. Returns true iff the adding status
- // was new to the set.
- bool add(ParameterStatus PS) {
- assert(PS.getKind() != ParameterStatus::Kind::NotVisited &&
- "the status cannot be NotVisited after visiting a block");
- if (Set.test(PS.getKind()))
- return false;
- Set.set(PS.getKind());
- if (PS.seenAnyCalls())
- Call = &PS.getCall();
- return true;
- }
-
- // Get one `ParameterStatus` to represent the set of them. The idea is to
- // take a join on them but let ESCAPE overrides error statuses.
- ParameterStatus summaryStatus() {
- unsigned Summary = 0x0;
-
- for (unsigned Idx :
- llvm::make_range(Set.set_bits_begin(), Set.set_bits_end()))
- Summary |= Idx;
- assert(Summary == 0x0 || Summary == 0x1 || Summary == 0x3 ||
- Summary == 0x5 || Summary == 0x7 ||
- Summary == 0xF && "expecting a defined value");
-
- ParameterStatus::Kind SummaryKind = ParameterStatus::Kind(Summary);
-
- if (SummaryKind > ParameterStatus::Kind::NON_ERROR_STATUS &&
- Set.test(ParameterStatus::Kind::Escaped)) {
- SummaryKind = ParameterStatus::Kind::Escaped;
- }
- if (ParameterStatus::seenAnyCalls(SummaryKind))
- return {SummaryKind, Call};
- return {SummaryKind};
- }
-};
-
/// State aggregates statuses of all tracked parameters.
class State {
public:
@@ -323,53 +274,6 @@ class State {
private:
ParamSizedVector<ParameterStatus> ParamData;
-
- friend class CFGBlockOutputState;
-
- State(ParamSizedVector<ParameterStatus> ParamData) : ParamData(ParamData) {}
-
- unsigned size() const { return ParamData.size(); }
-};
-
-// A different kind of "state" in addition to `State`. `CFGBlockOutputState`
-// are created for dealing with the non-termination issue due to `State`s are
-// not being updated monotonically at the output of each CFGBlock.
-// A `CFGBlockOutputState` is in fact a finite set of `State`s that
-// grows monotonically. One can extract a `State` from a `CFGBlockOutputState`.
-// Note that the `State`-extraction does NOT guarantee monotone but it
-// respects the existing semantics. Specifically, ESCAPE is "greater than"
-// other error states in a single path but is "less than" them at JOIN.
-//
-// `CFGBlockOutputState` will be used to terminate the fix-point computation.
-class CFGBlockOutputState {
-private:
- ParamSizedVector<ParamStatusSet> StatusSets;
-
-public:
- CFGBlockOutputState(unsigned Size) : StatusSets{Size} {};
-
- // Update this `CFGBlockOutputState` with a newly computed `State`. Return
- // true iff `CFGBlockOutputState` changed.
- bool update(const State &NewState) {
- bool Changed = false;
-
- for (unsigned Idx = 0; Idx < NewState.size(); ++Idx) {
- if (StatusSets[Idx].add(NewState.getStatusFor(Idx))) {
- Changed = true;
- }
- }
- return Changed;
- }
-
- // Return a `State` that represents the `CFGBlockOutputState`.
- State getState() {
- ParamSizedVector<ParameterStatus> ParamData{StatusSets.size()};
-
- for (unsigned Idx = 0; Idx < ParamData.size(); ++Idx) {
- ParamData[Idx] = StatusSets[Idx].summaryStatus();
- }
- return State{ParamData};
- }
};
/// A simple class that finds DeclRefExpr in the given expression.
@@ -735,8 +639,6 @@ class CalledOnceChecker : public ConstStmtVisitor<CalledOnceChecker> {
if (size() != 0) {
States =
CFGSizedVector<State>(FunctionCFG.getNumBlockIDs(), State(size()));
- CFGBlockOutputStates = CFGSizedVector<CFGBlockOutputState>(
- FunctionCFG.getNumBlockIDs(), CFGBlockOutputState(size()));
}
}
@@ -1403,17 +1305,17 @@ class CalledOnceChecker : public ConstStmtVisitor<CalledOnceChecker> {
}
/// \}
- /// Update output state for the CFGBlock.
- /// Returns true when the stored state changed.
+ /// Assign status to the given basic block.
+ ///
+ /// Returns true when the stored status changed.
bool assignState(const CFGBlock *BB, const State &ToAssign) {
- CFGBlockOutputState &OldOutputState =
- CFGBlockOutputStates[BB->getBlockID()];
-
- if (OldOutputState.update(ToAssign)) {
- getState(BB) = OldOutputState.getState();
- return true;
+ State &Current = getState(BB);
+ if (Current == ToAssign) {
+ return false;
}
- return false;
+
+ Current = ToAssign;
+ return true;
}
/// Join all incoming statuses for the given basic block.
@@ -1790,8 +1692,6 @@ class CalledOnceChecker : public ConstStmtVisitor<CalledOnceChecker> {
State CurrentState;
ParamSizedVector<const ParmVarDecl *> TrackedParams;
CFGSizedVector<State> States;
- // The mapping from each `CFGBlock` to its `CFGBlockOutputState`:
- CFGSizedVector<CFGBlockOutputState> CFGBlockOutputStates;
};
} // end anonymous namespace
diff --git a/clang/test/SemaObjC/warn-called-once.m b/clang/test/SemaObjC/warn-called-once.m
index f23e41e00ee298..dbe8dc1cf1ae17 100644
--- a/clang/test/SemaObjC/warn-called-once.m
+++ b/clang/test/SemaObjC/warn-called-once.m
@@ -1194,16 +1194,6 @@ - (void)test_escape_after_branch:(int)cond
escape(handler);
}
-- (void)test_termination:(int)cond
- withCompletion:(void (^)(void))handler {
- // The code below was able to cause non-termination but should be
- // fixed now:
- do {
- escape(handler);
- handler(); // expected-warning{{completion handler is called twice}} expected-note{{previous call is here; set to nil to indicate it cannot be called afterwards}}
- } while (cond);
-}
-
typedef void (^DeferredBlock)(void);
static inline void DefferedCallback(DeferredBlock *inBlock) { (*inBlock)(); }
#define _DEFERCONCAT(a, b) a##b
>From a4d18e63952e2b47dd97759bebbfd1095b346665 Mon Sep 17 00:00:00 2001
From: Ziqing Luo <ziqing at udel.edu>
Date: Thu, 25 Jan 2024 14:39:54 -0800
Subject: [PATCH 3/4] [-Wcompletion-handler] Fix a non-termination issue
The Called-Once dataflow analysis could never terminate as a
consequence of non-monotonic update on states. States of kind Escape
can override states leading to non-monotonic update.
This fix disallows the `Escape` state to override the `Reported`
state.
rdar://119671856
---
clang/lib/Analysis/CalledOnceCheck.cpp | 5 +++--
clang/test/SemaObjC/warn-called-once.m | 10 ++++++++++
2 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/clang/lib/Analysis/CalledOnceCheck.cpp b/clang/lib/Analysis/CalledOnceCheck.cpp
index 04c5f6aa9c7450..e4cb8d0f36a8df 100644
--- a/clang/lib/Analysis/CalledOnceCheck.cpp
+++ b/clang/lib/Analysis/CalledOnceCheck.cpp
@@ -163,7 +163,7 @@ class ParameterStatus {
NotVisited = 0x8, /* 1000 */
// We already reported a violation and stopped tracking calls for this
// parameter.
- Reported = 0x15, /* 1111 */
+ Reported = 0xF, /* 1111 */
LLVM_MARK_AS_BITMASK_ENUM(/* LargestValue = */ Reported)
};
@@ -932,7 +932,8 @@ class CalledOnceChecker : public ConstStmtVisitor<CalledOnceChecker> {
ParameterStatus &CurrentParamStatus = CurrentState.getStatusFor(Index);
// Escape overrides whatever error we think happened.
- if (CurrentParamStatus.isErrorStatus()) {
+ if (CurrentParamStatus.isErrorStatus() &&
+ CurrentParamStatus.getKind() != ParameterStatus::Kind::Reported) {
CurrentParamStatus = ParameterStatus::Escaped;
}
}
diff --git a/clang/test/SemaObjC/warn-called-once.m b/clang/test/SemaObjC/warn-called-once.m
index dbe8dc1cf1ae17..f23e41e00ee298 100644
--- a/clang/test/SemaObjC/warn-called-once.m
+++ b/clang/test/SemaObjC/warn-called-once.m
@@ -1194,6 +1194,16 @@ - (void)test_escape_after_branch:(int)cond
escape(handler);
}
+- (void)test_termination:(int)cond
+ withCompletion:(void (^)(void))handler {
+ // The code below was able to cause non-termination but should be
+ // fixed now:
+ do {
+ escape(handler);
+ handler(); // expected-warning{{completion handler is called twice}} expected-note{{previous call is here; set to nil to indicate it cannot be called afterwards}}
+ } while (cond);
+}
+
typedef void (^DeferredBlock)(void);
static inline void DefferedCallback(DeferredBlock *inBlock) { (*inBlock)(); }
#define _DEFERCONCAT(a, b) a##b
>From dba1123558b29a30acc510196f121cf2d0f1641b Mon Sep 17 00:00:00 2001
From: Ziqing Luo <ziqing at udel.edu>
Date: Thu, 25 Jan 2024 14:55:17 -0800
Subject: [PATCH 4/4] fix format
---
clang/lib/Analysis/CalledOnceCheck.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/clang/lib/Analysis/CalledOnceCheck.cpp b/clang/lib/Analysis/CalledOnceCheck.cpp
index e4cb8d0f36a8df..30cbd257b65e8f 100644
--- a/clang/lib/Analysis/CalledOnceCheck.cpp
+++ b/clang/lib/Analysis/CalledOnceCheck.cpp
@@ -932,7 +932,7 @@ class CalledOnceChecker : public ConstStmtVisitor<CalledOnceChecker> {
ParameterStatus &CurrentParamStatus = CurrentState.getStatusFor(Index);
// Escape overrides whatever error we think happened.
- if (CurrentParamStatus.isErrorStatus() &&
+ if (CurrentParamStatus.isErrorStatus() &&
CurrentParamStatus.getKind() != ParameterStatus::Kind::Reported) {
CurrentParamStatus = ParameterStatus::Escaped;
}
More information about the cfe-commits
mailing list