EricWF created this revision.
This patch fixes a number of issues with the analysis warnings emitted when a
coroutine may reach the end of the function w/o returning.
- Fix bug where coroutines with `return_value` are incorrectly diagnosed as
missing `co_return`'s.
- Rework diagnostic message to no longer say "non-void coroutine", because that
implies the coroutine doesn't have a void return type, which it might. In this
case a non-void coroutine is one who's promise type does not contain
`return_void()`
https://reviews.llvm.org/D33532
Files:
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/Sema/ScopeInfo.h
lib/Analysis/AnalysisDeclContext.cpp
lib/Sema/AnalysisBasedWarnings.cpp
lib/Sema/SemaCoroutine.cpp
lib/Sema/SemaDecl.cpp
test/SemaCXX/coreturn.cpp
test/SemaCXX/coroutines.cpp
Index: test/SemaCXX/coroutines.cpp
===================================================================
--- test/SemaCXX/coroutines.cpp
+++ test/SemaCXX/coroutines.cpp
@@ -818,3 +818,14 @@
co_return; //expected-note {{function is a coroutine due to use of 'co_return' here}}
}
+struct bad_promise_no_return_func {
+ coro<bad_promise_no_return_func> get_return_object();
+ suspend_always initial_suspend();
+ suspend_always final_suspend();
+ void unhandled_exception();
+};
+// FIXME: Make this ill-formed because the promise type declares
+// neither return_value(...) or return_void().
+coro<bad_promise_no_return_func> no_return_value_or_return_void() {
+ co_await a;
+}
Index: test/SemaCXX/coreturn.cpp
===================================================================
--- test/SemaCXX/coreturn.cpp
+++ test/SemaCXX/coreturn.cpp
@@ -18,6 +18,43 @@
void unhandled_exception();
};
+struct promise_void_return_value {
+ void get_return_object();
+ suspend_always initial_suspend();
+ suspend_always final_suspend();
+ void unhandled_exception();
+ void return_value(int);
+};
+
+struct VoidTagNoReturn {
+ struct promise_type {
+ VoidTagNoReturn get_return_object();
+ suspend_always initial_suspend();
+ suspend_always final_suspend();
+ void unhandled_exception();
+ };
+};
+
+struct VoidTagReturnValue {
+ struct promise_type {
+ VoidTagReturnValue get_return_object();
+ suspend_always initial_suspend();
+ suspend_always final_suspend();
+ void unhandled_exception();
+ void return_value(int);
+ };
+};
+
+struct VoidTagReturnVoid {
+ struct promise_type {
+ VoidTagReturnVoid get_return_object();
+ suspend_always initial_suspend();
+ suspend_always final_suspend();
+ void unhandled_exception();
+ void return_void();
+ };
+};
+
struct promise_float {
float get_return_object();
suspend_always initial_suspend();
@@ -34,8 +71,11 @@
void unhandled_exception();
};
-template <typename... T>
-struct std::experimental::coroutine_traits<void, T...> { using promise_type = promise_void; };
+template <>
+struct std::experimental::coroutine_traits<void> { using promise_type = promise_void; };
+
+template <typename T1>
+struct std::experimental::coroutine_traits<void, T1> { using promise_type = promise_void_return_value; };
template <typename... T>
struct std::experimental::coroutine_traits<float, T...> { using promise_type = promise_float; };
@@ -48,10 +88,66 @@
int test2() {
co_await a;
-} // expected-warning {{control reaches end of non-void coroutine}}
+} // expected-warning {{control reaches end of coroutine; which is undefined behavior because the promise type 'std::experimental::coroutine_traits<int>::promise_type' (aka 'promise_int') does not declare 'return_void()'}}
+
+int test2a(bool b) {
+ if (b)
+ co_return 42;
+} // expected-warning {{control may reach end of coroutine; which is undefined behavior because the promise type 'std::experimental::coroutine_traits<int, bool>::promise_type' (aka 'promise_int') does not declare 'return_void()'}}
int test3() {
co_await a;
b:
goto b;
}
+
+int test4() {
+ co_return 42;
+}
+
+void test5(int) {
+ co_await a;
+} // expected-warning {{control reaches end of coroutine; which is undefined behavior because}}
+
+void test6(int x) {
+ if (x)
+ co_return 42;
+} // expected-warning {{control may reach end of coroutine; which is undefined behavior because}}
+
+void test7(int y) {
+ if (y)
+ co_return 42;
+ else
+ co_return 101;
+}
+
+VoidTagReturnVoid test8() {
+ co_await a;
+}
+
+VoidTagReturnVoid test9(bool b) {
+ if (b)
+ co_return;
+}
+
+VoidTagReturnValue test10() {
+ co_await a;
+} // expected-warning {{control reaches end of coroutine}}
+
+VoidTagReturnValue test11(bool b) {
+ if (b)
+ co_return 42;
+} // expected-warning {{control may reach end of coroutine}}
+
+// FIXME: Promise types that declare neither return_value nor return_void
+// should be ill-formed. This test should be removed when that is fixed.
+VoidTagNoReturn test12() {
+ co_await a;
+} // expected-warning {{control reaches end of coroutine}}
+
+// FIXME: Promise types that declare neither return_value nor return_void
+// should be ill-formed. This test should be removed when that is fixed.
+VoidTagNoReturn test13(bool b) {
+ if (b)
+ co_await a;
+} // expected-warning {{control reaches end of coroutine}}
Index: lib/Sema/SemaDecl.cpp
===================================================================
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -12179,7 +12179,7 @@
sema::AnalysisBasedWarnings::Policy WP = AnalysisWarnings.getDefaultPolicy();
sema::AnalysisBasedWarnings::Policy *ActivePolicy = nullptr;
- if (getLangOpts().CoroutinesTS && getCurFunction()->CoroutinePromise)
+ if (getLangOpts().CoroutinesTS && getCurFunction()->isCoroutine())
CheckCompletedCoroutineBody(FD, Body);
if (FD) {
Index: lib/Sema/SemaCoroutine.cpp
===================================================================
--- lib/Sema/SemaCoroutine.cpp
+++ lib/Sema/SemaCoroutine.cpp
@@ -719,13 +719,16 @@
void Sema::CheckCompletedCoroutineBody(FunctionDecl *FD, Stmt *&Body) {
FunctionScopeInfo *Fn = getCurFunction();
- assert(Fn && Fn->CoroutinePromise && "not a coroutine");
-
+ assert(Fn && Fn->isCoroutine() && "not a coroutine");
if (!Body) {
assert(FD->isInvalidDecl() &&
"a null body is only allowed for invalid declarations");
return;
}
+ // We have a function that uses coroutine keywords, but we failed to build
+ // the promise type.
+ if (!Fn->CoroutinePromise)
+ return FD->setInvalidDecl();
if (isa<CoroutineBodyStmt>(Body)) {
// Nothing todo. the body is already a transformed coroutine body statement.
Index: lib/Sema/AnalysisBasedWarnings.cpp
===================================================================
--- lib/Sema/AnalysisBasedWarnings.cpp
+++ lib/Sema/AnalysisBasedWarnings.cpp
@@ -334,10 +334,6 @@
bool HasPlainEdge = false;
bool HasAbnormalEdge = false;
- // In a coroutine, only co_return statements count as normal returns. Remember
- // if we are processing a coroutine or not.
- const bool IsCoroutine = isa<CoroutineBodyStmt>(AC.getBody());
-
// Ignore default cases that aren't likely to be reachable because all
// enums in a switch(X) have explicit case statements.
CFGBlock::FilterOptions FO;
@@ -379,7 +375,7 @@
CFGStmt CS = ri->castAs<CFGStmt>();
const Stmt *S = CS.getStmt();
- if ((isa<ReturnStmt>(S) && !IsCoroutine) || isa<CoreturnStmt>(S)) {
+ if (isa<ReturnStmt>(S) || isa<CoreturnStmt>(S)) {
HasLiveReturn = true;
continue;
}
@@ -546,6 +542,7 @@
bool ReturnsVoid = false;
bool HasNoReturn = false;
+ bool IsCoroutine = S.getCurFunction() && S.getCurFunction()->isCoroutine();
if (const auto *FD = dyn_cast<FunctionDecl>(D)) {
if (const auto *CBody = dyn_cast<CoroutineBodyStmt>(Body))
@@ -574,24 +571,29 @@
// Short circuit for compilation speed.
if (CD.checkDiagnostics(Diags, ReturnsVoid, HasNoReturn))
return;
-
SourceLocation LBrace = Body->getLocStart(), RBrace = Body->getLocEnd();
+ auto EmitDiag = [&](SourceLocation Loc, unsigned DiagID) {
+ if (IsCoroutine)
+ S.Diag(Loc, DiagID) << S.getCurFunction()->CoroutinePromise->getType();
+ else
+ S.Diag(Loc, DiagID);
+ };
// Either in a function body compound statement, or a function-try-block.
switch (CheckFallThrough(AC)) {
case UnknownFallThrough:
break;
case MaybeFallThrough:
if (HasNoReturn)
- S.Diag(RBrace, CD.diag_MaybeFallThrough_HasNoReturn);
+ EmitDiag(RBrace, CD.diag_MaybeFallThrough_HasNoReturn);
else if (!ReturnsVoid)
- S.Diag(RBrace, CD.diag_MaybeFallThrough_ReturnsNonVoid);
+ EmitDiag(RBrace, CD.diag_MaybeFallThrough_ReturnsNonVoid);
break;
case AlwaysFallThrough:
if (HasNoReturn)
- S.Diag(RBrace, CD.diag_AlwaysFallThrough_HasNoReturn);
+ EmitDiag(RBrace, CD.diag_AlwaysFallThrough_HasNoReturn);
else if (!ReturnsVoid)
- S.Diag(RBrace, CD.diag_AlwaysFallThrough_ReturnsNonVoid);
+ EmitDiag(RBrace, CD.diag_AlwaysFallThrough_ReturnsNonVoid);
break;
case NeverFallThroughOrReturn:
if (ReturnsVoid && !HasNoReturn && CD.diag_NeverFallThroughOrReturn) {
@@ -2031,20 +2033,14 @@
// Warning: check missing 'return'
if (P.enableCheckFallThrough) {
- auto IsCoro = [&]() {
- if (auto *FD = dyn_cast<FunctionDecl>(D))
- if (FD->getBody() && isa<CoroutineBodyStmt>(FD->getBody()))
- return true;
- return false;
- };
const CheckFallThroughDiagnostics &CD =
(isa<BlockDecl>(D)
? CheckFallThroughDiagnostics::MakeForBlock()
: (isa<CXXMethodDecl>(D) &&
cast<CXXMethodDecl>(D)->getOverloadedOperator() == OO_Call &&
cast<CXXMethodDecl>(D)->getParent()->isLambda())
? CheckFallThroughDiagnostics::MakeForLambda()
- : (IsCoro()
+ : (fscope->isCoroutine()
? CheckFallThroughDiagnostics::MakeForCoroutine(D)
: CheckFallThroughDiagnostics::MakeForFunction(D)));
CheckFallThroughForBody(S, D, Body, blkExpr, CD, AC);
Index: lib/Analysis/AnalysisDeclContext.cpp
===================================================================
--- lib/Analysis/AnalysisDeclContext.cpp
+++ lib/Analysis/AnalysisDeclContext.cpp
@@ -92,6 +92,8 @@
IsAutosynthesized = false;
if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(D)) {
Stmt *Body = FD->getBody();
+ if (auto *CoroBody = dyn_cast_or_null<CoroutineBodyStmt>(Body))
+ Body = CoroBody->getBody();
if (Manager && Manager->synthesizeBodies()) {
Stmt *SynthesizedBody =
getBodyFarm(getASTContext(), Manager->Injector.get()).getBody(FD);
Index: include/clang/Sema/ScopeInfo.h
===================================================================
--- include/clang/Sema/ScopeInfo.h
+++ include/clang/Sema/ScopeInfo.h
@@ -388,6 +388,8 @@
(HasBranchProtectedScope && HasBranchIntoScope));
}
+ bool isCoroutine() const { return !FirstCoroutineStmtLoc.isInvalid(); }
+
void setFirstCoroutineStmt(SourceLocation Loc, StringRef Keyword) {
assert(FirstCoroutineStmtLoc.isInvalid() &&
"first coroutine statement location already set");
Index: include/clang/Basic/DiagnosticSemaKinds.td
===================================================================
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -539,10 +539,10 @@
def err_falloff_nonvoid_block : Error<
"control reaches end of non-void block">;
def warn_maybe_falloff_nonvoid_coroutine : Warning<
- "control may reach end of non-void coroutine">,
+ "control may reach end of coroutine; which is undefined behavior because the promise type %0 does not declare 'return_void()'">,
InGroup<ReturnType>;
def warn_falloff_nonvoid_coroutine : Warning<
- "control reaches end of non-void coroutine">,
+ "control reaches end of coroutine; which is undefined behavior because the promise type %0 does not declare 'return_void()'">,
InGroup<ReturnType>;
def warn_suggest_noreturn_function : Warning<
"%select{function|method}0 %1 could be declared with attribute 'noreturn'">,
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
