r303831 - [coroutines] Fix fallthrough diagnostics for coroutines
Eric Fiselier via cfe-commits
cfe-commits at lists.llvm.org
Wed May 24 19:16:53 PDT 2017
Author: ericwf
Date: Wed May 24 21:16:53 2017
New Revision: 303831
URL: http://llvm.org/viewvc/llvm-project?rev=303831&view=rev
Log:
[coroutines] Fix fallthrough diagnostics for coroutines
Summary:
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()`
As a side-effect of this patch, coroutine bodies that contain an invalid coroutine promise objects are marked as invalid.
Reviewers: GorNishanov, rsmith, aaron.ballman, majnemer
Reviewed By: GorNishanov
Subscribers: cfe-commits
Differential Revision: https://reviews.llvm.org/D33532
Modified:
cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
cfe/trunk/include/clang/Sema/ScopeInfo.h
cfe/trunk/lib/Analysis/AnalysisDeclContext.cpp
cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
cfe/trunk/lib/Sema/SemaCoroutine.cpp
cfe/trunk/lib/Sema/SemaDecl.cpp
cfe/trunk/test/SemaCXX/coreturn.cpp
cfe/trunk/test/SemaCXX/coroutines.cpp
Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=303831&r1=303830&r2=303831&view=diff
==============================================================================
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Wed May 24 21:16:53 2017
@@ -537,10 +537,10 @@ def err_maybe_falloff_nonvoid_block : Er
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'">,
Modified: cfe/trunk/include/clang/Sema/ScopeInfo.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/ScopeInfo.h?rev=303831&r1=303830&r2=303831&view=diff
==============================================================================
--- cfe/trunk/include/clang/Sema/ScopeInfo.h (original)
+++ cfe/trunk/include/clang/Sema/ScopeInfo.h Wed May 24 21:16:53 2017
@@ -388,6 +388,8 @@ public:
(HasBranchProtectedScope && HasBranchIntoScope));
}
+ bool isCoroutine() const { return !FirstCoroutineStmtLoc.isInvalid(); }
+
void setFirstCoroutineStmt(SourceLocation Loc, StringRef Keyword) {
assert(FirstCoroutineStmtLoc.isInvalid() &&
"first coroutine statement location already set");
Modified: cfe/trunk/lib/Analysis/AnalysisDeclContext.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/AnalysisDeclContext.cpp?rev=303831&r1=303830&r2=303831&view=diff
==============================================================================
--- cfe/trunk/lib/Analysis/AnalysisDeclContext.cpp (original)
+++ cfe/trunk/lib/Analysis/AnalysisDeclContext.cpp Wed May 24 21:16:53 2017
@@ -92,6 +92,8 @@ Stmt *AnalysisDeclContext::getBody(bool
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);
Modified: cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp?rev=303831&r1=303830&r2=303831&view=diff
==============================================================================
--- cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp (original)
+++ cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp Wed May 24 21:16:53 2017
@@ -334,10 +334,6 @@ static ControlFlowKind CheckFallThrough(
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 @@ static ControlFlowKind CheckFallThrough(
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 @@ static void CheckFallThroughForBody(Sema
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,8 +571,13 @@ static void CheckFallThroughForBody(Sema
// 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:
@@ -583,15 +585,15 @@ static void CheckFallThroughForBody(Sema
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,12 +2033,6 @@ AnalysisBasedWarnings::IssueWarnings(sem
// 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()
@@ -2044,7 +2040,7 @@ AnalysisBasedWarnings::IssueWarnings(sem
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);
Modified: cfe/trunk/lib/Sema/SemaCoroutine.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaCoroutine.cpp?rev=303831&r1=303830&r2=303831&view=diff
==============================================================================
--- cfe/trunk/lib/Sema/SemaCoroutine.cpp (original)
+++ cfe/trunk/lib/Sema/SemaCoroutine.cpp Wed May 24 21:16:53 2017
@@ -719,13 +719,16 @@ static FunctionDecl *findDeleteForPromis
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.
Modified: cfe/trunk/lib/Sema/SemaDecl.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDecl.cpp?rev=303831&r1=303830&r2=303831&view=diff
==============================================================================
--- cfe/trunk/lib/Sema/SemaDecl.cpp (original)
+++ cfe/trunk/lib/Sema/SemaDecl.cpp Wed May 24 21:16:53 2017
@@ -12179,7 +12179,7 @@ Decl *Sema::ActOnFinishFunctionBody(Decl
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) {
Modified: cfe/trunk/test/SemaCXX/coreturn.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/coreturn.cpp?rev=303831&r1=303830&r2=303831&view=diff
==============================================================================
--- cfe/trunk/test/SemaCXX/coreturn.cpp (original)
+++ cfe/trunk/test/SemaCXX/coreturn.cpp Wed May 24 21:16:53 2017
@@ -18,6 +18,43 @@ struct promise_void {
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 @@ struct promise_int {
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 @@ float test1() { co_await a; }
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}}
Modified: cfe/trunk/test/SemaCXX/coroutines.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/coroutines.cpp?rev=303831&r1=303830&r2=303831&view=diff
==============================================================================
--- cfe/trunk/test/SemaCXX/coroutines.cpp (original)
+++ cfe/trunk/test/SemaCXX/coroutines.cpp Wed May 24 21:16:53 2017
@@ -818,3 +818,14 @@ extern "C" int f(mismatch_gro_type_tag4)
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;
+}
More information about the cfe-commits
mailing list