Skip to content

[coroutine] Suppress unreachable-code warning on coroutine statements. #77454

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Feb 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 47 additions & 4 deletions clang/lib/Analysis/ReachableCode.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -453,26 +454,68 @@ bool DeadCodeScan::isDeadCodeRoot(const clang::CFGBlock *Block) {
return isDeadRoot;
}

static bool isValidDeadStmt(const Stmt *S) {
// Check if the given `DeadStmt` is a coroutine statement and is a substmt of
// the coroutine statement. `Block` is the CFGBlock containing the `DeadStmt`.
static bool isInCoroutineStmt(const Stmt *DeadStmt, const CFGBlock *Block) {
// The coroutine statement, co_return, co_await, or co_yield.
const Stmt *CoroStmt = nullptr;
// Find the first coroutine statement after the DeadStmt in the block.
bool AfterDeadStmt = false;
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 (S == DeadStmt)
AfterDeadStmt = true;
if (AfterDeadStmt &&
// For simplicity, we only check simple coroutine statements.
(llvm::isa<CoreturnStmt>(S) || llvm::isa<CoroutineSuspendExpr>(S))) {
CoroStmt = S;
break;
}
}
if (!CoroStmt)
return false;
struct Checker : RecursiveASTVisitor<Checker> {
const Stmt *DeadStmt;
bool CoroutineSubStmt = false;
Checker(const Stmt *S) : DeadStmt(S) {}
bool VisitStmt(const Stmt *S) {
if (S == DeadStmt)
CoroutineSubStmt = true;
return true;
}
// Statements captured in the CFG can be implicit.
bool shouldVisitImplicitCode() const { return true; }
};
Checker checker(DeadStmt);
checker.TraverseStmt(const_cast<Stmt *>(CoroStmt));
return checker.CoroutineSubStmt;
}

static bool isValidDeadStmt(const Stmt *S, const clang::CFGBlock *Block) {
if (S->getBeginLoc().isInvalid())
return false;
if (const BinaryOperator *BO = dyn_cast<BinaryOperator>(S))
return BO->getOpcode() != BO_Comma;
return true;
// Coroutine statements are never considered dead statements, because removing
// them may change the function semantic if it is the only coroutine statement
// of the coroutine.
return !isInCoroutineStmt(S, Block);
}

const Stmt *DeadCodeScan::findDeadCode(const clang::CFGBlock *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 (isValidDeadStmt(S))
if (isValidDeadStmt(S, Block))
return S;
}

CFGTerminator T = Block->getTerminator();
if (T.isStmtBranch()) {
const Stmt *S = T.getStmt();
if (S && isValidDeadStmt(S))
if (S && isValidDeadStmt(S, Block))
return S;
}

Expand Down
76 changes: 76 additions & 0 deletions clang/test/SemaCXX/coroutine-unreachable-warning.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
// 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();

struct Awaiter {
bool await_ready();
void await_suspend(auto);
int await_resume();
};
auto await_transform(const int& x) { return Awaiter{}; }
Comment on lines +15 to +21
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {}; }

Copy link
Collaborator Author

@hokein hokein Feb 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh my bad. I thought this was targeting co_await some_integer;. Makes sense then.

};
};

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 1;
}

task test6() {
abort();
1; // expected-warning {{code will never be executed}}
co_await 3;
}

task test7() {
// coroutine statements are not considered unreachable.
co_await 1;
abort();
co_await 2;
}

task test8() {
// coroutine statements are not considered unreachable.
abort();
co_return;
1 + 1; // expected-warning {{code will never be executed}}
}

task test9() {
abort();
// This warning is emitted on the declaration itself, rather the coroutine substmt.
int x = co_await 1; // expected-warning {{code will never be executed}}
}