From: Eric Fiselier Date: Thu, 25 May 2017 02:16:53 +0000 (+0000) Subject: [coroutines] Fix fallthrough diagnostics for coroutines X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=28be04dd8e51399b1955d413777003f80e81136f;p=clang [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 git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@303831 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td index 2013036d57..60039fac6e 100644 --- a/include/clang/Basic/DiagnosticSemaKinds.td +++ b/include/clang/Basic/DiagnosticSemaKinds.td @@ -537,10 +537,10 @@ def err_maybe_falloff_nonvoid_block : Error< 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; 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; def warn_suggest_noreturn_function : Warning< "%select{function|method}0 %1 could be declared with attribute 'noreturn'">, diff --git a/include/clang/Sema/ScopeInfo.h b/include/clang/Sema/ScopeInfo.h index 4487c7c2cc..4251fa649a 100644 --- a/include/clang/Sema/ScopeInfo.h +++ b/include/clang/Sema/ScopeInfo.h @@ -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"); diff --git a/lib/Analysis/AnalysisDeclContext.cpp b/lib/Analysis/AnalysisDeclContext.cpp index 6b58916162..7c0f5543da 100644 --- a/lib/Analysis/AnalysisDeclContext.cpp +++ b/lib/Analysis/AnalysisDeclContext.cpp @@ -92,6 +92,8 @@ Stmt *AnalysisDeclContext::getBody(bool &IsAutosynthesized) const { IsAutosynthesized = false; if (const FunctionDecl *FD = dyn_cast(D)) { Stmt *Body = FD->getBody(); + if (auto *CoroBody = dyn_cast_or_null(Body)) + Body = CoroBody->getBody(); if (Manager && Manager->synthesizeBodies()) { Stmt *SynthesizedBody = getBodyFarm(getASTContext(), Manager->Injector.get()).getBody(FD); diff --git a/lib/Sema/AnalysisBasedWarnings.cpp b/lib/Sema/AnalysisBasedWarnings.cpp index 895d00b3eb..db688b12cb 100644 --- a/lib/Sema/AnalysisBasedWarnings.cpp +++ b/lib/Sema/AnalysisBasedWarnings.cpp @@ -334,10 +334,6 @@ static ControlFlowKind CheckFallThrough(AnalysisDeclContext &AC) { 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(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(AnalysisDeclContext &AC) { CFGStmt CS = ri->castAs(); const Stmt *S = CS.getStmt(); - if ((isa(S) && !IsCoroutine) || isa(S)) { + if (isa(S) || isa(S)) { HasLiveReturn = true; continue; } @@ -546,6 +542,7 @@ static void CheckFallThroughForBody(Sema &S, const Decl *D, const Stmt *Body, bool ReturnsVoid = false; bool HasNoReturn = false; + bool IsCoroutine = S.getCurFunction() && S.getCurFunction()->isCoroutine(); if (const auto *FD = dyn_cast(D)) { if (const auto *CBody = dyn_cast(Body)) @@ -574,8 +571,13 @@ static void CheckFallThroughForBody(Sema &S, const Decl *D, const Stmt *Body, // 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 &S, const Decl *D, const Stmt *Body, 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(sema::AnalysisBasedWarnings::Policy P, // Warning: check missing 'return' if (P.enableCheckFallThrough) { - auto IsCoro = [&]() { - if (auto *FD = dyn_cast(D)) - if (FD->getBody() && isa(FD->getBody())) - return true; - return false; - }; const CheckFallThroughDiagnostics &CD = (isa(D) ? CheckFallThroughDiagnostics::MakeForBlock() @@ -2044,7 +2040,7 @@ AnalysisBasedWarnings::IssueWarnings(sema::AnalysisBasedWarnings::Policy P, cast(D)->getOverloadedOperator() == OO_Call && cast(D)->getParent()->isLambda()) ? CheckFallThroughDiagnostics::MakeForLambda() - : (IsCoro() + : (fscope->isCoroutine() ? CheckFallThroughDiagnostics::MakeForCoroutine(D) : CheckFallThroughDiagnostics::MakeForFunction(D))); CheckFallThroughForBody(S, D, Body, blkExpr, CD, AC); diff --git a/lib/Sema/SemaCoroutine.cpp b/lib/Sema/SemaCoroutine.cpp index 8f7011c985..45b3a48d15 100644 --- a/lib/Sema/SemaCoroutine.cpp +++ b/lib/Sema/SemaCoroutine.cpp @@ -719,13 +719,16 @@ static FunctionDecl *findDeleteForPromise(Sema &S, SourceLocation Loc, 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(Body)) { // Nothing todo. the body is already a transformed coroutine body statement. diff --git a/lib/Sema/SemaDecl.cpp b/lib/Sema/SemaDecl.cpp index 27c23e4f11..fd33001f92 100644 --- a/lib/Sema/SemaDecl.cpp +++ b/lib/Sema/SemaDecl.cpp @@ -12179,7 +12179,7 @@ Decl *Sema::ActOnFinishFunctionBody(Decl *dcl, Stmt *Body, 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) { diff --git a/test/SemaCXX/coreturn.cpp b/test/SemaCXX/coreturn.cpp index bdad227ea4..8bdf03ec67 100644 --- a/test/SemaCXX/coreturn.cpp +++ b/test/SemaCXX/coreturn.cpp @@ -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 -struct std::experimental::coroutine_traits { using promise_type = promise_void; }; +template <> +struct std::experimental::coroutine_traits { using promise_type = promise_void; }; + +template +struct std::experimental::coroutine_traits { using promise_type = promise_void_return_value; }; template struct std::experimental::coroutine_traits { 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::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::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}} diff --git a/test/SemaCXX/coroutines.cpp b/test/SemaCXX/coroutines.cpp index a39ad1b401..4588586d00 100644 --- a/test/SemaCXX/coroutines.cpp +++ b/test/SemaCXX/coroutines.cpp @@ -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 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 no_return_value_or_return_void() { + co_await a; +}