[coroutine] Suppress unreachable-code warning on coroutine statements. #77454
Conversation
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
clang/lib/Analysis/ReachableCode.cpp
Outdated
| // We suppress the unreachable warning for cases where an unreachable code is | ||
| // a substmt of the coroutine statement, becase removing it will change the | ||
| // function semantic if this is the only coroutine statement of the coroutine. | ||
| static bool isInCoroutineStmt(const CFGBlock *Block, const Stmt* S) { |
There was a problem hiding this comment.
Some explanations to help understand the code:
Blockis an unreachable block in CFG;Sis the first statement of theBlock, which can be diagnosed as dead;
Given the code task test() { std::abort(); co_return 1; } (full CFG), the detected unreachable Block is B1, and S is a DeclRefExpr (substmt of the co_return stmt) which refers to the implicit __promise var decl.
[B1]
1: __promise
2: [B1.1].return_value
3: 1
4: [B1.2]([B1.3])
5: co_return [B1.3]; Preds (1): B2(Unreachable)
Succs (1): B0
There was a problem hiding this comment.
I do not think this works for more interesting cases when coroutine statements are before the abort.
Eg:
task test() {
co_await 1;
abort();
co_await 2; // expected-warning {{code will never be executed}}
}would fail. We would want a warning in this case.
co_await 2 has its own block which would be passed to this function (B1 in CFG)
More test ideas:
bool foo();
task test() {
if (foo()) {
std::abort();
co_await 1; // expected-warning {{code will never be executed}}
}
co_await 2;
}There was a problem hiding this comment.
I discussed with @sam-mccall as well, we explored these options:
- Perform a global analysis on the CFG to make all these cases work perfect;
- Treat all coroutine statements as invalid dead statements, we never diagnose on these statements;
- Treat the current behaivor as intended, as technically these code are never executed during the runtime;
For 1), performing such global analysis (capturing all coroutine stmts in CFG, and figuring out all side effect of removing them) is sophisticated, and this model is not easy for human to understand and follow (as removing a coroutine statement may affect the whole analysis result).
For 3), users can suppress these diagnostics via the diagnostic pragma (the newly-introduced [[suppress]] is promising, but doesn't work for clang diagnostic).
2) is a simple model and solution. The tradeoff is that we might have some false negatives on some cases (as listed above), but I think it is fine.
Please take a look on the new change, and let me know what you think.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-analysis @llvm/pr-subscribers-clang Author: Haojian Wu (hokein) ChangesThis fixes #69219. Consider an example: Clang emits a CFG-based unreachable warning on the The fix to suppress the warning for this particular case. Full diff: https://github.com/llvm/llvm-project/pull/77454.diff 2 Files Affected:
diff --git a/clang/lib/Analysis/ReachableCode.cpp b/clang/lib/Analysis/ReachableCode.cpp
index 1bf0d9aec8620e..d839d2f999609d 100644
--- a/clang/lib/Analysis/ReachableCode.cpp
+++ b/clang/lib/Analysis/ReachableCode.cpp
@@ -17,6 +17,7 @@
#include "clang/AST/ExprCXX.h"
#include "clang/AST/ExprObjC.h"
#include "clang/AST/ParentMap.h"
+#include "clang/AST/RecursiveASTVisitor.h"
#include "clang/AST/StmtCXX.h"
#include "clang/Analysis/AnalysisDeclContext.h"
#include "clang/Analysis/CFG.h"
@@ -60,6 +61,45 @@ static bool isTrivialDoWhile(const CFGBlock *B, const Stmt *S) {
return false;
}
+// Check if the block starts with a coroutine statement and see if the given
+// unreachable 'S' is the substmt of the coroutine statement.
+//
+// We suppress the unreachable warning for cases where an unreachable code is
+// a substmt of the coroutine statement, becase removing it will change the
+// function semantic if this is the only coroutine statement of the coroutine.
+static bool isInCoroutineStmt(const CFGBlock *Block, const Stmt* S) {
+ // The coroutine statement, co_return, co_await, or co_yield.
+ const Stmt* CoroStmt = nullptr;
+ // Find the first coroutine statement in the block.
+ for (CFGBlock::const_iterator I = Block->begin(), E = Block->end(); I != E;
+ ++I)
+ if (std::optional<CFGStmt> CS = I->getAs<CFGStmt>()) {
+ const Stmt *S = CS->getStmt();
+ if (llvm::isa<CoreturnStmt>(S) || llvm::isa<CoroutineSuspendExpr>(S)) {
+ CoroStmt = S ;
+ break;
+ }
+ }
+ if (!CoroStmt)
+ return false;
+
+ struct Checker : RecursiveASTVisitor<Checker> {
+ const Stmt *StmtToCheck;
+ bool CoroutineSubStmt = false;
+ Checker(const Stmt *S) : StmtToCheck(S) {}
+ bool VisitStmt(const Stmt *S) {
+ if (S == StmtToCheck)
+ CoroutineSubStmt = true;
+ return true;
+ }
+ // The 'S' stmt captured in the CFG can be implicit.
+ bool shouldVisitImplicitCode() const { return true; }
+ };
+ Checker checker(S);
+ checker.TraverseStmt(const_cast<Stmt *>(CoroStmt));
+ return checker.CoroutineSubStmt;
+}
+
static bool isBuiltinUnreachable(const Stmt *S) {
if (const auto *DRE = dyn_cast<DeclRefExpr>(S))
if (const auto *FDecl = dyn_cast<FunctionDecl>(DRE->getDecl()))
@@ -623,7 +663,7 @@ void DeadCodeScan::reportDeadCode(const CFGBlock *B,
if (isa<BreakStmt>(S)) {
UK = reachable_code::UK_Break;
} else if (isTrivialDoWhile(B, S) || isBuiltinUnreachable(S) ||
- isBuiltinAssumeFalse(B, S, C)) {
+ isBuiltinAssumeFalse(B, S, C) || isInCoroutineStmt(B, S)) {
return;
}
else if (isDeadReturn(B, S)) {
diff --git a/clang/test/SemaCXX/coroutine-unreachable-warning.cpp b/clang/test/SemaCXX/coroutine-unreachable-warning.cpp
new file mode 100644
index 00000000000000..6ac5c34262b7e0
--- /dev/null
+++ b/clang/test/SemaCXX/coroutine-unreachable-warning.cpp
@@ -0,0 +1,50 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -std=c++20 -fsyntax-only -verify -Wunreachable-code
+
+#include "Inputs/std-coroutine.h"
+
+extern void abort (void) __attribute__ ((__noreturn__));
+
+struct task {
+ struct promise_type {
+ std::suspend_always initial_suspend();
+ std::suspend_always final_suspend() noexcept;
+ void return_void();
+ std::suspend_always yield_value(int) { return {}; }
+ task get_return_object();
+ void unhandled_exception();
+ };
+};
+
+task test1() {
+ abort();
+ co_yield 1;
+}
+
+task test2() {
+ abort();
+ 1; // expected-warning {{code will never be executed}}
+ co_yield 1;
+}
+
+task test3() {
+ abort();
+ co_return;
+}
+
+task test4() {
+ abort();
+ 1; // expected-warning {{code will never be executed}}
+ co_return;
+}
+
+
+task test5() {
+ abort();
+ co_await std::suspend_never{};
+}
+
+task test6() {
+ abort();
+ 1; // expected-warning {{code will never be executed}}
+ co_await std::suspend_never{};
+}
|
clang/lib/Analysis/ReachableCode.cpp
Outdated
| // We suppress the unreachable warning for cases where an unreachable code is | ||
| // a substmt of the coroutine statement, becase removing it will change the | ||
| // function semantic if this is the only coroutine statement of the coroutine. | ||
| static bool isInCoroutineStmt(const CFGBlock *Block, const Stmt* S) { |
There was a problem hiding this comment.
nit: rename S to Unreachable to avoid confusion with other variables named S.
clang/lib/Analysis/ReachableCode.cpp
Outdated
| // We suppress the unreachable warning for cases where an unreachable code is | ||
| // a substmt of the coroutine statement, becase removing it will change the | ||
| // function semantic if this is the only coroutine statement of the coroutine. | ||
| static bool isInCoroutineStmt(const CFGBlock *Block, const Stmt* S) { |
There was a problem hiding this comment.
I do not think this works for more interesting cases when coroutine statements are before the abort.
Eg:
task test() {
co_await 1;
abort();
co_await 2; // expected-warning {{code will never be executed}}
}would fail. We would want a warning in this case.
co_await 2 has its own block which would be passed to this function (B1 in CFG)
More test ideas:
bool foo();
task test() {
if (foo()) {
std::abort();
co_await 1; // expected-warning {{code will never be executed}}
}
co_await 2;
}
hokein
force-pushed
the
coroutine-unreachable
branch
2 times, most recently
from
c8a1951
to
ee6fa40
Compare
|
|
||
| struct Awaiter { | ||
| bool await_ready(); | ||
| void await_suspend(auto); | ||
| int await_resume(); | ||
| }; | ||
| auto await_transform(const int& x) { return Awaiter{}; } |
There was a problem hiding this comment.
nit: (Just to reduce the boilerplate not related to the test itself) we do not need to define a new awaitable here. Maybe just:
std::suspend_always await_transform(int) { return {}; }
There was a problem hiding this comment.
We have a testcase int a = co_await 1; where the return type of await_resume needs to be int, std::suspend_always is not sufficient enough for it.
There was a problem hiding this comment.
Oh my bad. I thought this was targeting co_await some_integer;. Makes sense then.
hokein
force-pushed
the
coroutine-unreachable
branch
from
165b715
to
c878bbd
Compare
llvm#77454) This fixes llvm#69219. Consider an example: ``` CoTask my_coroutine() { std::abort(); co_return 1; // unreachable code warning. } ``` Clang emits a CFG-based unreachable warning on the `co_return` statement (precisely the `1` subexpr). If we remove this statement, the program semantic is changed (my_coroutine is not a coroutine anymore). This patch fixes this issue by never considering coroutine statements as dead statements.
This fixes #69219.
Consider an example:
Clang emits a CFG-based unreachable warning on the
co_returnstatement (precisely the1subexpr). If we remove this statement, the program semantic is changed (my_coroutine is not a coroutine anymore).This patch fixes this issue by never considering coroutine statements as dead statements.